Fix descriptor bug in AttributeCache

Summary:
When the attribute is a descriptor, we have to pay more attention to
the descriptor's type in case it stops being a data descriptor, either through
an attribute assignment on its type (handled by watching the type in
`AttributeCache::fill()`) or an assignment to `__class__`. The latter case is
handled by moving the call of `jit::notifyICsTypeChanged()` from
`_PyJIT_TypeModified()` to `Runtime::notifyTypeModified()`, so it triggers in
more cases (type modification, `__class__` assignment, and type destruction).

I also changed `AttributeCache::typeChanged()` to be more conservative and
always clear everything, rather than adding the bookkeeping or other logic
necessary to detect this case and only clear what's strictly necessary.
Hopefully nobody's mutating types in a hot path anywhere.

Reviewed By: mpage

Differential Revision: D31979189

fbshipit-source-id: 484cb0f596720e7322358df95cf5bf419db647c9

CinderX/Jit/inline_cache.cpp

@@ -146,11 +146,9 @@ AttributeCache::~AttributeCache() {
   }
 }
 
-void AttributeCache::typeChanged(PyTypeObject* type) {
+void AttributeCache::typeChanged(PyTypeObject*) {
   for (auto& entry : entries()) {
-    if (entry.type() == type) {
-      entry.reset();
-    }
+    entry.reset();
   }
 }
 
@@ -178,6 +176,9 @@ void AttributeCache::fill(
       if (descr_type == &PyMemberDescr_Type) {
         mut->set_member_descr(type, descr);
       } else {
+        // If someone deletes descr_types's __set__ method, it will no longer
+        // be a data descriptor, and the cache kind has to change.
+        ac_watcher.watch(descr_type, this);
         mut->set_data_descr(type, descr);
       }
     } else {

CinderX/Jit/pyjit.cpp

@@ -2033,7 +2033,6 @@ void _PyJIT_TypeModified(PyTypeObject* type) {
   if (auto rt = Runtime::getUnchecked()) {
     rt->notifyTypeModified(type, type);
   }
-  jit::notifyICsTypeChanged(type);
 }
 
 void _PyJIT_TypeNameModified(PyTypeObject* type) {

CinderX/Jit/runtime.cpp

@@ -361,6 +361,8 @@ void Runtime::watchType(
 void Runtime::notifyTypeModified(
     BorrowedRef<PyTypeObject> lookup_type,
     BorrowedRef<PyTypeObject> new_type) {
+  notifyICsTypeChanged(lookup_type);
+
   ThreadedCompileSerialize guard;
   auto it = type_deopt_patchers_.find(lookup_type);
   if (it == type_deopt_patchers_.end()) {

CinderX/test_cinderx/test_cinderjit.py

@@ -1025,6 +1025,100 @@ def __init__(self, foo):
         self.assertEqual(get_foo(obj3), 400)
         self.assertEqual(get_foo(obj4), 600)
 
+    def test_descr_type_mutated(self):
+        class Descr:
+            def __get__(self, obj, ty):
+                return "I'm a getter!"
+
+            def __set__(self, obj, ty):
+                raise RuntimeError("unimplemented")
+
+        class C:
+            foo = Descr()
+
+        @cinder_support.failUnlessJITCompiled
+        def get_attr(o):
+            return o.foo
+
+        c = C()
+        self.assertEqual(get_attr(c), "I'm a getter!")
+        c.__dict__["foo"] = "I'm an attribute!"
+        self.assertEqual(get_attr(c), "I'm a getter!")
+        descr_set = Descr.__set__
+        del Descr.__set__
+        self.assertEqual(get_attr(c), "I'm an attribute!")
+        Descr.__set__ = descr_set
+        self.assertEqual(get_attr(c), "I'm a getter!")
+
+    def test_descr_type_changed(self):
+        class Descr:
+            def __get__(self, obj, ty):
+                return "get"
+
+            def __set__(self, obj, ty):
+                raise RuntimeError("unimplemented")
+
+        class GetDescr:
+            def __get__(self, obj, ty):
+                return "only get"
+
+        class C:
+            foo = Descr()
+
+        @cinder_support.failUnlessJITCompiled
+        def get_attr(o):
+            return o.foo
+
+        c = C()
+        self.assertEqual(get_attr(c), "get")
+        c.__dict__["foo"] = "attribute"
+        self.assertEqual(get_attr(c), "get")
+        C.__dict__["foo"].__class__ = GetDescr
+        self.assertEqual(get_attr(c), "attribute")
+        del c.__dict__["foo"]
+        self.assertEqual(get_attr(c), "only get")
+        C.__dict__["foo"].__class__ = Descr
+        self.assertEqual(get_attr(c), "get")
+
+    def test_type_destroyed(self):
+        class A:
+            pass
+
+        class Attr:
+            foo = "in Attr"
+
+        # When C is first created with A as a base, return a normal MRO. When
+        # __bases__ is reassigned later to use Attr as a base, return an MRO
+        # without C in it. This gives us a type with no reference cycles in it,
+        # which can be destroyed without running the GC (which calls
+        # type_clear() and hides the bug).
+        class NoSelfMRO(type):
+            def mro(cls):
+                if cls.__bases__ == (A,):
+                    return (cls, A, object)
+                return (cls.__bases__[0], object)
+
+        class C(A, metaclass=NoSelfMRO):
+            __slots__ = ()
+
+        C.__bases__ = (Attr,)
+
+        @cinder_support.failUnlessJITCompiled
+        def get_attr(o):
+            return o.foo
+
+        c = C()
+        self.assertEqual(get_attr(c), "in Attr")
+
+        del c
+        del C
+
+        class D:
+            foo = "in D"
+
+        d = D()
+        self.assertEqual(get_attr(d), "in D")
+
 
 class SetNonDataDescrAttrTests(unittest.TestCase):
     @cinder_support.failUnlessJITCompiled