Messages in this thread Patch in this message | | | Date | Sat, 14 Oct 2006 21:50:05 +0200 | From | Willy Tarreau <> | Subject | Re: 2.4.x: i386/x86_64 bitops clobberings |
| |
Hi Jan,
On Mon, Oct 09, 2006 at 08:27:54AM +0200, Jan Kiszka wrote: > Willy Tarreau wrote: > > I've done it too, but unfortunately, I discovered that it does not build > > with gcc-2.95 anymore, while 3.3 is fine : > > > > cyclades.c: In function `cyy_interrupt': > > /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm' > > cyclades.c: In function `cyz_handle_rx': > > /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm' > > > > 127 __asm__ __volatile__( LOCK_PREFIX > > 128 "btsl %2,%1\n\tsbbl %0,%0" > > 129 :"=r" (oldbit),"+m" (ADDR) > > 130 :"Ir" (nr) : "memory"); > > > > > > I don't know what the right solution should be. I'm not fond of #ifdefs, > > and I'm embarrassed to know that gcc can do all sorts of nasty things due > > to a wrong clobbering :-/ > > > > If you have any idea on the subject, I'm willing to try. > > What about > > #if __GNUC__ < 3 > #define ADDR_DEPS "=m" > #else > #define ADDR_DEPS "+m" > #endif > > __asm__ __volatile__( LOCK_PREFIX > "btsl %2,%1\n\tsbbl %0,%0" > :"=r" (oldbit),ADDR_DEPS (ADDR) > :"Ir" (nr) : "memory"); > > or something similar? Would keep the number of #ifs low.
I don't like it for two reasons :
- I don't know how gcc-3.0 to 3.2 behave on such code, so may be this would implicitly categorize them in the wrong group
- it makes the asm code very hard to read. Operand dependencies are very sensible tricks which need to be extremely clear.
I have searched the archives for previous occurrences of this recurring problem, and found another alternative which is said to work on all gcc versions. So here's the patch for both x86 and x86_64. I checked that it produces identical code as I obtain with the patch from 2.6. It also fixes your testcase for gcc-2.95 to 4.1.1.
Could you please give it a try ?
Andi, Linus, I'd be very interested in your comments on this patch, since you're more used to deal with this problem than me.
Thanks in advance, Willy
From 695f7e0b3806ff44def385856cda60d7f1c2df1c Mon Sep 17 00:00:00 2001 From: Willy Tarreau <w@1wt.eu> Date: Sat, 14 Oct 2006 21:39:06 +0200 Subject: [PATCH] 2.4.x: i386/x86_64 bitops clobberings
Problem reported by Jan Kiszka <jan.kiszka@web.de> : "After going through debugging hell with some out-of-tree code, I realised that this patch :
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b
makes a difference: current 2.6 works with the following code sequence as expected (printk is executed), 2.4 fails. I used gcc 3.3.6-13 (Debian).
unsigned long a = 1;
int module_init(void) { unsigned long b = 0; int x;
x = __test_and_set_bit(0, &b); if (__test_and_set_bit(0, &a)) printk("x = %d\n", x);
return -1; }
I ported the x86 part back to 2.4.33 and my problem disappeared. "
The problem with this patch is that the use of "+m" in asm statements triggers bugs in gcc 2.95 which is supported by kernel 2.4. For instance, cyclades.c does not build anymore :
$ gcc -O2 -pipe -c cyclades-c.c cyclades.c: In function `cyy_interrupt': /usr/src/git/linux-2.4/include/asm/bitops.h:134: inconsistent operand constraints in an `asm' cyclades.c: In function `cyz_handle_rx': /usr/src/git/linux-2.4/include/asm/bitops.h:134: inconsistent operand constraints in an `asm'
As explained by Richard Henderson here, the equivalent construct "=m"(x) : "m"(x) works on any version of gcc :
http://marc.theaimsgroup.com/?l=3Dlinux-kernel&m=3D107475162200773&w=3D2
I successfully verified on the example code above and on cyclades.c that the exact same code is produced on x86 with this construct with gcc 2.95.3, 3.3.6, 3.4.4, and 4.1.1, so it looks fine.
Just like in the former patch for 2.6, this one was applied to both i386 and x86_64.
Signed-Off-By: Willy Tarreau <w@1wt.eu> --- include/asm-i386/bitops.h | 22 +++++++++++----------- include/asm-x86_64/bitops.h | 22 +++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/include/asm-i386/bitops.h b/include/asm-i386/bitops.h index 9b6b55e..bb63c10 100644 --- a/include/asm-i386/bitops.h +++ b/include/asm-i386/bitops.h @@ -38,7 +38,7 @@ static __inline__ void set_bit(int nr, v __asm__ __volatile__( LOCK_PREFIX "btsl %1,%0" :"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); } /** @@ -55,7 +55,7 @@ static __inline__ void __set_bit(int nr, __asm__( "btsl %1,%0" :"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); } /** @@ -73,7 +73,7 @@ static __inline__ void clear_bit(int nr, __asm__ __volatile__( LOCK_PREFIX "btrl %1,%0" :"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); } #define smp_mb__before_clear_bit() barrier() #define smp_mb__after_clear_bit() barrier() @@ -92,7 +92,7 @@ static __inline__ void __change_bit(int __asm__ __volatile__( "btcl %1,%0" :"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); } /** @@ -109,7 +109,7 @@ static __inline__ void change_bit(int nr __asm__ __volatile__( LOCK_PREFIX "btcl %1,%0" :"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); } /** @@ -127,7 +127,7 @@ static __inline__ int test_and_set_bit(i __asm__ __volatile__( LOCK_PREFIX "btsl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr) : "memory"); + :"Ir" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -147,7 +147,7 @@ static __inline__ int __test_and_set_bit __asm__( "btsl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); return oldbit; } @@ -166,7 +166,7 @@ static __inline__ int test_and_clear_bit __asm__ __volatile__( LOCK_PREFIX "btrl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr) : "memory"); + :"Ir" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -186,7 +186,7 @@ static __inline__ int __test_and_clear_b __asm__( "btrl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr)); + :"Ir" (nr), "m" (ADDR)); return oldbit; } @@ -198,7 +198,7 @@ static __inline__ int __test_and_change_ __asm__ __volatile__( "btcl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr) : "memory"); + :"Ir" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -217,7 +217,7 @@ static __inline__ int test_and_change_bi __asm__ __volatile__( LOCK_PREFIX "btcl %2,%1\n\tsbbl %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"Ir" (nr) : "memory"); + :"Ir" (nr), "m" (ADDR) : "memory"); return oldbit; } diff --git a/include/asm-x86_64/bitops.h b/include/asm-x86_64/bitops.h index c8a4749..f6a2a4f 100644 --- a/include/asm-x86_64/bitops.h +++ b/include/asm-x86_64/bitops.h @@ -38,7 +38,7 @@ static __inline__ void set_bit(long nr, __asm__ __volatile__( LOCK_PREFIX "btsq %1,%0" :"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); } /** @@ -55,7 +55,7 @@ static __inline__ void __set_bit(long nr __asm__( "btsq %1,%0" :"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); } /** @@ -73,7 +73,7 @@ static __inline__ void clear_bit(long nr __asm__ __volatile__( LOCK_PREFIX "btrq %1,%0" :"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); } #define smp_mb__before_clear_bit() barrier() #define smp_mb__after_clear_bit() barrier() @@ -92,7 +92,7 @@ static __inline__ void __change_bit(long __asm__ __volatile__( "btcq %1,%0" :"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); } /** @@ -109,7 +109,7 @@ static __inline__ void change_bit(long n __asm__ __volatile__( LOCK_PREFIX "btcq %1,%0" :"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); } /** @@ -127,7 +127,7 @@ static __inline__ int test_and_set_bit(l __asm__ __volatile__( LOCK_PREFIX "btsq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr) : "memory"); + :"dIr" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -147,7 +147,7 @@ static __inline__ int __test_and_set_bit __asm__( "btsq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); return oldbit; } @@ -166,7 +166,7 @@ static __inline__ int test_and_clear_bit __asm__ __volatile__( LOCK_PREFIX "btrq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr) : "memory"); + :"dIr" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -186,7 +186,7 @@ static __inline__ int __test_and_clear_b __asm__( "btrq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr)); + :"dIr" (nr), "m" (ADDR)); return oldbit; } @@ -198,7 +198,7 @@ static __inline__ int __test_and_change_ __asm__ __volatile__( "btcq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr) : "memory"); + :"dIr" (nr), "m" (ADDR) : "memory"); return oldbit; } @@ -217,7 +217,7 @@ static __inline__ int test_and_change_bi __asm__ __volatile__( LOCK_PREFIX "btcq %2,%1\n\tsbbq %0,%0" :"=r" (oldbit),"=m" (ADDR) - :"dIr" (nr) : "memory"); + :"dIr" (nr), "m" (ADDR) : "memory"); return oldbit; } -- 1.4.1 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |