Merge pull request #884 from rudi-c/rewriterscan2

Scan rewriters for interior pointers

src/asm_writing/icinfo.cpp

@@ -128,6 +128,12 @@ void ICSlotRewrite::commit(CommitHook* hook) {
     llvm::sys::Memory::InvalidateInstructionCache(slot_start, ic->getSlotSize());
 }
 
+void ICSlotRewrite::gc_visit(GCVisitor* visitor) {
+    for (auto& dependency : dependencies) {
+        visitor->visitPotentialRedundant(dependency.first);
+    }
+}
+
 void ICSlotRewrite::addDependenceOn(ICInvalidator& invalidator) {
     dependencies.push_back(std::make_pair(&invalidator, invalidator.version()));
 }

src/asm_writing/icinfo.h

@@ -82,6 +82,8 @@ public:
 
     ICSlotInfo* prepareEntry();
 
+    void gc_visit(gc::GCVisitor* visitor);
+
     void addDependenceOn(ICInvalidator&);
     void commit(CommitHook* hook);
     void abort();

src/asm_writing/rewriter.cpp

@@ -1376,6 +1376,10 @@ void Rewriter::addDependenceOn(ICInvalidator& invalidator) {
     rewrite->addDependenceOn(invalidator);
 }
 
+void Rewriter::gc_visit(GCVisitor* visitor) {
+    rewrite->gc_visit(visitor);
+}
+
 Location Rewriter::allocScratch() {
     assertPhaseEmitting();
 

src/asm_writing/rewriter.h

@@ -28,6 +28,7 @@
 #include "asm_writing/assembler.h"
 #include "asm_writing/icinfo.h"
 #include "core/threading.h"
+#include "gc/gc.h"
 
 namespace pyston {
 
@@ -331,7 +332,7 @@ enum class ActionType { NORMAL, GUARD, MUTATION };
 // non-NULL fake pointer, definitely legit
 #define LOCATION_PLACEHOLDER ((RewriterVar*)1)
 
-class Rewriter : public ICSlotRewrite::CommitHook {
+class Rewriter : public ICSlotRewrite::CommitHook, public gc::GCVisitable {
 private:
     class RegionAllocator {
     public:
@@ -586,6 +587,8 @@ public:
 
     void addDependenceOn(ICInvalidator&);
 
+    void gc_visit(gc::GCVisitor* visitor);
+
     static Rewriter* createRewriter(void* rtn_addr, int num_args, const char* debug_name);
 
     static bool isLargeConstant(int64_t val) { return (val < (-1L << 31) || val >= (1L << 31) - 1); }

src/core/threading.cpp

@@ -56,6 +56,8 @@ private:
     bool saved;
     ucontext_t ucontext;
 
+    std::deque<gc::GCVisitable*> gc_objs_stacks;
+
 public:
     void* stack_start;
 
@@ -99,6 +101,13 @@ public:
 
     ucontext_t* getContext() { return &ucontext; }
 
+    void pushGCObject(gc::GCVisitable* obj) { gc_objs_stacks.push_back(obj); }
+
+    void popGCObject(gc::GCVisitable* obj) {
+        ASSERT(gc_objs_stacks.back() == obj, "push/pop of stack-bound object out of order");
+        gc_objs_stacks.pop_back();
+    }
+
     void pushGenerator(BoxedGenerator* g, void* new_stack_start, void* old_stack_limit) {
         previous_stacks.emplace_back(g, this->stack_start, old_stack_limit);
         this->stack_start = new_stack_start;
@@ -132,6 +141,10 @@ public:
             v->visitPotentialRange((void**)stack_info.stack_start, (void**)stack_info.stack_limit);
 #endif
         }
+
+        for (auto& obj : gc_objs_stacks) {
+            obj->gc_visit(v);
+        }
     }
 };
 static std::unordered_map<pthread_t, ThreadStateInternal*> current_threads;
@@ -149,6 +162,14 @@ void popGenerator() {
     current_internal_thread_state->popGenerator();
 }
 
+void pushGCObject(gc::GCVisitable* obj) {
+    current_internal_thread_state->pushGCObject(obj);
+}
+
+void popGCObject(gc::GCVisitable* obj) {
+    current_internal_thread_state->popGCObject(obj);
+}
+
 // These are guarded by threading_lock
 static int signals_waiting(0);
 static gc::GCVisitor* cur_visitor = NULL;

src/core/threading.h

@@ -31,6 +31,7 @@ class BoxedGenerator;
 
 namespace gc {
 class GCVisitor;
+class GCVisitable;
 }
 
 #if ENABLE_SAMPLING_PROFILER
@@ -61,6 +62,8 @@ void visitAllStacks(gc::GCVisitor* v);
 void pushGenerator(BoxedGenerator* g, void* new_stack_start, void* old_stack_limit);
 void popGenerator();
 
+void pushGCObject(gc::GCVisitable* obj);
+void popGCObject(gc::GCVisitable* obj);
 
 #ifndef THREADING_USE_GIL
 #define THREADING_USE_GIL 1

src/gc/collector.h

@@ -47,8 +47,8 @@ public:
 
     void operator=(Box* b) { value = b; }
 
-    operator Box*() { return value; }
     Box* operator->() { return value; }
+    Box* get() { return value; }
 };
 
 bool isNonheapRoot(void* p);

src/gc/gc.h

@@ -43,6 +43,15 @@ namespace pyston {
 
 class Box;
 
+namespace gc {
+class GCVisitable;
+}
+
+namespace threading {
+void pushGCObject(gc::GCVisitable* obj);
+void popGCObject(gc::GCVisitable* obj);
+}
+
 namespace gc {
 
 class TraceStack;
@@ -128,16 +137,73 @@ bool isValidGCMemory(void* p);
 // Whether p is valid gc memory and is set to have Python destructor semantics applied
 bool isValidGCObject(void* p);
 
+// Situation: Sometimes, we allocate an object on the stack (e.g. ASTInterpreter) who fields may be pointers
+// to objects in the Pyston heap. These pointers need to be scanned by the GC. Since the GC scans the entire
+// stack conservatively, these fields will be scanned. However, it is also possible that the stack-allocated
+// object points to a non-Pyston heap object which contains pointers to Pyston heap objects. In that case, the
+// conservative scanner won't get to those pointers.
+//
+// As such, objects who contain pointers to pointers to Pyston heap objects need a GC handler function.
+
+// Runtime objects who need to be visited by the GC should inherit from this.
+class GCVisitable {
+public:
+    virtual ~GCVisitable() = default;
+    virtual void gc_visit(GCVisitor* visitor) = 0;
+};
+
 // Use this if a C++ object needs to be allocated in our heap.
-class GCAllocatedRuntime {
+class GCAllocatedRuntime : public GCVisitable {
 public:
-    virtual ~GCAllocatedRuntime() {}
+    virtual ~GCAllocatedRuntime() = default;
 
     void* operator new(size_t size) __attribute__((visibility("default"))) { return gc_alloc(size, GCKind::RUNTIME); }
     void operator delete(void* ptr) __attribute__((visibility("default"))) { gc_free(ptr); }
 
     virtual void gc_visit(GCVisitor* visitor) = 0;
 };
+
+// This is a way to call gc_visit on objects whose lifetime is bound to the stack,
+// but may be contained within a unique_ptr or some other container.
+template <typename T> class UniqueScanningHandle {
+    T* obj = NULL;
+
+public:
+    UniqueScanningHandle(T* obj) : obj(obj) {
+#if MOVING_GC
+        if (obj) {
+            threading::pushGCObject(obj);
+        }
+#endif
+    }
+
+    ~UniqueScanningHandle() {
+#if MOVING_GC
+        if (obj) {
+            threading::popGCObject(obj);
+        }
+#endif
+        delete obj;
+    }
+
+    T* operator->() { return obj; }
+    T* get() { return obj; }
+    void reset(T* t = nullptr) {
+#if MOVING_GC
+        if (obj) {
+            threading::popGCObject(obj);
+        }
+#endif
+        delete obj;
+        obj = t;
+#if MOVING_GC
+        if (t) {
+            threading::pushGCObject(t);
+        }
+#endif
+    }
+};
+
 } // namespace gc
 }
 

src/runtime/objmodel.cpp

@@ -1550,7 +1550,7 @@ extern "C" Box* getclsattr(Box* obj, BoxedString* attr) {
     }
 
 #if 0
-    std::unique_ptr<Rewriter> rewriter(Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, 1, "getclsattr"));
+    gc::UniqueScanningHandle<Rewriter> rewriter(Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, 1, "getclsattr"));
 
     if (rewriter.get()) {
         //rewriter->trap();
@@ -1562,7 +1562,7 @@ extern "C" Box* getclsattr(Box* obj, BoxedString* attr) {
             rewriter->commit();
         }
 #else
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, "getclsattr"));
 
     if (rewriter.get()) {
@@ -1993,7 +1993,7 @@ template <ExceptionStyle S> Box* _getattrEntry(Box* obj, BoxedString* attr, void
 #endif
     }
 
-    std::unique_ptr<Rewriter> rewriter(Rewriter::createRewriter(return_addr, 2, "getattr"));
+    gc::UniqueScanningHandle<Rewriter> rewriter(Rewriter::createRewriter(return_addr, 2, "getattr"));
 
 #if 0 && STAT_TIMERS
     static uint64_t* st_id = Stats::getStatCounter("us_timer_slowpath_getattr_patchable");
@@ -2262,7 +2262,7 @@ extern "C" void setattr(Box* obj, BoxedString* attr, Box* attr_val) {
         return;
     }
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
 
     setattrofunc tp_setattro = obj->cls->tp_setattro;
@@ -2360,7 +2360,7 @@ extern "C" bool nonzero(Box* obj) {
     static StatCounter slowpath_nonzero("slowpath_nonzero");
     slowpath_nonzero.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 1, "nonzero"));
 
     RewriterVar* r_obj = NULL;
@@ -2456,16 +2456,18 @@ extern "C" bool nonzero(Box* obj) {
     static BoxedString* len_str = internStringImmortal("__len__");
 
     // try __nonzero__
-    CallRewriteArgs crewrite_args(rewriter.get(), r_obj, rewriter ? rewriter->getReturnDestination() : Location());
-    Box* rtn = callattrInternal0<CXX>(obj, nonzero_str, CLASS_ONLY, rewriter ? &crewrite_args : NULL, ArgPassSpec(0));
+    CallRewriteArgs crewrite_args(rewriter.get(), r_obj,
+                                  rewriter.get() ? rewriter->getReturnDestination() : Location());
+    Box* rtn
+        = callattrInternal0<CXX>(obj, nonzero_str, CLASS_ONLY, rewriter.get() ? &crewrite_args : NULL, ArgPassSpec(0));
     if (!crewrite_args.out_success)
         rewriter.reset();
 
     if (!rtn) {
         // try __len__
         crewrite_args
-            = CallRewriteArgs(rewriter.get(), r_obj, rewriter ? rewriter->getReturnDestination() : Location());
-        rtn = callattrInternal0<CXX>(obj, len_str, CLASS_ONLY, rewriter ? &crewrite_args : NULL, ArgPassSpec(0));
+            = CallRewriteArgs(rewriter.get(), r_obj, rewriter.get() ? rewriter->getReturnDestination() : Location());
+        rtn = callattrInternal0<CXX>(obj, len_str, CLASS_ONLY, rewriter.get() ? &crewrite_args : NULL, ArgPassSpec(0));
         if (!crewrite_args.out_success)
             rewriter.reset();
 
@@ -2738,7 +2740,7 @@ extern "C" i64 unboxedLen(Box* obj) {
     static StatCounter slowpath_unboxedlen("slowpath_unboxedlen");
     slowpath_unboxedlen.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 1, "unboxedLen"));
 
     BoxedInt* lobj;
@@ -2932,7 +2934,7 @@ Box* _callattrEntry(Box* obj, BoxedString* attr, CallattrFlags flags, Box* arg1,
     // Uncomment this to help debug if callsites aren't getting rewritten:
     // printf("Slowpath call: %p (%s.%s)\n", return_addr, obj->cls->tp_name, attr->c_str());
 
-    std::unique_ptr<Rewriter> rewriter(Rewriter::createRewriter(return_addr, num_orig_args, "callattr"));
+    gc::UniqueScanningHandle<Rewriter> rewriter(Rewriter::createRewriter(return_addr, num_orig_args, "callattr"));
     Box* rtn;
 
     LookupScope scope = flags.cls_only ? CLASS_ONLY : CLASS_OR_INST;
@@ -4055,7 +4057,8 @@ static Box* runtimeCallEntry(Box* obj, ArgPassSpec argspec, Box* arg1, Box* arg2
         assert(argspec.num_keywords == keyword_names->size());
         num_orig_args++;
     }
-    std::unique_ptr<Rewriter> rewriter(Rewriter::createRewriter(return_addr, num_orig_args, "runtimeCall"));
+    gc::UniqueScanningHandle<Rewriter> rewriter(Rewriter::createRewriter(return_addr, num_orig_args, "runtimeCall"));
+
     Box* rtn;
 
 #if 0 && STAT_TIMERS
@@ -4258,7 +4261,7 @@ extern "C" Box* binop(Box* lhs, Box* rhs, int op_type) {
     // int id = Stats::getStatId("slowpath_binop_" + *getTypeName(lhs) + op_name + *getTypeName(rhs));
     // Stats::log(id);
 
-    std::unique_ptr<Rewriter> rewriter((Rewriter*)NULL);
+    gc::UniqueScanningHandle<Rewriter> rewriter((Rewriter*)NULL);
     // Currently can't patchpoint user-defined binops since we can't assume that just because
     // resolving it one way right now (ex, using the value from lhs.__add__) means that later
     // we'll resolve it the same way, even for the same argument types.
@@ -4295,7 +4298,7 @@ extern "C" Box* augbinop(Box* lhs, Box* rhs, int op_type) {
     // int id = Stats::getStatId("slowpath_augbinop_" + *getTypeName(lhs) + op_name + *getTypeName(rhs));
     // Stats::log(id);
 
-    std::unique_ptr<Rewriter> rewriter((Rewriter*)NULL);
+    gc::UniqueScanningHandle<Rewriter> rewriter((Rewriter*)NULL);
     // Currently can't patchpoint user-defined binops since we can't assume that just because
     // resolving it one way right now (ex, using the value from lhs.__add__) means that later
     // we'll resolve it the same way, even for the same argument types.
@@ -4596,7 +4599,7 @@ extern "C" Box* compare(Box* lhs, Box* rhs, int op_type) {
     slowpath_compare.log();
     static StatCounter nopatch_compare("nopatch_compare");
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "compare"));
 
     if (rewriter.get()) {
@@ -4655,11 +4658,11 @@ extern "C" Box* unaryop(Box* operand, int op_type) {
 
     BoxedString* op_name = getOpName(op_type);
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 1, "unaryop"));
 
     Box* rtn = NULL;
-    if (rewriter) {
+    if (rewriter.get()) {
         CallRewriteArgs srewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getReturnDestination());
         rtn = callattrInternal0<CXX>(operand, op_name, CLASS_ONLY, &srewrite_args, ArgPassSpec(0));
         if (srewrite_args.out_success && rtn)
@@ -4893,7 +4896,7 @@ extern "C" Box* getitem(Box* target, Box* slice) {
     static StatCounter slowpath_getitem("slowpath_getitem");
     slowpath_getitem.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, "getitem"));
 
     Box* rtn;
@@ -4927,7 +4930,7 @@ extern "C" Box* getitem_capi(Box* target, Box* slice) noexcept {
     static StatCounter slowpath_getitem("slowpath_getitem");
     slowpath_getitem.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, "getitem"));
 
     Box* rtn;
@@ -4956,7 +4959,7 @@ extern "C" void setitem(Box* target, Box* slice, Box* value) {
     static StatCounter slowpath_setitem("slowpath_setitem");
     slowpath_setitem.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setitem"));
 
     static BoxedString* setitem_str = internStringImmortal("__setitem__");
@@ -4992,7 +4995,7 @@ extern "C" void delitem(Box* target, Box* slice) {
     static StatCounter slowpath_delitem("slowpath_delitem");
     slowpath_delitem.log();
 
-    std::unique_ptr<Rewriter> rewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 2, "delitem"));
 
     static BoxedString* delitem_str = internStringImmortal("__delitem__");
@@ -5155,7 +5158,7 @@ extern "C" Box* createBoxedIterWrapper(Box* o) {
 extern "C" Box* createBoxedIterWrapperIfNeeded(Box* o) {
     STAT_TIMER(t0, "us_timer_slowpath_createBoxedIterWrapperIfNeeded", 10);
 
-    std::unique_ptr<Rewriter> rewriter(Rewriter::createRewriter(
+    gc::UniqueScanningHandle<Rewriter> rewriter(Rewriter::createRewriter(
         __builtin_extract_return_addr(__builtin_return_address(0)), 1, "createBoxedIterWrapperIfNeeded"));
 
     static BoxedString* hasnext_str = internStringImmortal("__hasnext__");
@@ -5646,7 +5649,7 @@ extern "C" Box* getGlobal(Box* globals, BoxedString* name) {
     }
 
     { /* anonymous scope to make sure destructors get run before we err out */
-        std::unique_ptr<Rewriter> rewriter(
+        gc::UniqueScanningHandle<Rewriter> rewriter(
             Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "getGlobal"));
 
         Box* r;