[Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Michael S. Tsirkin]

Some headers use QEMU_BUILD_BUG_ON. This causes a problem
if the C file including that header happens to have
QEMU_BUILD_BUG_ON at the same line number.

Fix using a widely available extension: __COUNTER__.
If unavailable, provide a stub.

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

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index e0fb18b..bad25a9 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -89,8 +89,12 @@
     struct { \
         int:(x) ? -1 : 1; \
     }
+#ifdef __COUNTER__
 #define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \
-    glue(qemu_build_bug_on__, __LINE__) __attribute__((unused))
+    glue(qemu_build_bug_on__, __COUNTER__) __attribute__((unused))
+#else
+#define QEMU_BUILD_BUG_ON(x)
+#endif
 
 #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \
                                    sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))
-- 
MST

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 14:43, Michael S. Tsirkin <mst@redhat.com> wrote:
> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> if the C file including that header happens to have
> QEMU_BUILD_BUG_ON at the same line number.
>
> Fix using a widely available extension: __COUNTER__.
> If unavailable, provide a stub.

Specifically, it was introduced in gcc 4.3.0 (2008),
and is in at least clang 3.1 (2012).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Eric Blake]

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

On 01/31/2017 08:43 AM, Michael S. Tsirkin wrote:
> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> if the C file including that header happens to have
> QEMU_BUILD_BUG_ON at the same line number.
> 
> Fix using a widely available extension: __COUNTER__.

gcc 4.3 and later; clang documents it but doesn't say when it was added.

> If unavailable, provide a stub.

What fails in our build farm if we don't provide the stub? Can we just
require that our compiler is new enough to have __COUNTER__, or will
that break some of our existing targets?

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/compiler.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index e0fb18b..bad25a9 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -89,8 +89,12 @@
>      struct { \
>          int:(x) ? -1 : 1; \
>      }
> +#ifdef __COUNTER__

Better would be:

#if defined __COUNTER__ && __COUNTER__ != __COUNTER__

to ensure that it has the semantics we need (that's what gnulib's
LGPLv2+ verify.h uses).

>  #define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \
> -    glue(qemu_build_bug_on__, __LINE__) __attribute__((unused))
> +    glue(qemu_build_bug_on__, __COUNTER__) __attribute__((unused))

But I agree that this is the right change to make.

> +#else
> +#define QEMU_BUILD_BUG_ON(x)

This means we lose sanity checks on older compilers.  Gnulib documents
that on older compilers, you can use:

extern int (*dummy (void)) [sizeof (struct {...})];

coupled with -Wno-redundant-decls, which lets you redeclare the same
thing as many times as you want without any use of __LINE__ or
__COUNTER__.  On the other hand, I don't think we have to bend over
backwards for compilers that old (RHEL 6 is at gcc 4.4.7, but 4.3 has
__COUNTER__).

-- 
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] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 15:03, Eric Blake <eblake@redhat.com> wrote:
> On 01/31/2017 08:43 AM, Michael S. Tsirkin wrote:
>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>> if the C file including that header happens to have
>> QEMU_BUILD_BUG_ON at the same line number.
>>
>> Fix using a widely available extension: __COUNTER__.
>
> gcc 4.3 and later; clang documents it but doesn't say when it was added.
>
>> If unavailable, provide a stub.
>
> What fails in our build farm if we don't provide the stub? Can we just
> require that our compiler is new enough to have __COUNTER__, or will
> that break some of our existing targets?

I think the problem is not so much our build farm (which is
I think all recent enough to postdate introduction of
__COUNTER__) but the probability of there being users
out there who are still using older compilers to build
QEMU releases. We don't need to enforce compile time
asserts on those old compilers but given that it's
trivial to avoid actually breaking the build for them,
why not provide the do-nothing fallback?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Eric Blake]

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

On 01/31/2017 09:12 AM, Peter Maydell wrote:
> On 31 January 2017 at 15:03, Eric Blake <eblake@redhat.com> wrote:
>> On 01/31/2017 08:43 AM, Michael S. Tsirkin wrote:
>>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>>> if the C file including that header happens to have
>>> QEMU_BUILD_BUG_ON at the same line number.
>>>
>>> Fix using a widely available extension: __COUNTER__.
>>
>> gcc 4.3 and later; clang documents it but doesn't say when it was added.
>>
>>> If unavailable, provide a stub.
>>
>> What fails in our build farm if we don't provide the stub? Can we just
>> require that our compiler is new enough to have __COUNTER__, or will
>> that break some of our existing targets?
> 
> I think the problem is not so much our build farm (which is
> I think all recent enough to postdate introduction of
> __COUNTER__) but the probability of there being users
> out there who are still using older compilers to build
> QEMU releases. We don't need to enforce compile time
> asserts on those old compilers but given that it's
> trivial to avoid actually breaking the build for them,
> why not provide the do-nothing fallback?

The do-nothing fallback works for me; I'm just debating whether it is
dead code or whether we really would have a user building with a
compiler that old.

-- 
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] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 15:16, Eric Blake <eblake@redhat.com> wrote:
> The do-nothing fallback works for me; I'm just debating whether it is
> dead code or whether we really would have a user building with a
> compiler that old.

OSX 10.6's clang (whatever that was) is probably the most
likely I'd guess.

If we did want to require a newer compiler there are a
bunch of QEMU_GNUC_PREREQ(3,4) calls that could be
obsoleted. But we should probably do that as a separate
thread to get some consensus on what the minimum version
should be, and put a helpful check in configure and
a line in the release notes and so on...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Paolo Bonzini]

On 31/01/2017 10:26, Peter Maydell wrote:
> On 31 January 2017 at 15:16, Eric Blake <eblake@redhat.com> wrote:
>> The do-nothing fallback works for me; I'm just debating whether it is
>> dead code or whether we really would have a user building with a
>> compiler that old.
> 
> OSX 10.6's clang (whatever that was) is probably the most
> likely I'd guess.
> 
> If we did want to require a newer compiler there are a
> bunch of QEMU_GNUC_PREREQ(3,4) calls that could be
> obsoleted. But we should probably do that as a separate
> thread to get some consensus on what the minimum version
> should be, and put a helpful check in configure and
> a line in the release notes and so on...

We already require 4.1 for atomics.

Paolo

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 15:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> We already require 4.1 for atomics.

Oh good. I've just sent a patch which gets rid of all the
checks for "gcc must be at least X" where X is 4.1 or
earlier.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 31 January 2017 at 15:03, Eric Blake <eblake@redhat.com> wrote:
>> On 01/31/2017 08:43 AM, Michael S. Tsirkin wrote:
>>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>>> if the C file including that header happens to have
>>> QEMU_BUILD_BUG_ON at the same line number.
>>>
>>> Fix using a widely available extension: __COUNTER__.
>>
>> gcc 4.3 and later; clang documents it but doesn't say when it was added.
>>
>>> If unavailable, provide a stub.
>>
>> What fails in our build farm if we don't provide the stub? Can we just
>> require that our compiler is new enough to have __COUNTER__, or will
>> that break some of our existing targets?
>
> I think the problem is not so much our build farm (which is
> I think all recent enough to postdate introduction of
> __COUNTER__) but the probability of there being users
> out there who are still using older compilers to build
> QEMU releases. We don't need to enforce compile time
> asserts on those old compilers but given that it's
> trivial to avoid actually breaking the build for them,
> why not provide the do-nothing fallback?

I don't think we actually need __COUNTER__.

If we want it, providing a fallback is nice.  However, the "expand to
nothing" fallback is syntactically unsound.  A sound fallback expands to
the exact same syntactic construct as the real thing does, in this case
a declaration.  When it doesn't, we risk parse errors.

Of course, we may decide to accept that risk.

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Michael S. Tsirkin]

On Tue, Jan 31, 2017 at 04:34:34PM +0100, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 31 January 2017 at 15:03, Eric Blake <eblake@redhat.com> wrote:
> >> On 01/31/2017 08:43 AM, Michael S. Tsirkin wrote:
> >>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> >>> if the C file including that header happens to have
> >>> QEMU_BUILD_BUG_ON at the same line number.
> >>>
> >>> Fix using a widely available extension: __COUNTER__.
> >>
> >> gcc 4.3 and later; clang documents it but doesn't say when it was added.
> >>
> >>> If unavailable, provide a stub.
> >>
> >> What fails in our build farm if we don't provide the stub? Can we just
> >> require that our compiler is new enough to have __COUNTER__, or will
> >> that break some of our existing targets?
> >
> > I think the problem is not so much our build farm (which is
> > I think all recent enough to postdate introduction of
> > __COUNTER__) but the probability of there being users
> > out there who are still using older compilers to build
> > QEMU releases. We don't need to enforce compile time
> > asserts on those old compilers but given that it's
> > trivial to avoid actually breaking the build for them,
> > why not provide the do-nothing fallback?
> 
> I don't think we actually need __COUNTER__.

How would you fix the conflict above?

> If we want it, providing a fallback is nice.  However, the "expand to
> nothing" fallback is syntactically unsound.  A sound fallback expands
> to the exact same syntactic construct as the real thing does, in this
> case a declaration.  When it doesn't, we risk parse errors.
> Of course, we may decide to accept that risk.

That's the best I could do.

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Markus Armbruster]

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

> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> if the C file including that header happens to have
> QEMU_BUILD_BUG_ON at the same line number.
>
> Fix using a widely available extension: __COUNTER__.
> If unavailable, provide a stub.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/compiler.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index e0fb18b..bad25a9 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -89,8 +89,12 @@
>      struct { \
>          int:(x) ? -1 : 1; \
>      }
> +#ifdef __COUNTER__
>  #define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \
> -    glue(qemu_build_bug_on__, __LINE__) __attribute__((unused))
> +    glue(qemu_build_bug_on__, __COUNTER__) __attribute__((unused))
> +#else
> +#define QEMU_BUILD_BUG_ON(x)
> +#endif

__COUNTER__ was added to GNU cpp in 2007.  Good.  What about clang?
"clang -dM -E -x c /dev/null" comes up empty on my machine (3.8.0).

Can we use an extern declaration instead?  You can have any number of
these, as long as they match.  Have a look at the appended sketch.  It
compiles without -DBUGGY, and errors out with -DBUGGY, as it should.

>  
>  #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \
>                                     sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))

By the way, QEMU_BUILD_BUG_ON_ZERO() could use a comment explaining its
value.



#define QEMU_BUILD_BUG_ON_STRUCT(x) \
    struct { \
        int:(x) ? -1 : 1; \
    }

#define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \
                                   sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))

#define QEMU_BUILD_BUG_ON(x) \
    extern char qemu_build_bug_on_[QEMU_BUILD_BUG_ON_ZERO((x))]

QEMU_BUILD_BUG_ON(0);
QEMU_BUILD_BUG_ON(0);
#ifdef BUGGY
QEMU_BUILD_BUG_ON(1);
#endif

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Michael S. Tsirkin]

On Tue, Jan 31, 2017 at 04:26:13PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> > if the C file including that header happens to have
> > QEMU_BUILD_BUG_ON at the same line number.
> >
> > Fix using a widely available extension: __COUNTER__.
> > If unavailable, provide a stub.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/qemu/compiler.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > index e0fb18b..bad25a9 100644
> > --- a/include/qemu/compiler.h
> > +++ b/include/qemu/compiler.h
> > @@ -89,8 +89,12 @@
> >      struct { \
> >          int:(x) ? -1 : 1; \
> >      }
> > +#ifdef __COUNTER__
> >  #define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \
> > -    glue(qemu_build_bug_on__, __LINE__) __attribute__((unused))
> > +    glue(qemu_build_bug_on__, __COUNTER__) __attribute__((unused))
> > +#else
> > +#define QEMU_BUILD_BUG_ON(x)
> > +#endif
> 
> __COUNTER__ was added to GNU cpp in 2007.  Good.  What about clang?
> "clang -dM -E -x c /dev/null" comes up empty on my machine (3.8.0).
> 
> Can we use an extern declaration instead?  You can have any number of
> these, as long as they match.  Have a look at the appended sketch.  It
> compiles without -DBUGGY, and errors out with -DBUGGY, as it should.

I tried that first thing.
This generates lots of warnings if you have multiple users
within a function: gcc is unhappy about the redundant extern
declarations.


> >  
> >  #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \
> >                                     sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))
> 
> By the way, QEMU_BUILD_BUG_ON_ZERO() could use a comment explaining its
> value.
> 
> 
> 
> #define QEMU_BUILD_BUG_ON_STRUCT(x) \
>     struct { \
>         int:(x) ? -1 : 1; \
>     }
> 
> #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \
>                                    sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)))
> 
> #define QEMU_BUILD_BUG_ON(x) \
>     extern char qemu_build_bug_on_[QEMU_BUILD_BUG_ON_ZERO((x))]
> 
> QEMU_BUILD_BUG_ON(0);
> QEMU_BUILD_BUG_ON(0);
> #ifdef BUGGY
> QEMU_BUILD_BUG_ON(1);
> #endif

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Markus Armbruster]

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

> On Tue, Jan 31, 2017 at 04:26:13PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>> > if the C file including that header happens to have
>> > QEMU_BUILD_BUG_ON at the same line number.
>> >
>> > Fix using a widely available extension: __COUNTER__.
>> > If unavailable, provide a stub.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  include/qemu/compiler.h | 6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> > index e0fb18b..bad25a9 100644
>> > --- a/include/qemu/compiler.h
>> > +++ b/include/qemu/compiler.h
>> > @@ -89,8 +89,12 @@
>> >      struct { \
>> >          int:(x) ? -1 : 1; \
>> >      }
>> > +#ifdef __COUNTER__
>> >  #define QEMU_BUILD_BUG_ON(x) typedef QEMU_BUILD_BUG_ON_STRUCT(x) \
>> > -    glue(qemu_build_bug_on__, __LINE__) __attribute__((unused))
>> > +    glue(qemu_build_bug_on__, __COUNTER__) __attribute__((unused))
>> > +#else
>> > +#define QEMU_BUILD_BUG_ON(x)
>> > +#endif
>> 
>> __COUNTER__ was added to GNU cpp in 2007.  Good.  What about clang?
>> "clang -dM -E -x c /dev/null" comes up empty on my machine (3.8.0).
>> 
>> Can we use an extern declaration instead?  You can have any number of
>> these, as long as they match.  Have a look at the appended sketch.  It
>> compiles without -DBUGGY, and errors out with -DBUGGY, as it should.
>
> I tried that first thing.
> This generates lots of warnings if you have multiple users
> within a function: gcc is unhappy about the redundant extern
> declarations.

#define QEMU_BUILD_BUG_ON(x) \
    _Pragma("GCC diagnostic push") \
    _Pragma("GCC diagnostic ignored \"-Wredundant-decls\"") \
    extern char qemu_build_bug_on_[QEMU_BUILD_BUG_ON_ZERO((x))] __attribute__((unused)); \
    _Pragma("GCC diagnostic pop")

Drawback: natural use like

    QEMU_BUILD_BUG_ON(x);

expands into a declaration followed by an extra semicolon.

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Eric Blake]

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

On 01/31/2017 10:07 AM, Markus Armbruster wrote:
>> I tried that first thing.
>> This generates lots of warnings if you have multiple users
>> within a function: gcc is unhappy about the redundant extern
>> declarations.
> 
> #define QEMU_BUILD_BUG_ON(x) \
>     _Pragma("GCC diagnostic push") \
>     _Pragma("GCC diagnostic ignored \"-Wredundant-decls\"") \
>     extern char qemu_build_bug_on_[QEMU_BUILD_BUG_ON_ZERO((x))] __attribute__((unused)); \
>     _Pragma("GCC diagnostic pop")

Do we know if pragma GCC diagnostic push is supported in our minimum
compiler requirements (gcc 4.1, and presumably clang just ignores the
pragma)?

> 
> Drawback: natural use like
> 
>     QEMU_BUILD_BUG_ON(x);
> 
> expands into a declaration followed by an extra semicolon.

Can you emit a mid-declaration _Pragma, as in:

extern char ...[...] \
_Pragma("GCC diagnostic pop") \
__attribute__((__unused__));

Also, I guess this points out that we are inconsistent on whether we use
the __-decorated variants of all __attribute__ arguments; system headers
should always do it, since __unused__ is reserved to the implementation
but the user is free to #define unused and mess up the parse of the
header; but we aren't a system header and it obviously hasn't tripped us
up yet.

-- 
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] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 16:37, Eric Blake <eblake@redhat.com> wrote:
> On 01/31/2017 10:07 AM, Markus Armbruster wrote:
>>> I tried that first thing.
>>> This generates lots of warnings if you have multiple users
>>> within a function: gcc is unhappy about the redundant extern
>>> declarations.
>>
>> #define QEMU_BUILD_BUG_ON(x) \
>>     _Pragma("GCC diagnostic push") \
>>     _Pragma("GCC diagnostic ignored \"-Wredundant-decls\"") \
>>     extern char qemu_build_bug_on_[QEMU_BUILD_BUG_ON_ZERO((x))] __attribute__((unused)); \
>>     _Pragma("GCC diagnostic pop")
>
> Do we know if pragma GCC diagnostic push is supported in our minimum
> compiler requirements (gcc 4.1, and presumably clang just ignores the
> pragma)?

Nope; that's gcc 4.6 or newer. They're also a bit flaky between
gcc and clang because they rely on the warning being the same
in both cases. Using them in QEMU_BUILD_BUG_ON would be
massively overkill, especially since gcc 4.6 and on support
_Static_assert() which will get us better error messages
than this stuff.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Peter Maydell]

On 31 January 2017 at 15:26, Markus Armbruster <armbru@redhat.com> wrote:
> __COUNTER__ was added to GNU cpp in 2007.  Good.  What about clang?
> "clang -dM -E -x c /dev/null" comes up empty on my machine (3.8.0).

Since __COUNTER__ is magic it doesn't appear in the -dM output.
You can test for it working with
echo 'counter is __COUNTER__ __COUNTER__' | clang -E -x c -

thanks
-- PMM

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Daniel P. Berrange]

On Tue, Jan 31, 2017 at 04:43:52PM +0200, Michael S. Tsirkin wrote:
> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> if the C file including that header happens to have
> QEMU_BUILD_BUG_ON at the same line number.

Do we actually hit this problem in practice ?  Even if we do hit the
problem, it surely has a trivial workaround of just inserting/removing
a blank line somewhere in the file before the QEMU_BUILD_BUG_ON.

IOW, is it really a benefit to change to use COUNTER, given that it is
less portable that what we have today ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Michael S. Tsirkin]

On Tue, Jan 31, 2017 at 04:50:52PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 31, 2017 at 04:43:52PM +0200, Michael S. Tsirkin wrote:
> > Some headers use QEMU_BUILD_BUG_ON. This causes a problem
> > if the C file including that header happens to have
> > QEMU_BUILD_BUG_ON at the same line number.
> 
> Do we actually hit this problem in practice ?  Even if we do hit the
> problem, it surely has a trivial workaround of just inserting/removing
> a blank line somewhere in the file before the QEMU_BUILD_BUG_ON.
> 
> IOW, is it really a benefit to change to use COUNTER, given that it is
> less portable that what we have today ?
> 
> Regards,
> Daniel

It failed for me, and I'm not inclined to insert empty lines
to work around this for reasons for taste :)

> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Jan 31, 2017 at 04:43:52PM +0200, Michael S. Tsirkin wrote:
>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>> if the C file including that header happens to have
>> QEMU_BUILD_BUG_ON at the same line number.
>
> Do we actually hit this problem in practice ?  Even if we do hit the
> problem, it surely has a trivial workaround of just inserting/removing
> a blank line somewhere in the file before the QEMU_BUILD_BUG_ON.
>
> IOW, is it really a benefit to change to use COUNTER, given that it is
> less portable that what we have today ?

__COUNTER__ is the best solution I've seen so far.

The proposed fallback for compilers lacking __COUNTER__ is not quite
correct, but it's relatively unlikely to break, and I'm vanishingly
unlikely to compile with such an old compiler ever again, so I'm okay
with it.  I'd also be okay with making __COUNTER__ support a hard
requirement.

I guess that means:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] QEMU_BUILD_BUG_ON: use __COUNTER__

[Eric Blake]

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

On 01/31/2017 12:43 PM, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
>> On Tue, Jan 31, 2017 at 04:43:52PM +0200, Michael S. Tsirkin wrote:
>>> Some headers use QEMU_BUILD_BUG_ON. This causes a problem
>>> if the C file including that header happens to have
>>> QEMU_BUILD_BUG_ON at the same line number.
>>
>> Do we actually hit this problem in practice ?  Even if we do hit the
>> problem, it surely has a trivial workaround of just inserting/removing
>> a blank line somewhere in the file before the QEMU_BUILD_BUG_ON.
>>
>> IOW, is it really a benefit to change to use COUNTER, given that it is
>> less portable that what we have today ?
> 
> __COUNTER__ is the best solution I've seen so far.
> 
> The proposed fallback for compilers lacking __COUNTER__ is not quite
> correct, but it's relatively unlikely to break, and I'm vanishingly
> unlikely to compile with such an old compiler ever again, so I'm okay
> with it.  I'd also be okay with making __COUNTER__ support a hard
> requirement.

Making it a hard requirement is as simple as:

#if !defined __COUNTER__ || __COUNTER__ == __COUNTER__
#error Upgrade your compiler
#endif

> 
> I guess that means:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Since the v4 pull request was botched, if you'd like to add R-b for the
v5 pull request, you can also add

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]