Re: [Patch] Memory fencing fixes

["Joseph S. Myers" <joseph at codesourcery dot com>]

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.

>  #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, 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.

>  "	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.

-- 
Joseph S. Myers
joseph@codesourcery.com