Messages in this thread | | | Date | Wed, 13 Jan 2010 13:04:09 -0800 | From | "H. Peter Anvin" <> | Subject | Re: x86-32: clean up rwsem inline asm statements |
| |
On 01/13/2010 11:58 AM, George Spelvin wrote: >> As far as I can tell, very few of these assembly statements actually >> need a size at all -- the very first inc statement is purely to memory, >> and as such it needs a size marker, but almost any operation which is >> register-register or register-memory will simply take its size from the >> register operand. For those, it seems cleaner to simply drop the size >> suffix, and in fact this is the style we have been pushing people >> towards (use the suffix where mandatory or where the size is fixed >> anyway, to help catch bugs; use no suffix where the size can vary and is >> implied by the operands.) > > The one thing is that for a register-memory operation, using the > size of the memory operand can catch bugs where it doesn't match > the size of the register operand. > > GCC's inline asm doesn't make operand size very implicit, and it's > awkward to cast output operands, so there's a potential for bugs. > I especially get nervous when the operand itself is an immediate > constant, as I can't remember the ANSI rules for the type very well. > (Quick: is 0x80000000 an unsigned 32-bit int or a signed 64-bit one? > What about 2147483648 or 1<<31?) > > Since this is mostly inline functions, it's not so big a problem, but > I'd consider something like: > > static inline void __up_write(struct rw_semaphore *sem) > { > unsigned long tmp; > asm volatile("# beginning __up_write\n\t" > LOCK_PREFIX " xadd%z0 %1,(%2)\n\t" > /* tries to transition > 0xffff0001 -> 0x00000000 */ > " jz 1f\n" > " call call_rwsem_wake\n" > "1:\n\t" > "# ending __up_write\n" > : "+m" (sem->count), "=d" (tmp) > : "a" (sem), "1" (-RWSEM_ACTIVE_WRITE_BIAS) > : "memory", "cc"); > } > > Just in case the size of -RWSEM_ACTIVE_WRITE_BIAS doesn't match that > of sem->count. It'll explode when you try to run it, of course, but > there's something to be said for compile-time errors.
There are a number of things that can be done better... for one thing, "+m" (sem->count) and "a" (sem) is just bloody wrong. The right thing would be "a" (&sem->count) for proper robustness.
There is no real point in being concerned about the type of immediates, because the immediate type isn't really used... it shows up as a literal in the assembly language. However, if you're really concerned, the right thing to do is to do a cast in C, not playing games with the assembly.
-hpa
| |