Re: [Qemu-devel] [PATCH 4/8] target-i386: Re-introduce optimal breakpoint removal

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/09/2015 16:57, Richard Henderson wrote:
>>> >> +        /* Fold the global and local enable bits together into the
>>> >> +           global fields, then xor to show which registers have
>>> >> +           changed collective enable state.  */
>>> >> +        int mod = ((old_dr7 | old_dr7 * 2) ^ (new_dr7 | new_dr7 * 2)) & 0xff;
>> > 
>> > The AND is not needed at all but, if you add it, you might as well use
>> > "& 0xaa" which is clearer.  But even better, just do:
>> > 
>> >    target_ulong old_dr7 = env->dr[7];
>> >    int mod = old_dr7 ^ new_dr7;
>> >    ...
>> >    if ((mod & ~0xff) == 0) {
>> > 
>> > 
>> > and test with
>> > 
>> > 	if (mod & (3 << i * 2))
>> > 
>> > inside the loop.
> Nope.  I wrote that the first time myself.  We're interested in two different
> things: (1) whether or not something changed outside enable bits, and (2)
> whether the enable state changed.
> 
> Since (2) is a combination of both global and local enable bits, we must
> combine them *and then xor* to see if the enable state actually changes.  Just
> using (mod & (3 << n)) will report "change" when local enable turns off, but
> global enable remains on.  Which is not what we want.

Ah, I see now.  Perhaps

	int old_enable = (old_dr7 | (old_dr7 << 1)) & 0xaa;
	int new_enable = (new_dr7 | (new_dr7 << 1)) & 0xaa;

?

> Perhaps that comment could stand to be expanded...

I think at least for me it's just the expression that is too complex.

Paolo