[PATCH] D16403: Add scope information to CFG

This revision was automatically updated to reflect the committed changes. Closed by commit rC327258: [analyzer] Add scope information to CFG (authored by chefmax, committed by ). Repository: rC Clang https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisDeclContext.h inclu

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. In https://reviews.llvm.org/D16403#1031567, @thakis wrote: > Since this affects analysis-based warnings, have you checked if this patch > has any effect on compile times? Just in case - this is under an analyzer-only flag, so the actual warnings are not affected. I guess

[PATCH] D16403: Add scope information to CFG

thakis added a comment. Since this affects analysis-based warnings, have you checked if this patch has any effect on compile times? Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D16403: Add scope information to CFG

NoQ added inline comments. Comment at: lib/Analysis/CFG.cpp:1700 /// way return valid LocalScope object. LocalScope* CFGBuilder::createOrReuseLocalScope(LocalScope* Scope) { if (Scope) NoQ wrote: > It seems that something has moved asterisks to a weird spot

[PATCH] D16403: Add scope information to CFG

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I think we should land this and celebrate. @szepet: Ouch, i was sure i already answered this, sorry, dunno where this went. > So, LoopExit and ScopeExit would be the same but the underlying TriggerS

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. Now, the cfg for this example: void test_for_implicit_scope() { for (A a; A b = a;) A c; } looks like this: F5874197: CFG-implicit-for.dot Repository: rL LLVM https://reviews.llvm.org/D16403 __

[PATCH] D16403: Add scope information to CFG

m.ostapenko updated this revision to Diff 137150. m.ostapenko added a comment. Rebased and updated. Fix the issue with ordering between ScopeEnds and implicit destructors. Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisDeclContext.h include/cla

[PATCH] D16403: Add scope information to CFG

szepet added a comment. In https://reviews.llvm.org/D16403#1011218, @NoQ wrote: > Yeah, i mean, like, if we change the scope markers to also appear even when > no variables are present in the scope, then it would be possible to replace > loop markers with some of the scope markers, right? OK,

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. In https://reviews.llvm.org/D16403#1013346, @m.ostapenko wrote: > In https://reviews.llvm.org/D16403#1011218, @NoQ wrote: > > > Yeah, i mean, like, if we change the scope markers to also appear even when > > no variables are present in the scope, then it would be possible to

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. In https://reviews.llvm.org/D16403#1001466, @NoQ wrote: > I poked Devin offline and we agreed that the overall approach is good to go. > Maxim, thank you for picking it up! > > We still don't have scopes for segments of code that don't have any variables > in them,

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. In https://reviews.llvm.org/D16403#1011218, @NoQ wrote: > In https://reviews.llvm.org/D16403#1010096, @szepet wrote: > > > In https://reviews.llvm.org/D16403#992454, @NoQ wrote: > > > > > @szepet: so i see that `LoopExit` goes in the beginning of the cleanup > > > bl

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. In https://reviews.llvm.org/D16403#1010096, @szepet wrote: > In https://reviews.llvm.org/D16403#992454, @NoQ wrote: > > > @szepet: so i see that `LoopExit` goes in the beginning of the cleanup > > block after the loop, while various `ScopeEnd`s go after the `LoopExit`. > >

[PATCH] D16403: Add scope information to CFG

szepet added a comment. In https://reviews.llvm.org/D16403#992452, @NoQ wrote: > Thank you, this explanation looks very reasonable. > > All right, so right after the termination of the loop we have > > [B1] > 1: ForStmt (LoopExit) > 2: [B4.5].~A() (Implicit destructor) > 3: [B5.3].~A() (I

[PATCH] D16403: Add scope information to CFG

NoQ added a reviewer: rsmith. NoQ added a subscriber: rsmith. NoQ added a comment. Added @rsmith because we're trying to give him a heads up every time large CFG changes are coming, because if we mess up it would affect not only the analyzer but the whole compiler through analysis-based compiler

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. I poked Devin offline and we agreed that the overall approach is good to go. Maxim, thank you for picking it up! We still don't have scopes for segments of code that don't have any variables in them, so i guess it's not yet in the shape where it is super useful for loops,

[PATCH] D16403: Add scope information to CFG

m.ostapenko updated this revision to Diff 132393. m.ostapenko added a comment. Fix scope ends order (as discussed above) and adjust a testcase. Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisDeclContext.h include/clang/Analysis/CFG.h include/c

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. In https://reviews.llvm.org/D16403#992452, @NoQ wrote: > Thank you, this explanation looks very reasonable. > > All right, so right after the termination of the loop we have > > [B1] > 1: ForStmt (LoopExit) > 2: [B4.5].~A() (Implicit destructor) > 3: [B5.3].~A

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. @szepet: so i see that `LoopExit` goes in the beginning of the cleanup block after the loop, while various `ScopeEnd`s go after the `LoopExit`. Would loop unrolling be significantly broken if you simply subscribe to `ScopeEnd` instead of `LoopExit` and avoid cleaning up the

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. Thank you, this explanation looks very reasonable. All right, so right after the termination of the loop we have [B1] 1: ForStmt (LoopExit) 2: [B4.5].~A() (Implicit destructor) 3: [B5.3].~A() (Implicit destructor) 4: CFGScopeEnd(a) 5: CFGScopeEnd(b) ... where `[

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. Actually upload the files. F5792949: CFG-scopes-destructors-loopexit.dot F5792948: CFG-scopes-loopexit-lifetime.dot Repository: rL LLVM https://reviews.llvm.org/D16403 ___

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. Hi Artem, In https://reviews.llvm.org/D16403#989451, @NoQ wrote: > Hmm. @m.ostapenko @szepet @mgehre - I think it might be a good time to figure > out if `ScopeBegin`/`ScopeEnd`, `LoopEntrance`/`LoopExit`, `LifetimeEnds`, > `AutomaticObjectDtor` elements work nicel

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. > Do we need to have one-to-one mapping between ScopeBegins and corresponding > ScopeEnds or is it OK to assume that ScopeEnd can terminate several nested > scopes? It's fine if `ScopeEnds` terminates multiple scopes - as long as it is easy to find out what scopes are bein

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. Hmm. @m.ostapenko @szepet @mgehre - I think it might be a good time to figure out if `ScopeBegin`/`ScopeEnd`, `LoopEntrance`/`LoopExit`, `LifetimeEnds`, `AutomaticObjectDtor` elements work nicely together. How should they be ordered with respect to each other? Is any of the

[PATCH] D16403: Add scope information to CFG

m.ostapenko updated this revision to Diff 131091. m.ostapenko added a comment. Herald added a subscriber: llvm-commits. Rebased and ping. Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisDeclContext.h include/clang/Analysis/CFG.h include/clang/S

[PATCH] D16403: Add scope information to CFG

m.ostapenko updated this revision to Diff 130183. m.ostapenko retitled this revision from "Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements" to "Add scope information to CFG". m.ostapenko added a comment. Some code cleanup + updated test case. https://reviews.ll

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 129985. m.ostapenko added a comment. Hi Devin, now I'm very sorry for a such long delay. Now I have a bunch of time to proceed development of this patch (if scope contexts are still needed, of course). Regarding to approach you suggested (reuse LocalScop

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

dcoughlin added a comment. Maxim, thanks for commandeering the patch. I apologize for the long delay reviewing! I really like the approach of retrying without scopes enabled when we hit a construct we can't handle yet. This will make is possible to turn the feature on by default (and get it ru

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 112547. m.ostapenko added a comment. Ping^4 Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisContext.h include/clang/Analysis/CFG.h include/clang/StaticAnalyzer/Core/AnalyzerOptions.h lib/Analysis/Anal

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 111322. m.ostapenko added a comment. Ping^3 Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisContext.h include/clang/Analysis/CFG.h include/clang/StaticAnalyzer/Core/AnalyzerOptions.h lib/Analysis/Anal

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 110329. m.ostapenko added a comment. Rebased and ping. Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisContext.h include/clang/Analysis/CFG.h include/clang/StaticAnalyzer/Core/AnalyzerOptions.h lib/An

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 108644. m.ostapenko added a comment. Updated some comments. Could someone take a look please? Repository: rL LLVM https://reviews.llvm.org/D16403 Files: include/clang/Analysis/AnalysisContext.h include/clang/Analysis/CFG.h include/clang/StaticA

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 107894. m.ostapenko added a comment. Rebased and removed a bunch of stale changes. Also added a check for goto's: if we see GotoStmt and have cfg-scopes == true, make badCFG = true and retry without scopes enabled. This check will be removed once GotoStm

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko added a comment. In https://reviews.llvm.org/D16403#808104, @NoQ wrote: > I think the remaining switch-related code seems to be about C++17 switch > condition variables, i.e. `switch (int x = ...)`(?) Yeah, exactly. I can remove it from this patch if it looks confusing. Repositor

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

NoQ added a comment. I think the remaining switch-related code seems to be about C++17 switch condition variables, i.e. `switch (int x = ...)`(?) Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

szepet added a comment. Hello Maxim! Thanks for working on this! > Ugh, yeah, SwitchStmt is tricky (thank you for pointing to Duff's device > example!). I've tried to resolve several issues with switch revealed by this > testcase, but didn't succeed for now :(. So, it was decided to remove > S

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

m.ostapenko updated this revision to Diff 106408. m.ostapenko retitled this revision from "Add scope information to CFG" to "Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements". m.ostapenko added a comment. Updating the diff. I've dropped SwitchStmt support, let's im

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. In https://reviews.llvm.org/D16403#803518, @m.ostapenko wrote: > There is one more thing I'd like to clarify: since e.g. BreakStmt can > terminate multiple scopes, do I need to create ScopeEnd marks for all of them > to make analyzer's work easier? Because right now only on

[PATCH] D16403: Add scope information to CFG

m.ostapenko added a comment. Hi Artem, I'm sorry for a long delay (nasty corporate issues). In https://reviews.llvm.org/D16403#789957, @NoQ wrote: > Maxim, totally thanks for picking this up! > > Could you explain the idea behind `shouldDeferScopeEnd`, maybe in a code > comment before the funct

[PATCH] D16403: Add scope information to CFG

a.sidorin added a comment. Oh, I see now. I was wrongly thinking that these patches do the same thing and we're duplicating the work. Thank you very much for your explanation, Devin. Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-co

[PATCH] D16403: Add scope information to CFG

dcoughlin added a comment. > @dcoughlin As a reviewer of both patches - could you tell us what's the > difference between them? And how are we going to resolve this issue? These two patches are emitting markers in the CFG for different things. Here is how I would characterize the difference bet

[PATCH] D16403: Add scope information to CFG

zaks.anna added a comment. > @dcoughlin As a reviewer of both patches - could you tell us what's the > difference between them? And how are we going to resolve this issue? Unfortunately, @dcoughlin is on vacation this week; should be back next week. Repository: rL LLVM https://reviews.llvm.

[PATCH] D16403: Add scope information to CFG

NoQ added a comment. Maxim, totally thanks for picking this up! Could you explain the idea behind `shouldDeferScopeEnd`, maybe in a code comment before the function? In https://reviews.llvm.org/D16403#788926, @m.ostapenko wrote: > Current patch should support basic {If, While, For, Compound, S

[PATCH] D16403: Add scope information to CFG

a.sidorin added a comment. Hi Matthias, I have posted a comment about review duplication (more than a year ago!) in your review but you haven't answered. So, all this time we were thinking that you do separate non-related work. @dcoughlin As a reviewer of both patches - could you tell us what's

[PATCH] D16403: Add scope information to CFG

mgehre added a comment. Please consider also https://reviews.llvm.org/D15031 It already handles all possible control flows. Instead of ScopeBegin and ScopeEnd, it introduces LifetimeEnds elements. It's a more specific in that is also correctly models the order of lifetime expiry of variables and

[PATCH] D16403: Add scope information to CFG

m.ostapenko updated this revision to Diff 103719. m.ostapenko set the repository for this revision to rL LLVM. m.ostapenko added a project: clang. m.ostapenko added a comment. So, updating the diff. This is still a very experimental version and any feedback would be greatly appreciated. Current p