Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-103092: Port some_ctypes data types to heap types#113630

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
neonene wants to merge20 commits intopython:mainfromneonene:ctypes_simple
Closed

gh-103092: Port some_ctypes data types to heap types#113630

neonene wants to merge20 commits intopython:mainfromneonene:ctypes_simple

Conversation

neonene
Copy link
Contributor

@neoneneneonene commentedJan 1, 2024
edited
Loading

Currently,PyType_From* functions refuse or ignore creating classes whose metaclass overridestp_new (#103968):

>>> import ctypesTraceback (most recent call last):  File "<stdin>", line 1, in <module>    import ctypes  File "C:\cp\Lib\ctypes\__init__.py", line 8, in <module>    from _ctypes import Union, Structure, ArrayTypeError: Metaclasses with custom tp_new are not supported.

The ctypes extension needs some workaround for that.

UPDATE: AddedTypeError message above.

@neonene
Copy link
ContributorAuthor

@vstinner Could you review this behavior change?

@neoneneneonene changed the titlegh-103092: Make Simple_Type in ctypes into heap typesgh-103092: Port some_ctypes data types to heap typesJan 3, 2024
@neonene
Copy link
ContributorAuthor

This is an attempt without hacking type slots.

Problem: It is possible that accessing metaclasses cause a crash with bad arguments (e.g.type(c_int).__init__(...)), which this PR does not tackle.

cc PEP 687 experts:@erlend-aasland@kumaraditya303

@neonene
Copy link
ContributorAuthor

neonene commentedJan 6, 2024
edited
Loading

Leaving an alternative PR (#113774) which does not have the problem above. However,tp_new slot is hacked. Is there any better way to isolate_ctypes?

cc@encukou

@encukou
Copy link
Member

To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the*_Type pointers toctypes_state, but initializes them pointers to the existing static types for now? Also change all the*_Check calls -- those should look the same regardless of how the metaclass issue is solved. The next PR can then focus only on the problematic case.

IMO, the correct way with thecurrent heap type creation API is to move the initialization totp_init as you do, but ensure thattp_init was called exactly once: that is, all operations should fail cleanly if it wasnot run on a given type, and it should fail (or do nothing) if called a second time.

Another option is to improve the type creation API -- e.g. design a slot that works like__subclass_init__ but CPython guarantees to call it exactly once.

@vstinner
Copy link
Member

@vstinner Could you review this behavior change?

I would prefer a smaller PR. You moved 7 types. Can you start with a PR which change less types and less files at once?

@neonene
Copy link
ContributorAuthor

neonene commentedJan 10, 2024
edited
Loading

@encukou Is it worth opening a new issue for that? Maybe a custom metatp_init can be checked without switching to a heap type?

@encukou
Copy link
Member

Oh, I think it's worth writing a PEP for that :)
If you want to help there, a good first step would be a proof-of-concept PR (without an issue).

@neonene
Copy link
ContributorAuthor

neonene commentedJan 10, 2024
edited
Loading

Sorry, I'm not qualified, as I'm volunteering anonymously.
A fix for issues around#113055 without porting_ctypes is also fine with me.

@neoneneneonene marked this pull request as draftJanuary 19, 2024 01:46
@vstinner
Copy link
Member

Oh, I think it's worth writing a PEP for that :)

Why do you think that a PEP is needed for this specific change? Many static types were converted to heap types in the stdlib without a specific PEP.

@erlend-aasland
Copy link
Contributor

Oh, I think it's worth writing a PEP for that :)

Why do you think that a PEP is needed for this specific change? Many static types were converted to heap types in the stdlib without a specific PEP.

Ithink@encukou meant a PEP for this proposed idea in#113630 (comment) :)

Another option is to improve the type creation API -- e.g. design a slot that works likesubclass_init but CPython guarantees to call it exactly once.

vstinner reacted with thumbs up emoji

@vstinner
Copy link
Member

How are ctypes types than other stdlib extensions types? Is there a metaclass? Why do they need special code for__init__()? I don't understand and the PR doesn't say much about it.

@erlend-aasland
Copy link
Contributor

How are ctypes types than other stdlib extensions types? Is there a metaclass? Why do they need special code for__init__()? I don't understand and the PR doesn't say much about it.

_ctypes is not straight-forward to adapt, contrary to pretty much the rest of the extension modules in the stdlib. There is a lot of metaclass hacks.

@vstinner
Copy link
Member

Ah,PyCStructType_Type is special: it's a "meta type/class" says a comment. If it's special, I suggest to create a dedicated issue to collect more details about this specific type and how it's used. Currently, the PR is linked to a generic "Isolate Stdlib Extension Modules" issue.

pycstruct_type_slots defines thePy_tp_new slot asPyCStructType_new(). This function callsStructUnionType_new() which does the black magic:

/*  PyCStructType_Type - a meta type/class.  Creating a new class using this one as  __metaclass__ will call the constructor StructUnionType_new/init.  It replaces  the tp_dict member with a new instance of StgDict, and initializes the C  accessible fields somehow.*/

Extracts:

    /* create the new instance (which is a class,       since we are a metatype!) */

and:

    /* replace the class dict by our updated stgdict, which holds info       about storage requirements of the instances */

@erlend-aasland
Copy link
Contributor

+1 for a dedicated issue; there is a lot of issues connected to the isolation of_ctypes.

@vstinner
Copy link
Member

I created the issuegh-114314 "Convert _ctypes PyCStgDict meta static type to a meta heap type".

erlend-aasland reacted with thumbs up emoji

@vstinner
Copy link
Member

To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the *_Type pointers to ctypes_state, but initializes them pointers to the existing static types for now? Also change all the *_Check calls

I wrote PR#114316 for that. It's partially based on this PR.

@neonene
Copy link
ContributorAuthor

Unlike this PR, the issue#114314 seems to consider how to refactor the hacks around PyCStgDict, without customizingtp_new/init. If so, that also sounds reasonable to me.

Any way is fine with me to avoid the error related to#103968:

>>> import ctypesTraceback (most recent call last):  File "<stdin>", line 1, in <module>    import ctypes  File "C:\cp\Lib\ctypes\__init__.py", line 8, in <module>    from _ctypes import Union, Structure, ArrayTypeError: Metaclasses with custom tp_new are not supported.

@neonene

This comment was marked as outdated.

@neonene
Copy link
ContributorAuthor

Closing the draft in favor of#116458.

@neoneneneonene deleted the ctypes_simple branchMarch 13, 2024 18:23
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrently

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@neonene@encukou@vstinner@erlend-aasland

[8]ページ先頭

©2009-2025 Movatter.jp