[RFC PATCH 0/2] practical auto-QA de-noiser or -E_TOO_UGLY ?

[RFC PATCH 0/2] practical auto-QA de-noiser or -E_TOO_UGLY ?

[Jim Cromie]

Can I haz (in compiler.h)

  #define __no_side_effects(...)    /* checkpatch hint only */

module_param_named() if submitted now, would draw several warnings
from checkpatch --strict, on name, value (but not type).

  CHECK: Macro argument reuse '_var' - possible side-effects?

This macro allows an author to suppress that complaint, clearly and
declaratively, and strongly suggests that they used --strict, saw the
complaint, and silenced it.  So they know not to pass i++.

Do the possible QA/CI benefits suffice for the mild "hackish" flavor ?


Jim Cromie (2):
  checkpatch: add __no_side_effects() hint/assertion macro
  checkpatch: cosmetic-style tweak

 include/linux/compiler.h | 12 +++++++++
 scripts/checkpatch.pl    | 54 ++++++++++++++++++++++++++++++++++------
 2 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.51.0

[RFC PATCH 1/2] checkpatch: add __no_side_effects() hint/assertion macro

[Jim Cromie]

Depending upon your macros, `checkpatch --strict` may issue many
warnings like:

  CHECK: Macro argument reuse '_var' - possible side-effects?

In (at least) 2 cases, the usual advice to use ({ typeof .. })
or do{}while(0) does not apply:

1. top-of-scope (inc file-scope) declarative macros:

   #define module_param_named(name, value, type, perm)                        \
        param_check_##type(name, &(value));                                \
        module_param_cb(name, &param_ops_##type, &value, perm);            \
        __MODULE_PARM_TYPE(name, #type)

Given this macro-defn, checkpatch would carp about name and value.
Note that it wouldn't complain about type, even though type is
expanded 3 times, because it knew to strip # and ## constructs before
counting args.

2. #define for_simplicity(_i, _lim, ...) \
   	   for( _i = 0, _i < _lim; _i++ )  /* your loop body here */

Though not a compelling example, it should'nt be impossible.  ({})
fails because () detaches the expansion from the following { block }.
And the do-while adds a ';'.

So allow a macro author to assert that (_i) is safe to expand 2+ times:

   #define for_simplicity(_i, _lim, ...) \
   	   __no_side_effects(_i)	 \
   	   for( _i = 0, _i < _lim; _i++ )

This should help reduce auto-QA/pre-review noise, even though its a
homely little macro.

It has 2 parts:

1. compiler.h:  #define __no_side_effects(...)  /* nothing */
   so no functional change from its use.
   authors add it to "noisy" macros
   readily reviewable, and marked for it by name.

2. checkpatch: suppress the CHECK reporting:
   a. find the hint in macro body, capture the safe-args (_i, _j)
   b. strip the hint, reflecting the /* nothing */ above
   c. before issuing CHK() on arg-use-count > 1, skip if /safe-args/

3. --drx option, which enables a new sub drx_print("reason");

   a. used like: s/$pattern/drx_print("why")/e;
   b. calls it on 3 existing cases.

Here it is in action, on a patch that prompted this one:

drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_var##>
drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_var##>
drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_var##>
drx_print: dunno
  >> Matched (`$&`): <__builtin_constant_p(cls>
  >> Capture 1 (`$1`): <__builtin_constant_p>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: dunno
  >> Matched (`$&`): <##_model>
  >> Capture 1 (`$1`): <##>
drx_print: strip '#|## arg catenations
  >> Matched (`$&`): <#_flags>
drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_flags##>
drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_flags##>
drx_print: strip 'arg ##' catenations
  >> Matched (`$&`): <_flags##>
total: 0 errors, 0 warnings, 712 lines checked

This should be useful for hacking at checkpatch if/when the behavior
of an existing s/// or s///g is sufficiently mysterious that a few
s//drx_print("wtf")/ge modifications seem appropriate.  Some of them
will be worth keeping, since the "reason" can be inspected, improved,
verified by using --drx as needed.

NOTES:

This adds to the stack of heuristics: cpp doesn't mind expanding i++,
so checkpatch is paranoid; this quiets that paranoia clearly.

The macro call must *exclude* the trailing semi-colon.

practicality vs aesthetics.  Is less QA-noise worth the ugly ?

TODO: The "dunno" case shouldn't really catch the <##_model>, since
there's a subsequent rule for that.  I could have hidden this blemish
by moving the other rules up, but that also hides a maybe over-broad
stripping regex, and is -E_TOO_MANY_CHANGES.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

fix
---
 include/linux/compiler.h | 12 +++++++++++
 scripts/checkpatch.pl    | 46 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 64ff73c533e5..a348b45eacc0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -379,6 +379,18 @@ static inline void *offset_to_ptr(const int *off)
  */
 #define prevent_tail_call_optimization()	mb()
 
+/*
+ * tell checkpatch --strict that you know the named args (a subset of
+ * the containing macro's args) are safe for multiple expansions.
+ *
+ * Prefer ({ typeof ..}) or do{}while(0) when they work.  They would
+ * not work on module_param_named(name, value, type, perm), or on a
+ * locally useful "for_simplicity()" macro.
+ *
+ * NB: use at top of macro body, omit trailing semicolon.
+ */
+#define __no_side_effects(...)  /* checkpatch "annotation" helper */
+
 #include <asm/rwonce.h>
 
 #endif /* __LINUX_COMPILER_H */
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa8ef..27299f326804 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -169,6 +169,30 @@ my $DO_WHILE_0_ADVICE = q{
    Enjoy this qualification while we work to improve our heuristics.
 };
 
+# call this from s/$patt/drx_print("why")/e - to see whats happening there.
+my $drx_print = 0;
+sub drx_print {
+    return "" unless $drx_print;
+
+    my ($why) = @_;
+    # The magic regex variables are available here.
+    # $& contains the entire matched string.
+    # $1, $2, etc. contain captured groups.
+    print "drx_print: $why\n";
+    print "  >> Matched (`\$&`): <$&>\n";
+
+    # Only print captures if they exist
+    if (defined $1) {
+	print "  >> Capture 1 (`\$1`): <$1>\n";
+    }
+    if (defined $2) {
+	print "  >> Capture 2 (`\$2`): <$2>\n";
+    }
+    # The subroutine must return the replacement string.
+    # For stripping, this is an empty string.
+    return "";
+}
+
 sub uniq {
 	my %seen;
 	return grep { !$seen{$_}++ } @_;
@@ -348,6 +372,7 @@ GetOptions(
 	'no-color'	=> \$color,	#keep old behaviors of -nocolor
 	'nocolor'	=> \$color,	#keep old behaviors of -nocolor
 	'kconfig-prefix=s'	=> \${CONFIG_},
+	'drx'		=> \$drx_print,
 	'h|help'	=> \$help,
 	'version'	=> \$help
 ) or $help = 2;
@@ -6044,11 +6069,24 @@ sub process {
 			        next if ($arg =~ /\.\.\./);
 			        next if ($arg =~ /^type$/i);
 				my $tmp_stmt = $define_stmt;
-				$tmp_stmt =~ s/\b(__must_be_array|offsetof|sizeof|sizeof_field|__stringify|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
-				$tmp_stmt =~ s/\#+\s*$arg\b//g;
-				$tmp_stmt =~ s/\b$arg\s*\#\#//g;
+
+				$tmp_stmt =~ s{
+					\b(__must_be_array|offsetof|sizeof|sizeof_field|__stringify|
+					   typeof|__typeof__|__builtin\w+|typecheck
+					   \s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b }
+				{
+					drx_print("dunno");
+				}xge;
+
+				$tmp_stmt =~ s/\#+\s*$arg\b/drx_print("strip '#|## arg catenations")/ge;
+				$tmp_stmt =~ s/\b$arg\s*\#\#/drx_print("strip 'arg ##' catenations");/ge;
+
+				my $no_side_effect_vars = "";
+				if ($tmp_stmt =~ s/__no_side_effects\((.+)\)//) {
+					$no_side_effect_vars = $1;
+				}
 				my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
-				if ($use_cnt > 1) {
+				if ($use_cnt > 1 and $no_side_effect_vars !~ m/\b$arg\b/) {
 					CHK("MACRO_ARG_REUSE",
 					    "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx");
 				    }
-- 
2.51.0

[RFC PATCH 2/2] checkpatch: cosmetic-style tweak

[Jim Cromie]

no functional change, slightly narrower on-screen, maybe clearer ?

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 27299f326804..4449e809ef6f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4019,11 +4019,13 @@ sub process {
 				if ($newindent ne $goodtabindent &&
 				    $newindent ne $goodspaceindent) {
 
-					if (CHK("PARENTHESIS_ALIGNMENT",
-						"Alignment should match open parenthesis\n" . $hereprev) &&
-					    $fix && $line =~ /^\+/) {
+					CHK("PARENTHESIS_ALIGNMENT",
+					    "Alignment should match open parenthesis\n" . $hereprev)
+					and do {
+					    if ($fix && $line =~ /^\+/) {
 						$fixed[$fixlinenr] =~
 						    s/^\+[ \t]*/\+$goodtabindent/;
+					    }
 					}
 				}
 			}
-- 
2.51.0

Re: [RFC PATCH 2/2] checkpatch: cosmetic-style tweak

[Joe Perches]

On Wed, 2025-10-15 at 11:51 -0600, Jim Cromie wrote:
> no functional change, slightly narrower on-screen, maybe clearer ?
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4019,11 +4019,13 @@ sub process {
>  				if ($newindent ne $goodtabindent &&
>  				    $newindent ne $goodspaceindent) {
>  
> -					if (CHK("PARENTHESIS_ALIGNMENT",
> -						"Alignment should match open parenthesis\n" . $hereprev) &&
> -					    $fix && $line =~ /^\+/) {
> +					CHK("PARENTHESIS_ALIGNMENT",
> +					    "Alignment should match open parenthesis\n" . $hereprev)
> +					and do {
> +					    if ($fix && $line =~ /^\+/) {
>  						$fixed[$fixlinenr] =~
>  						    s/^\+[ \t]*/\+$goodtabindent/;
> +					    }

nack.

"and do" is not used anywhere by checkpatch
and I don't want to introduce it.