Page MenuHomePhabricator

APFloat/x87: Fix string conversion for "unnormal" values (pr35860)
ClosedPublic

Authored by labath on Jan 9 2018, 8:45 AM.

Details

Summary

Unnormal values are a feature of some very old x87 processors. We handle
them correctly for the most part -- the only exception was an unnormal
value whose significand happened to be zero. In this case the APFloat
was still initialized as normal number (category = fcNormal), but a
subsequent toString operation would assert because the math would
produce nonsensical values for the zero significand.

This commit fixes this by initializing the APFloat to fcZero for these
numbers.

The issue was discovered because LLDB would crash when trying to print
some "long double" values.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jan 9 2018, 8:45 AM

My recollection is that on 386 and later unnormals with a zero significand are treated as an invalid operand; should these print as 0 or as NaN?

My recollection is that on 386 and later unnormals with a zero significand are treated as an invalid operand; should these print as 0 or as NaN?

I was going off of the comment before initFromF80LongDoubleAPInt, which states that APFloat treats unnormals as regular values (i.e., like a pre-386 processor would). As far as I can tell this statement is true: the non-zero unnormal values will print out correctly (I haven't tried doing arithmetic on them).

So, this only makes sure that we treat "unnormal zero" the same way as other unnormal numbers. However, I can certainly see a case for printing them out differently. This would even be preferable for the debugger use case. OTOH, that will likely be a much more intrusive change than this.

(After checking the architecture manual)

On 387 and later, all unnormals is treated as an "invalid operand", which means they behave the same as a signaling NaN; I think we should probably print them as such. This is a somewhat more invasive change, as you say, but it's more correct behavior.

labath updated this revision to Diff 131454.Jan 25 2018, 7:52 AM

Treat unnormal values as NaN.

It doesn't seem there is anything in the llvm or clang test suite that would
depend on a particular treatment of unnormal values.

Ping @scanon. Is this the kind of change you had in mind?

labath added a comment.May 8 2018, 8:31 AM

@scanon: Could you take another look at this please? This issue is currently causing crashes in LLDB. I could work around this from there, but to me this looks like it should be handled inside APFloat (I haven't checked, but I wouldn't be surprised if it was possible to trigger this from LLVM as well).

If you don't feel comfortable reviewing this, could you suggest another reviewer for this patch?

Thank you.

scanon accepted this revision.May 8 2018, 8:34 AM

LGTM

This revision is now accepted and ready to land.May 8 2018, 8:34 AM
labath added a comment.May 9 2018, 8:14 AM

Thank you for the review.

This revision was automatically updated to reflect the committed changes.