From 5492d0eca51d73570c9af62039676a4263752194 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Wed, 27 Oct 2021 15:48:30 -0700 Subject: [PATCH] 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 | 9 +-- CinderX/Jit/pyjit.cpp | 1 - CinderX/Jit/runtime.cpp | 2 + CinderX/test_cinderx/test_cinderjit.py | 94 ++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 5 deletions(-) diff --git a/CinderX/Jit/inline_cache.cpp b/CinderX/Jit/inline_cache.cpp index a6379a372c8..553bc47ac4b 100644 --- a/CinderX/Jit/inline_cache.cpp +++ b/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 { diff --git a/CinderX/Jit/pyjit.cpp b/CinderX/Jit/pyjit.cpp index 26bd1c9df95..d9765e4599b 100644 --- a/CinderX/Jit/pyjit.cpp +++ b/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) { diff --git a/CinderX/Jit/runtime.cpp b/CinderX/Jit/runtime.cpp index 0cc6e02ebd9..5df9e9fb46d 100644 --- a/CinderX/Jit/runtime.cpp +++ b/CinderX/Jit/runtime.cpp @@ -361,6 +361,8 @@ void Runtime::watchType( void Runtime::notifyTypeModified( BorrowedRef lookup_type, BorrowedRef new_type) { + notifyICsTypeChanged(lookup_type); + ThreadedCompileSerialize guard; auto it = type_deopt_patchers_.find(lookup_type); if (it == type_deopt_patchers_.end()) { diff --git a/CinderX/test_cinderx/test_cinderjit.py b/CinderX/test_cinderx/test_cinderjit.py index 697fc97eefb..db8539daa7c 100644 --- a/CinderX/test_cinderx/test_cinderjit.py +++ b/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