Skip to content

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

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

Merged
merged 26 commits into from
Apr 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
971fd98
gh-130317: fix PyFloat_Pack2/Unpack2 for NaN's with payload
skirpichev Feb 22, 2025
b05720a
Merge branch 'master' into fix-nan-packing/130317
skirpichev Feb 25, 2025
f513b14
more tests in test_capi, fix also Pack/Unpack4
skirpichev Feb 25, 2025
218f6b3
+ different fix for Pack/Unpack2 (works as for floats)
skirpichev Feb 25, 2025
a7f5ed9
blacklist on win32
skirpichev Feb 25, 2025
c7c08ff
cleanup and comments
skirpichev Feb 25, 2025
de7b6aa
+ enable test_pack_unpack_roundtrip_nans on win32 for qNaN's
skirpichev Feb 25, 2025
e6c1c12
+1
skirpichev Feb 25, 2025
ac7bdcb
address review: warning on Windows
skirpichev Feb 25, 2025
b461b81
XXX try to revert win32 WA
skirpichev Feb 25, 2025
cbfbf05
Revert "XXX try to revert win32 WA"
skirpichev Feb 26, 2025
6a5dc1a
Merge branch 'master' into fix-nan-packing/130317
skirpichev Feb 26, 2025
86ab7c3
Merge branch 'main' into fix-nan-packing/130317
skirpichev Apr 27, 2025
32bdeed
revert redundant test
skirpichev Apr 27, 2025
773f51e
rename test
skirpichev Apr 27, 2025
b27c916
XXX revert win32 wa
skirpichev Apr 27, 2025
4dc5697
+1
skirpichev Apr 27, 2025
6f71e34
+1
skirpichev Apr 27, 2025
3e5a211
revert
skirpichev Apr 27, 2025
092d729
fixup value on win32
skirpichev Apr 27, 2025
6137c75
cleanup & comments
skirpichev Apr 27, 2025
00dfd66
address review: redo helper
skirpichev Apr 27, 2025
b7f727c
Apply suggestions from code review
skirpichev Apr 28, 2025
13e8310
restore comment
skirpichev Apr 28, 2025
bcf54f6
address review: ensure code works with strict aliasing
skirpichev Apr 28, 2025
9d20633
Apply suggestions from code review
skirpichev Apr 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions Lib/test/test_capi/test_float.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import math
import random
import sys
import unittest
import warnings
Expand Down Expand Up @@ -178,6 +179,39 @@ def test_pack_unpack_roundtrip(self):
else:
self.assertEqual(value2, value)

@unittest.skipUnless(HAVE_IEEE_754, "requires IEEE 754")
def test_pack_unpack_roundtrip_for_nans(self):
pack = _testcapi.float_pack
unpack = _testcapi.float_unpack
for _ in range(1000):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't think speed is really an issue here, but having the potential for 6000 failed test reports seems like major overkill. I think 10 would actually be plenty for this loop.

for size in (2, 4, 8):
sign = random.randint(0, 1)
signaling = random.randint(0, 1)
quiet = int(not signaling)
if size == 8:
payload = random.randint(signaling, 1 << 50)
i = (sign << 63) + (0x7ff << 52) + (quiet << 51) + payload
elif size == 4:
payload = random.randint(signaling, 1 << 21)
i = (sign << 31) + (0xff << 23) + (quiet << 22) + payload
elif size == 2:
payload = random.randint(signaling, 1 << 8)
i = (sign << 15) + (0x1f << 10) + (quiet << 9) + payload
data = bytes.fromhex(f'{i:x}')
for endian in (BIG_ENDIAN, LITTLE_ENDIAN):
with self.subTest(data=data, size=size, endian=endian):
data1 = data if endian == BIG_ENDIAN else data[::-1]
value = unpack(data1, endian)
if signaling and sys.platform == 'win32':
# On this platform sNaN becomes qNaN when returned
# from function. That's a known bug, e.g.
# https://developercommunity.visualstudio.com/t/155064
# (see also gh-130317).
value = _testcapi.float_set_snan(value)
data2 = pack(size, value, endian)
self.assertTrue(math.isnan(value))
self.assertEqual(data1, data2)


if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix :c:func:`PyFloat_Pack2` and :c:func:`PyFloat_Unpack2` for NaN's with
payload. This corrects round-trip for :func:`struct.unpack` and
:func:`struct.pack` in case of the IEEE 754 binary16 "half precision" type.
Patch by Sergey B Kirpichev.
11 changes: 10 additions & 1 deletion Modules/_testcapi/clinic/float.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions Modules/_testcapi/float.c
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,39 @@ test_string_to_double(PyObject *self, PyObject *Py_UNUSED(ignored))
}


/*[clinic input]
_testcapi.float_set_snan

obj: object
/

Make a signaling NaN.
[clinic start generated code]*/

static PyObject *
_testcapi_float_set_snan(PyObject *module, PyObject *obj)
/*[clinic end generated code: output=f43778a70f60aa4b input=c1269b0f88ef27ac]*/
{
if (!PyFloat_Check(obj)) {
PyErr_SetString(PyExc_ValueError, "float-point number expected");
return NULL;
}
double d = ((PyFloatObject *)obj)->ob_fval;
if (!isnan(d)) {
PyErr_SetString(PyExc_ValueError, "nan expected");
return NULL;
}
uint64_t v;
memcpy(&v, &d, 8);
v &= ~(1ULL << 51); /* make sNaN */
memcpy(&d, &v, 8);
return PyFloat_FromDouble(d);
}

static PyMethodDef test_methods[] = {
_TESTCAPI_FLOAT_PACK_METHODDEF
_TESTCAPI_FLOAT_UNPACK_METHODDEF
_TESTCAPI_FLOAT_SET_SNAN_METHODDEF
{"test_string_to_double", test_string_to_double, METH_NOARGS},
{NULL},
};
Expand Down
49 changes: 42 additions & 7 deletions Objects/floatobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2021,14 +2021,13 @@ PyFloat_Pack2(double x, char *data, int le)
bits = 0;
}
else if (isnan(x)) {
/* There are 2046 distinct half-precision NaNs (1022 signaling and
1024 quiet), but there are only two quiet NaNs that don't arise by
quieting a signaling NaN; we get those by setting the topmost bit
of the fraction field and clearing all other fraction bits. We
choose the one with the appropriate sign. */
sign = (copysign(1.0, x) == -1.0);
e = 0x1f;
bits = 512;

uint64_t v;
memcpy(&v, &x, sizeof(v));
v &= 0xffc0000000000ULL;
bits = (unsigned short)(v >> 42); /* NaN's type & payload */
}
else {
sign = (x < 0.0);
Expand Down Expand Up @@ -2192,6 +2191,21 @@ PyFloat_Pack4(double x, char *data, int le)
if (isinf(y) && !isinf(x))
goto Overflow;

/* correct y if x was a sNaN, transformed to qNaN by conversion */
if (isnan(x)) {
uint64_t v;

memcpy(&v, &x, 8);
if ((v & (1ULL << 51)) == 0) {
union float_val {
float f;
uint32_t u32;
} *py = (union float_val *)&y;

py->u32 &= ~(1 << 22); /* make sNaN */
}
}

unsigned char s[sizeof(float)];
memcpy(s, &y, sizeof(float));

Expand Down Expand Up @@ -2374,7 +2388,11 @@ PyFloat_Unpack2(const char *data, int le)
}
else {
/* NaN */
return sign ? -fabs(Py_NAN) : fabs(Py_NAN);
uint64_t v = sign ? 0xfff0000000000000ULL : 0x7ff0000000000000ULL;

v += (uint64_t)f << 42; /* add NaN's type & payload */
memcpy(&x, &v, sizeof(v));
return x;
}
}

Expand Down Expand Up @@ -2470,6 +2488,23 @@ PyFloat_Unpack4(const char *data, int le)
memcpy(&x, p, 4);
}

/* return sNaN double if x was sNaN float */
if (isnan(x)) {
uint32_t v;
memcpy(&v, &x, 4);

if ((v & (1 << 22)) == 0) {
double y = x; /* will make qNaN double */
union double_val {
double d;
uint64_t u64;
} *py = (union double_val *)&y;

py->u64 &= ~(1ULL << 51); /* make sNaN */
return y;
}
}

return x;
}
}
Expand Down
Loading