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: Move ctypes struct/union layout logic to Python by encukou · Pull Request #123352 · python/cpython

Conversation

@encukou

@encukou encukou commented Aug 26, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

As discussed in #97702, moving this logic to Python should make it easier to fix the various struct layout bugs in ctypes.

This PR should be strictly a refactoring of the code in main, except the _ctypes.CField class can now be instantiated from Python.

  • Issue: ctypes: bit field data does not survive round trip #97588

Sorry, something went wrong.

Uh oh!

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

All reactions
encukou added 30 commits August 26, 2024 16:14
@encukou
Add classes that will implement the struct packing
ac1d0a3
@encukou
Instantiate the layout and take _align_ from it
b35a97c
@encukou
Get base size and align; etc.
2cc8b40
@encukou
Sorta start creating CField objects
a9fd94a
@encukou
Turn constructor into initializer
c4d3c66
@encukou
Use goto for error handling
cbb3081
@encukou
Handle Py_DECREF(prop) in the error block
1adb23a
@encukou
Pass bit_size and name to CField
01ee4f8
@encukou
Use name from prop
eb7666c
@encukou
Use desc from prop
be6113f
@encukou
Use bit_size from prop; avoid pair
a7f05f9
@encukou
Pass None for undefined bit_size, not -1
ef24a0c
@encukou
Fix a refleak
728ec0a
@encukou
Move bitsize check to CField initializer
5ef81b3
@encukou
desc is proto
16c2d77
@encukou
Remove the unused TYPEFLAG_HASBITFIELD and TYPEFLAG_HASUNION
14fd92d
@encukou
Use -1 for "not is_bitfield"
e3337c2
@encukou
Handle _swappedbytes_ in Python
f3c678d
@encukou
Put layout mode in the field
0c47fa2
@encukou
error handling
2eefab1
@encukou
Take out is_bitfield arg
5b1723c
@encukou
Keep _pack_ on the field
9507752
@encukou
Stick the index in the struct
dc859c3
@encukou
Add packstate struct, with pfield_size
beeab0b
@encukou
Put pbitoffs in the state
accb589
@encukou
Put the size in the state
71fbfb8
@encukou
Add offset to the state
9c6e3b0
@encukou
Put align in the state
a6f8fdc
@encukou
Move getfunc/setfunc to __new__
6803815
@encukou
Start verifying the internal pack state
e0fc0af
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 27, 2024
@picnixz
picnixz self-requested a review August 27, 2024 15:13
picnixz
picnixz reviewed Aug 27, 2024
View reviewed changes

@picnixz picnixz left a comment •
edited
Loading

Uh oh!

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

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.

First round of comments on the C side. I'll do the Python files now.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

@picnixz
picnixz self-requested a review August 27, 2024 15:50
picnixz
picnixz reviewed Aug 27, 2024
View reviewed changes

@picnixz picnixz 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.

The way you construct the layout depends on whether it's a union or a struct and whether it's gcc or ms layout. I'm not sure whether it's easier to maintain one huge function or if you want to create separate factories. AFACIT, it could be easier for instance to have a function that determines whether it's a gcc layout or not, one function that is responsible for parsing _align_ and one function responsible for the _pack_ attribute. For the rest, they can be kept inside the get_layout function.

For the loop over the input fields, I don't know whether you want to keep a single one or factor it out (it could be clearer to review, but it will likely cause more code so I'm fine with the current implementation).

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_ctypes/test_bitfields.py Resolved

Uh oh!

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

Comment thread Lib/test/test_ctypes/test_bitfields.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

encukou and others added 6 commits August 28, 2024 15:56
@encukou @picnixz
Apply suggestions from code review
ff4bcf5
Co-authored-by: Bénédikt Tran <[email protected]>
@encukou
Move bit_size checks to assertions; they're checked in Python
6b9a8b7
@encukou
Move i declaration to for loops
991bab3
@encukou
Remove a clearly unneeded assert
298c822
@encukou
Add rudimentary test for bitfields in Union
3088e5d
@encukou
Improve error message for _field_ element unpacking
511e25d
encukou
encukou commented Aug 28, 2024
View reviewed changes

@encukou encukou left a comment

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.

Thank you for the review!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/stgdict.c Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

@picnixz
picnixz self-requested a review August 28, 2024 15:32
@encukou
Use correct length modifier
0689e59
picnixz
picnixz reviewed Aug 28, 2024
View reviewed changes
Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Lib/ctypes/_layout.py Resolved

Uh oh!

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

Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Modules/_ctypes/cfield.c Resolved

Uh oh!

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

Comment thread Lib/test/test_ctypes/test_bitfields.py Resolved

Uh oh!

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

encukou and others added 5 commits August 28, 2024 18:00
@encukou
Avoid a case where MSC and GCC's __attribute__((ms_struct)) disagree
a7c3724
@encukou
Adjust Windows assertions
bd53c28
@encukou @picnixz
Apply suggestions from code review
b387d30
Co-authored-by: Bénédikt Tran <[email protected]>
@encukou
Remove unused last_field
9cf18cd
@encukou
Remove the bitfield-in-union test, for now
96a5c0d
@encukou

encukou commented Sep 2, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Thank you for the reviews!
I plan to merge after 3.13 rc2 is out, and then start solving the layout bugs and missing tests :)

picnixz 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 Sep 4, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

RC2 is delayed. I'll merge now, if buildbots pass.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 4, 2024
@bedevere-bot

bedevere-bot commented Sep 4, 2024

Copy link
Copy Markdown

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

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 Sep 4, 2024
@picnixz

picnixz commented Sep 4, 2024

Copy link
Copy Markdown
Member

RC2 is delayed. I'll merge now, if buildbots pass.

The "now" being in at least 8 hours 😆

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.

@encukou
encukou merged commit ce9f84a into python:main Sep 5, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Sep 5, 2024
@encukou
encukou deleted the ctypes-pypack branch September 5, 2024 09:20
@brianschubert brianschubert mentioned this pull request Nov 17, 2024
Struct fields with size > 65535 bytes throw TypeError #126937
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

3 participants

@encukou @bedevere-bot @picnixz
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 01402520 — 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.