1. Patchew
  2. Xen
  3. View series

[XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO

Nicola Vetrini posted 1 patch 10 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/0b0d9a5614daac653b3bfbcfaa29a9b4c11286bb.1687793572.git.nicola.vetrini@bugseng.com
xen/include/xen/lib.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO
Posted by Nicola Vetrini 10 months, 1 week ago 
Redefine BUILD_BUG_ON_ZERO to avoid using a compiler extension
that gives an acceptable semantics to C99 undefined behavior 58
("A structure or union is defined as containing no named members
(6.7.2.1)").

The first definition includes an additional named field of type
char.

The chosen ill-formed construct for the second definition is a struct
with a named bitfield of width 0 when the condition is true,
which prevents the UB without using the compiler extension while keeping
the semantic of the construct.

The choice of the bitwise AND operation to bring the result to 0
when cond is false boils down to possibly better portability.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in V2:
- Avoid using a VLA as the compile-time assertion
- Do not drop _Static_assert
Changes in V3:
- Changed the operation to bring the result to 0 when the
construct does not lead to a compilation error
Changes in V4:
- Switched to a shorter construct for the second definition.
Changes in V5:
- Dropped 'U's from the macro definitions.
Changes in V6:
- Fixed patch submission.
---
 xen/include/xen/lib.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 67fc7c1d7e..a8958ed57b 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -51,9 +51,10 @@
    e.g. in a structure initializer (or where-ever else comma expressions
    aren't permitted). */
 #define BUILD_BUG_ON_ZERO(cond) \
-    sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); })
+    (sizeof(struct { char c; _Static_assert(!(cond), "!(" #cond ")"); }) & 0)
 #else
-#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
+#define BUILD_BUG_ON_ZERO(cond) \
+    (sizeof(struct { unsigned u : !(cond); }) & 0)
 #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
 #endif
 
-- 
2.34.1
Re: [XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO
Posted by Jan Beulich 10 months ago 
On 26.06.2023 17:37, Nicola Vetrini wrote:
> Redefine BUILD_BUG_ON_ZERO to avoid using a compiler extension
> that gives an acceptable semantics to C99 undefined behavior 58
> ("A structure or union is defined as containing no named members
> (6.7.2.1)").
> 
> The first definition includes an additional named field of type
> char.
> 
> The chosen ill-formed construct for the second definition is a struct
> with a named bitfield of width 0 when the condition is true,
> which prevents the UB without using the compiler extension while keeping
> the semantic of the construct.
> 
> The choice of the bitwise AND operation to bring the result to 0
> when cond is false boils down to possibly better portability.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

The code change looks okay to me now, but I'd like to double check
towards the testing you did with this change: While it is clear that
you will have checked that the tree builds with the adjustments, I
expect that would have been with a compiler supporting _Static_assert.
Did you also check with an older compiler, using the alternative
implementation? Plus did you also check both constructs for actually
triggering when the supplied condition turns out to be true?

Jan

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -51,9 +51,10 @@
>     e.g. in a structure initializer (or where-ever else comma expressions
>     aren't permitted). */
>  #define BUILD_BUG_ON_ZERO(cond) \
> -    sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); })
> +    (sizeof(struct { char c; _Static_assert(!(cond), "!(" #cond ")"); }) & 0)
>  #else
> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
> +#define BUILD_BUG_ON_ZERO(cond) \
> +    (sizeof(struct { unsigned u : !(cond); }) & 0)
>  #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
>  #endif
>
Re: [XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO
Posted by Nicola Vetrini 10 months ago 

On 04/07/23 16:24, Jan Beulich wrote:
> On 26.06.2023 17:37, Nicola Vetrini wrote:
>> Redefine BUILD_BUG_ON_ZERO to avoid using a compiler extension
>> that gives an acceptable semantics to C99 undefined behavior 58
>> ("A structure or union is defined as containing no named members
>> (6.7.2.1)").
>>
>> The first definition includes an additional named field of type
>> char.
>>
>> The chosen ill-formed construct for the second definition is a struct
>> with a named bitfield of width 0 when the condition is true,
>> which prevents the UB without using the compiler extension while keeping
>> the semantic of the construct.
>>
>> The choice of the bitwise AND operation to bring the result to 0
>> when cond is false boils down to possibly better portability.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> The code change looks okay to me now, but I'd like to double check
> towards the testing you did with this change: While it is clear that
> you will have checked that the tree builds with the adjustments, I
> expect that would have been with a compiler supporting _Static_assert.
> Did you also check with an older compiler, using the alternative
> implementation? Plus did you also check both constructs for actually
> triggering when the supplied condition turns out to be true?
> 
> Jan
> 

Besides using the build pipelines in gitlab, I checked just the macro on 
Compiler Explorer using gcc 4.4.7 and clang 16 
(https://godbolt.org/z/1d6vznxcW) and the construct is behaving as expected.

Regards,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO
Posted by Jan Beulich 10 months ago 
On 04.07.2023 16:53, Nicola Vetrini wrote:
> 
> 
> On 04/07/23 16:24, Jan Beulich wrote:
>> On 26.06.2023 17:37, Nicola Vetrini wrote:
>>> Redefine BUILD_BUG_ON_ZERO to avoid using a compiler extension
>>> that gives an acceptable semantics to C99 undefined behavior 58
>>> ("A structure or union is defined as containing no named members
>>> (6.7.2.1)").
>>>
>>> The first definition includes an additional named field of type
>>> char.
>>>
>>> The chosen ill-formed construct for the second definition is a struct
>>> with a named bitfield of width 0 when the condition is true,
>>> which prevents the UB without using the compiler extension while keeping
>>> the semantic of the construct.
>>>
>>> The choice of the bitwise AND operation to bring the result to 0
>>> when cond is false boils down to possibly better portability.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> The code change looks okay to me now, but I'd like to double check
>> towards the testing you did with this change: While it is clear that
>> you will have checked that the tree builds with the adjustments, I
>> expect that would have been with a compiler supporting _Static_assert.
>> Did you also check with an older compiler, using the alternative
>> implementation? Plus did you also check both constructs for actually
>> triggering when the supplied condition turns out to be true?
> 
> Besides using the build pipelines in gitlab, I checked just the macro on 
> Compiler Explorer using gcc 4.4.7 and clang 16 
> (https://godbolt.org/z/1d6vznxcW) and the construct is behaving as expected.

Great, thanks for confirming.

Jan
Re: [XEN PATCH v6] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO
Posted by Stefano Stabellini 10 months, 1 week ago 
On Mon, 26 Jun 2023, Nicola Vetrini wrote:
> Redefine BUILD_BUG_ON_ZERO to avoid using a compiler extension
> that gives an acceptable semantics to C99 undefined behavior 58
> ("A structure or union is defined as containing no named members
> (6.7.2.1)").
> 
> The first definition includes an additional named field of type
> char.
> 
> The chosen ill-formed construct for the second definition is a struct
> with a named bitfield of width 0 when the condition is true,
> which prevents the UB without using the compiler extension while keeping
> the semantic of the construct.
> 
> The choice of the bitwise AND operation to bring the result to 0
> when cond is false boils down to possibly better portability.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in V2:
> - Avoid using a VLA as the compile-time assertion
> - Do not drop _Static_assert
> Changes in V3:
> - Changed the operation to bring the result to 0 when the
> construct does not lead to a compilation error
> Changes in V4:
> - Switched to a shorter construct for the second definition.
> Changes in V5:
> - Dropped 'U's from the macro definitions.
> Changes in V6:
> - Fixed patch submission.
> ---
>  xen/include/xen/lib.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 67fc7c1d7e..a8958ed57b 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -51,9 +51,10 @@
>     e.g. in a structure initializer (or where-ever else comma expressions
>     aren't permitted). */
>  #define BUILD_BUG_ON_ZERO(cond) \
> -    sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); })
> +    (sizeof(struct { char c; _Static_assert(!(cond), "!(" #cond ")"); }) & 0)
>  #else
> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
> +#define BUILD_BUG_ON_ZERO(cond) \
> +    (sizeof(struct { unsigned u : !(cond); }) & 0)
>  #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
>  #endif
>  
> -- 
> 2.34.1
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.