Re: [PATCH] x86: fix wait code asm() constraints

[Keir Fraser <keir.xen@gmail.com>]

On 03/08/2012 11:34, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 03.08.12 at 12:04, Keir Fraser <keir@xen.org> wrote:
>> On 03/08/2012 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> In __prepare_to_wait(), properly mark early clobbered registers. By
>>> doing so, we at once eliminate the need to save/restore rCX and rDI.
>>> 
>>> In check_wakeup_from_wait(), make the current constraints match by
>>> removing the code that actuall alters registers. By adjusting the
>>> resume address in __prepare_to_wait(), we can simply re-use the copying
>>> operation there (rather than doing a second pointless copy in the
>>> opposite direction after branching to the resume point), which at once
>>> eliminates the need for re-loading rCX and rDI inside the asm().
>> 
>> First of all, this is code improvement, rather than a bug fix, right? The
>> asm constraints are correct for the code as it is, I believe.
> 
> No, the constraints aren't really correct at present (yet this is
> not visible as a functional bug in any way) - from a formal
> perspective, the early clobber specification is needed on _any_
> operand that doesn't retain its value throughout an asm(). Any
> future compiler could derive something from this that we don't
> intend.

I'm confused. The registers have the same values at the start and the end of
the asm statement. How can it possibly matter, even in theory, whether they
temporarily change in the middle? Is this fairly strong assumption written
down in the gcc documentation anywhere?

>> It also seems the patch splits into two independent parts:
>> 
>>  A. Not sure whether the trade-off of the rCX/rDI save/restore versus more
>> complex asm constraints makes sense.
>> 
>>  B. Separately, the adjustment of the restore return address, and avoiding
>> needing to reload rCX/rDI after label 1, as well as avoiding the copy in
>> check_wakeup_from_wait(), is very nice.
>> 
>> I'm inclined to take the second part only, and make it clearer in the
>> changeset comment that it is not a bug fix.
>> 
>> What do you think?
> 
> The patch could be split, yes, but where exactly the split(s)
> should be isn't that obvious to me. And as it's fixing the same
> kind of issue on both asm()-s, it seemed sensible to keep the
> changes together.

Yes, that confused me too -- the output constraints on the second asm can
hardly be wrong, or at least matter, since it never returns! Execution state
is completely reloaded within the asm statement.

 -- Keir

> Jan
>