Page MenuHomePhabricator

[StackSafety] Skip ambiguous lifetime analysis
ClosedPublic

Authored by vitalybuka on Jul 27 2020, 3:24 AM.

Details

Summary

If we can't identify alloca used in lifetime marker we
need to assume to worst case scenario.

Diff Detail

Event Timeline

vitalybuka created this revision.Jul 27 2020, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 3:24 AM
t.p.northover added inline comments.
llvm/include/llvm/Analysis/StackLifetime.h
124

Missing 'n' in "unknown"

llvm/lib/Analysis/StackLifetime.cpp
304

Doesn't this case need the loop below? It looks like it's undoing all the extra default initialization caused by the resize.

Actually, I'd be inclined to reword the whole thing in terms of resizing with the default and separately setting NumAllocas - 1 if that is the correct reading.

eugenis added inline comments.Jul 29 2020, 1:47 PM
llvm/test/Analysis/StackSafetyAnalysis/lifetime.ll
781–836

Is this a tab character?

vitalybuka added inline comments.Jul 29 2020, 7:56 PM
llvm/lib/Analysis/StackLifetime.cpp
304

Loop below sets Full range for uninteresting allocas, however with HasUnknowLifetimeStartOrEnd we can't be sure which are uninteresting,
Other than that I am not sure what are you asking.

llvm/test/Analysis/StackSafetyAnalysis/lifetime.ll
781–836

no

eugenis added inline comments.Jul 30 2020, 1:30 PM
llvm/lib/Analysis/StackLifetime.cpp
304

I think Vitaly's right, if we see a lifetime that can't be traced back to an alloca, we have no way of knowing which allocas are "not interesting" and must handle all of them conservatively.

eugenis accepted this revision.Jul 31 2020, 3:29 PM

LGTM

This revision is now accepted and ready to land.Jul 31 2020, 3:29 PM
This revision was landed with ongoing or failed builds.Aug 6 2020, 7:10 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka marked an inline comment as not done.
vitalybuka reopened this revision.Aug 7 2020, 2:00 PM
This revision is now accepted and ready to land.Aug 7 2020, 2:00 PM
This revision was automatically updated to reflect the committed changes.
ChuanqiXu added inline comments.
llvm/lib/Analysis/StackLifetime.cpp
78

Maybe we can't use findAllocaForValue to find alloca for lifetime marker. Here I find a pattern:

a = alloca struct ...
b = getelementptr from a
lifetime start (b)
/// ...
lifetime end (b)

And the code here would treat the lifetime span of b, which is part of a, as the whole lifetime of a. But the lifetime span of other component of a may not be the same with the lifetime span of b. I think it may cause mismatch.

vitalybuka added inline comments.Aug 26 2020, 2:25 PM
llvm/lib/Analysis/StackLifetime.cpp
78

Is this theoretical or something produces such patterns?
I guess we will need to fallback to HasUnknownLifetimeStartOrEnd if life does not cover entire alloca.

ChuanqiXu added inline comments.Aug 26 2020, 6:34 PM
llvm/lib/Analysis/StackLifetime.cpp
78

I don't find theoretical who produces this pattern. I find such pattern by using StackLifetime in practice. The pattern seems like related to Coroutine Elision which would put one Coroutine Frame structure into another Coroutine Frame. But I don't sure if there is any other situation where would produce similar patterns.
My local fix for this problem is:
replace

const AllocaInst *AI = llvm::findAllocaForValue(II->getArgOperand(1));

into

auto *OpInst = dyn_cast<Instruction>(II->getOperand(1));
auto *AI = dyn_cast<AllocaInst>(OpInst->stripPointerCasts());

which would fallback to HasUnknownLifetimeStartOrEnd if lifetime marker doesn't cover entire alloca as you say.