Skip to content
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

gh-128889: Zero out memory ctypes for generated struct layout tests #128944

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

encukou
Copy link
Member

@encukou encukou commented Jan 17, 2025

GCC 15 doesn't set padding bits to zero (as it's allowed to, according to standard). Let's zero the test structs explicitly.

Even with this PR, the tests still depend on the compiler keeping padding bits unchanged when surrounding values are set. (AFAIK, this is not guaranteed by the standard, and when bitfields are involved, changing padding bits might make sense as an optimization.)
Removing that limitation will be easier with introspection attributes I want to add in gh-128715.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM: I confirm that the change does fix the #128889 issue.

Without this change, test_ctypes.test_generated_structs fails. With the change, test_ctypes.test_generated_structs pass.

@encukou encukou merged commit 13475e0 into python:main Jan 21, 2025
41 checks passed
@encukou encukou deleted the ctypes-padding-bits branch January 21, 2025 15:29
@encukou
Copy link
Member Author

encukou commented Jan 21, 2025

Thank you for the check!

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 macOS 3.x has failed when building commit 13475e0.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/725/builds/9507) and take a look at the build logs.
  4. Check if the failure is related to this commit (13475e0) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/725/builds/9507

Failed tests:

  • test.test_multiprocessing_forkserver.test_processes
  • test_ssl

Failed subtests:

  • test_repr_rlock - test.test_multiprocessing_forkserver.test_processes.WithProcessesTestLock.test_repr_rlock
  • test_preauth_data_to_tls_server - test.test_ssl.TestPreHandshakeClose.test_preauth_data_to_tls_server

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/_test_multiprocessing.py", line 1530, in test_repr_rlock
    self.assertEqual('<RLock(SomeOtherThread, nonzero)>', repr(lock))
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: '<RLock(SomeOtherThread, nonzero)>' != '<RLock(None, 0)>'
- <RLock(SomeOtherThread, nonzero)>
+ <RLock(None, 0)>


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ssl.py", line 5094, in test_preauth_data_to_tls_server
    self.assertIn("before TLS handshake with data", wrap_error.args[1])
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'before TLS handshake with data' not found in '[SSL: RECORD_LAYER_FAILURE] record layer failure (_ssl.c:1066)'

@befeleme
Copy link
Contributor

I can confirm that the tests executed during the rpmbuild pass with the fix.

befeleme pushed a commit to fedora-python/cpython that referenced this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants