[Python-checkins] bpo-45107: Make LOAD_METHOD_CLASS safer and faster, clean up comments (GH-28177)

[Fidget-Spinner]

https://github.com/python/cpython/commit/70bed6f9936c811472b376edd93c37bcf8f06f35
commit: 70bed6f9936c811472b376edd93c37bcf8f06f35
branch: main
author: Ken Jin <28750310+Fidget-Spinner at users.noreply.github.com>
committer: Fidget-Spinner <28750310+Fidget-Spinner at users.noreply.github.com>
date: 2021-09-17T18:47:36+08:00
summary:

bpo-45107: Make LOAD_METHOD_CLASS safer and faster, clean up comments (GH-28177)

* Improve comments

* Check cls is a type, remove dict calculation

files:
M Python/ceval.c
M Python/specialize.c

diff --git a/Python/ceval.c b/Python/ceval.c
index bf95d50b62958..ab692fd8ded15 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -4496,6 +4496,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
         }
 
         TARGET(LOAD_METHOD_MODULE): {
+            /* LOAD_METHOD, for module methods */
             assert(cframe.use_tracing == 0);
             PyObject *owner = TOP();
             PyObject *res;
@@ -4515,15 +4516,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
             _PyObjectCache *cache2 = &caches[-2].obj;
 
             PyObject *cls = TOP();
-            PyTypeObject *cls_type = Py_TYPE(cls);
-            assert(cls_type->tp_dictoffset > 0);
-            PyObject *dict = *(PyObject **) ((char *)cls + cls_type->tp_dictoffset);
-            // Don't care if no dict -- tp_version_tag should catch anything wrong.
-            DEOPT_IF(dict != NULL && ((PyDictObject *)dict)->ma_keys->dk_version !=
-                cache1->dk_version_or_hint, LOAD_METHOD);
+            DEOPT_IF(!PyType_Check(cls), LOAD_METHOD);
             DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != cache1->tp_version,
                 LOAD_METHOD);
-            assert(cache1->dk_version_or_hint != 0);
             assert(cache1->tp_version != 0);
 
             STAT_INC(LOAD_METHOD, hit);
diff --git a/Python/specialize.c b/Python/specialize.c
index 8e04c8372d76c..398524cdb5ba1 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -974,20 +974,19 @@ _Py_Specialize_LoadMethod(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name,
         // Fall through.
     } // Else owner is maybe a builtin with no dict, or __slots__. Doesn't matter.
 
-    /* `descr` is borrowed. Just check tp_version_tag before accessing in case
-    *  it's deleted.  This is safe for methods (even inherited ones from super
-    *  classes!) as long as tp_version_tag is validated for two main reasons:
+    /* `descr` is borrowed. This is safe for methods (even inherited ones from
+    *  super classes!) as long as tp_version_tag is validated for two main reasons:
     *
     *  1. The class will always hold a reference to the method so it will
     *  usually not be GC-ed. Should it be deleted in Python, e.g.
     *  `del obj.meth`, tp_version_tag will be invalidated, because of reason 2.
     *
     *  2. The pre-existing type method cache (MCACHE) uses the same principles
-    *  of caching a borrowed descriptor. It does all the heavy lifting for us.
-    *  E.g. it invalidates on any MRO modification, on any type object
-    *  change along said MRO, etc. (see PyType_Modified usages in typeobject.c).
-    *  The type method cache has been working since Python 2.6 and it's
-    *  battle-tested.
+    *  of caching a borrowed descriptor. The MCACHE infrastructure does all the
+    *  heavy lifting for us. E.g. it invalidates tp_version_tag on any MRO
+    *  modification, on any type object change along said MRO, etc. (see
+    *  PyType_Modified usages in typeobject.c). The MCACHE has been
+    *  working since Python 2.6 and it's battle-tested.
     */
     cache2->obj = descr;
     cache1->dk_version_or_hint = keys_version;