This is an archive of the discontinued LLVM Phabricator instance.

[stack-safety] Local analysis implementation
ClosedPublic

Authored by vitalybuka on Nov 13 2018, 4:53 PM.

Details

Summary

Analysis produces StackSafetyInfo which contains information with how allocas
and parameters were used in functions.

From prototype by Evgenii Stepanov and Vlad Tsyrklevich.

Diff Detail

Repository
rL LLVM

Event Timeline

vitalybuka created this revision.Nov 13 2018, 4:53 PM
eugenis added inline comments.Nov 14 2018, 3:14 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
58 ↗(On Diff #173966)

Please add a short comment to this struct.
Maybe rename it to CallUseInfo? It describes a use of an address in a function call argument, not the call itself; also mention that Range is not access range, but an offset range.

239 ↗(On Diff #173966)

Please update the comment. This is not only about safe stack.

vitalybuka marked 2 inline comments as done.

docs

llvm/lib/Analysis/StackSafetyAnalysis.cpp
119 ↗(On Diff #173966)

Would be worth copying over the comment from the SafeStack pass:

/// Calculate the allocation size of a given alloca. Returns 0 if the
/// size can not be statically determined.
133 ↗(On Diff #173966)

Mark this a TODO and update the struct name.

135 ↗(On Diff #173966)

This uses a different naming convention than the structs above though it's part of the same 'type set' as the *Info structs above which is confusing. Couldn't immediately think of a fix that makes it nicer though. Might just be worth a comment to at least be explicit about why this is.

321 ↗(On Diff #173966)

Is this comment outdated? I can't figure out what it's referring to, Maybe @eugenis knows.

vitalybuka marked 4 inline comments as done.

addressing @vlad.tsyrklevich comments

vitalybuka added inline comments.Nov 15 2018, 12:02 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
135 ↗(On Diff #173966)

renamed
WDYT?

321 ↗(On Diff #173966)

This is correct but not optimal behavior. I've added FIXME: into "case Instruction::Ret:" and new test into IPA analysis

eugenis added inline comments.Nov 20 2018, 4:28 PM
llvm/lib/Analysis/StackSafetyAnalysis.cpp
321 ↗(On Diff #173966)

Right. We could track return values of function call the same as we track allocas and arguments, and have each function describe its return value as possibly being equal to an argument pointer with an offset range. I have not seen too many cases like that, and opted for simplicity: consider anything that escapes through return value untrackable and, on the caller side, consider return values safe (i.e. unrelated to any tracked object).

Btw, I don't like the name "leak". I think the usual term for this is escaped pointer / alloca, but full-set range is also possible when we can not prove anything about the range of offsets in a memory access - even if it is the only access. Rename it to smth like setUnknown() ?

vitalybuka marked an inline comment as done.

Rename setLeak

eugenis accepted this revision.Nov 21 2018, 3:41 PM

LGTM

This revision is now accepted and ready to land.Nov 21 2018, 3:41 PM

I've been busy and still haven't had a chance to run down why the behavior of %z in the NonConstantOffset() test changes so oddly with different select values.

llvm/lib/Analysis/StackSafetyAnalysis.cpp
29 ↗(On Diff #174265)

"mentions of the alloca in addition expressions with zero" (I didn't update the comment after this change was made.)

135 ↗(On Diff #173966)

Looks good, 'FunctionStackSummary' still needs to be updated and that comment should also have a TODO or FIXME to indicate it's a future idea.

I've been busy and still haven't had a chance to run down why the behavior of %z in the NonConstantOffset() test changes so oddly with different select values.

I'd like to submit as-is and investigate later and fix if needed, so I can have less patches to rebase.
This code is not hooked into any useful functionality yet.

modulo nits on updating comments that I left earlier

vitalybuka marked 3 inline comments as done.

Renames and comments

This revision was automatically updated to reflect the committed changes.