> 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+.*)(\/\/.*)?/) {
+ 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
