On Tue, Oct 17, 2017 at 1:35 PM, Ben Pfaff <[email protected]> wrote: > On Mon, Oct 16, 2017 at 05:08:34PM -0700, Yi-Hung Wei wrote: >> Thanks for the patch. I can verify that it works great with C++14 support. >> >> One minor problem is that it may run into some compiler errors if the >> C++ compiler does not support C++14 or the support is not enabled. >> There was a patch that hit similar issue: >> https://patchwork.ozlabs.org/patch/813112/ > > This is a good point. What if we fold in the following incremental? Thanks, I think it resolves the aforementioned issue. Just a few minor picks below.
Acked-by: Yi-Hung Wei <[email protected]> > I'm also appending the full patch. > > diff --git a/lib/ovs-atomic.h b/lib/ovs-atomic.h > index 5e4a9c1775b3..c5299fd08512 100644 > --- a/lib/ovs-atomic.h > +++ b/lib/ovs-atomic.h > @@ -325,11 +325,11 @@ > #include "ovs-atomic-pthreads.h" > #elif __has_extension(c_atomic) > #include "ovs-atomic-clang.h" > - #elif defined(__cplusplus) && HAVE_ATOMIC > + #elif HAVE_ATOMIC && __cplusplus >= 201103L > #include "ovs-atomic-c++.h" > - #elif HAVE_STDATOMIC_H > + #elif HAVE_STDATOMIC_H && !defined(__cplusplus) > #include "ovs-atomic-c11.h" > - #elif __GNUC__ >= 5 > + #elif __GNUC__ >= 5 && !defined(__cplusplus) > #error "GCC 5+ should have <stdatomic.h>" > #elif __GNUC__ >= 4 && __GNUC_MINOR__ >= 7 > #include "ovs-atomic-gcc4.7+.h" Not sure if it's worth to append this changes. - #elif __GNUC__ >= 4 && __GNUC_MINOR__ >= 7 + #elif (defined(__cplusplus) && __GNUC__ >= 5) || \ + (__GNUC__ >= 4 && __GNUC_MINOR__ >= 7) #include "ovs-atomic-gcc4.7+.h" #elif __GNUC__ && defined(__x86_64__) #include "ovs-atomic-x86_64.h" > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <[email protected]> > Date: Tue, 17 Oct 2017 13:34:29 -0700 > Subject: [PATCH] ovs-atomic: Add C++ compatible implementation. > > G++ 5 does not implement the _Atomic keyword, which is part of C11 but not > C++14, so the existing <stdatomic.h> based atomic implementation doesn't > work. This commit adds a new implementation based on the C++14 <atomic> Maybe C++11 <atomic> ? http://en.cppreference.com/w/cpp/atomic/atomic > header. > > In this area, C++ is pickier about types than C, so a few of the > definitions in ovs-atomic.h have to be updated to use more precise types > for integer constants. > > This updates the code that generates cxxtest.cc to #include <config.h> > (so that HAVE_ATOMIC is defined) and to automatically regenerate when the > program is reconfigured (because otherwise the #include <config.h>) won't > get added without a "make clean" step). > > "ovs-atomic.h" is not a public header, but apparently some code was > using it anyway. > > Fixes: 9c463631e8145 ("ovs-atomic: Report error for contradictory > configuration.") > Reported-by: Yi-Hung Wei <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > include/openvswitch/automake.mk | 10 ++++--- > lib/automake.mk | 1 + > lib/ovs-atomic-c++.h | 64 > +++++++++++++++++++++++++++++++++++++++++ > lib/ovs-atomic.h | 18 ++++++------ > m4/openvswitch.m4 | 3 ++ > 5 files changed, 84 insertions(+), 12 deletions(-) > create mode 100644 lib/ovs-atomic-c++.h > > diff --git a/include/openvswitch/automake.mk b/include/openvswitch/automake.mk > index eabeb1e971f3..6cb19b84e220 100644 > --- a/include/openvswitch/automake.mk > +++ b/include/openvswitch/automake.mk > @@ -41,10 +41,12 @@ if HAVE_CXX > # are acceptable as C++. > noinst_LTLIBRARIES += include/openvswitch/libcxxtest.la > nodist_include_openvswitch_libcxxtest_la_SOURCES = > include/openvswitch/cxxtest.cc > -include/openvswitch/cxxtest.cc: include/openvswitch/automake.mk > - $(AM_V_GEN)for header in $(openvswitchinclude_HEADERS); do \ > - echo $$header; \ > - done | sed 's,^include/\(.*\)$$,#include <\1>,' > $@ > +include/openvswitch/cxxtest.cc: \ > + include/openvswitch/automake.mk $(top_builddir)/config.status > + $(AM_V_GEN){ echo "#include <config.h>"; \ > + for header in $(openvswitchinclude_HEADERS); do \ > + echo $$header; \ > + done | sed 's,^include/\(.*\)$$,#include <\1>,'; } > $@ > endif > > # OVS does not use C++ itself, but it provides public header files > diff --git a/lib/automake.mk b/lib/automake.mk > index 2415f4cd6c25..ca1cf5dd2524 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -159,6 +159,7 @@ lib_libopenvswitch_la_SOURCES = \ > lib/ofp-version-opt.h \ > lib/ofp-version-opt.c \ > lib/ofpbuf.c \ > + lib/ovs-atomic-c++.h \ > lib/ovs-atomic-c11.h \ > lib/ovs-atomic-clang.h \ > lib/ovs-atomic-flag-gcc4.7+.h \ > diff --git a/lib/ovs-atomic-c++.h b/lib/ovs-atomic-c++.h > new file mode 100644 > index 000000000000..5bb88536d799 > --- /dev/null > +++ b/lib/ovs-atomic-c++.h > @@ -0,0 +1,64 @@ > +/* This header implements atomic operation primitives on compilers that > + * have built-in support for C11 <stdatomic.h> */ C++11 <atmoic> > +#ifndef IN_OVS_ATOMIC_H > +#error "This header should only be included indirectly via ovs-atomic.h." > +#endif > + > +#include <atomic> > + > +#define ATOMIC(TYPE) std::atomic<TYPE> > + > +using std::atomic_init; > + > +using std::memory_order_relaxed; > +using std::memory_order_consume; > +using std::memory_order_acquire; > +using std::memory_order_release; > +using std::memory_order_acq_rel; > +using std::memory_order_seq_cst; > + > +using std::atomic_thread_fence; > +using std::atomic_signal_fence; > +using std::atomic_is_lock_free; > + > +using std::atomic_store; > +using std::atomic_store_explicit; > + > +using std::atomic_compare_exchange_strong; > +using std::atomic_compare_exchange_strong_explicit; > +using std::atomic_compare_exchange_weak; > +using std::atomic_compare_exchange_weak_explicit; > + > +#define atomic_read(SRC, DST) \ > + atomic_read_explicit(SRC, DST, memory_order_seq_cst) > +#define atomic_read_explicit(SRC, DST, ORDER) \ > + (*(DST) = std::atomic_load_explicit(SRC, ORDER), \ > + (void) 0) > + > +#define atomic_add(RMW, ARG, ORIG) \ > + atomic_add_explicit(RMW, ARG, ORIG, memory_order_seq_cst) > +#define atomic_sub(RMW, ARG, ORIG) \ > + atomic_sub_explicit(RMW, ARG, ORIG, memory_order_seq_cst) > +#define atomic_or(RMW, ARG, ORIG) \ > + atomic_or_explicit(RMW, ARG, ORIG, memory_order_seq_cst) > +#define atomic_xor(RMW, ARG, ORIG) \ > + atomic_xor_explicit(RMW, ARG, ORIG, memory_order_seq_cst) > +#define atomic_and(RMW, ARG, ORIG) \ > + atomic_and_explicit(RMW, ARG, ORIG, memory_order_seq_cst) > + > +#define atomic_add_explicit(RMW, ARG, ORIG, ORDER) \ > + (*(ORIG) = std::atomic_fetch_add_explicit(RMW, ARG, ORDER), (void) 0) > +#define atomic_sub_explicit(RMW, ARG, ORIG, ORDER) \ > + (*(ORIG) = std::atomic_fetch_sub_explicit(RMW, ARG, ORDER), (void) 0) > +#define atomic_or_explicit(RMW, ARG, ORIG, ORDER) \ > + (*(ORIG) = std::atomic_fetch_or_explicit(RMW, ARG, ORDER), (void) 0) > +#define atomic_xor_explicit(RMW, ARG, ORIG, ORDER) \ > + (*(ORIG) = std::atomic_fetch_xor_explicit(RMW, ARG, ORDER), (void) 0) > +#define atomic_and_explicit(RMW, ARG, ORIG, ORDER) \ > + (*(ORIG) = std::atomic_fetch_and_explicit(RMW, ARG, ORDER), (void) 0) > + > +using std::atomic_flag; > +using std::atomic_flag_test_and_set_explicit; > +using std::atomic_flag_test_and_set; > +using std::atomic_flag_clear_explicit; > +using std::atomic_flag_clear; > diff --git a/lib/ovs-atomic.h b/lib/ovs-atomic.h > index d1b4e09e70d4..c5299fd08512 100644 > --- a/lib/ovs-atomic.h > +++ b/lib/ovs-atomic.h > @@ -325,9 +325,11 @@ > #include "ovs-atomic-pthreads.h" > #elif __has_extension(c_atomic) > #include "ovs-atomic-clang.h" > - #elif HAVE_STDATOMIC_H > + #elif HAVE_ATOMIC && __cplusplus >= 201103L > + #include "ovs-atomic-c++.h" > + #elif HAVE_STDATOMIC_H && !defined(__cplusplus) > #include "ovs-atomic-c11.h" > - #elif __GNUC__ >= 5 > + #elif __GNUC__ >= 5 && !defined(__cplusplus) > #error "GCC 5+ should have <stdatomic.h>" > #elif __GNUC__ >= 4 && __GNUC_MINOR__ >= 7 > #include "ovs-atomic-gcc4.7+.h" > @@ -446,7 +448,7 @@ atomic_count_inc(atomic_count *count) > { > unsigned int old; > > - atomic_add_relaxed(&count->count, 1, &old); > + atomic_add_relaxed(&count->count, 1u, &old); > > return old; > } > @@ -456,7 +458,7 @@ atomic_count_dec(atomic_count *count) > { > unsigned int old; > > - atomic_sub_relaxed(&count->count, 1, &old); > + atomic_sub_relaxed(&count->count, 1u, &old); > > return old; > } > @@ -486,7 +488,7 @@ struct ovs_refcount { > static inline void > ovs_refcount_init(struct ovs_refcount *refcount) > { > - atomic_init(&refcount->count, 1); > + atomic_init(&refcount->count, 1u); > } > > /* Increments 'refcount'. > @@ -498,7 +500,7 @@ ovs_refcount_ref(struct ovs_refcount *refcount) > { > unsigned int old_refcount; > > - atomic_add_explicit(&refcount->count, 1, &old_refcount, > + atomic_add_explicit(&refcount->count, 1u, &old_refcount, > memory_order_relaxed); > ovs_assert(old_refcount > 0); > } > @@ -521,7 +523,7 @@ ovs_refcount_unref(struct ovs_refcount *refcount) > { > unsigned int old_refcount; > > - atomic_sub_explicit(&refcount->count, 1, &old_refcount, > + atomic_sub_explicit(&refcount->count, 1u, &old_refcount, > memory_order_release); > ovs_assert(old_refcount > 0); > if (old_refcount == 1) { > @@ -619,7 +621,7 @@ ovs_refcount_unref_relaxed(struct ovs_refcount *refcount) > { > unsigned int old_refcount; > > - atomic_sub_explicit(&refcount->count, 1, &old_refcount, > + atomic_sub_explicit(&refcount->count, 1u, &old_refcount, > memory_order_relaxed); > ovs_assert(old_refcount > 0); > return old_refcount; > diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 > index 00ffad35f6b0..59e1352e3cf5 100644 > --- a/m4/openvswitch.m4 > +++ b/m4/openvswitch.m4 > @@ -632,6 +632,9 @@ AC_DEFUN([OVS_CHECK_CXX], > AX_CXX_COMPILE_STDCXX([11], [], [optional]) > if test $enable_Werror = yes && test $HAVE_CXX11 = 1; then > enable_cxx=: > + AC_LANG_PUSH([C++]) > + AC_CHECK_HEADERS([atomic]) > + AC_LANG_POP([C++]) > else > enable_cxx=false > fi > -- > 2.10.2 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
