checkpatch problem

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* checkpatch problem
@ 2010-09-07 13:09 David Howells
  2010-09-07 18:00 ` Andy Whitcroft
  0 siblings, 1 reply; 9+ messages in thread
From: David Howells @ 2010-09-07 13:09 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: dhowells, linux-kernel


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-07 13:09 checkpatch problem David Howells
@ 2010-09-07 18:00 ` Andy Whitcroft
  2010-09-07 19:41   ` Joe Perches
                     ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Andy Whitcroft @ 2010-09-07 18:00 UTC (permalink / raw)
  To: David Howells; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-07 18:00 ` Andy Whitcroft
@ 2010-09-07 19:41   ` Joe Perches
  2010-09-22 15:33   ` David Howells
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2010-09-07 19:41 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: David Howells, linux-kernel

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-07 18:00 ` Andy Whitcroft
  2010-09-07 19:41   ` Joe Perches
@ 2010-09-22 15:33   ` David Howells
  2010-09-22 17:43   ` David Howells
  2010-09-23 18:15   ` David Howells
  3 siblings, 0 replies; 9+ messages in thread
From: David Howells @ 2010-09-22 15:33 UTC (permalink / raw)
  To: Joe Perches; +Cc: dhowells, Andy Whitcroft, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-07 18:00 ` Andy Whitcroft
  2010-09-07 19:41   ` Joe Perches
  2010-09-22 15:33   ` David Howells
@ 2010-09-22 17:43   ` David Howells
  2010-09-23 18:15   ` David Howells
  3 siblings, 0 replies; 9+ messages in thread
From: David Howells @ 2010-09-22 17:43 UTC (permalink / raw)
  To: Joe Perches; +Cc: dhowells, Andy Whitcroft, linux-kernel


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-07 18:00 ` Andy Whitcroft
                     ` (2 preceding siblings ...)
  2010-09-22 17:43   ` David Howells
@ 2010-09-23 18:15   ` David Howells
  2010-09-23 18:27     ` Joe Perches
  2010-09-23 18:54     ` David Howells
  3 siblings, 2 replies; 9+ messages in thread
From: David Howells @ 2010-09-23 18:15 UTC (permalink / raw)
  To: Joe Perches; +Cc: dhowells, Andy Whitcroft, linux-kernel


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-23 18:15   ` David Howells
@ 2010-09-23 18:27     ` Joe Perches
  2010-09-23 18:54     ` David Howells
  1 sibling, 0 replies; 9+ messages in thread
From: Joe Perches @ 2010-09-23 18:27 UTC (permalink / raw)
  To: David Howells; +Cc: Andy Whitcroft, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-23 18:15   ` David Howells
  2010-09-23 18:27     ` Joe Perches
@ 2010-09-23 18:54     ` David Howells
  2010-09-23 20:31       ` Joe Perches
  1 sibling, 1 reply; 9+ messages in thread
From: David Howells @ 2010-09-23 18:54 UTC (permalink / raw)
  To: Joe Perches; +Cc: dhowells, Andy Whitcroft, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: checkpatch problem
  2010-09-23 18:54     ` David Howells
@ 2010-09-23 20:31       ` Joe Perches
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2010-09-23 20:31 UTC (permalink / raw)
  To: David Howells; +Cc: Andy Whitcroft, linux-kernel

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-09-23 20:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-07 13:09 checkpatch problem David Howells
2010-09-07 18:00 ` Andy Whitcroft
2010-09-07 19:41   ` Joe Perches
2010-09-22 15:33   ` David Howells
2010-09-22 17:43   ` David Howells
2010-09-23 18:15   ` David Howells
2010-09-23 18:27     ` Joe Perches
2010-09-23 18:54     ` David Howells
2010-09-23 20:31       ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome