Re: [PATCH] x86: fix variable_test_bit() asm constraints

[Keir Fraser <keir.fraser@eu.citrix.com>]

On 14/3/08 11:23, "Jan Beulich" <jbeulich@novell.com> wrote:

> I just sent a (much bigger, see below) patch to the same effect to the
> x86 Linux maintainers - in Xen, all the operations modifying bits do
> have "memory" clobbers, so it's just the test_bit() constraint that's
> wrong.

Yes, I agree that fix is needed, although it would be consistent with other
asm statements in that file to use a memory clobbers instead.

> However, I wonder whether the non-atomic ops aren't limiting things too
> much by having "memory" clobbers, they would much better be restricted
> to indicate just the changing memory location. This, however, would
> probably require some additional consideration given that Xen (other
> than Linux) isn't using -fno-strict-aliasing. Furthermore, these
> non-atomic operations, according to their comments, can be re-ordered,
> which contradicts the use of __asm__ __volatile__ (but note that
> removing this would probably require extra precautions to meet strict
> aliasing rules).

Xen *does* use -fno-strict-aliasing. I'm afraid I don't understand why
memory-clobber would be needed for the atomic ops, but a dummy memory
operand would suffice for non-atomic ops. I used memory clobber everywhere
because at least I'm certain that works. Same for 'asm volatile'. We've had
various problems with the bitops before, and I just want the darn things to
work!

> Further, using 'void *' for the 'addr' parameter appears dangerous,
> since bt{,c,r,s} access the full 32 bits (if 'unsigned long' was used
> properly here, 64 bits for x86-64) pointed at, so invalid uses like
> referencing a 'char' array cannot currently be caught.

Sure, but those invalid uses do exist, in x86-specific Xen code we inherited
from Linux (perhaps older versions of Linux though). I don't want a huge
patch that casts a large number of callers!

> Finally I find the leading 'd' constraints in the 'nr' assembly
> operands quite odd - what is the purpose of that? Linux is using just
> "Ir" here...

Inherited from asm-x86_64/bitops.h. The 'd' can indeed go.

 -- Keir