Check CAPI exceptions before signals

Previously we would check signals first.  Which means that we would then
call into a signal handler with an active exception, which would later
trigger asserts.

For CXX functions, the exception automatically wins over the signal checking.
CPython also checks signals first.

The only tricky thing is that this was happening because the signals stuff
was hooked deeper down the stack.  So pass down the CAPI-exception data as well.

src/codegen/compvars.cpp

@@ -347,10 +347,9 @@ public:
                     cstop = slice_val.stop ? slice_val.stop->makeConverted(emitter, UNKNOWN)->getValue()
                                            : emitter.setType(getNullPtr(g.llvm_value_type_ptr), RefType::BORROWED);
 
-                    llvm::Value* r
-                        = emitter.createCall3(info.unw_info, g.funcs.apply_slice, var->getValue(), cstart, cstop);
+                    llvm::Value* r = emitter.createCall3(info.unw_info, g.funcs.apply_slice, var->getValue(), cstart,
+                                                         cstop, CAPI, getNullPtr(g.llvm_value_type_ptr));
                     emitter.setType(r, RefType::OWNED);
-                    emitter.checkAndPropagateCapiException(info.unw_info, r, getNullPtr(g.llvm_value_type_ptr));
 
                     return new ConcreteCompilerVariable(static_cast<ConcreteCompilerType*>(return_type), r);
                 } else {
@@ -375,20 +374,18 @@ public:
 
             llvm::Value* uncasted
                 = emitter.createIC(pp, (void*)(target_exception_style == CAPI ? pyston::getitem_capi : pyston::getitem),
-                                   llvm_args, info.unw_info, target_exception_style);
+                                   llvm_args, info.unw_info, target_exception_style, getNullPtr(g.llvm_value_type_ptr));
             rtn = emitter.getBuilder()->CreateIntToPtr(uncasted, g.llvm_value_type_ptr);
             emitter.setType(rtn, RefType::OWNED);
         } else {
-            rtn = emitter.createCall2(info.unw_info,
-                                      target_exception_style == CAPI ? g.funcs.getitem_capi : g.funcs.getitem,
-                                      var->getValue(), converted_slice->getValue(), target_exception_style);
+            rtn = emitter.createCall2(
+                info.unw_info, target_exception_style == CAPI ? g.funcs.getitem_capi : g.funcs.getitem, var->getValue(),
+                converted_slice->getValue(), target_exception_style, getNullPtr(g.llvm_value_type_ptr));
             emitter.setType(rtn, RefType::OWNED);
         }
 
-        if (target_exception_style == CAPI) {
+        if (target_exception_style == CAPI)
             emitter.setNullable(rtn, true);
-            emitter.checkAndPropagateCapiException(info.unw_info, rtn, getNullPtr(g.llvm_value_type_ptr));
-        }
 
         return new ConcreteCompilerVariable(UNKNOWN, rtn);
     }
@@ -564,19 +561,18 @@ 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);
+        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);
     } else {
-        rtn_val = emitter.createCall2(info.unw_info, llvm_func, var->getValue(), ptr, target_exception_style);
+        rtn_val = emitter.createCall2(info.unw_info, llvm_func, var->getValue(), ptr, target_exception_style,
+                                      getNullPtr(g.llvm_value_type_ptr));
     }
 
     emitter.setType(rtn_val, RefType::OWNED);
     if (target_exception_style == CAPI)
         emitter.setNullable(rtn_val, true);
 
-    if (target_exception_style == CAPI)
-        emitter.checkAndPropagateCapiException(info.unw_info, rtn_val, getNullPtr(g.llvm_value_type_ptr));
-
     return new ConcreteCompilerVariable(UNKNOWN, rtn_val);
 }
 
@@ -664,7 +660,8 @@ static ConcreteCompilerVariable* _call(IREmitter& emitter, const OpInfo& info, l
 
         ICSetupInfo* pp = createCallsiteIC(info.getTypeRecorder(), args.size(), info.getBJitICInfo());
 
-        llvm::Instruction* uncasted = emitter.createIC(pp, func_addr, llvm_args, info.unw_info, target_exception_style);
+        llvm::Instruction* uncasted = emitter.createIC(pp, func_addr, llvm_args, info.unw_info, target_exception_style,
+                                                       getNullPtr(g.llvm_value_type_ptr));
         inst = uncasted;
 
         assert(llvm::cast<llvm::FunctionType>(llvm::cast<llvm::PointerType>(func->getType())->getElementType())
@@ -679,7 +676,8 @@ static ConcreteCompilerVariable* _call(IREmitter& emitter, const OpInfo& info, l
         //}
         // printf("%ld %ld\n", llvm_args.size(), args.size());
         // printf("\n");
-        inst = emitter.createCall(info.unw_info, func, llvm_args, target_exception_style);
+        inst = emitter.createCall(info.unw_info, func, llvm_args, target_exception_style,
+                                  getNullPtr(g.llvm_value_type_ptr));
         rtn = inst;
     }
 
@@ -690,10 +688,6 @@ static ConcreteCompilerVariable* _call(IREmitter& emitter, const OpInfo& info, l
     }
     assert(rtn->getType() == rtn_type->llvmType());
 
-    if (target_exception_style == CAPI) {
-        emitter.checkAndPropagateCapiException(info.unw_info, rtn, getNullPtr(g.llvm_value_type_ptr));
-    }
-
     for (auto v : array_passed_args)
         emitter.refUsed(v, inst);
 
@@ -1778,13 +1772,11 @@ public:
                                                                  : g.funcs.raiseAttributeErrorStrCapi;
                 llvm::CallSite call = emitter.createCall3(
                     info.unw_info, raise_func, embedRelocatablePtr(cls->tp_name, g.i8_ptr),
-                    embedRelocatablePtr(attr->data(), g.i8_ptr), getConstantInt(attr->size(), g.i64), exception_style);
-                if (exception_style == CAPI) {
-                    emitter.checkAndPropagateCapiException(info.unw_info, getNullPtr(g.llvm_value_type_ptr),
-                                                           getNullPtr(g.llvm_value_type_ptr));
-                } else {
+                    embedRelocatablePtr(attr->data(), g.i8_ptr), getConstantInt(attr->size(), g.i64), exception_style,
+                    IREmitter::ALWAYS_THROWS);
+
+                if (exception_style == CXX)
                     call.setDoesNotReturn();
-                }
                 return undefVariable();
             }
 
@@ -1841,13 +1833,10 @@ public:
                 llvm::CallSite call = emitter.createCall3(
                     info.unw_info, raise_func, embedRelocatablePtr(cls->tp_name, g.i8_ptr),
                     emitter.setType(embedRelocatablePtr(attr->data(), g.i8_ptr), RefType::BORROWED),
-                    getConstantInt(attr->size(), g.i64), exception_style);
-                if (exception_style == CAPI) {
-                    emitter.checkAndPropagateCapiException(info.unw_info, getNullPtr(g.llvm_value_type_ptr),
-                                                           getNullPtr(g.llvm_value_type_ptr));
-                } else {
+                    getConstantInt(attr->size(), g.i64), exception_style, IREmitter::ALWAYS_THROWS);
+
+                if (exception_style == CXX)
                     call.setDoesNotReturn();
-                }
             }
             return undefVariable();
         }
@@ -2087,9 +2076,9 @@ public:
                         g.llvm_value_type_ptr, { g.llvm_value_type_ptr, g.i64, g.i64 }, false);
                     llvm::Value* r = emitter.createCall3(
                         info.unw_info, embedConstantPtr((void*)PySequence_GetSlice, ft->getPointerTo()),
-                        var->getValue(), start, stop);
+                        var->getValue(), start, stop, CAPI, getNullPtr(g.llvm_value_type_ptr));
                     emitter.setType(r, RefType::OWNED);
-                    emitter.checkAndPropagateCapiException(info.unw_info, r, getNullPtr(g.llvm_value_type_ptr));
+                    emitter.setNullable(r, true);
 
                     return new ConcreteCompilerVariable(static_cast<ConcreteCompilerType*>(return_type), r);
                 }
@@ -2561,10 +2550,9 @@ public:
                     ExceptionStyle target_exception_style = info.preferredExceptionStyle();
 
                     if (target_exception_style == CAPI) {
-                        llvm::CallSite call = emitter.createCall(info.unw_info, g.funcs.raiseIndexErrorStrCapi,
-                                                                 embedConstantPtr("tuple", g.i8_ptr), CAPI);
-                        emitter.checkAndPropagateCapiException(info.unw_info, getNullPtr(g.llvm_value_type_ptr),
-                                                               getNullPtr(g.llvm_value_type_ptr));
+                        llvm::CallSite call
+                            = emitter.createCall(info.unw_info, g.funcs.raiseIndexErrorStrCapi,
+                                                 embedConstantPtr("tuple", g.i8_ptr), CAPI, IREmitter::ALWAYS_THROWS);
                     } else {
                         llvm::CallSite call = emitter.createCall(info.unw_info, g.funcs.raiseIndexErrorStr,
                                                                  embedConstantPtr("tuple", g.i8_ptr), CXX);

src/codegen/irgen.h

@@ -86,23 +86,32 @@ public:
 
     virtual llvm::Function* getIntrinsic(llvm::Intrinsic::ID) = 0;
 
+    // Special value for capi_exc_value that says that the target function always sets a capi exception.
+    static llvm::Value* ALWAYS_THROWS;
+
     virtual llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee,
                                           const std::vector<llvm::Value*>& args,
-                                          ExceptionStyle target_exception_style = CXX) = 0;
+                                          ExceptionStyle target_exception_style = CXX,
+                                          llvm::Value* capi_exc_value = NULL) = 0;
     virtual llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee,
-                                          ExceptionStyle target_exception_style = CXX) = 0;
+                                          ExceptionStyle target_exception_style = CXX,
+                                          llvm::Value* capi_exc_value = NULL) = 0;
     virtual llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
-                                          ExceptionStyle target_exception_style = CXX) = 0;
+                                          ExceptionStyle target_exception_style = CXX,
+                                          llvm::Value* capi_exc_value = NULL) = 0;
     virtual llvm::Instruction* createCall2(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
-                                           llvm::Value* arg2, ExceptionStyle target_exception_style = CXX) = 0;
+                                           llvm::Value* arg2, ExceptionStyle target_exception_style = CXX,
+                                           llvm::Value* capi_exc_value = NULL) = 0;
     virtual llvm::Instruction* createCall3(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
                                            llvm::Value* arg2, llvm::Value* arg3,
-                                           ExceptionStyle target_exception_style = CXX) = 0;
+                                           ExceptionStyle target_exception_style = CXX,
+                                           llvm::Value* capi_exc_value = NULL) = 0;
     virtual llvm::Instruction* createIC(const ICSetupInfo* pp, void* func_addr, const std::vector<llvm::Value*>& args,
-                                        const UnwindInfo& unw_info, ExceptionStyle target_exception_style = CXX) = 0;
+                                        const UnwindInfo& unw_info, ExceptionStyle target_exception_style = CXX,
+                                        llvm::Value* capi_exc_value = NULL) = 0;
 
-    virtual void checkAndPropagateCapiException(const UnwindInfo& unw_info, llvm::Value* returned_val,
-                                                llvm::Value* exc_val, bool double_check = false) = 0;
+    // virtual void checkAndPropagateCapiException(const UnwindInfo& unw_info, llvm::Value* returned_val,
+    // llvm::Value* exc_val, bool double_check = false) = 0;
 
     virtual llvm::Value* createDeopt(AST_stmt* current_stmt, AST_expr* node, llvm::Value* node_value) = 0;
 

src/codegen/irgen/irgenerator.cpp

@@ -353,6 +353,8 @@ llvm::Value* IRGenState::getGlobalsIfCustom() {
 // XXX This is pretty hacky, but I think I can get rid of it once I merge in Marius's new frame introspection work
 #define NO_CXX_INTERCEPTION ((llvm::BasicBlock*)-1)
 
+llvm::Value* IREmitter::ALWAYS_THROWS = ((llvm::Value*)1);
+
 class IREmitterImpl : public IREmitter {
 private:
     IRGenState* irstate;
@@ -404,10 +406,39 @@ private:
         setCurrentBasicBlock(join_block);
     }
 
+    void checkAndPropagateCapiException(const UnwindInfo& unw_info, llvm::Value* returned_val, llvm::Value* exc_val,
+                                        bool double_check = false) {
+        assert(!double_check); // need to call PyErr_Occurred
+
+        assert(exc_val);
+
+        llvm::BasicBlock* normal_dest
+            = llvm::BasicBlock::Create(g.context, curblock->getName(), irstate->getLLVMFunction());
+        normal_dest->moveAfter(curblock);
+
+        llvm::BasicBlock* exc_dest = irgenerator->getCAPIExcDest(curblock, unw_info.exc_dest, unw_info.current_stmt);
+
+        if (exc_val == ALWAYS_THROWS) {
+            assert(returned_val->getType() == g.void_);
+
+            llvm::BasicBlock* exc_dest = irgenerator->getCAPIExcDest(curblock, unw_info.exc_dest, unw_info.current_stmt);
+            getBuilder()->CreateBr(exc_dest);
+        } else {
+            assert(returned_val->getType() == exc_val->getType());
+            llvm::Value* check_val = getBuilder()->CreateICmpEQ(returned_val, exc_val);
+            llvm::BranchInst* nullcheck = getBuilder()->CreateCondBr(check_val, exc_dest, normal_dest);
+        }
+
+        setCurrentBasicBlock(normal_dest);
+    }
+
     llvm::CallSite emitCall(const UnwindInfo& unw_info, llvm::Value* callee, const std::vector<llvm::Value*>& args,
-                            ExceptionStyle target_exception_style) {
+                            ExceptionStyle target_exception_style, llvm::Value* capi_exc_value) {
         emitSetCurrentStmt(unw_info.current_stmt);
 
+        if (target_exception_style == CAPI)
+            assert(capi_exc_value);
+
         bool needs_cxx_interception;
         if (unw_info.exc_dest == NO_CXX_INTERCEPTION) {
             needs_cxx_interception = false;
@@ -434,6 +465,8 @@ private:
 
             llvm::InvokeInst* rtn = getBuilder()->CreateInvoke(callee, normal_dest, exc_dest, args);
 
+            ASSERT(target_exception_style == CXX, "otherwise need to call checkAndPropagateCapiException");
+
             // Note -- this code can often create critical edges between LLVM blocks.
             // The refcounting system has some support for handling this, but if we start generating
             // IR that it can't handle, we might have to break the critical edges here (or teach the
@@ -448,8 +481,11 @@ private:
                 emitPendingCallsCheck(NULL);
             return rtn;
         } else {
-
             llvm::CallInst* cs = getBuilder()->CreateCall(callee, args);
+
+            if (target_exception_style == CAPI)
+                checkAndPropagateCapiException(unw_info, cs, capi_exc_value);
+
             emitPendingCallsCheck(NULL);
             return cs;
         }
@@ -458,10 +494,13 @@ private:
     llvm::CallSite emitPatchpoint(llvm::Type* return_type, const ICSetupInfo* pp, llvm::Value* func,
                                   const std::vector<llvm::Value*>& args,
                                   const std::vector<llvm::Value*>& ic_stackmap_args, const UnwindInfo& unw_info,
-                                  ExceptionStyle target_exception_style) {
+                                  ExceptionStyle target_exception_style, llvm::Value* capi_exc_value) {
         if (pp == NULL)
             assert(ic_stackmap_args.size() == 0);
 
+        if (target_exception_style == CAPI)
+            assert(capi_exc_value);
+
         // Retrieve address of called function, currently handles the IR
         // embedConstantPtr() and embedRelocatablePtr() create.
         void* func_addr = nullptr;
@@ -504,6 +543,9 @@ private:
         llvm::Intrinsic::ID intrinsic_id;
         if (return_type->isIntegerTy() || return_type->isPointerTy()) {
             intrinsic_id = llvm::Intrinsic::experimental_patchpoint_i64;
+
+            if (capi_exc_value && capi_exc_value->getType()->isPointerTy())
+                capi_exc_value = getBuilder()->CreatePtrToInt(capi_exc_value, g.i64);
         } else if (return_type->isVoidTy()) {
             intrinsic_id = llvm::Intrinsic::experimental_patchpoint_void;
         } else if (return_type->isDoubleTy()) {
@@ -513,7 +555,7 @@ private:
             abort();
         }
         llvm::Function* patchpoint = this->getIntrinsic(intrinsic_id);
-        llvm::CallSite rtn = this->emitCall(unw_info, patchpoint, pp_args, target_exception_style);
+        llvm::CallSite rtn = this->emitCall(unw_info, patchpoint, pp_args, target_exception_style, capi_exc_value);
         return rtn;
     }
 
@@ -564,7 +606,7 @@ public:
 
     llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee,
                                   const std::vector<llvm::Value*>& args,
-                                  ExceptionStyle target_exception_style = CXX) override {
+                                  ExceptionStyle target_exception_style = CXX, llvm::Value* capi_exc_value = NULL) override {
 #ifndef NDEBUG
         // Copied the argument-type-checking from CallInst::init, since the patchpoint arguments don't
         // get checked.
@@ -582,36 +624,42 @@ public:
             }
         }
 #endif
-        return emitCall(unw_info, callee, args, target_exception_style).getInstruction();
+        return emitCall(unw_info, callee, args, target_exception_style, capi_exc_value).getInstruction();
     }
 
     llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee,
-                            ExceptionStyle target_exception_style = CXX) override {
-        return createCall(unw_info, callee, std::vector<llvm::Value*>(), target_exception_style);
+                                  ExceptionStyle target_exception_style = CXX,
+                                  llvm::Value* capi_exc_value = NULL) override {
+        return createCall(unw_info, callee, std::vector<llvm::Value*>(), target_exception_style, capi_exc_value);
     }
 
     llvm::Instruction* createCall(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
-                            ExceptionStyle target_exception_style = CXX) override {
-        return createCall(unw_info, callee, std::vector<llvm::Value*>({ arg1 }), target_exception_style);
+                                  ExceptionStyle target_exception_style = CXX,
+                                  llvm::Value* capi_exc_value = NULL) override {
+        return createCall(unw_info, callee, std::vector<llvm::Value*>({ arg1 }), target_exception_style,
+                          capi_exc_value);
     }
 
-    llvm::Instruction* createCall2(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1, llvm::Value* arg2,
-                             ExceptionStyle target_exception_style = CXX) override {
-        return createCall(unw_info, callee, { arg1, arg2 }, target_exception_style);
+    llvm::Instruction* createCall2(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
+                                   llvm::Value* arg2, ExceptionStyle target_exception_style = CXX,
+                                   llvm::Value* capi_exc_value = NULL) override {
+        return createCall(unw_info, callee, { arg1, arg2 }, target_exception_style, capi_exc_value);
     }
 
-    llvm::Instruction* createCall3(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1, llvm::Value* arg2,
-                             llvm::Value* arg3, ExceptionStyle target_exception_style = CXX) override {
-        return createCall(unw_info, callee, { arg1, arg2, arg3 }, target_exception_style);
+    llvm::Instruction* createCall3(const UnwindInfo& unw_info, llvm::Value* callee, llvm::Value* arg1,
+                                   llvm::Value* arg2, llvm::Value* arg3, ExceptionStyle target_exception_style = CXX,
+                                   llvm::Value* capi_exc_value = NULL) override {
+        return createCall(unw_info, callee, { arg1, arg2, arg3 }, target_exception_style, capi_exc_value);
     }
 
     llvm::Instruction* createIC(const ICSetupInfo* pp, void* func_addr, const std::vector<llvm::Value*>& args,
-                          const UnwindInfo& unw_info, ExceptionStyle target_exception_style = CXX) override {
+                                const UnwindInfo& unw_info, ExceptionStyle target_exception_style = CXX,
+                                llvm::Value* capi_exc_value = NULL) override {
         std::vector<llvm::Value*> stackmap_args;
 
         llvm::CallSite rtn = emitPatchpoint(pp->hasReturnValue() ? g.i64 : g.void_, pp,
                                             embedConstantPtr(func_addr, g.i8->getPointerTo()), args, stackmap_args,
-                                            unw_info, target_exception_style);
+                                            unw_info, target_exception_style, capi_exc_value);
 
         rtn.setCallingConv(pp->getCallingConvention());
         return rtn.getInstruction();
@@ -627,23 +675,6 @@ public:
         return rtn;
     }
 
-    void checkAndPropagateCapiException(const UnwindInfo& unw_info, llvm::Value* returned_val, llvm::Value* exc_val,
-                                        bool double_check = false) override {
-        assert(!double_check); // need to call PyErr_Occurred
-
-        llvm::BasicBlock* normal_dest
-            = llvm::BasicBlock::Create(g.context, curblock->getName(), irstate->getLLVMFunction());
-        normal_dest->moveAfter(curblock);
-
-        llvm::BasicBlock* exc_dest = irgenerator->getCAPIExcDest(curblock, unw_info.exc_dest, unw_info.current_stmt);
-
-        assert(returned_val->getType() == exc_val->getType());
-        llvm::Value* check_val = getBuilder()->CreateICmpEQ(returned_val, exc_val);
-        llvm::BranchInst* nullcheck = getBuilder()->CreateCondBr(check_val, exc_dest, normal_dest);
-
-        setCurrentBasicBlock(normal_dest);
-    }
-
     Box* getIntConstant(int64_t n) override {
         return irstate->getSourceInfo()->parent_module->getIntConstant(n);
     }
@@ -2424,9 +2455,7 @@ private:
 
             llvm::Value* exc_info = emitter.getBuilder()->CreateConstInBoundsGEP2_32(irstate->getFrameInfoVar(), 0, 0);
             if (target_exception_style == CAPI) {
-                emitter.createCall(unw_info, g.funcs.raise0_capi, exc_info, CAPI);
-                emitter.checkAndPropagateCapiException(unw_info, getNullPtr(g.llvm_value_type_ptr),
-                                                       getNullPtr(g.llvm_value_type_ptr));
+                emitter.createCall(unw_info, g.funcs.raise0_capi, exc_info, CAPI, IREmitter::ALWAYS_THROWS);
                 emitter.getBuilder()->CreateUnreachable();
             } else {
                 emitter.createCall(unw_info, g.funcs.raise0, exc_info);
@@ -2450,9 +2479,7 @@ private:
 
         llvm::Instruction* inst;
         if (target_exception_style == CAPI) {
-            inst = emitter.createCall(unw_info, g.funcs.raise3_capi, args, CAPI);
-            emitter.checkAndPropagateCapiException(unw_info, getNullPtr(g.llvm_value_type_ptr),
-                                                   getNullPtr(g.llvm_value_type_ptr));
+            inst = emitter.createCall(unw_info, g.funcs.raise3_capi, args, CAPI, IREmitter::ALWAYS_THROWS);
         } else {
             inst = emitter.createCall(unw_info, g.funcs.raise3, args, CXX);
         }