| Message ID | 20190827103621.1073-1-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] kbuild: enable unused-function warnings for W= build with Clang | expand |
On Tue, Aug 27, 2019 at 07:36:21PM +0900, Masahiro Yamada wrote: > GCC and Clang have different policy for -Wunused-function; GCC never > reports unused-function warnings for 'static inline' functions whereas > Clang reports them if they are defined in source files instead of > included headers although it has been suppressed since commit > abb2ea7dfd82 ("compiler, clang: suppress warning for unused static > inline functions"). > > We often miss to remove unused functions where 'static inline' is used > in .c files since there is no tool to detect them. Unused code remains > until somebody notices. For example, commit 075ddd75680f ("regulator: > core: remove unused rdev_get_supply()"). > > Let's remove __maybe_unused from the inline macro to allow Clang to > start finding unused static inline functions. As always, it is not a > good idea to sprinkle warnings for the normal build, so I added > -Wno-unsued-function for no W= build. > > Per the documentation [1], -Wno-unused-function will also disable > -Wunneeded-internal-declaration, which can help find bugs like > commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use > mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) > I added -Wunneeded-internal-declaration to address it. > > If you contribute to code clean-up, please run "make CC=clang W=1" > and check -Wunused-function warnings. You will find lots of unused > functions. > > Some of them are false-positives because the call-sites are disabled > by #ifdef. I do not like to abuse the inline keyword for suppressing > unused-function warnings because it is intended to be a hint for the > compiler's optimization. I prefer __maybe_unused or #ifdef around the > definition. > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Reviewed-by: Kees Cook <keescook@chromium.org> I am still not a big fan of this as I think it weakens clang as a standalone compiler but the change itself looks good so if it is going in anyways: Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> I'm sure Nick would like to weigh in as well before this gets merged.
On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Tue, Aug 27, 2019 at 07:36:21PM +0900, Masahiro Yamada wrote: > > GCC and Clang have different policy for -Wunused-function; GCC never > > reports unused-function warnings for 'static inline' functions whereas > > Clang reports them if they are defined in source files instead of > > included headers although it has been suppressed since commit > > abb2ea7dfd82 ("compiler, clang: suppress warning for unused static > > inline functions"). > > > > We often miss to remove unused functions where 'static inline' is used > > in .c files since there is no tool to detect them. Unused code remains > > until somebody notices. For example, commit 075ddd75680f ("regulator: > > core: remove unused rdev_get_supply()"). > > > > Let's remove __maybe_unused from the inline macro to allow Clang to > > start finding unused static inline functions. As always, it is not a > > good idea to sprinkle warnings for the normal build, so I added > > -Wno-unsued-function for no W= build. s/unsued/unused/ > > > > Per the documentation [1], -Wno-unused-function will also disable > > -Wunneeded-internal-declaration, which can help find bugs like > > commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use > > mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) > > I added -Wunneeded-internal-declaration to address it. > > > > If you contribute to code clean-up, please run "make CC=clang W=1" > > and check -Wunused-function warnings. You will find lots of unused > > functions. > > > > Some of them are false-positives because the call-sites are disabled > > by #ifdef. I do not like to abuse the inline keyword for suppressing > > unused-function warnings because it is intended to be a hint for the > > compiler's optimization. I prefer __maybe_unused or #ifdef around the > > definition. I'd say __maybe_unused for function parameters that are used depending of ifdefs in the body of the function, otherwise strictly ifdefs. > > > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > I am still not a big fan of this as I think it weakens clang as a > standalone compiler but the change itself looks good so if it is going > in anyways: > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > I'm sure Nick would like to weigh in as well before this gets merged. So right away for an x86_64 defconfig w/ this patch, clang points out: drivers/gpu/drm/i915/i915_sw_fence.c:84:20: warning: unused function 'debug_fence_init_onstack' [-Wunused-function] static inline void debug_fence_init_onstack(struct i915_sw_fence *fence) ^ drivers/gpu/drm/i915/i915_sw_fence.c:105:20: warning: unused function 'debug_fence_free' [-Wunused-function] static inline void debug_fence_free(struct i915_sw_fence *fence) ^ The first looks fishy (grep -r debug_fence_init_onstack), the second only has a callsite ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS. drivers/gpu/drm/i915/intel_guc_submission.c:1117:20: warning: unused function 'ctx_save_restore_disabled' [-Wunused-function] static inline bool ctx_save_restore_disabled(struct intel_context *ce) ^ drivers/gpu/drm/i915/display/intel_hdmi.c:1696:26: warning: unused function 'intel_hdmi_hdcp2_protocol' [-Wunused-function] enum hdcp_wired_protocol intel_hdmi_hdcp2_protocol(void) ^ arm64 defconfig builds cleanly, same with arm. Things might get more hairy with all{yes|mod}config, but the existing things it finds don't look insurmountable to me. In fact, I'll file bugs in our issue tracker (https://github.com/ClangBuiltLinux/linux/issues) for the above. So I'm not certain this patch weakens existing checks. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> -- Thanks, ~Nick Desaulniers
On Tue, Aug 27, 2019 at 01:58:05PM -0700, Nick Desaulniers wrote: > On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Tue, Aug 27, 2019 at 07:36:21PM +0900, Masahiro Yamada wrote: > > > GCC and Clang have different policy for -Wunused-function; GCC never > > > reports unused-function warnings for 'static inline' functions whereas > > > Clang reports them if they are defined in source files instead of > > > included headers although it has been suppressed since commit > > > abb2ea7dfd82 ("compiler, clang: suppress warning for unused static > > > inline functions"). > > > > > > We often miss to remove unused functions where 'static inline' is used > > > in .c files since there is no tool to detect them. Unused code remains > > > until somebody notices. For example, commit 075ddd75680f ("regulator: > > > core: remove unused rdev_get_supply()"). > > > > > > Let's remove __maybe_unused from the inline macro to allow Clang to > > > start finding unused static inline functions. As always, it is not a > > > good idea to sprinkle warnings for the normal build, so I added > > > -Wno-unsued-function for no W= build. > > s/unsued/unused/ > > > > > > > Per the documentation [1], -Wno-unused-function will also disable > > > -Wunneeded-internal-declaration, which can help find bugs like > > > commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use > > > mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) > > > I added -Wunneeded-internal-declaration to address it. > > > > > > If you contribute to code clean-up, please run "make CC=clang W=1" > > > and check -Wunused-function warnings. You will find lots of unused > > > functions. > > > > > > Some of them are false-positives because the call-sites are disabled > > > by #ifdef. I do not like to abuse the inline keyword for suppressing > > > unused-function warnings because it is intended to be a hint for the > > > compiler's optimization. I prefer __maybe_unused or #ifdef around the > > > definition. > > I'd say __maybe_unused for function parameters that are used depending > of ifdefs in the body of the function, otherwise strictly ifdefs. > > > > > > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > I am still not a big fan of this as I think it weakens clang as a > > standalone compiler but the change itself looks good so if it is going > > in anyways: > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > I'm sure Nick would like to weigh in as well before this gets merged. > > So right away for an x86_64 defconfig w/ this patch, clang points out: > > drivers/gpu/drm/i915/i915_sw_fence.c:84:20: warning: unused function > 'debug_fence_init_onstack' [-Wunused-function] > static inline void debug_fence_init_onstack(struct i915_sw_fence *fence) > ^ > drivers/gpu/drm/i915/i915_sw_fence.c:105:20: warning: unused function > 'debug_fence_free' [-Wunused-function] > static inline void debug_fence_free(struct i915_sw_fence *fence) > ^ > > The first looks fishy (grep -r debug_fence_init_onstack), the second > only has a callsite ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS. > > drivers/gpu/drm/i915/intel_guc_submission.c:1117:20: warning: unused > function 'ctx_save_restore_disabled' [-Wunused-function] > static inline bool ctx_save_restore_disabled(struct intel_context *ce) > ^ > drivers/gpu/drm/i915/display/intel_hdmi.c:1696:26: warning: unused > function 'intel_hdmi_hdcp2_protocol' [-Wunused-function] > enum hdcp_wired_protocol intel_hdmi_hdcp2_protocol(void) > ^ > arm64 defconfig builds cleanly, same with arm. Things might get more > hairy with all{yes|mod}config, but the existing things it finds don't > look insurmountable to me. In fact, I'll file bugs in our issue > tracker (https://github.com/ClangBuiltLinux/linux/issues) for the > above. > > So I'm not certain this patch weakens existing checks. Well, we no longer get -Wunused-function warnings without W=1. Sometimes, that warning is just a result of missed clean up but there have been instances where it was a real bug: https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ Having warnings not be equal between compilers out of the box causes confusion and irritation: https://crbug.com/974884 Is not the objective of ClangBuiltLinux to rely on GCC less? The only reason that we see the warnings crop up in i915 is because they add -Wall after all of the warnings get disabled (i.e. -Wno-unused-function -Wall so -Wunused-function gets enabled again). To get these warnings after this patch, W=1 has to be used and that results in a lot of extra warnings. x86_64 defconfig has one objtool warning right now, W=1 adds plenty more (from both -W flags and lack of kerneldoc annotations): https://gist.github.com/175afbca29ead14bd039ad46f4ab0ded This is just food for thought though. Cheers, Nathan
On Tue, Aug 27, 2019 at 2:34 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Tue, Aug 27, 2019 at 01:58:05PM -0700, Nick Desaulniers wrote: > > On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > On Tue, Aug 27, 2019 at 07:36:21PM +0900, Masahiro Yamada wrote: > > > > GCC and Clang have different policy for -Wunused-function; GCC never > > > > reports unused-function warnings for 'static inline' functions whereas > > > > Clang reports them if they are defined in source files instead of > > > > included headers although it has been suppressed since commit > > > > abb2ea7dfd82 ("compiler, clang: suppress warning for unused static > > > > inline functions"). > > > > > > > > We often miss to remove unused functions where 'static inline' is used > > > > in .c files since there is no tool to detect them. Unused code remains > > > > until somebody notices. For example, commit 075ddd75680f ("regulator: > > > > core: remove unused rdev_get_supply()"). > > > > > > > > Let's remove __maybe_unused from the inline macro to allow Clang to > > > > start finding unused static inline functions. As always, it is not a > > > > good idea to sprinkle warnings for the normal build, so I added > > > > -Wno-unsued-function for no W= build. > > > > s/unsued/unused/ > > > > > > > > > > Per the documentation [1], -Wno-unused-function will also disable > > > > -Wunneeded-internal-declaration, which can help find bugs like > > > > commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use > > > > mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) > > > > I added -Wunneeded-internal-declaration to address it. > > > > > > > > If you contribute to code clean-up, please run "make CC=clang W=1" > > > > and check -Wunused-function warnings. You will find lots of unused > > > > functions. > > > > > > > > Some of them are false-positives because the call-sites are disabled > > > > by #ifdef. I do not like to abuse the inline keyword for suppressing > > > > unused-function warnings because it is intended to be a hint for the > > > > compiler's optimization. I prefer __maybe_unused or #ifdef around the > > > > definition. > > > > I'd say __maybe_unused for function parameters that are used depending > > of ifdefs in the body of the function, otherwise strictly ifdefs. > > > > > > > > > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > > I am still not a big fan of this as I think it weakens clang as a > > > standalone compiler but the change itself looks good so if it is going > > > in anyways: > > > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > I'm sure Nick would like to weigh in as well before this gets merged. > > > > So right away for an x86_64 defconfig w/ this patch, clang points out: > > > > drivers/gpu/drm/i915/i915_sw_fence.c:84:20: warning: unused function > > 'debug_fence_init_onstack' [-Wunused-function] > > static inline void debug_fence_init_onstack(struct i915_sw_fence *fence) > > ^ > > drivers/gpu/drm/i915/i915_sw_fence.c:105:20: warning: unused function > > 'debug_fence_free' [-Wunused-function] > > static inline void debug_fence_free(struct i915_sw_fence *fence) > > ^ > > > > The first looks fishy (grep -r debug_fence_init_onstack), the second > > only has a callsite ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS. > > > > drivers/gpu/drm/i915/intel_guc_submission.c:1117:20: warning: unused > > function 'ctx_save_restore_disabled' [-Wunused-function] > > static inline bool ctx_save_restore_disabled(struct intel_context *ce) > > ^ > > drivers/gpu/drm/i915/display/intel_hdmi.c:1696:26: warning: unused > > function 'intel_hdmi_hdcp2_protocol' [-Wunused-function] > > enum hdcp_wired_protocol intel_hdmi_hdcp2_protocol(void) > > ^ > > arm64 defconfig builds cleanly, same with arm. Things might get more > > hairy with all{yes|mod}config, but the existing things it finds don't > > look insurmountable to me. In fact, I'll file bugs in our issue > > tracker (https://github.com/ClangBuiltLinux/linux/issues) for the > > above. > > > > So I'm not certain this patch weakens existing checks. > > Well, we no longer get -Wunused-function warnings without W=1. > Sometimes, that warning is just a result of missed clean up but there > have been instances where it was a real bug: > > https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ > > https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ > > Having warnings not be equal between compilers out of the box causes > confusion and irritation: https://crbug.com/974884 > > Is not the objective of ClangBuiltLinux to rely on GCC less? > > The only reason that we see the warnings crop up in i915 is because > they add -Wall after all of the warnings get disabled (i.e. > -Wno-unused-function -Wall so -Wunused-function gets enabled again). > > To get these warnings after this patch, W=1 has to be used and that > results in a lot of extra warnings. x86_64 defconfig has one objtool > warning right now, W=1 adds plenty more (from both -W flags and lack of > kerneldoc annotations): > > https://gist.github.com/175afbca29ead14bd039ad46f4ab0ded > > This is just food for thought though. So if we took just the hunk against include/linux/compiler_types.h from this patch, we'd be back in a situation pre-commit-abb2ea7dfd82 ("compiler, clang: suppress warning for unused static inline functions"). Hmm... I would like to minimize the number of Clang specific warnings that are disabled in scripts/Makefile.extrawarn. Masahiro, does your patch correctly make -Wunused-function work for clang at W=1? It looks like -Wunused gets added to warning-1, but then -Wno-unused-function gets added to KBUILD_CFLAGS after `warning` does. Will that work correctly? I'd imagine that at W=1, KBUILD_CFLAGS for clang will look like: ... -Wunused -Wno-unused-function ... which is probably not what we want? -- Thanks, ~Nick Desaulniers
Hi Nick, Nathan, On Wed, Aug 28, 2019 at 6:56 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Aug 27, 2019 at 2:34 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Tue, Aug 27, 2019 at 01:58:05PM -0700, Nick Desaulniers wrote: > > > On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > On Tue, Aug 27, 2019 at 07:36:21PM +0900, Masahiro Yamada wrote: > > > > > GCC and Clang have different policy for -Wunused-function; GCC never > > > > > reports unused-function warnings for 'static inline' functions whereas > > > > > Clang reports them if they are defined in source files instead of > > > > > included headers although it has been suppressed since commit > > > > > abb2ea7dfd82 ("compiler, clang: suppress warning for unused static > > > > > inline functions"). > > > > > > > > > > We often miss to remove unused functions where 'static inline' is used > > > > > in .c files since there is no tool to detect them. Unused code remains > > > > > until somebody notices. For example, commit 075ddd75680f ("regulator: > > > > > core: remove unused rdev_get_supply()"). > > > > > > > > > > Let's remove __maybe_unused from the inline macro to allow Clang to > > > > > start finding unused static inline functions. As always, it is not a > > > > > good idea to sprinkle warnings for the normal build, so I added > > > > > -Wno-unsued-function for no W= build. > > > > > > s/unsued/unused/ > > > > > > > > > > > > > Per the documentation [1], -Wno-unused-function will also disable > > > > > -Wunneeded-internal-declaration, which can help find bugs like > > > > > commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use > > > > > mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) > > > > > I added -Wunneeded-internal-declaration to address it. > > > > > > > > > > If you contribute to code clean-up, please run "make CC=clang W=1" > > > > > and check -Wunused-function warnings. You will find lots of unused > > > > > functions. > > > > > > > > > > Some of them are false-positives because the call-sites are disabled > > > > > by #ifdef. I do not like to abuse the inline keyword for suppressing > > > > > unused-function warnings because it is intended to be a hint for the > > > > > compiler's optimization. I prefer __maybe_unused or #ifdef around the > > > > > definition. > > > > > > I'd say __maybe_unused for function parameters that are used depending > > > of ifdefs in the body of the function, otherwise strictly ifdefs. > > > > > > > > > > > > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > > > > I am still not a big fan of this as I think it weakens clang as a > > > > standalone compiler but the change itself looks good so if it is going > > > > in anyways: > > > > > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > I'm sure Nick would like to weigh in as well before this gets merged. > > > > > > So right away for an x86_64 defconfig w/ this patch, clang points out: > > > > > > drivers/gpu/drm/i915/i915_sw_fence.c:84:20: warning: unused function > > > 'debug_fence_init_onstack' [-Wunused-function] > > > static inline void debug_fence_init_onstack(struct i915_sw_fence *fence) > > > ^ > > > drivers/gpu/drm/i915/i915_sw_fence.c:105:20: warning: unused function > > > 'debug_fence_free' [-Wunused-function] > > > static inline void debug_fence_free(struct i915_sw_fence *fence) > > > ^ > > > > > > The first looks fishy (grep -r debug_fence_init_onstack), the second > > > only has a callsite ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS. > > > > > > drivers/gpu/drm/i915/intel_guc_submission.c:1117:20: warning: unused > > > function 'ctx_save_restore_disabled' [-Wunused-function] > > > static inline bool ctx_save_restore_disabled(struct intel_context *ce) > > > ^ > > > drivers/gpu/drm/i915/display/intel_hdmi.c:1696:26: warning: unused > > > function 'intel_hdmi_hdcp2_protocol' [-Wunused-function] > > > enum hdcp_wired_protocol intel_hdmi_hdcp2_protocol(void) > > > ^ > > > arm64 defconfig builds cleanly, same with arm. Things might get more > > > hairy with all{yes|mod}config, but the existing things it finds don't > > > look insurmountable to me. In fact, I'll file bugs in our issue > > > tracker (https://github.com/ClangBuiltLinux/linux/issues) for the > > > above. > > > > > > So I'm not certain this patch weakens existing checks. > > > > Well, we no longer get -Wunused-function warnings without W=1. > > Sometimes, that warning is just a result of missed clean up but there > > have been instances where it was a real bug: > > > > https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ > > > > https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ > > > > Having warnings not be equal between compilers out of the box causes > > confusion and irritation: https://crbug.com/974884 > > > > Is not the objective of ClangBuiltLinux to rely on GCC less? > > > > The only reason that we see the warnings crop up in i915 is because > > they add -Wall after all of the warnings get disabled (i.e. > > -Wno-unused-function -Wall so -Wunused-function gets enabled again). > > > > To get these warnings after this patch, W=1 has to be used and that > > results in a lot of extra warnings. x86_64 defconfig has one objtool > > warning right now, W=1 adds plenty more (from both -W flags and lack of > > kerneldoc annotations): > > > > https://gist.github.com/175afbca29ead14bd039ad46f4ab0ded > > > > This is just food for thought though. > > So if we took just the hunk against include/linux/compiler_types.h > from this patch, we'd be back in a situation pre-commit-abb2ea7dfd82 > ("compiler, clang: suppress warning for unused static inline > functions"). Hmm... > > I would like to minimize the number of Clang specific warnings that > are disabled in scripts/Makefile.extrawarn. I agree. I do not want to carry this forever. After we clean up the warnings (it may take several development cycles), I want to turn on Wunused-function for all the build mode. > Masahiro, does your patch correctly make -Wunused-function work for > clang at W=1? It looks like -Wunused gets added to warning-1, but > then -Wno-unused-function gets added to KBUILD_CFLAGS after `warning` > does. Will that work correctly? I'd imagine that at W=1, > KBUILD_CFLAGS for clang will look like: > ... -Wunused -Wno-unused-function ... > which is probably not what we want? Hmm? -Wunused is added only when W=1. -Wno-unused-function is added only when W= was not passed. They do not happen at the same time. > -- > Thanks, > ~Nick Desaulniers -- Best Regards Masahiro Yamada
On Tue, Aug 27, 2019 at 7:58 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > On Wed, Aug 28, 2019 at 6:56 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > Masahiro, does your patch correctly make -Wunused-function work for > > clang at W=1? It looks like -Wunused gets added to warning-1, but > > then -Wno-unused-function gets added to KBUILD_CFLAGS after `warning` > > does. Will that work correctly? I'd imagine that at W=1, > > KBUILD_CFLAGS for clang will look like: > > ... -Wunused -Wno-unused-function ... > > which is probably not what we want? > > Hmm? > > -Wunused is added only when W=1. > > -Wno-unused-function is added only when W= was not passed. > > They do not happen at the same time. Acked-by: Nick Desaulniers <ndesaulniers@google.com> -- Thanks, ~Nick Desaulniers
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 599c27b56c29..14de8d0162fb 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -130,10 +130,6 @@ struct ftrace_likely_data { /* * Force always-inline if the user requests it so via the .config. - * GCC does not warn about unused static inline functions for - * -Wunused-function. This turns out to avoid the need for complex #ifdef - * directives. Suppress the warning in clang as well by using "unused" - * function attribute, which is redundant but not harmful for gcc. * Prefer gnu_inline, so that extern inline functions do not emit an * externally visible function. This makes extern inline behave as per gnu89 * semantics rather than c99. This prevents multiple symbol definition errors @@ -143,11 +139,9 @@ struct ftrace_likely_data { * (which would break users of __always_inline). */ #if !defined(CONFIG_OPTIMIZE_INLINING) -#define inline inline __attribute__((__always_inline__)) __gnu_inline \ - __maybe_unused notrace +#define inline inline __attribute__((__always_inline__)) __gnu_inline notrace #else -#define inline inline __gnu_inline \ - __maybe_unused notrace +#define inline inline __gnu_inline notrace #endif #define __inline__ inline diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index a74ce2e3c33e..69589f4bac48 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -70,5 +70,9 @@ KBUILD_CFLAGS += -Wno-initializer-overrides KBUILD_CFLAGS += -Wno-format KBUILD_CFLAGS += -Wno-sign-compare KBUILD_CFLAGS += -Wno-format-zero-length +KBUILD_CFLAGS += -Wno-unused-function +# -Wno-unused-function would also disable -Wunneeded-internal-declaration, +# but we want to keep it enabled. +KBUILD_CFLAGS += -Wunneeded-internal-declaration endif endif
GCC and Clang have different policy for -Wunused-function; GCC never reports unused-function warnings for 'static inline' functions whereas Clang reports them if they are defined in source files instead of included headers although it has been suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused static inline functions"). We often miss to remove unused functions where 'static inline' is used in .c files since there is no tool to detect them. Unused code remains until somebody notices. For example, commit 075ddd75680f ("regulator: core: remove unused rdev_get_supply()"). Let's remove __maybe_unused from the inline macro to allow Clang to start finding unused static inline functions. As always, it is not a good idea to sprinkle warnings for the normal build, so I added -Wno-unsued-function for no W= build. Per the documentation [1], -Wno-unused-function will also disable -Wunneeded-internal-declaration, which can help find bugs like commit 8289c4b6f2e5 ("platform/x86: mlx-platform: Properly use mlxplat_mlxcpld_msn201x_items"). (pointed out by Nathan Chancellor) I added -Wunneeded-internal-declaration to address it. If you contribute to code clean-up, please run "make CC=clang W=1" and check -Wunused-function warnings. You will find lots of unused functions. Some of them are false-positives because the call-sites are disabled by #ifdef. I do not like to abuse the inline keyword for suppressing unused-function warnings because it is intended to be a hint for the compiler's optimization. I prefer __maybe_unused or #ifdef around the definition. [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> Reviewed-by: Kees Cook <keescook@chromium.org> --- Changes in v2: - Add -Wunneeded-internal-declaration (per Nathan Chancellor) include/linux/compiler_types.h | 10 ++-------- scripts/Makefile.extrawarn | 4 ++++ 2 files changed, 6 insertions(+), 8 deletions(-) -- 2.17.1