Message-ID: <CAK8P3a3SOYmCstytN31sSr55GCfrX9avTkKAtohamuVrROFxAA@mail.gmail.com>
Date:   Wed, 7 Jun 2017 21:43:34 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Matthias Kaehlcke <mka@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Christoph Hellwig <hch@....de>, Jens Axboe <axboe@...nel.dk>,
        Steven Rostedt <rostedt@...dmis.org>,
        David Rientjes <rientjes@...gle.com>,
        Douglas Anderson <dianders@...omium.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Mark Brown <broonie@...nel.org>,
        David Miller <davem@...emloft.net>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [RFC] clang: 'unused-function' warning on static inline functions

On Tue, Jun 6, 2017 at 11:28 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Tue, Jun 6, 2017 at 2:23 PM, Matthias Kaehlcke <mka@...omium.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

Powered by blists - more mailing lists