Libguestfs - Jul 2022 - [nbdkit] ARRAY_SIZE [was: vddk: Demote another "phone home" error message to debug]

On Thu, Jul 07, 2022 at 12:58:53PM +0200, Laszlo Ersek
wrote:
> On 07/07/22 11:02, Richard W.M. Jones wrote:
> > Reported-by: Ming Xie
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2104720
> > ---
> I think it's time for us to introduce the ARRAY_SIZE macro.
> 
> Can you do that first perhaps, rebase the existing code, and then add this
new patch? :)
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.

Quoting the patch itself:

+/* This expression fails at compile time if 'expr' is true.  It does
+ * this by constructing a struct which has an impossible
+ * (negative-sized) array.
+ *
+ * If 'expr' is false then we subtract the sizes of the two identical
+ * structures, returning zero.
+ */

My summary of those docs: BUILD_BUG_ON_ZERO(1) is a build bug (forced
compiler error); BUILD_BUG_ON_ZERO(0) is 0.  To use the macro, I then
have to come up with a condition that results in false to avoid a
build bug (when normally, I like to come up with predicates that
result in true when I want to proceed, rather than predicates that
result in false).

To me, the name "BUILD_BUG_ON_ZERO" parses as "I want a build bug
if I
pass in 0", and not "I want to use 0 unchanged, and anything else to
be a build bug".  And once I get over my mis-parse of the name, I then
have to remember that ARRAY_SIZE must use '+ BUILD_BUG_ON_ZERO
(!TYPE_IS_ARRAY(a))' to mean "add 0 if 'a' is an array type,
because
'!ARRAY_TYPE(a)' is 0", which involved a double negative.

I don't mind if we keep the macro, but can we please use a better name
than the kernel folks have stuck themselves with, especially since we
are NOT copying the kernel's implementation of the macro (as our
licensing won't allow it in the first place)?

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; }))
/* If EXPR is true, produce 0. Otherwise, fail the build */
#define BUILD_BUG_UNLESS_TRUE(expr) \
  (!BUILD_BUG_STRUCT_SIZE(expr))

at which point the definition of ARRAY_SIZE(a) includes
'+ BUILD_BUG_UNLESS_TRUE(TYPE_IS_ARRAY(a))', which is easier (for me) to
read.

After writing the above, I also looked at how gnulib did things in
lib/verify.h.  Their arugment is that no single macro can be used in
all contexts, so they provide:
/* R must be non-zero, or compilation fails. */
verify_expr(R, E) /* for use in scalar contexts, result in E */
verify(R) /* for use in declaration contexts */

but it matches my bias of avoiding the double-negative (I find it easy
to write a predicate that must either be true or will cause
compilation failure).  Here, I would define ARRAY_SIZE by using
'+ verify_expr(TYPE_IS_ARRAY(a), 0)'

[Caution - I have touched gnulib's lib/verify.h in the past.  Although
much of it has been rewritten in the meantime as compilers have
progressed, I may still be tainted enough by my past work on gnulib's
implementation of compile-time checking that I don't trust myself to
do a genuinely clean-room reimplementation compatible with nbdkit's
licensing needs]

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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 2001
From: "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