From b61fece8523d0fa6d9cc6ad3fd855a136c34f0cd Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Thu, 24 Oct 2024 11:57:02 +0100 Subject: [PATCH] GH-125868: Fix STORE_ATTR_WITH_HINT specialization (GH-125876) --- Lib/dis.py | 4 +- Lib/test/test_opcache.py | 44 +++++++++++++++++++ ...-10-23-14-05-47.gh-issue-125868.uLfXYB.rst | 3 ++ Python/bytecodes.c | 7 ++- Python/executor_cases.c.h | 10 +++-- Python/generated_cases.c.h | 7 ++- 6 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-05-47.gh-issue-125868.uLfXYB.rst diff --git a/Lib/dis.py b/Lib/dis.py index e87e6a78469ab0..db69848e9ab8ee 100644 --- a/Lib/dis.py +++ b/Lib/dis.py @@ -778,8 +778,10 @@ def _get_instructions_bytes(code, linestarts=None, line_offset=0, co_positions=N if caches: cache_info = [] + cache_offset = offset for name, size in _cache_format[opname[deop]].items(): - data = code[offset + 2: offset + 2 + 2 * size] + data = code[cache_offset + 2: cache_offset + 2 + 2 * size] + cache_offset += size * 2 cache_info.append((name, size, data)) else: cache_info = None diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index acf8158b0d0ea1..cdcddb0d717f23 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1155,6 +1155,50 @@ class D(dict): pass {'a':1, 'b':2} ) + def test_125868(self): + + def make_special_dict(): + """Create a dictionary an object with a this table: + index | key | value + ----- | --- | ----- + 0 | 'b' | 'value' + 1 | 'b' | NULL + """ + class A: + pass + a = A() + a.a = 1 + a.b = 2 + d = a.__dict__.copy() + del d['a'] + del d['b'] + d['b'] = "value" + return d + + class NoInlineAorB: + pass + for i in range(ord('c'), ord('z')): + setattr(NoInlineAorB(), chr(i), i) + + c = NoInlineAorB() + c.a = 0 + c.b = 1 + self.assertFalse(_testinternalcapi.has_inline_values(c)) + + def f(o, n): + for i in range(n): + o.b = i + # Prime f to store to dict slot 1 + f(c, 100) + + test_obj = NoInlineAorB() + test_obj.__dict__ = make_special_dict() + self.assertEqual(test_obj.b, "value") + + #This should set x.b = 0 + f(test_obj, 1) + self.assertEqual(test_obj.b, 0) + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-05-47.gh-issue-125868.uLfXYB.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-05-47.gh-issue-125868.uLfXYB.rst new file mode 100644 index 00000000000000..dea250e7166ec6 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-05-47.gh-issue-125868.uLfXYB.rst @@ -0,0 +1,3 @@ +It was possible in 3.14.0a1 only for attribute lookup to give the wrong +value. This was due to an incorrect specialization in very specific +circumstances. This is fixed in 3.14.0a2. diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 62e9b5ddd1584c..eaf2537fa07d27 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2303,17 +2303,16 @@ dummy_func( assert(PyDict_CheckExact((PyObject *)dict)); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg); DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries); - PyObject *old_value; DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys)); PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; DEOPT_IF(ep->me_key != name); + PyObject *old_value = ep->me_value; + DEOPT_IF(old_value == NULL); /* Ensure dict is GC tracked if it needs to be */ if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) { _PyObject_GC_TRACK(dict); } - old_value = ep->me_value; - PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED; - _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value)); + _PyDict_NotifyEvent(tstate->interp, PyDict_EVENT_MODIFIED, dict, name, PyStackRef_AsPyObjectBorrow(value)); ep->me_value = PyStackRef_AsPyObjectSteal(value); // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault, // when dict only holds the strong reference to value in ep->me_value. diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 5df4986cd838b5..3a7015ccb78987 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2815,7 +2815,6 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - PyObject *old_value; if (!DK_IS_UNICODE(dict->ma_keys)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2825,14 +2824,17 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + PyObject *old_value = ep->me_value; + if (old_value == NULL) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } /* Ensure dict is GC tracked if it needs to be */ if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) { _PyObject_GC_TRACK(dict); } - old_value = ep->me_value; - PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED; _PyFrame_SetStackPointer(frame, stack_pointer); - _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value)); + _PyDict_NotifyEvent(tstate->interp, PyDict_EVENT_MODIFIED, dict, name, PyStackRef_AsPyObjectBorrow(value)); stack_pointer = _PyFrame_GetStackPointer(frame); ep->me_value = PyStackRef_AsPyObjectSteal(value); // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault, diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index efbf2fba8c3106..f658ae503cd70e 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -7443,18 +7443,17 @@ assert(PyDict_CheckExact((PyObject *)dict)); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg); DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries, STORE_ATTR); - PyObject *old_value; DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys), STORE_ATTR); PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; DEOPT_IF(ep->me_key != name, STORE_ATTR); + PyObject *old_value = ep->me_value; + DEOPT_IF(old_value == NULL, STORE_ATTR); /* Ensure dict is GC tracked if it needs to be */ if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) { _PyObject_GC_TRACK(dict); } - old_value = ep->me_value; - PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED; _PyFrame_SetStackPointer(frame, stack_pointer); - _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value)); + _PyDict_NotifyEvent(tstate->interp, PyDict_EVENT_MODIFIED, dict, name, PyStackRef_AsPyObjectBorrow(value)); stack_pointer = _PyFrame_GetStackPointer(frame); ep->me_value = PyStackRef_AsPyObjectSteal(value); // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault,