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-97588: Fix ctypes structs #97702

Merged
merged 128 commits into from
May 29, 2024
Merged

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Oct 1, 2022

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 5, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit ba61051 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 5, 2024
@encukou
Copy link
Member

encukou commented May 6, 2024

This is about as good as it'll be for 3.13. So there is a decision to make:

  • Put this in, and iterate with bugfixes. This PR is an improvement, but if more of those come in 3.14, things might break twice for users that rely on workarounds.
  • Delay to 3.14, and iterate with more freedom. I can fix this properly in 2024, if I'm allowed to spend dev-in-rez time on it.

I'm torn here. Does anyone want to play a more impartial judge? Or just chip in with an opinion? @zooba, @gpshead


For an idea about what could go into 3.14:

Things that are under-tested or missing, where fixes might break working code:

  • unions
  • Matching unsupported (non-PEP11) architectures & compilers
  • handling types whose alignment != size
  • GCC-style packed structs/bitfields (requires straddling bitfields, which needs a bigger refactoring)

Missing features that would IMO be needed before we can claim that ctypes can match a C struct in the common ABIs:

  • support for zero-length bit fields, which to most compilers means “avoid packing a bitfield together with the previous one” (but ctypes uses zero as “not a bitfield”)

Things needed for better testability or implementability of the above:

  • fields/types that are stored as PyLong_AsNativeBytes arguments, rather than struct codes
  • better reflection of ctypes
  • Hypothesis tests & fuzzing

For this PR: all buildbots fail on only:

  • s390x (here for example) -- might be due to stricter tests; today I can probably either fix this or at least build confidence to skip the test.
  • Stuff that's not ctypes-related: test_pyrepl, test_import and a few more pre-existing failures, test_cext, No space left on device.

@matthiasgoergens
Copy link
Contributor Author

This stuff has been broken since forever, and even this PR has been slumbering for a long time.

So I would vote in favour of taking your time and fixing this properly, and merging it into whatever version of Python is the one currently being worked on when you are ready.

But I have no strong feelings either way.

@encukou
Copy link
Member

encukou commented May 6, 2024

!buildbot s390x SLES PR

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit bc1225b 🤖

The command will test the builders whose names match following regular expression: s390x SLES PR

The builders matched are:

  • s390x SLES PR

@gvanrossum
Copy link
Member

This stuff has been broken since forever, and even this PR has been slumbering for a long time.

So I would vote in favour of taking your time and fixing this properly, and merging it into whatever version of Python is the one currently being worked on when you are ready.

But I have no strong feelings either way.

This is a tough one. I'm leaning in the same direction -- this is substantial work, and if you merge now you may spend the entire beta period having to tweak it, and possibly the 3.14 release cycle regretting it.

If you had come with this two weeks ago I would have said "put it in", but now it's very close to beta 1.

You could talk to the RM for a 3rd opinion.

@encukou
Copy link
Member

encukou commented May 6, 2024

@gpshead, you asked me to look into this. Was that to try to get it in 3.13, or just to move it forward?

@gpshead
Copy link
Member

gpshead commented May 6, 2024

I like the conclusions above that Matthias and Guido make: Save this for 3.14.

IIRC I asked you to look into it because it seemed hairy enough to understand the details of that I didn't imagine it would ever go forward without a committer dedicating time to understand it as you have. You've done a lot. Even if this went in for 3.13 I'd call it an improvement, but given more potential work to be done has been identified in the process, waiting for 3.14 isn't going to hurt anyone and will avoid repeated behavior-fix-transition pain.

@matthiasgoergens
Copy link
Contributor Author

Btw, if you are taking your time to fix this properly, I would like to suggest migrating the intricate logic that calculates the offsets etc from the specification from C to Python.

That way it's much easier to understand and maintain, and this calculation only happens once per struct definition, so it shouldn't be in any inner loop and doesn't need to be C-level fast.

When I wrote my prototype that you kindly took over, I actually started sketching out my logic in Python before translating it into C.

@encukou
Copy link
Member

encukou commented May 7, 2024

Thank you for confirming! 3.14 it is.

Yes, rewriting this in Python is the way to go.

it seemed hairy enough to understand the details of that I didn't imagine it would ever go forward without a committer dedicating time to understand it

Yup. There are a few modules like that; I was diving into argparse when you invited me here :)

@encukou
Copy link
Member

encukou commented May 10, 2024

I'll merge this when I'll be available to fix any issues quickly: after PyCon US, or at the sprints if someone wants to pair up there.

@encukou encukou merged commit 18c1a8d into python:main May 29, 2024
37 checks passed
@encukou
Copy link
Member

encukou commented May 29, 2024

Argh, I hit Enter while I was editing the commit message. Sorry for that :(
Should have been something like:

Structure layout, and especially bitfields, sometimes resulted in clearly
wrong behaviour like overlapping fields. This fixes the bug, and adds the
attribute `_layout_` to select either MSVC-like or GCC-like behavior.
This API may still change for 3.14 as we add more flexible struct packing
options.

Co-authored-by: Gregory P. Smith <gps@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
Structure layout, and especially bitfields, sometimes resulted in clearly
wrong behaviour like overlapping fields. This fixes

Co-authored-by: Gregory P. Smith <gps@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Structure layout, and especially bitfields, sometimes resulted in clearly
wrong behaviour like overlapping fields. This fixes

Co-authored-by: Gregory P. Smith <gps@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@zware
Copy link
Member

zware commented Aug 5, 2024

FYI: this appears to have broken tests on x86 (32-bit) Linux, see gh-121938.

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.