[PATCH v2 0/2] checkpatch --debug rx=1 facility

[PATCH v2 0/2] checkpatch --debug rx=1 facility

[Jim Cromie]

checkpatch uses a lot of s/// and s///g heuristics to cleanup
code-chunks, before inspecting for problems with more heurisitics.

add a drx_print("reason") helper, for use like:

  s/$patt//;	-> s/$patt/drx_print("does this")/e;
  s/$patt//g;	-> s/$patt/drx_print("does that")/ge;

(note the 'e' modifier)

To activate, pass --debug rx=1 or --debug rx="this"

Jim Cromie (2):
  checkpatch: add --debug rx=1|foo option and drx_print() helper
  checkpatch: 3 use-cases for --debug rx=1 option

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

-- 
2.51.0

[PATCH v2 1/2] checkpatch: add --debug rx=1|foo 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.

To help with maintenance of those harder "cleanup" cases, add a helper
fn: drx_print($reason), which is designed to be called from a s/// or
s///g statement (in the 'replacement' side), to "explain" itself.

You can use it to instrument the code to show its work, then validate
that explanation by experiment and exersize:

  s/$patt/drx_print("why")/e;		# maintainer's best guess
  s/$patt/drx_print("whys")/ge;		# note the 'e' modifier

To activate drx_print() output, pass "--debug rx=1" to enable all the
instrumented cleanup heuristics.  For more selectivity (in case usage
grows), pass: "--debug rx=foo" to select "foo" cleanups.

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 --debug rx=inspect ../linux.git/pt-1
drx_print: -arg-inspections-
  >> Matched (`$&`): <__builtin_constant_p(cls>
  >> Capture 1 (`$1`): <__builtin_constant_p>

Also validate --debug KEYs for clear error:

$ scripts/checkpatch.pl --strict ../linux.git/pt-1 --debug foo=1
Unknown debug key 'foo', expecting: 'values possible type attr rx'

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
v2 - extend --debug key=1 rather than add new option
---
 scripts/checkpatch.pl | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa8ef..c174e3bef2b2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -169,6 +169,34 @@ my $DO_WHILE_0_ADVICE = q{
    Enjoy this qualification while we work to improve our heuristics.
 };
 
+my $dbg_rx = 0;
+# call this from s/$patt/drx_print("why")/e - to see whats happening there.
+sub drx_print {
+	my ($reason) = @_;
+	return "" unless $dbg_rx;
+
+	if ($dbg_rx ne '1') {
+	    # $dbg_rx is seeking "reason"
+	    # search w/o using regex, to preserve caller s///e context.
+	    return "" if ($dbg_rx and index($reason, $dbg_rx) == -1);
+	}
+
+	# report what was matched and removed (in caller)
+	print "drx_print: $reason\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{$_}++ } @_;
@@ -451,7 +479,13 @@ my $dbg_values = 0;
 my $dbg_possible = 0;
 my $dbg_type = 0;
 my $dbg_attr = 0;
+
+my @known_keys = qw(values possible type attr rx);
+my %known_keys;
+$known_keys{$_}++ for @known_keys;
+
 for my $key (keys %debug) {
+	die "Unknown debug key '$key', expecting: '@known_keys'\n" unless $known_keys{$key};
 	## no critic
 	eval "\${dbg_$key} = '$debug{$key}';";
 	die "$@" if ($@);
-- 
2.51.0

[PATCH v2 2/2] checkpatch: 3 use-cases for --debug rx=1 option

[Jim Cromie]

Use the drx_print() helper in 3 cases inside code which counts macro
arg expansions.

$ scripts/checkpatch.pl --strict patch-1 --debug rx='##'
drx_print: 'arg ##' catenations
  >> Matched (`$&`): <_id##>
drx_print: 'arg ##' catenations
  >> Matched (`$&`): <_id##>
drx_print: '#|## arg' catenations
  >> Matched (`$&`): <##_model>
drx_print: '#|## arg' catenations
  >> Matched (`$&`): <##_model>

$ scripts/checkpatch.pl --strict patch-1 --debug rx='insp'
drx_print: -arg-inspections-
  >> Matched (`$&`): <__builtin_constant_p(cls>
  >> Capture 1 (`$1`): <__builtin_constant_p>

NB: see also the extended --debug key=1 facility:

$ scripts/checkpatch.pl --strict ../linux.git/pt-1 --debug foo=1
Unknown debug key 'foo', expecting: 'values possible type attr rx'

NB: I moved the 2 #|## strippers above the more complex macro, because
the latter caught one ## case that it needn't have.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c174e3bef2b2..240ddab17d89 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6078,9 +6078,17 @@ 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("'#|## arg' catenations")/ge;
+				$tmp_stmt =~ s/\b$arg\s*\#\#/drx_print("'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("-arg-inspections-");
+				}xge;
+
 				my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
 				if ($use_cnt > 1) {
 					CHK("MACRO_ARG_REUSE",
-- 
2.51.0

Re: [PATCH v2 2/2] checkpatch: 3 use-cases for --debug rx=1 option

[Joe Perches]

On Sun, 2025-10-26 at 14:21 -0600, Jim Cromie wrote:
> Use the drx_print() helper in 3 cases inside code which counts macro
> arg expansions.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6078,9 +6078,17 @@ 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("'#|## arg' catenations")/ge;
> +				$tmp_stmt =~ s/\b$arg\s*\#\#/drx_print("'arg ##' catenations");/ge;

stray trailing ; in the replacement ?

> +				$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 }

This might be easier to read using a qr but I'm not sure the
embedded capture groups and their use in drx_print is sensible
as it doesn't seem extensible.

our $stmt_stripper = qr{\b(
		__must_be_array |
		offsetof | typeof | __typeof__ |
		sizeof | sizeof_field |
		__builtin\w+
		typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)\(*\s*$arg\s*\)*
		
> +				{
> +					drx_print("-arg-inspections-");
> +				}xge;
> +
>  				my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
>  				if ($use_cnt > 1) {
>  					CHK("MACRO_ARG_REUSE",

Back with I suggested this a dozen years ago I thought it was overkill.
Maybe it is and the whole test should be offed.

https://lore.kernel.org/lkml/1352198139.16194.21.camel@joe-AO722/

Re: [PATCH v2 2/2] checkpatch: 3 use-cases for --debug rx=1 option

[jim.cromie]

On Sun, Oct 26, 2025 at 5:40 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2025-10-26 at 14:21 -0600, Jim Cromie wrote:
> > Use the drx_print() helper in 3 cases inside code which counts macro
> > arg expansions.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6078,9 +6078,17 @@ 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("'#|## arg' catenations")/ge;
> > +                             $tmp_stmt =~ s/\b$arg\s*\#\#/drx_print("'arg ##' catenations");/ge;
>
> stray trailing ; in the replacement ?
>
> > +                             $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 }
>
> This might be easier to read using a qr but I'm not sure the
> embedded capture groups and their use in drx_print is sensible
> as it doesn't seem extensible.
>

yes, the extra whitespace is better.
I will play with qr// see if the captures work the same.

> our $stmt_stripper = qr{\b(
>                 __must_be_array |
>                 offsetof | typeof | __typeof__ |
>                 sizeof | sizeof_field |
>                 __builtin\w+
>                 typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)\(*\s*$arg\s*\)*
>
> > +                             {
> > +                                     drx_print("-arg-inspections-");
> > +                             }xge;
> > +
> >                               my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
> >                               if ($use_cnt > 1) {
> >                                       CHK("MACRO_ARG_REUSE",
>
> Back with I suggested this a dozen years ago I thought it was overkill.
> Maybe it is and the whole test should be offed.
>

I am now playing with an __lvalue(x) macro,
based upon  __must_be_array(x),
it is a compile-time check, so it gives a high-quality signal to checkpatch
if x is an lval, the multiple expansion warnings can be silenced for x.

So if this works out, we could take off the wart, rather than the finger.

> https://lore.kernel.org/lkml/1352198139.16194.21.camel@joe-AO722/