From 4bfb393b4c2dd4f07046335c8be655a994677a6d Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Mon, 15 Feb 2021 17:39:36 -0800 Subject: [PATCH] [ObjC][ARC] Do not perform code motion on precise release calls This fixes a bug where an object can get deallocated before reaching the end of its full formal lifetime. rdar://72110887 rdar://74123176 --- lib/Transforms/ObjCARC/ObjCARCOpts.cpp | 3 -- lib/Transforms/ObjCARC/PtrState.cpp | 25 ++++---------- lib/Transforms/ObjCARC/PtrState.h | 3 +- test/Transforms/ObjCARC/basic.ll | 45 +++++++++----------------- test/Transforms/ObjCARC/code-motion.ll | 18 +++++++++++ 5 files changed, 41 insertions(+), 53 deletions(-) diff --git a/lib/Transforms/ObjCARC/ObjCARCOpts.cpp b/lib/Transforms/ObjCARC/ObjCARCOpts.cpp index 091ea143d6a..0769d466be9 100644 --- a/lib/Transforms/ObjCARC/ObjCARCOpts.cpp +++ b/lib/Transforms/ObjCARC/ObjCARCOpts.cpp @@ -1229,7 +1229,6 @@ static void CheckForUseCFGHazard(const Sequence SuccSSeq, SomeSuccHasSame = true; break; case S_Stop: - case S_Release: case S_MovableRelease: if (!S.IsKnownSafe() && !SuccSRRIKnownSafe) AllSuccsHaveSame = false; @@ -1257,7 +1256,6 @@ static void CheckForCanReleaseCFGHazard(const Sequence SuccSSeq, SomeSuccHasSame = true; break; case S_Stop: - case S_Release: case S_MovableRelease: case S_Use: if (!S.IsKnownSafe() && !SuccSRRIKnownSafe) @@ -1343,7 +1341,6 @@ ObjCARCOpt::CheckForCFGHazards(const BasicBlock *BB, case S_Retain: case S_None: case S_Stop: - case S_Release: case S_MovableRelease: break; } diff --git a/lib/Transforms/ObjCARC/PtrState.cpp b/lib/Transforms/ObjCARC/PtrState.cpp index 89937b20140..d10d5851d5e 100644 --- a/lib/Transforms/ObjCARC/PtrState.cpp +++ b/lib/Transforms/ObjCARC/PtrState.cpp @@ -44,8 +44,6 @@ raw_ostream &llvm::objcarc::operator<<(raw_ostream &OS, const Sequence S) { return OS << "S_CanRelease"; case S_Use: return OS << "S_Use"; - case S_Release: - return OS << "S_Release"; case S_MovableRelease: return OS << "S_MovableRelease"; case S_Stop: @@ -75,12 +73,10 @@ static Sequence MergeSeqs(Sequence A, Sequence B, bool TopDown) { } else { // Choose the side which is further along in the sequence. if ((A == S_Use || A == S_CanRelease) && - (B == S_Use || B == S_Release || B == S_Stop || B == S_MovableRelease)) + (B == S_Use || B == S_Stop || B == S_MovableRelease)) return A; // If both sides are releases, choose the more conservative one. - if (A == S_Stop && (B == S_Release || B == S_MovableRelease)) - return A; - if (A == S_Release && B == S_MovableRelease) + if (A == S_Stop && B == S_MovableRelease) return A; } @@ -184,7 +180,7 @@ bool BottomUpPtrState::InitBottomUp(ARCMDKindCache &Cache, Instruction *I) { // pairs by making PtrState hold a stack of states, but this is // simple and avoids adding overhead for the non-nested case. bool NestingDetected = false; - if (GetSeq() == S_Release || GetSeq() == S_MovableRelease) { + if (GetSeq() == S_MovableRelease) { LLVM_DEBUG( dbgs() << " Found nested releases (i.e. a release pair)\n"); NestingDetected = true; @@ -192,8 +188,10 @@ bool BottomUpPtrState::InitBottomUp(ARCMDKindCache &Cache, Instruction *I) { MDNode *ReleaseMetadata = I->getMetadata(Cache.get(ARCMDKindID::ImpreciseRelease)); - Sequence NewSeq = ReleaseMetadata ? S_MovableRelease : S_Release; + Sequence NewSeq = ReleaseMetadata ? S_MovableRelease : S_Stop; ResetSequenceProgress(NewSeq); + if (NewSeq == S_Stop) + InsertReverseInsertPt(I); SetReleaseMetadata(ReleaseMetadata); SetKnownSafe(HasKnownPositiveRefCount()); SetTailCallRelease(cast(I)->isTailCall()); @@ -208,7 +206,6 @@ bool BottomUpPtrState::MatchWithRetain() { Sequence OldSeq = GetSeq(); switch (OldSeq) { case S_Stop: - case S_Release: case S_MovableRelease: case S_Use: // If OldSeq is not S_Use or OldSeq is S_Use and we are tracking an @@ -243,7 +240,6 @@ bool BottomUpPtrState::HandlePotentialAlterRefCount(Instruction *Inst, SetSeq(S_CanRelease); return true; case S_CanRelease: - case S_Release: case S_MovableRelease: case S_Stop: case S_None: @@ -292,17 +288,11 @@ void BottomUpPtrState::HandlePotentialUse(BasicBlock *BB, Instruction *Inst, // Check for possible direct uses. switch (GetSeq()) { - case S_Release: case S_MovableRelease: if (CanUse(Inst, Ptr, PA, Class)) { LLVM_DEBUG(dbgs() << " CanUse: Seq: " << GetSeq() << "; " << *Ptr << "\n"); SetSeqAndInsertReverseInsertPt(S_Use); - } else if (Seq == S_Release && IsUser(Class)) { - LLVM_DEBUG(dbgs() << " PreciseReleaseUse: Seq: " << GetSeq() - << "; " << *Ptr << "\n"); - // Non-movable releases depend on any possible objc pointer use. - SetSeqAndInsertReverseInsertPt(S_Stop); } else if (const auto *Call = getreturnRVOperand(*Inst, Class)) { if (CanUse(Call, Ptr, PA, GetBasicARCInstKind(Call))) { LLVM_DEBUG(dbgs() << " ReleaseUse: Seq: " << GetSeq() << "; " @@ -378,7 +368,6 @@ bool TopDownPtrState::MatchWithRelease(ARCMDKindCache &Cache, case S_None: return false; case S_Stop: - case S_Release: case S_MovableRelease: llvm_unreachable("top-down pointer in bottom up state!"); } @@ -418,7 +407,6 @@ bool TopDownPtrState::HandlePotentialAlterRefCount( case S_None: return false; case S_Stop: - case S_Release: case S_MovableRelease: llvm_unreachable("top-down pointer in release state!"); } @@ -442,7 +430,6 @@ void TopDownPtrState::HandlePotentialUse(Instruction *Inst, const Value *Ptr, case S_None: return; case S_Stop: - case S_Release: case S_MovableRelease: llvm_unreachable("top-down pointer in release state!"); } diff --git a/lib/Transforms/ObjCARC/PtrState.h b/lib/Transforms/ObjCARC/PtrState.h index f143120ad6a..232db2bd33b 100644 --- a/lib/Transforms/ObjCARC/PtrState.h +++ b/lib/Transforms/ObjCARC/PtrState.h @@ -43,8 +43,7 @@ enum Sequence { S_Retain, ///< objc_retain(x). S_CanRelease, ///< foo(x) -- x could possibly see a ref count decrement. S_Use, ///< any use of x. - S_Stop, ///< like S_Release, but code motion is stopped. - S_Release, ///< objc_release(x). + S_Stop, ///< code motion is stopped. S_MovableRelease ///< objc_release(x), !clang.imprecise_release. }; diff --git a/test/Transforms/ObjCARC/basic.ll b/test/Transforms/ObjCARC/basic.ll index 9617eacd59f..d521ea1de21 100644 --- a/test/Transforms/ObjCARC/basic.ll +++ b/test/Transforms/ObjCARC/basic.ll @@ -723,14 +723,15 @@ return: ; CHECK-LABEL: define void @test8c( ; CHECK: entry: ; CHECK: t: -; CHECK: @llvm.objc.retain +; CHECK-NOT: @llvm.objc. ; CHECK: f: -; CHECK: @llvm.objc.retain +; CHECK-NOT: @llvm.objc. ; CHECK: mid: ; CHECK: u: +; CHECK: @llvm.objc.retain ; CHECK: @llvm.objc.release ; CHECK: g: -; CHECK: @llvm.objc.release +; CHECK-NOT: @llvm.objc. ; CHECK: return: ; CHECK: } define void @test8c(i32* %x, i1 %p, i1 %q) nounwind { @@ -1549,17 +1550,14 @@ done: ; Like test28. but with two releases. ; CHECK-LABEL: define void @test29( -; CHECK-NOT: @llvm.objc. -; CHECK: true: ; CHECK: call i8* @llvm.objc.retain +; CHECK: true: ; CHECK: call void @callee() ; CHECK: store -; CHECK: call void @llvm.objc.release -; CHECK-NOT: @llvm.objc.release ; CHECK: done: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: ohno: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: } define void @test29(i8* %p, i1 %x, i1 %y) { entry: @@ -1584,19 +1582,15 @@ ohno: ; with an extra release. ; CHECK-LABEL: define void @test30( -; CHECK-NOT: @llvm.objc. -; CHECK: true: ; CHECK: call i8* @llvm.objc.retain +; CHECK: true: ; CHECK: call void @callee() ; CHECK: store -; CHECK: call void @llvm.objc.release -; CHECK-NOT: @llvm.objc.release ; CHECK: false: -; CHECK-NOT: @llvm.objc. ; CHECK: done: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: ohno: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: } define void @test30(i8* %p, i1 %x, i1 %y, i1 %z) { entry: @@ -1626,14 +1620,11 @@ ohno: ; CHECK: call i8* @llvm.objc.retain(i8* %p) ; CHECK: call void @callee() ; CHECK: store -; CHECK: call void @llvm.objc.release -; CHECK-NOT: @llvm.objc.release ; CHECK: true: -; CHECK-NOT: @llvm.objc.release +; CHECK: call void @llvm.objc.release ; CHECK: false: -; CHECK-NOT: @llvm.objc.release +; CHECK: call void @llvm.objc.release ; CHECK: ret void -; CHECK-NOT: @llvm.objc.release ; CHECK: } define void @test31(i8* %p, i1 %x) { entry: @@ -1652,14 +1643,12 @@ false: ; Don't consider bitcasts or getelementptrs direct uses. ; CHECK-LABEL: define void @test32( -; CHECK-NOT: @llvm.objc. -; CHECK: true: ; CHECK: call i8* @llvm.objc.retain +; CHECK: true: ; CHECK: call void @callee() ; CHECK: store -; CHECK: call void @llvm.objc.release ; CHECK: done: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: } define void @test32(i8* %p, i1 %x) { entry: @@ -1681,14 +1670,12 @@ done: ; Do consider icmps to be direct uses. ; CHECK-LABEL: define void @test33( -; CHECK-NOT: @llvm.objc. -; CHECK: true: ; CHECK: call i8* @llvm.objc.retain +; CHECK: true: ; CHECK: call void @callee() ; CHECK: icmp -; CHECK: call void @llvm.objc.release ; CHECK: done: -; CHECK-NOT: @llvm.objc. +; CHECK: call void @llvm.objc.release ; CHECK: } define void @test33(i8* %p, i1 %x, i8* %y) { entry: diff --git a/test/Transforms/ObjCARC/code-motion.ll b/test/Transforms/ObjCARC/code-motion.ll index 7c3242ce857..7f254e69804 100644 --- a/test/Transforms/ObjCARC/code-motion.ll +++ b/test/Transforms/ObjCARC/code-motion.ll @@ -3,6 +3,8 @@ declare void @alterRefCount() declare void @use(i8*) +@g0 = global i8* null, align 8 + ; Check that ARC optimizer doesn't reverse the order of the retain call and the ; release call when there are debug instructions. @@ -20,6 +22,22 @@ define i32 @test(i8* %x, i8* %y, i8 %z, i32 %i) { ret i32 %i } +; ARC optimizer shouldn't move the release call, which is a precise release call +; past the call to @alterRefCount. + +; CHECK-LABEL: define void @test2( +; CHECK: call void @alterRefCount( +; CHECK: call void @llvm.objc.release( + +define void @test2() { + %v0 = load i8*, i8** @g0, align 8 + %v1 = tail call i8* @llvm.objc.retain(i8* %v0) + tail call void @use(i8* %v0) + tail call void @alterRefCount() + tail call void @llvm.objc.release(i8* %v0) + ret void +} + declare void @llvm.dbg.declare(metadata, metadata, metadata) declare i8* @llvm.objc.retain(i8*) local_unnamed_addr declare void @llvm.objc.release(i8*) local_unnamed_addr