* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-05-31 0:10 ` David Rientjes
0 siblings, 0 replies; 29+ messages in thread
From: David Rientjes @ 2017-05-31 0:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
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.
> 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).
> 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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 0:10 ` David Rientjes
@ 2017-05-31 1:53 ` Matthias Kaehlcke
-1 siblings, 0 replies; 29+ messages in thread
From: Matthias Kaehlcke @ 2017-05-31 1:53 UTC (permalink / raw)
To: David Rientjes
Cc: Doug Anderson, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel, Mark Brown, Ingo Molnar,
David Miller
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-05-31 1:53 ` Matthias Kaehlcke
0 siblings, 0 replies; 29+ messages in thread
From: Matthias Kaehlcke @ 2017-05-31 1:53 UTC (permalink / raw)
To: David Rientjes
Cc: Doug Anderson, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel, Mark Brown, Ingo Molnar,
David Miller
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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 0:10 ` David Rientjes
@ 2017-05-31 15:53 ` Doug Anderson
-1 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2017-05-31 15:53 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
Hi,
On Tue, May 30, 2017 at 5:10 PM, David Rientjes <rientjes@google.com> 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-05-31 15:53 ` Doug Anderson
0 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2017-05-31 15:53 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
Hi,
On Tue, May 30, 2017 at 5:10 PM, David Rientjes <rientjes@google.com> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 15:53 ` Doug Anderson
(?)
@ 2017-05-31 18:26 ` Mark Brown
-1 siblings, 0 replies; 29+ messages in thread
From: Mark Brown @ 2017-05-31 18:26 UTC (permalink / raw)
To: Doug Anderson
Cc: David Rientjes, Andrew Morton, Matthias Kaehlcke,
Christoph Lameter, Pekka Enberg, Joonsoo Kim, linux-mm,
linux-kernel, Ingo Molnar, David Miller
[-- Attachment #1: Type: text/plain, Size: 647 bytes --]
On Wed, May 31, 2017 at 08:53:40AM -0700, Doug Anderson wrote:
> 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.
It also means setting up some mechanism for distributing the blacklist
or that that every individual person or group doing clang stuff would
need to replicate the work.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 15:53 ` Doug Anderson
@ 2017-05-31 21:45 ` David Rientjes
-1 siblings, 0 replies; 29+ messages in thread
From: David Rientjes @ 2017-05-31 21:45 UTC (permalink / raw)
To: Doug Anderson
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
On Wed, 31 May 2017, Doug Anderson wrote:
> > 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.
>
But then you've eliminated the possibility of finding dead code again,
which is the only point to the warning :) So now we have patches going to
swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
__maybe_unused throughout the code to not increase LOC in select areas but
then we can't find dead code again.
My suggestion is to match gcc behavior and if anybody is in the business
of cleaning up truly dead code, send patches. Tools exist to do this
outside of relying on a minority compiler during compilation. Otherwise,
this is simply adding more burden to already swamped maintainers to
annotate every single static inline function that clang complains about.
I'd prefer to let them decide and this will be the extent of my
participation in this thread.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-05-31 21:45 ` David Rientjes
0 siblings, 0 replies; 29+ messages in thread
From: David Rientjes @ 2017-05-31 21:45 UTC (permalink / raw)
To: Doug Anderson
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
On Wed, 31 May 2017, Doug Anderson wrote:
> > 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.
>
But then you've eliminated the possibility of finding dead code again,
which is the only point to the warning :) So now we have patches going to
swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
__maybe_unused throughout the code to not increase LOC in select areas but
then we can't find dead code again.
My suggestion is to match gcc behavior and if anybody is in the business
of cleaning up truly dead code, send patches. Tools exist to do this
outside of relying on a minority compiler during compilation. Otherwise,
this is simply adding more burden to already swamped maintainers to
annotate every single static inline function that clang complains about.
I'd prefer to let them decide and this will be the extent of my
participation in this thread.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 21:45 ` David Rientjes
@ 2017-05-31 22:31 ` Doug Anderson
-1 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2017-05-31 22:31 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
Hi,
On Wed, May 31, 2017 at 2:45 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 31 May 2017, Doug Anderson wrote:
>
>> > 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.
>>
>
> But then you've eliminated the possibility of finding dead code again,
> which is the only point to the warning :) So now we have patches going to
> swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
> __maybe_unused throughout the code to not increase LOC in select areas but
> then we can't find dead code again.
True, once code is marked __maybe_unused once then it can no longer be
found later, even if it becomes completely dead. ...but this is also
no different for __maybe_unused code that is _not_ marked as "static
inline".
Basically, my argument here is that "static inline" functions in ".c"
files should not be treated differently than "static" functions in
".c" files. We have always agreed to add __maybe_unused for "static"
functions.
Also: allowing us to leave the warning turned on (and have no false
positives reported) mean that, as new code is added we'll be able to
find problems in the new code. This is where it's most important.
> My suggestion is to match gcc behavior and if anybody is in the business
> of cleaning up truly dead code, send patches. Tools exist to do this
> outside of relying on a minority compiler during compilation. Otherwise,
> this is simply adding more burden to already swamped maintainers to
> annotate every single static inline function that clang complains about.
> I'd prefer to let them decide and this will be the extent of my
> participation in this thread.
Maybe Matthias can give actual stats here, but I think "every single"
overstates the issue a bit. I get the impression we're talking
something like ~30-40 patches that don't actually delete dead code and
just add "maybe unused". Given that nobody has ever looked for these
functions before, I'd expect that new code is unlikely to cause a new
deluge of patches.
Note also that Matthias started a bigger thread to discuss this
(subject: [RFC] clang: 'unused-function' warning on static inline
functions). Maybe we should move the discussion there so it's not so
scattered?
-Doug
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-05-31 22:31 ` Doug Anderson
0 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2017-05-31 22:31 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Matthias Kaehlcke, Christoph Lameter,
Pekka Enberg, Joonsoo Kim, linux-mm, linux-kernel, Mark Brown,
Ingo Molnar, David Miller
Hi,
On Wed, May 31, 2017 at 2:45 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 31 May 2017, Doug Anderson wrote:
>
>> > 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.
>>
>
> But then you've eliminated the possibility of finding dead code again,
> which is the only point to the warning :) So now we have patches going to
> swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
> __maybe_unused throughout the code to not increase LOC in select areas but
> then we can't find dead code again.
True, once code is marked __maybe_unused once then it can no longer be
found later, even if it becomes completely dead. ...but this is also
no different for __maybe_unused code that is _not_ marked as "static
inline".
Basically, my argument here is that "static inline" functions in ".c"
files should not be treated differently than "static" functions in
".c" files. We have always agreed to add __maybe_unused for "static"
functions.
Also: allowing us to leave the warning turned on (and have no false
positives reported) mean that, as new code is added we'll be able to
find problems in the new code. This is where it's most important.
> My suggestion is to match gcc behavior and if anybody is in the business
> of cleaning up truly dead code, send patches. Tools exist to do this
> outside of relying on a minority compiler during compilation. Otherwise,
> this is simply adding more burden to already swamped maintainers to
> annotate every single static inline function that clang complains about.
> I'd prefer to let them decide and this will be the extent of my
> participation in this thread.
Maybe Matthias can give actual stats here, but I think "every single"
overstates the issue a bit. I get the impression we're talking
something like ~30-40 patches that don't actually delete dead code and
just add "maybe unused". Given that nobody has ever looked for these
functions before, I'd expect that new code is unlikely to cause a new
deluge of patches.
Note also that Matthias started a bigger thread to discuss this
(subject: [RFC] clang: 'unused-function' warning on static inline
functions). Maybe we should move the discussion there so it's not so
scattered?
-Doug
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
2017-05-31 22:31 ` Doug Anderson
@ 2017-06-01 0:01 ` Matthias Kaehlcke
-1 siblings, 0 replies; 29+ messages in thread
From: Matthias Kaehlcke @ 2017-06-01 0:01 UTC (permalink / raw)
To: Doug Anderson
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel, Mark Brown, Ingo Molnar,
David Miller
El Wed, May 31, 2017 at 03:31:26PM -0700 Doug Anderson ha dit:
> Hi,
>
> On Wed, May 31, 2017 at 2:45 PM, David Rientjes <rientjes@google.com> wrote:
> > On Wed, 31 May 2017, Doug Anderson wrote:
> >
> >> > 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.
> >>
> >
> > But then you've eliminated the possibility of finding dead code again,
> > which is the only point to the warning :) So now we have patches going to
> > swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
> > __maybe_unused throughout the code to not increase LOC in select areas but
> > then we can't find dead code again.
>
> True, once code is marked __maybe_unused once then it can no longer be
> found later, even if it becomes completely dead. ...but this is also
> no different for __maybe_unused code that is _not_ marked as "static
> inline".
>
> Basically, my argument here is that "static inline" functions in ".c"
> files should not be treated differently than "static" functions in
> ".c" files. We have always agreed to add __maybe_unused for "static"
> functions.
>
> Also: allowing us to leave the warning turned on (and have no false
> positives reported) mean that, as new code is added we'll be able to
> find problems in the new code. This is where it's most important.
>
>
> > My suggestion is to match gcc behavior and if anybody is in the business
> > of cleaning up truly dead code, send patches. Tools exist to do this
> > outside of relying on a minority compiler during compilation. Otherwise,
> > this is simply adding more burden to already swamped maintainers to
> > annotate every single static inline function that clang complains about.
> > I'd prefer to let them decide and this will be the extent of my
> > participation in this thread.
>
> Maybe Matthias can give actual stats here, but I think "every single"
> overstates the issue a bit. I get the impression we're talking
> something like ~30-40 patches that don't actually delete dead code and
> just add "maybe unused". Given that nobody has ever looked for these
> functions before, I'd expect that new code is unlikely to cause a new
> deluge of patches.
>
> Note also that Matthias started a bigger thread to discuss this
> (subject: [RFC] clang: 'unused-function' warning on static inline
> functions). Maybe we should move the discussion there so it's not so
> scattered?
I sent a list of instances from x86 and arm64 defconfig to the RFC
thread. For these configs its 25 instances distributed over different
subsystems, the number of patches would likely be around 20, since in
some cases multiple warnings can be addressed in a single patch.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] compiler, clang: suppress warning for unused static inline functions
@ 2017-06-01 0:01 ` Matthias Kaehlcke
0 siblings, 0 replies; 29+ messages in thread
From: Matthias Kaehlcke @ 2017-06-01 0:01 UTC (permalink / raw)
To: Doug Anderson
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel, Mark Brown, Ingo Molnar,
David Miller
El Wed, May 31, 2017 at 03:31:26PM -0700 Doug Anderson ha dit:
> Hi,
>
> On Wed, May 31, 2017 at 2:45 PM, David Rientjes <rientjes@google.com> wrote:
> > On Wed, 31 May 2017, Doug Anderson wrote:
> >
> >> > 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.
> >>
> >
> > But then you've eliminated the possibility of finding dead code again,
> > which is the only point to the warning :) So now we have patches going to
> > swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle
> > __maybe_unused throughout the code to not increase LOC in select areas but
> > then we can't find dead code again.
>
> True, once code is marked __maybe_unused once then it can no longer be
> found later, even if it becomes completely dead. ...but this is also
> no different for __maybe_unused code that is _not_ marked as "static
> inline".
>
> Basically, my argument here is that "static inline" functions in ".c"
> files should not be treated differently than "static" functions in
> ".c" files. We have always agreed to add __maybe_unused for "static"
> functions.
>
> Also: allowing us to leave the warning turned on (and have no false
> positives reported) mean that, as new code is added we'll be able to
> find problems in the new code. This is where it's most important.
>
>
> > My suggestion is to match gcc behavior and if anybody is in the business
> > of cleaning up truly dead code, send patches. Tools exist to do this
> > outside of relying on a minority compiler during compilation. Otherwise,
> > this is simply adding more burden to already swamped maintainers to
> > annotate every single static inline function that clang complains about.
> > I'd prefer to let them decide and this will be the extent of my
> > participation in this thread.
>
> Maybe Matthias can give actual stats here, but I think "every single"
> overstates the issue a bit. I get the impression we're talking
> something like ~30-40 patches that don't actually delete dead code and
> just add "maybe unused". Given that nobody has ever looked for these
> functions before, I'd expect that new code is unlikely to cause a new
> deluge of patches.
>
> Note also that Matthias started a bigger thread to discuss this
> (subject: [RFC] clang: 'unused-function' warning on static inline
> functions). Maybe we should move the discussion there so it's not so
> scattered?
I sent a list of instances from x86 and arm64 defconfig to the RFC
thread. For these configs its 25 instances distributed over different
subsystems, the number of patches would likely be around 20, since in
some cases multiple warnings can be addressed in a single patch.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 29+ messages in thread