Re: libstdc++/5037: Multithreaded read access to strings not threadsafe on Solaris/Sparc

[Alan Modra <amodra at bigpond dot net dot au>]

On Thu, Dec 06, 2001 at 06:33:32PM -0600, Loren James Rittle wrote:
> > I believe this file [libstdc++-v3/config/cpu/sparc/*/bits/atomicity.h]
> > is broken.  __exchange_and_add and __atomic_add use separate locks,
> > and so are not protected from each other.
> 
> [CC'ing libstdc++ list.]
> 
> Any best solution (in terms of contention) requires a locking word
> related to the first argument.  Would a designer of the atomicity.h
> abstraction layer prefer to provide such a solution?

I provided a solution for hppa a long time ago, and raised the sparc
issue, but Ben didn't like my hacks.  Frankly, I didn't particularly
like them either.  :)

For reference, here is a considerably cleaned up version of my hppa
patch, which should be easy to adapt for sparc.  hppa is particularly
nasty because the lock needs to be initialised non-zero, and locks only
work when aligned to 16 byte boundaries.

	* configure.target (cpu_include_dir): Set for hppa.
	* config/cpu/hppa/bits/atomicity.h: New file.
	* include/bits/basic_string.h (_S_empty_rep_storage): 16 byte align.
	* include/bits/basic_string.tcc (_S_empty_rep_storage): Here too.

Index: libstdc++-v3/configure.target
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/configure.target,v
retrieving revision 1.33
diff -u -p -r1.33 configure.target
--- configure.target	2001/11/23 16:29:00	1.33
+++ configure.target	2001/12/07 01:24:14
@@ -31,6 +31,9 @@ case "${target_cpu}" in
   cris)
     cpu_include_dir="config/cpu/cris"
     ;;
+  hppa*)
+    cpu_include_dir="config/cpu/hppa"
+    ;;
   ia64)
     cpu_include_dir="config/cpu/ia64"
     ;;
Index: libstdc++-v3/include/bits/basic_string.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/include/bits/basic_string.h,v
retrieving revision 1.12
diff -u -p -r1.12 basic_string.h
--- basic_string.h	2001/11/28 18:58:19	1.12
+++ basic_string.h	2001/12/07 01:24:15
@@ -219,7 +219,10 @@ namespace std
 
       // The following storage is init'd to 0 by the linker, resulting
       // (carefully) in an empty string with one reference.
-      static size_type _S_empty_rep_storage[(sizeof(_Rep) + sizeof(_CharT) + sizeof(size_type) - 1)/sizeof(size_type)];
+      static size_type _S_empty_rep_storage[
+	(sizeof(_Rep) + sizeof(_CharT)
+	 + sizeof(size_type) - 1)/sizeof(size_type)]
+      __attribute__ ((__aligned__ (16)));
 
       _CharT* 
       _M_data() const 
Index: libstdc++-v3/include/bits/basic_string.tcc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/include/bits/basic_string.tcc,v
retrieving revision 1.11
diff -u -p -r1.11 basic_string.tcc
--- basic_string.tcc	2001/11/28 18:58:19	1.11
+++ basic_string.tcc	2001/12/07 01:24:16
@@ -61,7 +61,12 @@ namespace std
   template<typename _CharT, typename _Traits, typename _Alloc>
     typename basic_string<_CharT, _Traits, _Alloc>::size_type
     basic_string<_CharT, _Traits, _Alloc>::_S_empty_rep_storage[
-    (sizeof(_Rep) + sizeof(_CharT) + sizeof(size_type) - 1)/sizeof(size_type)];
+    (sizeof(_Rep) + sizeof(_CharT) + sizeof(size_type) - 1)/sizeof(size_type)]
+    __attribute__ ((__aligned__ (16)))
+#ifdef _S_EMPTY_REP_INIT
+    _S_EMPTY_REP_INIT
+#endif
+  ;
 
   // NB: This is the special case for Input Iterators, used in
   // istreambuf_iterators, etc.
Index: libstdc++-v3/config/cpu/hppa/bits/atomicity.h
===================================================================
RCS file: atomicity.h
diff -N atomicity.h
--- /dev/null	Tue May  5 13:32:27 1998
+++ atomicity.h	Thu Dec  6 17:25:12 2001
@@ -0,0 +1,127 @@
+/* Low-level functions for atomic operations.  PA-RISC version. -*- C++ -*-
+   Copyright 2001 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Library General Public License as
+   published by the Free Software Foundation; either version 2 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Library General Public License for more details.
+
+   You should have received a copy of the GNU Library General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If not,
+   write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#ifndef _BITS_ATOMICITY_H
+#define _BITS_ATOMICITY_H	1
+
+/* Define this to 1 when everyone is using a sensible glibc.  */
+#define MALLOC_ALIGNS_TO_16BYTE 1
+
+/* Load and clear, the only PA-RISC atomic read-write operation.  Some
+   cpus only support ldcw on 16 byte aligned words.  *sigh*.  */
+static inline int
+__pa_ldcw (volatile int* lock)
+{
+  int ret;
+  __asm__ __volatile__ ("ldcw 0(%1),%0" : "=r" (ret) : "r" (lock));
+  return ret;
+}
+
+static inline int
+__pa_spin_lock (volatile int* lock)
+{
+  int ret;
+  while ((ret = __pa_ldcw (lock)) == 0)
+    /* If we don't grab the lock, don't clobber the bus with ldcw.  */
+    while (*lock == 0) /* spin */ ;
+  return ret;
+}
+
+
+class _Atomic_counter {
+ public:
+  int lock;
+  int count;
+#if ! MALLOC_ALIGNS_TO_16BYTE
+  /* Two locks???  Well, malloc allocates to 8 byte boundaries, and
+     thus either lock or alt_lock will be 16 byte aligned.  Use
+     whichever one gets the right alignment.  */
+  int alt_lock;
+#endif
+
+  inline
+  _Atomic_counter()
+  {
+    lock = 1;
+#if ! MALLOC_ALIGNS_TO_16BYTE
+    alt_lock = 1;
+#endif
+  }
+
+  inline int
+  operator=(const int __val) { return this->count = __val; }
+
+  inline bool
+  operator<(const int __rhs) const
+    { return this->count < __rhs; }
+
+  inline bool
+  operator>(const int __rhs) const
+    { return this->count > __rhs; }
+};
+
+typedef class _Atomic_counter _Atomic_word;
+
+/* This is horrible, but how else can we get the static initializer
+   correct with our lock(s) set non-zero? */
+#if MALLOC_ALIGNS_TO_16BYTE
+#define _S_EMPTY_REP_INIT = { 0, 0, 1, 0, 0 }
+#else
+#define _S_EMPTY_REP_INIT = { 0, 0, 1, 0, 1, 0 }
+#endif
+
+
+static inline int
+__attribute__ ((__unused__))
+__exchange_and_add (_Atomic_word* mem, int val)
+{
+  int result;
+  int tmp;
+#if MALLOC_ALIGNS_TO_16BYTE
+  int *lock = &mem->lock;
+#else
+  int *lock = (int *) ((long) &mem->alt_lock & ~15);
+#endif
+
+  tmp = __pa_spin_lock (lock);
+  result = mem->count;
+  mem->count += val;
+  __asm__ __volatile__("");
+  *lock = tmp;
+  return result;
+}
+
+static inline void
+__attribute__ ((__unused__))
+__atomic_add (_Atomic_word* mem, int val)
+{
+  int tmp;
+#if MALLOC_ALIGNS_TO_16BYTE
+  int *lock = &mem->lock;
+#else
+  int *lock = (int *) ((long) &mem->alt_lock & ~15);
+#endif
+
+  tmp = __pa_spin_lock (lock);
+  mem->count += val;
+  __asm__ __volatile__("");
+  *lock = tmp;
+}
+
+#endif