gcc 3.2.3 config/os/bits/i386/atomicity.h broken

[Ralf Corsepius]

Am Fre, 2003-04-25 um 23.16 schrieb Loren James Rittle:
> In article <3EA97DB6.8D34E964@OARcorp.com>,
> Joel Sherrill<joel.sherrill@OARcorp.com> writes:
> 
> > I don't read this list very often.  Too much on my plate already.
> > Angelo has been fighting this for a few months and has been stuck
> > at gcc 3.2.  I just got frustrated enough to pester him into debugging
> > it to the offending instruction and then homed in on that.  
> 
> OK, well, the bug is fixed in gcc 3.3.
> 
> > That's good to hear.  I had trouble finding clear documentation.  I
> > really would like to be more confident that EVERY CPU model i486 and
> > above has xaddl support.
> 
> I read as much as I could back when Benjamin (rightly) directed the
> i386 port to generic pending additional work.
> 
> > First, I have no real easy way to ensure that the above
> > implementation works at all.  By that I would want to see one thread
> > spin until the other finished this operation. :)
> 
> Well, it took us many releases after gcc 3.0 to get all the MP bugs.
> 
> > But I have no opposition to having the lock;xaddl implementation
> > selected if the appropriate CPU model cpp predefine is defined.
> 
> We can't to this given the current architecture.
Why? I think we already do.

Given the gcc-3.2.2/3 implementation of atomicity.h works sufficiently,
all we need to do is to detect such cases which do not support the xadd
instruction. 

Then, AFAIS, #ifdef'ing on __tune_i386__ is sufficient. This is what I
try to exploit in my rtems-gcc-3.2.2-patch below.

> > RTEMS uses multilibs on the i386 target so each variant could get
> > teh best algorithm, not just an algorithm.
> 
> Perhaps, but libstdc++ doesn't multilib the installed headers.
Up to gcc-3.2.x, AFAICT, none of gcc's libs multilibs its headers.
Conversely, all of the gnu-toolchain's headers are assumed to be
multilib-safe. If not, such cases have been considered to be bugs.

> >> Joel, are you willing and able to post a complete updated version of
> >> config/cpu/i386/atomicity.h?  If the code was reviewed by enough
> >> people, I'd really want to see it get into 3.3.
> 
> > Sure.  Would you want it to be stright the generic version of ifdef'ed
> > so when compiled for the higher level CPU models, it uses the
> > lock;xaddl?
> 
> > Does this have a PR?  
> 
> No, that is the exact solution that doesn't work.  ;-)  There may be no
> #ifdef regions in the header except the entire multiple inclusion
> guard.  The reason is that this header is pulled into headers exposed
> to users for templating purposes.
I don't agree on this (cf. to my patch below). You just must not change
the ABI and API if ifdefs get encountered.

Ralf

[Please CC me, I am not subscribed to the list.]



-------------- next part --------------
--- ../gcc-3.2.2/libstdc++-v3/config/cpu/i386/bits/atomicity.h	2001-04-29 18:18:04.000000000 +0200
+++ gcc-3.2.2/libstdc++-v3/config/cpu/i386/bits/atomicity.h	2003-04-25 13:56:41.000000000 +0200
@@ -36,11 +36,21 @@
 __attribute__ ((__unused__))
 __exchange_and_add (volatile _Atomic_word *__mem, int __val)
 {
+#if defined(__tune_i386__)
+/*
+ * generic version
+ * xaddl is not available for i386s
+ */
+  _Atomic_word __result = *__mem;
+  *__mem += __val;
+  return __result;
+#else
   register _Atomic_word __result;
   __asm__ __volatile__ ("lock; xaddl %0,%2"
 			: "=r" (__result) 
                         : "0" (__val), "m" (*__mem) 
                         : "memory");
+#endif
   return __result;
 }
 
@@ -48,8 +58,16 @@
 __attribute__ ((__unused__))
 __atomic_add (volatile _Atomic_word* __mem, int __val)
 {
+#if defined(__tune_i386__)
+/*
+ * generic version
+ * xaddl is not available for i386s
+ */
+  *__mem += __val;
+#else
   __asm__ __volatile__ ("lock; addl %0,%1"
 			: : "ir" (__val), "m" (*__mem) : "memory");
+#endif
 }
 
 #endif /* atomicity.h */