[PATCH] Fix ODR violations in code using <ext/atomicity.h>

[PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

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

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?

It does change the size of a few files, e.g. for x86_64 some are
bigger and some are smaller, overall slightly smaller:

   text	   data	    bss	    dec	    hex	filename

   5400	      8	    352	   5760	   1680	src/c++11/cow-locale_init.o
   5402	      8	    352	   5762	   1682	src/c++11/cow-locale_init.o [always_inline]

  22772	   2056	      0	  24828	   60fc	src/c++11/cow-shim_facets.o
  22710	   2056	      0	  24766	   60be	src/c++11/cow-shim_facets.o [always_inline]

  49458	   2184	      0	  51642	   c9ba	src/c++11/cow-sstream-inst.o
  49482	   2184	      0	  51666	   c9d2	src/c++11/cow-sstream-inst.o [always_inline]

   7461	      8	      0	   7469	   1d2d	src/c++11/cow-stdexcept.o
   7464	      8	      0	   7472	   1d30	src/c++11/cow-stdexcept.o [always_inline]

  16753	      8	     32	  16793	   4199	src/c++11/cow-string-inst.o
  16756	      8	     32	  16796	   419c	src/c++11/cow-string-inst.o [always_inline]

  17009	      8	     32	  17049	   4299	src/c++11/cow-wstring-inst.o
  17008	      8	     32	  17048	   4298	src/c++11/cow-wstring-inst.o [always_inline]

  20342	   2072	      0	  22414	   578e	src/c++11/cxx11-shim_facets.o
  20276	   2072	      0	  22348	   574c	src/c++11/cxx11-shim_facets.o [always_inline]

   2929	    240	      8	   3177	    c69	src/c++11/future.o
   2945	    240	      8	   3193	    c79	src/c++11/future.o [always_inline]

   6789	    120	      0	   6909	   1afd	src/c++11/ios-inst.o
   6750	    120	      0	   6870	   1ad6	src/c++11/ios-inst.o [always_inline]

   2364	     57	      8	   2429	    97d	src/c++11/ios.o
   2362	     57	      8	   2427	    97b	src/c++11/ios.o [always_inline]

  94493	   2712	    192	  97397	  17c75	src/c++11/locale-inst.o
  94498	   2712	    192	  97402	  17c7a	src/c++11/locale-inst.o [always_inline]

  93094	   2712	    192	  95998	  176fe	src/c++11/wlocale-inst.o
  93113	   2712	    192	  96017	  17711	src/c++11/wlocale-inst.o [always_inline]

  20189	    208	      0	  20397	   4fad	src/c++17/cow-fs_dir.o
  20118	    208	      0	  20326	   4f66	src/c++17/cow-fs_dir.o [always_inline]

  55077	      8	      0	  55085	   d72d	src/c++17/cow-fs_ops.o
  55103	      8	      0	  55111	   d747	src/c++17/cow-fs_ops.o [always_inline]

  40200	    304	      0	  40504	   9e38	src/c++17/cow-fs_path.o
  40204	    304	      0	  40508	   9e3c	src/c++17/cow-fs_path.o [always_inline]

  18672	    208	      0	  18880	   49c0	src/c++17/fs_dir.o
  18628	    208	      0	  18836	   4994	src/c++17/fs_dir.o [always_inline]

  60604	    160	      0	  60764	   ed5c	src/c++17/fs_ops.o
  60606	    160	      0	  60766	   ed5e	src/c++17/fs_ops.o [always_inline]

  39568	    304	      0	  39872	   9bc0	src/c++17/fs_path.o
  39561	    304	      0	  39865	   9bb9	src/c++17/fs_path.o [always_inline]

   3653	      8	      0	   3661	    e4d	src/c++98/ios_init.o
   3656	      8	      0	   3664	    e50	src/c++98/ios_init.o [always_inline]

   6878	    184	     84	   7146	   1bea	src/c++98/locale.o
   6896	    184	     84	   7164	   1bfc	src/c++98/locale.o [always_inline]

   6693	    768	   5368	  12829	   321d	src/c++98/locale_init.o
   6710	    768	   5368	  12846	   322e	src/c++98/locale_init.o [always_inline]

   7222	      8	      0	   7230	   1c3e	src/c++98/localename.o
   7223	      8	      0	   7231	   1c3f	src/c++98/localename.o [always_inline]

   4036	    144	    208	   4388	   1124	src/c++98/pool_allocator.o
   4044	    144	    208	   4396	   112c	src/c++98/pool_allocator.o [always_inline]

   1955	    584	      0	   2539	    9eb	src/c++98/stdexcept.o
   1957	    584	      0	   2541	    9ed	src/c++98/stdexcept.o [always_inline]

  25199	    208	      0	  25407	   633f	src/filesystem/cow-dir.o
  25248	    208	      0	  25456	   6370	src/filesystem/cow-dir.o [always_inline]

  71134	      8	      0	  71142	  115e6	src/filesystem/cow-ops.o
  71146	      8	      0	  71154	  115f2	src/filesystem/cow-ops.o [always_inline]

  21995	    184	      0	  22179	   56a3	src/filesystem/cow-path.o
  22013	    184	      0	  22197	   56b5	src/filesystem/cow-path.o [always_inline]

  23708	    208	      0	  23916	   5d6c	src/filesystem/dir.o
  23690	    208	      0	  23898	   5d5a	src/filesystem/dir.o [always_inline]

  70268	    160	      0	  70428	  1131c	src/filesystem/ops.o
  70233	    160	      0	  70393	  112f9	src/filesystem/ops.o [always_inline]

 815915   15841    6476  838232   cca58 TOTAL
 815802   15841    6476  838119   cc9e7 TOTAL [always_inline]




[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2519 bytes --]

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index ac98c0d02e7..ace1c8b565e 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,11 @@
+2019-06-21  Jonathan Wakely  <jwakely@redhat.com>
+
+	* include/ext/atomicity.h [_GLIBCXX_ATOMIC_BUILTINS] (__atomic_add)
+	(__exchange_and_add): Replace 'static' specifier with always_inline
+	attribute.
+	(__exchange_and_add_single, __atomic_add_single)
+	(__exchange_and_add_dispatch, __atomic_add_dispatch): Likewise.
+
 2019-06-20  Jonathan Wakely  <jwakely@redhat.com>
 
 	* acinclude.m4 (GLIBCXX_ENABLE_DEBUG): Only do debug build for final
diff --git a/libstdc++-v3/include/ext/atomicity.h b/libstdc++-v3/include/ext/atomicity.h
index 0783a58e01a..5cee388854b 100644
--- a/libstdc++-v3/include/ext/atomicity.h
+++ b/libstdc++-v3/include/ext/atomicity.h
@@ -44,24 +44,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // __exchange_and_add_dispatch
   // __atomic_add_dispatch
 #ifdef _GLIBCXX_ATOMIC_BUILTINS
-  static inline _Atomic_word 
+  inline _Atomic_word
+  __attribute__((__always_inline__))
   __exchange_and_add(volatile _Atomic_word* __mem, int __val)
   { return __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
 
-  static inline void
+  inline void
+  __attribute__((__always_inline__))
   __atomic_add(volatile _Atomic_word* __mem, int __val)
   { __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
 #else
   _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
+  inline _Atomic_word
+  __attribute__((__always_inline__))
   __exchange_and_add_single(_Atomic_word* __mem, int __val)
   {
     _Atomic_word __result = *__mem;
@@ -69,12 +70,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return __result;
   }
 
-  static inline void
+  inline void
+  __attribute__((__always_inline__))
   __atomic_add_single(_Atomic_word* __mem, int __val)
   { *__mem += __val; }
 
-  static inline _Atomic_word
-  __attribute__ ((__unused__))
+  inline _Atomic_word
+  __attribute__ ((__always_inline__))
   __exchange_and_add_dispatch(_Atomic_word* __mem, int __val)
   {
 #ifdef __GTHREADS
@@ -87,8 +89,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #endif
   }
 
-  static inline void
-  __attribute__ ((__unused__))
+  inline void
+  __attribute__ ((__always_inline__))
   __atomic_add_dispatch(_Atomic_word* __mem, int __val)
   {
 #ifdef __GTHREADS

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Nathan Sidwell]

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!

nathan

-- 
Nathan Sidwell

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

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

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.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4128 bytes --]

commit 56040dad3b55a98f6e5090d04cb49945c40df39c
Author: Jonathan Wakely <jwakely@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)

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

On 21/06/19 18:08 +0100, Jonathan Wakely wrote:
>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.

Oops, I spoke too soon:

FAIL: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc (test for excess errors)
UNRESOLVED: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc compilation failed to produce executable
FAIL: tr1/2_general_utilities/shared_ptr/thread/mutex_weaktoshared.cc (test for excess errors)
UNRESOLVED: tr1/2_general_utilities/shared_ptr/thread/mutex_weaktoshared.cc compilation failed to produce executable

Those tests explicitly use the __atomic_add function that the patch
removes. But those would be easy to adjust.

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

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

On 21/06/19 18:13 +0100, Jonathan Wakely wrote:
>On 21/06/19 18:08 +0100, Jonathan Wakely wrote:
>>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.
>
>Oops, I spoke too soon:
>
>FAIL: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc (test for excess errors)
>UNRESOLVED: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc compilation failed to produce executable
>FAIL: tr1/2_general_utilities/shared_ptr/thread/mutex_weaktoshared.cc (test for excess errors)
>UNRESOLVED: tr1/2_general_utilities/shared_ptr/thread/mutex_weaktoshared.cc compilation failed to produce executable
>
>Those tests explicitly use the __atomic_add function that the patch
>removes. But those would be easy to adjust.

I decided against the simplification in the second patch, and
committed the attached one which is closer to the first patch I sent
(preserving the __atomic_add and __exchange_and_add functions even
when they just call the built-ins).

Tested x86_64-linux, powerpc64-linux, powerpc-aix. Committed to trunk.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3225 bytes --]

commit 0d26b6506f20dcb1c956338baba0c26f623e25f2
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jul 5 15:15:46 2019 +0100

    Fix ODR violations in code using <ext/atomicity.h>
    
    Because the inline versions of __exchange_and_add and __atomic_add are
    also marked static, they cannot be used from templates or other inline
    functions without ODR violations. This change gives them external
    linkage, but adds the always_inline attribute.
    
            * include/ext/atomicity.h [_GLIBCXX_ATOMIC_BUILTINS] (__atomic_add)
            (__exchange_and_add): Replace static specifier with always_inline
            attribute.
            (__exchange_and_add_single, __atomic_add_single): Likewise.
            (__exchange_and_add_dispatch, __atomic_add_dispatch): Likewise. Also
            combine !__gthread_active_p() and !__GTHREADS branches.

diff --git a/libstdc++-v3/include/ext/atomicity.h b/libstdc++-v3/include/ext/atomicity.h
index 0783a58e01a..73225b3de20 100644
--- a/libstdc++-v3/include/ext/atomicity.h
+++ b/libstdc++-v3/include/ext/atomicity.h
@@ -44,24 +44,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // __exchange_and_add_dispatch
   // __atomic_add_dispatch
 #ifdef _GLIBCXX_ATOMIC_BUILTINS
-  static inline _Atomic_word 
+  inline _Atomic_word
+  __attribute__((__always_inline__))
   __exchange_and_add(volatile _Atomic_word* __mem, int __val)
   { return __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
 
-  static inline void
+  inline void
+  __attribute__((__always_inline__))
   __atomic_add(volatile _Atomic_word* __mem, int __val)
   { __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
 #else
   _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
+  inline _Atomic_word
+  __attribute__((__always_inline__))
   __exchange_and_add_single(_Atomic_word* __mem, int __val)
   {
     _Atomic_word __result = *__mem;
@@ -69,36 +70,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return __result;
   }
 
-  static inline void
+  inline void
+  __attribute__((__always_inline__))
   __atomic_add_single(_Atomic_word* __mem, int __val)
   { *__mem += __val; }
 
-  static inline _Atomic_word
-  __attribute__ ((__unused__))
+  inline _Atomic_word
+  __attribute__ ((__always_inline__))
   __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
+    return __exchange_and_add_single(__mem, __val);
   }
 
-  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);
-#else
-    __atomic_add_single(__mem, __val);
 #endif
+    __atomic_add_single(__mem, __val);
   }
 
 _GLIBCXX_END_NAMESPACE_VERSION

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Daniel Krügler]

Am Fr., 5. Juli 2019 um 18:13 Uhr schrieb Jonathan Wakely <jwakely@redhat.com>:
>
[..]
> I decided against the simplification in the second patch, and
> committed the attached one which is closer to the first patch I sent
> (preserving the __atomic_add and __exchange_and_add functions even
> when they just call the built-ins).
>
> Tested x86_64-linux, powerpc64-linux, powerpc-aix. Committed to trunk.

Unrelated to the actual patch, I noticed some explicit "throw()" forms
used as exception specifications - shouldn't these be replaced by
either explicit "noexcept" or at least by a library macro that expands
to one or the other? (I'm sorry, if such unrelated questions are
considered as inappropriate for this list).

- Daniel

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

On 05/07/19 20:23 +0200, Daniel Krügler wrote:
>Am Fr., 5. Juli 2019 um 18:13 Uhr schrieb Jonathan Wakely <jwakely@redhat.com>:
>>
>[..]
>> I decided against the simplification in the second patch, and
>> committed the attached one which is closer to the first patch I sent
>> (preserving the __atomic_add and __exchange_and_add functions even
>> when they just call the built-ins).
>>
>> Tested x86_64-linux, powerpc64-linux, powerpc-aix. Committed to trunk.
>
>Unrelated to the actual patch, I noticed some explicit "throw()" forms
>used as exception specifications - shouldn't these be replaced by
>either explicit "noexcept" or at least by a library macro that expands
>to one or the other?

Yes, they should be _GLIBCXX_NOTHROW.

>(I'm sorry, if such unrelated questions are
>considered as inappropriate for this list).

Entirely appropriate, thanks!

Re: [PATCH] Fix ODR violations in code using <ext/atomicity.h>

[Jonathan Wakely]

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

On 05/07/19 19:44 +0100, Jonathan Wakely wrote:
>On 05/07/19 20:23 +0200, Daniel Krügler wrote:
>>Am Fr., 5. Juli 2019 um 18:13 Uhr schrieb Jonathan Wakely <jwakely@redhat.com>:
>>>
>>[..]
>>>I decided against the simplification in the second patch, and
>>>committed the attached one which is closer to the first patch I sent
>>>(preserving the __atomic_add and __exchange_and_add functions even
>>>when they just call the built-ins).
>>>
>>>Tested x86_64-linux, powerpc64-linux, powerpc-aix. Committed to trunk.
>>
>>Unrelated to the actual patch, I noticed some explicit "throw()" forms
>>used as exception specifications - shouldn't these be replaced by
>>either explicit "noexcept" or at least by a library macro that expands
>>to one or the other?
>
>Yes, they should be _GLIBCXX_NOTHROW.
>
>>(I'm sorry, if such unrelated questions are
>>considered as inappropriate for this list).
>
>Entirely appropriate, thanks!


Here's a patch to fix that and the dumb mistake I made in
__atomic_add_dispatch. I'll commit after testing finishes.





[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1285 bytes --]

commit 1542776ebab8848109d125d05a548fca5efb513a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat Jul 6 21:24:51 2019 +0100

    Fix recent regression in __atomic_add_dispatch
    
            * include/ext/atomicity.h (__exchange_and_add, __atomic_add): Replace
            throw() with _GLIBCXX_NOTHROW.
            (__atomic_add_dispatch): Return after performing atomic increment.

diff --git a/libstdc++-v3/include/ext/atomicity.h b/libstdc++-v3/include/ext/atomicity.h
index 73225b3de20..333c8843e14 100644
--- a/libstdc++-v3/include/ext/atomicity.h
+++ b/libstdc++-v3/include/ext/atomicity.h
@@ -55,10 +55,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); }
 #else
   _Atomic_word
-  __exchange_and_add(volatile _Atomic_word*, int) throw ();
+  __exchange_and_add(volatile _Atomic_word*, int) _GLIBCXX_NOTHROW;
 
   void
-  __atomic_add(volatile _Atomic_word*, int) throw ();
+  __atomic_add(volatile _Atomic_word*, int) _GLIBCXX_NOTHROW;
 #endif
 
   inline _Atomic_word
@@ -92,7 +92,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
 #ifdef __GTHREADS
     if (__gthread_active_p())
-      __atomic_add(__mem, __val);
+      {
+	__atomic_add(__mem, __val);
+	return;
+      }
 #endif
     __atomic_add_single(__mem, __val);
   }