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-128715: Expose ctypes.CField, with info attributes #128950

Merged
merged 35 commits into from
Mar 24, 2025

Conversation

encukou
Copy link
Member

@encukou encukou commented Jan 17, 2025

Expose the type of struct/union field descriptors (_ctypes._CField) as ctypes.CField. (This is mainly to make it easier to refer to it -- for typing and documentation. Creating CFields manually is not supported.)

Add attributes for easier introspection:

  • name & type, as defined in _fields_
  • byte_size & byte_offset, specify the chunk of memory to read/write
  • bit_size & bit_offset, which specify the shift & mask for bitfields
  • is_bitfield & is_anonymous booleans

The existing offset remains, as an alias for byte_offset.
The existing size is unchanged. Usually it is the same as byte_size, but for bitfields, it contains a bit-packed combination of bit_size & bit_offset.

Update the repr() of CField.

Use the same values internally as well. (Except bit_size, which might overflow Py_ssize_t for very large types. Instead, in C use bitfield_size, which is 0 for non-bitfields and thus serves as is_bitfield flag. Different name used clarity.)
Old names are removed from the C implementation to ensure a clean transition.
For simplicity, I keep byte_size in CFieldObject for now, even though it's redundant: it's the size of the underlying type.

Add a generic test that ensures the values are consistent, and that we don't have overlapping fields in structs. Use this check throughout test_ctypes, wherever a potentially interesting Structure or Union is created. (Tests for how simple structs behave after they're created don't need the checks, but I erred on the side of adding checks.)

Lift the restriction on maximum field size that was temporarily added in GH-126938. The max size is now Py_ssize_t.

This PR does not yet touch cfield.c getters & setters: the bit-packed “size” argument is computed and passed to those.


📚 Documentation preview 📚: https://cpython-previews--128950.org.readthedocs.build/

@encukou encukou marked this pull request as draft January 31, 2025 16:47
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2025
@encukou
Copy link
Member Author

encukou commented Feb 7, 2025

I'll continue next week :)

- Add alignment requirement
- Mention that ob_size is unreliable if you don't control it
- Add some links for context
- basicsize should include the base type in generaly not just PyObject

This adds a “by-the-way” link to `PyObject_New`, which shouldn't be
used for GC types. In order to be comfortable linking to it, I also
add a link to `PyObject_GC_New` from its docs. And the same for
`*Var` variants, while I'm here.
@picnixz
Copy link
Member

picnixz commented Feb 8, 2025

Should I review what you wrote now or do you prefer me to wait?

@encukou
Copy link
Member Author

encukou commented Feb 21, 2025

Sorry, I missed the comment! I'd appreciate a review.

The failure on 32-bit Debian is due to an existing bug, but such an exotic one that it's not worth fixing now: #130410
(It shows that the tests work, though!)

@encukou encukou marked this pull request as ready for review February 25, 2025 12:39
@encukou
Copy link
Member Author

encukou commented Mar 10, 2025

@picnixz, should I wait for your review?
No pressure, feel free to say no :)

@picnixz
Copy link
Member

picnixz commented Mar 10, 2025

Oh I forgot... I'll have maybe a bit of time before taking my plane or I'll be back on Wednesday. If you want to wait until then, I'll review it otherwise just go ahead!

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Some comments (hard to do a better review on a phone)

Copy link
Member Author

@encukou encukou left a 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!

@encukou
Copy link
Member Author

encukou commented Mar 20, 2025

Thanks for the reviews everyone!
I'll merge tomorrow if there are no objections.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I don't think I've much more to say. My main concern was the unicode-exactness of the name which is now resolved so I'm fine.

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

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

Results will be shown at:

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

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 Mar 24, 2025
@encukou encukou merged commit 0e53038 into python:main Mar 24, 2025
122 of 123 checks passed
@encukou encukou deleted the ctypes-bitfield-3 branch March 24, 2025 13:18
@encukou
Copy link
Member Author

encukou commented Mar 24, 2025

Almost tomorrow.... :)

Thank you for the reviews, everyone!

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

Successfully merging this pull request may close these issues.

4 participants