checkpatch problem

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

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

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(-)

index 2039acd..f2ae4d5 100755
@@ -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

Works for me.

David

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

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

[]

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

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

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.