[XEN PATCH v3] 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/db883456d7d210e2ea9ac4a7b402dda73c3ea8cd.1687421893.git.nicola.vetrini@bugseng.com
There is a newer version of this series
xen/include/xen/lib.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[XEN PATCH v3] 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 chosen ill-formed construct is a negative bitwidth in a
bitfield within a struct containing at least one named member,
which prevents the UB while keeping the semantics of the construct.

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
---
 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..d5f9c4a18a 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 ")"); }) & 0U)
 #else
-#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
+#define BUILD_BUG_ON_ZERO(cond) \
+    (sizeof(struct { unsigned u : (cond) ? -1 : 1; }) & 0U)
 #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
 #endif
 
-- 
2.34.1
Re: [XEN PATCH v3] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO.
Posted by Jan Beulich 10 months, 1 week ago
On 22.06.2023 10:24, Nicola Vetrini wrote:
> --- 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 ")"); }) & 0U)
>  #else
> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
> +#define BUILD_BUG_ON_ZERO(cond) \
> +    (sizeof(struct { unsigned u : (cond) ? -1 : 1; }) & 0U)

Given your remark on named bitfields not being allowed to be zero length
(which hopefully holds universally, i.e. isn't subject to compiler
extensions), how about

#define BUILD_BUG_ON_ZERO(cond) (sizeof(struct { int _:!(cond); }) & 0)

? (Implicitly, as said before, I question the value of the U suffixes here.
Even better might be to make sure the expression is explicitly not of
unsigned type, to avoid surprises if someone used ~BUILD_BUG_ON_ZERO()
somewhere.)

Jan
Re: [XEN PATCH v3] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO.
Posted by Nicola Vetrini 10 months, 1 week ago

On 22/06/23 10:56, Jan Beulich wrote:
> On 22.06.2023 10:24, Nicola Vetrini wrote:
>> --- 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 ")"); }) & 0U)
>>   #else
>> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
>> +#define BUILD_BUG_ON_ZERO(cond) \
>> +    (sizeof(struct { unsigned u : (cond) ? -1 : 1; }) & 0U)
> 
> Given your remark on named bitfields not being allowed to be zero length
> (which hopefully holds universally, i.e. isn't subject to compiler
> extensions), how about
> 
> #define BUILD_BUG_ON_ZERO(cond) (sizeof(struct { int _:!(cond); }) & 0)

Well, the reason for the change is to drop any assumption on 
compiler-specific quirks that may arise (it would be a surprise if this 
wasn't the case, though)

> 
> ? (Implicitly, as said before, I question the value of the U suffixes here.
> Even better might be to make sure the expression is explicitly not of
> unsigned type, to avoid surprises if someone used ~BUILD_BUG_ON_ZERO()
> somewhere.)
> 
> Jan

The documentation for the macro definition states that the expression 
must have value 0 and type size_t when cond is false. If I understand 
correctly what you're saying here, then this is not allowed by the 
documentation.

Regards,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH v3] xen/include: avoid using a compiler extension for BUILD_BUG_ON_ZERO.
Posted by Jan Beulich 10 months, 1 week ago
On 22.06.2023 11:34, Nicola Vetrini wrote:
> 
> 
> On 22/06/23 10:56, Jan Beulich wrote:
>> On 22.06.2023 10:24, Nicola Vetrini wrote:
>>> --- 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 ")"); }) & 0U)
>>>   #else
>>> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
>>> +#define BUILD_BUG_ON_ZERO(cond) \
>>> +    (sizeof(struct { unsigned u : (cond) ? -1 : 1; }) & 0U)
>>
>> Given your remark on named bitfields not being allowed to be zero length
>> (which hopefully holds universally, i.e. isn't subject to compiler
>> extensions), how about
>>
>> #define BUILD_BUG_ON_ZERO(cond) (sizeof(struct { int _:!(cond); }) & 0)
> 
> Well, the reason for the change is to drop any assumption on 
> compiler-specific quirks that may arise (it would be a surprise if this 
> wasn't the case, though)

And the reason for my suggestion was that it's shorter, and typically I
view "shorter" as also "easier to read/follow".

>> ? (Implicitly, as said before, I question the value of the U suffixes here.
>> Even better might be to make sure the expression is explicitly not of
>> unsigned type, to avoid surprises if someone used ~BUILD_BUG_ON_ZERO()
>> somewhere.)
> 
> The documentation for the macro definition states that the expression 
> must have value 0 and type size_t when cond is false. If I understand 
> correctly what you're saying here, then this is not allowed by the 
> documentation.

Well, it's the comment that says this, yes. Question is whether the
comment mandates the behavior or merely describes the current
implementation. Imo it's the latter ... But anyway, switching the
result type is independent of the present goal.

Jan