[llvm] r371262 - [IR] CallBrInst: scan+update arg list when indirect dest list changes
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 9 01:58:40 PDT 2019
Merged to release_90 in r371376.
Please make sure to let me know if there are any follow-ups or issues
related to this.
On Fri, Sep 6, 2019 at 11:48 PM Nick Desaulniers via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> Author: nickdesaulniers
> Date: Fri Sep 6 14:50:11 2019
> New Revision: 371262
>
> URL: http://llvm.org/viewvc/llvm-project?rev=371262&view=rev
> Log:
> [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
>
> Modified:
> llvm/trunk/include/llvm/IR/Instructions.h
> llvm/trunk/lib/IR/Instructions.cpp
> llvm/trunk/unittests/IR/InstructionsTest.cpp
>
> Modified: llvm/trunk/include/llvm/IR/Instructions.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=371262&r1=371261&r2=371262&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Instructions.h (original)
> +++ llvm/trunk/include/llvm/IR/Instructions.h Fri Sep 6 14:50:11 2019
> @@ -3942,6 +3942,9 @@ class CallBrInst : public CallBase {
> ArrayRef<BasicBlock *> IndirectDests, ArrayRef<Value *> Args,
> ArrayRef<OperandBundleDef> 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<BasicBlock>(*(&Op<-1>() - getNumIndirectDests() - 1));
> }
> BasicBlock *getIndirectDest(unsigned i) const {
> - return cast<BasicBlock>(*(&Op<-1>() - getNumIndirectDests() + i));
> + return cast_or_null<BasicBlock>(*(&Op<-1>() - getNumIndirectDests() + i));
> }
> SmallVector<BasicBlock *, 16> getIndirectDests() const {
> SmallVector<BasicBlock *, 16> IndirectDests;
> @@ -4091,6 +4094,7 @@ public:
> *(&Op<-1>() - getNumIndirectDests() - 1) = reinterpret_cast<Value *>(B);
> }
> void setIndirectDest(unsigned i, BasicBlock *B) {
> + updateArgBlockAddresses(i, B);
> *(&Op<-1>() - getNumIndirectDests() + i) = reinterpret_cast<Value *>(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<Value *>(NewSucc);
> + return i == 0 ? setDefaultDest(NewSucc) : setIndirectDest(i - 1, NewSucc);
> }
>
> unsigned getNumSuccessors() const { return getNumIndirectDests() + 1; }
>
> Modified: llvm/trunk/lib/IR/Instructions.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instructions.cpp?rev=371262&r1=371261&r2=371262&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Instructions.cpp (original)
> +++ llvm/trunk/lib/IR/Instructions.cpp Fri Sep 6 14:50:11 2019
> @@ -822,6 +822,17 @@ void CallBrInst::init(FunctionType *FTy,
> 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<BlockAddress>(getArgOperand(ArgNo)) == Old)
> + setArgOperand(ArgNo, New);
> + }
> +}
> +
> CallBrInst::CallBrInst(const CallBrInst &CBI)
> : CallBase(CBI.Attrs, CBI.FTy, CBI.getType(), Instruction::CallBr,
> OperandTraits<CallBase>::op_end(this) - CBI.getNumOperands(),
>
> Modified: llvm/trunk/unittests/IR/InstructionsTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=371262&r1=371261&r2=371262&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)
> +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Fri Sep 6 14:50:11 2019
> @@ -1061,5 +1061,56 @@ TEST(InstructionsTest, FNegInstruction)
> FNeg->deleteValue();
> }
>
> +TEST(InstructionsTest, CallBrInstruction) {
> + LLVMContext Context;
> + std::unique_ptr<Module> 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<CallBrInst>(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<BlockAddress>(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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list