checkpatch problem

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

Re: checkpatch problem

[Andy Whitcroft]

On Tue, Sep 07, 2010 at 02:09:42PM +0100, David Howells wrote:
> 
> 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.

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

Re: checkpatch problem

[Joe Perches]

On Tue, 2010-09-07 at 19:00 +0100, Andy Whitcroft wrote:
> On Tue, Sep 07, 2010 at 02:09:42PM +0100, David Howells wrote:
> > 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'.
> 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

Re: checkpatch problem

[David Howells]

Joe Perches <joe@perches.com> wrote:

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

Works for me.

David

Re: checkpatch problem

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> 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

Re: checkpatch problem

[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

Re: checkpatch problem

[Joe Perches]

On Thu, 2010-09-23 at 19:15 +0100, David Howells wrote:
> 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:
[]
> 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

Re: checkpatch problem

[David Howells]

Joe Perches <joe@perches.com> wrote:

> 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

Re: checkpatch problem

[Joe Perches]

On Thu, 2010-09-23 at 19:54 +0100, David Howells wrote:
> Joe Perches <joe@perches.com> wrote:
> > 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);

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.