Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Peter Maydell]

On 19 March 2014 06:21, Stefan Weil <address@hidden> wrote:
> Am 18.03.2014 22:30, schrieb Richard Henderson:
>> TCG now requires unspecified behavior rather than a potential crash,
>> bring the C shift within the letter of the law.
>
> I know that C does not define the result of some shift / rotate
> operations, but I don't understand the sentence above. Why does TCG or
> TCI require unspecified behaviour now? Where was or is a potential crash?

The specific example we hit is that for variable shifts by a value
which happens to be zero, the x86 frontend will emit a shift
by -1. (It doesn't subsequently use that result because there's
a later movcond in the TCG op sequence.)

So we had a choice of:
 (a) make the x86 frontend slower and more awkward (and perhaps
      also other frontends which did the same thing, if any)
 (b) tighten the definition of the TCG shift ops from "undefined
    behaviour, may crash" to "unspecified behaviour, must produce
    a result but it could be anything"

and since the latter isn't a problem for the backends where
performance is important (ie the native ones), because
the hardware always acts like 'unknown result' rather than
'might blow up' (b) is a clearly better choice.

> The modifications below won't harm, but make the TCG interpreter slower.
> Are they (all) necessary? Are there test cases which fail with the old code?

If you want to try to find them you could run with clang
-fsanitize=undefined. (Try 'make check', the code we run
during the ACPI test does this.)

>>
>> Signed-off-by: Richard Henderson <address@hidden>
>> ---
>>  tci.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tci.c b/tci.c
>> index 0202ed9..6523ab8 100644
>> --- a/tci.c
>> +++ b/tci.c
>> @@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t 
>> *tb_ptr)
>>              t0 = *tb_ptr++;
>>              t1 = tci_read_ri32(&tb_ptr);
>>              t2 = tci_read_ri32(&tb_ptr);
>> -            tci_write_reg32(t0, t1 << t2);
>> +            tci_write_reg32(t0, t1 << (t2 & 31));
>>              break;
>>          case INDEX_op_shr_i32:
>>              t0 = *tb_ptr++;
>>              t1 = tci_read_ri32(&tb_ptr);
>>              t2 = tci_read_ri32(&tb_ptr);
>> -            tci_write_reg32(t0, t1 >> t2);
>> +            tci_write_reg32(t0, t1 >> (t2 & 31));
>
> Right shifts of unsigned values with unsigned shift count are always
> defined, aren't they?

No. The C standard says "If the value of the right operand is
negative or is greater than or equal to the width of the promoted
left operand, the behavior is undefined." for all shifts.

thanks
-- PMM