This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: [PATCH] Add BeOS support for libstdc++-v3


Richard Henderson <rth@redhat.com> writes:

| > | + {
| > | +   unsigned long retval;
| > | +   long int readval;
| > | +   __asm__ ("lock");
| > | +   retval = *p;
| > | +
| 
| This is completely bogus.
| 
| One, you don't know for certain what instruction is going to
| follow the lock.  Two, the lock prefix is only applicable to
| specific instructions and movl isn't one of them.
| 
| > | +   __asm__ __volatile__ ("lock; xchgl %3, %1; sete %0"
| > | +                         : "=q" (ret), "=m" (*p), "=a" (readval)
| > | +                         : "r" (newval), "m" (*p));
| 
| Three, this does not even pretend to implement an atomic swap.
| Since xchgl does not set flags, this puts garbage into your
| return value.  In fact, since "ret" is not even defined, I can't
| imagine that this even compiles.

Aha, I trusted Daniel when he said things went well for him.  Thanks
for the spotting.

| Four, this is hardly BeOS specific.  If code like this is to
| be included at all (instead of using atomicity.h), then it 
| should be ifdef __i386__ and written like so:
| 
|   inline unsigned long _Atomic_swap(unsigned long *__p, unsigned long q)
|   {
|     __asm__ __volatile__ ("lock; xchgl %0,%1"
| 			  : "=r"(q), "=m"(*__p)
| 			  : "m"(*__p), "0"(q));
|     return q;
|   }

Daniel, could rework this part?

-- Gaby

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]