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

[Doug Anderson]

Hi,

On Tue, May 30, 2017 at 5:10 PM, David Rientjes <rientjes@xxxxxxxxxx> wrote:
> 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.
>
> 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.

As Matthias said, let's not argue about ifdeffs and instead talk about
adding "maybe unused".  100% of these cases _can_ be solved by adding
"maybe unused".  Then, if a maintainer thinks that an ifdef is cleaner
/ better in a particular case, we can use an ifdef in that case.

Do you believe that adding "maybe unused" tags significantly uglifies
the code?  I personally find them documenting.


>> 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).

What you're suggesting here is that we don't land any "maybe unused"
tags and don't land any ifdeffs.  Instead, it would be the burden of
the person who is running this tool to ignore the false positives and
just provide patches which remove dead code.

It is certainly possible that something like this could be done (I
think Coverity works something like this), but I'm not sure there are
any volunteers.  Doing this would require a person to setup and
monitor a clang builder and then setup a list of false positives.  For
each new warning this person would need to analyze the warning and
either send a patch or add it to the list of false positives.

If, instead, we make it easy for everyone running with clang (yes, not
too many) to notice the errors then we spread the burden out.

Given that adding "maybe unused" (IMHO) doesn't uglify the code and
the total number of patches needed is small, it seems like that's a
good way to go.


-Doug