From: Nick Desaulniers Date: Fri, 6 Sep 2019 21:50:11 +0000 (+0000) Subject: [IR] CallBrInst: scan+update arg list when indirect dest list changes X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=aaff490524e497661820debab4be5146e5065fe6;p=llvm [IR] CallBrInst: scan+update arg list when indirect dest list changes Summary: There's an unspoken invariant of callbr that the list of BlockAddress Constants in the "function args" list match the BasicBlocks in the "other labels" list. (This invariant is being added to the LangRef in https://reviews.llvm.org/D67196). When modifying the any of the indirect destinations of a callbr instruction (possible jump targets), we need to update the function arguments if the argument is a BlockAddress whose BasicBlock refers to the indirect destination BasicBlock being replaced. Otherwise, many transforms that modify successors will end up violating that invariant. A recent change to the arm64 Linux kernel exposed this bug, which prevents the kernel from booting. I considered maintaining a mapping from indirect destination BasicBlock to argument operand BlockAddress, but this ends up being a one to potentially many (though usually one) mapping. Also, the list of arguments to a function (or more typically inline assembly) ends up being less than 10. The implementation is significantly simpler to just rescan the full list of arguments. Because of the one to potentially many relationship, the full arg list must be scanned (we can't stop at the first instance). Thanks to the following folks that reported the issue and helped debug it: * Nathan Chancellor * Will Deacon * Andrew Murray * Craig Topper Link: https://bugs.llvm.org/show_bug.cgi?id=43222 Link: https://github.com/ClangBuiltLinux/linux/issues/649 Link: https://lists.infradead.org/pipermail/linux-arm-kernel/2019-September/678330.html Reviewers: craig.topper, chandlerc Reviewed By: craig.topper Subscribers: void, javed.absar, kristof.beyls, hiraditya, llvm-commits, nathanchance, srhines Tags: #llvm Differential Revision: https://reviews.llvm.org/D67252 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@371262 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/llvm/IR/Instructions.h b/include/llvm/IR/Instructions.h index b7e1d0fa18a..009ec25a65a 100644 --- a/include/llvm/IR/Instructions.h +++ b/include/llvm/IR/Instructions.h @@ -3942,6 +3942,9 @@ class CallBrInst : public CallBase { ArrayRef IndirectDests, ArrayRef Args, ArrayRef Bundles, const Twine &NameStr); + /// Should the Indirect Destinations change, scan + update the Arg list. + void updateArgBlockAddresses(unsigned i, BasicBlock *B); + /// Compute the number of operands to allocate. static int ComputeNumOperands(int NumArgs, int NumIndirectDests, int NumBundleInputs = 0) { @@ -4079,7 +4082,7 @@ public: return cast(*(&Op<-1>() - getNumIndirectDests() - 1)); } BasicBlock *getIndirectDest(unsigned i) const { - return cast(*(&Op<-1>() - getNumIndirectDests() + i)); + return cast_or_null(*(&Op<-1>() - getNumIndirectDests() + i)); } SmallVector getIndirectDests() const { SmallVector IndirectDests; @@ -4091,6 +4094,7 @@ public: *(&Op<-1>() - getNumIndirectDests() - 1) = reinterpret_cast(B); } void setIndirectDest(unsigned i, BasicBlock *B) { + updateArgBlockAddresses(i, B); *(&Op<-1>() - getNumIndirectDests() + i) = reinterpret_cast(B); } @@ -4100,11 +4104,10 @@ public: return i == 0 ? getDefaultDest() : getIndirectDest(i - 1); } - void setSuccessor(unsigned idx, BasicBlock *NewSucc) { - assert(idx < getNumIndirectDests() + 1 && + void setSuccessor(unsigned i, BasicBlock *NewSucc) { + assert(i < getNumIndirectDests() + 1 && "Successor # out of range for callbr!"); - *(&Op<-1>() - getNumIndirectDests() -1 + idx) = - reinterpret_cast(NewSucc); + return i == 0 ? setDefaultDest(NewSucc) : setIndirectDest(i - 1, NewSucc); } unsigned getNumSuccessors() const { return getNumIndirectDests() + 1; } diff --git a/lib/IR/Instructions.cpp b/lib/IR/Instructions.cpp index eb2f8827b11..18d7719ecb3 100644 --- a/lib/IR/Instructions.cpp +++ b/lib/IR/Instructions.cpp @@ -822,6 +822,17 @@ void CallBrInst::init(FunctionType *FTy, Value *Fn, BasicBlock *Fallthrough, setName(NameStr); } +void CallBrInst::updateArgBlockAddresses(unsigned i, BasicBlock *B) { + assert(getNumIndirectDests() > i && "IndirectDest # out of range for callbr"); + if (BasicBlock *OldBB = getIndirectDest(i)) { + BlockAddress *Old = BlockAddress::get(OldBB); + BlockAddress *New = BlockAddress::get(B); + for (unsigned ArgNo = 0, e = getNumArgOperands(); ArgNo != e; ++ArgNo) + if (dyn_cast(getArgOperand(ArgNo)) == Old) + setArgOperand(ArgNo, New); + } +} + CallBrInst::CallBrInst(const CallBrInst &CBI) : CallBase(CBI.Attrs, CBI.FTy, CBI.getType(), Instruction::CallBr, OperandTraits::op_end(this) - CBI.getNumOperands(), diff --git a/unittests/IR/InstructionsTest.cpp b/unittests/IR/InstructionsTest.cpp index 9b9efe33cfe..b2ea3844d33 100644 --- a/unittests/IR/InstructionsTest.cpp +++ b/unittests/IR/InstructionsTest.cpp @@ -1061,5 +1061,56 @@ TEST(InstructionsTest, FNegInstruction) { FNeg->deleteValue(); } +TEST(InstructionsTest, CallBrInstruction) { + LLVMContext Context; + std::unique_ptr M = parseIR(Context, R"( +define void @foo() { +entry: + callbr void asm sideeffect "// XXX: ${0:l}", "X"(i8* blockaddress(@foo, %branch_test.exit)) + to label %land.rhs.i [label %branch_test.exit] + +land.rhs.i: + br label %branch_test.exit + +branch_test.exit: + %0 = phi i1 [ true, %entry ], [ false, %land.rhs.i ] + br i1 %0, label %if.end, label %if.then + +if.then: + ret void + +if.end: + ret void +} +)"); + Function *Foo = M->getFunction("foo"); + auto BBs = Foo->getBasicBlockList().begin(); + CallBrInst &CBI = cast(BBs->front()); + ++BBs; + ++BBs; + BasicBlock &BranchTestExit = *BBs; + ++BBs; + BasicBlock &IfThen = *BBs; + + // Test that setting the first indirect destination of callbr updates the dest + EXPECT_EQ(&BranchTestExit, CBI.getIndirectDest(0)); + CBI.setIndirectDest(0, &IfThen); + EXPECT_EQ(&IfThen, CBI.getIndirectDest(0)); + + // Further, test that changing the indirect destination updates the arg + // operand to use the block address of the new indirect destination basic + // block. This is a critical invariant of CallBrInst. + BlockAddress *IndirectBA = BlockAddress::get(CBI.getIndirectDest(0)); + BlockAddress *ArgBA = cast(CBI.getArgOperand(0)); + EXPECT_EQ(IndirectBA, ArgBA) + << "After setting the indirect destination, callbr had an indirect " + "destination of '" + << CBI.getIndirectDest(0)->getName() << "', but a argument of '" + << ArgBA->getBasicBlock()->getName() << "'. These should always match:\n" + << CBI; + EXPECT_EQ(IndirectBA->getBasicBlock(), &IfThen); + EXPECT_EQ(ArgBA->getBasicBlock(), &IfThen); +} + } // end anonymous namespace } // end namespace llvm