* [RFC] clang: 'unused-function' warning on static inline functions @ 2017-05-30 18:13 Matthias Kaehlcke 2017-05-31 23:55 ` Matthias Kaehlcke 0 siblings, 1 reply; 16+ messages in thread From: Matthias Kaehlcke @ 2017-05-30 18:13 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-05-30 18:13 [RFC] clang: 'unused-function' warning on static inline functions Matthias Kaehlcke @ 2017-05-31 23:55 ` Matthias Kaehlcke 2017-06-06 11:16 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Matthias Kaehlcke @ 2017-05-31 23:55 UTC (permalink / raw) To: linux-kernel Cc: Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Linus Torvalds, Guenter Roeck, Mark Brown, David Miller 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-05-31 23:55 ` Matthias Kaehlcke @ 2017-06-06 11:16 ` Arnd Bergmann 2017-06-06 16:32 ` Linus Torvalds 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-06-06 11:16 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Linus Torvalds, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 11:16 ` Arnd Bergmann @ 2017-06-06 16:32 ` Linus Torvalds 2017-06-06 21:23 ` Matthias Kaehlcke 0 siblings, 1 reply; 16+ messages in thread From: Linus Torvalds @ 2017-06-06 16:32 UTC (permalink / raw) To: Arnd Bergmann Cc: Matthias Kaehlcke, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 16:32 ` Linus Torvalds @ 2017-06-06 21:23 ` Matthias Kaehlcke 2017-06-06 21:28 ` Linus Torvalds 2017-06-06 21:29 ` Jens Axboe 0 siblings, 2 replies; 16+ messages in thread From: Matthias Kaehlcke @ 2017-06-06 21:23 UTC (permalink / raw) To: Linus Torvalds Cc: Arnd Bergmann, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 21:23 ` Matthias Kaehlcke @ 2017-06-06 21:28 ` Linus Torvalds 2017-06-07 0:28 ` Matthias Kaehlcke 2017-06-07 19:43 ` Arnd Bergmann 2017-06-06 21:29 ` Jens Axboe 1 sibling, 2 replies; 16+ messages in thread From: Linus Torvalds @ 2017-06-06 21:28 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Arnd Bergmann, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 21:28 ` Linus Torvalds @ 2017-06-07 0:28 ` Matthias Kaehlcke 2017-06-07 5:59 ` David Rientjes 2017-06-07 19:43 ` Arnd Bergmann 1 sibling, 1 reply; 16+ messages in thread From: Matthias Kaehlcke @ 2017-06-07 0:28 UTC (permalink / raw) To: Linus Torvalds Cc: Arnd Bergmann, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 0:28 ` Matthias Kaehlcke @ 2017-06-07 5:59 ` David Rientjes 0 siblings, 0 replies; 16+ messages in thread From: David Rientjes @ 2017-06-07 5:59 UTC (permalink / raw) To: Linus Torvalds, Matthias Kaehlcke Cc: Arnd Bergmann, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 21:28 ` Linus Torvalds 2017-06-07 0:28 ` Matthias Kaehlcke @ 2017-06-07 19:43 ` Arnd Bergmann 2017-06-07 20:36 ` Linus Torvalds 2017-06-08 8:59 ` Arnd Bergmann 1 sibling, 2 replies; 16+ messages in thread From: Arnd Bergmann @ 2017-06-07 19:43 UTC (permalink / raw) To: Linus Torvalds Cc: Matthias Kaehlcke, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller, Tom Herbert 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 19:43 ` Arnd Bergmann @ 2017-06-07 20:36 ` Linus Torvalds 2017-06-07 21:24 ` Steven Rostedt 2017-06-08 8:59 ` Arnd Bergmann 1 sibling, 1 reply; 16+ messages in thread From: Linus Torvalds @ 2017-06-07 20:36 UTC (permalink / raw) To: Arnd Bergmann Cc: Matthias Kaehlcke, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller, Tom Herbert 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 20:36 ` Linus Torvalds @ 2017-06-07 21:24 ` Steven Rostedt 0 siblings, 0 replies; 16+ messages in thread From: Steven Rostedt @ 2017-06-07 21:24 UTC (permalink / raw) To: Linus Torvalds Cc: Arnd Bergmann, Matthias Kaehlcke, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller, Tom Herbert 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 19:43 ` Arnd Bergmann 2017-06-07 20:36 ` Linus Torvalds @ 2017-06-08 8:59 ` Arnd Bergmann 1 sibling, 0 replies; 16+ messages in thread From: Arnd Bergmann @ 2017-06-08 8:59 UTC (permalink / raw) To: Linus Torvalds Cc: Matthias Kaehlcke, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Jens Axboe, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller, Tom Herbert 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 21:23 ` Matthias Kaehlcke 2017-06-06 21:28 ` Linus Torvalds @ 2017-06-06 21:29 ` Jens Axboe 2017-06-07 8:17 ` Arnd Bergmann 1 sibling, 1 reply; 16+ messages in thread From: Jens Axboe @ 2017-06-06 21:29 UTC (permalink / raw) To: Matthias Kaehlcke, Linus Torvalds Cc: Arnd Bergmann, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-06 21:29 ` Jens Axboe @ 2017-06-07 8:17 ` Arnd Bergmann 2017-06-07 12:58 ` Steven Rostedt 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-06-07 8:17 UTC (permalink / raw) To: Jens Axboe Cc: Matthias Kaehlcke, Linus Torvalds, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, Steven Rostedt, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 8:17 ` Arnd Bergmann @ 2017-06-07 12:58 ` Steven Rostedt 2017-06-07 13:12 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Steven Rostedt @ 2017-06-07 12:58 UTC (permalink / raw) To: Arnd Bergmann Cc: Jens Axboe, Matthias Kaehlcke, Linus Torvalds, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] clang: 'unused-function' warning on static inline functions 2017-06-07 12:58 ` Steven Rostedt @ 2017-06-07 13:12 ` Arnd Bergmann 0 siblings, 0 replies; 16+ messages in thread From: Arnd Bergmann @ 2017-06-07 13:12 UTC (permalink / raw) To: Steven Rostedt Cc: Jens Axboe, Matthias Kaehlcke, Linus Torvalds, Linux Kernel Mailing List, Andrew Morton, Greg Kroah-Hartman, Ingo Molnar, Thomas Gleixner, Christoph Hellwig, David Rientjes, Douglas Anderson, Guenter Roeck, Mark Brown, David Miller 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-06-08 8:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-30 18:13 [RFC] clang: 'unused-function' warning on static inline functions Matthias Kaehlcke 2017-05-31 23:55 ` Matthias Kaehlcke 2017-06-06 11:16 ` Arnd Bergmann 2017-06-06 16:32 ` Linus Torvalds 2017-06-06 21:23 ` Matthias Kaehlcke 2017-06-06 21:28 ` Linus Torvalds 2017-06-07 0:28 ` Matthias Kaehlcke 2017-06-07 5:59 ` David Rientjes 2017-06-07 19:43 ` Arnd Bergmann 2017-06-07 20:36 ` Linus Torvalds 2017-06-07 21:24 ` Steven Rostedt 2017-06-08 8:59 ` Arnd Bergmann 2017-06-06 21:29 ` Jens Axboe 2017-06-07 8:17 ` Arnd Bergmann 2017-06-07 12:58 ` Steven Rostedt 2017-06-07 13:12 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).