Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 15:07, Markus Armbruster wrote:
> Paolo Bonzini <address@hidden> writes:
> 
>> On 04/11/2015 12:05, Richard Henderson wrote:
>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>>> ...
>>>>>
>>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>>> gods.
>>>>
>>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>>
>>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>>
>>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>>> and back to int64_t on the from_fixed assignment.  I must be
>>>> misunderstanding your suggestion.
>>>
>>>   int64_t scaled = (uint64_t)src << scale;
>>>
>>> I.e. one explicit conversion and one implicit conversion.
>>
>> That does the job, but it also does look like a typo...
> 
> Make the implicit conversion explicit then.

Sorry, but I'll say it again: there's _no way_ that a sane compiler will
_ever_ use this particular bit of undefined behavior.

I'm generally against uglifying the code to placate ubsan, but
especially so in this case: it is not common code and it would only
affect people running fpackfix under ubsan.

Paolo