| Message ID | 20190819105138.5053-1-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: enable unused-function warnings for W= build with Clang | expand |
On Mon, Aug 19, 2019 at 07:51:38PM +0900, Masahiro Yamada wrote: > GCC and Clang have different policy for -Wunused-function; GCC does > not report unused-function warnings at all for the functions marked > as 'static inline'. Clang does report unused-function warnings if they > are defined in source files instead of headers. > > We could use Clang for detecting unused functions, but it has been > suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress > warning for unused static inline functions"). > > So, we never notice left-over code if functions in .c files are > marked as inline. > > Let's remove __maybe_unused from the inline macro. As always, it is > not a good idea to sprinkle warnings for the normal build. So, these > warnings will be shown for the W= build. > > 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 might affect the compiler's > optimization. When I need to fix unused-functions warnings, I prefer > adding #ifdef or __maybe_unused to function definitions. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> So if I understand everything correctly, this change allows us to start finding unused static inline functions with clang at W=1 but disables -Wunused-function by default... I am not sure that is a good tradeoff as I am pretty sure that W=1 is fairly noisy for clang although I haven't checked lately. I'd argue most regular developers do not build with W=1 meaning -Wunused-function generally will not be run with clang at all, missing stuff like this: https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ Furthermore, per the documemtation [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"). [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function Is there a way to conditionally remove __maybe_unused from the inline defintion so that we keep the current behavior but we can still selectively find potentially unused functions? Cheers, Nathan
On Mon, Aug 19, 2019 at 07:51:38PM +0900, Masahiro Yamada wrote: > GCC and Clang have different policy for -Wunused-function; GCC does > not report unused-function warnings at all for the functions marked > as 'static inline'. Clang does report unused-function warnings if they > are defined in source files instead of headers. > > We could use Clang for detecting unused functions, but it has been > suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress > warning for unused static inline functions"). > > So, we never notice left-over code if functions in .c files are > marked as inline. > > Let's remove __maybe_unused from the inline macro. As always, it is > not a good idea to sprinkle warnings for the normal build. So, these > warnings will be shown for the W= build. > > 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 might affect the compiler's > optimization. When I need to fix unused-functions warnings, I prefer > adding #ifdef or __maybe_unused to function definitions. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > > include/linux/compiler_types.h | 10 ++-------- > scripts/Makefile.extrawarn | 1 + > 2 files changed, 3 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..92f542797e03 100644 > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -70,5 +70,6 @@ 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 > endif > endif > -- > 2.17.1 >
Hi Nathan, On Tue, Aug 20, 2019 at 1:09 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Mon, Aug 19, 2019 at 07:51:38PM +0900, Masahiro Yamada wrote: > > GCC and Clang have different policy for -Wunused-function; GCC does > > not report unused-function warnings at all for the functions marked > > as 'static inline'. Clang does report unused-function warnings if they > > are defined in source files instead of headers. > > > > We could use Clang for detecting unused functions, but it has been > > suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress > > warning for unused static inline functions"). > > > > So, we never notice left-over code if functions in .c files are > > marked as inline. > > > > Let's remove __maybe_unused from the inline macro. As always, it is > > not a good idea to sprinkle warnings for the normal build. So, these > > warnings will be shown for the W= build. > > > > 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 might affect the compiler's > > optimization. When I need to fix unused-functions warnings, I prefer > > adding #ifdef or __maybe_unused to function definitions. > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > So if I understand everything correctly, this change allows us to start > finding unused static inline functions with clang at W=1 but disables > -Wunused-function by default... I am not sure that is a good tradeoff > as I am pretty sure that W=1 is fairly noisy for clang although I > haven't checked lately. I'd argue most regular developers do not build > with W=1 meaning -Wunused-function generally will not be run with clang > at all, missing stuff like this: Try "git log --grep=W=1" Some people are making efforts to fix W=1 warnings. I believe somebody will start to remove unused static inline functions. > > https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ > > https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ > > Furthermore, per the documemtation [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"). > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function How about this? KBUILD_CFLAGS += -Wno-unused-function KBUILD_CFLAGS += -Wunneeded-internal-declaration > Is there a way to conditionally remove __maybe_unused from the inline > defintion so that we keep the current behavior but we can still > selectively find potentially unused functions? It would be possible by tweaking include/linux/compiler_types.h but I am not a big fan of uglifying the 'inline' replacement any more.
On Tue, Aug 20, 2019 at 01:58:26AM +0900, Masahiro Yamada wrote: > Hi Nathan, > > On Tue, Aug 20, 2019 at 1:09 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Mon, Aug 19, 2019 at 07:51:38PM +0900, Masahiro Yamada wrote: > > > GCC and Clang have different policy for -Wunused-function; GCC does > > > not report unused-function warnings at all for the functions marked > > > as 'static inline'. Clang does report unused-function warnings if they > > > are defined in source files instead of headers. > > > > > > We could use Clang for detecting unused functions, but it has been > > > suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress > > > warning for unused static inline functions"). > > > > > > So, we never notice left-over code if functions in .c files are > > > marked as inline. > > > > > > Let's remove __maybe_unused from the inline macro. As always, it is > > > not a good idea to sprinkle warnings for the normal build. So, these > > > warnings will be shown for the W= build. > > > > > > 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 might affect the compiler's > > > optimization. When I need to fix unused-functions warnings, I prefer > > > adding #ifdef or __maybe_unused to function definitions. > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > So if I understand everything correctly, this change allows us to start > > finding unused static inline functions with clang at W=1 but disables > > -Wunused-function by default... I am not sure that is a good tradeoff > > as I am pretty sure that W=1 is fairly noisy for clang although I > > haven't checked lately. I'd argue most regular developers do not build > > with W=1 meaning -Wunused-function generally will not be run with clang > > at all, missing stuff like this: > > > Try "git log --grep=W=1" > > Some people are making efforts to fix W=1 warnings. > I believe somebody will start to remove unused static inline functions. Yes, it could be a good way to get people involved with working with clang. > > > > > > > https://lore.kernel.org/lkml/20190523010235.GA105588@archlinux-epyc/ > > > > https://lore.kernel.org/lkml/1558574945-19275-1-git-send-email-skomatineni@nvidia.com/ > > > > Furthermore, per the documemtation [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"). > > > > [1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function > > > How about this? > > KBUILD_CFLAGS += -Wno-unused-function > KBUILD_CFLAGS += -Wunneeded-internal-declaration Yes, that would work. > > > > > Is there a way to conditionally remove __maybe_unused from the inline > > defintion so that we keep the current behavior but we can still > > selectively find potentially unused functions? > > It would be possible by tweaking include/linux/compiler_types.h > but I am not a big fan of uglifying the 'inline' replacement any more. I agree that ugly is not ideal but I think it is even less ideal to weaken the default set of warnings for clang... Merely food for thought though. Cheers, Nathan
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..92f542797e03 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -70,5 +70,6 @@ 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 endif endif
GCC and Clang have different policy for -Wunused-function; GCC does not report unused-function warnings at all for the functions marked as 'static inline'. Clang does report unused-function warnings if they are defined in source files instead of headers. We could use Clang for detecting unused functions, but it has been suppressed since commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused static inline functions"). So, we never notice left-over code if functions in .c files are marked as inline. Let's remove __maybe_unused from the inline macro. As always, it is not a good idea to sprinkle warnings for the normal build. So, these warnings will be shown for the W= build. 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 might affect the compiler's optimization. When I need to fix unused-functions warnings, I prefer adding #ifdef or __maybe_unused to function definitions. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- include/linux/compiler_types.h | 10 ++-------- scripts/Makefile.extrawarn | 1 + 2 files changed, 3 insertions(+), 8 deletions(-)