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-97588: Fix ctypes structs by matthiasgoergens · Pull Request #97702 · python/cpython

Conversation

@matthiasgoergens

@matthiasgoergens matthiasgoergens commented Oct 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The existing code is quite a mess and doesn't correspond to what gcc nor clang nor msvc are doing.

This PR fixes all the issues below.

  • Issue: ctypes: bit field data does not survive round trip #97588
  • Issue: ctypes: bitfield lost data with union on linux platform #95496
  • Issue: bitfield layout wrong in ctypes #84039
  • Issue: [Linux] ctypes packs bitfields Incorrectly #73939
  • Issue: ctypes mixed-types bitfield layout nonsensical; doesn't match compiler. #59324
  • Issue: Incorrect struct definition with bitfields #86098

Sorry, something went wrong.

Uh oh!

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

gpshead reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@matthiasgoergens
pythongh-97588: Fix ctypes structs
7922585
@bedevere-bot bedevere-bot added the awaiting review label Oct 1, 2022
blurb-it Bot and others added 23 commits October 1, 2022 09:56
@blurb-it
📜🤖 Added by blurb_it.
2307932
@matthiasgoergens
Handling pack as well
47f826c
@matthiasgoergens
Handle basedict, too
5a32211
@matthiasgoergens
Merge branch 'main' into fix-bitfield-clean
8d79d8f
@matthiasgoergens
Merge branch 'main' into fix-bitfield-clean
2d07375
@matthiasgoergens
Merge remote-tracking branch 'matthias/fix-bitfield-clean' into fix-b…
8bc8535
…itfield-clean
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
c3d162b
@matthiasgoergens
Split windows and linux
e5ed9ac
@matthiasgoergens
Compiles
b0f9819
@matthiasgoergens
Abstract out proto stuff
2dee0e3
@matthiasgoergens
Clean align
79ef347
@matthiasgoergens
Hypothesis tests pass
6170dad
@matthiasgoergens
Formatting
871ca1a
@matthiasgoergens
Clean up
c162144
@matthiasgoergens
Adapt tests for Windows
a57cf2c
@matthiasgoergens
Fix order
3f7c4cc
@matthiasgoergens
Fix alignment test
e39a271
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
359ed58
@matthiasgoergens
Avoid casting
52ef8d2
@matthiasgoergens
Fixup
e8102c4
@matthiasgoergens
Merge branch 'main' into fix-bitfield-clean
600e144
@matthiasgoergens
Merge branch 'main' into fix-bitfield-clean
9bad706
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
5121857
@matthiasgoergens matthiasgoergens mentioned this pull request Oct 6, 2022
gh-97588: Failing tests to demonstrate the issue #97589
Closed
matthiasgoergens added 4 commits October 8, 2022 13:49
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
6cd27ea
@matthiasgoergens
Add ability to force msvc compatibility even when not doing any packing
6b6fa8a
@matthiasgoergens
More tests
ca9d580
@matthiasgoergens
Merge branch 'main' into fix-bitfield-clean
1401ee4
@bedevere-bot

bedevere-bot commented May 5, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

encukou commented May 6, 2024

Copy link
Copy Markdown
Member

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.
gpshead reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

encukou added 2 commits May 6, 2024 11:37
@encukou
Use proper PyArg_ParseTuple code for Py_ssize_t
8991444
@encukou
Merge in the main branch
bc1225b
@matthiasgoergens

matthiasgoergens commented May 6, 2024

Copy link
Copy Markdown
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 and gpshead reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented May 6, 2024

Copy link
Copy Markdown
Member

!buildbot s390x SLES PR

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented May 6, 2024

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented May 6, 2024

Copy link
Copy Markdown
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 and gpshead reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented May 6, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead

gpshead commented May 6, 2024

Copy link
Copy Markdown
Member

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.

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

@matthiasgoergens

matthiasgoergens commented May 7, 2024

Copy link
Copy Markdown
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.

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

@encukou

encukou commented May 7, 2024

Copy link
Copy Markdown
Member

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

matthiasgoergens reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented May 10, 2024

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou
encukou merged commit 18c1a8d into python:main May 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label May 29, 2024
@encukou

encukou commented May 29, 2024

Copy link
Copy Markdown
Member

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 <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>
All reactions

Sorry, something went wrong.

Uh oh!

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

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@matthiasgoergens @gpshead
@encukou @estyxx
pythongh-97588: Align ctypes struct layout to GCC/MSVC (pythonGH-97702)
d53fca6
Structure layout, and especially bitfields, sometimes resulted in clearly
wrong behaviour like overlapping fields. This fixes

Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>
@zware

zware commented Aug 5, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner AA-Turner mentioned this pull request Aug 24, 2024
GH-123299: Move ctypes What's New entry to 3.14 #123300
Merged
@encukou encukou mentioned this pull request Aug 26, 2024
gh-97588: Move ctypes struct/union layout logic to Python #123352
Merged
This was referenced Sep 10, 2024
bitfield layout wrong in ctypes #84039
Closed
Incorrect struct definition with bitfields #86098
Closed
@encukou encukou mentioned this pull request Jan 17, 2025
ctypes: bitfield lost data with union on linux platform #95496
Closed
@slozier slozier mentioned this pull request Mar 6, 2025
Implement struct bitfield layout rules according to GCC IronLanguages/ironpython3#1926
Merged
This was referenced May 13, 2025
Implement configurable bitfield allocation strategy #56737
Closed
c_uint32 bitfields break structures #68478
Closed
Structure field size/ofs __str__ wrong with large size fields #70045
Closed
Ctypes unions with bitfield members that do not share memory #83211
Closed
@matthiasgoergens matthiasgoergens mentioned this pull request Jul 30, 2026
gh-96821: Fix undefined behaviour in _ctypes/cfield.c #96925
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@encukou encukou encukou left review comments
@zooba zooba zooba left review comments
@gvanrossum gvanrossum gvanrossum left review comments
@markshannon markshannon Awaiting requested review from markshannon
@iritkatriel iritkatriel Awaiting requested review from iritkatriel
+1 more reviewer
@ab22ht ab22ht ab22ht approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@gpshead gpshead

@encukou encukou

@zooba zooba

Labels

topic-ctypes

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.

13 participants

@matthiasgoergens @gpshead @Fidget-Spinner @pitrou @furkanonder @arhadthedev @zooba @encukou @eirannejad @bedevere-bot @gvanrossum @zware @ab22ht
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.