* [PATCH 0/3] checkpatch tweaks @ 2021-06-26 3:40 Jim Cromie 2021-06-26 3:40 ` [PATCH 1/3] checkpatch: skip spacing tests on linker scripts Jim Cromie ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Jim Cromie @ 2021-06-26 3:40 UTC (permalink / raw) To: joe, linux-kernel; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-26 3:40 [PATCH 0/3] checkpatch tweaks Jim Cromie @ 2021-06-26 3:40 ` Jim Cromie 2021-06-26 4:15 ` Joe Perches 2021-06-26 3:40 ` [PATCH 2/3] checkpatch: tweak extern in C warning Jim Cromie 2021-06-26 3:40 ` [PATCH 3/3] checkpatch: suppress BUG_ON warn when it is named in commitmsg Jim Cromie 2 siblings, 1 reply; 15+ messages in thread From: Jim Cromie @ 2021-06-26 3:40 UTC (permalink / raw) To: joe, linux-kernel; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-26 3:40 ` [PATCH 1/3] checkpatch: skip spacing tests on linker scripts Jim Cromie @ 2021-06-26 4:15 ` Joe Perches 2021-06-28 16:53 ` jim.cromie 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2021-06-26 4:15 UTC (permalink / raw) To: Jim Cromie, linux-kernel 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-26 4:15 ` Joe Perches @ 2021-06-28 16:53 ` jim.cromie 2021-06-29 16:48 ` jim.cromie 0 siblings, 1 reply; 15+ messages in thread From: jim.cromie @ 2021-06-28 16:53 UTC (permalink / raw) To: Joe Perches; +Cc: LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-28 16:53 ` jim.cromie @ 2021-06-29 16:48 ` jim.cromie 2021-06-29 18:28 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: jim.cromie @ 2021-06-29 16:48 UTC (permalink / raw) To: Joe Perches; +Cc: LKML 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 && ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-29 16:48 ` jim.cromie @ 2021-06-29 18:28 ` Joe Perches 2021-06-29 19:50 ` jim.cromie 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2021-06-29 18:28 UTC (permalink / raw) To: jim.cromie; +Cc: LKML 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*$/) { ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-29 18:28 ` Joe Perches @ 2021-06-29 19:50 ` jim.cromie 2021-06-29 20:01 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: jim.cromie @ 2021-06-29 19:50 UTC (permalink / raw) To: Joe Perches; +Cc: LKML 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*$/) { > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-29 19:50 ` jim.cromie @ 2021-06-29 20:01 ` Joe Perches 2021-06-30 16:38 ` jim.cromie 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2021-06-29 20:01 UTC (permalink / raw) To: jim.cromie; +Cc: LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-29 20:01 ` Joe Perches @ 2021-06-30 16:38 ` jim.cromie 2021-06-30 17:12 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: jim.cromie @ 2021-06-30 16:38 UTC (permalink / raw) To: Joe Perches; +Cc: LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] checkpatch: skip spacing tests on linker scripts 2021-06-30 16:38 ` jim.cromie @ 2021-06-30 17:12 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2021-06-30 17:12 UTC (permalink / raw) To: jim.cromie; +Cc: LKML 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]); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] checkpatch: tweak extern in C warning 2021-06-26 3:40 [PATCH 0/3] checkpatch tweaks Jim Cromie 2021-06-26 3:40 ` [PATCH 1/3] checkpatch: skip spacing tests on linker scripts Jim Cromie @ 2021-06-26 3:40 ` Jim Cromie 2021-06-26 4:31 ` Joe Perches 2021-06-26 3:40 ` [PATCH 3/3] checkpatch: suppress BUG_ON warn when it is named in commitmsg Jim Cromie 2 siblings, 1 reply; 15+ messages in thread From: Jim Cromie @ 2021-06-26 3:40 UTC (permalink / raw) To: joe, linux-kernel; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] checkpatch: tweak extern in C warning 2021-06-26 3:40 ` [PATCH 2/3] checkpatch: tweak extern in C warning Jim Cromie @ 2021-06-26 4:31 ` Joe Perches 2021-06-27 3:47 ` jim.cromie 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2021-06-26 4:31 UTC (permalink / raw) To: Jim Cromie, linux-kernel 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] checkpatch: tweak extern in C warning 2021-06-26 4:31 ` Joe Perches @ 2021-06-27 3:47 ` jim.cromie 2021-06-30 17:33 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: jim.cromie @ 2021-06-27 3:47 UTC (permalink / raw) To: Joe Perches; +Cc: LKML 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] checkpatch: tweak extern in C warning 2021-06-27 3:47 ` jim.cromie @ 2021-06-30 17:33 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2021-06-30 17:33 UTC (permalink / raw) To: jim.cromie; +Cc: LKML 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] checkpatch: suppress BUG_ON warn when it is named in commitmsg 2021-06-26 3:40 [PATCH 0/3] checkpatch tweaks Jim Cromie 2021-06-26 3:40 ` [PATCH 1/3] checkpatch: skip spacing tests on linker scripts Jim Cromie 2021-06-26 3:40 ` [PATCH 2/3] checkpatch: tweak extern in C warning Jim Cromie @ 2021-06-26 3:40 ` Jim Cromie 2 siblings, 0 replies; 15+ messages in thread From: Jim Cromie @ 2021-06-26 3:40 UTC (permalink / raw) To: joe, linux-kernel; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-30 17:33 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-26 3:40 [PATCH 0/3] checkpatch tweaks Jim Cromie 2021-06-26 3:40 ` [PATCH 1/3] checkpatch: skip spacing tests on linker scripts Jim Cromie 2021-06-26 4:15 ` Joe Perches 2021-06-28 16:53 ` jim.cromie 2021-06-29 16:48 ` jim.cromie 2021-06-29 18:28 ` Joe Perches 2021-06-29 19:50 ` jim.cromie 2021-06-29 20:01 ` Joe Perches 2021-06-30 16:38 ` jim.cromie 2021-06-30 17:12 ` Joe Perches 2021-06-26 3:40 ` [PATCH 2/3] checkpatch: tweak extern in C warning Jim Cromie 2021-06-26 4:31 ` Joe Perches 2021-06-27 3:47 ` jim.cromie 2021-06-30 17:33 ` Joe Perches 2021-06-26 3:40 ` [PATCH 3/3] checkpatch: suppress BUG_ON warn when it is named in commitmsg Jim Cromie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).