| Message ID | 9d222cc83013aaa67b45638b27f5975b60aecb37.1687332385.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v2] xen/include: avoid undefined behavior. | expand |
On 21.06.2023 09:58, Nicola Vetrini wrote: > Redefine BUILD_BUG_ON_ZERO to fully comply with C99 avoiding > undefined behavior 58 ("A structure or union is defined as > containing no named members (6.7.2.1)." Here and in the title I'm not happy about you referencing undefined behavior. What we do here is use a well-known compiler extension (and I'm sure you're aware we do so elsewhere, where it's actually going to remain as is from all I can tell right now). > --- 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 ")"); }) - 1U) To be compatible with whatever odd ABIs new ports may have, maybe better to AND or multiply with 0? (I also don't think a U suffix is warranted here.) > #else > -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); }) > +#define BUILD_BUG_ON_ZERO(cond) \ > + (sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - sizeof(unsigned)) What's wrong with just giving the bitfield a name: #define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int _:-!!(cond); }) Jan
On 21/06/23 10:48, Jan Beulich wrote: > On 21.06.2023 09:58, Nicola Vetrini wrote: >> Redefine BUILD_BUG_ON_ZERO to fully comply with C99 avoiding >> undefined behavior 58 ("A structure or union is defined as >> containing no named members (6.7.2.1)." > > Here and in the title I'm not happy about you referencing undefined > behavior. What we do here is use a well-known compiler extension (and I'm > sure you're aware we do so elsewhere, where it's actually going to remain > as is from all I can tell right now). > Noted, I'll change the commit message. >> --- 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 ")"); }) - 1U) > > To be compatible with whatever odd ABIs new ports may have, maybe better to > AND or multiply with 0? (I also don't think a U suffix is warranted here.) Good idea > >> #else >> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); }) >> +#define BUILD_BUG_ON_ZERO(cond) \ >> + (sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - sizeof(unsigned)) > > What's wrong with just giving the bitfield a name: > > #define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int _:-!!(cond); }) > A named bitfield with zero width is not allowed by the standard, which is why the fix introduces a constant positive width. I'll send a v3 shortly addressing your previous remarks, though.
On 22.06.2023 10:15, Nicola Vetrini wrote: > On 21/06/23 10:48, Jan Beulich wrote: >> On 21.06.2023 09:58, Nicola Vetrini wrote: >>> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); }) >>> +#define BUILD_BUG_ON_ZERO(cond) \ >>> + (sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - sizeof(unsigned)) >> >> What's wrong with just giving the bitfield a name: >> >> #define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int _:-!!(cond); }) >> > > A named bitfield with zero width is not allowed by the standard, which > is why the fix introduces a constant positive width. I'll send a v3 > shortly addressing your previous remarks, though. Oh, right, but easy to overcome, I think: int _:1 - !!(cond); Jan
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 67fc7c1d7e..e57d272772 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 ")"); }) - 1U) #else -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); }) +#define BUILD_BUG_ON_ZERO(cond) \ + (sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - sizeof(unsigned)) #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) #endif
Redefine BUILD_BUG_ON_ZERO to fully comply with C99 avoiding 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 for any memory layout of the struct (this motivates the "sizeof(unsigned) * 8" in the definition of the macro). 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 --- xen/include/xen/lib.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)