This is an archive of the discontinued LLVM Phabricator instance.

[CFG] [analyzer] Add construction context for DeclStmt.
ClosedPublic

Authored by NoQ on Jan 30 2018, 11:41 AM.

Details

Summary

Adding construction contexts is easy. This patch builds on top of D42672 to add construction contexts for constructors that are direct initializers (without any ExprWithCleanups wrappers - but those would need to be covered later) of variable DeclStmts.

The analyzer still doesn't use them, and existing construction contexts are not yet enough as a drop-in replacement for existing mechanisms in the analyzer.

I should probably make some sort of scope guard later - that'd push the construction context on the stack when it gets constructed and pop it when it is destroyed.

Diff Detail

Event Timeline

NoQ created this revision.Jan 30 2018, 11:41 AM

I should probably make some sort of scope guard later - that'd push the construction context on the stack when it gets constructed and pop it when it is destroyed.

Yes please, existing code could evolve and get stack into inconsistent state

Naive question: what does the stack of construction contexts represent?

lib/Analysis/CFG.cpp
2382

Would be great to have an explicit constructor call here, if possible

NoQ added a comment.Jan 30 2018, 2:21 PM

Naive question: what does the stack of construction contexts represent?

It represents our expectation to encounter situations when we are in multiple construction contexts simultaneously, which we have entered (i.e. encountered the trigger statement during recursive descend) in a particular order, and will resolve (i.e. create the constructor element) in the opposite order.

Hmm, it seems that we don't have tests for this yet. I should remove it until i get us some tests.

NoQ updated this revision to Diff 132055.Jan 30 2018, 2:43 PM

Remove the stack of contexts for now.

It is nice to see how cleanly this slides in.

lib/Analysis/CFG.cpp
2382

I agree with George. I think we should constructors for each of the scenarios we expect (DeclStatement, NewExpr, etc.) This will make it harder to accidentally construct the context with the wrong triggering statement and also serve to document the kinds of triggers we expect.

NoQ added inline comments.Feb 1 2018, 2:30 PM
lib/Analysis/CFG.cpp
2382

Forgot to mention, this is already done in the next patch, D42719.

NoQ updated this revision to Diff 132658.Feb 2 2018, 1:06 PM
NoQ added a subscriber: mgehre.

Rebase. Add direct tests for testing the newly added functionality specifically.

I guessed that i should rather remove the test/Analysis/lifetime-cfg-output.cpp tests that were originally added because so far there's no indication that users of LifetimeEnds blocks would like to anyhow interact with the new CFGConstructor block (not sure, @mgehre, would you like to have these?).

NoQ updated this revision to Diff 132687.Feb 2 2018, 2:28 PM

Rebase.

NoQ edited the summary of this revision. (Show Details)Feb 2 2018, 4:16 PM
NoQ updated this revision to Diff 133332.Feb 7 2018, 4:20 PM

Fix function names in the newly added tests to be less dramatic and more correct.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 9 2018, 5:57 PM
This revision was automatically updated to reflect the committed changes.