Re: [Qemu-devel] Hang bug in 80-bit float square root implementation

[Andrew Dutcher]

Alright! I'll test that new patch and submit it.

Thanks,
- Andrew

On Fri, Aug 12, 2016 at 4:01 AM, Peter Maydell <address@hidden> wrote:
> On 10 August 2016 at 04:35, Andrew Dutcher <address@hidden> wrote:
>> Hello!
>>
>> I ran into an issue where qemu (specifically, the unicorn engine)
>> would hang forever in the middle of the emulated square root
>> instruction under certain circumstances. I eventually tracked the
>> issue down to the square root of an "unnormal" long double, one
>> without the integer part bit set.
>>
>> Test case:
>>
>> cat > hang.s <<EOF
>> .intel_syntax noprefix
>>
>> .global _start
>> _start:
>>   xor eax,eax
>>   inc eax
>>   push eax
>>   push eax
>>   push eax
>>   fld tbyte ptr [esp]
>>   fsqrt
>>   int 0x80
>> EOF
>> as --32 hang.s -o hang.o
>> ld -melf_i386 hang.o -o hang
>> qemu-i386 ./hang
>>
>> qemu will hang! Assuming it jits the fsqrt as the soft float
>> implementation and doesn't just use a native fsqrt instruction. I have
>> no idea if qemu can do this, but it doesn't hurt to cover that base.
>
> Hi; thanks for this patch and test case. You're right that QEMU
> definitely shouldn't hang here, but I don't think this patch
> is the correct fix for it. As you say, there's been variation
> in how unnormals (80-bit-precision values with non-zero exponent
> field and the 'integer' bit zero) should be handled, but the
> Intel 64 and IA-32 architectures software developer's manual says
> that "The Intel 387 match coprocessor and later IA-32 processors
> generate an invalid-operation exception when [pseudo-NaNs,
> pseudo-infinities and un-normal numbers] are encountered as operands"
> (page 8-20 in the version of the doc I have). I checked on my
> x86 box (a Core i7-3770) and it does indeed do this. Since QEMU
> doesn't try to emulate the 287 we can stick to emulating just
> the "invalid-operation" behaviour and don't need to also try
> to emulate what the 287 did.
>
> I think that would look something like:
>
>     if (floatx80_invalid_encoding(x)) {
>         float_raise(float_flag_invalid, status);
>         return floatx80_default_nan(status);
>     }
>
> at the start of the relevant floatx80 functions (which is definitely
> more than just the sqrt function; would need to check the Intel
> docs for exactly which ones). floatx80_invalid_encoding()
> would be a new function that identifies the pseudo-NaN,
> pseudo-infinity and un-normal formats.
>
> (The other class of odd encoding is pseudo-denormal, but those
> are different because they are still supposed to be handled as
> inputs, they just aren't generated as outputs.)
>
>> I've never submitted a patch here before, so I hope this is the right
>> way to do it!
>
> It's pretty close. We have some documentation on patch formats
> and so on here: http://wiki.qemu.org/Contribute/SubmitAPatch
>
> The most important thing that you need to do is remember to
> add a "Signed-off-by:" line; we can fix up most other minor
> problems, but without the signed-off-by we can't accept the
> patch at all.
>
> thanks
> -- PMM