locking problem with mips atomicity

[Michael Eager]

The MIPS architecture spec says that there should not be a load,
store or prefetch between issuing a LL instruction and a following SC
instruction.  (MIPS32 Arch, Vol 2, rev 0.95, SC instruction description, p 186)

The macros in libstdc++-v3/config/cpu/mips/atomicity.h (and similar code
in glibc) could generate a load between the LL and SC if they were inlined.
The comments in the macros still mentione inline, but "inline" was removed 
in the most recent mod to this header (2/27/04).  The functions generated 
without inline do not appear to have this problem, but previous versions of 
gcc inlined the code do.  (Offhand, I think removing inline for locking 
code will have an adverse affect on performance.)

This program (adapted from mcount.c in glibc) shows the problem:

mcount.c:
typedef int _Atomic_word;

#include "atomicity.h"
using namespace __gnu_cxx;

struct gmonparam {
        _Atomic_word state;
};
extern struct gmonparam _gmonparam ;

static void __attribute__ ((__used__)) __mcount ()
{
        register struct gmonparam *p;
        p = &_gmonparam;
        if (! __exchange_and_add(&p->state, 1))
          return;
        p->state = 0;
}

The code generated for exchange_and_add is (lwc0 == LL, swc0 == SC):

  10:   8f830000        lw      v1,0(gp)
  14:   00000000        nop
  18:   c0630000        lwc0    $3,0(v1)
  1c:   00432021        addu    a0,v0,v1
  20:   8f810000        lw      at,0(gp)    <<<<<< BAD
  24:   00000000        nop
  28:   e0240000        swc0    $4,0(at)
  2c:   1080fff8        beqz    a0,10 <_Z8__mcountv+0x10>
  30:   00000000        nop
  34:   10600004        beqz    v1,48 <_Z8__mcountv+0x48>

The attached patch loads the address of the location being
modified into a reg before the LL, and references it explicitly,
so that the following is generated instead:

  10:   8f840000        lw      a0,0(gp)
  14:   c0830000        lwc0    $3,0(a0)
  18:   00432821        addu    a1,v0,v1
  1c:   e0850000        swc0    $5,0(a0)
  20:   10a0fffb        beqz    a1,10 <_Z8__mcountv+0x10>
  24:   00000000        nop
  28:   10600004        beqz    v1,3c <_Z8__mcountv+0x3c>


--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085
-------------- next part --------------
Index: libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ libstdc++-v3/config/cpu/mips/atomicity.h	15 Mar 2004 23:55:31 -0000
@@ -35,7 +35,7 @@
   __attribute__ ((__unused__))
   __exchange_and_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result, __tmp;
+    _Atomic_word __result, __tmp, temp;
     
     __asm__ __volatile__
       ("/* Inline exchange & add */\n\t"
@@ -44,13 +44,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "la      %3,%2\n\t"
+       "ll	%0,0(%3)\n\t"
+       "addu	%1,%5,%0\n\t"
+       "sc	%1,0(%3)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
+       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem), "=r" (temp)
        : "m" (*__mem), "r"(__val));
     
     return __result;
@@ -60,7 +61,7 @@
   __attribute__ ((__unused__))
   __atomic_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result;
+    _Atomic_word __result, temp;
     
     __asm__ __volatile__
       ("/* Inline atomic add */\n\t"
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "la      %2,%1\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%0,%4,%0\n\t"
+       "sc	%0,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
+       : "=&r"(__result), "=m"(*__mem), "=r" (temp)
      : "m" (*__mem), "r"(__val));
   }
 } // namespace __gnu_cxx