[PATCH 1/2] x86/bootflag: Change some static functions to bool

[PATCH 1/2] x86/bootflag: Change some static functions to bool

[Uros Bizjak]

The return values of some functions are of boolean type. Change the
type of these function to bool and adjust their return values.

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/kernel/bootflag.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
index 3fed7ae58b60..4d89a2d80d0f 100644
--- a/arch/x86/kernel/bootflag.c
+++ b/arch/x86/kernel/bootflag.c
@@ -20,7 +20,7 @@
 
 int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
 
-static int __init parity(u8 v)
+static bool __init parity(u8 v)
 {
 	int x = 0;
 	int i;
@@ -30,7 +30,7 @@ static int __init parity(u8 v)
 		v >>= 1;
 	}
 
-	return x;
+	return !!x;
 }
 
 static void __init sbf_write(u8 v)
@@ -66,14 +66,14 @@ static u8 __init sbf_read(void)
 	return v;
 }
 
-static int __init sbf_value_valid(u8 v)
+static bool __init sbf_value_valid(u8 v)
 {
 	if (v & SBF_RESERVED)		/* Reserved bits */
-		return 0;
+		return false;
 	if (!parity(v))
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 static int __init sbf_init(void)
-- 
2.42.0

[PATCH 2/2] x86/bootflag: Use __builtin_parity() when available

[Uros Bizjak]

Compilers (GCC and clang) provide optimized __builtin_parity() function
that returns the parity of X, i.e. the number of 1-bits in X modulo 2.

Use __builtin_parity() built-in function to optimize parity(). This
improves generated parity code to just:

  57:	84 db                	test   %bl,%bl
  59:	7a 5c                	jp     b7 <sbf_init+0xa7>

where TEST instruction sets parity flag (PF) in EFLAGS, exercised by the
follow-up jump instruction.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/kernel/bootflag.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
index 4d89a2d80d0f..d63247d7abd4 100644
--- a/arch/x86/kernel/bootflag.c
+++ b/arch/x86/kernel/bootflag.c
@@ -22,6 +22,9 @@ int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
 
 static bool __init parity(u8 v)
 {
+#if __has_builtin(__builtin_parity)
+	int x = __builtin_parity(v);
+#else
 	int x = 0;
 	int i;
 
@@ -29,7 +32,7 @@ static bool __init parity(u8 v)
 		x ^= (v & 1);
 		v >>= 1;
 	}
-
+#endif
 	return !!x;
 }
 
-- 
2.42.0

Re: [PATCH 2/2] x86/bootflag: Use __builtin_parity() when available

[Uros Bizjak]

On Wed, Jan 29, 2025 at 4:49 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Compilers (GCC and clang) provide optimized __builtin_parity() function
> that returns the parity of X, i.e. the number of 1-bits in X modulo 2.
>
> Use __builtin_parity() built-in function to optimize parity(). This
> improves generated parity code to just:
>
>   57:   84 db                   test   %bl,%bl
>   59:   7a 5c                   jp     b7 <sbf_init+0xa7>
>
> where TEST instruction sets parity flag (PF) in EFLAGS, exercised by the
> follow-up jump instruction.

Unfortunately, this approach won't fly. As said by Pedro elsewhere:

__builtin functions aren't strictly required to be inlined and can
generate a library call if the compiler so chooses (how often this
happens depends on the optimization level and architecture). e.g:
https://godbolt.org/z/zKPb9hbaM

Patch is retracted.

Uros.

>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>  arch/x86/kernel/bootflag.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> index 4d89a2d80d0f..d63247d7abd4 100644
> --- a/arch/x86/kernel/bootflag.c
> +++ b/arch/x86/kernel/bootflag.c
> @@ -22,6 +22,9 @@ int sbf_port __initdata = -1; /* set via acpi_boot_init() */
>
>  static bool __init parity(u8 v)
>  {
> +#if __has_builtin(__builtin_parity)
> +       int x = __builtin_parity(v);
> +#else
>         int x = 0;
>         int i;
>
> @@ -29,7 +32,7 @@ static bool __init parity(u8 v)
>                 x ^= (v & 1);
>                 v >>= 1;
>         }
> -
> +#endif
>         return !!x;
>  }
>
> --
> 2.42.0
>

[tip: x86/boot] x86/boot: Change some static bootflag functions to bool

[tip-bot2 for Uros Bizjak]

The following commit has been merged into the x86/boot branch of tip:

Commit-ID:     5bebe2e4fe27bf68b4993d62be2f8bae9e6229d6
Gitweb:        https://git.kernel.org/tip/5bebe2e4fe27bf68b4993d62be2f8bae9e6229d6
Author:        Uros Bizjak <ubizjak@gmail.com>
AuthorDate:    Wed, 29 Jan 2025 16:47:50 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 22 Feb 2025 12:30:59 +01:00

x86/boot: Change some static bootflag functions to bool

The return values of some functions are of boolean type. Change the
type of these function to bool and adjust their return values.

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20250129154920.6773-1-ubizjak@gmail.com
---
 arch/x86/kernel/bootflag.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
index 3fed7ae..4d89a2d 100644
--- a/arch/x86/kernel/bootflag.c
+++ b/arch/x86/kernel/bootflag.c
@@ -20,7 +20,7 @@
 
 int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
 
-static int __init parity(u8 v)
+static bool __init parity(u8 v)
 {
 	int x = 0;
 	int i;
@@ -30,7 +30,7 @@ static int __init parity(u8 v)
 		v >>= 1;
 	}
 
-	return x;
+	return !!x;
 }
 
 static void __init sbf_write(u8 v)
@@ -66,14 +66,14 @@ static u8 __init sbf_read(void)
 	return v;
 }
 
-static int __init sbf_value_valid(u8 v)
+static bool __init sbf_value_valid(u8 v)
 {
 	if (v & SBF_RESERVED)		/* Reserved bits */
-		return 0;
+		return false;
 	if (!parity(v))
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 static int __init sbf_init(void)

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Jiri Slaby]

On 29. 01. 25, 16:47, Uros Bizjak wrote:
> The return values of some functions are of boolean type. Change the
> type of these function to bool and adjust their return values.
> 
> No functional change intended.
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>   arch/x86/kernel/bootflag.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> index 3fed7ae58b60..4d89a2d80d0f 100644
> --- a/arch/x86/kernel/bootflag.c
> +++ b/arch/x86/kernel/bootflag.c
> @@ -20,7 +20,7 @@
>   
>   int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
>   
> -static int __init parity(u8 v)
> +static bool __init parity(u8 v)
>   {
>   	int x = 0;
>   	int i;
> @@ -30,7 +30,7 @@ static int __init parity(u8 v)
>   		v >>= 1;
>   	}
>   
> -	return x;
> +	return !!x;

This "!!" is unnecessary and only obfuscates the code, right?

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Uros Bizjak]

On Mon, Feb 24, 2025 at 8:18 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 29. 01. 25, 16:47, Uros Bizjak wrote:
> > The return values of some functions are of boolean type. Change the
> > type of these function to bool and adjust their return values.
> >
> > No functional change intended.
> >
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > ---
> >   arch/x86/kernel/bootflag.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> > index 3fed7ae58b60..4d89a2d80d0f 100644
> > --- a/arch/x86/kernel/bootflag.c
> > +++ b/arch/x86/kernel/bootflag.c
> > @@ -20,7 +20,7 @@
> >
> >   int sbf_port __initdata = -1;       /* set via acpi_boot_init() */
> >
> > -static int __init parity(u8 v)
> > +static bool __init parity(u8 v)
> >   {
> >       int x = 0;
> >       int i;
> > @@ -30,7 +30,7 @@ static int __init parity(u8 v)
> >               v >>= 1;
> >       }
> >
> > -     return x;
> > +     return !!x;
>
> This "!!" is unnecessary and only obfuscates the code, right?

Not really, this idiom is used in place of (x != 0) to change the type
to the return type of the function in a pedantic way.

Uros.

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Jiri Slaby]

On 24. 02. 25, 8:24, Uros Bizjak wrote:
> On Mon, Feb 24, 2025 at 8:18 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>>
>> On 29. 01. 25, 16:47, Uros Bizjak wrote:
>>> The return values of some functions are of boolean type. Change the
>>> type of these function to bool and adjust their return values.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> ---
>>>    arch/x86/kernel/bootflag.c | 12 ++++++------
>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
>>> index 3fed7ae58b60..4d89a2d80d0f 100644
>>> --- a/arch/x86/kernel/bootflag.c
>>> +++ b/arch/x86/kernel/bootflag.c
>>> @@ -20,7 +20,7 @@
>>>
>>>    int sbf_port __initdata = -1;       /* set via acpi_boot_init() */
>>>
>>> -static int __init parity(u8 v)
>>> +static bool __init parity(u8 v)
>>>    {
>>>        int x = 0;
>>>        int i;
>>> @@ -30,7 +30,7 @@ static int __init parity(u8 v)
>>>                v >>= 1;
>>>        }
>>>
>>> -     return x;
>>> +     return !!x;
>>
>> This "!!" is unnecessary and only obfuscates the code, right?
> 
> Not really, this idiom is used in place of (x != 0) to change the type
> to the return type of the function in a pedantic way.

Care to explain what exactly it changes?

-- 
js
suse labs

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Uros Bizjak]

On Mon, Feb 24, 2025 at 8:27 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 24. 02. 25, 8:24, Uros Bizjak wrote:
> > On Mon, Feb 24, 2025 at 8:18 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> >>
> >> On 29. 01. 25, 16:47, Uros Bizjak wrote:
> >>> The return values of some functions are of boolean type. Change the
> >>> type of these function to bool and adjust their return values.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>> Cc: Ingo Molnar <mingo@kernel.org>
> >>> Cc: Borislav Petkov <bp@alien8.de>
> >>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> >>> Cc: "H. Peter Anvin" <hpa@zytor.com>
> >>> ---
> >>>    arch/x86/kernel/bootflag.c | 12 ++++++------
> >>>    1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> >>> index 3fed7ae58b60..4d89a2d80d0f 100644
> >>> --- a/arch/x86/kernel/bootflag.c
> >>> +++ b/arch/x86/kernel/bootflag.c
> >>> @@ -20,7 +20,7 @@
> >>>
> >>>    int sbf_port __initdata = -1;       /* set via acpi_boot_init() */
> >>>
> >>> -static int __init parity(u8 v)
> >>> +static bool __init parity(u8 v)
> >>>    {
> >>>        int x = 0;
> >>>        int i;
> >>> @@ -30,7 +30,7 @@ static int __init parity(u8 v)
> >>>                v >>= 1;
> >>>        }
> >>>
> >>> -     return x;
> >>> +     return !!x;
> >>
> >> This "!!" is unnecessary and only obfuscates the code, right?
> >
> > Not really, this idiom is used in place of (x != 0) to change the type
> > to the return type of the function in a pedantic way.
>
> Care to explain what exactly it changes?

The internal compiler representation of the following testcase:

_Bool foo (int x) { return x; }

is:

--cut here--
_Bool foo (int x)
{
 _Bool _2;

 <bb 2> [local count: 1073741824]:
 _2 = x_1(D) != 0;
 return _2;
}
--cut here--

For me, !!x in the source means that the change of types was
intentional. Surely, the compiler can do it by itself, so at the end
of the day, it is just a matter of personal taste.

FYI, the whole function will soon be removed and replaced with a
generic parity8() function.

Uros.

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Jiri Slaby]

On 24. 02. 25, 8:39, Uros Bizjak wrote:
> The internal compiler representation of the following testcase:
> 
> _Bool foo (int x) { return x; }
> 
> is:
> 
> --cut here--
> _Bool foo (int x)
> {
>   _Bool _2;
> 
>   <bb 2> [local count: 1073741824]:
>   _2 = x_1(D) != 0;
>   return _2;

Yes, exactly as dictated by the C99 standard.

> }
> --cut here--
> 
> For me, !!x in the source means that the change of types was
> intentional. Surely, the compiler can do it by itself, so at the end
> of the day, it is just a matter of personal taste.

I've just learnt, that we even have that in CodingStyle:
===
> 17) Using bool
> --------------
> 
> The Linux kernel bool type is an alias for the C99 _Bool type. bool values can
> only evaluate to 0 or 1, and implicit or explicit conversion to bool
> automatically converts the value to true or false. When using bool types the
> !! construction is not needed, which eliminates a class of bugs.
===

> FYI, the whole function will soon be removed and replaced with a
> generic parity8() function.

Yeah, your post in that thread brought me here, in fact. Though, I 
didn't realize it actually removes this very function in full.

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Uros Bizjak]

On Mon, Feb 24, 2025 at 8:48 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 24. 02. 25, 8:39, Uros Bizjak wrote:
> > The internal compiler representation of the following testcase:
> >
> > _Bool foo (int x) { return x; }
> >
> > is:
> >
> > --cut here--
> > _Bool foo (int x)
> > {
> >   _Bool _2;
> >
> >   <bb 2> [local count: 1073741824]:
> >   _2 = x_1(D) != 0;
> >   return _2;
>
> Yes, exactly as dictated by the C99 standard.
>
> > }
> > --cut here--
> >
> > For me, !!x in the source means that the change of types was
> > intentional. Surely, the compiler can do it by itself, so at the end
> > of the day, it is just a matter of personal taste.
>
> I've just learnt, that we even have that in CodingStyle:

Ugh, I didn't know that either... Thanks for the info.

Regarding the issue itself, I can create a follow-up patch that
removes !!x, or perhaps we can go all the way and substitute the
function with parity8() generic function, available in bitops.h.

Thanks,
Uros.

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Ingo Molnar]

* Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 02. 25, 8:39, Uros Bizjak wrote:
> > The internal compiler representation of the following testcase:
> > 
> > _Bool foo (int x) { return x; }
> > 
> > is:
> > 
> > --cut here--
> > _Bool foo (int x)
> > {
> >   _Bool _2;
> > 
> >   <bb 2> [local count: 1073741824]:
> >   _2 = x_1(D) != 0;
> >   return _2;
> 
> Yes, exactly as dictated by the C99 standard.
> 
> > }
> > --cut here--
> > 
> > For me, !!x in the source means that the change of types was
> > intentional. Surely, the compiler can do it by itself, so at the end
> > of the day, it is just a matter of personal taste.
> 
> I've just learnt, that we even have that in CodingStyle:
> ===
> > 17) Using bool
> > --------------
> > 
> > The Linux kernel bool type is an alias for the C99 _Bool type. bool values can
> > only evaluate to 0 or 1, and implicit or explicit conversion to bool
> > automatically converts the value to true or false. When using bool types the
> > !! construction is not needed, which eliminates a class of bugs.
> ===

This rule doesn't apply here, because the !! operation isn't done on 
bool types: 'x' in the parity() function is an 'int'...

So this CodingStyle entry is a red herring, and the !! is absolutely 
used in the kernel as an explicit marker of intentional type conversion 
to bool.

Thanks,

	Ingo

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Jiri Slaby]

On 24. 02. 25, 19:58, Ingo Molnar wrote:
> So this CodingStyle entry is a red herring, and the !! is absolutely
> used in the kernel

Sure, for intended conversion to either 0 or 1.

> as an explicit marker of intentional type conversion
> to bool.

With this in mind, you would have to write "if (!!x)" everywhere.

I don't want such constructions in code I maintain. (Nor for return 
values.) But this is not code I maintain (obviously), so your call after 
all.

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[H. Peter Anvin]

On February 25, 2025 10:31:37 PM PST, Jiri Slaby <jirislaby@kernel.org> wrote:
>On 24. 02. 25, 19:58, Ingo Molnar wrote:
>> So this CodingStyle entry is a red herring, and the !! is absolutely
>> used in the kernel
>
>Sure, for intended conversion to either 0 or 1.
>
>> as an explicit marker of intentional type conversion
>> to bool.
>
>With this in mind, you would have to write "if (!!x)" everywhere.
>
>I don't want such constructions in code I maintain. (Nor for return values.) But this is not code I maintain (obviously), so your call after all.
>
>thanks,

Uh, no, that's not the point.

The point is that:

!!x

... is the same as 

x ? true : false

... which if promoted to an integer, intentionally or not, becomes

x ? 1 : 0

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Jiri Slaby]

On 26. 02. 25, 8:17, H. Peter Anvin wrote:
> On February 25, 2025 10:31:37 PM PST, Jiri Slaby <jirislaby@kernel.org> wrote:
>> On 24. 02. 25, 19:58, Ingo Molnar wrote:
>>> So this CodingStyle entry is a red herring, and the !! is absolutely
>>> used in the kernel

label1:

>> Sure, for intended conversion to either 0 or 1.




>>> as an explicit marker of intentional type conversion
>>> to bool.
>>
>> With this in mind, you would have to write "if (!!x)" everywhere.
>>
>> I don't want such constructions in code I maintain. (Nor for return values.) But this is not code I maintain (obviously), so your call after all.
>>
>> thanks,
> 
> Uh, no, that's not the point.
> 
> The point is that:
> 
> !!x
> 
> ... is the same as
> 
> x ? true : false
> 
> ... which if promoted to an integer, intentionally or not, becomes
> 
> x ? 1 : 0

Which I write at label1 above. Sorry, am I missing something?


"!!x" when implicitly converted to bool is the very same as simple "x".

-- 
js
suse labs

Re: [PATCH 1/2] x86/bootflag: Change some static functions to bool

[Ingo Molnar]

* Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 02. 25, 19:58, Ingo Molnar wrote:
> > So this CodingStyle entry is a red herring, and the !! is absolutely
> > used in the kernel
> 
> Sure, for intended conversion to either 0 or 1.
> 
> > as an explicit marker of intentional type conversion
> > to bool.
> 
> With this in mind, you would have to write "if (!!x)" everywhere.

No, why would I? In a conditional statement any type conversion is for 
that evaluation alone and any mistakes are limited to that statement.

On a return statement the value continues to live on in the call 
context and has a far longer lifetime. Marking that the type conversion 
from int to bool was intentional is prudent, and the use of '!!' is 
common practice within the kernel:

  starship:~/tip> git grep '!!' -- '*.[ch]' | wc -l
  10739

Thanks,

	Ingo