| Message ID | 1484772931-16272-4-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
"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>
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> >
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.
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.
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.
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.
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.
"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.
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.
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. */
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(+)