> From: Joe Perches <joe@xxxxxxxxxxx>
> Sent: Monday, March 25, 2024 10:16 AM
> To: Mac Xu; Barry Song
> Cc: akpm@xxxxxxxxxxxxxxxxxxxx; apw@xxxxxxxxxxxxx; broonie@xxxxxxxxxx; chenhuacai@xxxxxxxxxxx; chris@xxxxxxxxxx; corbet@xxxxxxx; dwaipayanray1@xxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; jcmvbkbc@xxxxxxxxx; linux@xxxxxxxxxxxx; lukas.bulwahn@xxxxxxxxx; mm-commits@xxxxxxxxxxxxxxx; sfr@xxxxxxxxxxxxxxxx; v-songbaohua@xxxxxxxx
> Subject: Re: + scripts-checkpatch-check-unused-parameters-for-function-like-macro.patch added to mm-nonmm-unstable branch
>
> On Mon, 2024-03-25 at 09:43 +0000, Mac Xu wrote:
> > > On Sun, Mar 24, 2024 at 11:59 AM Joe Perches <joe@xxxxxxxxxxx> wrote:
> > > >
> > > > On Sun, 2024-03-24 at 11:40 +1300, Barry Song wrote:
> > > > > On Sat, Mar 23, 2024 at 7:16 PM Joe Perches <joe@xxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Sat, 2024-03-23 at 17:14 +1300, Barry Song wrote:
> > > > > > > > > The patch titled
> > > > > > > > > Subject: scripts: checkpatch: check unused parameters for function-like macro
> > > > > > > > > has been added to the -mm mm-nonmm-unstable branch. Its filename is
> > > > > > > > > scripts-checkpatch-check-unused-parameters-for-function-like-macro.patch
> > > > > > > >
> > > > > > > > Conceptually I don't disagree but I wonder what the false
> > > > > > > > positive rate is.
> > > > > > >
> > > > > > > Hi Joe,
> > > > > > >
> > > > > > > If your concern is whether this script might produce inaccurate reports, I'm confident that
> > > > > > > the false positive rate is 0%. Xining, please correct me if I'm mistaken.
> > > > > >
> > > > > > I presume it's low but non-zero.
> > > > > >
> > > > > > I think the false positives are going to be
> > > > > > in some cases where a type is used as a macro
> > > > > > argument in #ifdef/#else blocks
> > > > > >
> > > > > > #ifdef CONFIG_<FOO>
> > > > > >
> > > > > > #define foo(bar, baz, ...) \
> > > > > > static inline func##bar(baz, ...) \
> > > > > > { \
> > > > > > bar baz = ... \
> > > > > > ... \
> > > > > > }
> > > > > >
> > > > > > #else
> > > > > >
> > > > > > #define foo(bar, baz, ...)
> > > > > >
> > > > > > #endif
> > > > > >
> > > > > > where it's not reasonably possible to use a
> > > > > > static inline.
> > > > >
> > > > > I do find some exceptions such as include/uapi/linux/const.h
> > > > >
> > > > > #ifdef __ASSEMBLY__
> > > > > #define _AC(X,Y) X
> > > > > #define _AT(T,X) X
> > > > > #else
> > > > > #define __AC(X,Y) (X##Y)
> > > > > #define _AC(X,Y) __AC(X,Y)
> > > > > #define _AT(T,X) ((T)(X))
> > > > > #endif
> > > > []
> > > > > I'll check if Xining has some approach to detect this kind of ifdef/else/endif,
> > > > > and remove those false positives.
> > > >
> > > > checkpatch isn't expected to be perfect.
> > > >
> > > > So I wouldn't worry about it, I just wondered if
> > > > the exception rate is near the warning rate.
> > > >
> > > > I expect it's not, but if it is, then perhaps
> > > > the warning isn't useful.
> > > >
> > > > Do you know how many existing cases there are in
> > > > the kernel that might trip this warning?
> > > I can find numerous cases, lots of func-like macros don't evaluate
> > > their arguments.
> > > They potentially might lead to build warnings.
> > >
> > > for example,
> > >
> > > WARNING: Parameter 'val' is not used in function-like macro #332:
> > > FILE: arch/arm64/include/asm/fpsimd.h:332:
> > > +#define sve_cond_update_zcr_vq(val, reg) do { } while (0)
> > >
> > > WARNING: Parameter 'reg' is not used in function-like macro #332:
> > > FILE: arch/arm64/include/asm/fpsimd.h:332:
> > > +#define sve_cond_update_zcr_vq(val, reg) do { } while (0)
> > >
> > > WARNING: Parameter 'p' is not used in function-like macro #21: FILE:
> > > arch/arm64/include/asm/kprobes.h:21:
> > > +#define flush_insn_slot(p) do { } while (0)
> > >
> > > WARNING: Parameter 'vcpu' is not used in function-like macro #875:
> > > FILE: arch/arm64/include/asm/kvm_host.h:875:
> > > +#define vcpu_has_ptrauth(vcpu) false
> > >
> > > WARNING: Parameter 'ex' is not used in function-like macro #120: FILE:
> > > arch/arm64/include/asm/elf.h:120:
> > > +#define compat_elf_read_implies_exec(ex, stk) (stk == EXSTACK_DEFAULT)
> > >
> > > WARNING: Parameter 'ex' is not used in function-like macro #161: FILE:
> > > arch/arm64/include/asm/elf.h:161:
> > > +#define SET_PERSONALITY(ex) \
> > > +({ \
> > > + clear_thread_flag(TIF_32BIT); \
> > > + current->personality &= ~READ_IMPLIES_EXEC; \
> > > +})
> > >
> > > WARNING: Parameter 'start' is not used in function-like macro #121:
> > > FILE: arch/xtensa/include/asm/cacheflush.h:121:
> > > +#define flush_cache_vunmap(start,end) flush_cache_all()
> > >
> > >
> > > WARNING: Parameter 'end' is not used in function-like macro #121:
> > > FILE: arch/xtensa/include/asm/cacheflush.h:121:
> > > +#define flush_cache_vunmap(start,end) flush_cache_all()
> > >
> > >
> > > WARNING: Parameter 'mm' is not used in function-like macro #140: FILE:
> > > arch/xtensa/include/asm/cacheflush.h:140:
> > > +#define flush_cache_mm(mm) do { } while (0)
> > >
> > >
> > > WARNING: Parameter 'mm' is not used in function-like macro #141: FILE:
> > > arch/xtensa/include/asm/cacheflush.h:141:
> > > +#define flush_cache_dup_mm(mm) do { } while (0)
> > >
> > > ...
> > >
> > > And I do find some funny false positive:
> > >
> > > WARNING: Parameter '...' is not used in function-like macro #1090:
> > > FILE: arch/arm64/include/asm/kvm_host.h:1090:
> > > +#define kvm_call_hyp(f, ...) f(__VA_ARGS__)
> > >
> > > ...
> > >
> > > I think we at least need to fix this '...' case.
> > >
> > > And since sometimes we are reporting false positives, do we consider
> > > downgrading it to WARN from ERROR?
> > >
> > > Thanks
> > > Barry
> >
> > Hi Joe, Barry,
> >
> > I've fixed the false alarm case with "..." as suggested by Barry.
> >
> > Given the fact that this script named as "checkpatch", and a patch is about
> > the diff's rather than the full source code, this indicates that we are checking
> > with limited information, which is likely to introduce false alarms and
> > false positives, so, I'd suggest that we downgrade the ERROR to WARN.
> >
> > the changes in checkpatch.pl are as below:
> > +if ($dstat =~ /^\+\s*#\s*define\s+$Ident\s*(\((?:[^\(\)]++|(?-1))*\))\s+(\S+.*)(\/\/.*)?/) {
>
> Remove the "\s*" after $Ident otherwise this will
> match on #define foo (cast) (value) like:
>
> drivers/acpi/acpica/aclocal.h:#define ACPI_GLOBAL_LOCK (acpi_semaphore) (-1)
>
> Why the // comment match and not a comment starting with /* ?
>
> And you should use $balanced_parens
>
> $Ident$balanced_parens\s*
>
> as the macro definition with arguments is not required
> to have whitespace after arguments
Hi Joe,
Thank you for your comments, Here're my changes per your suggestion:
1. use $balanced_parens rather than raw regular expression statement to catch arguments
2. match optional trailing comment by either // or /*
3. use \s* rather than \s+ after argument section, as whitespace is not required for
macro definition with arguments
Here's the updated code:
+# 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+.*)(\/[\/*].*)?/) {
+ my $params = $1 || "";
+ my $body = $2 || "";
+
+ # 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 "") {
+ my @paramList = split /,\s*/, $params;
+ foreach my $param(@paramList) {
+ 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");
+ }
+ }
+ }
+}
Thanks,
Xining
