clang: 'unused-function' warning on static inline functions

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

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

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

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

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

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.

To detect dead code?

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?

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

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

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

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?

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

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

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

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.

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

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

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

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