Libguestfs - Jul 2022 - [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) 1
I'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

On Thu, Jul 07, 2022 at 03:10:19PM -0500, Eric Blake
wrote:
> 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.
The error message when it fails is very lengthy, but also I'm not too
worried there.

Pushed as 9b3a2d6c55dac825846617cc1d10111aea1793b4.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org