Page MenuHomeFreeBSD

Fix lib/msun/tests/csqrt_test on platforms with 128-bit long double
ClosedPublic

Authored by arichardson on Mar 5 2021, 11:17 AM.

Details

Summary

If long double has more than 64 mantissa bits, using uint64_t to hold the
mantissa bits will truncate the value and result in test failures. To fix
this problem use uint128_t since all platforms that have
LDBL_MANT_DIG__ > 64 also have compiler support for 128-bit integers.

Depends on D28798

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lib/msun/tests/csqrt_test.c
293–297

What is CHECK_FPEQUAL? I don't see it by grepping or in the review stack.

348–354

I don't get either the FIXME comment or how this fixes i386. Can you explain?

lib/msun/tests/csqrt_test.c
293–297

Sorry, this is missing and added by another review that I apparently haven't uploaded yet. It's just a wrapper around ATF_CHECK that also prints the mismatched values.

348–354

I added the FIXME in an earlier commit since using DBL_MANT_DIG for AArch64 seemed wrong. LDBL_MANT_DIG would previously result in overflow so was wrong for all architectures with LDBL_MANT_DIG > 64.
I haven't tested i386, but since it should be using ld80 I would expect it to behave in the same way as amd64?

lib/msun/tests/csqrt_test.c
293–297

Okay, that would be fine. I just wanted to make sure it was still an exact check.

348–354

I think you might be reading it backward? Only i386 gets DBL_MANT_DIG, because i386 long doubles are weird and broken. It was probably confusing and bad style to use ifndef instead of ifdef.

lib/msun/tests/csqrt_test.c
348–354

Ah indeed I did invert the condition. Will update revision to drop this change.

arichardson added inline comments.
lib/msun/tests/csqrt_test.c
293–297

I've now uploaded it as D29091.

rlibby requested changes to this revision.Mar 5 2021, 11:10 PM

Change looks fine once the i386 hack is restored.

lib/msun/tests/csqrt_test.c
260–272

Maybe like this? This is just for testing the test though, not the interesting code, so take it or leave it.

This revision now requires changes to proceed.Mar 5 2021, 11:10 PM
arichardson marked 5 inline comments as done.

Apply review suggestions and add a comment regarding the i386 ifdef

lib/msun/tests/csqrt_test.c
261

128

354

It's weirder than this... Here's a reference: https://lists.freebsd.org/pipermail/freebsd-numerics/2012-September/000288.html

In short, I would change "precision" to "rounding" or "rounding precision", otherwise the comment doesn't seem to explain why LDBL_MANT_DIG isn't just 53 on i386.

lib/msun/tests/csqrt_test.c
261

Thanks, that was a stupid typo. Should actually test the code before updating...

arichardson marked 2 inline comments as done.

Fix typo and improve comment

This revision is now accepted and ready to land.Mon, Mar 22, 2:54 PM