Re: [Patch] Memory fencing fixes

[Jeff Law <law at redhat dot com>]

On 02/20/2013 01:59 PM, Joseph S. Myers wrote:
> On Mon, 4 Feb 2013, Jeff Law wrote:
>
> > diff --git a/ports/sysdeps/arm/bits/atomic.h b/ports/sysdeps/arm/bits/atomic.h
> > index 6e20df7..e2660b1 100644
> > --- a/ports/sysdeps/arm/bits/atomic.h
> > +++ b/ports/sysdeps/arm/bits/atomic.h
> > @@ -42,6 +42,7 @@ void __arm_link_error (void);
> >   # define atomic_full_barrier() __sync_synchronize ()
> >   #else
> >   # define atomic_full_barrier() __arm_assisted_full_barrier ()
> > +# define atomic_asm_full_barrier() __arm_asm_assisted_full_barrier ()
> >   #endif
>
> This is only defining atomic_asm_full_barrier in one case of this
> conditional - don't you need a definition in the
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 case as well?  It looks like you're
> using this macro unconditionally.
I thought I took this from Carlos's or your work.  What's the right code 
to use when GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is defined?  just 
__sync_synchronize?


> >   #ifdef __thumb2__
> >   # define __arm_assisted_full_barrier() \
> >        __asm__ __volatile__						      \
> > @@ -25,6 +26,11 @@
> >   	      "movt\tip, #0xffff\n\t"					      \
> >   	      "blx\tip"							      \
> >   	      : : : "ip", "lr", "cc", "memory");
> > +/* The asm variant is used as an insert into existing asm statements.  */
> > +# define __arm_asm_assisted_full_barrier() \
> > +       "       movw    ip, #0x0fa0\n" \
> > +       "       movt    ip, #0xffff\n" \
> > +       "       blx     ip"
>
> I think this should be using \t in the same cases as the existing asm
> rather than sequences of spaces,
I'll fix that.



 and instead of duplicating things you
> should define __arm_asm_assisted_full_barrier first, then use it inside
> __arm_assisted_full_barrier.  Likewise for the ARM-mode definition.
OK.


> >   "	beq	4f\n"
> > -"5:	mov	r0, r6\n"
> > +	atomic_asm_full_barrier()
> > +"5:	ldr     r4, 6f\n"
>
> The existing code is using tabs both to indent instructions and for the
> whitespace between instruction names and operands.  This patch appears to
> be introducing an inconsistency, spaces rather than tabs between
> instruction names and operands.
Will fix.


Thanks,
Jeff