linux-kernel - Re: [patch] compiler, clang: suppress warning for unused static inline functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170531015324.GW141096@google.com>
Date:   Tue, 30 May 2017 18:53:24 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     David Rientjes <rientjes@...gle.com>
Cc:     Doug Anderson <dianders@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        Joonsoo Kim <iamjoonsoo.kim@....com>, linux-mm@...ck.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [patch] compiler, clang: suppress warning for unused static
 inline functions

El Tue, May 30, 2017 at 05:10:10PM -0700 David Rientjes ha dit:

> On Wed, 24 May 2017, Doug Anderson wrote:
> 
> > * Matthias has been sending out individual patches that take each
> > particular case into account to try to remove the warnings.  In some
> > cases this removes totally dead code.  In other cases this adds
> > __maybe_unused.  ...and as a last resort it uses #ifdef.  In each of
> > these individual patches we wouldn't want a list of all other patches,
> > I think.
> > 
> 
> Again, I defer to maintainers like Andrew and Ingo who have to deal with 
> an enormous amount of patches on how they would like to handle it; I don't 
> think myself or anybody else who doesn't deal with a large number of 
> patches should be mandating how it's handled.

Nobody is mandating anything, Doug and I are merely expressing our
POVs, trying to convince maintainers about the benefits of keeping the
warning enabled. If the final outcome is to suppress the warning, so
be it.

> For reference, the patchset that this patch originated from added 8 lines 
> and removed 1, so I disagree that this cleans anything up; in reality, it 
> obfuscates the code and makes the #ifdef nesting more complex.

On the risk of sounding like a broken record: In almost all cases
the use of #ifdef is an option and __maybe_unused can be used
instead. I got feedback from some maintainers that they preferred
using #ifdef, so I used it in instances where it seemed reasonable.

> > If you just want a list of things in response to this thread...
> > 
> > Clang's behavior has found some dead code, as shown by:
> > 
> > * https://patchwork.kernel.org/patch/9732161/
> >   ring-buffer: Remove unused function __rb_data_page_index()
> > * https://patchwork.kernel.org/patch/9735027/
> >   r8152: Remove unused function usb_ocp_read()
> > * https://patchwork.kernel.org/patch/9735053/
> >   net1080: Remove unused function nc_dump_ttl()
> > * https://patchwork.kernel.org/patch/9741513/
> >   crypto: rng: Remove unused function __crypto_rng_cast()
> > * https://patchwork.kernel.org/patch/9741539/
> >   x86/ioapic: Remove unused function IO_APIC_irq_trigger()
> > * https://patchwork.kernel.org/patch/9741549/
> >   ASoC: Intel: sst: Remove unused function sst_restore_shim64()
> > * https://patchwork.kernel.org/patch/9743225/
> >   ASoC: cht_bsw_max98090_ti: Remove unused function cht_get_codec_dai()
> > 
> > ...plus more examples...
> > 
> 
> Let us please not confuse the matter by suggesting that you cannot 
> continue to do this work by simply removing the __attribute__((unused)) 
> and emailing kernel-janitors to cleanup unused code (which should already 
> be significantly small by the sheer fact that it is inlined).

The cleanup of current dead code isn't the primary goal here, the
warning could help to prevent more unused code (and stupid errors as
outlined by Doug) to creep in the kernel. Ideally these would be caught
by automated builds like http://kerneltests.org/.

> > However, clang's behavior has also led to patches that add a
> > "__maybe_unused" attribute (usually no increase in LOC unless it
> > causes word wrap) and also added a handful of #ifdefs, as you've
> > pointed out.  The example we already talked about was:
> > 
> 
> The good work to remove truly dead code may easily continue while not 
> adding more and more LOC to suppress these warnings for a compiler that is 
> very heavily in the minority.

The LOC argument in its literal sense does not apply in many cases,
where adding __maybe_unused doesn't introduce a line wrap.

Again, it's not so much a question of suppressing warnings for a
minority compiler, but keeping a useful tool at a seemingly reasonable
cost.