Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
On Tue, Aug 27, 2019 at 7:58 PM Masahiro Yamada wrote: > On Wed, Aug 28, 2019 at 6:56 AM Nick Desaulniers > 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 -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
Hi Nick, Nathan, On Wed, Aug 28, 2019 at 6:56 AM Nick Desaulniers wrote: > > On Tue, Aug 27, 2019 at 2:34 PM Nathan Chancellor > 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 > > > 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 > > > > > Reviewed-by: Kees Cook > > > > > > > > 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 > > > > > > > > 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-skomatin...@nvidia.com/ > > > > Having warnings not be equal between compilers out of the box causes > > confusion and
Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
On Tue, Aug 27, 2019 at 2:34 PM Nathan Chancellor 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 > > 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 > > > > Reviewed-by: Kees Cook > > > > > > 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 > > > > > > 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-skomatin...@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
Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
On Tue, Aug 27, 2019 at 01:58:05PM -0700, Nick Desaulniers wrote: > On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor > 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 > > > Reviewed-by: Kees Cook > > > > 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 > > > > 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-skomatin...@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
Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
On Tue, Aug 27, 2019 at 12:28 PM Nathan Chancellor 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 > > Reviewed-by: Kees Cook > > 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 > > 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 Tested-by: Nick Desaulniers -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang
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 > Reviewed-by: Kees Cook 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 I'm sure Nick would like to weigh in as well before this gets merged.