checkpatch problem

[David Howells]

Hi,

Checkpatch generates the following messages for inline asm strings:

WARNING: unnecessary whitespace before a quoted newline
#49: FILE: arch/m32r/include/asm/irqflags.h:31:
+ "ld24 %0, #0 ; Use 32-bit insn. \n\t"

however, inline asm is more readable if I can tabulate things, including the
newline markers:

asm volatile (
"ld24 %0, #0 ; Use 32-bit insn. \n\t"
"mvfc %1, psw ; No interrupt can be accepted here. \n\t"
"mvtc %0, psw \n\t"
"and3 %0, %1, #0xffbf \n\t"
"mvtc %0, psw \n\t"
: "=&r" (tmpreg0), "=&r" (tmpreg1)
:
: "cbit", "memory");

Can you please fix it, even if it's only to permit multiple TAB chars before
'\n'.

Whilst this check is probaly fine for strings to be displayed in some way, this
doesn't necessarily apply to inline asm strings. Furthermore, the extra white
space does not impact the resulting binary.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Andy Whitcroft]

Post by David Howells
Hi,
WARNING: unnecessary whitespace before a quoted newline
+ "ld24 %0, #0 ; Use 32-bit insn. \n\t"
however, inline asm is more readable if I can tabulate things, including the
asm volatile (
"ld24 %0, #0 ; Use 32-bit insn. \n\t"
"mvfc %1, psw ; No interrupt can be accepted here. \n\t"
"mvtc %0, psw \n\t"
"and3 %0, %1, #0xffbf \n\t"
"mvtc %0, psw \n\t"
: "=&r" (tmpreg0), "=&r" (tmpreg1)
: "cbit", "memory");
Can you please fix it, even if it's only to permit multiple TAB chars before
'\n'.
Whilst this check is probaly fine for strings to be displayed in some way, this
doesn't necessarily apply to inline asm strings. Furthermore, the extra white
space does not impact the resulting binary.

A tricky one to know how to detect it as different. Often we do not
have the asm markers to hint us to change style. This affects us often
as gcc abuses the meaning of almost every character and has different
spacing for them too.

Will think on it.

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Joe Perches]

Post by Andy Whitcroft

Post by David Howells
WARNING: unnecessary whitespace before a quoted newline
+ "ld24 %0, #0 ; Use 32-bit insn. \n\t"
however, inline asm is more readable if I can tabulate things, including the
asm volatile (
"ld24 %0, #0 ; Use 32-bit insn. \n\t"
"mvfc %1, psw ; No interrupt can be accepted here. \n\t"
"mvtc %0, psw \n\t"
"and3 %0, %1, #0xffbf \n\t"
"mvtc %0, psw \n\t"
: "=&r" (tmpreg0), "=&r" (tmpreg1)
: "cbit", "memory");
Can you please fix it, even if it's only to permit multiple TAB chars before
'\n'.

A tricky one to know how to detect it as different. Often we do not
have the asm markers to hint us to change style. This affects us often
as gcc abuses the meaning of almost every character and has different
spacing for them too.

Maybe restrict the test to $logFunctions that have whitespace
before newlines?

Something like below.

Caveat: it doesn't necessarily report on the proper line.

For instance:
printk(KERN_DEBUG "ABCDEF \n");
vs
printk(KERN_DEBUG "ABC"
"DEF \n");

The second example reports the whitespace on the first line,
not the second line.

scripts/checkpatch.pl | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2039acd..f2ae4d5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1419,11 +1419,6 @@ sub process {
WARN("line over 80 characters\n" . $herecurr);
}

-# check for spaces before a quoted newline
- if ($rawline =~ /^.*\".*\s\\n/) {
- WARN("unnecessary whitespace before a quoted newline\n" . $herecurr);
- }
-
# check for adding lines without a newline.
if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) {
WARN("adding a line without newline at end of file\n" . $herecurr);
@@ -2335,6 +2330,22 @@ sub process {
}
}

+# check for whitespace before newlines in logging functions
+
+ if ($line =~ /^.*$logFunctions/) {
+ my $ln = $linenr;
+ my $cnt = $realcnt;
+ my ($off, $dstat, $dcond, $rest);
+ ($dstat, $dcond, $ln, $cnt, $off) =
+ ctx_statement_block($linenr, $realcnt, 0);
+ for (my $n = 0; $n < $cnt; $n++) {
+ my $l = $rawlines[$ln-1+$n];
+ if ($l =~ /\".*[ \t]\\n/) {
+ WARN("Logging function has unnecessary whitespace before a newline\n" . $herecurr);
+ }
+ }
+ }
+
# multi-statement macros should be enclosed in a do while loop, grab the
# first statement and ensure its the whole macro if its not enclosed
# in a known good container


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[David Howells]

Post by Joe Perches
Maybe restrict the test to $logFunctions that have whitespace
before newlines?
Something like below.
Caveat: it doesn't necessarily report on the proper line.
printk(KERN_DEBUG "ABCDEF \n");
vs
printk(KERN_DEBUG "ABC"
"DEF \n");
The second example reports the whitespace on the first line,
not the second line.
scripts/checkpatch.pl | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)

Works for me.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[David Howells]

Post by David Howells
Works for me.

Having said that, I now see:

WARNING: Logging function has unnecessary whitespace before a newline
#967: FILE: arch/mn10300/kernel/smp.c:56:
+#define Dprintk(fmt, ...) printk(fmt, ##__VA_ARGS__)

WARNING: Logging function has unnecessary whitespace before a newline
#967: FILE: arch/mn10300/kernel/smp.c:56:
+#define Dprintk(fmt, ...) printk(fmt, ##__VA_ARGS__)

WARNING: Logging function has unnecessary whitespace before a newline
#967: FILE: arch/mn10300/kernel/smp.c:56:
+#define Dprintk(fmt, ...) printk(fmt, ##__VA_ARGS__)

WARNING: Logging function has unnecessary whitespace before a newline
#967: FILE: arch/mn10300/kernel/smp.c:56:
+#define Dprintk(fmt, ...) printk(fmt, ##__VA_ARGS__)

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[David Howells]

The problem is that the second matcher (looking for " to \\n) matches on more
than just the logfunction line. Instrumenting the code supplied by your patch
thusly:

if ($line =~ /^.*($logFunctions)/) {
print "QQQQ $1 QQQQ\n";
my $ln = $linenr;
my $cnt = $realcnt;
my ($off, $dstat, $dcond, $rest);
($dstat, $dcond, $ln, $cnt, $off) =
ctx_statement_block($linenr, $realcnt, 0);
for (my $n = 0; $n < $cnt; $n++) {
my $l = $rawlines[$ln-1+$n];
if ($l =~ /(\".*[ \t]\\n)/) {
print "&&&$line&&&\n";
print "^^^^^ Matched '$1' ^^^^^\n";
WARN("Logging function has unnecessary whitespace before a newline\n" . $herecurr);
}
}
}

I see:

QQQQ printk QQQQ
QQQQ printk QQQQ
QQQQ printk QQQQ
QQQQ printk QQQQ
QQQQ printk QQQQ
&&&+#define Dprintk(fmt, ...) printk(KERN_DEBUG fmt, ##__VA_ARGS__)&&&
^^^^^ Matched '"mov %0,sp \n' ^^^^^
&&&+#define Dprintk(fmt, ...) printk(KERN_DEBUG fmt, ##__VA_ARGS__)&&&
^^^^^ Matched '"jmp (%1) \n' ^^^^^
&&&+#define Dprintk(fmt, ...) printk(KERN_DEBUG fmt, ##__VA_ARGS__)&&&
^^^^^ Matched '" movhu (%1),%0 \n' ^^^^^


It seems that it doesn't like a #define dealing with a logging function
followed by an inline asm statement that has whitespace before '\\n'.

I suspect checkpatch doesn't handle #defines correctly, and goes beyond their
end looking for a semicolon.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Joe Perches]

Post by David Howells
The problem is that the second matcher (looking for " to \\n) matches on more
than just the logfunction line. Instrumenting the code supplied by your patch

[]

Post by David Howells
I suspect checkpatch doesn't handle #defines correctly, and goes beyond their
end looking for a semicolon.

Hi David.

Exactly right. Andy has a version in his testing directory
that fixes this #define run-on block and speeds up checkpatch
runtime rather a lot for certain files like .h files that have
nothing but #defines.

Try applying my patch to this newer version:

http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[David Howells]

Post by Joe Perches
Exactly right. Andy has a version in his testing directory
that fixes this #define run-on block and speeds up checkpatch
runtime rather a lot for certain files like .h files that have
nothing but #defines.
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

It still doesn't work. In fact, I'm seeing more "Logging function has
unnecessary whitespace before a newline" warnings, such as on this:

printk(KERN_ERR
"BUG: CPU#%d started up but did not get a callout!\n",
cpu);

where I wasn't before.

I am still getting them on #defines:

-:6841: WARNING: Logging function has unnecessary whitespace before a newline
#6841: FILE: arch/mn10300/kernel/smp.c:57:
+#define Dprintk(fmt, ...) printk(KERN_DEBUG fmt, ##__VA_ARGS__)


Also, --emacs mode appears to be broken. It sticks something like "#7768: "
on the front of the filename encoded in the patch:

-:7768: WARNING: Logging function has unnecessary whitespace before a newline
#7768: FILE: arch/mn10300/kernel/smp.c:984:
+ printk(KERN_ERR

which emacs interprets as a filename. Without that, emacs happily strips off
the 'FILE: ' prefix and uses the filename and line number. I don't
particularly care about the line number in the patch: I'm not editing the
patch - I'm editing the file contributing to the patch.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Joe Perches]

Post by David Howells

Post by Joe Perches
Exactly right. Andy has a version in his testing directory
that fixes this #define run-on block and speeds up checkpatch
runtime rather a lot for certain files like .h files that have
nothing but #defines.
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

It still doesn't work. In fact, I'm seeing more "Logging function has
printk(KERN_ERR
"BUG: CPU#%d started up but did not get a callout!\n",
cpu);

Yes, it's completely borked.

ctx_statement_block returns code beyond the current
statement terminating ";".

Andy may be able to do something about it or know
about some appropriate knob to twist.

I don't really want to learn too much about
checkpatch internals. Sorry.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/