Message ID | 20200326170121.13045-1-alex.bennee@linaro.org |
---|---|
State | New |
Headers | show |
Series | qemu/atomic.h: add #ifdef guards for stdatomic.h | expand |
On Thu, 26 Mar 2020 at 17:01, Alex Bennée <alex.bennee@linaro.org> wrote: > > Deep inside the FreeBSD netmap headers we end up including stdatomic.h > which clashes with qemu's atomic functions which are modelled along > the C11 standard. To avoid a massive rename lets just ifdef around the > problem. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/qemu/atomic.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index f9cd24c8994..ff72db51154 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -208,11 +208,14 @@ > /* Provide shorter names for GCC atomic builtins, return old value */ > #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) > #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) > + > +#ifndef atomic_fetch_add > #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) > +#endif This will work around FreeBSD's current implementation in particular, but I don't think there's anything in the C11 spec that mandates that atomic_fetch_add() and friends have to be macros and not simply functions... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 26 Mar 2020 at 17:01, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Deep inside the FreeBSD netmap headers we end up including stdatomic.h >> which clashes with qemu's atomic functions which are modelled along >> the C11 standard. To avoid a massive rename lets just ifdef around the >> problem. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/qemu/atomic.h | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >> index f9cd24c8994..ff72db51154 100644 >> --- a/include/qemu/atomic.h >> +++ b/include/qemu/atomic.h >> @@ -208,11 +208,14 @@ >> /* Provide shorter names for GCC atomic builtins, return old value */ >> #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) >> #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) >> + >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) >> +#endif > > This will work around FreeBSD's current implementation in particular, > but I don't think there's anything in the C11 spec that mandates that > atomic_fetch_add() and friends have to be macros and not simply > functions... Sure there are two alternative options: - Move to using stdatomic headers - on Linux they seem to be C++ only - Rename all out atomic functions - seems a bit of a big patch for rc releases I suspect we should look at option two for 5.1
On 26/03/20 18:14, Peter Maydell wrote: >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) >> +#endif > > This will work around FreeBSD's current implementation in particular, > but I don't think there's anything in the C11 spec that mandates that > atomic_fetch_add() and friends have to be macros and not simply > functions... That's not a problem as long as they are all functions, the macros would simply override the function-based implementation. Paolo
On 3/26/20 10:01 AM, Alex Bennée wrote: > + > +#ifndef atomic_fetch_add > #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) > +#endif Do we really get sequential consistency from <stdatomic.h>? Should we not in fact #undef as a workaround instead? r~
On 26/03/20 20:58, Richard Henderson wrote: >> + >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) >> +#endif > Do we really get sequential consistency from <stdatomic.h>? Yes, it's the default value (to pass a memory order you need atomic_fetch_*_explicit). Paolo > Should we not in fact #undef as a workaround instead?
On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 26/03/20 18:14, Peter Maydell wrote: > >> +#ifndef atomic_fetch_add > >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) > >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) > >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) > >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) > >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) > >> +#endif > > > > This will work around FreeBSD's current implementation in particular, > > but I don't think there's anything in the C11 spec that mandates that > > atomic_fetch_add() and friends have to be macros and not simply > > functions... > > That's not a problem as long as they are all functions, the macros would > simply override the function-based implementation. Oh yes, so it would. I think I was also vaguely thinking in terms of FreeBSD being the leading edge of "one day most or all of our hosts will have a full stdatomic.h", so maybe we should shift to use-host-stdatomic-by-default, with the use of the gcc __atomic* as the fallback at some point ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 26/03/20 18:14, Peter Maydell wrote: >> >> +#ifndef atomic_fetch_add >> >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) >> >> +#endif >> > >> > This will work around FreeBSD's current implementation in particular, >> > but I don't think there's anything in the C11 spec that mandates that >> > atomic_fetch_add() and friends have to be macros and not simply >> > functions... >> >> That's not a problem as long as they are all functions, the macros would >> simply override the function-based implementation. > > Oh yes, so it would. I think I was also vaguely thinking in terms > of FreeBSD being the leading edge of "one day most or all of our > hosts will have a full stdatomic.h", so maybe we should shift to > use-host-stdatomic-by-default, with the use of the gcc __atomic* > as the fallback at some point ? At some point but I suspect not right now. So what's the conclusion for this patch? Are people happy with it as a sticking plaster I can apply to the bounced testing PR?
On 3/27/20 2:51 AM, Alex Bennée wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> On 26/03/20 18:14, Peter Maydell wrote: >>>>> +#ifndef atomic_fetch_add >>>>> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) >>>>> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) >>>>> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) >>>>> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >>>>> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) >>>>> +#endif >>>> >>>> This will work around FreeBSD's current implementation in particular, >>>> but I don't think there's anything in the C11 spec that mandates that >>>> atomic_fetch_add() and friends have to be macros and not simply >>>> functions... >>> >>> That's not a problem as long as they are all functions, the macros would >>> simply override the function-based implementation. >> >> Oh yes, so it would. I think I was also vaguely thinking in terms >> of FreeBSD being the leading edge of "one day most or all of our >> hosts will have a full stdatomic.h", so maybe we should shift to >> use-host-stdatomic-by-default, with the use of the gcc __atomic* >> as the fallback at some point ? > > At some point but I suspect not right now. > > So what's the conclusion for this patch? Are people happy with it as a > sticking plaster I can apply to the bounced testing PR? > I guess I'm happy with it. Have a Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index f9cd24c8994..ff72db51154 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -208,11 +208,14 @@ /* Provide shorter names for GCC atomic builtins, return old value */ #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) + +#ifndef atomic_fetch_add #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) +#endif #define atomic_inc_fetch(ptr) __atomic_add_fetch(ptr, 1, __ATOMIC_SEQ_CST) #define atomic_dec_fetch(ptr) __atomic_sub_fetch(ptr, 1, __ATOMIC_SEQ_CST) @@ -392,11 +395,14 @@ /* Provide shorter names for GCC atomic builtins. */ #define atomic_fetch_inc(ptr) __sync_fetch_and_add(ptr, 1) #define atomic_fetch_dec(ptr) __sync_fetch_and_add(ptr, -1) + +#ifndef atomic_fetch_add #define atomic_fetch_add(ptr, n) __sync_fetch_and_add(ptr, n) #define atomic_fetch_sub(ptr, n) __sync_fetch_and_sub(ptr, n) #define atomic_fetch_and(ptr, n) __sync_fetch_and_and(ptr, n) #define atomic_fetch_or(ptr, n) __sync_fetch_and_or(ptr, n) #define atomic_fetch_xor(ptr, n) __sync_fetch_and_xor(ptr, n) +#endif #define atomic_inc_fetch(ptr) __sync_add_and_fetch(ptr, 1) #define atomic_dec_fetch(ptr) __sync_add_and_fetch(ptr, -1)
Deep inside the FreeBSD netmap headers we end up including stdatomic.h which clashes with qemu's atomic functions which are modelled along the C11 standard. To avoid a massive rename lets just ifdef around the problem. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/qemu/atomic.h | 6 ++++++ 1 file changed, 6 insertions(+)