8000 bpo-41073: PyType_GetSlot() can now accept static types. by shihai1991 · Pull Request #21931 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Nov 10, 2020
Merged

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

merged 27 commits into from
Nov 10, 2020

Conversation

shihai1991
Copy link
Member
@shihai1991 shihai1991 commented Aug 20, 2020 •
edited by miss-islington
Loading

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

@shihai1991 shihai1991 mentioned this pull request Aug 20, 2020
Closed
encukou
encukou requested changes Aug 21, 2020
View reviewed changes
Copy link
Member
@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.

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.

@bedevere-bot
Copy link

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
Copy link
Member Author

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
Copy link
Member Author
shihai1991 commented Aug 21, 2020 •
edited
Loading

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
Copy link
Member
encukou commented Aug 21, 2020

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
Copy link
Member Author

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 shihai1991 requested a review from encukou August 24, 2020 11:13
@shihai1991
Copy link
Member Author

requested changes; please review again.

@shihai1991 shihai1991 changed the title bpo-41073: PyType_GetSlot() could accept static types. [WIP] bpo-41073: PyType_GetSlot() could accept static types. Aug 26, 2020
@shihai1991
Copy link
Member Author

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

@encukou
Copy link
Member
encukou commented Sep 1, 2020 •
edited by bedevere-bot
Loading

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
Copy link
Member Author
shihai1991 commented Sep 2, 2020 •
edited
Loading

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
Copy link
Member Author

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

@shihai1991
Merge remote-tracking branch 'origin/master' into bpo_41073_add_types…
7537c86 
…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)
  ...
@shihai1991
Fix compile error
fe9f858
@encukou
Copy link
Member
encukou commented Sep 8, 2020

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
Copy link
Member Author

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 : )

8000
@shihai1991 shihai1991 requested a review from encukou November 2, 2020 01:04
@encukou
Call test_get_statictype_slots in the test suite
ccdd9b6
@encukou
Raise exception on the invalid slot 0
9194565
@encukou
Use if rather than assert in tests
708cd82 
C asserts are removed in non-debug builds.
@encukou
Add/adjust comments & news entries
b351b2c
@encukou
Align method definitions in _testcapi
2df9180 
This is how most of the table is formatted.
Fix a few lines around the one added.
@encukou
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).
@encukou
Remove the loop in PyType_GetSlot; we already know the position
943174b
@encukou
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).
@encukou
Copy link
Member
encukou commented Nov 3, 2020 •
edited
Loading

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
Copy link
Member Author
shihai1991 commented Nov 4, 2020 •
edited
Loading

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
shihai1991 commented Nov 7, 2020
View reviewed changes
Lib/test/test_capi.py Outdated
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right!

shihai1991 and others added 2 commits November 7, 2020 22:10
@shihai1991
Merge remote-tracking branch 'origin/master' into bpo_41073_add_types…
2d72953 
…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)
  ...
@encukou
Remove redundant test function
6a688d9
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2020
@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 10, 2020
@encukou encukou changed the title bpo-41073: PyType_GetSlot() could accept static types. bpo-41073: PyType_GetSlot() can now accept static types. Nov 10, 2020
@miss-islington
Copy link
Contributor

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

@miss-islington miss-islington merged commit a13b26c into python:master Nov 10, 2020
@encukou
Copy link
Member
encukou commented Nov 10, 2020

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

@shihai1991
Copy link
Member Author

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

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

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@shihai1991 @adorilson
bpo-41073: PyType_GetSlot() can now accept static types. (pythonGH-21931
2565903 
)

PyType_GetSlot() can now accept static types.

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

Automerge-Triggered-By: GH:encukou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou approved these changes

@markshannon markshannon Awaiting requested review from markshannon

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

Successfully merging this pull request may close these issues.
5 participants
@shihai1991 @bedevere-bot @encukou @miss-islington @the-knights-who-say-ni
0