[RFC] clang: 'unused-function' warning on static inline functions

[RFC] clang: 'unused-function' warning on static inline functions

[Matthias Kaehlcke]

Hi,

There has been discussion spread over different threads on how to deal
with 'unused-function' warnings raised by clang on static inline
functions. gcc in general does not emit warnings for unused static
inline functions, clang does if the function is in a .c file.

When building the kernel with clang several 'unused-function' warnings
are emitted, about half of them point out actual dead code, the others
are false positives stemming from functions that are only used with
certain combinations of configuration options.

I consider the warning a useful tool to detect unused code and started
to submit patches that remove dead code and suppress the false
positives. The dead code removal was generally well received,
maintainer response to suppressing the false positives was mixed. Some
patches were accepted, in other cases maintainers raised concerns
that adding '__maybe_unused' or putting a function inside an #ifdef
block just adds clutter to the code without providing any benefits,
and clang should behave like gcc in this aspect.

Most discussion took place in these two threads:

zlib: Put get_unaligned16() inside #ifdef block
https://patchwork.kernel.org/patch/9741413/

compiler, clang: suppress warning for unused static inline functions
https://patchwork.kernel.org/patch/9746913/

I understand the reluctance to adding clutter to the code (though one
could argue that __maybe_unused serves as documentation; using #ifdef
is an option, but is not needed in most cases), but I don't think that
it would be preferable to have clang behave like gcc. Silencing the
warning on false positives removes clutter from the build log and
makes it easier to spot the actual dead code.

These are examples of unused code that has been removed thanks to the
warning on static inline functions:

x86/ioapic: Remove unused function IO_APIC_irq_trigger()
https://patchwork.kernel.org/patch/9741539/

cfq-iosched: Delete unused function min_vdisktime()
https://patchwork.kernel.org/patch/9751327/

r8152: Remove unused function usb_ocp_read()
https://patchwork.kernel.org/patch/9735027/

net1080: Remove unused function nc_dump_ttl()
https://patchwork.kernel.org/patch/9735053/

crypto: rng: Remove unused function __crypto_rng_cast()
https://patchwork.kernel.org/patch/9741515/

perf/core: Remove unused function perf_cgroup_event_cgrp_time()
https://patchwork.kernel.org/patch/9744129/

net: jme: Remove unused functions
https://patchwork.kernel.org/patch/9744673/

ASoC: Intel: sst: Remove unused function sst_restore_shim64()
https://patchwork.kernel.org/patch/9741551/

A further code removal that was spotted in the context of the previous
patch:

ASoC: Intel: sst: Delete sst_shim_regs64; saved regs are never used
https://patchwork.kernel.org/patch/9754923/

The goal of this thread is to arrive to a conclusion on how to deal
with the warning. If we want clang to help to detect unused static
inline functions I think we should suppress the warning on false
positives (the number is limited, at least for x86 and arm64
defconfig, patches for most instances have already been sent out). And
if maintainers consider that having the extra ability to spot dead
code doesn't justify the clutter of marking some functions as
__maybe_used (or using #ifdef if preferred) we should probably apply
Davids patch (https://patchwork.kernel.org/patch/9746913/) to make
clang behave like gcc for unused static inline functions.

Thanks

Matthias

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Matthias Kaehlcke]

El Tue, May 30, 2017 at 11:13:06AM -0700 Matthias Kaehlcke ha dit:

> Hi,
> 
> There has been discussion spread over different threads on how to deal
> with 'unused-function' warnings raised by clang on static inline
> functions. gcc in general does not emit warnings for unused static
> inline functions, clang does if the function is in a .c file.
> 
> When building the kernel with clang several 'unused-function' warnings
> are emitted, about half of them point out actual dead code, the others
> are false positives stemming from functions that are only used with
> certain combinations of configuration options.
> 
> I consider the warning a useful tool to detect unused code and started
> to submit patches that remove dead code and suppress the false
> positives. The dead code removal was generally well received,
> maintainer response to suppressing the false positives was mixed. Some
> patches were accepted, in other cases maintainers raised concerns
> that adding '__maybe_unused' or putting a function inside an #ifdef
> block just adds clutter to the code without providing any benefits,
> and clang should behave like gcc in this aspect.
> 
> Most discussion took place in these two threads:
> 
> zlib: Put get_unaligned16() inside #ifdef block
> https://patchwork.kernel.org/patch/9741413/
> 
> compiler, clang: suppress warning for unused static inline functions
> https://patchwork.kernel.org/patch/9746913/
> 
> I understand the reluctance to adding clutter to the code (though one
> could argue that __maybe_unused serves as documentation; using #ifdef
> is an option, but is not needed in most cases), but I don't think that
> it would be preferable to have clang behave like gcc. Silencing the
> warning on false positives removes clutter from the build log and
> makes it easier to spot the actual dead code.
> 
> These are examples of unused code that has been removed thanks to the
> warning on static inline functions:
> 
> x86/ioapic: Remove unused function IO_APIC_irq_trigger()
> https://patchwork.kernel.org/patch/9741539/
> 
> cfq-iosched: Delete unused function min_vdisktime()
> https://patchwork.kernel.org/patch/9751327/
> 
> r8152: Remove unused function usb_ocp_read()
> https://patchwork.kernel.org/patch/9735027/
> 
> net1080: Remove unused function nc_dump_ttl()
> https://patchwork.kernel.org/patch/9735053/
> 
> crypto: rng: Remove unused function __crypto_rng_cast()
> https://patchwork.kernel.org/patch/9741515/
> 
> perf/core: Remove unused function perf_cgroup_event_cgrp_time()
> https://patchwork.kernel.org/patch/9744129/
> 
> net: jme: Remove unused functions
> https://patchwork.kernel.org/patch/9744673/
> 
> ASoC: Intel: sst: Remove unused function sst_restore_shim64()
> https://patchwork.kernel.org/patch/9741551/
> 
> A further code removal that was spotted in the context of the previous
> patch:
> 
> ASoC: Intel: sst: Delete sst_shim_regs64; saved regs are never used
> https://patchwork.kernel.org/patch/9754923/
> 
> The goal of this thread is to arrive to a conclusion on how to deal
> with the warning. If we want clang to help to detect unused static
> inline functions I think we should suppress the warning on false
> positives (the number is limited, at least for x86 and arm64
> defconfig, patches for most instances have already been sent out). And
> if maintainers consider that having the extra ability to spot dead
> code doesn't justify the clutter of marking some functions as
> __maybe_used (or using #ifdef if preferred) we should probably apply
> Davids patch (https://patchwork.kernel.org/patch/9746913/) to make
> clang behave like gcc for unused static inline functions.

As suggested in one of the other threads, a list of the false
positives (from x86 and arm64 defconfig) that would need attention in
case of leaving the warning enabled:

arch/x86/kernel/cpu/common.c:267:19: warning: unused function 'flag_is_changeable_p' [-Wunused-function]
arch/x86/platform/efi/efi_64.c:240:1: warning: unused function 'virt_to_phys_or_null_size' [-Wunused-function]
block/cfq-iosched.c:451:1: warning: unused function 'cfq_clear_cfqq_sync' [-Wunused-function]
block/cfq-iosched.c:590:20: warning: unused function 'cfqg_stats_set_start_group_wait_time' [-Wunused-function]
block/cfq-iosched.c:591:20: warning: unused function 'cfqg_stats_end_empty_time' [-Wunused-function]
drivers/gpu/drm/drm_mm.c:165:1: warning: unused function 'drm_mm_interval_tree_insert' [-Wunused-function]
drivers/gpu/drm/drm_mm.c:165:1: warning: unused function 'drm_mm_interval_tree_iter_next' [-Wunused-function]
drivers/gpu/drm/i915/i915_sw_fence.c:97:20: warning: unused function 'debug_fence_free' [-Wunused-function]
drivers/hid/hid-sony.c:2105:20: warning: unused function 'sony_send_output_report' [-Wunused-function]
drivers/media/media-entity.c:41:27: warning: unused function 'intf_type' [-Wunused-function]
drivers/watchdog/s3c2410_wdt.c:212:35: warning: unused function 'freq_to_wdt' [-Wunused-function]
drivers/watchdog/s3c2410_wdt.c:308:19: warning: unused function 's3c2410wdt_is_running' [-Wunused-function]
kernel/events/core.c:928:19: warning: unused function 'perf_cgroup_event_cgrp_time' [-Wunused-function]
kernel/locking/osq_lock.c:24:19: warning: unused function 'node_cpu' [-Wunused-function]
kernel/power/snapshot.c:1256:21: warning: unused function 'saveable_highmem_page' [-Wunused-function]
kernel/sched/cputime.c:258:19: warning: unused function 'account_other_time' [-Wunused-function]
lib/zlib_inflate/inffast.c:31:1: warning: unused function 'get_unaligned16' [-Wunused-function]
mm/page_alloc.c:1312:30: warning: unused function 'meminit_pfn_in_nid' [-Wunused-function]
mm/slub.c:1328:21: warning: unused function 'slab_free_hook' [-Wunused-function]
mm/slub.c:1945:28: warning: unused function 'tid_to_cpu' [-Wunused-function]
mm/slub.c:1950:29: warning: unused function 'tid_to_event' [-Wunused-function]
net/netfilter/nf_conntrack_netlink.c:534:22: warning: unused function 'ctnetlink_proto_size' [-Wunused-function]
net/netfilter/nf_conntrack_netlink.c:551:22: warning: unused function 'ctnetlink_acct_size' [-Wunused-function]
net/netfilter/nf_conntrack_netlink.c:561:19: warning: unused function 'ctnetlink_secctx_size' [-Wunused-function]
net/netfilter/nf_conntrack_netlink.c:577:22: warning: unused function 'ctnetlink_timestamp_size' [-Wunused-function]

The list does not include instances that have already been addressed
in maintainer trees or warnings about actual dead code. Obviously
there will be more instances for other architectures and
configurations.

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Arnd Bergmann]

On Thu, Jun 1, 2017 at 1:55 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> ASoC: Intel: sst: Delete sst_shim_regs64; saved regs are never used
>> https://patchwork.kernel.org/patch/9754923/
>>
>> The goal of this thread is to arrive to a conclusion on how to deal
>> with the warning. If we want clang to help to detect unused static
>> inline functions I think we should suppress the warning on false
>> positives (the number is limited, at least for x86 and arm64
>> defconfig, patches for most instances have already been sent out). And
>> if maintainers consider that having the extra ability to spot dead
>> code doesn't justify the clutter of marking some functions as
>> __maybe_used (or using #ifdef if preferred) we should probably apply
>> Davids patch (https://patchwork.kernel.org/patch/9746913/) to make
>> clang behave like gcc for unused static inline functions.
>
> As suggested in one of the other threads, a list of the false
> positives (from x86 and arm64 defconfig) that would need attention in
> case of leaving the warning enabled:
>
> arch/x86/kernel/cpu/common.c:267:19: warning: unused function 'flag_is_changeable_p' [-Wunused-function]
> arch/x86/platform/efi/efi_64.c:240:1: warning: unused function 'virt_to_phys_or_null_size' [-Wunused-function]
> block/cfq-iosched.c:451:1: warning: unused function 'cfq_clear_cfqq_sync' [-Wunused-function]
> block/cfq-iosched.c:590:20: warning: unused function 'cfqg_stats_set_start_group_wait_time' [-Wunused-function]
> block/cfq-iosched.c:591:20: warning: unused function 'cfqg_stats_end_empty_time' [-Wunused-function]
> drivers/gpu/drm/drm_mm.c:165:1: warning: unused function 'drm_mm_interval_tree_insert' [-Wunused-function]
> drivers/gpu/drm/drm_mm.c:165:1: warning: unused function 'drm_mm_interval_tree_iter_next' [-Wunused-function]
> drivers/gpu/drm/i915/i915_sw_fence.c:97:20: warning: unused function 'debug_fence_free' [-Wunused-function]
> drivers/hid/hid-sony.c:2105:20: warning: unused function 'sony_send_output_report' [-Wunused-function]
> drivers/media/media-entity.c:41:27: warning: unused function 'intf_type' [-Wunused-function]
> drivers/watchdog/s3c2410_wdt.c:212:35: warning: unused function 'freq_to_wdt' [-Wunused-function]
> drivers/watchdog/s3c2410_wdt.c:308:19: warning: unused function 's3c2410wdt_is_running' [-Wunused-function]
> kernel/events/core.c:928:19: warning: unused function 'perf_cgroup_event_cgrp_time' [-Wunused-function]
> kernel/locking/osq_lock.c:24:19: warning: unused function 'node_cpu' [-Wunused-function]
> kernel/power/snapshot.c:1256:21: warning: unused function 'saveable_highmem_page' [-Wunused-function]
> kernel/sched/cputime.c:258:19: warning: unused function 'account_other_time' [-Wunused-function]
> lib/zlib_inflate/inffast.c:31:1: warning: unused function 'get_unaligned16' [-Wunused-function]
> mm/page_alloc.c:1312:30: warning: unused function 'meminit_pfn_in_nid' [-Wunused-function]
> mm/slub.c:1328:21: warning: unused function 'slab_free_hook' [-Wunused-function]
> mm/slub.c:1945:28: warning: unused function 'tid_to_cpu' [-Wunused-function]
> mm/slub.c:1950:29: warning: unused function 'tid_to_event' [-Wunused-function]
> net/netfilter/nf_conntrack_netlink.c:534:22: warning: unused function 'ctnetlink_proto_size' [-Wunused-function]
> net/netfilter/nf_conntrack_netlink.c:551:22: warning: unused function 'ctnetlink_acct_size' [-Wunused-function]
> net/netfilter/nf_conntrack_netlink.c:561:19: warning: unused function 'ctnetlink_secctx_size' [-Wunused-function]
> net/netfilter/nf_conntrack_netlink.c:577:22: warning: unused function 'ctnetlink_timestamp_size' [-Wunused-function]
>
> The list does not include instances that have already been addressed
> in maintainer trees or warnings about actual dead code. Obviously
> there will be more instances for other architectures and
> configurations.

Those should all be fairly easy to address, I'd vote for leaving the
warning enabled
in clang, and possibly asking the gcc maintainers to add a similar feature for
warning about it.

On a more general note, I'd suggest classifying clang warning options
like this as 'W=1'
until we have addressed the known output, i.e. move all the warnings
we currently avoid
with clang into scripts/Makefile.extrawarn so that they are active
when building with
'make W=1' but disabled by default, leading to a clean build with a
plain 'make -s'.

I have started a patch set to eventually move all warning options into a new
include/linux/compiler-warnings.h that would allow a more convenient way of
controlling the warning options depending on the compiler version, e.g. some
warnings are useful with recent gcc versions but show lots of clutter
when enabled
on older versions.

        Arnd

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Linus Torvalds]

On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> Those should all be fairly easy to address, I'd vote for leaving the
> warning enabled
> in clang, and possibly asking the gcc maintainers to add a similar feature for
> warning about it.

Hell no. That warning is pointless shit.

The function is inlined. If it's not used, why the f*ck should you get
a warning? Just to add more pointless #ifdef'fery around a function
that is perhaps only used under certain circumstances?

No. That warning is garbage, and we will ignore it, and we sure as
hell will not ask gcc to add more garbage warnings.

                         Linus

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Matthias Kaehlcke]

El Tue, Jun 06, 2017 at 09:32:35AM -0700 Linus Torvalds ha dit:

> On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > Those should all be fairly easy to address, I'd vote for leaving the
> > warning enabled
> > in clang, and possibly asking the gcc maintainers to add a similar feature for
> > warning about it.
> 
> Hell no. That warning is pointless shit.

I tend to disagree, the warning is useful to detect truly unused
static inline functions, which should be removed, rather than be
carried around/maintained for often long periods of time.

Also lets not forget that the kernel is somewhat special with its
heavy use of #ifdefs , which leads to the larger number of false
positives. At least for many other projects the warning makes perfect
sense IMO. A switch to enable/disable it could be interesting.

> The function is inlined. If it's not used, why the f*ck should you get
> a warning?

To detect dead code?

> Just to add more pointless #ifdef'fery around a function that is
> perhaps only used under certain circumstances?

The vast majority of instances could be fixed by simply marking the
function as __maybe_unused, which is already widely used in the
kernel. I don't fully understand why adding the attribute to static
inline functions (in .c files) is considered an offense, while it is a
standard practice for non-inline function. Granted, unused static
inline functions don't waste (binary) space, but doesn't the extra
capability to detect dead code also have a value?

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Linus Torvalds]

On Tue, Jun 6, 2017 at 2:23 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> I tend to disagree, the warning is useful to detect truly unused
> static inline functions, which should be removed, rather than be
> carried around/maintained for often long periods of time.

That may be true in other projects, but we really do have a lot of
code that is conditionally used. The warning is just not useful.

I agree that we could use "__maybe_unused", but at the same time, I
don't really see the point. There's no way in hell we'd ever do that
for inlines that are in header files (*of course* they may be unused),
why would we then haev a magical rule like "let's do it for inlines in
C files".

I applied the patch from David Rientjes to just make "inline"
automatically mean "maybe unused" for clang.

                    Linus

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Jens Axboe]

On 06/06/2017 03:23 PM, Matthias Kaehlcke wrote:
> El Tue, Jun 06, 2017 at 09:32:35AM -0700 Linus Torvalds ha dit:
> 
>> On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> Those should all be fairly easy to address, I'd vote for leaving the
>>> warning enabled
>>> in clang, and possibly asking the gcc maintainers to add a similar feature for
>>> warning about it.
>>
>> Hell no. That warning is pointless shit.
> 
> I tend to disagree, the warning is useful to detect truly unused
> static inline functions, which should be removed, rather than be
> carried around/maintained for often long periods of time.

One example is the patch sent for CFQ, which has macros for define
functions for setting/clearing bits on the queue:

#define CFQ_CFQQ_FNS(name)

for one bit, we never clear the bit after we set it, we only set it and
later test for it. Hence the clear variant of that function is unused.

Now I get a warning. The fix to that would be to define a new variant of
CFQ_CFQQ_FNS() that only declares the exact one I need for the version
that is never cleared.

Or the fix is to just ignore the bogus warning on an unused inline.  I
greatly prefer the latter.

The counter example is this one:

http://git.kernel.dk/cgit/linux-block/commit/?id=03ea8ad78cfb2910862c8dfcd2a627fc04097db2

where it is truly just dead junk. I'd rather just leave the dead junk
than have pointless warnings, if I have to choose one of the two
outcomes.

-- 
Jens Axboe

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Matthias Kaehlcke]

El Tue, Jun 06, 2017 at 02:28:03PM -0700 Linus Torvalds ha dit:

> I applied the patch from David Rientjes to just make "inline"
> automatically mean "maybe unused" for clang.

Unfortunately as is the patch doesn't work:

include/linux/compiler-clang.h:20:9: error: 'inline' macro redefined [-Werror,-Wmacro-redefined]
#define inline inline __attribute__((unused))
        ^
include/linux/compiler-gcc.h:78:9: note: previous definition is here
#define inline          inline          notrace

Another version of David's patch (https://lkml.org/lkml/2017/5/24/878)
first undefines 'inline' before redefining it:

#ifdef inline
#undef inline
#define inline inline __attribute__((unused))
#endif

This works at least in the sense of not causing compiler errors. I
couldn't validate if it actually still indicates the compiler to
inline a function, since in any case 'inline' is only a
recommendation. In the few experiments I did without the patch clang
didn't make a difference between static inline and non-inline
functions.

The redefinition above could be used to fix the build error, however
it would imply to lose the extra attributes from compiler-gcc.h.

Re: [RFC] clang: 'unused-function' warning on static inline functions

[David Rientjes]

On Tue, 6 Jun 2017, Matthias Kaehlcke wrote:

> Unfortunately as is the patch doesn't work:
> 
> include/linux/compiler-clang.h:20:9: error: 'inline' macro redefined [-Werror,-Wmacro-redefined]
> #define inline inline __attribute__((unused))
>         ^
> include/linux/compiler-gcc.h:78:9: note: previous definition is here
> #define inline          inline          notrace
> 
> Another version of David's patch (https://lkml.org/lkml/2017/5/24/878)
> first undefines 'inline' before redefining it:
> 
> #ifdef inline
> #undef inline
> #define inline inline __attribute__((unused))
> #endif
> 
> This works at least in the sense of not causing compiler errors. I
> couldn't validate if it actually still indicates the compiler to
> inline a function, since in any case 'inline' is only a
> recommendation. In the few experiments I did without the patch clang
> didn't make a difference between static inline and non-inline
> functions.
> 
> The redefinition above could be used to fix the build error, however
> it would imply to lose the extra attributes from compiler-gcc.h.
> 

I've followed up with a patch that handles this behavior in compiler-gcc.h 
since clang defines __GNUC__ as well, so clang gets both compiler-gcc.h 
and compiler-clang.h behavior.

I've tested it, but please feel free to add your Tested-by for more 
confidence in the change.

Linus, who currently maintains include/linux/compiler*.h changes?  I see 
only an entry for a sparse maintainer.  I'd happily manage a cross 
compiler setup if it would be helpful to prevent this type of issue in the 
future.  First question would be if there is a minimum gcc major version 
intended to be supported?

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Arnd Bergmann]

On Tue, Jun 6, 2017 at 11:29 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 06/06/2017 03:23 PM, Matthias Kaehlcke wrote:
>> El Tue, Jun 06, 2017 at 09:32:35AM -0700 Linus Torvalds ha dit:
>>
>>> On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>
>>>> Those should all be fairly easy to address, I'd vote for leaving the
>>>> warning enabled
>>>> in clang, and possibly asking the gcc maintainers to add a similar feature for
>>>> warning about it.
>>>
>>> Hell no. That warning is pointless shit.
>>
>> I tend to disagree, the warning is useful to detect truly unused
>> static inline functions, which should be removed, rather than be
>> carried around/maintained for often long periods of time.
>
> One example is the patch sent for CFQ, which has macros for define
> functions for setting/clearing bits on the queue:
>
> #define CFQ_CFQQ_FNS(name)
>
> for one bit, we never clear the bit after we set it, we only set it and
> later test for it. Hence the clear variant of that function is unused.
>
> Now I get a warning. The fix to that would be to define a new variant of
> CFQ_CFQQ_FNS() that only declares the exact one I need for the version
> that is never cleared.
>
> Or the fix is to just ignore the bogus warning on an unused inline.  I
> greatly prefer the latter.
>
> The counter example is this one:
>
> http://git.kernel.dk/cgit/linux-block/commit/?id=03ea8ad78cfb2910862c8dfcd2a627fc04097db2
>
> where it is truly just dead junk. I'd rather just leave the dead junk
> than have pointless warnings, if I have to choose one of the two
> outcomes.

This is a relatively rare case, with an inline function defined by a macro, and
I sent a patch for a similar one in 1f318a8bafcf ("modules: mark
__inittest/__exittest
as __maybe_unused"). I think this is a case where the __maybe_unused
annotation is reasonable, though for the other instances of unused inline
functions in .c files, there is often a better way: typically the only caller
of a function is inside of an #ifdef and moving the inline function definition
into the same #ifdef block makes it clearer what is going on.

      Arnd

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Steven Rostedt]

On Wed, 7 Jun 2017 10:17:18 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tue, Jun 6, 2017 at 11:29 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > On 06/06/2017 03:23 PM, Matthias Kaehlcke wrote:  
> >> El Tue, Jun 06, 2017 at 09:32:35AM -0700 Linus Torvalds ha dit:
> >>  
> >>> On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:  
> >>>>
> >>>> Those should all be fairly easy to address, I'd vote for leaving the
> >>>> warning enabled
> >>>> in clang, and possibly asking the gcc maintainers to add a similar feature for
> >>>> warning about it.  
> >>>
> >>> Hell no. That warning is pointless shit.  
> >>
> >> I tend to disagree, the warning is useful to detect truly unused
> >> static inline functions, which should be removed, rather than be
> >> carried around/maintained for often long periods of time.  
> >
> > One example is the patch sent for CFQ, which has macros for define
> > functions for setting/clearing bits on the queue:
> >
> > #define CFQ_CFQQ_FNS(name)
> >
> > for one bit, we never clear the bit after we set it, we only set it and
> > later test for it. Hence the clear variant of that function is unused.
> >
> > Now I get a warning. The fix to that would be to define a new variant of
> > CFQ_CFQQ_FNS() that only declares the exact one I need for the version
> > that is never cleared.
> >
> > Or the fix is to just ignore the bogus warning on an unused inline.  I
> > greatly prefer the latter.
> >
> > The counter example is this one:
> >
> > http://git.kernel.dk/cgit/linux-block/commit/?id=03ea8ad78cfb2910862c8dfcd2a627fc04097db2
> >
> > where it is truly just dead junk. I'd rather just leave the dead junk
> > than have pointless warnings, if I have to choose one of the two
> > outcomes.  
> 
> This is a relatively rare case, with an inline function defined by a macro, and
> I sent a patch for a similar one in 1f318a8bafcf ("modules: mark
> __inittest/__exittest
> as __maybe_unused"). I think this is a case where the __maybe_unused
> annotation is reasonable, though for the other instances of unused inline
> functions in .c files, there is often a better way: typically the only caller
> of a function is inside of an #ifdef and moving the inline function definition
> into the same #ifdef block makes it clearer what is going on.
> 

How many warnings does the TRACE_EVENT() macros produce? That macro
creates a lot of static inline functions for every event. For example,
it will create an rcuidle() version of a tracepoint for those locations
that need to be called where RCU is not watching. It also adds a
trace_#tracepoint#_enabled() function for those that want to test if
the tracepoint is enabled or not.

That is, every tracepoint creates static inlined functions for cases
that are used by only a few. I'm sure they will cause lots of warnings
because there will be a lot of unused static inlined functions.

-- Steve

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Arnd Bergmann]

On Wed, Jun 7, 2017 at 2:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 7 Jun 2017 10:17:18 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:

>> > where it is truly just dead junk. I'd rather just leave the dead junk
>> > than have pointless warnings, if I have to choose one of the two
>> > outcomes.
>>
>> This is a relatively rare case, with an inline function defined by a macro, and
>> I sent a patch for a similar one in 1f318a8bafcf ("modules: mark
>> __inittest/__exittest
>> as __maybe_unused"). I think this is a case where the __maybe_unused
>> annotation is reasonable, though for the other instances of unused inline
>> functions in .c files, there is often a better way: typically the only caller
>> of a function is inside of an #ifdef and moving the inline function definition
>> into the same #ifdef block makes it clearer what is going on.
>>
>
> How many warnings does the TRACE_EVENT() macros produce? That macro
> creates a lot of static inline functions for every event. For example,
> it will create an rcuidle() version of a tracepoint for those locations
> that need to be called where RCU is not watching. It also adds a
> trace_#tracepoint#_enabled() function for those that want to test if
> the tracepoint is enabled or not.
>
> That is, every tracepoint creates static inlined functions for cases
> that are used by only a few. I'm sure they will cause lots of warnings
> because there will be a lot of unused static inlined functions.

The TRACE_EVENT() macro is always invoked from header files, so
clang does not warn about unused instances. The only other header
I found that defines macros that cause unused inline functions is
include/linux/interval_tree_generic.h, but I only saw two warnings
from that in the whole allmodconfig build.

        Arnd

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Arnd Bergmann]

On Tue, Jun 6, 2017 at 11:28 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 6, 2017 at 2:23 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>>
>> I tend to disagree, the warning is useful to detect truly unused
>> static inline functions, which should be removed, rather than be
>> carried around/maintained for often long periods of time.
>
> That may be true in other projects, but we really do have a lot of
> code that is conditionally used. The warning is just not useful.

After going through all the warnings, I don't see the conditionally
used ones as a problem at all, I only found a handful of instances
that actually need an additional __maybe_unused or #ifdef.

> I agree that we could use "__maybe_unused", but at the same time, I
> don't really see the point. There's no way in hell we'd ever do that
> for inlines that are in header files (*of course* they may be unused),
> why would we then have a magical rule like "let's do it for inlines in
> C files".

The main reason I see for it is that a lot of the unused inline functions
in C files are mistakes, while in headers they are more often intentional.
The difference between a 'static' function in a C file and a 'static inline'
function in the same file is very small, epecially with
CONFIG_OPTIMIZE_INLINING, and the choice is often arbitrary.

I did an ARM allmodconfig build with clang to figure out what the actual
numbers, and I found 328 warnings in 160 files, and addressed them
in a throwaway patch at https://pastebin.ca/3830365

I agree we should not turn on the warning for unused inlines
in C files unconditionally with clang, simply because there are
so many files that need patching (I misread the finding in
Matthias' original mail and thought it was way less), and many
drivers end up defining a whole family of inline functions (e.g.
one for each hardware register in a driver) which intentionally
include ones that are unused.

I still think it would be helpful to warn about them with "make W=1"
for people to catch the unintentional cases when they look for them in
their own code, just like we warn for unused "static const" variables.

With an ad-hoc classification of the files I got

- never used anywhere: 141
- conditionally used, would need workaround: 5
- definition should be moved into #ifdef: 4
- inline functions defined by macro, intentionally maybe_unused: 6
- 'inline' should have been '__maybe_unused' instead : 4

I even found one function that should be called but is not:
__ila_hash_secret_init(). This one might be a serious bug,
or it might be harmless.

 [Adding Tom Herbert to Cc here, Tom, please have a look
at net/ipv6/ila/ila_xlat.c for the missing initialization of hashrnd]

       Arnd

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Linus Torvalds]

On Wed, Jun 7, 2017 at 12:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> The main reason I see for it is that a lot of the unused inline functions
> in C files are mistakes,

Bah. Blah blah blah.

The clang warnign doesn't actually really buy us anything, and it's a
completely pointless difference to gcc.

I'm not in the least interested in supporting these kinds of pointless
differences.

The people who are interested in making the kernel compile well with
clang should care about the things that matter, not annoying people
with idiotic patches.

So stop the idiotic patches.  When clang actually adds _value_, that's
one thing. Right now it's just stupid noise.

For some reason compiler people think that "more warnings are good".
No. They are not. More noise without any value is absolutely not good,
and an unused inline function si by definition not something we care
about.

Really. Fit the clang noise. Get clang to generate good code.

Once clang has actually proven itself, and we haev years of clang
under our belt, and clang isn't just a toy and a source of bugs and
pointless warnings as far as kernel builds are concerned, THEN we can
start talking about actually making use of clang features.

Right now it should be about "don't be a f*cking pain in the arse!"

            Linus

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Steven Rostedt]

On Wed, 7 Jun 2017 13:36:27 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Jun 7, 2017 at 12:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > The main reason I see for it is that a lot of the unused inline functions
> > in C files are mistakes,  
> 
> Bah. Blah blah blah.
> 
> The clang warnign doesn't actually really buy us anything, and it's a
> completely pointless difference to gcc.
> 
> I'm not in the least interested in supporting these kinds of pointless
> differences.
> 
> The people who are interested in making the kernel compile well with
> clang should care about the things that matter, not annoying people
> with idiotic patches.
> 
> So stop the idiotic patches.  When clang actually adds _value_, that's
> one thing. Right now it's just stupid noise.
> 
> For some reason compiler people think that "more warnings are good".
> No. They are not. More noise without any value is absolutely not good,
> and an unused inline function si by definition not something we care
> about.
> 
> Really. Fit the clang noise. Get clang to generate good code.
> 
> Once clang has actually proven itself, and we haev years of clang
> under our belt, and clang isn't just a toy and a source of bugs and
> pointless warnings as far as kernel builds are concerned, THEN we can
> start talking about actually making use of clang features.
> 
> Right now it should be about "don't be a f*cking pain in the arse!"
> 

Personally, I don't find the unused static inline function warning that
helpful either. But the only worry I have to totally ignoring them, is
that they could contain buggy code, which may either be cut-and-pasted
into code that is used, or one day used, and then inject buggy code.

But other than that, I pretty much agree with your assessment on this
one.

-- Steve

Re: [RFC] clang: 'unused-function' warning on static inline functions

[Arnd Bergmann]

On Wed, Jun 7, 2017 at 9:43 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 6, 2017 at 11:28 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:

> I even found one function that should be called but is not:
> __ila_hash_secret_init(). This one might be a serious bug,
> or it might be harmless.
>
>  [Adding Tom Herbert to Cc here, Tom, please have a look
> at net/ipv6/ila/ila_xlat.c for the missing initialization of hashrnd]

I submitted a patch for this one now, and another one for a function
in kernel/cpu.c that was obviously meant to be removed not too long
ago. I checked that all other instances of the warning I found in the
kernel are indeed harmless and don't need a patch when there is
no warning.

       Arnd