[Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups

[Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups

[Michael S. Tsirkin]

Turns out virtio kept using ARRAY_SIZE on fields which stopped
being arrays, this was noticed by a coverity scan.
Fix this up, and fix up the ARRAY_SIZE macro so that this
bug does not reappear in any other place.

Michael S. Tsirkin (4):
  virtio: fix up max size checks
  compiler: drop ; after BUILD_BUG_ON
  compiler: expression version of QEMU_BUILD_BUG_ON
  ARRAY_SIZE: check that argument is an array

 include/qemu/compiler.h |  5 ++++-
 include/qemu/osdep.h    |  8 +++++++-
 hw/virtio/virtio.c      | 27 +++++++++++----------------
 3 files changed, 22 insertions(+), 18 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks

[Michael S. Tsirkin]

Coverity reports that ARRAY_SIZE(elem->out_sg) (and all the others too)
is wrong because elem->out_sg is a pointer.

However, the check is not in the right place and the max_size argument
of virtqueue_map_iovec can be removed.  The check on in_num/out_num
should be moved to qemu_get_virtqueue_element instead, before the call
to virtqueue_alloc_element.

Cc: qemu-stable@nongnu.org
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Fixes: 3724650db07057333879484c8bc7d900b5c1bf8e ("virtio: introduce virtqueue_alloc_element")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 34065c7..6e34f05 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -599,23 +599,11 @@ static void virtqueue_undo_map_desc(unsigned int out_num, unsigned int in_num,
 
 static void virtqueue_map_iovec(VirtIODevice *vdev, struct iovec *sg,
                                 hwaddr *addr, unsigned int *num_sg,
-                                unsigned int max_size, int is_write)
+                                int is_write)
 {
     unsigned int i;
     hwaddr len;
 
-    /* Note: this function MUST validate input, some callers
-     * are passing in num_sg values received over the network.
-     */
-    /* TODO: teach all callers that this can fail, and return failure instead
-     * of asserting here.
-     * When we do, we might be able to re-enable NDEBUG below.
-     */
-#ifdef NDEBUG
-#error building with NDEBUG is not supported
-#endif
-    assert(*num_sg <= max_size);
-
     for (i = 0; i < *num_sg; i++) {
         len = sg[i].iov_len;
         sg[i].iov_base = dma_memory_map(vdev->dma_as,
@@ -635,13 +623,8 @@ static void virtqueue_map_iovec(VirtIODevice *vdev, struct iovec *sg,
 
 void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem)
 {
-    virtqueue_map_iovec(vdev, elem->in_sg, elem->in_addr, &elem->in_num,
-                        MIN(ARRAY_SIZE(elem->in_sg), ARRAY_SIZE(elem->in_addr)),
-                        1);
-    virtqueue_map_iovec(vdev, elem->out_sg, elem->out_addr, &elem->out_num,
-                        MIN(ARRAY_SIZE(elem->out_sg),
-                        ARRAY_SIZE(elem->out_addr)),
-                        0);
+    virtqueue_map_iovec(vdev, elem->in_sg, elem->in_addr, &elem->in_num, 1);
+    virtqueue_map_iovec(vdev, elem->out_sg, elem->out_addr, &elem->out_num, 0);
 }
 
 static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num)
@@ -840,6 +823,16 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
 
     qemu_get_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
 
+    /* TODO: teach all callers that this can fail, and return failure instead
+     * of asserting here.
+     * When we do, we might be able to re-enable NDEBUG below.
+     */
+#ifdef NDEBUG
+#error building with NDEBUG is not supported
+#endif
+    assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
+    assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
+
     elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
     elem->index = data.index;
 
-- 
MST

[Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Michael S. Tsirkin]

All users include the trailing ;, let's require that
so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
produce unexpected results.

Not a huge problem for QEMU since our style requires the use
of {} but seems cleaner nevertheless.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/compiler.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 157698b..2882470 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -86,7 +86,8 @@
 #define type_check(t1,t2) ((t1*)0 - (t2*)0)
 
 #define QEMU_BUILD_BUG_ON(x) \
-    typedef char glue(qemu_build_bug_on__,__LINE__)[(x)?-1:1] __attribute__((unused));
+    typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
+        __attribute__((unused))
 
 #if defined __GNUC__
 # if !QEMU_GNUC_PREREQ(4, 4)
-- 
MST

[Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Michael S. Tsirkin]

QEMU_BUILD_BUG_ON uses a typedef in order to be safe
to use outside functions, but sometimes it's useful
to have a version that can be used within an expression.
Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
that return zero after checking condition at build time.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/compiler.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 2882470..f4cf13b 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -89,6 +89,8 @@
     typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
         __attribute__((unused))
 
+#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
+
 #if defined __GNUC__
 # if !QEMU_GNUC_PREREQ(4, 4)
    /* gcc versions before 4.4.x don't support gnu_printf, so use printf. */
-- 
MST

[Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array

[Michael S. Tsirkin]

It's a familiar pattern: some code uses ARRAY_SIZE, then refactoring
changes the argument from an array to a pointer to a dynamically
allocated buffer.  Code keeps compiling but any ARRAY_SIZE calls now
return the size of the pointer divided by element size.

Let's add build time checks to ARRAY_SIZE before we allow more
of these in the code-base.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/osdep.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 689f253..24bfda0 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -199,7 +199,13 @@ extern int daemon(int, int);
 #endif
 
 #ifndef ARRAY_SIZE
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+/*
+ * &(x)[0] is always a pointer - if it's same type as x then the argument is a
+ * pointer, not an array as expected.
+ */
+#define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + QEMU_BUILD_BUG_ON_ZERO( \
+                        __builtin_types_compatible_p(typeof(x), \
+                                                     typeof(&(x)[0]))))
 #endif
 
 int qemu_daemon(int nochdir, int noclose);
-- 
MST

Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Peter Maydell]

On 18 January 2017 at 20:55, Michael S. Tsirkin <mst@redhat.com> wrote:
> All users include the trailing ;, let's require that
> so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
> produce unexpected results.

When would it ever make sense for a build-time
assert to be the only thing inside a runtime
conditional anyway?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 1484772931-16272-1-git-send-email-mst@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1484772931-16272-1-git-send-email-mst@redhat.com -> patchew/1484772931-16272-1-git-send-email-mst@redhat.com
Switched to a new branch 'test'
798c743 ARRAY_SIZE: check that argument is an array
f3f401d compiler: expression version of QEMU_BUILD_BUG_ON
0f5bfd8 compiler: drop ; after BUILD_BUG_ON
489ff1b virtio: fix up max size checks

=== OUTPUT BEGIN ===
Checking PATCH 1/4: virtio: fix up max size checks...
Checking PATCH 2/4: compiler: drop ; after BUILD_BUG_ON...
ERROR: space required after that ',' (ctx:VxV)
#25: FILE: include/qemu/compiler.h:89:
+    typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
                                          ^

total: 1 errors, 0 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: compiler: expression version of QEMU_BUILD_BUG_ON...
Checking PATCH 4/4: ARRAY_SIZE: check that argument is an array...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Michael S. Tsirkin]

On Wed, Jan 18, 2017 at 09:04:03PM +0000, Peter Maydell wrote:
> On 18 January 2017 at 20:55, Michael S. Tsirkin <mst@redhat.com> wrote:
> > All users include the trailing ;, let's require that
> > so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
> > produce unexpected results.
> 
> When would it ever make sense for a build-time
> assert to be the only thing inside a runtime
> conditional anyway?
> 
> thanks
> -- PMM

What do I know? Do you want to keep the trailing ; in there?
I can drop this patch.

-- 
MST

Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Michael S. Tsirkin]

On Wed, Jan 18, 2017 at 11:16:07PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 18, 2017 at 09:04:03PM +0000, Peter Maydell wrote:
> > On 18 January 2017 at 20:55, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > All users include the trailing ;, let's require that
> > > so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
> > > produce unexpected results.
> > 
> > When would it ever make sense for a build-time
> > assert to be the only thing inside a runtime
> > conditional anyway?
> > 
> > thanks
> > -- PMM
> 
> What do I know? Do you want to keep the trailing ; in there?
> I can drop this patch.

Maybe a better justification:


QEMU_BUILD_BUG_ON(0)

QEMU_BUILD_BUG_ON(0)

looks ugly but is allowed by the current implementation.
Let's drop the trailing ; within the macro to make sure all
users include the ';'.


> -- 
> MST

Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Peter Maydell]

On 18 January 2017 at 21:23, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jan 18, 2017 at 11:16:07PM +0200, Michael S. Tsirkin wrote:
>> On Wed, Jan 18, 2017 at 09:04:03PM +0000, Peter Maydell wrote:
>> > On 18 January 2017 at 20:55, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > All users include the trailing ;, let's require that
>> > > so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
>> > > produce unexpected results.
>> >
>> > When would it ever make sense for a build-time
>> > assert to be the only thing inside a runtime
>> > conditional anyway?
>> >
>> > thanks
>> > -- PMM
>>
>> What do I know? Do you want to keep the trailing ; in there?
>> I can drop this patch.
>
> Maybe a better justification:
>
>
> QEMU_BUILD_BUG_ON(0)
>
> QEMU_BUILD_BUG_ON(0)
>
> looks ugly but is allowed by the current implementation.
> Let's drop the trailing ; within the macro to make sure all
> users include the ';'.

Yeah, sorry, I should have been clearer -- I think dropping
the trailing semicolon is right, I just thought the
motivating example was a little odd. Not worth a respin
just to fix the commit message.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> All users include the trailing ;, let's require that
> so that uses such as if (a) QEMU_BUILD_BUG_ON(); do not
> produce unexpected results.
>
> Not a huge problem for QEMU since our style requires the use
> of {} but seems cleaner nevertheless.

I think the actual problem is that it sets a bad example.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/compiler.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 157698b..2882470 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -86,7 +86,8 @@
>  #define type_check(t1,t2) ((t1*)0 - (t2*)0)
>  
>  #define QEMU_BUILD_BUG_ON(x) \
> -    typedef char glue(qemu_build_bug_on__,__LINE__)[(x)?-1:1] __attribute__((unused));
> +    typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
> +        __attribute__((unused))
>  
>  #if defined __GNUC__
>  # if !QEMU_GNUC_PREREQ(4, 4)

Preferably with the commit message clarified a bit:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
> to use outside functions, but sometimes it's useful
> to have a version that can be used within an expression.
> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
> that return zero after checking condition at build time.

Following Linux's example makes sense, but I can't help but wonder
whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/compiler.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 2882470..f4cf13b 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -89,6 +89,8 @@
>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
>          __attribute__((unused))
>  
> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
> +
>  #if defined __GNUC__
>  # if !QEMU_GNUC_PREREQ(4, 4)
>     /* gcc versions before 4.4.x don't support gnu_printf, so use printf. */

More so since your QEMU_BUILD_BUG_ON_ZERO seems easier to understand: no
token pasting.

Anyway,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> It's a familiar pattern: some code uses ARRAY_SIZE, then refactoring
> changes the argument from an array to a pointer to a dynamically
> allocated buffer.  Code keeps compiling but any ARRAY_SIZE calls now
> return the size of the pointer divided by element size.
>
> Let's add build time checks to ARRAY_SIZE before we allow more
> of these in the code-base.

Yes, please!

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/osdep.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 689f253..24bfda0 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -199,7 +199,13 @@ extern int daemon(int, int);
>  #endif
>  
>  #ifndef ARRAY_SIZE
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +/*
> + * &(x)[0] is always a pointer - if it's same type as x then the argument is a
> + * pointer, not an array as expected.
> + */
> +#define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + QEMU_BUILD_BUG_ON_ZERO( \
> +                        __builtin_types_compatible_p(typeof(x), \
> +                                                     typeof(&(x)[0]))))

Please break the line near the operator, not within one of its operands:

   #define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0]))			\
                          + QEMU_BUILD_BUG_ON_ZERO(			\
                               __builtin_types_compatible_p(typeof(x),	\
                                                            typeof(&(x)[0]))))
>  #endif
>  
>  int qemu_daemon(int nochdir, int noclose);

With the confusing line break tiedied up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks

[Cornelia Huck]

On Wed, 18 Jan 2017 22:55:40 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Coverity reports that ARRAY_SIZE(elem->out_sg) (and all the others too)
> is wrong because elem->out_sg is a pointer.
> 
> However, the check is not in the right place and the max_size argument
> of virtqueue_map_iovec can be removed.  The check on in_num/out_num
> should be moved to qemu_get_virtqueue_element instead, before the call
> to virtqueue_alloc_element.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 3724650db07057333879484c8bc7d900b5c1bf8e ("virtio: introduce virtqueue_alloc_element")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Paolo Bonzini]

On 19/01/2017 09:12, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
>> to use outside functions, but sometimes it's useful
>> to have a version that can be used within an expression.
>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
>> that return zero after checking condition at build time.
> 
> Following Linux's example makes sense, but I can't help but wonder
> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().

I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
_Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.

But we can indeed redefine QEMU_BUILD_BUG_ON to
(void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
support for _Static_assert.


>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  include/qemu/compiler.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 2882470..f4cf13b 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -89,6 +89,8 @@
>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
>>          __attribute__((unused))
>>  
>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))

Linux here uses:

#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))

and the issue is that sizeof(int[(x) ? -1 : 1]) could be
runtime-evaluated (the type is a variable-length array).

Paolo

>>  #if defined __GNUC__
>>  # if !QEMU_GNUC_PREREQ(4, 4)
>>     /* gcc versions before 4.4.x don't support gnu_printf, so use printf. */
> 
> More so since your QEMU_BUILD_BUG_ON_ZERO seems easier to understand: no
> token pasting.
> 
> Anyway,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array

[Peter Maydell]

On 19 January 2017 at 08:20, Markus Armbruster <armbru@redhat.com> wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> It's a familiar pattern: some code uses ARRAY_SIZE, then refactoring
>> changes the argument from an array to a pointer to a dynamically
>> allocated buffer.  Code keeps compiling but any ARRAY_SIZE calls now
>> return the size of the pointer divided by element size.
>>
>> Let's add build time checks to ARRAY_SIZE before we allow more
>> of these in the code-base.
>
> Yes, please!
>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  include/qemu/osdep.h | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 689f253..24bfda0 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -199,7 +199,13 @@ extern int daemon(int, int);
>>  #endif
>>
>>  #ifndef ARRAY_SIZE
>> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +/*
>> + * &(x)[0] is always a pointer - if it's same type as x then the argument is a
>> + * pointer, not an array as expected.
>> + */
>> +#define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + QEMU_BUILD_BUG_ON_ZERO( \
>> +                        __builtin_types_compatible_p(typeof(x), \
>> +                                                     typeof(&(x)[0]))))
>
> Please break the line near the operator, not within one of its operands:
>
>    #define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0]))                  \
>                           + QEMU_BUILD_BUG_ON_ZERO(                     \
>                                __builtin_types_compatible_p(typeof(x),  \
>                                                             typeof(&(x)[0]))))


The other possible approach to the long-lines issue would be to do what
the Linux kernel does and abstract out a MUST_BE_ARRAY() macro.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 19/01/2017 09:12, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
>>> to use outside functions, but sometimes it's useful
>>> to have a version that can be used within an expression.
>>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
>>> that return zero after checking condition at build time.
>> 
>> Following Linux's example makes sense, but I can't help but wonder
>> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().
>
> I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
> _Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.

Okay.

> But we can indeed redefine QEMU_BUILD_BUG_ON to
> (void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
> support for _Static_assert.

Yes, please.

>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  include/qemu/compiler.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>>> index 2882470..f4cf13b 100644
>>> --- a/include/qemu/compiler.h
>>> +++ b/include/qemu/compiler.h
>>> @@ -89,6 +89,8 @@
>>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
>>>          __attribute__((unused))
>>>  
>>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
>
> Linux here uses:
>
> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>
> and the issue is that sizeof(int[(x) ? -1 : 1]) could be
> runtime-evaluated (the type is a variable-length array).

Let's copy both macros from Linux.

Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

On 01/18/2017 02:55 PM, Michael S. Tsirkin wrote:
> It's a familiar pattern: some code uses ARRAY_SIZE, then refactoring
> changes the argument from an array to a pointer to a dynamically
> allocated buffer.  Code keeps compiling but any ARRAY_SIZE calls now
> return the size of the pointer divided by element size.
> 
> Let's add build time checks to ARRAY_SIZE before we allow more
> of these in the code-base.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/osdep.h | 8 +++++++-

>  
>  #ifndef ARRAY_SIZE
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +/*
> + * &(x)[0] is always a pointer - if it's same type as x then the argument is a
> + * pointer, not an array as expected.
> + */
> +#define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + QEMU_BUILD_BUG_ON_ZERO( \
> +                        __builtin_types_compatible_p(typeof(x), \

Are we sure that __builtin_types_compatible_p() is supported for all
versions of gcc and clang that we support, or does this need further
#ifdefs?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array

[Michael S. Tsirkin]

On Thu, Jan 19, 2017 at 08:53:34AM -0600, Eric Blake wrote:
> On 01/18/2017 02:55 PM, Michael S. Tsirkin wrote:
> > It's a familiar pattern: some code uses ARRAY_SIZE, then refactoring
> > changes the argument from an array to a pointer to a dynamically
> > allocated buffer.  Code keeps compiling but any ARRAY_SIZE calls now
> > return the size of the pointer divided by element size.
> > 
> > Let's add build time checks to ARRAY_SIZE before we allow more
> > of these in the code-base.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/qemu/osdep.h | 8 +++++++-
> 
> >  
> >  #ifndef ARRAY_SIZE
> > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +/*
> > + * &(x)[0] is always a pointer - if it's same type as x then the argument is a
> > + * pointer, not an array as expected.
> > + */
> > +#define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])) + QEMU_BUILD_BUG_ON_ZERO( \
> > +                        __builtin_types_compatible_p(typeof(x), \
> 
> Are we sure that __builtin_types_compatible_p() is supported for all
> versions of gcc and clang that we support, or does this need further
> #ifdefs?
> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


We seem to use it without ifdefs elsewhere.

-- 
MST

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Michael S. Tsirkin]

On Thu, Jan 19, 2017 at 02:33:40PM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 19/01/2017 09:12, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
> >>> to use outside functions, but sometimes it's useful
> >>> to have a version that can be used within an expression.
> >>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
> >>> that return zero after checking condition at build time.
> >> 
> >> Following Linux's example makes sense, but I can't help but wonder
> >> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().
> >
> > I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
> > _Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.
> 
> Okay.
> 
> > But we can indeed redefine QEMU_BUILD_BUG_ON to
> > (void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
> > support for _Static_assert.
> 
> Yes, please.


I don't think we can because we use QEMU_BUILD_BUG_ON outside
any functions. I don't think you can put 0 there.

> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>  include/qemu/compiler.h | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> >>> index 2882470..f4cf13b 100644
> >>> --- a/include/qemu/compiler.h
> >>> +++ b/include/qemu/compiler.h
> >>> @@ -89,6 +89,8 @@
> >>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
> >>>          __attribute__((unused))
> >>>  
> >>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
> >
> > Linux here uses:
> >
> > #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
> >
> > and the issue is that sizeof(int[(x) ? -1 : 1]) could be
> > runtime-evaluated (the type is a variable-length array).
> 
> Let's copy both macros from Linux.

I prefer our variant, I don't think it's portable to assume that
sizeof(struct {int:0}) is 0. Besides, Linux code is GPLv2 only and this
file is 2 or later.

-- 
MST

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On 01/19/2017 01:25 PM, Michael S. Tsirkin wrote:

>>>>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
>>>
>>> Linux here uses:
>>>
>>> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>>
>>> and the issue is that sizeof(int[(x) ? -1 : 1]) could be
>>> runtime-evaluated (the type is a variable-length array).

Use of the struct with an int bitfield forces 'e' to be a non-zero
compile-time constant to compile; while [(x) ? -1 : 1] could indeed
compile even if not a compile-time constant.  So I agree that we want to
prefer something that forces x to be a compile-time constant.

>>
>> Let's copy both macros from Linux.
> 
> I prefer our variant, I don't think it's portable to assume that
> sizeof(struct {int:0}) is 0. Besides, Linux code is GPLv2 only and this
> file is 2 or later.

I agree that using the Linux version is problematic from a licensing
perspective, as well as the portability of getting sizeof() to produce
0.  But at least it uses bitfields to force a compile time constant.

Here's a third approach: gnulib has a LGPLv2+ version that does:

====
# define _GL_VERIFY_TYPE(R, DIAGNOSTIC) \
    struct { unsigned int _gl_verify_error_if_negative: (R) ? 1 : -1; }

#define _GL_VERIFY_TRUE(R, DIAGNOSTIC) \
   (!!sizeof (_GL_VERIFY_TYPE (R, DIAGNOSTIC)))

/* Verify requirement R at compile-time, as a declaration without a
   trailing ';'.  If R is false, fail at compile-time, preferably
   with a diagnostic that includes the string-literal DIAGNOSTIC.

   Unfortunately, unlike C11, this implementation must appear as an
   ordinary declaration, and cannot appear inside struct { ... }.  */

#ifdef _GL_HAVE__STATIC_ASSERT
# define _GL_VERIFY _Static_assert
#else
# define _GL_VERIFY(R, DIAGNOSTIC)                                     \
    extern int (*_GL_GENSYM (_gl_verify_function) (void))              \
      [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
#endif

/* Verify requirement R at compile-time.  Return the value of the
   expression E.  */

#define verify_expr(R, E) \
   (_GL_VERIFY_TRUE (R, "verify_expr (" #R ", " #E ")") ? (E) : (E))

/* Verify requirement R at compile-time, as a declaration without a
   trailing ';'.  */

#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
====

where BUILD_BUG_ON_ZERO(e) pretty much maps to verify_expr(!(e), 0), if
you want to copy any of those ideas.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Michael S. Tsirkin]

On Thu, Jan 19, 2017 at 02:33:40PM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 19/01/2017 09:12, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
> >>> to use outside functions, but sometimes it's useful
> >>> to have a version that can be used within an expression.
> >>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
> >>> that return zero after checking condition at build time.
> >> 
> >> Following Linux's example makes sense, but I can't help but wonder
> >> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().
> >
> > I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
> > _Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.
> 
> Okay.
> 
> > But we can indeed redefine QEMU_BUILD_BUG_ON to
> > (void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
> > support for _Static_assert.
> 
> Yes, please.
> 
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>  include/qemu/compiler.h | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> >>> index 2882470..f4cf13b 100644
> >>> --- a/include/qemu/compiler.h
> >>> +++ b/include/qemu/compiler.h
> >>> @@ -89,6 +89,8 @@
> >>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
> >>>          __attribute__((unused))
> >>>  
> >>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
> >
> > Linux here uses:
> >
> > #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
> >
> > and the issue is that sizeof(int[(x) ? -1 : 1]) could be
> > runtime-evaluated (the type is a variable-length array).
> 
> Let's copy both macros from Linux.

I don't like the one in Linux because it's gcc specific (sizeof is
undefined for an empty structure).

But since people are worried about compiler using a variable sized
array, we should worry about this for QEMU_BUILD_BUG_ON too. I'll fix it
up.

-- 
MST

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Michael S. Tsirkin]

On Thu, Jan 19, 2017 at 02:58:48PM -0600, Eric Blake wrote:
> On 01/19/2017 01:25 PM, Michael S. Tsirkin wrote:
> 
> >>>>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
> >>>
> >>> Linux here uses:
> >>>
> >>> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
> >>>
> >>> and the issue is that sizeof(int[(x) ? -1 : 1]) could be
> >>> runtime-evaluated (the type is a variable-length array).
> 
> Use of the struct with an int bitfield forces 'e' to be a non-zero
> compile-time constant to compile; while [(x) ? -1 : 1] could indeed
> compile even if not a compile-time constant.  So I agree that we want to
> prefer something that forces x to be a compile-time constant.
> 
> >>
> >> Let's copy both macros from Linux.
> > 
> > I prefer our variant, I don't think it's portable to assume that
> > sizeof(struct {int:0}) is 0. Besides, Linux code is GPLv2 only and this
> > file is 2 or later.
> 
> I agree that using the Linux version is problematic from a licensing
> perspective, as well as the portability of getting sizeof() to produce
> 0.  But at least it uses bitfields to force a compile time constant.
> 
> Here's a third approach: gnulib has a LGPLv2+ version that does:
> 
> ====
> # define _GL_VERIFY_TYPE(R, DIAGNOSTIC) \
>     struct { unsigned int _gl_verify_error_if_negative: (R) ? 1 : -1; }
> 
> #define _GL_VERIFY_TRUE(R, DIAGNOSTIC) \
>    (!!sizeof (_GL_VERIFY_TYPE (R, DIAGNOSTIC)))
> 
> /* Verify requirement R at compile-time, as a declaration without a
>    trailing ';'.  If R is false, fail at compile-time, preferably
>    with a diagnostic that includes the string-literal DIAGNOSTIC.
> 
>    Unfortunately, unlike C11, this implementation must appear as an
>    ordinary declaration, and cannot appear inside struct { ... }.  */
> 
> #ifdef _GL_HAVE__STATIC_ASSERT
> # define _GL_VERIFY _Static_assert
> #else
> # define _GL_VERIFY(R, DIAGNOSTIC)                                     \
>     extern int (*_GL_GENSYM (_gl_verify_function) (void))              \
>       [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
> #endif
> 
> /* Verify requirement R at compile-time.  Return the value of the
>    expression E.  */
> 
> #define verify_expr(R, E) \
>    (_GL_VERIFY_TRUE (R, "verify_expr (" #R ", " #E ")") ? (E) : (E))
> 
> /* Verify requirement R at compile-time, as a declaration without a
>    trailing ';'.  */
> 
> #define verify(R) _GL_VERIFY (R, "verify (" #R ")")
> ====
> 
> where BUILD_BUG_ON_ZERO(e) pretty much maps to verify_expr(!(e), 0), if
> you want to copy any of those ideas.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 




I coded up something similar independently.
Will post in a minute.

-- 
MST

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Jan 19, 2017 at 02:33:40PM +0100, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 19/01/2017 09:12, Markus Armbruster wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> 
>> >>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
>> >>> to use outside functions, but sometimes it's useful
>> >>> to have a version that can be used within an expression.
>> >>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
>> >>> that return zero after checking condition at build time.
>> >> 
>> >> Following Linux's example makes sense, but I can't help but wonder
>> >> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().
>> >
>> > I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
>> > _Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.
>> 
>> Okay.
>> 
>> > But we can indeed redefine QEMU_BUILD_BUG_ON to
>> > (void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
>> > support for _Static_assert.
>> 
>> Yes, please.
>
>
> I don't think we can because we use QEMU_BUILD_BUG_ON outside
> any functions. I don't think you can put 0 there.

Point taken.  Still, we should be able to factor out a common core of
the bug condition.

>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >>> ---
>> >>>  include/qemu/compiler.h | 2 ++
>> >>>  1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> >>> index 2882470..f4cf13b 100644
>> >>> --- a/include/qemu/compiler.h
>> >>> +++ b/include/qemu/compiler.h
>> >>> @@ -89,6 +89,8 @@
>> >>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
>> >>>          __attribute__((unused))
>> >>>  
>> >>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
>> >
>> > Linux here uses:
>> >
>> > #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>> >
>> > and the issue is that sizeof(int[(x) ? -1 : 1]) could be
>> > runtime-evaluated (the type is a variable-length array).
>> 
>> Let's copy both macros from Linux.
>
> I prefer our variant, I don't think it's portable to assume that
> sizeof(struct {int:0}) is 0. Besides, Linux code is GPLv2 only and this
> file is 2 or later.

Use (sizeof(struct { int: X }) - sizeof(struct { int: 0 })) if you care
for portability to lesser compilers.  I don't, because we use common
extensions supported by both GCC and Clang all over the place.

The idea to use bitfield size is not copyrightable, only expressions of
the idea.

Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON

[Michael S. Tsirkin]

On Fri, Jan 20, 2017 at 08:21:28AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jan 19, 2017 at 02:33:40PM +0100, Markus Armbruster wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > On 19/01/2017 09:12, Markus Armbruster wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> 
> >> >>> QEMU_BUILD_BUG_ON uses a typedef in order to be safe
> >> >>> to use outside functions, but sometimes it's useful
> >> >>> to have a version that can be used within an expression.
> >> >>> Following what Linux does, introduce QEMU_BUILD_BUG_ON_ZERO
> >> >>> that return zero after checking condition at build time.
> >> >> 
> >> >> Following Linux's example makes sense, but I can't help but wonder
> >> >> whether we need both QEMU_BUILD_BUG_ON_ZERO() and QEMU_BUILD_BUG_ON().
> >> >
> >> > I think so, most notably QEMU_BUILD_BUG_ON was added to C11 as
> >> > _Static_assert but QEMU_BUILD_BUG_ON_ZERO wasn't.
> >> 
> >> Okay.
> >> 
> >> > But we can indeed redefine QEMU_BUILD_BUG_ON to
> >> > (void)QEMU_BUILD_BUG_ON_ZERO(x) like Linux does, until we add optional
> >> > support for _Static_assert.
> >> 
> >> Yes, please.
> >
> >
> > I don't think we can because we use QEMU_BUILD_BUG_ON outside
> > any functions. I don't think you can put 0 there.
> 
> Point taken.  Still, we should be able to factor out a common core of
> the bug condition.
> 
> >> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> >>> ---
> >> >>>  include/qemu/compiler.h | 2 ++
> >> >>>  1 file changed, 2 insertions(+)
> >> >>>
> >> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> >> >>> index 2882470..f4cf13b 100644
> >> >>> --- a/include/qemu/compiler.h
> >> >>> +++ b/include/qemu/compiler.h
> >> >>> @@ -89,6 +89,8 @@
> >> >>>      typedef char glue(qemu_build_bug_on__,__LINE__)[(x) ? -1 : 1] \
> >> >>>          __attribute__((unused))
> >> >>>  
> >> >>> +#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(int[(x) ? -1 : 1]) - sizeof(int))
> >> >
> >> > Linux here uses:
> >> >
> >> > #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
> >> >
> >> > and the issue is that sizeof(int[(x) ? -1 : 1]) could be
> >> > runtime-evaluated (the type is a variable-length array).
> >> 
> >> Let's copy both macros from Linux.
> >
> > I prefer our variant, I don't think it's portable to assume that
> > sizeof(struct {int:0}) is 0. Besides, Linux code is GPLv2 only and this
> > file is 2 or later.
> 
> Use (sizeof(struct { int: X }) - sizeof(struct { int: 0 })) if you care
> for portability to lesser compilers.  I don't, because we use common
> extensions supported by both GCC and Clang all over the place.
> 
> The idea to use bitfield size is not copyrightable, only expressions of
> the idea.

Right I did that before reading this reply.

-- 
MST