On Thu, Jun 1, 2017 at 1:55 AM, Matthias Kaehlcke <[email protected]> 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
On Tue, Jun 6, 2017 at 4:16 AM, Arnd Bergmann <[email protected]> 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
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 <[email protected]> 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?
On Tue, Jun 6, 2017 at 2:23 PM, Matthias Kaehlcke <[email protected]> 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
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 <[email protected]> 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
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.
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?
On Tue, Jun 6, 2017 at 11:29 PM, Jens Axboe <[email protected]> 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 <[email protected]> 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
On Wed, 7 Jun 2017 10:17:18 +0200
Arnd Bergmann <[email protected]> wrote:
> On Tue, Jun 6, 2017 at 11:29 PM, Jens Axboe <[email protected]> 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 <[email protected]> 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
On Wed, Jun 7, 2017 at 2:58 PM, Steven Rostedt <[email protected]> wrote:
> On Wed, 7 Jun 2017 10:17:18 +0200
> Arnd Bergmann <[email protected]> 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
On Tue, Jun 6, 2017 at 11:28 PM, Linus Torvalds
<[email protected]> wrote:
> On Tue, Jun 6, 2017 at 2:23 PM, Matthias Kaehlcke <[email protected]> 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
On Wed, Jun 7, 2017 at 12:43 PM, Arnd Bergmann <[email protected]> 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
On Wed, 7 Jun 2017 13:36:27 -0700
Linus Torvalds <[email protected]> wrote:
> On Wed, Jun 7, 2017 at 12:43 PM, Arnd Bergmann <[email protected]> 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
On Wed, Jun 7, 2017 at 9:43 PM, Arnd Bergmann <[email protected]> wrote:
> On Tue, Jun 6, 2017 at 11:28 PM, Linus Torvalds
> <[email protected]> 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