gh-149239: Deopt LOAD_ATTR_CLASS_WITH_METACLASS_CHECK on reassigning __class__#149269
gh-149239: Deopt LOAD_ATTR_CLASS_WITH_METACLASS_CHECK on reassigning __class__#149269sobolevn wants to merge 5 commits intopython:mainfrom
LOAD_ATTR_CLASS_WITH_METACLASS_CHECK on reassigning __class__#149269Conversation
…sining `__class__`
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
LG, just one test nit.
Documentation build overview
38 files changed ·
|
There was a problem hiding this comment.
Note: this file is still there for some reason 🤔
Should it be ignored in #149311 ?
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
LGTM. Let's wait for @markshannon though.
| op(_LOAD_ATTR_CLASS, (descr/4, owner -- attr)) { | ||
| PyTypeObject *descr_type = Py_TYPE(descr); | ||
| PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); | ||
| EXIT_IF((descr_type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0 |
There was a problem hiding this comment.
Why are we not exiting if (descr_type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) != 0?
If you made Descriptor immutable in the test above, this guard wouldn't trigger and the test would fail
There was a problem hiding this comment.
The enum specialization supports mutable types
There was a problem hiding this comment.
Should not EXIT_IF(descr_type != (PyTypeObject *)owner_o); be enough? I applied this change.
There was a problem hiding this comment.
With this change test_call_len_known_length starts to fail :/
I will revert it.
There was a problem hiding this comment.
You need to add a test case for changing the class of an object to an immutable descriptor class.
There was a problem hiding this comment.
So, here's the logic:
It is checked that type is mutable here: specialize_class_load_attr in Python/specialize.c
bool metaclass_check = false;
if ((Py_TYPE(cls)->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0) {
metaclass_check = true;
}So, we can rely on this check in _LOAD_ATTR_CLASS which is shared between LOAD_ATTR_CLASS and LOAD_ATTR_CLASS_WITH_METACLASS_CHECK. The separation is based on (Py_TYPE(cls)->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0
There was a problem hiding this comment.
That checks that the class the object is changed from is immutable, but not whether the class it is changed to is immutable.
markshannon
changed the title
LOAD_ATTR_CLASS_WITH_METACLASS_CHECK on reassining __class__LOAD_ATTR_CLASS_WITH_METACLASS_CHECK on reassigning __class__
|
Turns out, there's another problem: when I moved tests from I need a bit of help here, @Fidget-Spinner :) |
|
Hi @sobolevn, the bug only fires when the same specialised instruction is re-executed after |
4 participants
Uh oh!
There was an error while loading. Please reload this page.