[PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[Andy Shevchenko]

When flag_is_changeable_p() is unused, it prevents kernel builds
with clang, `make W=1` and CONFIG_WERROR=y:

arch/x86/kernel/cpu/common.c:351:19: error: unused function 'flag_is_changeable_p' [-Werror,-Wunused-function]
  351 | static inline int flag_is_changeable_p(u32 flag)
      |                   ^~~~~~~~~~~~~~~~~~~~

Fix this by moving core around to make sure flag_is_changeable_p() is
always being used.

See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
inline functions for W=1 build").

While at it, fix the argument type to be unsigned long along with
the local variables, although it currently only runs in 32-bit cases.
Besides that, makes it return boolean instead of int. This induces
the change of the returning type of have_cpuid_p() to be boolean
as well.

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v5: fixed type of the local vars (hpa), made it return bool (hpa)
v4: fixed the type of parameter (hpa)
v3: rewritten as suggested (Dave)
v2: marked both 32- and 64-bit cases

 arch/x86/include/asm/cpuid.h |  8 +++++---
 arch/x86/kernel/cpu/common.c | 39 +++++++++++++++++-------------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
index ca4243318aad..239b9ba5c398 100644
--- a/arch/x86/include/asm/cpuid.h
+++ b/arch/x86/include/asm/cpuid.h
@@ -6,6 +6,8 @@
 #ifndef _ASM_X86_CPUID_H
 #define _ASM_X86_CPUID_H
 
+#include <linux/types.h>
+
 #include <asm/string.h>
 
 struct cpuid_regs {
@@ -20,11 +22,11 @@ enum cpuid_regs_idx {
 };
 
 #ifdef CONFIG_X86_32
-extern int have_cpuid_p(void);
+bool have_cpuid_p(void);
 #else
-static inline int have_cpuid_p(void)
+static inline bool have_cpuid_p(void)
 {
-	return 1;
+	return true;
 }
 #endif
 static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a5f221ea5688..f361336416ae 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -276,21 +276,13 @@ static int __init x86_noinvpcid_setup(char *s)
 }
 early_param("noinvpcid", x86_noinvpcid_setup);
 
-#ifdef CONFIG_X86_32
-static int cachesize_override = -1;
-static int disable_x86_serial_nr = 1;
-
-static int __init cachesize_setup(char *str)
-{
-	get_option(&str, &cachesize_override);
-	return 1;
-}
-__setup("cachesize=", cachesize_setup);
-
 /* Standard macro to see if a specific flag is changeable */
-static inline int flag_is_changeable_p(u32 flag)
+static inline bool flag_is_changeable_p(unsigned long flag)
 {
-	u32 f1, f2;
+	unsigned long f1, f2;
+
+	if (!IS_ENABLED(CONFIG_X86_32))
+		return true;
 
 	/*
 	 * Cyrix and IDT cpus allow disabling of CPUID
@@ -313,11 +305,22 @@ static inline int flag_is_changeable_p(u32 flag)
 		      : "=&r" (f1), "=&r" (f2)
 		      : "ir" (flag));
 
-	return ((f1^f2) & flag) != 0;
+	return (f1 ^ f2) & flag;
 }
 
+#ifdef CONFIG_X86_32
+static int cachesize_override = -1;
+static int disable_x86_serial_nr = 1;
+
+static int __init cachesize_setup(char *str)
+{
+	get_option(&str, &cachesize_override);
+	return 1;
+}
+__setup("cachesize=", cachesize_setup);
+
 /* Probe for the CPUID instruction */
-int have_cpuid_p(void)
+bool have_cpuid_p(void)
 {
 	return flag_is_changeable_p(X86_EFLAGS_ID);
 }
@@ -349,10 +352,6 @@ static int __init x86_serial_nr_setup(char *s)
 }
 __setup("serialnumber", x86_serial_nr_setup);
 #else
-static inline int flag_is_changeable_p(u32 flag)
-{
-	return 1;
-}
 static inline void squash_the_stupid_serial_number(struct cpuinfo_x86 *c)
 {
 }
@@ -1088,7 +1087,6 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
 
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
 {
-#ifdef CONFIG_X86_32
 	int i;
 
 	/*
@@ -1109,7 +1107,6 @@ static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
 				break;
 			}
 		}
-#endif
 }
 
 #define NO_SPECULATION		BIT(0)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[H. Peter Anvin]

On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>When flag_is_changeable_p() is unused, it prevents kernel builds
>with clang, `make W=1` and CONFIG_WERROR=y:
>
>arch/x86/kernel/cpu/common.c:351:19: error: unused function 'flag_is_changeable_p' [-Werror,-Wunused-function]
>  351 | static inline int flag_is_changeable_p(u32 flag)
>      |                   ^~~~~~~~~~~~~~~~~~~~
>
>Fix this by moving core around to make sure flag_is_changeable_p() is
>always being used.
>
>See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
>inline functions for W=1 build").
>
>While at it, fix the argument type to be unsigned long along with
>the local variables, although it currently only runs in 32-bit cases.
>Besides that, makes it return boolean instead of int. This induces
>the change of the returning type of have_cpuid_p() to be boolean
>as well.
>
>Suggested-by: Dave Hansen <dave.hansen@intel.com>
>Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>---
>
>v5: fixed type of the local vars (hpa), made it return bool (hpa)
>v4: fixed the type of parameter (hpa)
>v3: rewritten as suggested (Dave)
>v2: marked both 32- and 64-bit cases
>
> arch/x86/include/asm/cpuid.h |  8 +++++---
> arch/x86/kernel/cpu/common.c | 39 +++++++++++++++++-------------------
> 2 files changed, 23 insertions(+), 24 deletions(-)
>
>diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
>index ca4243318aad..239b9ba5c398 100644
>--- a/arch/x86/include/asm/cpuid.h
>+++ b/arch/x86/include/asm/cpuid.h
>@@ -6,6 +6,8 @@
> #ifndef _ASM_X86_CPUID_H
> #define _ASM_X86_CPUID_H
> 
>+#include <linux/types.h>
>+
> #include <asm/string.h>
> 
> struct cpuid_regs {
>@@ -20,11 +22,11 @@ enum cpuid_regs_idx {
> };
> 
> #ifdef CONFIG_X86_32
>-extern int have_cpuid_p(void);
>+bool have_cpuid_p(void);
> #else
>-static inline int have_cpuid_p(void)
>+static inline bool have_cpuid_p(void)
> {
>-	return 1;
>+	return true;
> }
> #endif
> static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
>diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>index a5f221ea5688..f361336416ae 100644
>--- a/arch/x86/kernel/cpu/common.c
>+++ b/arch/x86/kernel/cpu/common.c
>@@ -276,21 +276,13 @@ static int __init x86_noinvpcid_setup(char *s)
> }
> early_param("noinvpcid", x86_noinvpcid_setup);
> 
>-#ifdef CONFIG_X86_32
>-static int cachesize_override = -1;
>-static int disable_x86_serial_nr = 1;
>-
>-static int __init cachesize_setup(char *str)
>-{
>-	get_option(&str, &cachesize_override);
>-	return 1;
>-}
>-__setup("cachesize=", cachesize_setup);
>-
> /* Standard macro to see if a specific flag is changeable */
>-static inline int flag_is_changeable_p(u32 flag)
>+static inline bool flag_is_changeable_p(unsigned long flag)
> {
>-	u32 f1, f2;
>+	unsigned long f1, f2;
>+
>+	if (!IS_ENABLED(CONFIG_X86_32))
>+		return true;
> 
> 	/*
> 	 * Cyrix and IDT cpus allow disabling of CPUID
>@@ -313,11 +305,22 @@ static inline int flag_is_changeable_p(u32 flag)
> 		      : "=&r" (f1), "=&r" (f2)
> 		      : "ir" (flag));
> 
>-	return ((f1^f2) & flag) != 0;
>+	return (f1 ^ f2) & flag;
> }
> 
>+#ifdef CONFIG_X86_32
>+static int cachesize_override = -1;
>+static int disable_x86_serial_nr = 1;
>+
>+static int __init cachesize_setup(char *str)
>+{
>+	get_option(&str, &cachesize_override);
>+	return 1;
>+}
>+__setup("cachesize=", cachesize_setup);
>+
> /* Probe for the CPUID instruction */
>-int have_cpuid_p(void)
>+bool have_cpuid_p(void)
> {
> 	return flag_is_changeable_p(X86_EFLAGS_ID);
> }
>@@ -349,10 +352,6 @@ static int __init x86_serial_nr_setup(char *s)
> }
> __setup("serialnumber", x86_serial_nr_setup);
> #else
>-static inline int flag_is_changeable_p(u32 flag)
>-{
>-	return 1;
>-}
> static inline void squash_the_stupid_serial_number(struct cpuinfo_x86 *c)
> {
> }
>@@ -1088,7 +1087,6 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
> 
> static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
> {
>-#ifdef CONFIG_X86_32
> 	int i;
> 
> 	/*
>@@ -1109,7 +1107,6 @@ static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
> 				break;
> 			}
> 		}
>-#endif
> }
> 
> #define NO_SPECULATION		BIT(0)

Looks good to me:

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>

But another question: why the hell does clang complain about an unused static inline function?!

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[Andy Shevchenko]

On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> >inline functions for W=1 build").

^^^ (1)

> Looks good to me:
> 
> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>

Thank you!

> But another question: why the hell does clang complain about an unused static inline function?!

Does (1) shed a bit of light to this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[H. Peter Anvin]

On November 8, 2024 4:41:16 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
>> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
>> >inline functions for W=1 build").
>
>^^^ (1)
>
>> Looks good to me:
>> 
>> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>
>Thank you!
>
>> But another question: why the hell does clang complain about an unused static inline function?!
>
>Does (1) shed a bit of light to this?
>

How on earth is that supposed to work?! We have static inline functions in headers all over the place that are only used in certain circumstances. 

Is this a good thing, really? Or is it noise?

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[Andy Shevchenko]

On Fri, Nov 08, 2024 at 04:48:16PM +0100, H. Peter Anvin wrote:
> On November 8, 2024 4:41:16 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
> >> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> >> >inline functions for W=1 build").

^^^ (1)

...

> >> But another question: why the hell does clang complain about an unused static inline function?!
> >
> >Does (1) shed a bit of light to this?
> 
> How on earth is that supposed to work?! We have static inline functions in
> headers all over the place that are only used in certain circumstances.
> 
> Is this a good thing, really? Or is it noise?

This is a good question and IIRC somebody else already asked something similar.

Clang people are Cc'ed here, I leave this to them to comment on,
I am not an expert here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[H. Peter Anvin]

On November 8, 2024 5:29:52 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Fri, Nov 08, 2024 at 04:48:16PM +0100, H. Peter Anvin wrote:
>> On November 8, 2024 4:41:16 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
>> >> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>...
>
>> >> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
>> >> >inline functions for W=1 build").
>
>^^^ (1)
>
>...
>
>> >> But another question: why the hell does clang complain about an unused static inline function?!
>> >
>> >Does (1) shed a bit of light to this?
>> 
>> How on earth is that supposed to work?! We have static inline functions in
>> headers all over the place that are only used in certain circumstances.
>> 
>> Is this a good thing, really? Or is it noise?
>
>This is a good question and IIRC somebody else already asked something similar.
>
>Clang people are Cc'ed here, I leave this to them to comment on,
>I am not an expert here.
>

So there are two questions:

1. How? How does this work, technically? 
2. Why? Is this really the right thing to do, or will it result in a bunch of unnecessary #ifdeffery instead?

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[Nathan Chancellor]

On Fri, Nov 08, 2024 at 04:48:16PM +0100, H. Peter Anvin wrote:
> On November 8, 2024 4:41:16 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
> >> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> >> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> >> >inline functions for W=1 build").
> >
> >^^^ (1)
> >
> >> Looks good to me:
> >> 
> >> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> >
> >Thank you!
> >
> >> But another question: why the hell does clang complain about an unused static inline function?!
> >
> >Does (1) shed a bit of light to this?
> >
> 
> How on earth is that supposed to work?! We have static inline functions in headers all over the place that are only used in certain circumstances. 
> 
> Is this a good thing, really? Or is it noise?

Did you read the commit message of 6863f5643dd7 or just the title?

The difference between Clang and GCC is only around static inline
function in .c files, not .h files.

  $ cat test.h
  static inline void unused_inline_in_h(void) {}

  $ cat test.c
  #include "test.h"

  static inline void unused_inline_in_c(void) {}

  static void unused_in_c(void) {}

  $ gcc -Wall -c -o /dev/null test.c
  test.c:5:13: warning: ‘unused_in_c’ defined but not used [-Wunused-function]
      5 | static void unused_in_c(void) {}
        |             ^~~~~~~~~~~

  $ clang -fsyntax-only -Wall test.c
  test.c:3:20: warning: unused function 'unused_inline_in_c' [-Wunused-function]
      3 | static inline void unused_inline_in_c(void) {}
        |                    ^~~~~~~~~~~~~~~~~~
  test.c:5:13: warning: unused function 'unused_in_c' [-Wunused-function]
      5 | static void unused_in_c(void) {}
        |             ^~~~~~~~~~~
  2 warnings generated.

I do not think there are too many instances of unused static inline
functions in .c files but Andy might be able to speak more around how
many instances he has had to fix across the tree. I can see how this
difference can be useful for catching dead code and maybe even making
code cleaner but if it proves to be too much of an annoyance for the
wider community, we could potentially discuss reverting 6863f5643dd7.

Cheers,
Nathan

Re: [PATCH v5 1/1] x86/cpu: Make sure flag_is_changeable_p() is always being used

[H. Peter Anvin]

On November 8, 2024 6:29:20 PM GMT+01:00, Nathan Chancellor <nathan@kernel.org> wrote:
>On Fri, Nov 08, 2024 at 04:48:16PM +0100, H. Peter Anvin wrote:
>> On November 8, 2024 4:41:16 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >On Fri, Nov 08, 2024 at 04:35:17PM +0100, H. Peter Anvin wrote:
>> >> On November 8, 2024 4:30:10 PM GMT+01:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >
>> >> >See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
>> >> >inline functions for W=1 build").
>> >
>> >^^^ (1)
>> >
>> >> Looks good to me:
>> >> 
>> >> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>> >
>> >Thank you!
>> >
>> >> But another question: why the hell does clang complain about an unused static inline function?!
>> >
>> >Does (1) shed a bit of light to this?
>> >
>> 
>> How on earth is that supposed to work?! We have static inline functions in headers all over the place that are only used in certain circumstances. 
>> 
>> Is this a good thing, really? Or is it noise?
>
>Did you read the commit message of 6863f5643dd7 or just the title?
>
>The difference between Clang and GCC is only around static inline
>function in .c files, not .h files.
>
>  $ cat test.h
>  static inline void unused_inline_in_h(void) {}
>
>  $ cat test.c
>  #include "test.h"
>
>  static inline void unused_inline_in_c(void) {}
>
>  static void unused_in_c(void) {}
>
>  $ gcc -Wall -c -o /dev/null test.c
>  test.c:5:13: warning: ‘unused_in_c’ defined but not used [-Wunused-function]
>      5 | static void unused_in_c(void) {}
>        |             ^~~~~~~~~~~
>
>  $ clang -fsyntax-only -Wall test.c
>  test.c:3:20: warning: unused function 'unused_inline_in_c' [-Wunused-function]
>      3 | static inline void unused_inline_in_c(void) {}
>        |                    ^~~~~~~~~~~~~~~~~~
>  test.c:5:13: warning: unused function 'unused_in_c' [-Wunused-function]
>      5 | static void unused_in_c(void) {}
>        |             ^~~~~~~~~~~
>  2 warnings generated.
>
>I do not think there are too many instances of unused static inline
>functions in .c files but Andy might be able to speak more around how
>many instances he has had to fix across the tree. I can see how this
>difference can be useful for catching dead code and maybe even making
>code cleaner but if it proves to be too much of an annoyance for the
>wider community, we could potentially discuss reverting 6863f5643dd7.
>
>Cheers,
>Nathan

I'm on the road traveling and have limited ability to look things up at the moment. 

However, in .c files the value becomes very very small: in .h files an unused inline becomes a drag on compile time because it effects a number of compilation units, but for a .c file it is just one.