* [RFC PATCH 1/2] checkpatch: add __no_side_effects() hint/assertion macro
2025-10-15 17:51 [RFC PATCH 0/2] practical auto-QA de-noiser or -E_TOO_UGLY ? Jim Cromie
@ 2025-10-15 17:51 ` Jim Cromie
2025-10-15 17:51 ` [RFC PATCH 2/2] checkpatch: cosmetic-style tweak Jim Cromie
1 sibling, 0 replies; 4+ messages in thread
From: Jim Cromie @ 2025-10-15 17:51 UTC (permalink / raw)
To: linux-kernel; +Cc: 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, ¶m_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
^ permalink raw reply [flat|nested] 4+ messages in thread