Skip to content

Commit b593d91

Browse files
committed
Restore _GUARD_KEYS_VERSION in LOAD_ATTR specializations
The 3.14 and 3.15 interpreters were missing the shared-keys version check in LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES and LOAD_ATTR_METHOD_WITH_VALUES, so a warmed load site kept returning the cached class attribute after an instance stored the attribute through the type's shared keys. This is what breaks SQLAlchemy's deferred column loads in gh-115. The restored blocks match what CPython's case generator emits for these opcodes on 3.14, and the cache entry they read is already populated: specialize_attr_loadclassattr writes the keys version whenever META_PYTHON is not defined.
1 parent 5f1996a commit b593d91

3 files changed

Lines changed: 157 additions & 4 deletions

File tree

cinderx/Interpreter/3.14/Includes/generated_cases.c.h

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10059,7 +10059,18 @@
1005910059
JUMP_TO_PREDICTED(LOAD_ATTR);
1006010060
}
1006110061
}
10062-
/* Skip 2 cache entries */
10062+
// _GUARD_KEYS_VERSION
10063+
{
10064+
uint32_t keys_version = read_u32(&this_instr[4].cache);
10065+
PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner));
10066+
PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls;
10067+
PyDictKeysObject *keys = owner_heap_type->ht_cached_keys;
10068+
if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) {
10069+
UPDATE_MISS_STATS(LOAD_ATTR);
10070+
assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR));
10071+
JUMP_TO_PREDICTED(LOAD_ATTR);
10072+
}
10073+
}
1006310074
// _LOAD_ATTR_METHOD_WITH_VALUES
1006410075
{
1006510076
PyObject *descr = read_obj(&this_instr[6].cache);
@@ -10242,7 +10253,18 @@
1024210253
JUMP_TO_PREDICTED(LOAD_ATTR);
1024310254
}
1024410255
}
10245-
/* Skip 2 cache entries */
10256+
// _GUARD_KEYS_VERSION
10257+
{
10258+
uint32_t keys_version = read_u32(&this_instr[4].cache);
10259+
PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner));
10260+
PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls;
10261+
PyDictKeysObject *keys = owner_heap_type->ht_cached_keys;
10262+
if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) {
10263+
UPDATE_MISS_STATS(LOAD_ATTR);
10264+
assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR));
10265+
JUMP_TO_PREDICTED(LOAD_ATTR);
10266+
}
10267+
}
1024610268
// _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES
1024710269
{
1024810270
PyObject *descr = read_obj(&this_instr[6].cache);

cinderx/Interpreter/3.15/Includes/generated_cases.c.h

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10240,7 +10240,18 @@
1024010240
JUMP_TO_PREDICTED(LOAD_ATTR);
1024110241
}
1024210242
}
10243-
/* Skip 2 cache entries */
10243+
// _GUARD_KEYS_VERSION
10244+
{
10245+
uint32_t keys_version = read_u32(&this_instr[4].cache);
10246+
PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner));
10247+
PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls;
10248+
PyDictKeysObject *keys = owner_heap_type->ht_cached_keys;
10249+
if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) {
10250+
UPDATE_MISS_STATS(LOAD_ATTR);
10251+
assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR));
10252+
JUMP_TO_PREDICTED(LOAD_ATTR);
10253+
}
10254+
}
1024410255
// _LOAD_ATTR_METHOD_WITH_VALUES
1024510256
{
1024610257
PyObject *descr = read_obj(&this_instr[6].cache);
@@ -10423,7 +10434,18 @@
1042310434
JUMP_TO_PREDICTED(LOAD_ATTR);
1042410435
}
1042510436
}
10426-
/* Skip 2 cache entries */
10437+
// _GUARD_KEYS_VERSION
10438+
{
10439+
uint32_t keys_version = read_u32(&this_instr[4].cache);
10440+
PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner));
10441+
PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls;
10442+
PyDictKeysObject *keys = owner_heap_type->ht_cached_keys;
10443+
if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) {
10444+
UPDATE_MISS_STATS(LOAD_ATTR);
10445+
assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR));
10446+
JUMP_TO_PREDICTED(LOAD_ATTR);
10447+
}
10448+
}
1042710449
// _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES
1042810450
{
1042910451
PyObject *descr = read_obj(&this_instr[6].cache);
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
3+
# pyre-strict
4+
5+
"""Regression tests for gh-115: a load site specialized to
6+
LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES or LOAD_ATTR_METHOD_WITH_VALUES must
7+
deopt once the attribute is stored on an instance. Runs in a subprocess
8+
because it depends on precise interpreter warm-up state."""
9+
10+
import subprocess
11+
import sys
12+
import unittest
13+
14+
import cinderx
15+
16+
cinderx.init()
17+
18+
from cinderx.test_support import ENCODING, subprocess_env
19+
20+
21+
def run_snippet(source: str) -> "subprocess.CompletedProcess[str]":
22+
return subprocess.run(
23+
[sys.executable, "-c", source],
24+
stdout=subprocess.PIPE,
25+
stderr=subprocess.STDOUT,
26+
encoding=ENCODING,
27+
env=subprocess_env(),
28+
)
29+
30+
31+
class LoadAttrKeysVersionTests(unittest.TestCase):
32+
@unittest.skipUnless(sys.version_info >= (3, 14), "3.14+ opcodes")
33+
def test_instance_attr_shadows_class_default(self) -> None:
34+
proc = run_snippet(
35+
"""if 1:
36+
import cinderx.jit
37+
cinderx.jit.auto()
38+
import dis
39+
40+
class C:
41+
rcs = None
42+
43+
def make_without():
44+
o = C.__new__(C)
45+
o.x1 = 1
46+
return o
47+
48+
def make_with():
49+
o = C.__new__(C)
50+
o.x1 = 1
51+
o.rcs = (1, 2, 3)
52+
return o
53+
54+
def read(o):
55+
return o.rcs
56+
57+
for _ in range(300):
58+
assert read(make_without()) is None
59+
60+
names = {i.opname for i in dis.get_instructions(read, adaptive=True)}
61+
assert "LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES" in names, sorted(names)
62+
63+
got = read(make_with())
64+
assert got == (1, 2, 3), f"stale class default returned: {got!r}"
65+
"""
66+
)
67+
self.assertEqual(proc.returncode, 0, proc.stdout)
68+
69+
@unittest.skipUnless(sys.version_info >= (3, 14), "3.14+ opcodes")
70+
def test_instance_attr_shadows_method(self) -> None:
71+
proc = run_snippet(
72+
"""if 1:
73+
import cinderx.jit
74+
cinderx.jit.auto()
75+
import dis
76+
77+
class C:
78+
def m(self):
79+
return "class-method"
80+
81+
def make_plain():
82+
o = C.__new__(C)
83+
o.x1 = 1
84+
return o
85+
86+
def make_shadowed():
87+
o = C.__new__(C)
88+
o.x1 = 1
89+
o.m = lambda: "instance-attr"
90+
return o
91+
92+
def call_m(o):
93+
return o.m()
94+
95+
for _ in range(300):
96+
assert call_m(make_plain()) == "class-method"
97+
98+
names = {i.opname for i in dis.get_instructions(call_m, adaptive=True)}
99+
assert "LOAD_ATTR_METHOD_WITH_VALUES" in names, sorted(names)
100+
101+
got = call_m(make_shadowed())
102+
assert got == "instance-attr", f"shadowed method ignored: {got!r}"
103+
"""
104+
)
105+
self.assertEqual(proc.returncode, 0, proc.stdout)
106+
107+
108+
if __name__ == "__main__":
109+
unittest.main()

0 commit comments

Comments
 (0)