[Patch] Memory fencing fixes

[Jeff Law]

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