Fix inheriting extension classes from builtins

Previously, we would just call these "conservative python" objects,
and scan their memory conservatively.  The problem with this is that
the builtin type might have defined a custom GC handler that needs to
be called in addition to the conservative scanning (ie it stores GC
pointers out-of-band that are not discoverable via other gc pointers).

We had dealt with these kinds of issues before which is why I added
the "conservative python kind", but I think the real solution here is
to say that to the GC, these objects are just python objects, and
then let the type machinery decide how to scan the objects, including
how to handle the inheritance rules.  We were already putting
"conservativeGCHandler" as the gc_handler on these conservative types,
so let's use it.

src/capi/typeobject.cpp

@@ -31,6 +31,29 @@ extern "C" void conservativeGCHandler(GCVisitor* v, Box* b) noexcept {
     v->visitPotentialRange((void* const*)b, (void* const*)((char*)b + b->cls->tp_basicsize));
 }
 
+extern "C" void conservativeAndBasesGCHandler(GCVisitor* v, Box* b) noexcept {
+    // TODO: this function is expensive.  We should try to make sure it doesn't get used
+    // that often, or to come up with a better approach.
+
+    // Call all the custom gc handlers defined anywhere in the hierarchy:
+    assert(PyTuple_CheckExact(b->cls->tp_mro));
+    for (auto c : *static_cast<BoxedTuple*>(b->cls->tp_mro)) {
+        if (!PyType_Check(c))
+            continue;
+
+        auto gc_visit = static_cast<BoxedClass*>(c)->gc_visit;
+
+        // Skip conservativeGCHandler since it's slow, and skip conservativeAndBasesGCHandler since
+        // it would cause an infinite loop:
+        if (gc_visit == conservativeGCHandler || gc_visit == conservativeAndBasesGCHandler)
+            continue;
+
+        gc_visit(v, b);
+    }
+
+    conservativeGCHandler(v, b);
+}
+
 static int check_num_args(PyObject* ob, int n) noexcept {
     if (!PyTuple_CheckExact(ob)) {
         PyErr_SetString(PyExc_SystemError, "PyArg_UnpackTuple() argument list is not a tuple");
@@ -3323,6 +3346,15 @@ extern "C" int PyType_Ready(PyTypeObject* cls) noexcept {
     if (cls->tp_alloc == &PystonType_GenericAlloc)
         cls->tp_alloc = &PyType_GenericAlloc;
 
+    // If an extension class visits from a Pyston class that does custom visiting,
+    // the base class needs to call the parent's visit function in case it visits
+    // non-inline data.  There's not an easy way to put in a function pointer here
+    // that defers to a specific class's gc_visit, even if it's a base class, since
+    // the gc_visit could get inherited by subclasses.  For now just use an expensive
+    // function, conservativeAndBasesGCHandler
+    if (base->gc_visit != object_cls->gc_visit && base->gc_visit != &conservativeGCHandler)
+        cls->gc_visit = &conservativeAndBasesGCHandler;
+    else
         cls->gc_visit = &conservativeGCHandler;
     cls->is_user_defined = true;
 

src/gc/collector.cpp

@@ -271,18 +271,15 @@ bool isValidGCObject(void* p) {
     GCAllocation* al = global_heap.getAllocationFromInteriorPointer(p);
     if (!al)
         return false;
-    return al->user_data == p && (al->kind_id == GCKind::CONSERVATIVE_PYTHON || al->kind_id == GCKind::PYTHON);
+    return al->user_data == p && al->kind_id == GCKind::PYTHON;
 }
 
 void registerPythonObject(Box* b) {
     assert(isValidGCMemory(b));
     auto al = GCAllocation::fromUserData(b);
 
-    if (al->kind_id == GCKind::CONSERVATIVE) {
-        al->kind_id = GCKind::CONSERVATIVE_PYTHON;
-    } else {
-        assert(al->kind_id == GCKind::PYTHON);
-    }
+    assert(al->kind_id == GCKind::CONSERVATIVE || al->kind_id == GCKind::PYTHON);
+    al->kind_id = GCKind::PYTHON;
 
     assert(b->cls);
     if (hasOrderedFinalizer(b->cls)) {
@@ -386,7 +383,7 @@ static __attribute__((always_inline)) void visitByGCKind(void* p, GCVisitor& vis
     GCKind kind_id = al->kind_id;
     if (kind_id == GCKind::UNTRACKED) {
         // Nothing to do here.
-    } else if (kind_id == GCKind::CONSERVATIVE || kind_id == GCKind::CONSERVATIVE_PYTHON) {
+    } else if (kind_id == GCKind::CONSERVATIVE) {
         uint32_t bytes = al->kind_data;
         visitor.visitPotentialRange((void**)p, (void**)((char*)p + bytes));
     } else if (kind_id == GCKind::PRECISE) {
@@ -521,7 +518,7 @@ static void graphTraversalMarking(TraceStack& stack, GCVisitor& visitor) {
         GCAllocation* al = GCAllocation::fromUserData(p);
 
 #if TRACE_GC_MARKING
-        if (al->kind_id == GCKind::PYTHON || al->kind_id == GCKind::CONSERVATIVE_PYTHON)
+        if (al->kind_id == GCKind::PYTHON)
             GC_TRACE_LOG("Looking at %s object %p\n", static_cast<Box*>(p)->cls->tp_name, p);
         else
             GC_TRACE_LOG("Looking at non-python allocation %p\n", p);

src/gc/gc.h

@@ -59,10 +59,6 @@ enum class GCKind : uint8_t {
     // because it contains pointers into our heap or our heap points to these
     // objects. These objects inherit from GCAllocatedRuntime.
     RUNTIME = 5,
-
-    // A Python object where we don't have a way to visit precisely with a GC
-    // handler function. These are usually Python objects allocated in C extensions.
-    CONSERVATIVE_PYTHON = 6,
 };
 
 extern "C" void* gc_alloc(size_t nbytes, GCKind kind);

src/gc/heap.cpp

@@ -170,7 +170,7 @@ __attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>*
     VALGRIND_ENABLE_ERROR_REPORTING;
 #endif
 
-    if (alloc_kind == GCKind::PYTHON || alloc_kind == GCKind::CONSERVATIVE_PYTHON) {
+    if (alloc_kind == GCKind::PYTHON) {
 #ifndef NVALGRIND
         VALGRIND_DISABLE_ERROR_REPORTING;
 #endif
@@ -186,8 +186,7 @@ __attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>*
             return false;
         }
 
-        ASSERT(!hasOrderedFinalizer(b->cls) || hasFinalized(al) || alloc_kind == GCKind::CONSERVATIVE_PYTHON, "%s",
-               getTypeName(b));
+        ASSERT(!hasOrderedFinalizer(b->cls) || hasFinalized(al), "%s", getTypeName(b));
 
         if (b->cls->tp_dealloc != dealloc_null && b->cls->has_safe_tp_dealloc) {
             gc_safe_destructors.log();
@@ -273,17 +272,6 @@ void addStatistic(HeapStatistics* stats, GCAllocation* al, int nbytes) {
     } else if (al->kind_id == GCKind::CONSERVATIVE) {
         stats->conservative.nallocs++;
         stats->conservative.nbytes += nbytes;
-    } else if (al->kind_id == GCKind::CONSERVATIVE_PYTHON) {
-        stats->conservative_python.nallocs++;
-        stats->conservative_python.nbytes += nbytes;
-
-        if (stats->collect_cls_stats) {
-            Box* b = (Box*)al->user_data;
-            auto& t = stats->by_cls[b->cls];
-
-            t.nallocs++;
-            t.nbytes += nbytes;
-        }
     } else if (al->kind_id == GCKind::UNTRACKED) {
         stats->untracked.nallocs++;
         stats->untracked.nbytes += nbytes;

src/runtime/util.cpp

@@ -168,11 +168,8 @@ extern "C" void dumpEx(void* p, int levels) {
         return;
     }
 
-    if (al->kind_id == gc::GCKind::PYTHON || al->kind_id == gc::GCKind::CONSERVATIVE_PYTHON) {
-        if (al->kind_id == gc::GCKind::PYTHON)
-            printf("Python object (precisely scanned)\n");
-        else
-            printf("Python object (conservatively scanned)\n");
+    if (al->kind_id == gc::GCKind::PYTHON) {
+        printf("Python object\n");
         Box* b = (Box*)p;
 
         printf("Class: %s", getFullTypeName(b).c_str());