[PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Masahiro Yamada]

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(-)

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
-- 
2.17.1

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Nathan Chancellor]

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.

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Nick Desaulniers]

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

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Nathan Chancellor]

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

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Nick Desaulniers]

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

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Masahiro Yamada]

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

Re: [PATCH v2] kbuild: enable unused-function warnings for W= build with Clang

[Nick Desaulniers]

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