* [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks
2017-01-18 20:55 [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups Michael S. Tsirkin
@ 2017-01-18 20:55 ` Michael S. Tsirkin
2017-01-19 9:46 ` Cornelia Huck
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON Michael S. Tsirkin
` (3 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, qemu-stable
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks Michael S. Tsirkin
@ 2017-01-19 9:46 ` Cornelia Huck
0 siblings, 0 replies; 25+ messages in thread
From: Cornelia Huck @ 2017-01-19 9:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, pbonzini, qemu-stable
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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 20:55 [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups Michael S. Tsirkin
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks Michael S. Tsirkin
@ 2017-01-18 20:55 ` Michael S. Tsirkin
2017-01-18 21:04 ` Peter Maydell
2017-01-19 8:09 ` Markus Armbruster
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON Michael S. Tsirkin
` (2 subsequent siblings)
4 siblings, 2 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Peter Maydell, Eric Blake, Richard Henderson
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON Michael S. Tsirkin
@ 2017-01-18 21:04 ` Peter Maydell
2017-01-18 21:16 ` Michael S. Tsirkin
2017-01-19 8:09 ` Markus Armbruster
1 sibling, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2017-01-18 21:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: QEMU Developers, Paolo Bonzini, Eric Blake, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 21:04 ` Peter Maydell
@ 2017-01-18 21:16 ` Michael S. Tsirkin
2017-01-18 21:23 ` Michael S. Tsirkin
0 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 21:16 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Paolo Bonzini, Eric Blake, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 21:16 ` Michael S. Tsirkin
@ 2017-01-18 21:23 ` Michael S. Tsirkin
2017-01-18 21:53 ` Peter Maydell
0 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 21:23 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Paolo Bonzini, Eric Blake, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 21:23 ` Michael S. Tsirkin
@ 2017-01-18 21:53 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2017-01-18 21:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: QEMU Developers, Paolo Bonzini, Eric Blake, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON Michael S. Tsirkin
2017-01-18 21:04 ` Peter Maydell
@ 2017-01-19 8:09 ` Markus Armbruster
1 sibling, 0 replies; 25+ messages in thread
From: Markus Armbruster @ 2017-01-19 8:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, pbonzini, Richard Henderson, Peter Maydell
"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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-18 20:55 [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups Michael S. Tsirkin
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 1/4] virtio: fix up max size checks Michael S. Tsirkin
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 2/4] compiler: drop ; after BUILD_BUG_ON Michael S. Tsirkin
@ 2017-01-18 20:55 ` Michael S. Tsirkin
2017-01-19 8:12 ` Markus Armbruster
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array Michael S. Tsirkin
2017-01-18 21:06 ` [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups no-reply
4 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Peter Maydell, Eric Blake, Richard Henderson
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON Michael S. Tsirkin
@ 2017-01-19 8:12 ` Markus Armbruster
2017-01-19 10:01 ` Paolo Bonzini
0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2017-01-19 8:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, pbonzini, Richard Henderson, Peter Maydell
"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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 8:12 ` Markus Armbruster
@ 2017-01-19 10:01 ` Paolo Bonzini
2017-01-19 13:33 ` Markus Armbruster
0 siblings, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2017-01-19 10:01 UTC (permalink / raw)
To: Markus Armbruster, Michael S. Tsirkin
Cc: qemu-devel, Richard Henderson, Peter Maydell
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>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 10:01 ` Paolo Bonzini
@ 2017-01-19 13:33 ` Markus Armbruster
2017-01-19 19:25 ` Michael S. Tsirkin
2017-01-19 21:01 ` Michael S. Tsirkin
0 siblings, 2 replies; 25+ messages in thread
From: Markus Armbruster @ 2017-01-19 13:33 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S. Tsirkin, Peter Maydell, qemu-devel, Richard Henderson
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 13:33 ` Markus Armbruster
@ 2017-01-19 19:25 ` Michael S. Tsirkin
2017-01-19 20:58 ` Eric Blake
2017-01-20 7:21 ` Markus Armbruster
2017-01-19 21:01 ` Michael S. Tsirkin
1 sibling, 2 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-19 19:25 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Peter Maydell, qemu-devel, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 19:25 ` Michael S. Tsirkin
@ 2017-01-19 20:58 ` Eric Blake
2017-01-19 21:02 ` Michael S. Tsirkin
2017-01-20 7:21 ` Markus Armbruster
1 sibling, 1 reply; 25+ messages in thread
From: Eric Blake @ 2017-01-19 20:58 UTC (permalink / raw)
To: Michael S. Tsirkin, Markus Armbruster
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Peter Maydell
[-- 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 --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 20:58 ` Eric Blake
@ 2017-01-19 21:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-19 21:02 UTC (permalink / raw)
To: Eric Blake
Cc: Markus Armbruster, Paolo Bonzini, Richard Henderson, qemu-devel,
Peter Maydell
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 19:25 ` Michael S. Tsirkin
2017-01-19 20:58 ` Eric Blake
@ 2017-01-20 7:21 ` Markus Armbruster
2017-01-20 17:50 ` Michael S. Tsirkin
1 sibling, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2017-01-20 7:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Peter Maydell
"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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-20 7:21 ` Markus Armbruster
@ 2017-01-20 17:50 ` Michael S. Tsirkin
0 siblings, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-20 17:50 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Peter Maydell
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON
2017-01-19 13:33 ` Markus Armbruster
2017-01-19 19:25 ` Michael S. Tsirkin
@ 2017-01-19 21:01 ` Michael S. Tsirkin
1 sibling, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-19 21:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Peter Maydell, qemu-devel, Richard Henderson
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array
2017-01-18 20:55 [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups Michael S. Tsirkin
` (2 preceding siblings ...)
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 3/4] compiler: expression version of QEMU_BUILD_BUG_ON Michael S. Tsirkin
@ 2017-01-18 20:55 ` Michael S. Tsirkin
2017-01-19 8:20 ` Markus Armbruster
2017-01-19 14:53 ` Eric Blake
2017-01-18 21:06 ` [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups no-reply
4 siblings, 2 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 20:55 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, Eric Blake, Peter Maydell, Markus Armbruster, Sergey Fedorov
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array Michael S. Tsirkin
@ 2017-01-19 8:20 ` Markus Armbruster
2017-01-19 11:00 ` Peter Maydell
2017-01-19 14:53 ` Eric Blake
1 sibling, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2017-01-19 8:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, pbonzini, Sergey Fedorov, Peter Maydell
"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>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array
2017-01-19 8:20 ` Markus Armbruster
@ 2017-01-19 11:00 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2017-01-19 11:00 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S. Tsirkin, QEMU Developers, Paolo Bonzini, Sergey Fedorov
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array Michael S. Tsirkin
2017-01-19 8:20 ` Markus Armbruster
@ 2017-01-19 14:53 ` Eric Blake
2017-01-19 15:06 ` Michael S. Tsirkin
1 sibling, 1 reply; 25+ messages in thread
From: Eric Blake @ 2017-01-19 14:53 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: pbonzini, Peter Maydell, Markus Armbruster, Sergey Fedorov
[-- 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 --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array
2017-01-19 14:53 ` Eric Blake
@ 2017-01-19 15:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2017-01-19 15:06 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, pbonzini, Peter Maydell, Markus Armbruster, Sergey Fedorov
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups
2017-01-18 20:55 [Qemu-devel] [PATCH v2 0/4] virtio: ARRAY_SIZE fixups Michael S. Tsirkin
` (3 preceding siblings ...)
2017-01-18 20:55 ` [Qemu-devel] [PATCH v2 4/4] ARRAY_SIZE: check that argument is an array Michael S. Tsirkin
@ 2017-01-18 21:06 ` no-reply
4 siblings, 0 replies; 25+ messages in thread
From: no-reply @ 2017-01-18 21:06 UTC (permalink / raw)
To: mst; +Cc: famz, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 25+ messages in thread