Richard W.M. Jones
2022-Jul-07 19:24 UTC
[Libguestfs] [nbdkit] ARRAY_SIZE [was: vddk: Demote another "phone home" error message to debug]
On Thu, Jul 07, 2022 at 01:46:28PM -0500, Eric Blake wrote:> I see that is now done as part of 0fa23df5c. While > include/array-size.h looks okay, I personally have a tough time with > include/compiler-macros.h BUILD_BUG_ON_ZERO(expr). Every time I > encounter a similarly-named macro in other projects, I have to go > re-read the docs, because (to at least me) the name is not > self-describing.You're not the first person to complain about that patch. How about the attached patch which incorporates your suggestion from later in the email.> I'm also worried that your code uses 'struct { int > _array_size_failed[(expr)...]; }', which may end up causing VLA > situations, where (mistakenly) passing in a non-constant expr could > compile (as a VLA) rather than letting the compiler diagnose that expr > wasn't consant. Hence, I like bitfields better than array sizes when > trying to force a conditional compiler error, as it gets VLA out of > the picture. > > If I were designing it from scratch, I might create: > > /* If EXPR is true, produce a non-zero struct size. Otherwise, fail the build */ > #define BUILD_BUG_STRUCT_SIZE(expr) \ > (sizeof (struct { int: (expr) ? 1 : -1; }))One thing I didn't understand about this is how the unnamed bitfield works, I didn't know that was permitted? Or am I not parsing this right? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW -------------- next part -------------->From c62cffde2be0debcbb35590a1ffc993161fcdb28 Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones at redhat.com> Date: Thu, 7 Jul 2022 20:21:39 +0100 Subject: [PATCH] common/include: Rename BUILD_BUG_ON_ZERO to something more meaningful Updates: commit 0fa23df5cd5dc97a55857416ea81d5de6d867c18 Thanks: Laszlo Ersek, Eric Blake --- common/include/array-size.h | 2 +- common/include/compiler-macros.h | 22 ++++++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/common/include/array-size.h b/common/include/array-size.h index b6d33dde..3212d9dc 100644 --- a/common/include/array-size.h +++ b/common/include/array-size.h @@ -36,6 +36,6 @@ #include "compiler-macros.h" #define ARRAY_SIZE(a) \ - ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_ON_ZERO (!TYPE_IS_ARRAY(a))) + ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_UNLESS_TRUE (TYPE_IS_ARRAY(a))) #endif /* NBDKIT_ARRAY_SIZE_H */ diff --git a/common/include/compiler-macros.h b/common/include/compiler-macros.h index 504e0085..1a6cba2b 100644 --- a/common/include/compiler-macros.h +++ b/common/include/compiler-macros.h @@ -35,24 +35,26 @@ #ifndef __cplusplus -/* This expression fails at compile time if 'expr' is true. It does - * this by constructing a struct which has an impossible - * (negative-sized) array. +/* BUILD_BUG_UNLESS_TRUE(1) => 0 + * BUILD_BUG_UNLESS_TRUE(0) => build time failure * - * If 'expr' is false then we subtract the sizes of the two identical - * structures, returning zero. + * It works by constructing a struct which has an impossible + * (negative-sized) bit-field in the build failure case. + * + * The Linux kernel calls this BUILD_BUG_ON_ZERO which is a + * confusing name, but has the same semantics as above. */ -#define BUILD_BUG_ON_ZERO_SIZEOF(expr) \ - (sizeof (struct { int _array_size_failed[(expr) ? -1 : 1]; })) -#define BUILD_BUG_ON_ZERO(expr) \ - (BUILD_BUG_ON_ZERO_SIZEOF(expr) - BUILD_BUG_ON_ZERO_SIZEOF(expr)) +#define BUILD_BUG_STRUCT_SIZE(expr) \ + (sizeof (struct { int: (expr) ? 1 : -1; })) +#define BUILD_BUG_UNLESS_TRUE(expr) \ + (BUILD_BUG_STRUCT_SIZE(expr) - BUILD_BUG_STRUCT_SIZE(expr)) #define TYPE_IS_ARRAY(a) \ (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) #else /* __cplusplus */ -#define BUILD_BUG_ON_ZERO(expr) 0 +#define BUILD_BUG_UNLESS_TRUE(expr) 0 #define TYPE_IS_ARRAY(a) 1 #endif /* __cplusplus */ -- 2.37.0.rc2
Eric Blake
2022-Jul-07 20:10 UTC
[Libguestfs] [nbdkit] ARRAY_SIZE [was: vddk: Demote another "phone home" error message to debug]
On Thu, Jul 07, 2022 at 08:24:43PM +0100, Richard W.M. Jones wrote:> On Thu, Jul 07, 2022 at 01:46:28PM -0500, Eric Blake wrote: > > I see that is now done as part of 0fa23df5c. While > > include/array-size.h looks okay, I personally have a tough time with > > include/compiler-macros.h BUILD_BUG_ON_ZERO(expr). Every time I > > encounter a similarly-named macro in other projects, I have to go > > re-read the docs, because (to at least me) the name is not > > self-describing. > > You're not the first person to complain about that patch. How about > the attached patch which incorporates your suggestion from later in > the email. > > > I'm also worried that your code uses 'struct { int > > _array_size_failed[(expr)...]; }', which may end up causing VLA > > situations, where (mistakenly) passing in a non-constant expr could > > compile (as a VLA) rather than letting the compiler diagnose that expr > > wasn't consant. Hence, I like bitfields better than array sizes when > > trying to force a conditional compiler error, as it gets VLA out of > > the picture. > > > > If I were designing it from scratch, I might create: > > > > /* If EXPR is true, produce a non-zero struct size. Otherwise, fail the build */ > > #define BUILD_BUG_STRUCT_SIZE(expr) \ > > (sizeof (struct { int: (expr) ? 1 : -1; })) > > One thing I didn't understand about this is how the unnamed bitfield > works, I didn't know that was permitted? Or am I not parsing > this right?Modern C allows anonymous bitfields (C17 section 6.7.2.1 constraint 12, see also footnote 128 "An unnamed bit-field structure member is useful for padding to conform to externally imposed layouts."; fairly unchanged from C99 6.7.2.1 constraint 11 and footnote 105); it also calls out the grammar that permits an anonymous struct with a single anonymous bitfield as its lone member: struct-or-union-specifier: struct-or-union identifieropt { struct-declaration-list } struct-or-union: struct struct-declaration-list: struct-declaration struct-declaration: specifier-qualifier-list struct-declarator-listopt ; specifier-qualifier-list: type-specifier specifier-qualifier-listopt struct-declarator-list: struct-declarator struct-declarator: declaratoropt : constant-expression However, constraint 8 in C17 6.7.2.1 then says "If the struct-declaration-list does not contain any named members, either directly or via an anonymous structure or anonymous union, the behavior is undefined." so we may also need to add a dummy named member alongside our anonymous bitfield, or give our bitfield a name (we already have macros for generating names using __COUNTER__ in include/unique-name.h); but gcc-12.1.1 and clang-14.0.0 on Fedora 36 appeared happy without that extra step. I guess it depends on what older compilers do for whether we need to add in more content. There's also a question of whether the error message when the constraint fails is going to be legible enough to give the developer a hint as to what they did wrong when they pass in a non-array type; but I'm less worried about that.> Subject: [PATCH] common/include: Rename BUILD_BUG_ON_ZERO to something more > meaningful > > Updates: commit 0fa23df5cd5dc97a55857416ea81d5de6d867c18 > Thanks: Laszlo Ersek, Eric Blake > --- > common/include/array-size.h | 2 +- > common/include/compiler-macros.h | 22 ++++++++++++---------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/common/include/array-size.h b/common/include/array-size.h > index b6d33dde..3212d9dc 100644 > --- a/common/include/array-size.h > +++ b/common/include/array-size.h > @@ -36,6 +36,6 @@ > #include "compiler-macros.h" > > #define ARRAY_SIZE(a) \ > - ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_ON_ZERO (!TYPE_IS_ARRAY(a))) > + ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_UNLESS_TRUE (TYPE_IS_ARRAY(a))) > > #endif /* NBDKIT_ARRAY_SIZE_H */ > diff --git a/common/include/compiler-macros.h b/common/include/compiler-macros.h > index 504e0085..1a6cba2b 100644 > --- a/common/include/compiler-macros.h > +++ b/common/include/compiler-macros.h > @@ -35,24 +35,26 @@ > > #ifndef __cplusplus > > -/* This expression fails at compile time if 'expr' is true. It does > - * this by constructing a struct which has an impossible > - * (negative-sized) array. > +/* BUILD_BUG_UNLESS_TRUE(1) => 0 > + * BUILD_BUG_UNLESS_TRUE(0) => build time failure > * > - * If 'expr' is false then we subtract the sizes of the two identical > - * structures, returning zero. > + * It works by constructing a struct which has an impossible > + * (negative-sized) bit-field in the build failure case. > + * > + * The Linux kernel calls this BUILD_BUG_ON_ZERO which is a > + * confusing name, but has the same semantics as above.Well, not quite the same: BUILD_BUG_ON_ZERO(!cond) => BUILD_BUG_UNLESS_TRUE(cond)> */ > -#define BUILD_BUG_ON_ZERO_SIZEOF(expr) \ > - (sizeof (struct { int _array_size_failed[(expr) ? -1 : 1]; })) > -#define BUILD_BUG_ON_ZERO(expr) \ > - (BUILD_BUG_ON_ZERO_SIZEOF(expr) - BUILD_BUG_ON_ZERO_SIZEOF(expr)) > +#define BUILD_BUG_STRUCT_SIZE(expr) \ > + (sizeof (struct { int: (expr) ? 1 : -1; })) > +#define BUILD_BUG_UNLESS_TRUE(expr) \ > + (BUILD_BUG_STRUCT_SIZE(expr) - BUILD_BUG_STRUCT_SIZE(expr))My email was only lightly tested (intentionally so, to minimize the risk of license tainting based on my past experience and research while composing my mail, to leave you as the implementor); but your implementation based on my ideas looks right.> > #define TYPE_IS_ARRAY(a) \ > (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) > > #else /* __cplusplus */ > > -#define BUILD_BUG_ON_ZERO(expr) 0 > +#define BUILD_BUG_UNLESS_TRUE(expr) 0 > #define TYPE_IS_ARRAY(a) 1I'm sure there are ways to implement these in C++; but I'm not too worried about it for the short term ;) If you'll clean up the one last issue in the comment that I pointed out about kernel equivalency, then: Acked-by: Eric Blake <eblake at redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org