From d35366bccae0016418660337ce94e3d7d0ff391e Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Sat, 7 Nov 2020 11:19:27 +0100 Subject: [PATCH] [CaptureTracking] Correctly handle multiple uses in one instruction If the same value is used multiple times in the same instruction, CaptureTracking may end up reporting the wrong use as being captured, and/or report the same use as being captured multiple times. Make sure that all checks take the use operand number into account, rather than performing unreliable comparisons against the used value. I'm not sure whether this can cause any problems in practice, but at least some capture trackers (ArgUsesTracker, AACaptureUseTracker) do care about which call argument is captured. --- llvm/lib/Analysis/CaptureTracking.cpp | 27 +++++------ llvm/unittests/Analysis/CaptureTrackingTest.cpp | 59 +++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp index 0cb9069..ec997f8 100644 --- a/llvm/lib/Analysis/CaptureTracking.cpp +++ b/llvm/lib/Analysis/CaptureTracking.cpp @@ -254,7 +254,6 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, while (!Worklist.empty()) { const Use *U = Worklist.pop_back_val(); Instruction *I = cast(U->getUser()); - V = U->get(); switch (I->getOpcode()) { case Instruction::Call: @@ -292,13 +291,11 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, // that loading a value from a pointer does not cause the pointer to be // captured, even though the loaded value might be the pointer itself // (think of self-referential objects). - for (auto IdxOpPair : enumerate(Call->data_ops())) { - int Idx = IdxOpPair.index(); - Value *A = IdxOpPair.value(); - if (A == V && !Call->doesNotCapture(Idx)) - // The parameter is not marked 'nocapture' - captured. - if (Tracker->captured(U)) - return; + if (Call->isDataOperand(U) && + !Call->doesNotCapture(Call->getDataOperandNo(U))) { + // The parameter is not marked 'nocapture' - captured. + if (Tracker->captured(U)) + return; } break; } @@ -312,9 +309,9 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, // "va-arg" from a pointer does not cause it to be captured. break; case Instruction::Store: - // Stored the pointer - conservatively assume it may be captured. - // Volatile stores make the address observable. - if (V == I->getOperand(0) || cast(I)->isVolatile()) + // Stored the pointer - conservatively assume it may be captured. + // Volatile stores make the address observable. + if (U->getOperandNo() == 0 || cast(I)->isVolatile()) if (Tracker->captured(U)) return; break; @@ -325,7 +322,7 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, // but the value being stored is. // Volatile stores make the address observable. auto *ARMWI = cast(I); - if (ARMWI->getValOperand() == V || ARMWI->isVolatile()) + if (U->getOperandNo() == 1 || ARMWI->isVolatile()) if (Tracker->captured(U)) return; break; @@ -337,7 +334,7 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, // but the value being stored is. // Volatile stores make the address observable. auto *ACXI = cast(I); - if (ACXI->getCompareOperand() == V || ACXI->getNewValOperand() == V || + if (U->getOperandNo() == 1 || U->getOperandNo() == 2 || ACXI->isVolatile()) if (Tracker->captured(U)) return; @@ -352,14 +349,14 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker, AddUses(I); break; case Instruction::ICmp: { - unsigned Idx = (I->getOperand(0) == V) ? 0 : 1; + unsigned Idx = U->getOperandNo(); unsigned OtherIdx = 1 - Idx; if (auto *CPN = dyn_cast(I->getOperand(OtherIdx))) { // Don't count comparisons of a no-alias return value against null as // captures. This allows us to ignore comparisons of malloc results // with null, for example. if (CPN->getType()->getAddressSpace() == 0) - if (isNoAliasCall(V->stripPointerCasts())) + if (isNoAliasCall(U->get()->stripPointerCasts())) break; if (!I->getFunction()->nullPointerIsDefined()) { auto *O = I->getOperand(Idx)->stripPointerCastsSameRepresentation(); diff --git a/llvm/unittests/Analysis/CaptureTrackingTest.cpp b/llvm/unittests/Analysis/CaptureTrackingTest.cpp index e4c8a9f..b8257c4 100644 --- a/llvm/unittests/Analysis/CaptureTrackingTest.cpp +++ b/llvm/unittests/Analysis/CaptureTrackingTest.cpp @@ -73,3 +73,62 @@ TEST(CaptureTracking, MaxUsesToExplore) { Test("test_few_uses", 6, 4); Test("test_many_uses", 50, 30); } + +struct CollectingCaptureTracker : public CaptureTracker { + SmallVector Captures; + void tooManyUses() { } + bool captured(const Use *U) override { + Captures.push_back(U); + return false; + } +}; + +TEST(CaptureTracking, MultipleUsesInSameInstruction) { + StringRef Assembly = R"( + declare void @call(i8*, i8*, i8*) + + define void @test(i8* %arg, i8** %ptr) { + call void @call(i8* %arg, i8* nocapture %arg, i8* %arg) [ "bundle"(i8* %arg) ] + cmpxchg i8** %ptr, i8* %arg, i8* %arg acq_rel monotonic + icmp eq i8* %arg, %arg + ret void + } + )"; + + LLVMContext Context; + SMDiagnostic Error; + auto M = parseAssemblyString(Assembly, Error, Context); + ASSERT_TRUE(M) << "Bad assembly?"; + + Function *F = M->getFunction("test"); + Value *Arg = &*F->arg_begin(); + BasicBlock *BB = &F->getEntryBlock(); + Instruction *Call = &*BB->begin(); + Instruction *CmpXChg = Call->getNextNode(); + Instruction *ICmp = CmpXChg->getNextNode(); + + CollectingCaptureTracker CT; + PointerMayBeCaptured(Arg, &CT); + EXPECT_EQ(7u, CT.Captures.size()); + // Call arg 1 + EXPECT_EQ(Call, CT.Captures[0]->getUser()); + EXPECT_EQ(0u, CT.Captures[0]->getOperandNo()); + // Call arg 3 + EXPECT_EQ(Call, CT.Captures[1]->getUser()); + EXPECT_EQ(2u, CT.Captures[1]->getOperandNo()); + // Operand bundle arg + EXPECT_EQ(Call, CT.Captures[2]->getUser()); + EXPECT_EQ(3u, CT.Captures[2]->getOperandNo()); + // Cmpxchg compare operand + EXPECT_EQ(CmpXChg, CT.Captures[3]->getUser()); + EXPECT_EQ(1u, CT.Captures[3]->getOperandNo()); + // Cmpxchg new value operand + EXPECT_EQ(CmpXChg, CT.Captures[4]->getUser()); + EXPECT_EQ(2u, CT.Captures[4]->getOperandNo()); + // ICmp first operand + EXPECT_EQ(ICmp, CT.Captures[5]->getUser()); + EXPECT_EQ(0u, CT.Captures[5]->getOperandNo()); + // ICmp second operand + EXPECT_EQ(ICmp, CT.Captures[6]->getUser()); + EXPECT_EQ(1u, CT.Captures[6]->getOperandNo()); +} -- 2.7.4