[PATCH 0/3] checkpatch --strict | ci-pipeline improvements

[PATCH 0/3] checkpatch --strict | ci-pipeline improvements

[Jim Cromie]

Several CI-pipelines call to checkpatch.pl --strict, so CHECK reports
there can disrupt pipeline flow.  This patchset addresses one such check.

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

Multi-statement declarative macros (both top-of-scope and file-scope),
commonly fall victim to this warning, checkpatch even has a list of
named exceptions to silence some of the checks.

This patchset provides a more focussed approach:

1 #define __chkp_no_side_effects(...)   /* nothing */
  so no functional change due to its use.

2 teach checkpatch how to silence THAT WARNING on that _var
  find the macro call in macro-body, extract no-side-effect-vars
  process the macro body as usual, count arg expansions
  before issuing CHECK, check the no-side-effect-vars

3 authors add the assertion to their noisy macros
  active assertion of safety, per variable, not full pardon by name
  indicates they tested against --strict

Jim Cromie (3):
  checkpatch: add --drx option and drx_print() helper
  checkpatch: recognize __chkp_no_side_effects(_var) hint macro
  compiler.h: add __chkp_no_side_effects() empty hint/assertion macro

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

-- 
2.51.0

[PATCH 1/3] checkpatch: add --drx option and drx_print() helper

[Jim Cromie]

checkpatch has ~235 heuristic s/$patt// statements which strip
code-snippets that are "OK", leaving the remainder for further
heuristics to apply further "cleanups".

Many of these have obvious purpose, but surely some are inscrutable.
For those cases, add drx_print($reason) helper, which is designed to
be called from a s/// or s///g statement (in the 'replacement' side),
to "explain" what it is doing.

You can use it to instrument the code to "explain" itself, then
validate that explanation by experiment and exersize:

  s/$patt/drx_print("why")/e;
  s/$patt/drx_print("whys")/ge;

To activate the "debug" output, pass --drx or --drx=why to enable all
(or just matching) cleanup heuristics to validate the "whys".

Here it is in action, on a patch which triggered enough noise that I
wanted this visibility into what it was doing.

$ scripts/checkpatch.pl --strict --drx=builtins ../linux.git/pt-1
drx_print: -builtins-
  >> Matched (`$&`): <__builtin_constant_p(cls>
  >> Capture 1 (`$1`): <__builtin_constant_p>

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

fixup
---
 scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa8ef..2b0275dcc5a4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -74,6 +74,7 @@ my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANC
 my $git_command ='export LANGUAGE=en_US.UTF-8; git';
 my $tabsize = 8;
 my ${CONFIG_} = "CONFIG_";
+my $drx; # enable s/$patt/drx_print("reason")/e; debugging
 
 my %maybe_linker_symbol; # for externs in c exceptions, when seen in *vmlinux.lds.h
 
@@ -169,6 +170,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.
+sub drx_print {
+	my ($why) = @_;
+	return "" unless defined $drx;		# --drx or --drx=
+
+	# avoid regex test to preserve caller's match, captures
+	return "" if (defined $drx && $drx ne '' && index($why, $drx) == -1);
+
+	# report what was matched and removed
+	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 s/$pat//
+	# statements (our target use), this is an empty string.
+	return "";
+}
+
 sub uniq {
 	my %seen;
 	return grep { !$seen{$_}++ } @_;
@@ -348,6 +373,7 @@ GetOptions(
 	'no-color'	=> \$color,	#keep old behaviors of -nocolor
 	'nocolor'	=> \$color,	#keep old behaviors of -nocolor
 	'kconfig-prefix=s'	=> \${CONFIG_},
+	'drx:s'		=> \$drx,
 	'h|help'	=> \$help,
 	'version'	=> \$help
 ) or $help = 2;
-- 
2.51.0

[PATCH 2/3] checkpatch: recognize __chkp_no_side_effects(_var) hint macro

[Jim Cromie]

Teach checkpatch --strict to allow an author to suppress specific
var-reuse complaints:

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

Consider:

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

Without the +line, this kind of multi-statement macro-defn would cause
checkpatch to carp about name and value being reused.

Note that it wouldn't complain about type, even though type is
expanded 3 times, because checkpatch knew to strip # and ## constructs
on an arg before counting its uses.

module_param_named is actually a named $exception, amongst a ~dozen
others, so it wouldn't draw the complaint (I said kind of above), but
this approach doesn't require further checkpatch tweaks, is specific
to the _vars named, and is author initiated, not a carte-blanche
exception on the macro-name.

IIRC, trybot kicks of a CI pipeline that calls checkpatch --strict,
this hint could help reduce CI-noise going forward.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b0275dcc5a4..bc807c20fcde 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6070,11 +6070,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/\#+\s*$arg\b/drx_print("strip '#|## arg catenations")/ge;
+				$tmp_stmt =~ s/\b$arg\s*\#\#/drx_print("strip 'arg ##' catenations");/ge;
+
+				$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("-builtins-");
+				}xge;
+
+				my $no_side_effect_vars = "";
+				if ($tmp_stmt =~ s/__chkp_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

[PATCH 3/3] compiler.h: add __chkp_no_side_effects() empty hint/assertion macro

[Jim Cromie]

cpp has no intrinsic protection against macro arg side-effects, so to
compensate, checkpatch is paranoid:

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

Allow an author to suppress these warnings on _var by adding
'__chkp_no_side_effects(_var)' to the body of macros which trigger
that warning.  This may reduce blowouts in CI pipelines.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/compiler.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 64ff73c533e5..96f6bfbd4088 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 __chkp_no_side_effects(...)  /* checkpatch "annotation" helper */
+
 #include <asm/rwonce.h>
 
 #endif /* __LINUX_COMPILER_H */
-- 
2.51.0

Re: [PATCH 1/3] checkpatch: add --drx option and drx_print() helper

[Joe Perches]

On Sat, 2025-10-25 at 15:15 -0600, Jim Cromie wrote:
> checkpatch has ~235 heuristic s/$patt// statements which strip
> code-snippets that are "OK", leaving the remainder for further
> heuristics to apply further "cleanups".
> 
> Many of these have obvious purpose, but surely some are inscrutable.
> For those cases, add drx_print($reason) helper, which is designed to
> be called from a s/// or s///g statement (in the 'replacement' side),
> to "explain" what it is doing.
> 
> You can use it to instrument the code to "explain" itself, then
> validate that explanation by experiment and exersize:
> 
>   s/$patt/drx_print("why")/e;
>   s/$patt/drx_print("whys")/ge;
> 
> To activate the "debug" output, pass --drx or --drx=why to enable all
> (or just matching) cleanup heuristics to validate the "whys".
> 
> Here it is in action, on a patch which triggered enough noise that I
> wanted this visibility into what it was doing.
> 
> $ scripts/checkpatch.pl --strict --drx=builtins ../linux.git/pt-1
> drx_print: -builtins-
>   >> Matched (`$&`): <__builtin_constant_p(cls>
>   >> Capture 1 (`$1`): <__builtin_constant_p>

I'd prefer you extend the existing --debug KEY=[0|1] mechanism.

Re: [PATCH 3/3] compiler.h: add __chkp_no_side_effects() empty hint/assertion macro

[Linus Torvalds]

On Sat, 25 Oct 2025 at 14:15, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> Allow an author to suppress these warnings on _var by adding
> '__chkp_no_side_effects(_var)' to the body of macros which trigger
> that warning.  This may reduce blowouts in CI pipelines.

How about just not doing that checkpatch thing at all if it causes problems?

Seriously, I think checkpatch often causes more problems than it
fixes. If it then causes us to write even uglier macros - and it's not
like our macros are pretty in general - it really is just causing
pain.

I think we should cut down checkpatch to things that are obvious and
real issues. Not things that then cause people to make code worse.

                 Linus

Re: [PATCH 2/3] checkpatch: recognize __chkp_no_side_effects(_var) hint macro

[Joe Perches]

On Sat, 2025-10-25 at 15:15 -0600, Jim Cromie wrote:
> Teach checkpatch --strict to allow an author to suppress specific
> var-reuse complaints:

It seems to me that annotating source code to silence checkpatch
isn't a great idea.

If it is a great idea, maybe a general CHECKPATCH_ON/OFF(type)
mechanism might be better than specific controls.