Skip to content

gh-130317: Skip test_pack_unpack_roundtrip_for_nans() on x86 #133155

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 1 commit into from
Apr 30, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 29, 2025

Reduce also the number of iterations from 1000 to 10 to ease
debugging failures and prevent "command line too line" error when
tests are re-run.

@vstinner
Copy link
Member Author

cc @skirpichev

@vstinner
Copy link
Member Author

Example of failure with 32-bit float on x86:

float_pack:  {0x00,0x00,0x00,0x20,0xdc,0xd1,0xf2,0x7f}
Float_Pack4: {0x00,0x00,0x00,0x20,0xdc,0xd1,0xfa,0x7f}

0xf2 in _testcapi.float_pack() becomes 0xfa in PyFloat_Pack4(). The sNaN becomes a qNaN just by the calling convention, there is nothing that we can do.

There is a similar problem with PyFloat_Pack2().

I'm not sure why it works with PyFloat_Pack8().

Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

LGTM (as a temporary fix). Don't close issue.

0xf2 in _testcapi.float_pack() becomes 0xfa in PyFloat_Pack4(). The sNaN becomes a qNaN just by the calling convention, there is nothing that we can do.

I'm not sure why it works with PyFloat_Pack8().

It shouldn't, if your explanation is valid.

@skirpichev
Copy link
Member

I'm not sure why it works with PyFloat_Pack8().

Hmm, my tests show that on 32-bit sNaN becomes qNaN after calling. Are you sure that size=8 tests works?

@vstinner
Copy link
Member Author

!buildbot x86

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 2d62e7b 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133155%2Fmerge

The command will test the builders whose names match following regular expression: x86

The builders matched are:

  • x86 Debian Non-Debug with X PR
  • x86-64 MacOS Intel NoGIL PR
  • x86-64 macOS PR
  • x86-64 MacOS Intel ASAN NoGIL PR
  • x86 Debian Installed with X PR

@vstinner
Copy link
Member Author

Are you sure that size=8 tests works?

It works on Python built with gcc -O3. But it failed on the x86 Debian buildbot :-(

Reduce also the number of iterations from 1000 to 10 to ease
debugging failures and prevent "command line too line" error when
tests are re-run.
@vstinner vstinner force-pushed the test_float_roundtrip branch from 2d62e7b to c39b3c7 Compare April 30, 2025 09:33
@vstinner vstinner changed the title gh-130317: Fix test_pack_unpack_roundtrip_for_nans() on x86 gh-130317: Skip test_pack_unpack_roundtrip_for_nans() on x86 Apr 30, 2025
@skirpichev
Copy link
Member

It works on Python built with gcc -O3

I guess, optimizer avoid using FPU registers

As a quick fix I suggest to disable testing in 32-bit mode. And also reduce number of tests as suggested, 10 should be enough.

In a follow-up PR I'll do cleanup. Probably float_set_snan() is not helpful, as well as other attempts to workaround passing of sNaN.

@vstinner
Copy link
Member Author

@skirpichev: I rewrote my PR to simply skip the test on x86 Debian, but not skip the test on x86 Windows. Please review the updated PR.

@vstinner
Copy link
Member Author

!buildbot x86 Debian

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit c39b3c7 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133155%2Fmerge

The command will test the builders whose names match following regular expression: x86 Debian

The builders matched are:

  • x86 Debian Installed with X PR
  • x86 Debian Non-Debug with X PR

Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

Lets merge when bots pass.

@vstinner vstinner merged commit 0f23e84 into python:main Apr 30, 2025
40 checks passed
@vstinner vstinner deleted the test_float_roundtrip branch April 30, 2025 10:01
@vstinner
Copy link
Member Author

Merged, thanks for the review.

Sadly, I don't think that we can work around "compiler bugs" if sNaN becomes qNaN as soon as we call a simple function taking a double. Skipping the test sounds like the only option.

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

Successfully merging this pull request may close these issues.

3 participants