Hang bug in 80-bit float square root implementation

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.

Here is my patch to the bug. It attempts to normalize the unnormal
floats before square rooting them. I don't know if it makes it produce
the correct results, but I also don't know if there's any good ground
truth for what the result should be, since my understanding is that
different x86 family processors handle unnormal floats differently.

I've never submitted a patch here before, so I hope this is the right
way to do it!

On 10 August 2016 at 04:35, Andrew Dutcher <andrewrdutcher@gmail.com> 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

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

Thanks,
- Andrew

On Fri, Aug 12, 2016 at 4:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 August 2016 at 04:35, Andrew Dutcher <andrewrdutcher@gmail.com> 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