| Message ID | 1484772931-16272-5-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
"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>
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
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?
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.
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);
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(-)