GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-92678: Fix MRO calculation for C extension types with manual __dict__ by Fidget-Spinner · Pull Request #95242 · python/cpython

Conversation

Fidget-Spinner commented Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

We need to account for C extension types with don't set Py_TPFLAGS_MANAGED_DICT and manually manage their own dict instead.

This may be unsightly, but this special case is special enough to break the rules. Unless we get a proper class creation and dict management API in 3.12, we will break a lot of code.

Issue: MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix MRO calculation for types with manual __dict__

97e36b5

Fidget-Spinner requested a review from pablogsal

July 25, 2022 12:10

Fidget-Spinner requested a review from markshannon as a code owner

July 25, 2022 12:10

bedevere-bot added the awaiting core review label

Jul 25, 2022

Fidget-Spinner changed the title ~~Fix MRO calculation for C extension types with manual __dict__~~ gh-92678: Fix MRO calculation for C extension types with manual __dict__

Jul 25, 2022

pablogsal commented Jul 25, 2022

Copy link

Copy Markdown

Member

What issue does this corresponds to?😅 I need a bit of context here to review it 😉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner commented

Jul 25, 2022

View reviewed changes

Comment thread

Objects/typeobject.c

Comment on lines +2244 to +2249

		if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&
		type->tp_dictoffset && base->tp_dictoffset == 0 &&
		type->tp_dictoffset + sizeof(PyObject *) == t_size &&
		type->tp_flags & Py_TPFLAGS_HEAPTYPE) {
		t_size -= sizeof(PyObject *);
		}

Fidget-Spinner Jul 25, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This reintroduces the old code pre-3.11, but guards it with a check for not Py_TPFLAGS_MANAGED_DICT.

8319114#diff-1decebeef15f4e0b0ce106c665751ec55068d4d1d1825847925ad4f528b5b872L2254

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal Jul 25, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The condition is a bit arcane. Could you capture it in a variable or use a macro or inline function so we can name what we are checking for? Currently is a bit difficult to decipher what happens when all of these are true or false

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fidget-Spinner Jul 25, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So funny thing is I also found the old code arcane and couldn't really understand it well. From my rough guesstimation

    // The __dict__ isn't automatically managed by CPython.
    if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&
        // That a dictoffset is set. Meaning it's a manual __dict__.
        // And the base class doesn't have a __dict__
        type->tp_dictoffset && base->tp_dictoffset == 0 &&
        // Huh?
        type->tp_dictoffset + sizeof(PyObject *) == t_size &&
        // Not a static type.
        type->tp_flags & Py_TPFLAGS_HEAPTYPE) {
        t_size -= sizeof(PyObject *);
    }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fidget-Spinner Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this is a really cursed example of Hyrum's law.

The extra_ivars code assumes that __weakreflist__ comes first then __dict__. In mypyc it has to follow this order to work pre-3.11. Now in 3.11 when we remove that assumption, all the C extensions using this with complicated MROs break 🤦 .

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 and hauntsaninja reacted with eyes emoji

All reactions

👀 2 reactions

Fidget-Spinner commented Jul 25, 2022

Copy link

Copy Markdown

Member Author

What issue does this corresponds to?😅 I need a bit of context here to review it 😉

Sorry. gh-92678

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner mentioned this pull request

Jul 25, 2022

MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678

Closed

markshannon reviewed

Jul 25, 2022

View reviewed changes

Comment thread

Objects/typeobject.c

		type->tp_weaklistoffset + sizeof(PyObject *) == t_size &&
		type->tp_flags & Py_TPFLAGS_HEAPTYPE)
		t_size -= sizeof(PyObject *);
		if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&

markshannon Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is is correct to check Py_TPFLAGS_MANAGED_DICT during class creation?
It is set during class creation: https://github.com/python/cpython/blob/main/Objects/typeobject.c#L2989

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fidget-Spinner Jul 25, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think setting the flag happens before the MRO check. https://github.com/python/cpython/blob/main/Objects/typeobject.c#L3174-L3186
(type_new_descriptors then PyType_Ready ).
(PyType_Ready -> type_ready -> type_ready_mro)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Aug 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If you factor out the check into a macro, the code would be a bit clearer.

#define FIELD_AT_OFFSET(name, offset) type->tp_ ## name && base->tp_## name == 0 ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Fidget-Spinner closed this

Jul 25, 2022

Fidget-Spinner deleted the fix_mro_conflicts branch

July 29, 2022 08:30

Fidget-Spinner restored the fix_mro_conflicts branch

August 3, 2022 09:52

Fidget-Spinner reopened this

Aug 3, 2022

Fidget-Spinner commented Aug 3, 2022

Copy link

Copy Markdown

Member Author

Reopened because this is an alternative fix.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon reviewed

Aug 3, 2022

View reviewed changes

markshannon left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks like it should fix the regression in #95589

Could you add some tests please?
You'll need to two classes each for tp_dictoffset and tp_weaklistoffset to the `_testcapi module then test for multiple inheritance of all pairs in test_capi.py

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/typeobject.c

		type->tp_weaklistoffset + sizeof(PyObject *) == t_size &&
		type->tp_flags & Py_TPFLAGS_HEAPTYPE)
		t_size -= sizeof(PyObject *);
		if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&

markshannon Aug 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If you factor out the check into a macro, the code would be a bit clearer.

#define FIELD_AT_OFFSET(name, offset) type->tp_ ## name && base->tp_## name == 0 ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/typeobject.c

		type->tp_weaklistoffset + sizeof(PyObject *) == t_size &&
		type->tp_flags & Py_TPFLAGS_HEAPTYPE)
		t_size -= sizeof(PyObject *);
		if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&

markshannon Aug 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We need to check for tp_dictoffset and tp_weaklistoffset, and shouldn't presume the order.
So we need to check for weakrefs, then dict, then weakrefs again (or vice versa).

if (FIELD_AT_OFFSET(dictoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}
if (FIELD_AT_OFFSET(weaklistoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}
if (FIELD_AT_OFFSET(dictoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon mentioned this pull request

Aug 3, 2022

Flawed assumptions about tp_dictoffset in inheritance. #95589

Closed

markshannon commented Aug 3, 2022

Copy link

Copy Markdown

Member

@Fidget-Spinner
As the RC is on Friday, I've implemented the suggested changes as #95596
Apologies for treading on your toes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner closed this

Aug 3, 2022

Reviewers

markshannon

markshannon left review comments

pablogsal

Awaiting requested review from pablogsal

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Fidget-Spinner commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jul 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented Jul 25, 2022

Uh oh!

markshannon Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented Aug 3, 2022

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Aug 3, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fidget-Spinner commented Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

markshannon Jul 25, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.