On 21/06/19 13:01 -0400, Nathan Sidwell wrote:
On 6/21/19 12:01 PM, Jonathan Wakely wrote:
Nathan noticed that the 'static inline' functions in <ext/atomicity.h>
cause ODR violations when used from inline functions or templates (see
[basic.def.odr] p12 bullet (12.2)). His modules branch now diagnoses
those violations, so we need a fix.
Looking at the history (r114044 by Paolo) I believe the idea was indeed
to allow different definitions to be used in different TUs. I think
__attribute__((__always_inline__)) is a better match for that than
'static inline', and doesn't violate the ODR (at least, not if all TUs
have the same values for __GTHREADS and _GLIBCXX_ATOMIC_BUILTINS).
These functions still violate this rule in [dcl.inline]:
C++17: "If a function with external linkage is declared inline in one
translation unit, it shall be declared inline in all translation units
in which it appears; no diagnostic is required."
C++2a WP: "If a function or variable with external or module linkage
is declared inline in one translation unit, there shall be a reachable
inline declaration in all translation units in which it is declared;
no diagnostic is required."
But that doesn't seem to be diagnosable by today's implementations.
Does this change seem reasonable?
yes, thanks!
Actually, I think I prefer this version. This produces identical code
to the always_inline version, but without the indirection to
additional inline functions, i.e. just inline the relevant code into
the _dispatch functions.
Tests are still running but no failures so far, as expected.
commit 56040dad3b55a98f6e5090d04cb49945c40df39c
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Fri Jun 21 17:52:23 2019 +0100
Simplify
diff --git a/libstdc++-v3/include/ext/atomicity.h b/libstdc++-v3/include/ext/atomicity.h
index 0783a58e01a..30453c92315 100644
--- a/libstdc++-v3/include/ext/atomicity.h
+++ b/libstdc++-v3/include/ext/atomicity.h
@@ -43,62 +43,49 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// To abstract locking primitives across all thread policies, use:
// __exchange_and_add_dispatch
// __atomic_add_dispatch
-#ifdef _GLIBCXX_ATOMIC_BUILTINS
- static inline _Atomic_word
- __exchange_and_add(volatile _Atomic_word* __mem, int __val)
- { return __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
-
- static inline void
- __atomic_add(volatile _Atomic_word* __mem, int __val)
- { __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
-#else
+#ifndef _GLIBCXX_ATOMIC_BUILTINS
_Atomic_word
- __attribute__ ((__unused__))
__exchange_and_add(volatile _Atomic_word*, int) throw ();
void
- __attribute__ ((__unused__))
__atomic_add(volatile _Atomic_word*, int) throw ();
#endif
- static inline _Atomic_word
- __exchange_and_add_single(_Atomic_word* __mem, int __val)
+ inline _Atomic_word
+ __attribute__ ((__always_inline__))
+ __exchange_and_add_dispatch(_Atomic_word* __mem, int __val)
{
+#ifdef __GTHREADS
+ if (__gthread_active_p())
+ {
+#ifdef _GLIBCXX_ATOMIC_BUILTINS
+ return __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL);
+#else
+ return __exchange_and_add(__mem, __val);
+#endif
+ }
+#endif
_Atomic_word __result = *__mem;
*__mem += __val;
return __result;
}
- static inline void
- __atomic_add_single(_Atomic_word* __mem, int __val)
- { *__mem += __val; }
-
- static inline _Atomic_word
- __attribute__ ((__unused__))
- __exchange_and_add_dispatch(_Atomic_word* __mem, int __val)
- {
-#ifdef __GTHREADS
- if (__gthread_active_p())
- return __exchange_and_add(__mem, __val);
- else
- return __exchange_and_add_single(__mem, __val);
-#else
- return __exchange_and_add_single(__mem, __val);
-#endif
- }
-
- static inline void
- __attribute__ ((__unused__))
+ inline void
+ __attribute__ ((__always_inline__))
__atomic_add_dispatch(_Atomic_word* __mem, int __val)
{
#ifdef __GTHREADS
if (__gthread_active_p())
- __atomic_add(__mem, __val);
- else
- __atomic_add_single(__mem, __val);
+ {
+#ifdef _GLIBCXX_ATOMIC_BUILTINS
+ __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL);
#else
- __atomic_add_single(__mem, __val);
+ __atomic_add(__mem, __val);
#endif
+ }
+ else
+#endif
+ *__mem += __val;
}
_GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/src/c++98/mt_allocator.cc b/libstdc++-v3/src/c++98/mt_allocator.cc
index f842c6f9cfd..9b26f3dc993 100644
--- a/libstdc++-v3/src/c++98/mt_allocator.cc
+++ b/libstdc++-v3/src/c++98/mt_allocator.cc
@@ -302,7 +302,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
if (__reclaimed > 1024)
{
__bin._M_used[__thread_id] -= __reclaimed;
- __atomic_add(&__reclaimed_base[__thread_id], -__reclaimed);
+ __atomic_fetch_add(&__reclaimed_base[__thread_id], -__reclaimed,
+ __ATOMIC_ACQ_REL);
}
if (__remove >= __net_used)
@@ -332,7 +333,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
if (__block->_M_thread_id == __thread_id)
--__bin._M_used[__thread_id];
else
- __atomic_add(&__reclaimed_base[__block->_M_thread_id], 1);
+ __atomic_fetch_add(&__reclaimed_base[__block->_M_thread_id], 1,
+ __ATOMIC_ACQ_REL);
__block->_M_next = __bin._M_first[__thread_id];
__bin._M_first[__thread_id] = __block;
@@ -379,7 +381,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
reinterpret_cast<_Atomic_word*>(__bin._M_used + __max_threads);
const _Atomic_word __reclaimed = __reclaimed_base[__thread_id];
__bin._M_used[__thread_id] -= __reclaimed;
- __atomic_add(&__reclaimed_base[__thread_id], -__reclaimed);
+ __atomic_fetch_add(&__reclaimed_base[__thread_id], -__reclaimed,
+ __ATOMIC_ACQ_REL);
__gthread_mutex_lock(__bin._M_mutex);
if (__bin._M_first[0] == 0)
