linux-kernel - Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for function-like macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <MN0PR20MB47171D2D78C8E71286E133BCF3382@MN0PR20MB4717.namprd20.prod.outlook.com>
Date: Sun, 31 Mar 2024 13:43:33 +0000
From: Mac Xu <mac.xxn@...look.com>
To: Joe Perches <joe@...ches.com>, Barry Song <21cnbao@...il.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
CC: "apw@...onical.com" <apw@...onical.com>, "broonie@...nel.org"
	<broonie@...nel.org>, "chenhuacai@...ngson.cn" <chenhuacai@...ngson.cn>,
	"chris@...kel.net" <chris@...kel.net>, "corbet@....net" <corbet@....net>,
	"dwaipayanray1@...il.com" <dwaipayanray1@...il.com>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux@...ck-us.net" <linux@...ck-us.net>, "lukas.bulwahn@...il.com"
	<lukas.bulwahn@...il.com>, "sfr@...b.auug.org.au" <sfr@...b.auug.org.au>,
	"v-songbaohua@...o.com" <v-songbaohua@...o.com>, "workflows@...r.kernel.org"
	<workflows@...r.kernel.org>, Max Filippov <jcmvbkbc@...il.com>
Subject: Re: [PATCH v4 2/2] scripts: checkpatch: check unused parameters for
 function-like macro


> On Thu, 2024-03-28 at 15:21 +1300, Barry Song wrote:
> > From: Xining Xu <mac.xxn@...look.com>
> >
> > If function-like macros do not utilize a parameter, it might result in a
> > build warning.  In our coding style guidelines, we advocate for utilizing
> > static inline functions to replace such macros.  This patch verifies
> > compliance with the new rule.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6109,6 +6109,36 @@ sub process {
> >                                WARN("TRAILING_SEMICOLON",
> >                                     "macros should not use a trailing semicolon\n" . "$herectx");
> >                        }
> > +
> > +                     # match "\s*" rather than "\s+" after the balanced parens, as macro definition with arguments
> > +                     # is not required to have whitespace after arguments
> > +                     if ($dstat =~ /^\+\s*#\s*define\s+$Ident$balanced_parens\s*(\S+.*)(\/[\/*].*)?/) {
> 
> I think '(\/[\/*].*)?' doesn't do what you expect
> perhaps '(\/[\/\*].*)?'
> though I don't know why this should be capture group

I'd wanted to capture the comment to handle a case where a unused param happens to appears in a comment

> 
> > +                             my $params = $1 || "";
> 
> 
> > +                             my $body = $2 || "";
> 
> Should never get the || "" as the 2nd capture group is not optional
> 
> > +
> > +                         # get the individual params
> > +                             $params =~ tr/()//d;
> > +                             # remove leading and trailing whitespace
> > +                             $params =~ s/^\s+|\s+$//g;
> > +
> > +                             $ctx =~ s/\n*$//;
> > +                             my $cnt = statement_rawlines($ctx);
> > +                             my $herectx = get_stat_here($linenr, $cnt, $here);
> > +
> > +                             if ($params ne "") {
> 
> probably unnecessary
> 
> > +                                     my @paramList = split /,\s*/, $params;
> 
> please use split() with parentheses
> 
> > +                                     foreach my $param(@paramList) {
> 
> maybe
>                                 foreach my $param (split(/,/, $params) {
>                                         $param = trim($param);
>                                         next if ($param =~ /\.\.\.$/);
> > +                                             if ($param =~ /\.\.\.$/) {
> > +                                                     # if the param name ends with "...", skip the check
> > +                                                     next;
> > +                                             }
> > +                                             if ($body !~ /\b$param\b/) {
> > +                                                     WARN("UNUSED_PARAM_IN_MACRO",
> > +                                                             "Parameter '$param' is not used in function-like macro\n" . "$herectx");
> > +                                             }
> > +                                     }
> It seems this logic is a bit redundant to existing
> code and might be better added in the block that starts
> 
> (line 6026)
> # check if any macro arguments are reused (ignore '...' and 'type')
> 
> as that already does each param in a #define and
> ignores ... and type

Hi Joe,

Thank you for your comments with insights, as you said, code block of line 6026 is a better place to 
place this new logic, as it already handles the logic I'd wanted like extracting, splitting and trimming 
the arguments, excluding the trailing comments etc.

By placing the logic in the new place, code duplicates are drastically reduced.

Here's my new code (inserted from line 6044):
+# check if this is an unused argument
+        if ($define_stmt !~ /\b$arg\b/) {
+                WARN("UNUSED_ARG_IN_MACRO",
+                        "Argument '$arg' is not used in function-like macro\n" . "$herectx");
+        }
+}

Please note that I use the wording of "arg/argument" instead of "param/parameter" for consistency, 
please let me know if if this is the correct wording to use here.

Thanks,
Mac.