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
> + my $params = $1 || "";
> + my $body = $2 || "";
> +
> + # get the individual params
> + $params =~ tr/()//d;
> + # remove leading and trailing whitespace
> + $params =~ s/^\s+|\s+$//g;
trim
