| Message ID | 1484859998-25074-3-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/19/2017 03:07 PM, Michael S. Tsirkin wrote: > There are theoretical concerns that some compilers might not trigger > build failures on attempts to define an array of size -1 and make it a > variable sized array instead. Rather, the concern is that if someone changes code so that the 'x' of QEMU_BUILD_BUG_ON(x) is no longer a compile time constant, we want a compile-time failure rather than a runtime variable-sized array that may or may not crash. > Let rewrite using a struct with a negative > bit field size instead as there are no dynamic bit field sizes. This is > similar to what Linux does. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/qemu/compiler.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > I don't know if you want to tweak the commit wording, but I'm okay with the patch itself. Reviewed-by: Eric Blake <eblake@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes: > There are theoretical concerns that some compilers might not trigger > build failures on attempts to define an array of size -1 and make it a > variable sized array instead. Let rewrite using a struct with a negative > bit field size instead as there are no dynamic bit field sizes. This is > similar to what Linux does. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/qemu/compiler.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index 7512082..c6f673e 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -85,9 +85,12 @@ > #define typeof_field(type, field) typeof(((type *)0)->field) > #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)) > +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ > + struct { \ > + int qemu_build_bug_on : (x) ? -1 : 1; \ > + } The qemu_build_bug_on name space pollution is harmless, but quite unnecessary: the name can be simply omitted (unnamed bit-field). > +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ > + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) > > #if defined __GNUC__ > # if !QEMU_GNUC_PREREQ(4, 4)
Eric Blake <eblake@redhat.com> writes: > On 01/19/2017 03:07 PM, Michael S. Tsirkin wrote: >> There are theoretical concerns that some compilers might not trigger >> build failures on attempts to define an array of size -1 and make it a >> variable sized array instead. > > Rather, the concern is that if someone changes code so that the 'x' of > QEMU_BUILD_BUG_ON(x) is no longer a compile time constant, we want a > compile-time failure rather than a runtime variable-sized array that may > or may not crash. >> Let rewrite using a struct with a negative >> bit field size instead as there are no dynamic bit field sizes. This is >> similar to what Linux does. >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> include/qemu/compiler.h | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> > > I don't know if you want to tweak the commit wording, but I'm okay with > the patch itself. For what it's worth, I'd prefer a rewording.
* Michael S. Tsirkin (mst@redhat.com) wrote: > There are theoretical concerns that some compilers might not trigger > build failures on attempts to define an array of size -1 and make it a > variable sized array instead. Let rewrite using a struct with a negative > bit field size instead as there are no dynamic bit field sizes. This is > similar to what Linux does. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/qemu/compiler.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index 7512082..c6f673e 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -85,9 +85,12 @@ > #define typeof_field(type, field) typeof(((type *)0)->field) > #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)) > +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ > + struct { \ > + int qemu_build_bug_on : (x) ? -1 : 1; \ > + } The problem with this is it can't be used as an expression, where as your previous version could. I've got a similar case (see previous reply) that needed an expression bug-on that would evaluate to zero. Dave > +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ > + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) > > #if defined __GNUC__ > # if !QEMU_GNUC_PREREQ(4, 4) > -- > MST > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 01/20/2017 03:41 AM, Dr. David Alan Gilbert wrote: >> -#define QEMU_BUILD_BUG_ON(x) \ >> - typedef char glue(qemu_build_bug_on__, __LINE__)[(x) ? -1 : 1] \ >> - __attribute__((unused)) >> +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ >> + struct { \ >> + int qemu_build_bug_on : (x) ? -1 : 1; \ >> + } > > The problem with this is it can't be used as an expression, where as > your previous version could. > I've got a similar case (see previous reply) that needed an expression > bug-on that would evaluate to zero. Keep reading the series - the version usable in an expression is QEMU_BUILD_BUG_ON_ZERO, introduced in 3/4. Patch 2/4 is just rewriting the existing check, with no change in the fact that it is still a declaration.
* Eric Blake (eblake@redhat.com) wrote: > On 01/20/2017 03:41 AM, Dr. David Alan Gilbert wrote: > > >> -#define QEMU_BUILD_BUG_ON(x) \ > >> - typedef char glue(qemu_build_bug_on__, __LINE__)[(x) ? -1 : 1] \ > >> - __attribute__((unused)) > >> +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ > >> + struct { \ > >> + int qemu_build_bug_on : (x) ? -1 : 1; \ > >> + } > > > > The problem with this is it can't be used as an expression, where as > > your previous version could. > > I've got a similar case (see previous reply) that needed an expression > > bug-on that would evaluate to zero. > > Keep reading the series - the version usable in an expression is > QEMU_BUILD_BUG_ON_ZERO, introduced in 3/4. Patch 2/4 is just rewriting > the existing check, with no change in the fact that it is still a > declaration. Ah hmm, yeh that's OK. Dave > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jan 20, 2017 at 08:42:41AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > There are theoretical concerns that some compilers might not trigger > > build failures on attempts to define an array of size -1 and make it a > > variable sized array instead. Let rewrite using a struct with a negative > > bit field size instead as there are no dynamic bit field sizes. This is > > similar to what Linux does. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > include/qemu/compiler.h | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > > index 7512082..c6f673e 100644 > > --- a/include/qemu/compiler.h > > +++ b/include/qemu/compiler.h > > @@ -85,9 +85,12 @@ > > #define typeof_field(type, field) typeof(((type *)0)->field) > > #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)) > > +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ > > + struct { \ > > + int qemu_build_bug_on : (x) ? -1 : 1; \ > > + } > > The qemu_build_bug_on name space pollution is harmless, but quite > unnecessary: the name can be simply omitted (unnamed bit-field). I have concerns about it's portability though. I remember we had to get rid of unnamed fields in some structs at some point for the sake of some old compiler. > > +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ > > + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) > > > > #if defined __GNUC__ > > # if !QEMU_GNUC_PREREQ(4, 4)
On 20/01/2017 17:57, Michael S. Tsirkin wrote: > On Fri, Jan 20, 2017 at 08:42:41AM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> There are theoretical concerns that some compilers might not trigger >>> build failures on attempts to define an array of size -1 and make it a >>> variable sized array instead. Let rewrite using a struct with a negative >>> bit field size instead as there are no dynamic bit field sizes. This is >>> similar to what Linux does. >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> include/qemu/compiler.h | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h >>> index 7512082..c6f673e 100644 >>> --- a/include/qemu/compiler.h >>> +++ b/include/qemu/compiler.h >>> @@ -85,9 +85,12 @@ >>> #define typeof_field(type, field) typeof(((type *)0)->field) >>> #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)) >>> +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ >>> + struct { \ >>> + int qemu_build_bug_on : (x) ? -1 : 1; \ >>> + } >> >> The qemu_build_bug_on name space pollution is harmless, but quite >> unnecessary: the name can be simply omitted (unnamed bit-field). > > I have concerns about it's portability though. I remember > we had to get rid of unnamed fields in some structs at some point > for the sake of some old compiler. Unnamed bitfields are in C89 and we definitely use unnamed unions. Maybe that was an unnamed struct or scalar. Paolo >>> +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ >>> + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) >>> >>> #if defined __GNUC__ >>> # if !QEMU_GNUC_PREREQ(4, 4)
On Fri, Jan 20, 2017 at 06:09:52PM +0100, Paolo Bonzini wrote: > > > On 20/01/2017 17:57, Michael S. Tsirkin wrote: > > On Fri, Jan 20, 2017 at 08:42:41AM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >>> There are theoretical concerns that some compilers might not trigger > >>> build failures on attempts to define an array of size -1 and make it a > >>> variable sized array instead. Let rewrite using a struct with a negative > >>> bit field size instead as there are no dynamic bit field sizes. This is > >>> similar to what Linux does. > >>> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> include/qemu/compiler.h | 9 ++++++--- > >>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > >>> index 7512082..c6f673e 100644 > >>> --- a/include/qemu/compiler.h > >>> +++ b/include/qemu/compiler.h > >>> @@ -85,9 +85,12 @@ > >>> #define typeof_field(type, field) typeof(((type *)0)->field) > >>> #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)) > >>> +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ > >>> + struct { \ > >>> + int qemu_build_bug_on : (x) ? -1 : 1; \ > >>> + } > >> > >> The qemu_build_bug_on name space pollution is harmless, but quite > >> unnecessary: the name can be simply omitted (unnamed bit-field). > > > > I have concerns about it's portability though. I remember > > we had to get rid of unnamed fields in some structs at some point > > for the sake of some old compiler. > > Unnamed bitfields are in C89 and we definitely use unnamed unions. > Maybe that was an unnamed struct or scalar. > > Paolo I don't think we use unnamed bitfields anywhere though. do we? > >>> +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ > >>> + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) > >>> > >>> #if defined __GNUC__ > >>> # if !QEMU_GNUC_PREREQ(4, 4)
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Jan 20, 2017 at 06:09:52PM +0100, Paolo Bonzini wrote: >> >> >> On 20/01/2017 17:57, Michael S. Tsirkin wrote: >> > On Fri, Jan 20, 2017 at 08:42:41AM +0100, Markus Armbruster wrote: >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> >> >>> There are theoretical concerns that some compilers might not trigger >> >>> build failures on attempts to define an array of size -1 and make it a >> >>> variable sized array instead. Let rewrite using a struct with a negative >> >>> bit field size instead as there are no dynamic bit field sizes. This is >> >>> similar to what Linux does. >> >>> >> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >>> --- >> >>> include/qemu/compiler.h | 9 ++++++--- >> >>> 1 file changed, 6 insertions(+), 3 deletions(-) >> >>> >> >>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h >> >>> index 7512082..c6f673e 100644 >> >>> --- a/include/qemu/compiler.h >> >>> +++ b/include/qemu/compiler.h >> >>> @@ -85,9 +85,12 @@ >> >>> #define typeof_field(type, field) typeof(((type *)0)->field) >> >>> #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)) >> >>> +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ >> >>> + struct { \ >> >>> + int qemu_build_bug_on : (x) ? -1 : 1; \ >> >>> + } >> >> >> >> The qemu_build_bug_on name space pollution is harmless, but quite >> >> unnecessary: the name can be simply omitted (unnamed bit-field). >> > >> > I have concerns about it's portability though. I remember >> > we had to get rid of unnamed fields in some structs at some point >> > for the sake of some old compiler. >> >> Unnamed bitfields are in C89 and we definitely use unnamed unions. >> Maybe that was an unnamed struct or scalar. >> >> Paolo > > I don't think we use unnamed bitfields anywhere though. do we? If we were talking about some obscure GCC extension, this would be a valid question. But we're talking about an ISO C feature that's pretty central to how bit-fields work, and older than quite a few hackers. [...]
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 7512082..c6f673e 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -85,9 +85,12 @@ #define typeof_field(type, field) typeof(((type *)0)->field) #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)) +#define QEMU_BUILD_BUG_ON_STRUCT(x) \ + struct { \ + int qemu_build_bug_on : (x) ? -1 : 1; \ + } +#define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \ + glue(qemu_build_bug_on__, __LINE__) __attribute__((unused)) #if defined __GNUC__ # if !QEMU_GNUC_PREREQ(4, 4)
There are theoretical concerns that some compilers might not trigger build failures on attempts to define an array of size -1 and make it a variable sized array instead. Let rewrite using a struct with a negative bit field size instead as there are no dynamic bit field sizes. This is similar to what Linux does. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/qemu/compiler.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)