gh-130317: fix PyFloat_Pack/Unpack[24] for NaN's with payload

[skirpichev]

Previously (1) all NaN's payload in PyFloat_Pack/Unpack2() was ignored and (2) type conversions (float <-> double) in PyFloat_Pack/Unpack4() broke pack-unpack round-trip for sNaN's.

---

• Issue: struct (un)packing of half-precision nan floats is non-invertible #130317

[skirpichev]

Ok, I've added new tests in test_capi.test_floats (in principle, test_struct's tests are redundant).

win32 behavior (you can't unset "quiet" bit for NaN) looks as a bug for me.

[vstinner]

win32 behavior (you can't unset "quiet" bit for NaN) looks as a bug for me.

Any idea why there is a bug only on Windows?

[skirpichev]

Any idea why there is a bug only on Windows?

Only on win32. Though, I suspect the situation could be worse. Maybe I should revert win32-workaround and test this with some buildbots?

C17 says: "This specification does not define the behavior of signaling NaNs." But I don't think this means you can't flip appropriate bit in float/double to make a signaling NaN.

[skirpichev]

Ok, I think it's ready for review.

Test for Windows failed in 32-bit mode (signaling NaN type not preserved in roundtrip), that seems to be a known issue: https://developercommunity.visualstudio.com/t/155064 (sNaN returned from function becomes qNaN). See also https://developercommunity.visualstudio.com/t/903305 and https://en.delphipraxis.net/topic/12198-delphi-win32-quiets-signaling-nan-on-function-return/.

In principle, it's possible to workaround this for the struct module. But not for C-API (PyFloat_Unpack*).

[vstinner]

LGTM. I just have some minor comments.

[vstinner]

100 tests should be enough to validate the implementation, no?

Suggested change

	for _ in range(1000):
	for _ in range(100):

1000 tests might be a little bit too slow, I don't think that it's worth it.

[skirpichev]

Up to you, the test looks instantaneous on my system. 0.3sec vs 0.03. Where the threshold?

[vstinner]

Merged, thank you.

[vstinner]

Should we backport this change? It's unclear to me if it's a new feature or a bugfix. The issue is reported as a bug report.

[skirpichev]

The issue is reported as a bug report.

Strictly speaking, it's a bug, though maybe a very minor one (that's why there was no label for 3.13 backport).

From IEEE 754 (2008), 6.2.3: "Conversion of a quiet NaN from a narrower format to a wider format in the same radix, and then back to the same narrower format, should not change the quiet NaN payload in any way except to make it canonical."

E.g. in C nan's payload is preserved in conversions (e.g. float->double) or partially preserved (e.g. double -> float).