llvm JIT: refcount safety for signals

The IR we generated looked something like this:

1: i64 %a = doIC()
2: checkPendingCalls()
3: Box* %b = (Box*)%b

The problem is that the refcounter only knew about %b, starting on
line 3.  So for line 2, the refcounter didn't think it needed to decref
%a if an exception was thrown.

So the first thing this commit does is it adds check that when we refcount-track
a value, if it is a cast, then the cast must be right next to the thing it is casting.
This is stricter than we need but it should be easy enough to do and be sufficient.

Another approach would have been to have the refcount checker be able to track the
non-box-like values, ie track %a in the above example.  This would need some checking
to make sure that people didn't forget to track that.  In the end I thought this would
be simpler.

The second thing is it adds a helper function createAfter that inserts the cast in
the right location.  This could have worked by having createIC do the casting itself.
But this was a little bit tricky since some callers want to know the Instruction of the
call itself, and if createIC only returned the cast then those would get confused.  It
could have returned both, but that seemed complicated.

src/codegen/compvars.cpp

@@ -373,10 +373,10 @@ public:
             llvm_args.push_back(var->getValue());
             llvm_args.push_back(converted_slice->getValue());
 
-            llvm::Value* uncasted
+            llvm::Instruction* uncasted
                 = emitter.createIC(pp, (void*)(target_exception_style == CAPI ? pyston::getitem_capi : pyston::getitem),
                                    llvm_args, info.unw_info, target_exception_style, getNullPtr(g.llvm_value_type_ptr));
-            rtn = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+            rtn = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
             emitter.setType(rtn, RefType::OWNED);
         } else {
             rtn = emitter.createCall2(
@@ -419,9 +419,9 @@ public:
         // var has __iter__()
         emitter.setCurrentBasicBlock(bb_has_iter);
         ICSetupInfo* pp = createGenericIC(info.getTypeRecorder(), true, 128);
-        llvm::Value* uncasted = emitter.createIC(pp, (void*)pyston::createBoxedIterWrapperIfNeeded,
-                                                 { converted_iter_call->getValue() }, info.unw_info);
-        llvm::Value* value_has_iter = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+        llvm::Instruction* uncasted = emitter.createIC(pp, (void*)pyston::createBoxedIterWrapperIfNeeded,
+                                                       { converted_iter_call->getValue() }, info.unw_info);
+        llvm::Value* value_has_iter = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
         emitter.setType(value_has_iter, RefType::OWNED);
         llvm::BasicBlock* value_has_iter_bb = emitter.currentBasicBlock();
         auto has_iter_terminator = emitter.getBuilder()->CreateBr(bb_join);
@@ -475,8 +475,8 @@ public:
             llvm_args.push_back(converted_rhs->getValue());
             llvm_args.push_back(getConstantInt(op_type, g.i32));
 
-            llvm::Value* uncasted = emitter.createIC(pp, rt_func_addr, llvm_args, info.unw_info);
-            rtn = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+            llvm::Instruction* uncasted = emitter.createIC(pp, rt_func_addr, llvm_args, info.unw_info);
+            rtn = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
         } else {
             rtn = emitter.createCall3(info.unw_info, rt_func, var->getValue(), converted_rhs->getValue(),
                                       getConstantInt(op_type, g.i32));
@@ -562,9 +562,9 @@ CompilerVariable* UnknownType::getattr(IREmitter& emitter, const OpInfo& info, C
         llvm_args.push_back(var->getValue());
         llvm_args.push_back(ptr);
 
-        llvm::Value* uncasted = emitter.createIC(pp, raw_func, llvm_args, info.unw_info, target_exception_style,
-                                                 getNullPtr(g.llvm_value_type_ptr));
-        rtn_val = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+        llvm::Instruction* uncasted = emitter.createIC(pp, raw_func, llvm_args, info.unw_info, target_exception_style,
+                                                       getNullPtr(g.llvm_value_type_ptr));
+        rtn_val = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
     } else {
         rtn_val = emitter.createCall2(info.unw_info, llvm_func, var->getValue(), ptr, target_exception_style,
                                       getNullPtr(g.llvm_value_type_ptr));
@@ -666,7 +666,7 @@ _call(IREmitter& emitter, const OpInfo& info, llvm::Value* func, ExceptionStyle
 
         assert(llvm::cast<llvm::FunctionType>(llvm::cast<llvm::PointerType>(func->getType())->getElementType())
                    ->getReturnType() == g.llvm_value_type_ptr);
-        rtn = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+        rtn = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
     } else {
         // printf("\n");
         // func->dump();
@@ -792,8 +792,8 @@ ConcreteCompilerVariable* UnknownType::unaryop(IREmitter& emitter, const OpInfo&
         llvm_args.push_back(converted->getValue());
         llvm_args.push_back(getConstantInt(op_type, g.i32));
 
-        llvm::Value* uncasted = emitter.createIC(pp, (void*)pyston::unaryop, llvm_args, info.unw_info);
-        rtn = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+        llvm::Instruction* uncasted = emitter.createIC(pp, (void*)pyston::unaryop, llvm_args, info.unw_info);
+        rtn = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
     } else {
         rtn = emitter.createCall2(info.unw_info, g.funcs.unaryop, converted->getValue(),
                                   getConstantInt(op_type, g.i32));

src/codegen/irgen/irgenerator.cpp

@@ -403,7 +403,8 @@ private:
             if (exc_dest) {
                 builder.CreateInvoke(g.funcs.makePendingCalls, join_block, exc_dest);
             } else {
-                builder.CreateCall(g.funcs.makePendingCalls);
+                auto call = builder.CreateCall(g.funcs.makePendingCalls);
+                irstate->getRefcounts()->setMayThrow(call);
                 builder.CreateBr(join_block);
             }
         }
@@ -676,10 +677,10 @@ public:
 
     llvm::Value* createDeopt(AST_stmt* current_stmt, AST_expr* node, llvm::Value* node_value) override {
         ICSetupInfo* pp = createDeoptIC();
-        llvm::Value* v
+        llvm::Instruction* v
             = createIC(pp, (void*)pyston::deopt, { embedRelocatablePtr(node, g.llvm_astexpr_type_ptr), node_value },
                        UnwindInfo(current_stmt, NULL, /* is_after_deopt*/ true));
-        llvm::Value* rtn = getBuilder()->CreateIntToPtr(v, g.llvm_value_type_ptr);
+        llvm::Value* rtn = createAfter<llvm::IntToPtrInst>(v, v, g.llvm_value_type_ptr, "");
         setType(rtn, RefType::OWNED);
         return rtn;
     }
@@ -1293,8 +1294,8 @@ private:
             llvm_args.push_back(emitter.setType(embedRelocatablePtr(node->id.getBox(), g.llvm_boxedstring_type_ptr),
                                                 RefType::BORROWED));
 
-            llvm::Value* uncasted = emitter.createIC(pp, (void*)pyston::getGlobal, llvm_args, unw_info);
-            llvm::Value* r = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
+            llvm::Instruction* uncasted = emitter.createIC(pp, (void*)pyston::getGlobal, llvm_args, unw_info);
+            llvm::Value* r = createAfter<llvm::IntToPtrInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
             emitter.setType(r, RefType::OWNED);
             return new ConcreteCompilerVariable(UNKNOWN, r);
         } else {
@@ -1432,9 +1433,9 @@ private:
         ConcreteCompilerVariable* cvar = var->makeConverted(emitter, var->getBoxType());
 
         std::vector<llvm::Value*> args{ cvar->getValue() };
-        llvm::Value* rtn = emitter.createCall(unw_info, g.funcs.repr, args);
-        emitter.setType(rtn, RefType::BORROWED); // Well, really it's owned, and we handoff the ref to the bitcast
-        rtn = emitter.getBuilder()->CreateBitCast(rtn, g.llvm_value_type_ptr);
+        llvm::Instruction* uncasted = emitter.createCall(unw_info, g.funcs.repr, args);
+        emitter.setType(uncasted, RefType::BORROWED); // Well, really it's owned, and we handoff the ref to the bitcast
+        auto rtn = createAfter<llvm::BitCastInst>(uncasted, uncasted, g.llvm_value_type_ptr, "");
         emitter.setType(rtn, RefType::OWNED);
 
         return new ConcreteCompilerVariable(STR, rtn);

src/codegen/irgen/refcounts.cpp

@@ -53,6 +53,26 @@ static int numPredecessors(llvm::BasicBlock* b) {
 llvm::Value* RefcountTracker::setType(llvm::Value* v, RefType reftype) {
     assert(!llvm::isa<llvm::UndefValue>(v));
 
+    // Force tracked cast expressions to be immediately after the thing they cast.
+    // Otherwise there is the opportunity for things to happen between them, which
+    // may cause the refcount state to be examined, and the setType() call will not
+    // be seen yet.
+    //
+    // We could relax this restriction by looking through the cast, or by requiring
+    // the caller to also call setType() on the uncasted value.  This is a simpler
+    // fix for now though.
+    if (llvm::CastInst* cast = llvm::dyn_cast<llvm::CastInst>(v)) {
+        auto uncasted = cast->getOperand(0);
+        auto uncasted_inst = llvm::cast<llvm::Instruction>(uncasted);
+        auto uncasted_invoke = llvm::dyn_cast<llvm::InvokeInst>(uncasted_inst);
+        if (uncasted_invoke)
+            assert(uncasted_invoke->getNormalDest()->getFirstNonPHI() == cast
+                   && "Refcount-tracked casts must be immediately after the value they cast");
+        else
+            assert(uncasted_inst->getNextNode() == cast
+                   && "Refcount-tracked casts must be immediately after the value they cast");
+    }
+
     auto& var = this->vars[v];
 
     assert(var.reftype == reftype || var.reftype == RefType::UNKNOWN);
@@ -898,6 +918,7 @@ void RefcountTracker::addRefcounts(IRGenState* irstate) {
                 if (state.ending_refs[inst] != starting_refs) {
                     llvm::Instruction* insertion_pt = NULL;
                     llvm::BasicBlock* insertion_block = NULL, * insertion_from_block = NULL;
+                    assert(inst != inst->getParent()->getTerminator());
                     insertion_pt = inst->getNextNode();
                     while (llvm::isa<llvm::PHINode>(insertion_pt)) {
                         insertion_pt = insertion_pt->getNextNode();

src/codegen/irgen/util.h

@@ -19,6 +19,8 @@
 
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Instructions.h"
 
 namespace llvm {
 class Constant;
@@ -46,6 +48,20 @@ const void* getValueOfRelocatableSym(const std::string& str);
 void visitRelocatableSymsMap(gc::GCVisitor* visitor);
 
 void dumpPrettyIR(llvm::Function* f);
+
+// Insert an instruction at the first valid point *after* the given instruction.
+// The non-triviality of this is that if the given instruction is an invoke, we have
+// to be careful about where we place the new instruction -- this puts it on the
+// normal-case destination.
+//
+// Note: I wish the `create_after` argument could be placed after the `Args... args` one.
+// And I think that that should be valid, but clang doesn't seem to be accepting it.
+template <typename T, typename... Args> T* createAfter(llvm::Instruction* create_after, Args... args) {
+    if (llvm::InvokeInst* ii = llvm::dyn_cast<llvm::InvokeInst>(create_after)) {
+        return new T(args..., ii->getNormalDest()->getFirstInsertionPt());
+    } else
+        return new T(args..., create_after->getNextNode());
+}
 }
 
 #endif