Skip to content

Commit

Permalink
[objc-arc] Do not mismatch up retains inside a for loop with releases…
Browse files Browse the repository at this point in the history 
… outside said for loop in the presense of differing provenance caused by escaping blocks.

This occurs due to an alloca representing a separate ownership from the
original pointer. Thus consider the following pseudo-IR:

  objc_retain(%a)
  for (...) {
    objc_retain(%a)
    %block <- %a
    F(%block)
    objc_release(%block)
  }
  objc_release(%a)

From the perspective of the optimizer, the %block is a separate
provenance from the original %a. Thus the optimizer pairs up the inner
retain for %a and the outer release from %a, resulting in segfaults.

This is fixed by noting that the signature of a mismatch of
retain/releases inside the for loop is a Use/CanRelease top down with an
None bottom up (since bottom up the Retain-CanRelease-Use-Release
sequence is completed by the inner objc_retain, but top down due to the
differing provenance from the objc_release said sequence is not
completed). In said case in CheckForCFGHazards, we now clear the state
of %a implying that no pairing will occur.

Additionally a test case is included.

rdar://12969722

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@179747 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@gottesmm
gottesmm committed Apr 18, 2013
1 parent 8a70920 commit 0556900
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 96 deletions.
227 changes: 131 additions & 96 deletions lib/Transforms/ObjCARC/ObjCARCOpts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,65 @@ void ObjCARCOpt::OptimizeIndividualCalls(Function &F) {
}
}

/// If we have a top down pointer in the S_Use state, make sure that there are
/// no CFG hazards by checking the states of various bottom up pointers.
static void CheckForUseCFGHazard(const Sequence SuccSSeq,
const bool SuccSRRIKnownSafe,
PtrState &S,
bool &SomeSuccHasSame,
bool &AllSuccsHaveSame,
bool &ShouldContinue) {
switch (SuccSSeq) {
case S_CanRelease: {
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe) {
S.ClearSequenceProgress();
break;
}
ShouldContinue = true;
break;
}
case S_Use:
SomeSuccHasSame = true;
break;
case S_Stop:
case S_Release:
case S_MovableRelease:
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe)
AllSuccsHaveSame = false;
break;
case S_Retain:
llvm_unreachable("bottom-up pointer in retain state!");
case S_None:
llvm_unreachable("This should have been handled earlier.");
}
}

/// If we have a Top Down pointer in the S_CanRelease state, make sure that
/// there are no CFG hazards by checking the states of various bottom up
/// pointers.
static void CheckForCanReleaseCFGHazard(const Sequence SuccSSeq,
const bool SuccSRRIKnownSafe,
PtrState &S,
bool &SomeSuccHasSame,
bool &AllSuccsHaveSame) {
switch (SuccSSeq) {
case S_CanRelease:
SomeSuccHasSame = true;
break;
case S_Stop:
case S_Release:
case S_MovableRelease:
case S_Use:
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe)
AllSuccsHaveSame = false;
break;
case S_Retain:
llvm_unreachable("bottom-up pointer in retain state!");
case S_None:
llvm_unreachable("This should have been handled earlier.");
}
}

/// Check for critical edges, loop boundaries, irreducible control flow, or
/// other CFG structures where moving code across the edge would result in it
/// being executed more.
Expand All @@ -1670,106 +1729,82 @@ ObjCARCOpt::CheckForCFGHazards(const BasicBlock *BB,
// If any top-down local-use or possible-dec has a succ which is earlier in
// the sequence, forget it.
for (BBState::ptr_iterator I = MyStates.top_down_ptr_begin(),
E = MyStates.top_down_ptr_end(); I != E; ++I)
switch (I->second.GetSeq()) {
default: break;
case S_Use: {
const Value *Arg = I->first;
const TerminatorInst *TI = cast<TerminatorInst>(&BB->back());
bool SomeSuccHasSame = false;
bool AllSuccsHaveSame = true;
PtrState &S = I->second;
succ_const_iterator SI(TI), SE(TI, false);

for (; SI != SE; ++SI) {
Sequence SuccSSeq = S_None;
bool SuccSRRIKnownSafe = false;
// If VisitBottomUp has pointer information for this successor, take
// what we know about it.
DenseMap<const BasicBlock *, BBState>::iterator BBI =
BBStates.find(*SI);
assert(BBI != BBStates.end());
const PtrState &SuccS = BBI->second.getPtrBottomUpState(Arg);
SuccSSeq = SuccS.GetSeq();
SuccSRRIKnownSafe = SuccS.RRI.KnownSafe;
switch (SuccSSeq) {
case S_None:
case S_CanRelease: {
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe) {
S.ClearSequenceProgress();
break;
}
continue;
}
case S_Use:
SomeSuccHasSame = true;
break;
case S_Stop:
case S_Release:
case S_MovableRelease:
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe)
AllSuccsHaveSame = false;
break;
case S_Retain:
llvm_unreachable("bottom-up pointer in retain state!");
}
}
// If the state at the other end of any of the successor edges
// matches the current state, require all edges to match. This
// guards against loops in the middle of a sequence.
if (SomeSuccHasSame && !AllSuccsHaveSame)
E = MyStates.top_down_ptr_end(); I != E; ++I) {
PtrState &S = I->second;
const Sequence Seq = I->second.GetSeq();

// We only care about S_Retain, S_CanRelease, and S_Use.
if (Seq == S_None)
continue;

// Make sure that if extra top down states are added in the future that this
// code is updated to handle it.
assert((Seq == S_Retain || Seq == S_CanRelease || Seq == S_Use) &&
"Unknown top down sequence state.");

const Value *Arg = I->first;
const TerminatorInst *TI = cast<TerminatorInst>(&BB->back());
bool SomeSuccHasSame = false;
bool AllSuccsHaveSame = true;

succ_const_iterator SI(TI), SE(TI, false);

for (; SI != SE; ++SI) {
// If VisitBottomUp has pointer information for this successor, take
// what we know about it.
const DenseMap<const BasicBlock *, BBState>::iterator BBI =
BBStates.find(*SI);
assert(BBI != BBStates.end());
const PtrState &SuccS = BBI->second.getPtrBottomUpState(Arg);
const Sequence SuccSSeq = SuccS.GetSeq();

// If bottom up, the pointer is in an S_None state, clear the sequence
// progress since the sequence in the bottom up state finished
// suggesting a mismatch in between retains/releases. This is true for
// all three cases that we are handling here: S_Retain, S_Use, and
// S_CanRelease.
if (SuccSSeq == S_None) {
S.ClearSequenceProgress();
break;
}
case S_CanRelease: {
const Value *Arg = I->first;
const TerminatorInst *TI = cast<TerminatorInst>(&BB->back());
bool SomeSuccHasSame = false;
bool AllSuccsHaveSame = true;
PtrState &S = I->second;
succ_const_iterator SI(TI), SE(TI, false);

for (; SI != SE; ++SI) {
Sequence SuccSSeq = S_None;
bool SuccSRRIKnownSafe = false;
// If VisitBottomUp has pointer information for this successor, take
// what we know about it.
DenseMap<const BasicBlock *, BBState>::iterator BBI =
BBStates.find(*SI);
assert(BBI != BBStates.end());
const PtrState &SuccS = BBI->second.getPtrBottomUpState(Arg);
SuccSSeq = SuccS.GetSeq();
SuccSRRIKnownSafe = SuccS.RRI.KnownSafe;
switch (SuccSSeq) {
case S_None: {
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe) {
S.ClearSequenceProgress();
break;
}
continue;
}

// If we have S_Use or S_CanRelease, perform our check for cfg hazard
// checks.
const bool SuccSRRIKnownSafe = SuccS.RRI.KnownSafe;

// *NOTE* We do not use Seq from above here since we are allowing for
// S.GetSeq() to change while we are visiting basic blocks.
switch(S.GetSeq()) {
case S_Use: {
bool ShouldContinue = false;
CheckForUseCFGHazard(SuccSSeq, SuccSRRIKnownSafe, S,
SomeSuccHasSame, AllSuccsHaveSame,
ShouldContinue);
if (ShouldContinue)
continue;
}
case S_CanRelease:
SomeSuccHasSame = true;
break;
case S_Stop:
case S_Release:
case S_MovableRelease:
case S_Use:
if (!S.RRI.KnownSafe && !SuccSRRIKnownSafe)
AllSuccsHaveSame = false;
break;
case S_Retain:
llvm_unreachable("bottom-up pointer in retain state!");
}
break;
}
case S_CanRelease: {
CheckForCanReleaseCFGHazard(SuccSSeq, SuccSRRIKnownSafe,
S, SomeSuccHasSame,
AllSuccsHaveSame);
break;
}
case S_Retain:
case S_None:
case S_Stop:
case S_Release:
case S_MovableRelease:
break;
}
// If the state at the other end of any of the successor edges
// matches the current state, require all edges to match. This
// guards against loops in the middle of a sequence.
if (SomeSuccHasSame && !AllSuccsHaveSame)
S.ClearSequenceProgress();
break;
}
}

// If the state at the other end of any of the successor edges
// matches the current state, require all edges to match. This
// guards against loops in the middle of a sequence.
if (SomeSuccHasSame && !AllSuccsHaveSame)
S.ClearSequenceProgress();
}
}

bool
Expand Down
36 changes: 36 additions & 0 deletions test/Transforms/ObjCARC/cfg-hazards.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ declare void @use_pointer(i8*)
declare i8* @objc_retain(i8*)
declare void @objc_release(i8*)
declare void @callee()
declare void @block_callee(void ()*)

; CHECK: define void @test0(
; CHECK: call i8* @objc_retain(
Expand Down Expand Up @@ -394,6 +395,41 @@ exit:
ret void
}

; Do not improperly pair retains in a for loop with releases outside of a for
; loop when the proper pairing is disguised by a separate provenance represented
; by an alloca.
; rdar://12969722

; CHECK: define void @test13(i8* %a) [[NUW]] {
; CHECK: entry:
; CHECK: tail call i8* @objc_retain(i8* %a) [[NUW]]
; CHECK: loop:
; CHECK: tail call i8* @objc_retain(i8* %a) [[NUW]]
; CHECK: call void @block_callee
; CHECK: call void @objc_release(i8* %reloaded_a) [[NUW]]
; CHECK: exit:
; CHECK: call void @objc_release(i8* %a) [[NUW]]
; CHECK: }
define void @test13(i8* %a) nounwind {
entry:
%block = alloca i8*
%a1 = tail call i8* @objc_retain(i8* %a) nounwind
br label %loop

loop:
%a2 = tail call i8* @objc_retain(i8* %a) nounwind
store i8* %a, i8** %block, align 8
%casted_block = bitcast i8** %block to void ()*
call void @block_callee(void ()* %casted_block)
%reloaded_a = load i8** %block, align 8
call void @objc_release(i8* %reloaded_a) nounwind, !clang.imprecise_release !0
br i1 undef, label %loop, label %exit

exit:
call void @objc_release(i8* %a) nounwind, !clang.imprecise_release !0
ret void
}

; CHECK: attributes [[NUW]] = { nounwind }

!0 = metadata !{}

0 comments on commit 0556900

Please sign in to comment.