Page MenuHomePhabricator

[StaticAnalyzer] LoopUnrolling: Exclude cases where the counter is escaped before the loop

Authored by szepet on Jul 19 2017, 6:01 PM.



Adding escape check for the counter variable of the loop.
It is achieved by jumping back on the ExplodedGraph to its declStmt.
When the analyzer encounters a Stmt which can result pointer escape then we not considering unrolling the given loop.

Diff Detail


Event Timeline

szepet created this revision.Jul 19 2017, 6:01 PM
NoQ edited edge metadata.Jul 25 2017, 2:58 AM

Hmm, what prevents us from reusing existing matchers against S in isPossiblyEscaped()? Also i'd probably prefer to avoid recursion (we don't want stack traces of 225k frames).

Otherwise looks great, this should work!

szepet added a comment.EditedJul 26 2017, 6:22 AM

Because I was not sure about the implementation detail of that. I dont want to write duplicated code so I tried to come up with a solution that the parameter of the matchers could be llvm::PointerUnion<const Decl *, const char *>. So I could define an equalsNode function which seems something like this: static internal::Matcher<Decl> equalsNode(llvm::PointerUnion<const Decl *, const char *> PU){..} and returns the suitable matcher and this could be used by the other matchers. However, PointerUnion does not let me to store char*. (This triggers the following assert in /home/Project/gsoc/llvm/include/llvm/ADT/PointerIntPair.h :
static_assert(IntBits <= PtrTraits::NumLowBitsAvailable, ,"PointerIntPair with integer size too large for pointer");)

I could use a simple union and an enum value as parameter but if you have better idea or know how to use PointerUnion correctly in this case please let me know!

NoQ added a comment.Jul 27 2017, 12:48 AM

Maybe use decl matcher as the parameter, and pass either equalsNode(Foo) or equalsBoundNode("foo") as argument value?

szepet updated this revision to Diff 108443.Jul 27 2017, 3:49 AM

Good idea, thanks!
Using matchers to identify possible escape stmts.
Test cases updated.

NoQ added a comment.Jul 27 2017, 4:58 AM

Yay, looks cool.

50 ↗(On Diff #108443)

Maybe check here that the counter variable is local?

Or in isPossiblyEscaped(). Because globals are always escaped.

54 ↗(On Diff #108443)

Can we use the same trick with decl matcher here and in hasSuspiciousStmt(), for consistency? I suspect it'd even be nicer.

68 ↗(On Diff #108443)

Suggesting VarNodeMatcher as parameter name. EqualityCheck makes me imagine something that involves == or !=.

szepet updated this revision to Diff 108562.Jul 27 2017, 6:11 PM
szepet marked 3 inline comments as done.

Requested changes made.
Description updated.

NoQ accepted this revision.Aug 14 2017, 7:09 AM

Looks great now!

This revision is now accepted and ready to land.Aug 14 2017, 7:09 AM
This revision was automatically updated to reflect the committed changes.