bpo-41073: PyType_GetSlot() can now accept static types. #21931

shihai1991 · 2020-08-20T17:23:17Z

PyType_GetSlot() can now accept static types.

Co-Authored-By: Petr Viktorin encukou@gmail.com

https://bugs.python.org/issue41073

Automerge-Triggered-By: GH:encukou

encukou

Can you also test getting Py_nb_add and Py_mp_length from PyLong_Type?
That will expose the reason this is complicated: static types don't have the sub-slots structs allocated as part of the type.

Include/typeslots.h

bedevere-bot · 2020-08-21T09:38:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

shihai1991 · 2020-08-21T14:41:41Z

Can you also test getting Py_nb_add and Py_mp_length from PyLong_Type?
That will expose the reason this is complicated: static types don't have the sub-slots structs allocated as part of the type.

sure. Let me try it.

shihai1991 · 2020-08-21T15:20:08Z

That will expose the reason this is complicated: static types don't have the sub-slots structs allocated as part of the typ

the offsetof in typeslots.inc have us the result.
Can we find a way to avoid calling the slots or subslots of heaptype or use another typeslots.inc something like statictypeslots.inc to keep the offsetof of statictype?

encukou · 2020-08-21T16:27:38Z

the offsetof in typeslots.inc have us the result.

Right. The offsetoff is done for PyHeapTypeObject – that is, only for heap types.

Static types, PyTypeObject, don't have fixed offsets for all the slots; they're separately declared structs. See for example NoneType, which only has the PyNumberMethods table (Py_nb_*), but none of the others (like PySequenceMethods, py_sq_*).

You could record which sub-slots table each slot belongs to, in a similar table as typeslots.h/typeslots.inc. Then, for non-heap types, first find the sub-slot table (like tp_as_number), if it's NULL then return NULL, and otherwise look inside it.

shihai1991 · 2020-08-23T08:55:21Z

You could record which sub-slots table each slot belongs to, in a similar table as typeslots.h/typeslots.inc. Then, for non-heap types, first find the sub-slot table (like tp_as_number), if it's NULL then return NULL, and otherwise look inside it.

Hi petr. Do we have developers to get sub-slots of static types by PyType_GetSlot() ? Return the slots is good enough?

shihai1991 · 2020-08-24T11:13:52Z

requested changes; please review again.

shihai1991 · 2020-08-27T16:06:16Z

@vstinner Hi, victor. Do you have some suggestions about this PR?

encukou · 2020-09-01T15:26:37Z

Sorry, I do not understand what the recent changes are supposed to do.
Again, can you also test getting Py_nb_add and Py_mp_length from PyLong_Type? It seems that the existing slots still can't be used with PyType_GetSlot safely.
I don't see how the new set of Py_type_* constants is helping solve bpo-41073. What happens if you use them with heap types?

shihai1991 · 2020-09-02T10:54:23Z

Sorry, I do not understand what the recent changes are supposed to do.
Again, can you also test getting Py_nb_add and Py_mp_length from PyLong_Type? It seems that the existing slots still can't be used with PyType_GetSlot safely.

Ok, copy that. I will try to add some tests to against this function.

I don't see how the new set of Py_type_* constants is helping solve bpo-41073. What happens if you use them with heap types?

This is a problem. Can we add a new function something like PyStaticType_GetSlot?

shihai1991 · 2020-09-04T19:13:27Z

@encukou Hi, petr. I updated this PR and add some testcases. Pls take a look again when you have free time.

…lots1 * origin/master: (63 commits) bpo-41627: Distinguish 32 and 64-bit user site packages on Windows (pythonGH-22098) bpo-38585: Remove references to defusedexpat (pythonGH-22095) bpo-41721: Add xlc options (pythonGH-22096) bpo-40486: Specify what happens if directory content change diring iteration (pythonGH-22025) bpo-41638: Improve ProgrammingError message for absent parameter. (pythonGH-21999) bpo-41713: _signal doesn't use multi-phase init (pythonGH-22087) bpo-41700: Skip test if the locale is not supported (pythonGH-22081) [doc] Update documentation on logging optimization. (pythonGH-22075) Fix 'gather' rules in the python parser generator (pythonGH-22021) bpo-41697: Correctly handle KeywordOrStarred when parsing arguments in the parser (pythonGH-22077) [doc] Fix a typo in the graphlib docs (python#22030) bpo-1635741: Port _signal module to multi-phase init (PEP 489) (pythonGH-22049) bpo-39883: Use BSD0 license for code in docs (pythonGH-17635) bpo-39010: Improve test shutdown (python#22066) bpo-41696: Fix handling of debug mode in asyncio.run (python#22069) bpo-41690: Use a loop to collect args in the parser instead of recursion (pythonGH-22053) closes bpo-41689: Preserve text signature from tp_doc in C heap type creation. (pythonGH-22058) Fix invalid escape sequences in the peg_highlight Sphinx extension (pythonGH-22047) bpo-41675: Modernize siginterrupt calls (pythonGH-22028) bpo-41685: Don't pin setuptools version anymore in Doc/Makefile (pythonGH-22062) ...

encukou · 2020-09-08T12:20:16Z

I think this PR is getting more and more complicated, without getting closer to solving the issue.
I recommend going back to the drawing board.

I think the issue can be solved like this:

For each slot, record two pieces of information:

Which sub-slots table it is in (for example, PyNumberMethods, PyMappingMethods, or PyTypeObject itself). This can be stored as number: offsetof(PyTypeObject, tp_as_number), offsetof(PyTypeObject, tp_as_mapping) etc. Or 0 for PyTypeObject itself.
Offset within that structure, e.g. offsetof(PyNumberMethods, nb_add).

The one-number entries currently in Objects/typeslots.inc would not be needed any more.

Given this info, PyType_GetSlot() would do two steps (for both static and heap types):

Find the pointer to the sub-slots table. If it is NULL, return NULL as the slot's value.
In the sub-slots table, find the actual slot, and return it.

That way, there would be no need for new constants for type slots, or a new function specific to static types.

shihai1991 · 2020-09-08T13:54:01Z

I think this PR is getting more and more complicated, without getting closer to solving the issue.
I recommend going back to the drawing board.

I think the issue can be solved like this:

For each slot, record two pieces of information:

Which sub-slots table it is in (for example, PyNumberMethods, PyMappingMethods, or PyTypeObject itself). This can be stored as number: offsetof(PyTypeObject, tp_as_number), offsetof(PyTypeObject, tp_as_mapping) etc. Or 0 for PyTypeObject itself.

Offset within that structure, e.g. offsetof(PyNumberMethods, nb_add).

The one-number entries currently in Objects/typeslots.inc would not be needed any more.

Given this info, PyType_GetSlot() would do two steps (for both static and heap types):

Find the pointer to the sub-slots table. If it is NULL, return NULL as the slot's value.

In the sub-slots table, find the actual slot, and return it.

That way, there would be no need for new constants for type slots, or a new function specific to static types.

Thanks for your blueprint, petr. Let me consider it again : )

C asserts are removed in non-debug builds.

This is how most of the table is formatted. Fix a few lines around the one added.

IMO, things are more easier to follow this way (combined with the comment added to pyslot_offsets).

This way the table will be kept in sync if a slot is removed (which shouldn't happen).

encukou · 2020-11-03T21:14:28Z

Hi,
I started doing some small nitpicks as changes directly in the code, but then turned out it was more edits than I anticipated. Nevertheless, since I had the changes ready, I pushed them to this PR. Let me know if you want them removed from your PR!
Or, if you're happy with them, I can merge.

shihai1991 · 2020-11-04T01:08:45Z

Hi,
I started doing some small nitpicks as changes directly in the code, but then turned out it was more edits than I anticipated. Nevertheless, since I had the changes ready, I pushed them to this PR. Let me know if you want them removed from your PR!
Or, if you're happy with them, I can merge.

Hi, petr. You are the co-author, so you can go ahead ;)

shihai1991 · 2020-11-07T13:54:26Z

Lib/test/test_capi.py

@@ -544,6 +544,10 @@ def test_pynumber_tobase(self):
        self.assertRaises(TypeError, pynumber_tobase, '123', 10)
        self.assertRaises(SystemError, pynumber_tobase, 123, 0)

+    def test_get_statictype_slots(self):
+        from _testcapi import test_get_statictype_slots


Why do we need add this testcase in python level? Test_testcapi will take care of it:)

Ah, you're right!

…lots1 * origin/master: (80 commits) bpo-42282: Fold constants inside named expressions (pythonGH-23190) bpo-41028: Doc: Move switchers to docsbuild-scripts. (pythonGH-20969) bpo-42133: update parts of the stdlib to fall back to `__spec__.loader` when `__loader__` is missing (python#22929) Remove outdated reference to pywin32 from platform module (pythonGH-22005) bpo-41832: PyType_FromModuleAndSpec() now accepts NULL tp_doc (pythonGH-23123) Minor grammar edits for the descriptor howto guide (GH-python#23175) bpo-42179: Doc/tutorial: Remove mention of __cause__ (pythonGH-23162) bpo-26389: Allow passing an exception object in the traceback module (pythonGH-22610) bpo-42260: PyConfig_Read() only parses argv once (pythonGH-23168) bpo-42260: Add _PyConfig_FromDict() (pythonGH-23167) bpo-41877 Check for asert, aseert, assrt in mocks (pythonGH-23165) [docs] fix wrongly named AsyncContextDecorator (pythonGH-23164) bpo-42262: Add Py_NewRef() and Py_XNewRef() (pythonGH-23152) bpo-42266: Handle monkey-patching descriptors in LOAD_ATTR cache (pythonGH-23157) bpo-40816 Add AsyncContextDecorator class (pythonGH-20516) bpo-42260: Add _PyInterpreterState_SetConfig() (pythonGH-23158) Disable peg generator tests when building with PGO (pythonGH-23141) bpo-1635741: _sqlite3 uses PyModule_AddObjectRef() (pythonGH-23148) bpo-1635741: Fix PyInit_pyexpat() error handling (pythonGH-22489) bpo-42260: Main init modify sys.flags in-place (pythonGH-23150) ...

bedevere-bot · 2020-11-10T14:48:38Z

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

miss-islington · 2020-11-10T20:53:46Z

@shihai1991: Status check is done, and it's a success ✅ .

encukou · 2020-11-10T20:54:28Z

And there it is! Thank you for the effort, Hai Shi!

shihai1991 · 2020-11-11T01:01:57Z

And there it is! Thank you for the effort, Hai Shi!

Thanks for your guide, petr. Learned much in this PR ;)

) PyType_GetSlot() can now accept static types. Co-Authored-By: Petr Viktorin <encukou@gmail.com> Automerge-Triggered-By: GH:encukou

shihai1991 added 5 commits August 21, 2020 01:18

PyType_GetSlot() could accept static types

25e6a1b

update docs' style

7fcf9eb

use typeslots not in limit apis

159fc09

add test cases to against PyType_GetSlot()

588a847

update typeslots.h

ec0348d

the-knights-who-say-ni added the CLA signed label Aug 20, 2020

bedevere-bot added the awaiting review label Aug 20, 2020

shihai1991 mentioned this pull request Aug 20, 2020

bpo-41073: PyType_GetSlot() could accept static types. #21395

Closed

encukou requested changes Aug 21, 2020

View reviewed changes

Include/typeslots.h Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting review labels Aug 21, 2020

Add generate statictypeslots feature

abda762

revert typeslots.inc to master base

6d94381

shihai1991 requested a review from encukou August 24, 2020 11:13

shihai1991 changed the title ~~bpo-41073: PyType_GetSlot() could accept static types.~~ [WIP] bpo-41073: PyType_GetSlot() could accept static types. Aug 26, 2020

update get static type slots

eb3907d

shihai1991 added 2 commits September 5, 2020 10:11

Fix compile error

fe9f858

apply petr's comment

c1844e3

shihai1991 requested a review from encukou November 2, 2020 01:04

encukou added 8 commits November 3, 2020 15:07

Call test_get_statictype_slots in the test suite

ccdd9b6

Raise exception on the invalid slot 0

9194565

Use if rather than assert in tests

708cd82

C asserts are removed in non-debug builds.

Add/adjust comments & news entries

b351b2c

Align method definitions in _testcapi

2df9180

This is how most of the table is formatted. Fix a few lines around the one added.

Rewrite the slot-setting code a bit

2b077e3

IMO, things are more easier to follow this way (combined with the comment added to pyslot_offsets).

Remove the loop in PyType_GetSlot; we already know the position

943174b

typeslots.py: Add back code writing null entries

2d8b57f

This way the table will be kept in sync if a slot is removed (which shouldn't happen).

shihai1991 commented Nov 7, 2020

View reviewed changes

shihai1991 and others added 2 commits November 7, 2020 22:10

Remove redundant test function

6a688d9

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2020

Merge master branch; regenerate typeslots.inc

fc2c06b

encukou approved these changes Nov 10, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 10, 2020

encukou changed the title ~~bpo-41073: PyType_GetSlot() could accept static types.~~ bpo-41073: PyType_GetSlot() can now accept static types. Nov 10, 2020

encukou added the 🤖 automerge label Nov 10, 2020

miss-islington merged commit a13b26c into python:master Nov 10, 2020

bedevere-bot removed the awaiting merge label Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-41073: PyType_GetSlot() can now accept static types. #21931

bpo-41073: PyType_GetSlot() can now accept static types. #21931

shihai1991 commented Aug 20, 2020 •

edited by miss-islington

Loading

encukou left a comment

bedevere-bot commented Aug 21, 2020

shihai1991 commented Aug 21, 2020

shihai1991 commented Aug 21, 2020 •

edited

Loading

encukou commented Aug 21, 2020

shihai1991 commented Aug 23, 2020

shihai1991 commented Aug 24, 2020

shihai1991 commented Aug 27, 2020

encukou commented Sep 1, 2020 •

edited by bedevere-bot

Loading

shihai1991 commented Sep 2, 2020 •

edited

Loading

shihai1991 commented Sep 4, 2020

encukou commented Sep 8, 2020

shihai1991 commented Sep 8, 2020

encukou commented Nov 3, 2020 •

edited

Loading

shihai1991 commented Nov 4, 2020 •

edited

Loading

shihai1991 Nov 7, 2020

encukou Nov 10, 2020

bedevere-bot commented Nov 10, 2020

miss-islington commented Nov 10, 2020

encukou commented Nov 10, 2020

shihai1991 commented Nov 11, 2020

bpo-41073: PyType_GetSlot() can now accept static types. #21931

bpo-41073: PyType_GetSlot() can now accept static types. #21931

Conversation

shihai1991 commented Aug 20, 2020 • edited by miss-islington Loading

encukou left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 21, 2020

shihai1991 commented Aug 21, 2020

shihai1991 commented Aug 21, 2020 • edited Loading

encukou commented Aug 21, 2020

shihai1991 commented Aug 23, 2020

shihai1991 commented Aug 24, 2020

shihai1991 commented Aug 27, 2020

encukou commented Sep 1, 2020 • edited by bedevere-bot Loading

shihai1991 commented Sep 2, 2020 • edited Loading

shihai1991 commented Sep 4, 2020

encukou commented Sep 8, 2020

shihai1991 commented Sep 8, 2020

encukou commented Nov 3, 2020 • edited Loading

shihai1991 commented Nov 4, 2020 • edited Loading

shihai1991 Nov 7, 2020

Choose a reason for hiding this comment

encukou Nov 10, 2020

Choose a reason for hiding this comment

bedevere-bot commented Nov 10, 2020

miss-islington commented Nov 10, 2020

encukou commented Nov 10, 2020

shihai1991 commented Nov 11, 2020

shihai1991 commented Aug 20, 2020 •

edited by miss-islington

Loading

shihai1991 commented Aug 21, 2020 •

edited

Loading

encukou commented Sep 1, 2020 •

edited by bedevere-bot

Loading

shihai1991 commented Sep 2, 2020 •

edited

Loading

encukou commented Nov 3, 2020 •

edited

Loading

shihai1991 commented Nov 4, 2020 •

edited

Loading