linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthias Kaehlcke <mka@chromium.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
	Steven Rostedt <rostedt@goodmis.org>,
	David Rientjes <rientjes@google.com>,
	Douglas Anderson <dianders@chromium.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Mark Brown <broonie@kernel.org>,
	David Miller <davem@davemloft.net>,
	Tom Herbert <tom@herbertland.com>
Subject: Re: [RFC] clang: 'unused-function' warning on static inline functions
Date: Wed, 7 Jun 2017 21:43:34 +0200	[thread overview]
Message-ID: <CAK8P3a3SOYmCstytN31sSr55GCfrX9avTkKAtohamuVrROFxAA@mail.gmail.com> (raw)
In-Reply-To: <CA+55aFzzcz9p7c_VS2JDd-1735Yfb8Mgu6YJTYnFAY2KSiW5gA@mail.gmail.com>

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

  parent reply	other threads:[~2017-06-07 19:43 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK8P3a3SOYmCstytN31sSr55GCfrX9avTkKAtohamuVrROFxAA@mail.gmail.com \
    --to=arnd@arndb.de \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=broonie@kernel.org \
    --cc=davem@davemloft.net \
    --cc=dianders@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@lst.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mingo@kernel.org \
    --cc=mka@chromium.org \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=tom@herbertland.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).