[PATCH 0/3] checkpatch tweaks

[PATCH 0/3] checkpatch tweaks

[Jim Cromie]

here are 3 minor tweaks to checkpatch
- skip reporting of a SPACING test, based on filename exclusion.
  *.lds.h are linker scripts, some rules don't apply
- exclude extern-C warning for symbol patterns common to vmlinux.lds.h
  (_start|_stop|_end) currently, pls tweak as fitting.
- subdue warning on BUG_ON if BUG_ON is mentioned in commit log

Jim Cromie (3):
  checkpatch: skip spacing tests on linker scripts
  checkpatch: tweak extern in C warning
  checkpatch: suppress BUG_ON warn when it is named in commitmsg

 scripts/checkpatch.pl | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[Jim Cromie]

Before issuing a WARNING on spacing, exclude reports on linker
scripts, which don't comport to C style rules.  skip_on_filename()
skips on *.lds.h files, I think covering all cases.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 scripts/checkpatch.pl | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 23697a6b1eaa..4fe9fab20009 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2546,6 +2546,11 @@ sub get_raw_comment {
 	return $comment;
 }
 
+sub skip_on_filename {
+	my $fname = shift;
+	return $fname =~ m@\.lds\.h$@;
+}
+
 sub exclude_global_initialisers {
 	my ($realfile) = @_;
 
@@ -5134,7 +5139,8 @@ sub process {
 							}
 						}
 					} elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
-						if (ERROR("SPACING",
+						if (!skip_on_filename($realfile) &&
+						    ERROR("SPACING",
 							  "need consistent spacing around '$op' $at\n" . $hereptr)) {
 							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
 							if (defined $fix_elements[$n + 2]) {
-- 
2.31.1

[PATCH 2/3] checkpatch: tweak extern in C warning

[Jim Cromie]

The extern-in-C rule has one important exception: the symbol is
defined in/by the linker script.  By convention, these almost always
contain: _start, _stop, _end.  Suppress the warning on such symbols.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4fe9fab20009..a8dfba53b593 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6910,7 +6910,8 @@ sub process {
 		    $stat =~ /^.\s*extern\s+/)
 		{
 			WARN("AVOID_EXTERNS",
-			     "externs should be avoided in .c files\n" .  $herecurr);
+			     "externs should be avoided in .c files\n($stat)\n" .  $herecurr)
+			    unless $stat =~ /_start|_stop|_end/;
 		}
 
 # check for function declarations that have arguments without identifier names
-- 
2.31.1

[PATCH 3/3] checkpatch: suppress BUG_ON warn when it is named in commitmsg

[Jim Cromie]

allow mention of BUG_ON in the preamble/commitmsg/intro to silence the
warning normally issued when one is added.  This presumes the commit
message will adequately explain the reason "BUG_ON" is appropriate.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 scripts/checkpatch.pl | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8dfba53b593..32612f39d742 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2560,6 +2560,18 @@ sub exclude_global_initialisers {
 		$realfile =~ m@/bpf/.*\.bpf\.c$@;
 }
 
+sub commitmsg_refers_to {	# see if $srch is in commit message
+	my ($srch, $lines) = @_;
+	#print "ok checking for $srch in $lines lines\n";
+	for my $i (0..$lines) {
+		if ($rawlines[$i] =~ /$srch/) {
+			print "\thmm: $srch mentioned in preamble, presuming it is explained\n";
+			return 1;
+		}
+	}
+	return 0;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -2586,6 +2598,7 @@ sub process {
 	my $has_patch_separator = 0;	#Found a --- line
 	my $has_commit_log = 0;		#Encountered lines before patch
 	my $commit_log_lines = 0;	#Number of commit log lines
+	my $eopreamble = 0;		# above truncates at =~ /^\. \w+/
 	my $commit_log_possible_stack_dump = 0;
 	my $commit_log_long_line = 0;
 	my $commit_log_has_diff = 0;
@@ -2731,6 +2744,7 @@ sub process {
 		    ($line =~ /^rename (?:from|to) \S+\s*$/ ||
 		     $line =~ /^diff --git a\/[\w\/\.\_\-]+ b\/\S+\s*$/))) {
 			$is_patch = 1;
+			$eopreamble = $linenr;
 		}
 
 #extract the line range in the file after the patch is applied
@@ -4654,7 +4668,7 @@ sub process {
 		}
 
 # avoid BUG() or BUG_ON()
-		if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
+		if ($line =~ /\b(BUG|BUG_ON)\b/ && !commitmsg_refers_to($1, $eopreamble)) {
 			my $msg_level = \&WARN;
 			$msg_level = \&CHK if ($file);
 			&{$msg_level}("AVOID_BUG",
-- 
2.31.1

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[Joe Perches]

On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> Before issuing a WARNING on spacing, exclude reports on linker
> scripts, which don't comport to C style rules.  skip_on_filename()
> skips on *.lds.h files, I think covering all cases.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2546,6 +2546,11 @@ sub get_raw_comment {
>  	return $comment;
>  }
> 
> +sub skip_on_filename {
> +	my $fname = shift;
> +	return $fname =~ m@\.lds\.h$@;
> +}

nak.  This is poor naming for what is not a generic function.

> @@ -5134,7 +5139,8 @@ sub process {
>  							}
>  						}
>  					} elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
> -						if (ERROR("SPACING",
> +						if (!skip_on_filename($realfile) &&
> +						    ERROR("SPACING",
>  							  "need consistent spacing around '$op' $at\n" . $hereptr)) {
>  							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
>  							if (defined $fix_elements[$n + 2]) {

This .lds.h test is also used in one other place.

It might be better to avoid all tests in .lds.h files by using a
"next if" much earlier.

Re: [PATCH 2/3] checkpatch: tweak extern in C warning

[Joe Perches]

On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> The extern-in-C rule has one important exception: the symbol is
> defined in/by the linker script.  By convention, these almost always
> contain: _start, _stop, _end.  Suppress the warning on such symbols.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> @@ -6910,7 +6910,8 @@ sub process {
>  		    $stat =~ /^.\s*extern\s+/)
>  		{
>  			WARN("AVOID_EXTERNS",
> -			     "externs should be avoided in .c files\n" .  $herecurr);
> +			     "externs should be avoided in .c files\n($stat)\n" .  $herecurr)
> +			    unless $stat =~ /_start|_stop|_end/;

nak.

As far as I can tell, there's no reason these symbols
should not be in .h files.

besides that:

output is single line, $stat should not be used and
using unless is not desired.

Re: [PATCH 2/3] checkpatch: tweak extern in C warning

[jim.cromie]

On Sat, Jun 26, 2021 at 12:46 PM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > The extern-in-C rule has one important exception: the symbol is
> > defined in/by the linker script.  By convention, these almost always
> > contain: _start, _stop, _end.  Suppress the warning on such symbols.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > @@ -6910,7 +6910,8 @@ sub process {
> >                   $stat =~ /^.\s*extern\s+/)
> >               {
> >                       WARN("AVOID_EXTERNS",
> > -                          "externs should be avoided in .c files\n" .  $herecurr);
> > +                          "externs should be avoided in .c files\n($stat)\n" .  $herecurr)
> > +                         unless $stat =~ /_start|_stop|_end/;
>
> nak.
>
> As far as I can tell, there's no reason these symbols
> should not be in .h files.
>

judging from the codebase, it has been a case-by-case decision,
with 8/10 of the linker-vars extern'd into C files, not headers.

[jimc@frodo wk-test]$ perl -ne '/(\w*_(?:start|stop|end)(?:_\w+))/ and
print "$1\n"' `find . -name \*.lds.h` > symbols
[jimc@frodo wk-test]$ wc symbols
  99   99 2112 symbols
[jimc@frodo wk-test]$ grep -n -r --exclude-dir=builds/ -f symbols .  |
grep -E '\.c:' | grep extern | wc
     79     331    6402
[jimc@frodo wk-test]$ grep -n -r --exclude-dir=builds/ -f symbols .  |
grep -E '\.h' | grep extern | wc
     19      81    1581

8/10 cases dont expose these symbols in headers,
Makes sense to me, mostly theyre internal, and often double-underscored too.
2/10 are presumably in headers for specific reasons.


> besides that:
>
> output is single line, $stat should not be used and
> using unless is not desired.
>

could you clarify ?
style issues are easy, std if form...
$stat is already used, it must contain extern to get here.
checking it for a likely-linker-symbol seems fair.

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[jim.cromie]

Yes, I agree.

On Sun, Jun 27, 2021 at 9:17 PM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > Before issuing a WARNING on spacing, exclude reports on linker
> > scripts, which don't comport to C style rules.  skip_on_filename()
> > skips on *.lds.h files, I think covering all cases.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2546,6 +2546,11 @@ sub get_raw_comment {
> >       return $comment;
> >  }
> >
> > +sub skip_on_filename {
> > +     my $fname = shift;
> > +     return $fname =~ m@\.lds\.h$@;
> > +}
>
> nak.  This is poor naming for what is not a generic function.

indeed.



>
> > @@ -5134,7 +5139,8 @@ sub process {
> >                                                       }
> >                                               }
> >                                       } elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
> > -                                             if (ERROR("SPACING",
> > +                                             if (!skip_on_filename($realfile) &&
> > +                                                 ERROR("SPACING",
> >                                                         "need consistent spacing around '$op' $at\n" . $hereptr)) {
> >                                                       $good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
> >                                                       if (defined $fix_elements[$n + 2]) {
>
> This .lds.h test is also used in one other place.
>
> It might be better to avoid all tests in .lds.h files by using a
> "next if" much earlier.

YES.  I see the code uses  'next if' syntax. so this would work.

   next if ($filename =~ m@\.lds\.h$@)

I will find a good spot for this line.

thanks
Jim

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[jim.cromie]

hi Joe,

> >
> > This .lds.h test is also used in one other place.
> >
> > It might be better to avoid all tests in .lds.h files by using a
> > "next if" much earlier.
>

checkpatch: subtle decrufting

sub process() uses a next-if statement to end a block of tests early,
because following tests pertain only to certain types of source files.
That statement has some history:

 $ grep -P 'sub process|next if \(\$realfile' blame-check
 0a920b5b666d0 (Andy Whitcroft      2007-06-01 00:46:48 -0700 2558)
sub process {
 d6430f71805aa (Joe Perches         2016-12-12 16:46:28 -0800 3621)
next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
 de4c924c26504 (Geert Uytterhoeven  2014-10-13 15:51:46 -0700 3712)
next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 b9ea10d691ecb (Andy Whitcroft      2008-10-15 22:02:24 -0700 3973)
next if ($realfile !~ /\.(h|c)$/);

Commit:b9ea adds the early-block-termination line, then 2 subsequent
commits (de4c, d643) copy that line up/earlier in sub process (with
different filetype selection), largely masking the purposes of the
older/later lines (block-early-terminate to skip file-type specific
tests).

This code is hurting my brain.
its the combo of
3 different multiple file-type selections, each stricter than previous
early block terminate semantics
!~ (one more logical inversion)

commit:de4c   allows further testing on *.pl files
but commit:d643  doesnt allow those same files to reach that spot
ISTM this wasnt quite intended.

The 3rd/original usage still has some effect,
forex when a dts file reaches that line

How to resolve these issues ?
changing d643 to allow *.pl to fall thru for further testing
is probably the best small move.

FWIW, one version of a 1-line fix for *.lds.h files.
this one adds the new line after the 1st of the 3 blame-lines.
Maybe it should be added after the SPDX check (which would complain)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6469870837d0..1655d88fe5e3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3633,6 +3633,7 @@ sub process {

 # check we are in a valid source file if not then ignore this hunk
                next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
+               next if ($realfile =~ /\.lds\.h$/); # linker scripts
arent real *.h

 # check for using SPDX-License-Identifier on the wrong line number
                if ($realline != $checklicenseline &&

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[Joe Perches]

On Tue, 2021-06-29 at 10:48 -0600, jim.cromie@gmail.com wrote:
> hi Joe,

hey Jim.

> > > This .lds.h test is also used in one other place.
> > > 
> > > It might be better to avoid all tests in .lds.h files by using a
> > > "next if" much earlier.
> 
> checkpatch: subtle decrufting
> 
> sub process() uses a next-if statement to end a block of tests early,
> because following tests pertain only to certain types of source files.
> That statement has some history:
> 
>  $ grep -P 'sub process|next if \(\$realfile' blame-check
>  0a920b5b666d0 (Andy Whitcroft      2007-06-01 00:46:48 -0700 2558) sub process {
>  d6430f71805aa (Joe Perches         2016-12-12 16:46:28 -0800 3621) next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
>  de4c924c26504 (Geert Uytterhoeven  2014-10-13 15:51:46 -0700 3712) next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);

Looks like I should have also removed the |pl from this block
when I removed it from commit d6430f71805aa. 

Oh well, no real harm done...

>  b9ea10d691ecb (Andy Whitcroft      2008-10-15 22:02:24 -0700 3973) next if ($realfile !~ /\.(h|c)$/);
> 
> Commit:b9ea adds the early-block-termination line, then 2 subsequent
> commits (de4c, d643) copy that line up/earlier in sub process (with
> different filetype selection), largely masking the purposes of the
> older/later lines (block-early-terminate to skip file-type specific
> tests).

Not really.

The first in file order next-if commit d6430f71805aa was a
modification of the earlier commits listed below:

5368df20fb364e
00df344fd06fd6
0a920b5b666d0b

All of these were just additions of various file types to the test.

> This code is hurting my brain.

Perhaps Advil or another leaded or unleaded beverage might help.
They help me...

> changing d643 to allow *.pl to fall thru for further testing
> is probably the best small move.

Definitely not as it's there specifically to avoid long line tests in perl.

> FWIW, one version of a 1-line fix for *.lds.h files.
> this one adds the new line after the 1st of the 3 blame-lines.
> Maybe it should be added after the SPDX check (which would complain)

Maybe a slight reworking of all the "next if" tests would work.

I moved the incorrect spdx line number test up, but didn't test
whether or not it's appropriate here as I don't know of a case
of the top of my head.  I also don't know if the linker .lds.h
files should be tested for long lines or not.

It looks like these files are mostly < 80 columns

$ git ls-files -- '*.lds.h'| xargs cat | awk '{print length($0), $0;}' | sort -rn | head
106 #define DATA_MAIN .data .data.[0-9a-zA-Z_]* .data..L* .data..compoundliteral* .data.$__unnamed_* .data.$L*
94 #if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) || \
79 /* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
78  * [__init_begin, __init_end] is the init section that may be freed after init
78 #if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
77  * .init section and thus will be preserved for later use in the decompressed
77 #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
77  * <asm/module.lds.h> can specify arch-specific sections for linking modules.
76 #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
76  * .boot.data variables are kept in separate .boot.data.<var name> sections,

---
 scripts/checkpatch.pl | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 461d4221e4a4a..ea198499e16df 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3617,9 +3617,6 @@ sub process {
 			     "It's generally not useful to have the filename in the file\n" . $herecurr);
 		}
 
-# check we are in a valid source file if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
-
 # check for using SPDX-License-Identifier on the wrong line number
 		if ($realline != $checklicenseline &&
 		    $rawline =~ /\bSPDX-License-Identifier:/ &&
@@ -3628,6 +3625,9 @@ sub process {
 			     "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
 		}
 
+# check we are in a valid source file if not then ignore this hunk
+		next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
+
 # line length limit (with some exclusions)
 #
 # There are a few types of lines that may extend beyond $max_line_length:
@@ -3708,8 +3708,8 @@ sub process {
 			     "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
 		}
 
-# check we are in a valid source file C or perl if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
+# check we are in a valid source C or .dts? file, if not then ignore this hunk
+		next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
 # more than $tabsize must use tabs.
@@ -3737,6 +3737,9 @@ sub process {
 			}
 		}
 
+# skip all following test for linker files.
+		next if ($realfile =~ /\.lds\.h$/);
+
 # check for assignments on the start of a line
 		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
 			my $operator = $1;
@@ -3970,7 +3973,7 @@ sub process {
 		}
 
 # check we are in a valid C source file if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c)$/);
+		next if ($realfile !~ /\.(?:h|c)$/);
 
 # check for unusual line ending [ or (
 		if ($line =~ /^\+.*([\[\(])\s*$/) {

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[jim.cromie]

On Tue, Jun 29, 2021 at 12:28 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2021-06-29 at 10:48 -0600, jim.cromie@gmail.com wrote:
> > hi Joe,
>
> hey Jim.
>
> > > > This .lds.h test is also used in one other place.
> > > >
> > > > It might be better to avoid all tests in .lds.h files by using a
> > > > "next if" much earlier.
> >
> > checkpatch: subtle decrufting
> >
> > sub process() uses a next-if statement to end a block of tests early,
> > because following tests pertain only to certain types of source files.
> > That statement has some history:
> >
> >  $ grep -P 'sub process|next if \(\$realfile' blame-check
> >  0a920b5b666d0 (Andy Whitcroft      2007-06-01 00:46:48 -0700 2558) sub process {
> >  d6430f71805aa (Joe Perches         2016-12-12 16:46:28 -0800 3621) next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> >  de4c924c26504 (Geert Uytterhoeven  2014-10-13 15:51:46 -0700 3712) next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
>
> Looks like I should have also removed the |pl from this block
> when I removed it from commit d6430f71805aa.
>
> Oh well, no real harm done...
>
> >  b9ea10d691ecb (Andy Whitcroft      2008-10-15 22:02:24 -0700 3973) next if ($realfile !~ /\.(h|c)$/);
> >
> > Commit:b9ea adds the early-block-termination line, then 2 subsequent
> > commits (de4c, d643) copy that line up/earlier in sub process (with
> > different filetype selection), largely masking the purposes of the
> > older/later lines (block-early-terminate to skip file-type specific
> > tests).
>
> Not really.
>
> The first in file order next-if commit d6430f71805aa was a
> modification of the earlier commits listed below:
>
> 5368df20fb364e
> 00df344fd06fd6
> 0a920b5b666d0b
>
> All of these were just additions of various file types to the test.
>
> > This code is hurting my brain.
>
> Perhaps Advil or another leaded or unleaded beverage might help.
> They help me...
>
> > changing d643 to allow *.pl to fall thru for further testing
> > is probably the best small move.
>
> Definitely not as it's there specifically to avoid long line tests in perl.
>

Ok, I see that..  its all pretty subtle.


> > FWIW, one version of a 1-line fix for *.lds.h files.
> > this one adds the new line after the 1st of the 3 blame-lines.
> > Maybe it should be added after the SPDX check (which would complain)
>

I tried that 1-liner : next if lds.h
after each of those 3 blame-lines,  the latter 2 have effect
(subtracting errors)
the first doesnt improve on the later one.


[jimc@frodo wk-other]$ git log
commit 6825b43bae4cd51808050de3769f1d3df5b0bc76 (HEAD -> checkpatch-2)
Author: Jim Cromie <jim.cromie@gmail.com>
Date:   Tue Jun 29 12:34:40 2021 -0600

    checkpatch: skip more tests on linker scripts

    before:
    total: 1 errors, 7 warnings, 1152 lines checked

    after:
    total: 0 errors, 1 warnings, 1152 lines checked

commit a9f9a26a299097f9b6524a25681cc45c04aec6b5
Author: Jim Cromie <jim.cromie@gmail.com>
Date:   Tue Jun 29 12:31:29 2021 -0600

    checkpatch: skip (c|h) tests on linker scripts

    testing with:
    $ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h

    before:
    total: 18 errors, 8 warnings, 1152 lines checked

    after:
    total: 1 errors, 7 warnings, 1152 lines checked




> Maybe a slight reworking of all the "next if" tests would work.
>
> I moved the incorrect spdx line number test up, but didn't test
> whether or not it's appropriate here as I don't know of a case
> of the top of my head.  I also don't know if the linker .lds.h
> files should be tested for long lines or not.

ISTM the -f option is that test-case - use existing file ( vmlinux.lds.h )
as that reference, since it exists, everything in it is "approved style"

The current reference elicits 18 errors and 8 warnings,
ISTM those tests are inappropriate for this kind of file.


>
> It looks like these files are mostly < 80 columns
>
> $ git ls-files -- '*.lds.h'| xargs cat | awk '{print length($0), $0;}' | sort -rn | head
> 106 #define DATA_MAIN .data .data.[0-9a-zA-Z_]* .data..L* .data..compoundliteral* .data.$__unnamed_* .data.$L*
> 94 #if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) || \
> 79 /* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
> 78  * [__init_begin, __init_end] is the init section that may be freed after init
> 78 #if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
> 77  * .init section and thus will be preserved for later use in the decompressed
> 77 #define RESERVEDMEM_OF_TABLES()      OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
> 77  * <asm/module.lds.h> can specify arch-specific sections for linking modules.
> 76 #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
> 76  * .boot.data variables are kept in separate .boot.data.<var name> sections,
>
> ---
>  scripts/checkpatch.pl | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)


this does 3 different things

- non-capturing matches  -  these add no functionality,

- moves the skip-remaining-tests check after SPDX
   that feels like a legal Q: should it be on all files ?
   moving it does seem proper though.

- adds the skip linker-script
  since i went ahead and added it 3 times to see errs/warns
  I didnt consider your precise placement,
  how does it do with 18/8 errs/warnings on ref-test ?


>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 461d4221e4a4a..ea198499e16df 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3617,9 +3617,6 @@ sub process {
>                              "It's generally not useful to have the filename in the file\n" . $herecurr);
>                 }
>
> -# check we are in a valid source file if not then ignore this hunk
> -               next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> -
>  # check for using SPDX-License-Identifier on the wrong line number
>                 if ($realline != $checklicenseline &&
>                     $rawline =~ /\bSPDX-License-Identifier:/ &&
> @@ -3628,6 +3625,9 @@ sub process {
>                              "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
>                 }
>
> +# check we are in a valid source file if not then ignore this hunk
> +               next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
> +
>  # line length limit (with some exclusions)
>  #
>  # There are a few types of lines that may extend beyond $max_line_length:
> @@ -3708,8 +3708,8 @@ sub process {
>                              "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
>                 }
>
> -# check we are in a valid source file C or perl if not then ignore this hunk
> -               next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
> +# check we are in a valid source C or .dts? file, if not then ignore this hunk
> +               next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
>
>  # at the beginning of a line any tabs must come first and anything
>  # more than $tabsize must use tabs.
> @@ -3737,6 +3737,9 @@ sub process {
>                         }
>                 }
>
> +# skip all following test for linker files.
> +               next if ($realfile =~ /\.lds\.h$/);
> +
>  # check for assignments on the start of a line
>                 if ($sline =~ /^\+\s+($Assignment)[^=]/) {
>                         my $operator = $1;
> @@ -3970,7 +3973,7 @@ sub process {
>                 }
>
>  # check we are in a valid C source file if not then ignore this hunk
> -               next if ($realfile !~ /\.(h|c)$/);
> +               next if ($realfile !~ /\.(?:h|c)$/);
>
>  # check for unusual line ending [ or (
>                 if ($line =~ /^\+.*([\[\(])\s*$/) {
>

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[Joe Perches]

On Tue, 2021-06-29 at 13:50 -0600, jim.cromie@gmail.com wrote:
> this does 3 different things
> 
> - non-capturing matches  -  these add no functionality,

true, it's nominally a bit faster through.

> - moves the skip-remaining-tests check after SPDX
>    that feels like a legal Q: should it be on all files ?
>    moving it does seem proper though.

to me too.

> - adds the skip linker-script
>   since i went ahead and added it 3 times to see errs/warns
>   I didnt consider your precise placement,
>   how does it do with 18/8 errs/warnings on ref-test ?

$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse
include/asm-generic/vmlinux.lds.h:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
include/asm-generic/vmlinux.lds.h:43: WARNING: please, no space before tabs
include/asm-generic/vmlinux.lds.h:101: CHECK: line length of 106 exceeds 100 columns
include/asm-generic/vmlinux.lds.h:390: WARNING: please, no space before tabs
include/asm-generic/vmlinux.lds.h:546: ERROR: code indent should use tabs where possible
total: 1 errors, 3 warnings, 1 checks, 1184 lines checked

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[jim.cromie]

On Tue, Jun 29, 2021 at 2:01 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2021-06-29 at 13:50 -0600, jim.cromie@gmail.com wrote:
> > this does 3 different things
> >
> > - non-capturing matches  -  these add no functionality,
>
> true, it's nominally a bit faster through.
>
> > - moves the skip-remaining-tests check after SPDX
> >    that feels like a legal Q: should it be on all files ?
> >    moving it does seem proper though.
>
> to me too.
>
> > - adds the skip linker-script
> >   since i went ahead and added it 3 times to see errs/warns
> >   I didnt consider your precise placement,
> >   how does it do with 18/8 errs/warnings on ref-test ?
>
> $ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse

cool options.
<Aside>
some oddities are hidden there;
Im seeing the err/warn counts change along with use of those options.
not a big deal, but it is mildly surprising
forex:
$ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
...
total: 18 errors, 7 warnings, 1164 lines checked
$ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
...
total: 9 errors, 7 warnings, 95 checks, 1164 lines checked


> include/asm-generic/vmlinux.lds.h:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> include/asm-generic/vmlinux.lds.h:43: WARNING: please, no space before tabs
> include/asm-generic/vmlinux.lds.h:101: CHECK: line length of 106 exceeds 100 columns
> include/asm-generic/vmlinux.lds.h:390: WARNING: please, no space before tabs
> include/asm-generic/vmlinux.lds.h:546: ERROR: code indent should use tabs where possible
> total: 1 errors, 3 warnings, 1 checks, 1184 lines checked
>

2nd one is I think pedantic comment formatting, but on the whole, fair
complaints.
and I see your insertion spot is right between my 2 picks.
works for me.

just to note, this is about a generalization of

commit 263afd39c06f5939ef943e0d535380d4b8e56484
Author: Chris Down <chris@chrisdown.name>
Date:   Thu Feb 25 17:22:04 2021 -0800

    checkpatch: don't warn about colon termination in linker scripts

Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts

[Joe Perches]

On Wed, 2021-06-30 at 10:38 -0600, jim.cromie@gmail.com wrote:
> On Tue, Jun 29, 2021 at 2:01 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Tue, 2021-06-29 at 13:50 -0600, jim.cromie@gmail.com wrote:
> > > this does 3 different things
> > > 
> > > - non-capturing matches  -  these add no functionality,
> > 
> > true, it's nominally a bit faster through.
> > 
> > > - moves the skip-remaining-tests check after SPDX
> > >    that feels like a legal Q: should it be on all files ?
> > >    moving it does seem proper though.
> > 
> > to me too.
> > 
> > > - adds the skip linker-script
> > >   since i went ahead and added it 3 times to see errs/warns
> > >   I didnt consider your precise placement,
> > >   how does it do with 18/8 errs/warnings on ref-test ?
> > 
> > $ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse
> 
> cool options.
> <Aside>
> some oddities are hidden there;
> Im seeing the err/warn counts change along with use of those options.
> not a big deal, but it is mildly surprising
> forex:
> $ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
> ...
> total: 18 errors, 7 warnings, 1164 lines checked
> $ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
> ...
> total: 9 errors, 7 warnings, 95 checks, 1164 lines checked


The difference is a --strict test 'if ($check)' that precedes and
in effect 'overrides' the ERROR output for that output around line 5120.

$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
[]
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)

vs:

$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
[]
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)

> just to note, this is about a generalization of
> 
> commit 263afd39c06f5939ef943e0d535380d4b8e56484
> Author: Chris Down <chris@chrisdown.name>
> Date:   Thu Feb 25 17:22:04 2021 -0800
> 
>     checkpatch: don't warn about colon termination in linker scripts

Which means the additional test in that commit should be removed too.

Maybe:
---
 scripts/checkpatch.pl | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 461d4221e4a4a..f4f5826054214 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3617,9 +3617,6 @@ sub process {
 			     "It's generally not useful to have the filename in the file\n" . $herecurr);
 		}
 
-# check we are in a valid source file if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
-
 # check for using SPDX-License-Identifier on the wrong line number
 		if ($realline != $checklicenseline &&
 		    $rawline =~ /\bSPDX-License-Identifier:/ &&
@@ -3628,6 +3625,9 @@ sub process {
 			     "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
 		}
 
+# check we are in a valid source file if not then ignore this hunk
+		next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
+
 # line length limit (with some exclusions)
 #
 # There are a few types of lines that may extend beyond $max_line_length:
@@ -3708,8 +3708,8 @@ sub process {
 			     "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
 		}
 
-# check we are in a valid source file C or perl if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
+# check we are in a valid source C or .dts? file, if not then ignore this hunk
+		next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
 # more than $tabsize must use tabs.
@@ -3737,6 +3737,9 @@ sub process {
 			}
 		}
 
+# skip all following test for linker files.
+		next if ($realfile =~ /\.lds\.h$/);
+
 # check for assignments on the start of a line
 		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
 			my $operator = $1;
@@ -3970,7 +3973,7 @@ sub process {
 		}
 
 # check we are in a valid C source file if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c)$/);
+		next if ($realfile !~ /\.(?:h|c)$/);
 
 # check for unusual line ending [ or (
 		if ($line =~ /^\+.*([\[\(])\s*$/) {
@@ -5147,7 +5150,7 @@ sub process {
 				# A colon needs no spaces before when it is
 				# terminating a case value or a label.
 				} elsif ($opv eq ':C' || $opv eq ':L') {
-					if ($ctx =~ /Wx./ and $realfile !~ m@.*\.lds\.h$@) {
+					if ($ctx =~ /Wx./) {
 						if (ERROR("SPACING",
 							  "space prohibited before that '$op' $at\n" . $hereptr)) {
 							$good = rtrim($fix_elements[$n]) . trim($fix_elements[$n + 1]);

Re: [PATCH 2/3] checkpatch: tweak extern in C warning

[Joe Perches]

On Sat, 2021-06-26 at 21:47 -0600, jim.cromie@gmail.com wrote:
> On Sat, Jun 26, 2021 at 12:46 PM Joe Perches <joe@perches.com> wrote:
> > On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > > The extern-in-C rule has one important exception: the symbol is
> > > defined in/by the linker script.  By convention, these almost always
> > > contain: _start, _stop, _end.  Suppress the warning on such symbols.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > @@ -6910,7 +6910,8 @@ sub process {
> > >                   $stat =~ /^.\s*extern\s+/)
> > >               {
> > >                       WARN("AVOID_EXTERNS",
> > > -                          "externs should be avoided in .c files\n" .  $herecurr);
> > > +                          "externs should be avoided in .c files\n($stat)\n" .  $herecurr)
> > > +                         unless $stat =~ /_start|_stop|_end/;
> > 
> > nak.
> > 
> > As far as I can tell, there's no reason these symbols
> > should not be in .h files.
> 
> judging from the codebase, it has been a case-by-case decision,
> with 8/10 of the linker-vars extern'd into C files, not headers.
[]
> > besides that:
> > 
> > output is single line, $stat should not be used and
> > using unless is not desired.
> > 
> 
> could you clarify ?
> style issues are easy, std if form...
> $stat is already used, it must contain extern to get here.

Sure, it's used as part of a test but it's never output as part of
an error message.  $stat strips any leading '+' from the 2nd and
subsequent lines.

There's a mechanism used in several other tests to show these lines.

				my $cnt = statement_rawlines($stat);
				my $herectx = get_stat_here($linenr, $cnt, $here);

with the output of $herectx.

> checking it for a likely-linker-symbol seems fair.