GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-130317: fix PyFloat_Pack/Unpack[24] for NaN's with payload by skirpichev · Pull Request #130452 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Feb 22, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added the awaiting review label Feb 22, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 22, 2025
struct (un)packing of half-precision nan floats is non-invertible #130317
Closed
@skirpichev
skirpichev marked this pull request as draft February 22, 2025 10:40
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 22, 2025
@skirpichev
skirpichev force-pushed the fix-nan-packing/130317 branch from 7da9a1f to a6e99f9 Compare February 22, 2025 11:19
@skirpichev
pythongh-130317: fix PyFloat_Pack2/Unpack2 for NaN's with payload
971fd98
@skirpichev
skirpichev force-pushed the fix-nan-packing/130317 branch from a6e99f9 to 971fd98 Compare February 23, 2025 03:15
@skirpichev
skirpichev marked this pull request as ready for review February 23, 2025 03:53
@bedevere-app bedevere-app Bot added the awaiting review label Feb 23, 2025
vstinner

This comment was marked as outdated.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

This comment was marked as outdated.

Sign in to view
skirpichev added 2 commits February 25, 2025 03:20
@skirpichev
Merge branch 'master' into fix-nan-packing/130317
b05720a
@skirpichev
more tests in test_capi, fix also Pack/Unpack4
f513b14
@skirpichev
skirpichev marked this pull request as draft February 25, 2025 06:04
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 25, 2025
skirpichev added 3 commits February 25, 2025 09:26
@skirpichev
+ different fix for Pack/Unpack2 (works as for floats)
218f6b3
@skirpichev
blacklist on win32
a7f5ed9
@skirpichev
cleanup and comments
c7c08ff
@skirpichev skirpichev changed the title gh-130317: fix PyFloat_Pack2/Unpack2 for NaN's with payload gh-130317: fix PyFloat_Pack/Unpack[24] for NaN's with payload Feb 25, 2025
skirpichev added 2 commits February 25, 2025 10:29
@skirpichev
+ enable test_pack_unpack_roundtrip_nans on win32 for qNaN's
de7b6aa
@skirpichev
+1
e6c1c12
@skirpichev
skirpichev force-pushed the fix-nan-packing/130317 branch from 139501d to e6c1c12 Compare February 25, 2025 08:24
@skirpichev
skirpichev marked this pull request as ready for review February 25, 2025 09:12
@bedevere-app bedevere-app Bot added the awaiting review label Feb 25, 2025
@skirpichev

skirpichev commented Feb 25, 2025

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
skirpichev requested a review from vstinner February 25, 2025 09:16
vstinner
vstinner reviewed Feb 25, 2025
View reviewed changes
Comment thread Objects/floatobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 25, 2025

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
address review: warning on Windows
ac7bdcb
@skirpichev

skirpichev commented Feb 25, 2025

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting review label Apr 27, 2025
@skirpichev

skirpichev commented Apr 27, 2025

Copy link
Copy Markdown
Member Author

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*).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Apr 27, 2025
View reviewed changes
Comment thread Modules/_testcapi/float.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/float.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
address review: redo helper
00dfd66
@skirpichev
skirpichev requested a review from vstinner April 27, 2025 15:18
vstinner
vstinner reviewed Apr 28, 2025
View reviewed changes
Comment thread Modules/_testcapi/float.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_float.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/floatobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/floatobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/floatobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 3 commits April 28, 2025 13:42
@skirpichev @vstinner
Apply suggestions from code review
b7f727c
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev
restore comment
13e8310
@skirpichev
address review: ensure code works with strict aliasing
bcf54f6
@skirpichev
skirpichev requested a review from vstinner April 28, 2025 11:57
vstinner
vstinner approved these changes Apr 28, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. I just have some minor comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_capi/test_float.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_float.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_float.py
def test_pack_unpack_roundtrip_for_nans(self):
pack = _testcapi.float_pack
unpack = _testcapi.float_unpack
for _ in range(1000):

@vstinner vstinner Apr 28, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skirpichev skirpichev Apr 28, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@zware zware Apr 29, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Apr 28, 2025
skirpichev
skirpichev commented Apr 28, 2025
View reviewed changes
Comment thread Lib/test/test_capi/test_float.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev @vstinner
Apply suggestions from code review
9d20633
Co-authored-by: Victor Stinner <[email protected]>
@vstinner
vstinner merged commit 6157135 into python:main Apr 28, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 28, 2025
@vstinner

vstinner commented Apr 28, 2025

Copy link
Copy Markdown
Member

Merged, thank you.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 28, 2025

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
skirpichev deleted the fix-nan-packing/130317 branch April 28, 2025 13:29
@skirpichev

skirpichev commented Apr 28, 2025

Copy link
Copy Markdown
Member Author

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).

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

test_capi.test_float fails on x86 Debian Installed with X 3.x: https://buildbot.python.org/#/builders/1244/builds/5176

test.pythoninfo:

CC.version: gcc (Debian 12.2.0-14) 12.2.0

builtins.float.double_format: IEEE, little-endian
builtins.float.float_format: IEEE, little-endian

platform.architecture: 32bit ELF
platform.freedesktop_os_release[NAME]: Debian GNU/Linux
platform.freedesktop_os_release[VERSION]: 12 (bookworm)
platform.libc_ver: glibc 2.36
platform.platform: Linux-6.1.0-33-686-pae-i686-with-glibc2.36

sys.float_info: sys.float_info(max=1.7976931348623157e+308, max_exp=1024, max_10_exp=308, min=2.2250738585072014e-308, min_exp=-1021, min_10_exp=-307, dig=15, mant_dig=53, epsilon=2.220446049250313e-16, radix=2, rounds=1)
sys.float_repr_style: short

sys.maxsize: 2147483647

There are failures on 16, 32 and 64 bit floats (size: 2, 4, 8). Examples:

FAIL: test_pack_unpack_roundtrip_for_nans (test.test_capi.test_float.CAPIFloatTest.test_pack_unpack_roundtrip_for_nans)
(data=b'\x7f\x98Pr', size=4, endian=0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/test_capi/test_float.py", line 213, in test_pack_unpack_roundtrip_for_nans
    self.assertEqual(data1, data2)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
AssertionError: b'\x7f\x98Pr' != b'\x7f\xd8Pr'

======================================================================
FAIL: test_pack_unpack_roundtrip_for_nans (test.test_capi.test_float.CAPIFloatTest.test_pack_unpack_roundtrip_for_nans) 
(data=b'|\xd6', size=2, endian=0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/3.x.ware-debian-x86.installed/build/target/lib/python3.14/test/test_capi/test_float.py", line 213, in test_pack_unpack_roundtrip_for_nans
    self.assertEqual(data1, data2)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
AssertionError: b'|\xd6' != b'~\xd6'

======================================================================
FAIL: test_pack_unpack_roundtrip_for_nans (test.test_capi.test_float.CAPIFloatTest.test_pack_unpack_roundtrip_for_nans) 
(data=b'\x7f\xf2h\xf8\x1bU*\xc1', size=8, endian=0)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/3.x.ware-debian-x86.installed/build/target/lib/python3.14/test/test_capi/test_float.py", line 213, in test_pack_unpack_roundtrip_for_nans
    self.assertEqual(data1, data2)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
AssertionError: b'\x7f\xf2h\xf8\x1bU*\xc1' != b'\x7f\xfah\xf8\x1bU*\xc1'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

I can reproduce the issue on Fedora 42 by building Python using gcc -m32:

./configure CFLAGS="-m32" LDFLAGS="-m32"
sed -i -e 's!#define PY_HAVE_PERF_TRAMPOLINE 1!#undef PY_HAVE_PERF_TRAMPOLINE!g' pyconfig.h
make
./python -m test -v test_capi.test_float
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Apr 29, 2025

Copy link
Copy Markdown
Member Author

Hmm, again in all cases sNaN seems transformed to qNaN.

I think underlying reason is same and we should apply workaround for 32-bit modes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

I think underlying reason is same and we should apply workaround for 32-bit modes.

I replaced if signaling and sys.platform == 'win32': with if signaling and True: in test_capi.test_float, but it didn't work around the issue :-( Maybe _testcapi.float_set_snan() doesn't work on x86.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

Aha, just copying a double to another double clears the sNaN flag:

(gdb) n
33	    double d = ((PyFloatObject *)obj)->ob_fval;
(gdb) n
35	    switch (size)
(gdb) p ((PyFloatObject *)obj)->ob_fval
$3 = nan(0x5e5454683683)
(gdb) p d
$4 = nan(0x85e5454683683)

d is not equal to ob_fval.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

The sNaN flag is easily lost. I added some debug of the uint64_t value in different functions. Between float_pack() and PyFloat_Pack4(), the sNaN flag is lost:

float_set_snan(): v=fffbce9e80000000 (before)
float_set_snan(): v=fff3ce9e80000000 (sNaN)
float_pack():     v=fff3ce9e80000000
PyFloat_Pack4():  v=fffbce9e80000000

PyFloat_Pack4() is called with the wrong value :-(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Apr 29, 2025

Copy link
Copy Markdown
Member Author

Maybe _testcapi.float_set_snan() doesn't work on x86.

Hmm, indeed. I should finally do memcpy() to created PyFloat's ob_fval, then return it.

Does it work:

diff --git a/Modules/_testcapi/float.c b/Modules/_testcapi/float.c
index 2feeb205d8..b382883fa8 100644
--- a/Modules/_testcapi/float.c
+++ b/Modules/_testcapi/float.c
@@ -181,9 +181,12 @@ _testcapi_float_set_snan(PyObject *module, PyObject *obj)
     }
     uint64_t v;
     memcpy(&v, &d, 8);
-    v &= ~(1ULL << 51); /* make sNaN */
-    memcpy(&d, &v, 8);
-    return PyFloat_FromDouble(d);
+    if (v & (1ULL << 51)) {
+        v -= (1ULL << 51); /* make sNaN */
+    }
+    PyObject *ret = PyFloat_FromDouble(0.0);
+    memcpy(&(((PyFloatObject *)ret)->ob_fval), &v, 8);
+    return ret;
 }
 
 static PyMethodDef test_methods[] = {

?

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 29, 2025

Copy link
Copy Markdown
Member

I wrote #133150 to fix 3 bugs, but it's not enough to fix the test on x86.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented May 5, 2025

Copy link
Copy Markdown
Member

Tests were fixed by #133204.

zware reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware left review comments
@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@skirpichev @vstinner @bedevere-bot @zware
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.