Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix get10Byte on platforms where long double is >80 bits. #1195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

katrinafyi
Copy link

For platforms such as aarch64 with a non 80-bit "long double" type, the tests fail since they assume an 80-bit long double type.

retdec-tests-utils
/build/source/tests/utils/byte_value_storage_tests.cpp:357: Failure
Expected equality of these values:
  0.31348419189453125
  val
    Which is: 0

Specifically, the tests construct a long double with the byte pattern: 00000000000081a0fd3f000000000000. When interpreted as:

  • a float 80, this is 0.31348419189453125 as expected,
  • a float 128, this is 1.95671814821071780e-4942 which fails the test and displays as zero.

We fix this by restricting systemHasLongDouble to return true only when the platform has 80-bit long double (detected by 64-bit mantissa). Then, we change the fallback get10ByteImpl to construct a correct long double by using the language's builtin float handling, instead of naively copying

This handles both where long double is smaller and greater than 80 bits, though the trip through double does limit maximum precision. Given the fallback already assumes 64-bits is acceptable, I don't think this is a big problem.

If more precision is desired, it is not too hard to write a float10 to float16 method. I found the code in the linked perl blog didn't seem to be correct, but it is possible with not too much difficulty.

for platforms with a non 80-bit "long double" type, we
change get10ByteImpl to reduce to double then use the
language's conversion to long double.

this handles both where long double is smaller and greater
than 80 bits, though only 64 bits of precision are preserved.

this fixes the tests on aarch64 platforms.
Copy link
Collaborator

@PeterMatula PeterMatula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine I guess 👍 .
But for whatever reason, CI tests refuse to run for it, and I don't even see an option to trigger them manually. I will have to look at it, and get it to work, so that we see if it works on Windows, etc. (I was a bit worried about LDBL_MANT_DIG, but it looks like it is in standard).

Looking at this whole thing again, I'm not even sure there should be systemHasLongDouble(). It would be best to avoid using it altogether, but that would require deeper investigation into what is actually happening and why.

@katrinafyi
Copy link
Author

katrinafyi commented Apr 29, 2024

Thanks for the review! These changes are certainly an ad-hoc fix, if you can further refactor the code, that would certainly be better.

I think, in general, is a difficult problem to place a fixed number of bytes into a numeric type. While investigating this issue, I found the standard does not like prescribing explicit widths to its number types.

For integers, this is somewhat alleviated by the intN_t types, but the situation is murkier for floating types. Indeed, LDBL_MANT_DIG == 64 is used since there's no direct way to access the long double width.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants