[Python-checkins] GH-99298: Clean up attribute specializations (GH-99398)

Thu Nov 17 18:09:25 EST 2022

https://github.com/python/cpython/commit/b629fdd88ac1c20439b49cbc9aa33b27cd5f6daf
commit: b629fdd88ac1c20439b49cbc9aa33b27cd5f6daf
branch: main
author: Brandt Bucher <brandtbucher at microsoft.com>
committer: brandtbucher <brandtbucher at gmail.com>
date: 2022年11月17日T15:09:18-08:00
summary:
GH-99298: Clean up attribute specializations (GH-99398)
files:
A Misc/NEWS.d/next/Core and Builtins/2022-11-10-16-53-40.gh-issue-99298.HqRJES.rst
M Include/internal/pycore_code.h
M Python/bytecodes.c
M Python/generated_cases.c.h
M Python/specialize.c

diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h
index 0af240ca3621..ba36ee38d2b0 100644
--- a/Include/internal/pycore_code.h
+++ b/Include/internal/pycore_code.h
@@ -213,10 +213,10 @@ extern int _PyLineTable_PreviousAddressRange(PyCodeAddressRange *range);
 
 /* Specialization functions */
 
-extern int _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr,
- PyObject *name);
-extern int _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr,
+extern void _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr,
 PyObject *name);
+extern void _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr,
+ PyObject *name);
 extern void _Py_Specialize_LoadGlobal(PyObject *globals, PyObject *builtins,
 _Py_CODEUNIT *instr, PyObject *name);
 extern void _Py_Specialize_BinarySubscr(PyObject *sub, PyObject *container,
diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-11-10-16-53-40.gh-issue-99298.HqRJES.rst b/Misc/NEWS.d/next/Core and Builtins/2022-11-10-16-53-40.gh-issue-99298.HqRJES.rst
new file mode 100644
index 000000000000..79a5b3ba98cf
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2022-11-10-16-53-40.gh-issue-99298.HqRJES.rst	
@@ -0,0 +1,3 @@
+Remove the remaining error paths for attribute specializations, and refuse
+to specialize attribute accesses on types that haven't had
+:c:func:`PyType_Ready` called on them yet.
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 27ef45fba066..a3e02674c290 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -1137,11 +1137,7 @@ dummy_func(
 PyObject *owner = TOP();
 PyObject *name = GETITEM(names, oparg);
 next_instr--;
- if (_Py_Specialize_StoreAttr(owner, next_instr, name)) {
- // "undo" the rewind so end up in the correct handler:
- next_instr++;
- goto error;
- }
+ _Py_Specialize_StoreAttr(owner, next_instr, name);
 DISPATCH_SAME_OPARG();
 }
 STAT_INC(STORE_ATTR, deferred);
@@ -1713,11 +1709,7 @@ dummy_func(
 PyObject *owner = TOP();
 PyObject *name = GETITEM(names, oparg>>1);
 next_instr--;
- if (_Py_Specialize_LoadAttr(owner, next_instr, name)) {
- // "undo" the rewind so end up in the correct handler:
- next_instr++;
- goto error;
- }
+ _Py_Specialize_LoadAttr(owner, next_instr, name);
 DISPATCH_SAME_OPARG();
 }
 STAT_INC(LOAD_ATTR, deferred);
diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index b60655884c1c..ba00203da301 100644
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -1136,11 +1136,7 @@
 PyObject *owner = TOP();
 PyObject *name = GETITEM(names, oparg);
 next_instr--;
- if (_Py_Specialize_StoreAttr(owner, next_instr, name)) {
- // "undo" the rewind so end up in the correct handler:
- next_instr++;
- goto error;
- }
+ _Py_Specialize_StoreAttr(owner, next_instr, name);
 DISPATCH_SAME_OPARG();
 }
 STAT_INC(STORE_ATTR, deferred);
@@ -1718,11 +1714,7 @@
 PyObject *owner = TOP();
 PyObject *name = GETITEM(names, oparg>>1);
 next_instr--;
- if (_Py_Specialize_LoadAttr(owner, next_instr, name)) {
- // "undo" the rewind so end up in the correct handler:
- next_instr++;
- goto error;
- }
+ _Py_Specialize_LoadAttr(owner, next_instr, name);
 DISPATCH_SAME_OPARG();
 }
 STAT_INC(LOAD_ATTR, deferred);
diff --git a/Python/specialize.c b/Python/specialize.c
index eea9d1c74884..cd09b188b7fa 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -663,32 +663,33 @@ static int specialize_attr_loadmethod(PyObject* owner, _Py_CODEUNIT* instr, PyOb
 PyObject* descr, DescriptorClassification kind);
 static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name);
 
-int
+void
 _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 {
 assert(_PyOpcode_Caches[LOAD_ATTR] == INLINE_CACHE_ENTRIES_LOAD_ATTR);
 _PyAttrCache *cache = (_PyAttrCache *)(instr + 1);
+ PyTypeObject *type = Py_TYPE(owner);
+ if (!_PyType_IsReady(type)) {
+ // We *might* not really need this check, but we inherited it from
+ // PyObject_GenericGetAttr and friends... and this way we still do the
+ // right thing if someone forgets to call PyType_Ready(type):
+ SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER);
+ goto fail;
+ }
 if (PyModule_CheckExact(owner)) {
- int err = specialize_module_load_attr(owner, instr, name, LOAD_ATTR,
- LOAD_ATTR_MODULE);
- if (err) {
+ if (specialize_module_load_attr(owner, instr, name, LOAD_ATTR,
+ LOAD_ATTR_MODULE))
+ {
 goto fail;
 }
 goto success;
 }
 if (PyType_Check(owner)) {
- int err = specialize_class_load_attr(owner, instr, name);
- if (err) {
+ if (specialize_class_load_attr(owner, instr, name)) {
 goto fail;
 }
 goto success;
 }
- PyTypeObject *type = Py_TYPE(owner);
- if (type->tp_dict == NULL) {
- if (PyType_Ready(type) < 0) {
- return -1;
- }
- }
 PyObject *descr = NULL;
 DescriptorClassification kind = analyze_descriptor(type, name, &descr, 0);
 assert(descr != NULL || kind == ABSENT || kind == GETSET_OVERRIDDEN);
@@ -803,14 +804,9 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 case ABSENT:
 break;
 }
- int err = specialize_dict_access(
- owner, instr, type, kind, name,
- LOAD_ATTR, LOAD_ATTR_INSTANCE_VALUE, LOAD_ATTR_WITH_HINT
- );
- if (err < 0) {
- return -1;
- }
- if (err) {
+ if (specialize_dict_access(owner, instr, type, kind, name, LOAD_ATTR,
+ LOAD_ATTR_INSTANCE_VALUE, LOAD_ATTR_WITH_HINT))
+ {
 goto success;
 }
 fail:
@@ -818,20 +814,26 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 assert(!PyErr_Occurred());
 _Py_SET_OPCODE(*instr, LOAD_ATTR);
 cache->counter = adaptive_counter_backoff(cache->counter);
- return 0;
+ return;
 success:
 STAT_INC(LOAD_ATTR, success);
 assert(!PyErr_Occurred());
 cache->counter = adaptive_counter_cooldown();
- return 0;
 }
 
-int
+void
 _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 {
 assert(_PyOpcode_Caches[STORE_ATTR] == INLINE_CACHE_ENTRIES_STORE_ATTR);
 _PyAttrCache *cache = (_PyAttrCache *)(instr + 1);
 PyTypeObject *type = Py_TYPE(owner);
+ if (!_PyType_IsReady(type)) {
+ // We *might* not really need this check, but we inherited it from
+ // PyObject_GenericSetAttr and friends... and this way we still do the
+ // right thing if someone forgets to call PyType_Ready(type):
+ SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER);
+ goto fail;
+ }
 if (PyModule_CheckExact(owner)) {
 SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OVERRIDDEN);
 goto fail;
@@ -890,15 +892,9 @@ _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 case ABSENT:
 break;
 }
-
- int err = specialize_dict_access(
- owner, instr, type, kind, name,
- STORE_ATTR, STORE_ATTR_INSTANCE_VALUE, STORE_ATTR_WITH_HINT
- );
- if (err < 0) {
- return -1;
- }
- if (err) {
+ if (specialize_dict_access(owner, instr, type, kind, name, STORE_ATTR,
+ STORE_ATTR_INSTANCE_VALUE, STORE_ATTR_WITH_HINT))
+ {
 goto success;
 }
 fail:
@@ -906,12 +902,11 @@ _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
 assert(!PyErr_Occurred());
 _Py_SET_OPCODE(*instr, STORE_ATTR);
 cache->counter = adaptive_counter_backoff(cache->counter);
- return 0;
+ return;
 success:
 STAT_INC(STORE_ATTR, success);
 assert(!PyErr_Occurred());
 cache->counter = adaptive_counter_cooldown();
- return 0;
 }
 
 
