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
