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-103509: PEP 697 -- Limited C API for Extending Opaque Types #103511

Merged
merged 29 commits into from
May 4, 2023

Conversation

encukou
Copy link
Member

@encukou encukou commented Apr 13, 2023

PEP 697 describes the change. Some details it doesn't cover:

  • _PyHeapType_GET_MEMBERS is moved from Include/internal to the only .c file that uses it.
  • Tests demonstrate the API is usable for the purpose, and verify the layout with brutish-force.

TODO:

  • What's New entry
  • Fix the alignof & max_align_t

@encukou encukou marked this pull request as draft April 13, 2023 15:01
@erlend-aasland erlend-aasland self-requested a review April 16, 2023 21:25
@encukou
Copy link
Member Author

encukou commented Apr 17, 2023

typeobject.c includes <stddef.h> and <stdalign.h>, which should define max_align_t and alignof in C11. Yet, the Windows build complains:

D:\a\cpython\cpython\Objects\typeobject.c(3558,34): warning C4013: 'alignof' undefined; assuming extern returning int [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]
D:\a\cpython\cpython\Objects\typeobject.c(3558,53): error C2065: 'max_align_t': undeclared identifier [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]

What am I missing?
@zooba, could you help?

(The reference I linked to lists alignof as removed in C23, which is a bit misleading. The macro will be removed because it will become a keyword.)

@zooba
Copy link
Member

zooba commented Apr 17, 2023

Looks like you need stdalign.h for alignof (defines the alignof macro to _Alignof), and max_align_t only seems to exist for C++ via cstddef, where it's mapped to double.

@encukou
Copy link
Member Author

encukou commented Apr 19, 2023

That means I can't use max_align_t in CPython? Well, I can work around it.
But I was under the impression that MSVC did support C11. I can start the discussion to add max_align_t (and possibly alignof?) as exceptions to PEP 7. Do you know of any other C11 required features MSVC (or our configuration of it) doesn't support?

@encukou encukou marked this pull request as ready for review April 20, 2023 08:52
@encukou encukou requested a review from corona10 as a code owner April 20, 2023 08:52
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 20, 2023
@bedevere-bot
Copy link

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

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 Apr 20, 2023
@encukou encukou marked this pull request as draft April 20, 2023 09:41
@arhadthedev
Copy link
Member

The TraceRefs buildbot failure:

Modules/gcmodule.c:450: visit_decref: Assertion "!_PyObject_IsFreed(op)" failed
Enable tracemalloc to get the memory block allocation traceback

is likely caused by gh-103621.

Copy link
Member

@arhadthedev arhadthedev left a comment

Choose a reason for hiding this comment

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

void* cannot participate in address arithmetics. You need to cast both data_ptr and instance to char * to get distance in bytes.

Modules/_testcapi/heaptype_relative.c Outdated Show resolved Hide resolved
Modules/_testcapi/heaptype_relative.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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

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 Apr 27, 2023
Smaller objects might not be aligned to ALIGNOF_MAX_ALIGN_T.
The offsets calculated for PEP 697 should be aligned, though.
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 28, 2023
@bedevere-bot
Copy link

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

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 Apr 28, 2023
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Very nice!

Sorry for the late review; it took some time to dig through everything.

Doc/c-api/object.rst Outdated Show resolved Hide resolved
Doc/c-api/type.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
PC/python3dll.c Outdated Show resolved Hide resolved
Python/structmember.c Outdated Show resolved Hide resolved
Objects/typeobject.c Show resolved Hide resolved
Objects/typeobject.c Show resolved Hide resolved
@encukou
Copy link
Member Author

encukou commented May 2, 2023

Thank you for the thorough review!
This should address the remaining comments.

@encukou encukou requested a review from arhadthedev May 2, 2023 15:20
@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 2, 2023

Thanks! I've got some more comments, but we can deal with those in a follow-up PR if you think it's worth it. Let's land this before the feature freeze kicks in.

(I'm on mobile now; I won't be able to check the changes until tomorrow.)

Copy link
Member

@arhadthedev arhadthedev left a comment

Choose a reason for hiding this comment

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

Please, keep in mind that I neither participated in the PR discussion nor seen the PEP before. So I looked at the rendered docs in the same fashion as an outside user would do.

Here are three optional rough proposals and a forgotten full stop. However, they are non-binding suggestions that can be properly considered later, after 3.12b1 is out (otherwise the two remaining weeks will pass swiftly in the bikeshedding).

I've limited myself to the documentation because I have no expertise in the C API to check such a volume of code in such a short period of time.

Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/object.rst Outdated Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/type.rst Outdated Show resolved Hide resolved
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
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! Fresh eyes is exactly what the documentation needs.

It will also need a tutorial to bring the concepts together -- but when trying to write a tutorial, I keep finding rough edges that should be fixed first :)

Doc/c-api/object.rst Show resolved Hide resolved
Doc/c-api/object.rst Show resolved Hide resolved
@encukou
Copy link
Member Author

encukou commented May 4, 2023

Thanks for the reviews!

I'll merge, and leave the issues open for other PRs.

@encukou encukou merged commit cd9a56c into python:main May 4, 2023
@encukou encukou deleted the extend-opaque-sq branch May 4, 2023 07:56
@erlend-aasland
Copy link
Contributor

Thanks for the reviews!

I'll merge, and leave the issues open for other PRs.

Good job! This is a very nice feature.

@encukou
Copy link
Member Author

encukou commented May 4, 2023

Thank you!

FWIW, here's a follow-up on the alignof(max_align_t): https://discuss.python.org/t/requiring-compilers-c11-standard-mode-to-build-cpython/26481

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.

5 participants