This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] Memory fencing fixes


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


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