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

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

[David Rientjes]

GCC explicitly does not warn for unused static inline functions for
-Wunused-function.  The manual states:

	Warn whenever a static function is declared but not defined or
	a non-inline static function is unused.

Clang does warn for static inline functions that are unused.

It turns out that suppressing the warnings avoids potentially complex
#ifdef directives, which also reduces LOC.

Supress the warning for clang.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/compiler-clang.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -15,3 +15,10 @@
  * with any version that can compile the kernel
  */
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
+
+/*
+ * GCC does not warn about unused static inline functions for
+ * -Wunused-function.  This turns out to avoid the need for complex #ifdef
+ * directives.  Suppress the warning in clang as well.
+ */
+#define inline inline __attribute__((unused))

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

[David Rientjes]

GCC explicitly does not warn for unused static inline functions for
-Wunused-function.  The manual states:

	Warn whenever a static function is declared but not defined or
	a non-inline static function is unused.

Clang does warn for static inline functions that are unused.

It turns out that suppressing the warnings avoids potentially complex
#ifdef directives, which also reduces LOC.

Supress the warning for clang.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/compiler-clang.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -15,3 +15,10 @@
  * with any version that can compile the kernel
  */
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
+
+/*
+ * GCC does not warn about unused static inline functions for
+ * -Wunused-function.  This turns out to avoid the need for complex #ifdef
+ * directives.  Suppress the warning in clang as well.
+ */
+#define inline inline __attribute__((unused))

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

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

[Matthias Kaehlcke]

El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:

> GCC explicitly does not warn for unused static inline functions for
> -Wunused-function.  The manual states:
> 
> 	Warn whenever a static function is declared but not defined or
> 	a non-inline static function is unused.
> 
> Clang does warn for static inline functions that are unused.
> 
> It turns out that suppressing the warnings avoids potentially complex
> #ifdef directives, which also reduces LOC.
> 
> Supress the warning for clang.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---

As expressed earlier in other threads, I don't think gcc's behavior is
preferable in this case. The warning on static inline functions (only
in .c files) allows to detect truly unused code. About 50% of the
warnings I have looked into so far fall into this category.

In my opinion it is more valuable to detect dead code than not having
a few more __maybe_unused attributes (there aren't really that many
instances, at least with x86 and arm64 defconfig). In most cases it is
not necessary to use #ifdef, it is an option which is preferred by
some maintainers. The reduced LOC is arguable, since dectecting dead
code allows to remove it.

I'm not a kernel maintainer, so it's not my decision whether this
warning should be silenced, my personal opinion is that it's benfits
outweigh the inconveniences of dealing with half-false positives,
generally caused by the heavy use of #ifdef by the kernel itself.

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

[Matthias Kaehlcke]

El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:

> GCC explicitly does not warn for unused static inline functions for
> -Wunused-function.  The manual states:
> 
> 	Warn whenever a static function is declared but not defined or
> 	a non-inline static function is unused.
> 
> Clang does warn for static inline functions that are unused.
> 
> It turns out that suppressing the warnings avoids potentially complex
> #ifdef directives, which also reduces LOC.
> 
> Supress the warning for clang.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---

As expressed earlier in other threads, I don't think gcc's behavior is
preferable in this case. The warning on static inline functions (only
in .c files) allows to detect truly unused code. About 50% of the
warnings I have looked into so far fall into this category.

In my opinion it is more valuable to detect dead code than not having
a few more __maybe_unused attributes (there aren't really that many
instances, at least with x86 and arm64 defconfig). In most cases it is
not necessary to use #ifdef, it is an option which is preferred by
some maintainers. The reduced LOC is arguable, since dectecting dead
code allows to remove it.

I'm not a kernel maintainer, so it's not my decision whether this
warning should be silenced, my personal opinion is that it's benfits
outweigh the inconveniences of dealing with half-false positives,
generally caused by the heavy use of #ifdef by the kernel itself.

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

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

[Andrew Morton]

On Wed, 24 May 2017 14:22:29 -0700 Matthias Kaehlcke <mka@chromium.org> wrote:

> I'm not a kernel maintainer, so it's not my decision whether this
> warning should be silenced, my personal opinion is that it's benfits
> outweigh the inconveniences of dealing with half-false positives,
> generally caused by the heavy use of #ifdef by the kernel itself.

Please resend and include this info in the changelog.  Describe
instances where this warning has resulted in actual runtime or
developer-visible benefits.

Where possible an appropriate I suggest it is better to move the
offending function into a header file, rather than adding ifdefs.

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

[Andrew Morton]

On Wed, 24 May 2017 14:22:29 -0700 Matthias Kaehlcke <mka@chromium.org> wrote:

> I'm not a kernel maintainer, so it's not my decision whether this
> warning should be silenced, my personal opinion is that it's benfits
> outweigh the inconveniences of dealing with half-false positives,
> generally caused by the heavy use of #ifdef by the kernel itself.

Please resend and include this info in the changelog.  Describe
instances where this warning has resulted in actual runtime or
developer-visible benefits.

Where possible an appropriate I suggest it is better to move the
offending function into a header file, rather than adding ifdefs.

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

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

[Doug Anderson]

Hi,

On Wed, May 24, 2017 at 2:32 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 24 May 2017 14:22:29 -0700 Matthias Kaehlcke <mka@chromium.org> wrote:
>
>> I'm not a kernel maintainer, so it's not my decision whether this
>> warning should be silenced, my personal opinion is that it's benfits
>> outweigh the inconveniences of dealing with half-false positives,
>> generally caused by the heavy use of #ifdef by the kernel itself.
>
> Please resend and include this info in the changelog.  Describe
> instances where this warning has resulted in actual runtime or
> developer-visible benefits.
>
> Where possible an appropriate I suggest it is better to move the
> offending function into a header file, rather than adding ifdefs.

Can you clarify what you're asking for here?

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

* Matthias is arguing here _against_ David's patch.


The best I can understand is that you're asking David to add
Matthias's objections into his patch description, then say why we're
still disabling this warning?

---

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


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:

* https://patchwork.kernel.org/patch/9738139/
  mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

We can, of course, discuss the best way to solve each individual
issue.  ...and if we can find a way around #ifdef in most places that
seems ideal.  If people really think the ability to spot dead code is
not important, though, then disabling the warning globally like
David's patch is the way to go.


Note that in addition to spotting some dead code, clang's warnings
also have the ability to identify "paste-o" bugs during development
that would be harder to find if these warnings were disabled.  It's
unlikely problems like this would last long in the kernel, but
certainly I've made paste-o mistakes like this and then spent quite a
while trying to figure out why things weren't working until my eyes
finally spotted my stupidity.  Like:

static inline void its_a_dog(void) {
  pr_info("It's a dog\n");
}

static inline void its_a_cat(void) {
  pr_info("It's a dog\n");
}

static void foo(void) {
  if (strcmp(animal, "cat") == 0) {
    /* It's a cat! */
    its_a_cat();
  } else {
    /* It's a dog! */
   its_a_cat();
  }
}

Clang would (nicely) tell me that its_a_dog() is unused.  This is a
stupid example but I've made this type of mistake in the past for
sure.


-Doug

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

[Doug Anderson]

Hi,

On Wed, May 24, 2017 at 2:32 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 24 May 2017 14:22:29 -0700 Matthias Kaehlcke <mka@chromium.org> wrote:
>
>> I'm not a kernel maintainer, so it's not my decision whether this
>> warning should be silenced, my personal opinion is that it's benfits
>> outweigh the inconveniences of dealing with half-false positives,
>> generally caused by the heavy use of #ifdef by the kernel itself.
>
> Please resend and include this info in the changelog.  Describe
> instances where this warning has resulted in actual runtime or
> developer-visible benefits.
>
> Where possible an appropriate I suggest it is better to move the
> offending function into a header file, rather than adding ifdefs.

Can you clarify what you're asking for here?

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

* Matthias is arguing here _against_ David's patch.


The best I can understand is that you're asking David to add
Matthias's objections into his patch description, then say why we're
still disabling this warning?

---

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


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:

* https://patchwork.kernel.org/patch/9738139/
  mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

We can, of course, discuss the best way to solve each individual
issue.  ...and if we can find a way around #ifdef in most places that
seems ideal.  If people really think the ability to spot dead code is
not important, though, then disabling the warning globally like
David's patch is the way to go.


Note that in addition to spotting some dead code, clang's warnings
also have the ability to identify "paste-o" bugs during development
that would be harder to find if these warnings were disabled.  It's
unlikely problems like this would last long in the kernel, but
certainly I've made paste-o mistakes like this and then spent quite a
while trying to figure out why things weren't working until my eyes
finally spotted my stupidity.  Like:

static inline void its_a_dog(void) {
  pr_info("It's a dog\n");
}

static inline void its_a_cat(void) {
  pr_info("It's a dog\n");
}

static void foo(void) {
  if (strcmp(animal, "cat") == 0) {
    /* It's a cat! */
    its_a_cat();
  } else {
    /* It's a dog! */
   its_a_cat();
  }
}

Clang would (nicely) tell me that its_a_dog() is unused.  This is a
stupid example but I've made this type of mistake in the past for
sure.


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

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

[Ingo Molnar]

* Matthias Kaehlcke <mka@chromium.org> wrote:

> El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:
> 
> > GCC explicitly does not warn for unused static inline functions for
> > -Wunused-function.  The manual states:
> > 
> > 	Warn whenever a static function is declared but not defined or
> > 	a non-inline static function is unused.
> > 
> > Clang does warn for static inline functions that are unused.
> > 
> > It turns out that suppressing the warnings avoids potentially complex
> > #ifdef directives, which also reduces LOC.
> > 
> > Supress the warning for clang.
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> 
> As expressed earlier in other threads, I don't think gcc's behavior is
> preferable in this case. The warning on static inline functions (only
> in .c files) allows to detect truly unused code. About 50% of the
> warnings I have looked into so far fall into this category.
> 
> In my opinion it is more valuable to detect dead code than not having
> a few more __maybe_unused attributes (there aren't really that many
> instances, at least with x86 and arm64 defconfig). In most cases it is
> not necessary to use #ifdef, it is an option which is preferred by
> some maintainers. The reduced LOC is arguable, since dectecting dead
> code allows to remove it.

Static inline functions in headers are often not dead code.

Thanks,

	Ingo

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

[Ingo Molnar]

* Matthias Kaehlcke <mka@chromium.org> wrote:

> El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:
> 
> > GCC explicitly does not warn for unused static inline functions for
> > -Wunused-function.  The manual states:
> > 
> > 	Warn whenever a static function is declared but not defined or
> > 	a non-inline static function is unused.
> > 
> > Clang does warn for static inline functions that are unused.
> > 
> > It turns out that suppressing the warnings avoids potentially complex
> > #ifdef directives, which also reduces LOC.
> > 
> > Supress the warning for clang.
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> 
> As expressed earlier in other threads, I don't think gcc's behavior is
> preferable in this case. The warning on static inline functions (only
> in .c files) allows to detect truly unused code. About 50% of the
> warnings I have looked into so far fall into this category.
> 
> In my opinion it is more valuable to detect dead code than not having
> a few more __maybe_unused attributes (there aren't really that many
> instances, at least with x86 and arm64 defconfig). In most cases it is
> not necessary to use #ifdef, it is an option which is preferred by
> some maintainers. The reduced LOC is arguable, since dectecting dead
> code allows to remove it.

Static inline functions in headers are often not dead code.

Thanks,

	Ingo

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

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

[Matthias Kaehlcke]

El Thu, May 25, 2017 at 07:52:07AM +0200 Ingo Molnar ha dit:

> 
> * Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:
> > 
> > > GCC explicitly does not warn for unused static inline functions for
> > > -Wunused-function.  The manual states:
> > > 
> > > 	Warn whenever a static function is declared but not defined or
> > > 	a non-inline static function is unused.
> > > 
> > > Clang does warn for static inline functions that are unused.
> > > 
> > > It turns out that suppressing the warnings avoids potentially complex
> > > #ifdef directives, which also reduces LOC.
> > > 
> > > Supress the warning for clang.
> > > 
> > > Signed-off-by: David Rientjes <rientjes@google.com>
> > > ---
> > 
> > As expressed earlier in other threads, I don't think gcc's behavior is
> > preferable in this case. The warning on static inline functions (only
> > in .c files) allows to detect truly unused code. About 50% of the
> > warnings I have looked into so far fall into this category.
> > 
> > In my opinion it is more valuable to detect dead code than not having
> > a few more __maybe_unused attributes (there aren't really that many
> > instances, at least with x86 and arm64 defconfig). In most cases it is
> > not necessary to use #ifdef, it is an option which is preferred by
> > some maintainers. The reduced LOC is arguable, since dectecting dead
> > code allows to remove it.
> 
> Static inline functions in headers are often not dead code.

Sure, there is no intention to delete these and clang doesn't raise
warnings about unused static inline functions in headers.

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

[Matthias Kaehlcke]

El Thu, May 25, 2017 at 07:52:07AM +0200 Ingo Molnar ha dit:

> 
> * Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > El Wed, May 24, 2017 at 02:01:15PM -0700 David Rientjes ha dit:
> > 
> > > GCC explicitly does not warn for unused static inline functions for
> > > -Wunused-function.  The manual states:
> > > 
> > > 	Warn whenever a static function is declared but not defined or
> > > 	a non-inline static function is unused.
> > > 
> > > Clang does warn for static inline functions that are unused.
> > > 
> > > It turns out that suppressing the warnings avoids potentially complex
> > > #ifdef directives, which also reduces LOC.
> > > 
> > > Supress the warning for clang.
> > > 
> > > Signed-off-by: David Rientjes <rientjes@google.com>
> > > ---
> > 
> > As expressed earlier in other threads, I don't think gcc's behavior is
> > preferable in this case. The warning on static inline functions (only
> > in .c files) allows to detect truly unused code. About 50% of the
> > warnings I have looked into so far fall into this category.
> > 
> > In my opinion it is more valuable to detect dead code than not having
> > a few more __maybe_unused attributes (there aren't really that many
> > instances, at least with x86 and arm64 defconfig). In most cases it is
> > not necessary to use #ifdef, it is an option which is preferred by
> > some maintainers. The reduced LOC is arguable, since dectecting dead
> > code allows to remove it.
> 
> Static inline functions in headers are often not dead code.

Sure, there is no intention to delete these and clang doesn't raise
warnings about unused static inline functions in headers.

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

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

[Joe Perches]

On Thu, 2017-05-25 at 09:14 -0700, Matthias Kaehlcke wrote:
> clang doesn't raise
> warnings about unused static inline functions in headers.

Is any "#include" file a "header" to clang or only "*.h" files?

For instance:

The kernel has ~500 .c files that other .c files #include.
Are unused inline functions in those .c files reported?

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

[Joe Perches]

On Thu, 2017-05-25 at 09:14 -0700, Matthias Kaehlcke wrote:
> clang doesn't raise
> warnings about unused static inline functions in headers.

Is any "#include" file a "header" to clang or only "*.h" files?

For instance:

The kernel has ~500 .c files that other .c files #include.
Are unused inline functions in those .c files reported?

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

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

[Matthias Kaehlcke]

Hi Joe,

El Thu, May 25, 2017 at 09:48:53AM -0700 Joe Perches ha dit:

> On Thu, 2017-05-25 at 09:14 -0700, Matthias Kaehlcke wrote:
> > clang doesn't raise
> > warnings about unused static inline functions in headers.
> 
> Is any "#include" file a "header" to clang or only "*.h" files?
> 
> For instance:
> 
> The kernel has ~500 .c files that other .c files #include.
> Are unused inline functions in those .c files reported?

Any "#include" file is a "header" to clang, no warnings are generated
for unused inline functions in included .c files.

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

[Matthias Kaehlcke]

Hi Joe,

El Thu, May 25, 2017 at 09:48:53AM -0700 Joe Perches ha dit:

> On Thu, 2017-05-25 at 09:14 -0700, Matthias Kaehlcke wrote:
> > clang doesn't raise
> > warnings about unused static inline functions in headers.
> 
> Is any "#include" file a "header" to clang or only "*.h" files?
> 
> For instance:
> 
> The kernel has ~500 .c files that other .c files #include.
> Are unused inline functions in those .c files reported?

Any "#include" file is a "header" to clang, no warnings are generated
for unused inline functions in included .c files.

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

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

[David Rientjes]

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.

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

[David Rientjes]

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>

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

[Matthias Kaehlcke]

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.

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

[Matthias Kaehlcke]

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>

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@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

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@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>

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

[Mark Brown]

[-- 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 --]

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

[David Rientjes]

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.

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

[David Rientjes]

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>

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

[Doug Anderson]

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

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

[Doug Anderson]

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>

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

[Matthias Kaehlcke]

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.

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

[Matthias Kaehlcke]

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>