[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 262845.
baloghadamsoftware added a comment.

Added support for `ParamRegions` as original region for captured variables by 
blocks and lambdas. Only 2 failing tests left.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -56,7 +56,7 @@
 }
 
 namespace rdar13265460 {
-  struct TrivialSubclass : public Trivial {
+struct TrivialSubclass : public Trivial {
 TrivialSubclass(int x) : Trivial(x), anotherValue(-x) {}
 int anotherValue;
   };
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-5{{UNKNOWN}}
-#endif
-#else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
@@ -1237,7 +1229,7 @@
 struct S {
   S() {}
   S(const S &) {}
-};
+  };
 
 void test() {
   int x = 0;
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/Store.cpp:472
+  break;
+}
+

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > Probably this loop could be written better, without `break` at the end, but 
> > for now it des what it should do. For captured parameters of functions and 
> > blocks we must look for the original `CallExpr` and `LocationContext`. If 
> > it does not exist (we analyze the block of the lambda top-level) we revert 
> > to `VarRegion` since the captured parameters are simple variales for the 
> > block or lambda. However, we cannot do this if the block or lambda is not 
> > analyzed top-level, thus the approach I use above seems to be the correct 
> > one. However, this completely breaks the test `objc-radar17039661.m`. Even 
> > order of the `postCall()` hooks is changed and the test fails because it 
> > cannot find the bug. I try to attach the two different outputs annotated by 
> > debug printouts. @NoQ, do you have an idea what could be wrong here? First 
> > I thought on `BlockDataRegion`s where it seems I have to duplicate lots of 
> > code and also change the capture interface to also enable `ParamRegions` 
> > for the captures. However, in this case it does not seem to play a role.
> The really strange thing is that I originally used a recoursive approach 
> here, instead of the loop, which I still believe is the right one. However, 
> in that case the test failed even if I removed all creations of 
> `ParamRegion`s. The only difference then was that the `LocationContext` of 
> the captured region was the top-level `LocationContext`. This alone changed 
> the calling order of the checker hooks and this happens here as well. It is 
> not the `VarRegion` vs `ParamRegion` problem but the `LocationContext` of the 
> region. I still do not see why this influences the calling order of these 
> hooks. I am already debugging it for almost 15 hours without any clue.
Problem solved. Captured variables are always variables, even if they were 
paramseters originally. I added support for binding the old values to the new 
regions upon capture for `ParamRegion`s as well. This solved the failing test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/Store.cpp:472
+  break;
+}
+

baloghadamsoftware wrote:
> Probably this loop could be written better, without `break` at the end, but 
> for now it des what it should do. For captured parameters of functions and 
> blocks we must look for the original `CallExpr` and `LocationContext`. If it 
> does not exist (we analyze the block of the lambda top-level) we revert to 
> `VarRegion` since the captured parameters are simple variales for the block 
> or lambda. However, we cannot do this if the block or lambda is not analyzed 
> top-level, thus the approach I use above seems to be the correct one. 
> However, this completely breaks the test `objc-radar17039661.m`. Even order 
> of the `postCall()` hooks is changed and the test fails because it cannot 
> find the bug. I try to attach the two different outputs annotated by debug 
> printouts. @NoQ, do you have an idea what could be wrong here? First I 
> thought on `BlockDataRegion`s where it seems I have to duplicate lots of code 
> and also change the capture interface to also enable `ParamRegions` for the 
> captures. However, in this case it does not seem to play a role.
The really strange thing is that I originally used a recoursive approach here, 
instead of the loop, which I still believe is the right one. However, in that 
case the test failed even if I removed all creations of `ParamRegion`s. The 
only difference then was that the `LocationContext` of the captured region was 
the top-level `LocationContext`. This alone changed the calling order of the 
checker hooks and this happens here as well. It is not the `VarRegion` vs 
`ParamRegion` problem but the `LocationContext` of the region. I still do not 
see why this influences the calling order of these hooks. I am already 
debugging it for almost 15 hours without any clue.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

I also tried to compare the exploded graphs, but unfortunately the rewriter 
Python script crashed on them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

F11870916: correct.txt 

F11870922: incorrect.txt 

See the difference between the correct (master branch) and the incorrect (my 
attempt) outputs.




Comment at: clang/lib/StaticAnalyzer/Core/Store.cpp:472
+  break;
+}
+

Probably this loop could be written better, without `break` at the end, but for 
now it des what it should do. For captured parameters of functions and blocks 
we must look for the original `CallExpr` and `LocationContext`. If it does not 
exist (we analyze the block of the lambda top-level) we revert to `VarRegion` 
since the captured parameters are simple variales for the block or lambda. 
However, we cannot do this if the block or lambda is not analyzed top-level, 
thus the approach I use above seems to be the correct one. However, this 
completely breaks the test `objc-radar17039661.m`. Even order of the 
`postCall()` hooks is changed and the test fails because it cannot find the 
bug. I try to attach the two different outputs annotated by debug printouts. 
@NoQ, do you have an idea what could be wrong here? First I thought on 
`BlockDataRegion`s where it seems I have to duplicate lots of code and also 
change the capture interface to also enable `ParamRegions` for the captures. 
However, in this case it does not seem to play a role.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 262430.
baloghadamsoftware added a comment.

Fixed and error in `call_once.cpp` but stuck with debugging of 
`objc-radar17039661.m`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
@@ -25,7 +25,7 @@
 }
 
 void end(const std::vector ) {
-  V.end();
+  const auto i0 = V.end();
 
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 261826.
baloghadamsoftware added a comment.

Only 4 tests failing, but still lot to do.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -56,7 +56,7 @@
 }
 
 namespace rdar13265460 {
-  struct TrivialSubclass : public Trivial {
+struct TrivialSubclass : public Trivial {
 TrivialSubclass(int x) : Trivial(x), anotherValue(-x) {}
 int anotherValue;
   };
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-5{{UNKNOWN}}
-#endif
-#else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
@@ -1237,7 +1229,7 @@
 struct S {
   S() {}
   S(const S &) {}
-};
+  };
 
 void test() {
   int x = 0;
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:614
+  SVB.makeLoc(MRMgr.getParamRegion(ICC->getInheritingConstructor(),
+   Idx, CalleeCtx));
+Bindings.push_back(std::make_pair(ParamLoc, ArgVal));

baloghadamsoftware wrote:
> Is this the correct handling of this kind of calls?
It's not ideal because we're producing a different region for the inherited 
constructor whereas in reality it's the same region as in the inheriting 
constructor.

Let's not dig too deeply into this though; i'll be perfectly happy with doing 
whatever doesn't crash and adding a FIXME for later. I'm pretty sure my 
previous patch is also incorrect for the same reason.

A few tests won't hurt though, and they might help you find the right approach 
as well. After all, the analyzer is a C++ interpreter; all questions about "is 
this the correct handling...?" can be answered by comparing the behavior of the 
analyzer with the behavior of the actual program at run-time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 261218.
baloghadamsoftware added a comment.

Bug reporter visitors updated, failing tests reduced from 19 to 6.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 260944.
baloghadamsoftware added a comment.

Now I made some good progress, thank you @NoQ for your suggestions. Not all of 
them are implemented yet (currently the +-1 with the indices seem to be 
working, thus I will fix that part later), but now I managed to reduce failing 
tests from 340 to 19. Most of these tests are about missing notes, so the next 
task is handling the new `ParamRegion` in the bug reporter visitors. I already 
made some progress there too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added a comment.

Very slow progress, but lots of open questions. @NoQ, please try to answer them.




Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:614
+  SVB.makeLoc(MRMgr.getParamRegion(ICC->getInheritingConstructor(),
+   Idx, CalleeCtx));
+Bindings.push_back(std::make_pair(ParamLoc, ArgVal));

Is this the correct handling of this kind of calls?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:2466
+  // FIXME: What to do with function parameters captured by a block?
+  //Searching the function call in the call stack may fail.
+  if (!BD || Index < BD->param_size()) {

What to do with function parameters captured by a block? I tried to search for 
the owner function backwards in the chain of stack frames, but I did not find 
it.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:2474
+  if (CMD->isOverloadedOperator())
+  ++Index;
+}

Here we have no `CallEvent` so no better came to my mind.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:204
+llvm_unreachable("Maybe we forgot something...");
+  }
+

This whole  branch should be moved to a separate function called e.g. 
`getArgExpr()`. But what to do with param `0` of `CXXNewExpr()`? It is the size 
parameter for which we do not have an `Expr`. For the greater indices we 
subtract `1` from the `Index`.



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:601-603
+if (LCtx->getAnalysis()->isLive(Loc,
+  PR->getOriginExpr()))
+  return true;

baloghadamsoftware wrote:
> NoQ wrote:
> > > I implemented the basic `isLive()` and `getBinding()` functions which 
> > > reduced the number of failing tests by `0`.
> > 
> > This line in particular is very incorrect. In fact, most of the time the 
> > result will be `false`.
> OK, but what should I check for liveness here? We do not have a `Decl`. We 
> could retrieve the `Expr` for the `Arg`, except for `CXXNewOperator` because 
> there the size argument cannot be retrieved.
Here could help the function `getArgExpr()` of `ParamRegion`, I suppose. We 
should put it into the place of `getOriginExpr()`. Is this correct?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 260659.
baloghadamsoftware added a comment.

Wrong diff uploaded previously.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 260657.
baloghadamsoftware added a comment.

Getting the type of parameter regions has no crashes on the test suite.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:601-603
+if (LCtx->getAnalysis()->isLive(Loc,
+  PR->getOriginExpr()))
+  return true;

NoQ wrote:
> > I implemented the basic `isLive()` and `getBinding()` functions which 
> > reduced the number of failing tests by `0`.
> 
> This line in particular is very incorrect. In fact, most of the time the 
> result will be `false`.
OK, but what should I check for liveness here? We do not have a `Decl`. We 
could retrieve the `Expr` for the `Arg`, except for `CXXNewOperator` because 
there the size argument cannot be retrieved.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2007125 , @NoQ wrote:

> > Yes, indeed very poorly. A C++ operator call is either implemented as 
> > CXXOperatorCall or CXXMethodCall randomly. In the former case the this 
> > parameter is explicit at the beginning, in the latter case it is implicit.
>
> We have methods on `CallEvent` to handle this, namely 
> `getAdjustedParameterIndex()` and `getASTArgumentIndex()`. They were added in 
> D49443 .


Unfortunately, in `ExprEngine::VisitCommonDeclRefExpr` we do not have 
`CallEvent`, only `CallExpr`.

> These are blocks. They are like lambdas, just different syntax. They aren't 
> part of Objective-C; they're a non-standard extension to C supported by both 
> gcc and clang.
> 
> The line you quoted is not a call, it only defines a block and assigns it 
> into a variable of block pointer type. Here's the full code of the test:
> 
>   358 void testNilReturnWithBlock(Dummy *p) {
>   359   p = 0;
>   360   Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) {
>   361 return p; // TODO: We should warn in blocks.
>   362   };
>   363   myblock();
>   364 }
>
> 
> The block is invoked on line 363. Parameter `p` of the surrounding function 
> is accessible from the block because it was captured; it is not a parameter 
> of the block itself.

Sorry, all my studies were about standard things only. Everything else was 
banned. This is the philosophy of my university, it seems.

What do you suggest, how to retrieve the type for them? We stored the 
`CallExpr` as `OriginExpr` for the region. Should we store something else in 
case of blocks, or should we handle this in `getType()`? How to distinguish 
parameters of the block from the captured parameters of the function? This case 
sounds very complicated to handle.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Oh, it is not only //Objective-C//. I have been programming in //C// for 25+ 
years and teaching it for 15+ years, but I never met such syntactical 
construction:

  dispatch_sync(queue, ^(void){ 

Here it is worse than at the //Objective-C// code above, because the index is 
`1`, but the number of args is `0`. Thus this is an overindexing by 2. What are 
these strange constructions and how to get parameter type if they have no 
arguments? (Even if I get the function declaration from the stack frame, they 
have no parameters.) How can we have a parameter for them? How to handle them, 
how to return the type for a parameter that should not exist but it does?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Yes, indeed very poorly. A C++ operator call is either implemented as 
> CXXOperatorCall or CXXMethodCall randomly. In the former case the this 
> parameter is explicit at the beginning, in the latter case it is implicit.

We have methods on `CallEvent` to handle this, namely 
`getAdjustedParameterIndex()` and `getASTArgumentIndex()`. They were added in 
D49443 .

> But this is not the only problem with indices. I have no solution for the 
> following call (is it really a call?) resulting in an assertion because 
> overindexing in `nullability.mm`:
> 
>   Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) {
> 
> 
> Even if I try to get the `FunctionDecl` it still says that it has `0` 
> parameters, exactly what `CallExpr` says, but we //have// a parameter here 
> with index of `0`. How can it be? How should I handle this strange case? I 
> only have very basic //Objective-C// knowledge.

These are blocks. They are like lambdas, just different syntax. They aren't 
part of Objective-C; they're a non-standard extension to C supported by both 
gcc and clang.

The line you quoted is not a call, it only defines a block and assigns it into 
a variable of block pointer type. Here's the full code of the test:

  358 void testNilReturnWithBlock(Dummy *p) {
  359   p = 0;
  360   Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) {
  361 return p; // TODO: We should warn in blocks.
  362   };
  363   myblock();
  364 }

The block is invoked on line 363. Parameter `p` of the surrounding function is 
accessible from the block because it was captured; it is not a parameter of the 
block itself.

> As far as I know, you have more than 10 years experience in the Analyzer. I 
> work on the analyzer for less than 5 years, and not full time, because I also 
> create Tidy checkers sometimes, I also have our internal stuff (most of which 
> I would like to upstream).

I started in mid-2014. I too am regularly distracted to stuff that isn't 
particularly educational about the analyzer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2006106 , @NoQ wrote:

> If your change causes a crash, please debug it. That's a completely normal 
> thing that programmers do every day. Unfortunately, I can't afford to 
> spoon-feed you the solution step-by-step.


I do not require it, but I noticed to you at the very beginning that I need 
assistance. I work on the analyzer for less than 5 years, and not full time, 
because I also create Tidy checkers sometimes, I also have our internal stuff 
(most of which I would like to upstream). As far as I know, you have more than 
10 years experience in the Analyzer. I am already have deep knowledge in some 
parts (e.g. constraint handling, AST), I am somewhat familiar with others 
(`SValBuilder`, `ExprEngine`, `BugReporter` etc.) but I never touched regions 
and store.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2007038 , 
@baloghadamsoftware wrote:

> As I wrote, I debugged and analyzed it and I am facing a problem for which I 
> need help. Operator calls are very poorly implemented in the AST.


Yes, indeed very poorly. A C++ operator call is either implemented as 
`CXXOperatorCall` or `CXXMethodCall` randomly. In the former case the `this` 
parameter is explicit at the beginning, in the latter case it is implicit. I 
managed to solve it using this piece of code:

  const auto *Call = cast(CallSite);
  unsigned Index = PVD->getFunctionScopeIndex();
  // For `CallExpr` of C++ member operators we must shift the index by 1
  if (isa(Call)) {
if (const auto *CMD = dyn_cast(SFC->getDecl())) {
  if (CMD->isOverloadedOperator())
++Index;
}
  }

But this is not the only problem with indices. I have no solution for the 
following call (is it really a call?) resulting in an assertion because 
overindexing in `nullability.mm`:

  Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) {

Even if I try to get the `FunctionDecl` it still says that it has `0` 
parameters, exactly what `CallExpr` says, but we //have// a parameter here with 
index of `0`. How can it be? How should I handle this strange case? I only have 
very basic //Objective-C// knowledge.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2006106 , @NoQ wrote:

> If your change causes a crash, please debug it. That's a completely normal 
> thing that programmers do every day. Unfortunately, I can't afford to 
> spoon-feed you the solution step-by-step.


As I wrote, I debugged and analyzed it and I am facing a problem for which I 
need help. Operator calls are very poorly implemented in the AST.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

If your change causes a crash, please debug it. That's a completely normal 
thing that programmers do every day. Unfortunately, I can't afford to 
spoon-feed you the solution step-by-step.




Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:601-603
+if (LCtx->getAnalysis()->isLive(Loc,
+  PR->getOriginExpr()))
+  return true;

> I implemented the basic `isLive()` and `getBinding()` functions which reduced 
> the number of failing tests by `0`.

This line in particular is very incorrect. In fact, most of the time the result 
will be `false`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 260387.
baloghadamsoftware added a comment.

I implemented the basic `isLive()` and `getBinding()` functions which reduced 
the number of failing tests by `0`. I also implemented dynamic calculation of 
the type which increased the number of failing tests by `4`. The reason is 
probably that when `ParamRegion` is created in `Expr::VisitCommonDeclRefExpr()` 
we cannot retrieve information whether the call is a C++ member operator call. 
If it is, we should shift the index by `1`. Using the wrong index and thus 
returning the wrong type causes failing tests and crashes (assertions).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2005413 , 
@baloghadamsoftware wrote:

>   llvm-project/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:333: 
> clang::ento::ProgramStateRef 
> clang::ento::ExprEngine::createTemporaryRegionIfNeeded(clang::ento::ProgramStateRef,
>  const clang::LocationContext*, const clang::Expr*, const clang::Expr*, const 
> clang::ento::SubRegion**): Assertion `!InitValWithAdjustments.getAs() || 
> Loc::isLocType(Result->getType()) || 
> Result->getType()->isMemberPointerType()' failed.
>


For this one I have to increment the `Index` by one if it is a member call. 
Either at the creation of the region or at getting the type. However, after an 
hour of research I found no way to determine this for operator calls. An 
operator call is `CXXOperatorCallExpr` without a method that tells whether it 
is a member or a non-member operator. This is a problem because 
`ParmVarDecl::getFunctionScopeIndex()` returns the real index, but 
`CallExpr::getArg()` takes the "raw" index, where `0` means the implicit `this` 
argument for member calls.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2005419 , 
@baloghadamsoftware wrote:

> Furthermore, there are also problems with `CXXNewExpr`:
>
>   llvm-project/clang/include/clang/AST/ExprCXX.h:2239: clang::Expr* 
> clang::CXXNewExpr::getPlacementArg(unsigned int): Assertion `(I < 
> getNumPlacementArgs()) && "Index out of range!"' failed.
>


This one seems to be resolved by subtracting `1` from `Index` since the first 
argument is not a placement argument but the size.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2005033 , @NoQ wrote:

> > How do we calculate the type then?
>
> Argument expression type + account for value kind (lvalue argument expression 
> means reference parameter, xvalue argument expression mean rvalue reference 
> parameter).


OK, I reid this piece of code instead of storing the type explicitly:

  QualType ParamRegion::getValueType() const {
const Expr *Arg = nullptr;
if (const auto *CE = dyn_cast(OriginExpr)) {
  Arg = CE->getArg(Index);
} else if (const auto *CCE = dyn_cast(OriginExpr)) {
  Arg = CCE->getArg(Index);
} else if (const auto *OCME = dyn_cast(OriginExpr)) {
  Arg = OCME->getArg(Index);
} else if (const auto *CNE = dyn_cast(OriginExpr)) {
  Arg = CNE->getPlacementArg(Index);
} else {
  // FIXME: Any other kind of `Expr`?
  llvm_unreachable("Maybe we forgot something...");
}
  
assert(Arg);
  
switch (Arg->getValueKind()) {
case VK_RValue:
  return Arg->getType();
case VK_LValue:
  return getContext().getLValueReferenceType(Arg->getType());
case VK_XValue:
  return getContext().getRValueReferenceType(Arg->getType());
default:
  llvm_unreachable("Invalid value kind.");
}
  }

This results in the following assertion:

  llvm-project/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:333: 
clang::ento::ProgramStateRef 
clang::ento::ExprEngine::createTemporaryRegionIfNeeded(clang::ento::ProgramStateRef,
 const clang::LocationContext*, const clang::Expr*, const clang::Expr*, const 
clang::ento::SubRegion**): Assertion `!InitValWithAdjustments.getAs() || 
Loc::isLocType(Result->getType()) || Result->getType()->isMemberPointerType()' 
failed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

And this one:

  'Assume' not implemented for this NonLoc
  UNREACHABLE executed at 
/home/edmbalo/llvm-project/clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp:67!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Furthermore, there are also problems with `CXXNewExpr`:

  llvm-project/clang/include/clang/AST/ExprCXX.h:2239: clang::Expr* 
clang::CXXNewExpr::getPlacementArg(unsigned int): Assertion `(I < 
getNumPlacementArgs()) && "Index out of range!"' failed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#2005218 , 
@baloghadamsoftware wrote:

> In D77229#2005042 , @NoQ wrote:
>
> > > no crashes but 130 of 550 analyzer tests failing.
> >
> > You most likely need to fill in the reachibility and liveness code with the 
> > behavior of the new region. I don't seem to see it in your current code but 
> > their default behavior is clearly incorrect; instead, they should behave 
> > exactly like the old region used to.
>
>
> Do you mean the `getBinding...()` function of `RegionStore` and the 
> `isLive...()` functions of `SymbolManager`?


Yup, you're right, i should have mentioned basically the whole RegionStore's 
load/store process instead of just the escape/invalidation process that relies 
on reachibility.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#2004964 , 
@baloghadamsoftware wrote:

> This is not the proper way, `MemRegion`s for parameters in inlined functions 
> should remain `VarRegion`s because parameters **are** variables inside. On 
> the other hand, from outside it does not matter what kind of regions they 
> are. We mainly need the region there to use it as key for `GDM` maps. That is 
> why `ParamRegion`s should only be created for functions that are not inlined. 
> If they have no definition then they cannot be inlined so a `ParamRegion` is 
> OK for them. However, if they have a definition then there is no problem 
> finding the same `Decl` because the definition is that same `Decl` that we 
> use everywhere so it can and should remain a `VarRegion`.


I'm not opposed to this solution. I'm not sure it's actually easier; i agree 
that it may even be less code but because i already tried to do this and 
failed, i'm worried that the tail of regressions may actually be longer.

That said, as of now you cannot know in advance whether the function will be 
inlined or not. "Having a definition" is probably the best approximation that 
you'll ever get.

Also it complicates the construction of the region, given that in order to 
figure out whether we have a definition you suddenly need access to the 
`Environment` (in case of inlined calls by function pointers you cannot figure 
out from the AST whether we have a definition or not) which is a fairly 
unfortunate layering violation. Is such information always available in all the 
places in which we want to reconstruct the region? Will it always be available? 
How will you ensure that?

Also please check if the following example behaves correctly:

  class C { ... };
  
  void foo(C, int) {}
  
  int set_foo_ptr(void (**func)(C, int)) {
*func = foo;
return 0;
  }
  
  void bar() {
void (*func)(C, int);
func(C(), set_foo_ptr());
  }

What's the evaluation order here? Do we actually invoke `foo()` from `bar()` 
here or do we read from the `func` variable before assigning into it thus 
causing a call to uninitialized pointer? In the former case your plan is doomed 
because you need a region into which `C()` lands earlier than you get to know 
any `Decl` at all.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2005042 , @NoQ wrote:

> > no crashes but 130 of 550 analyzer tests failing.
>
> You most likely need to fill in the reachibility and liveness code with the 
> behavior of the new region. I don't seem to see it in your current code but 
> their default behavior is clearly incorrect; instead, they should behave 
> exactly like the old region used to.


Do you mean the `getBinding...()` function of `RegionStore` and the 
`isLive...()` functions of `SymbolManager`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#2005042 , @NoQ wrote:

> > no crashes but 130 of 550 analyzer tests failing.
>
> You most likely need to fill in the reachibility and liveness code with the 
> behavior of the new region. I don't seem to see it in your current code but 
> their default behavior is clearly incorrect; instead, they should behave 
> exactly like the old region used to.


That sound a lot of code duplication for me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#2004964 , 
@baloghadamsoftware wrote:

> Even if I put weeks or months of work into it the code will be worse at the 
> end because there will be lots of branches upon the nature of the variable 
> and alternative ways for reaching the `Decl` from their regions.


What specifically do you want from the `Decl`? Is it the type? We have it. Is 
it the very fact that it's a parameter? We have it. Is it the index of the 
parameter? We have it. Is it the name of the parameter? It's so irrelevant that 
it's not even necessarily consistent across redeclarations, but if you really 
need it you may try to obtain the `Decl` whenever it's available, as the amount 
of cases when the `Decl` is available hasn't changed. What else does a `Decl` 
have to offer?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> How do we calculate the type then?

Argument expression type + account for value kind (lvalue argument expression 
means reference parameter, xvalue argument expression mean rvalue reference 
parameter).

Note that for C-style variadic functions, parameter declarations for variadic 
arguments are also not available. It doesn't mean that we will never be able to 
analyze them. We can analyze everything: after all, we can CodeGen everything 
from the AST. On the contrary, the solution you're implementing now would work 
for C-style variadic functions as well, finally allowing us to inline them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> no crashes but 130 of 550 analyzer tests failing.

You most likely need to fill in the reachibility and liveness code with the 
behavior of the new region. I don't seem to see it in your current code but 
their default behavior is clearly incorrect; instead, they should behave 
exactly like the old region used to.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

I spent some time on thinking about this during the weekend. It is no wonder 
that the tests fail. It seems that many parts of the Analyzer core exploit the 
fact that the `MemRegion`s of parameters are `VarRegion`s.  This is no wonder 
because all `ParamVarDecl` are `VarDecl` and in an inlined function parameters 
behave like variables. This is the correct behavior which I think should not be 
changed. Even if I put weeks or months of work into it the code will be worse 
at the end because there will be lots of branches upon the nature of the 
variable and alternative ways for reaching the `Decl` from their regions. This 
is not the proper way, `MemRegion`s for parameters in inlined functions should 
remain `VarRegion`s because parameters **are** variables inside. On the other 
hand, from outside it does not matter what kind of regions they are. We mainly 
need the region there to use it as key for `GDM` maps. That is why 
`ParamRegion`s should only be created for functions that are not inlined. If 
they have no definition then they cannot be inlined so a `ParamRegion` is OK 
for them. However, if they have a definition then there is no problem finding 
the same `Decl` because the definition is that same `Decl` that we use 
everywhere so it can and should remain a `VarRegion`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 260265.
baloghadamsoftware added a comment.

All regions of parameters are `ParamRegions`, type stored explicitly, no 
crashes but 130 of 550 analyzer tests failing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > There should be only one way to express the parameter region. Let's 
> > > > > > call this one simply `ParamRegion` or something like that, and 
> > > > > > `assert(!isa(D))` in the constructor of `VarRegion`.
> > > > > Do you mean that we should use `ParamRegion` in every case, thus also 
> > > > > when we have the definitioan for the function? I wonder whether it 
> > > > > breaks too many things.
> > > > This will surely not work. The common handling of `ParamVarDecl` and 
> > > > `VarDecl` is soo deeply rooted in the whole analyzer that separating 
> > > > them means creation of a totally new analyzer engine from scratch.
> > > More specifically: whenever a function is inlined, its parameters are 
> > > used as variables via `DeclRefExpr`s. A `DeclRefExpr` refers to a `Decl` 
> > > which is a `ParamVarDecl` but that has reference neither for the 
> > > `CallExpr` (since it is not related to the call, but to the 
> > > `FunctionDecl` or `ObjCMethodDecl`) nore for its `Index` in the call. The 
> > > former is the real problem that cannot be solved even on theoretical 
> > > level: a function which is inlined cannot depend on the different 
> > > `CallExpr`s where it is called. Even worse, if the function is analyzed 
> > > top-level it has not `CallExpr` at all so using `ParamRegion` for its 
> > > parameters is completely impossible.
> > > A `DeclRefExpr` refers to a `Decl` which is a `ParamVarDecl` but that has 
> > > reference neither for the `CallExpr` (since it is not related to the 
> > > call, but to the `FunctionDecl` or `ObjCMethodDecl`)
> > 
> > The call site is available as part of the current location context.
> > 
> > > nore for its Index in the call
> > 
> > It's available as part of `ParmVarDecl`.
> > 
> > > The former is the real problem that cannot be solved even on theoretical 
> > > level: a function which is inlined cannot depend on the different 
> > > `CallExpr`s where it is called
> > 
> > It already depends on the `CallExpr`. `ParmVarDecl`-based `VarRegion`s are 
> > different for different call sites even if `ParmVarDecl` is the same; 
> > moreover, they reside in non-overlapping memory spaces.
> > 
> > > Even worse, if the function is analyzed top-level it has not `CallExpr` 
> > > at all so using `ParamRegion` for its parameters is completely impossible.
> > 
> > Ok, let's make an exception for this case and either use the old 
> > `VarRegion` (given that there's no redecl confusion in this case) or allow 
> > the `CallExpr` to be null (it's still trivially easy to extract all the 
> > necessary information).
> > 
> > > The common handling of `ParamVarDecl` and `VarDecl` is soo deeply rooted 
> > > in the whole analyzer that separating them means creation of a totally 
> > > new analyzer engine from scratch.
> > 
> > I don't see any indication of that yet.
> > or allow the `CallExpr` to be `null`
> 
> How do we calculate the type then? Or should we store it explicitly?
> I don't see any indication of that yet.

But I do. Even if I fix most of the crashes almost all the tests fail for 
different reasons.

But I also have crashes: what to do with functions called through pointers? 
These calls do not have direct callee, thus determining the type of the 
parameter based on the call is difficult. If the function has no prototype, 
then it is impossible. Maybe we should store the type explicitely indeed?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

NoQ wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > There should be only one way to express the parameter region. Let's 
> > > > > call this one simply `ParamRegion` or something like that, and 
> > > > > `assert(!isa(D))` in the constructor of `VarRegion`.
> > > > Do you mean that we should use `ParamRegion` in every case, thus also 
> > > > when we have the definitioan for the function? I wonder whether it 
> > > > breaks too many things.
> > > This will surely not work. The common handling of `ParamVarDecl` and 
> > > `VarDecl` is soo deeply rooted in the whole analyzer that separating them 
> > > means creation of a totally new analyzer engine from scratch.
> > More specifically: whenever a function is inlined, its parameters are used 
> > as variables via `DeclRefExpr`s. A `DeclRefExpr` refers to a `Decl` which 
> > is a `ParamVarDecl` but that has reference neither for the `CallExpr` 
> > (since it is not related to the call, but to the `FunctionDecl` or 
> > `ObjCMethodDecl`) nore for its `Index` in the call. The former is the real 
> > problem that cannot be solved even on theoretical level: a function which 
> > is inlined cannot depend on the different `CallExpr`s where it is called. 
> > Even worse, if the function is analyzed top-level it has not `CallExpr` at 
> > all so using `ParamRegion` for its parameters is completely impossible.
> > A `DeclRefExpr` refers to a `Decl` which is a `ParamVarDecl` but that has 
> > reference neither for the `CallExpr` (since it is not related to the call, 
> > but to the `FunctionDecl` or `ObjCMethodDecl`)
> 
> The call site is available as part of the current location context.
> 
> > nore for its Index in the call
> 
> It's available as part of `ParmVarDecl`.
> 
> > The former is the real problem that cannot be solved even on theoretical 
> > level: a function which is inlined cannot depend on the different 
> > `CallExpr`s where it is called
> 
> It already depends on the `CallExpr`. `ParmVarDecl`-based `VarRegion`s are 
> different for different call sites even if `ParmVarDecl` is the same; 
> moreover, they reside in non-overlapping memory spaces.
> 
> > Even worse, if the function is analyzed top-level it has not `CallExpr` at 
> > all so using `ParamRegion` for its parameters is completely impossible.
> 
> Ok, let's make an exception for this case and either use the old `VarRegion` 
> (given that there's no redecl confusion in this case) or allow the `CallExpr` 
> to be null (it's still trivially easy to extract all the necessary 
> information).
> 
> > The common handling of `ParamVarDecl` and `VarDecl` is soo deeply rooted in 
> > the whole analyzer that separating them means creation of a totally new 
> > analyzer engine from scratch.
> 
> I don't see any indication of that yet.
> or allow the `CallExpr` to be `null`

How do we calculate the type then? Or should we store it explicitly?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+  Optional getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+  Optional getArgObject(unsigned Index, unsigned BlockCount) const;
+
+  Optional getReturnObject(unsigned BlockCount) const;

baloghadamsoftware wrote:
> NoQ wrote:
> > I believe these functions don't need to be optional. There's always a 
> > parameter location and a location to return to (even if the latter is 
> > incorrect due to unimplemented construction context, it'll still be some 
> > dummy temporary region that you can use).
> Do you mean we should return the original `SVal` if parameter or return value 
> location fails? (Thus `LazyCompoundVal` in worst case?
It simply should not fail.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > There should be only one way to express the parameter region. Let's 
> > > > call this one simply `ParamRegion` or something like that, and 
> > > > `assert(!isa(D))` in the constructor of `VarRegion`.
> > > Do you mean that we should use `ParamRegion` in every case, thus also 
> > > when we have the definitioan for the function? I wonder whether it breaks 
> > > too many things.
> > This will surely not work. The common handling of `ParamVarDecl` and 
> > `VarDecl` is soo deeply rooted in the whole analyzer that separating them 
> > means creation of a totally new analyzer engine from scratch.
> More specifically: whenever a function is inlined, its parameters are used as 
> variables via `DeclRefExpr`s. A `DeclRefExpr` refers to a `Decl` which is a 
> `ParamVarDecl` but that has reference neither for the `CallExpr` (since it is 
> not related to the call, but to the `FunctionDecl` or `ObjCMethodDecl`) nore 
> for its `Index` in the call. The former is the real problem that cannot be 
> solved even on theoretical level: a function which is inlined cannot depend 
> on the different `CallExpr`s where it is called. Even worse, if the function 
> is analyzed top-level it has not `CallExpr` at all so using `ParamRegion` for 
> its parameters is completely impossible.
> A `DeclRefExpr` refers to a `Decl` which is a `ParamVarDecl` but that has 
> reference neither for the `CallExpr` (since it is not related to the call, 
> but to the `FunctionDecl` or `ObjCMethodDecl`)

The call site is available as part of the current location context.

> nore for its Index in the call

It's available as part of `ParmVarDecl`.

> The former is the real problem that cannot be solved even on theoretical 
> level: a function which is inlined cannot depend on the different `CallExpr`s 
> where it is called

It already depends on the `CallExpr`. `ParmVarDecl`-based `VarRegion`s are 
different for different call sites even if `ParmVarDecl` is the same; moreover, 
they reside in non-overlapping memory spaces.

> Even worse, if the function is analyzed top-level it has not `CallExpr` at 
> all so using `ParamRegion` for its parameters is completely impossible.

Ok, let's make an exception for this case and either use the old `VarRegion` 
(given that there's no redecl confusion in this case) or allow the `CallExpr` 
to be null (it's still trivially easy to extract all the necessary information).

> The common handling of `ParamVarDecl` and `VarDecl` is soo deeply rooted in 
> the whole analyzer that separating them means creation of a totally new 
> analyzer engine from scratch.

I don't see any indication of that yet.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > There should be only one way to express the parameter region. Let's call 
> > > this one simply `ParamRegion` or something like that, and 
> > > `assert(!isa(D))` in the constructor of `VarRegion`.
> > Do you mean that we should use `ParamRegion` in every case, thus also when 
> > we have the definitioan for the function? I wonder whether it breaks too 
> > many things.
> This will surely not work. The common handling of `ParamVarDecl` and 
> `VarDecl` is soo deeply rooted in the whole analyzer that separating them 
> means creation of a totally new analyzer engine from scratch.
More specifically: whenever a function is inlined, its parameters are used as 
variables via `DeclRefExpr`s. A `DeclRefExpr` refers to a `Decl` which is a 
`ParamVarDecl` but that has reference neither for the `CallExpr` (since it is 
not related to the call, but to the `FunctionDecl` or `ObjCMethodDecl`) nore 
for its `Index` in the call. The former is the real problem that cannot be 
solved even on theoretical level: a function which is inlined cannot depend on 
the different `CallExpr`s where it is called. Even worse, if the function is 
analyzed top-level it has not `CallExpr` at all so using `ParamRegion` for its 
parameters is completely impossible.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

baloghadamsoftware wrote:
> NoQ wrote:
> > There should be only one way to express the parameter region. Let's call 
> > this one simply `ParamRegion` or something like that, and 
> > `assert(!isa(D))` in the constructor of `VarRegion`.
> Do you mean that we should use `ParamRegion` in every case, thus also when we 
> have the definitioan for the function? I wonder whether it breaks too many 
> things.
This will surely not work. The common handling of `ParamVarDecl` and `VarDecl` 
is soo deeply rooted in the whole analyzer that separating them means creation 
of a totally new analyzer engine from scratch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 7 inline comments as done.
baloghadamsoftware added a comment.

Thank you for your comments. Of course I plan to upload all these changes in at 
least three different patches. But first I needed a version that is working. 
However if I split this up I will have to write lots of unit tests.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+  Optional getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+  Optional getArgObject(unsigned Index, unsigned BlockCount) const;
+
+  Optional getReturnObject(unsigned BlockCount) const;

NoQ wrote:
> I believe these functions don't need to be optional. There's always a 
> parameter location and a location to return to (even if the latter is 
> incorrect due to unimplemented construction context, it'll still be some 
> dummy temporary region that you can use).
Do you mean we should return the original `SVal` if parameter or return value 
location fails? (Thus `LazyCompoundVal` in worst case?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

NoQ wrote:
> There should be only one way to express the parameter region. Let's call this 
> one simply `ParamRegion` or something like that, and 
> `assert(!isa(D))` in the constructor of `VarRegion`.
Do you mean that we should use `ParamRegion` in every case, thus also when we 
have the definitioan for the function? I wonder whether it breaks too many 
things.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1066
+  QualType getValueType() const override {
+return CallE->getType();
+  }

NoQ wrote:
> This should be the type of the parameter, not the return type of the call.
Yes, sorry, my mistake.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:278-279
+
+const ConstructionContext
+*CallEvent::getConstructionContext(unsigned BlockCount) const {
+  const CFGBlock *Block;

NoQ wrote:
> This function obviously does not depend on `BlockCount`. You might as well 
> always use `0` as `BlockCount`.
> 
> The ideal solution, of course, is for `CallEvent` to keep a `CFGElementRef` 
> from the start.
I plan to make that change in a separate patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 259272.
baloghadamsoftware added a comment.

First version that seems to be working :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
 glob = 1;
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#endif
+// expected-warning@-2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-// FIXME: Why is destructor not inlined in C++17
 clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-// expected-warning@-3{{TRUE}}
-#else
-// expected-warning@-5{{UNKNOWN}}
-#endif
+// expected-warning@-2{{TRUE}}
 #else
-// expected-warning@-8{{UNKNOWN}}
+// expected-warning@-4{{UNKNOWN}}
 #endif
   } else {
 // The destructor is not called on this branch.
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -93,6 +93,6 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
+  clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$
   clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
@@ -25,7 +25,7 @@
 }
 
 void end(const std::vector ) {
-  V.end();
+  const auto i0 = V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#1995079 , 
@baloghadamsoftware wrote:

> My first attempt for a new special kind of region for parameters of functions 
> without definitions. Currently it causes more failed tests and assertions 
> than ignoring the problem of different `Decl`s. I will investigate these 
> problems tomorrow, but this path seems difficult.


Thank you!! I recommend a separate patch for this :) Yes it's a bit intrusive 
because you'll need to add support for the new region in the `RegionStore` but 
there's nothing extraordinary here as most of the time it'll be treated the 
same way as the old parameter region.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+  Optional getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+  Optional getArgObject(unsigned Index, unsigned BlockCount) const;
+
+  Optional getReturnObject(unsigned BlockCount) const;

I believe these functions don't need to be optional. There's always a parameter 
location and a location to return to (even if the latter is incorrect due to 
unimplemented construction context, it'll still be some dummy temporary region 
that you can use).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;

There should be only one way to express the parameter region. Let's call this 
one simply `ParamRegion` or something like that, and 
`assert(!isa(D))` in the constructor of `VarRegion`.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1066
+  QualType getValueType() const override {
+return CallE->getType();
+  }

This should be the type of the parameter, not the return type of the call.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:235
 
 void IteratorModeling::checkBind(SVal Loc, SVal Val, const Stmt *S,
  CheckerContext ) const {

Do you still need `checkBind` at all?



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:278-279
+
+const ConstructionContext
+*CallEvent::getConstructionContext(unsigned BlockCount) const {
+  const CFGBlock *Block;

This function obviously does not depend on `BlockCount`. You might as well 
always use `0` as `BlockCount`.

The ideal solution, of course, is for `CallEvent` to keep a `CFGElementRef` 
from the start.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:559
+  } else {
+os << "ParamWithoutVarRegion";
+  }

We could still dump `CallE->getID()` and `Index`.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1208
+  return getSubRegion(CE, Index,
+ getStackLocalsRegion(STC));
+}

StackArguments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 259060.
baloghadamsoftware added a comment.

My first attempt for a new special kind of region for parameters of functions 
without definitions. Currently it causes more failed tests and assertions than 
ignoring the problem of different `Decl`s. I will investigate these problems 
tomorrow, but this path seems difficult.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
@@ -25,7 +25,7 @@
 }
 
 void end(const std::vector ) {
-  V.end();
+  const auto i0 = V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
@@ -41,10 +41,10 @@
 // Move
 
 void move_assignment(std::vector , std::vector ) {
-  V1.cbegin();
-  V1.cend();
-  V2.cbegin();
-  V2.cend();
+  const auto i0 = V1.cbegin();
+  const auto i1 = V1.cend();
+  const auto i2 = V2.cbegin();
+  const auto i3 = V2.cend();
   long B1 = clang_analyzer_container_begin(V1);
   long E1 = clang_analyzer_container_end(V1);
   long B2 = clang_analyzer_container_begin(V2);
@@ -70,8 +70,8 @@
 void clang_analyzer_dump(void*);
 
 void push_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -90,8 +90,8 @@
 /// past-the-end position of the container is incremented).
 
 void emplace_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -110,8 +110,8 @@
 /// past-the-end position of the container is decremented).
 
 void pop_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -131,8 +131,8 @@
 /// position of the container is decremented).
 
 void push_front(std::list , int n) {
-  L.cbegin();
-  L.cend();
+  const auto i0 = L.cbegin();
+  const auto i1 = L.cend();
 
   

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 258798.
baloghadamsoftware added a comment.

I commented out lines which prevent creation of `StackFrameContext` for 
non-inlined functions. Now everything works regarding the iterator checkers. I 
also simplified things and created new methods for `CallEvent`: 
`getArgObject()` and `getReturnObject()`.

Two tests fail now: `temporaries.cpp` emits `TRUE` in lines `894` and `918` (I 
wonder whether they are correct) and `explain-svals.cpp` emits `lazily frozen 
compound value of parameter ''` in line `96` which is surely incorrect.

The main problem remains to solve is what we already discussed: either to find 
always the same `Decl` or create a new kind of region for arguments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,7 +1871,8 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
   
 // CHECK:  "checker_messages": [
@@ -1879,4 +1880,6 @@
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
@@ -25,7 +25,7 @@
 }
 
 void end(const std::vector ) {
-  V.end();
+  const auto i0 = V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
@@ -41,10 +41,10 @@
 // Move
 
 void move_assignment(std::vector , std::vector ) {
-  V1.cbegin();
-  V1.cend();
-  V2.cbegin();
-  V2.cend();
+  const auto i0 = V1.cbegin();
+  const auto i1 = V1.cend();
+  const auto i2 = V2.cbegin();
+  const auto i3 = V2.cend();
   long B1 = clang_analyzer_container_begin(V1);
   long E1 = clang_analyzer_container_end(V1);
   long B2 = clang_analyzer_container_begin(V2);
@@ -70,8 +70,8 @@
 void clang_analyzer_dump(void*);
 
 void push_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -90,8 +90,8 @@
 /// past-the-end position of the container is incremented).
 
 void emplace_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -110,8 +110,8 @@
 /// past-the-end position of the container is decremented).
 
 void pop_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -131,8 +131,8 @@
 /// position of the container is decremented).
 
 void push_front(std::list 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#1983693 , 
@baloghadamsoftware wrote:

> I am trying to understand where I have to implement this but I have some 
> problems. The first is that when a function is not inlined it does not have a 
> `StackFrameContext`. What to use instead? I usually try to use the caller's 
> stack frame but it seems to be incorrect and causes assertions. Or should we 
> create a stack frame for non-inlined functions as well?


Even if the function is inlined, we need the stack frame context before it's 
actually created (in fact, it's needed even before we construct the arguments). 
The whole point of D49443  was to learn how to 
pro-actively create stack frame contexts and lifetime-extend it back in time so 
that they were live until the call starts (where they actually become live). 
This part doesn't really change in your case.

> For using the same `FunctionDecl`: would it help if we always use the "first" 
> `Decl` in the chain?

The word you're looking for is "canonical decl". The difficulty is not finding 
such decl but it's making sure it's used consistently, otherwise you won't be 
able to look up the parameter.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#1980665 , @NoQ wrote:

> Aha, ok, sounds like i thought that it's not worth it to inline the 
> constructor for an argument when the call itself is not inlined, therefore i 
> didn't model the construction context for this call. See also D49443#1193290 
> . Your example shows that i was 
> wrong to give up and this is something we should totally implement.
>
> The proper solution is to model the target region in `ExprEngine` as a 
> parameter region based on the `ParmVarDecl` that //would have been used if 
> the function was inlined//, instead of the dummy temporary. Then it'll be 
> kept alive as an object under construction.
>
> The most annoying part is to make sure that you're using the same 
> `FunctionDecl` consistently everywhere. If this turns out to be too annoying, 
> we could also replace `VarRegion{ParmVarDecl}` with a new sort of region that 
> doesn't include a specific `ParmVarDecl` but merely a parameter index (and 
> maybe a type or a call site expr), so that not to bother with redeclarations; 
> this is the right thing to do anyway.


I am trying to understand where I have to implement this but I have some 
problems. The first is that when a function is not inlined it does not have a 
`StackFrameContext`. What to use instead? I usually try to use the caller's 
stack frame but it seems to be incorrect and causes assertions. Or should we 
create a stack frame for non-inlined functions as well?

Where are the parameter regions created? In `case 
ConstructionContext::ArgumentKind` of function 
`ExprEngine::handleConstructionContext()`? Or in 
`CallEvent::getParameterLocation()`? Or somewhere else? I tried to look at 
D49443 , especially the diff where you gave up 
regions for parameters of non-inlined functions but there you only had changes 
in `CallEvent`.

For using the same `FunctionDecl`: would it help if we always use the "first" 
`Decl` in the chain?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Aha, ok, sounds like i thought that it's not worth it to inline the constructor 
for an argument when the call itself is not inlined, therefore i didn't model 
the construction context for this call. See also D49443#1193290 
. Your example shows that i was wrong 
to give up and this is something we should totally implement.

The proper solution is to model the target region in `ExprEngine` as a 
parameter region based on the `ParmVarDecl` that //would have been used if the 
function was inlined//, instead of the dummy temporary. Then it'll be kept 
alive as an object under construction.

The most annoying part is to make sure that you're using the same 
`FunctionDecl` consistently everywhere. If this turns out to be too annoying, 
we could also replace `VarRegion{ParmVarDecl}` with a new sort of region that 
doesn't include a specific `ParmVarDecl` but merely a parameter index (and 
maybe a type or a call site expr), so that not to bother with redeclarations; 
this is the right thing to do anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

OK, I made some experiments. In function `vector_insert_begin()` of test file 
`iterator-modeling.cpp`, line `960` we pass `i0` to `std::vector::insert()`. 
This function has no body at all so regardless of inlining of STL functions is 
enabled, this function is never inlined. Probably for this reason, because it 
does not have its `StackFrame` function `handleConstructionContext()` returns a 
C++ temporary at the end of the function. This temporary is not added to the 
construction context of the function thus it is not marked as live before 
removing dead symbols.

I see two ways now to resolve this. The first is to add these temporaries into 
the construction context. This requires the assertion in 
`ExprEngine::finishArgumentConstruction()` (around line 544, but I have some 
debug printouts there, at the moment so the line number may not be accurate) to 
be changed because it assumes that the region is a `VarRegion`. The second is 
somehow to recognize these temporary regions and mark them as alive based on 
the `CallEvent` by somehow checking the regions bound to its arguments but I 
wonder how exactly to do this.

@NoQ, I need you comments about the right direction about proceeding further.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 256310.
baloghadamsoftware added a comment.

OK. I solved almost every problem except that `checkDeadSymbols()` removes the 
iterator position keyed by the temporary region representing the argument from 
the map before it is needed. Either I must somehow recognize that although the 
region is not "live" but the call whose argument it stores is not postchecked 
yet. I have no idea how to do this. Or, which I would prefer is to modify 
`SymbolReaper` so that `isLive()` returns true for temporary regions storing 
arguments of calls not postchecked yet. I have no idea either how to do this. 
Maybe should we take the `Expr` of `CXXTempObjectRegion`, use `ParentMap` to 
recognize whether it is a parameter for a `FunctionDecl`, but how to check 
whether the call for the function of the `FunctionDecl` is not postchecked yet? 
@NoQ, could you please, help me in this? Thx!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/container-modeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -28,7 +28,7 @@
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
-void begin(const std::vector ) {
+/*void begin(const std::vector ) {
   auto i = v.begin();
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // expected-warning{{TRUE}}
@@ -945,7 +945,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); // expected-warning{{$L.end() - 1}} FIXME: should be $L.end() - 2
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$L.end()}}
   // clang_analyzer_express(clang_analyzer_iterator_position(i3)); FIXME: expect warning $L.end() - 1
-}
+}*/
 
 /// std::vector-like containers: Only the iterators before the insertion point
 ///  remain valid. The past-the-end iterator is also
@@ -965,7 +965,7 @@
   // clang_analyzer_express(clang_analyzer_iterator_position(i2)); FIXME: expect warning $V.begin() - 1
 }
 
-void vector_insert_behind_begin(std::vector , int n) {
+/*void vector_insert_behind_begin(std::vector , int n) {
   auto i0 = V.cbegin(), i1 = ++V.cbegin(), i2 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
@@ -1880,3 +1880,4 @@
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 }
+*/
Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -17,7 +17,7 @@
 void clang_analyzer_warnIfReached();
 
 void begin(const std::vector ) {
-  V.begin();
+  const auto i0 = V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
@@ -25,7 +25,7 @@
 }
 
 void end(const std::vector ) {
-  V.end();
+  const auto i0 = V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
@@ -41,10 +41,10 @@
 // Move
 
 void move_assignment(std::vector , std::vector ) {
-  V1.cbegin();
-  V1.cend();
-  V2.cbegin();
-  V2.cend();
+  const auto i0 = V1.cbegin();
+  const auto i1 = V1.cend();
+  const auto i2 = V2.cbegin();
+  const auto i3 = V2.cend();
   long B1 = clang_analyzer_container_begin(V1);
   long E1 = clang_analyzer_container_end(V1);
   long B2 = clang_analyzer_container_begin(V2);
@@ -70,8 +70,8 @@
 void clang_analyzer_dump(void*);
 
 void push_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
@@ -90,8 +90,8 @@
 /// past-the-end position of the container is incremented).
 
 void emplace_back(std::vector , int n) {
-  V.cbegin();
-  V.cend();
+  const auto i0 = V.cbegin();
+  const auto i1 = 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#1969524 , @NoQ wrote:

> In D77229#1969455 , 
> @baloghadamsoftware wrote:
>
> > The problem is that the `CFGElement` for function `iterator begin() { 
> > return iterator(_start); }` is just a `CFGStmt` and not 
> > `CFGCXXRecordTypedCall`.
>
>
> In which code? It's not about the function, it's about the caller context.


Hmm, it seems that if I do not use the return value of the function explicitly, 
then no construction context is generated for it. Maybe this is how it should 
work so I have to modify the tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

In D77229#1969524 , @NoQ wrote:

> In D77229#1969455 , 
> @baloghadamsoftware wrote:
>
> > The problem is that the `CFGElement` for function `iterator begin() { 
> > return iterator(_start); }` is just a `CFGStmt` and not 
> > `CFGCXXRecordTypedCall`.
>
>
> In which code? It's not about the function, it's about the caller context.


In the very first test of `container_modeling.cpp`:

  void begin(const std::vector ) {
V.begin();
  
clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
clang_analyzer_express(clang_analyzer_container_begin(V)); // 
expected-warning{{$V.begin()}}
   // 
expected-note@-1{{$V.begin()}}
  }




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > Did you mean this piece of code? It returns `_object{struct 
> > > > > simple_iterator_base, S44016}`. Is this correct? If so, I will factor 
> > > > > out this code and put it into a common function to be used by both 
> > > > > this function and the original one.
> > > > No, this one's for members, we've been talking about base classes.
> > > Oh yes, I see it now. But which one then? Maybe line 585? Or the whole 
> > > `switch` expression? Sorry, I am not sure I fully understand this piece 
> > > of code.
> > Now it returns `{SymRegion{reg_$0 > this>},simple_iterator_base}`. Is it correct?
> I don't know. What code are you analyzing in this thread of discussion?
Lines 1778-1813 of `iterator-modeling.cpp`. The only one test not commented out.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#1969455 , 
@baloghadamsoftware wrote:

> The problem is that the `CFGElement` for function `iterator begin() { return 
> iterator(_start); }` is just a `CFGStmt` and not `CFGCXXRecordTypedCall`.


In which code? It's not about the function, it's about the caller context.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> I am totally lost here. What is wrong?

Looks like a bug. Every constructor and every function that returns an object 
by value should have a construction context.




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > Did you mean this piece of code? It returns `_object{struct 
> > > > simple_iterator_base, S44016}`. Is this correct? If so, I will factor 
> > > > out this code and put it into a common function to be used by both this 
> > > > function and the original one.
> > > No, this one's for members, we've been talking about base classes.
> > Oh yes, I see it now. But which one then? Maybe line 585? Or the whole 
> > `switch` expression? Sorry, I am not sure I fully understand this piece of 
> > code.
> Now it returns `{SymRegion{reg_$0 this>},simple_iterator_base}`. Is it correct?
I don't know. What code are you analyzing in this thread of discussion?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Another showstopper: the test `container_modeling.cpp` passed with analyzer 
option `container-inlining=false`. However it faild with 
`container-inlining=true -DINLINE=1`. The problem is that the `CFGElement` for 
function `iterator begin() { return iterator(_start); }` is just a `CFGStmt` 
and not `CFGCXXRecordTypedCall`. I started debugging `CFGBuilder` and its 
construction context is never created. The strange thing is that to have a 
construction context created the function `consumeConstructionContext()` should 
have been called for the function. This function is called by 
`findConstructionContexts()`, but this never happens for `begin()`, just for 
the construction inside it. Actually, `VisitCallExpr()` never calls this 
function, only for the arguments. I am totally lost here. What is wrong?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > Did you mean this piece of code? It returns `_object{struct 
> > > simple_iterator_base, S44016}`. Is this correct? If so, I will factor out 
> > > this code and put it into a common function to be used by both this 
> > > function and the original one.
> > No, this one's for members, we've been talking about base classes.
> Oh yes, I see it now. But which one then? Maybe line 585? Or the whole 
> `switch` expression? Sorry, I am not sure I fully understand this piece of 
> code.
Now it returns `{SymRegion{reg_$0},simple_iterator_base}`. Is it correct?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 255922.
baloghadamsoftware added a comment.

Next attempt.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -28,7 +28,7 @@
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
-void begin(const std::vector ) {
+/*void begin(const std::vector ) {
   auto i = v.begin();
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // expected-warning{{TRUE}}
@@ -1773,7 +1773,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i3)); // expected-warning{{$i1 + 2}} FIXME: Should be $i1 + 1
   // clang_analyzer_express(clang_analyzer_iterator_position(i5)); FIXME: expect warning $i1 + 1
   clang_analyzer_express(clang_analyzer_iterator_position(i4)); // expected-warning{{$FL.end()}}
-}
+  }*/
 
 struct simple_iterator_base {
   simple_iterator_base();
@@ -1812,7 +1812,7 @@
   }
 }
 
-void iter_diff(std::vector ) {
+/*void iter_diff(std::vector ) {
   auto i0 = V.begin(), i1 = V.end();
   ptrdiff_t len = i1 - i0; // no-crash
 }
@@ -1880,3 +1880,4 @@
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 }
+*/
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -109,6 +109,96 @@
   return LValue;
 }
 
+Optional ExprEngine::retrieveFromConstructionContext(
+ProgramStateRef State, const LocationContext *LCtx,
+const ConstructionContext *CC) const {
+  if (CC) {
+switch (CC->getKind()) {
+case ConstructionContext::CXX17ElidedCopyVariableKind:
+case ConstructionContext::SimpleVariableKind: {
+  const auto *DSCC = cast(CC);
+  const auto *DS = DSCC->getDeclStmt();
+  return getObjectUnderConstruction(State, DS, LCtx);
+}
+case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
+case ConstructionContext::SimpleConstructorInitializerKind: {
+  const auto *ICC = cast(CC);
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}
+case ConstructionContext::SimpleReturnedValueKind:
+case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
+  const StackFrameContext *SFC = LCtx->getStackFrame();
+  if (const LocationContext *CallerLCtx = SFC->getParent()) {
+auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
+   .getAs();
+if (!RTC) {
+  // We were unable to find the correct construction context for the
+  // call in the parent stack frame. This is equivalent to not being
+  // able to find construction context at all.
+  break;
+}
+if (isa(CallerLCtx)) {
+  // Unwrap block invocation contexts. They're mostly part of
+  // the current stack frame.
+  CallerLCtx = CallerLCtx->getParent();
+  assert(!isa(CallerLCtx));
+}
+return retrieveFromConstructionContext(
+  State, CallerLCtx, RTC->getConstructionContext());
+  }
+  break;
+}
+case ConstructionContext::ElidedTemporaryObjectKind: {
+  assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
+  const auto *TCC = cast(CC);
+  Optional RetVal = retrieveFromConstructionContext(
+  State, LCtx, TCC->getConstructionContextAfterElision());
+  if (RetVal.hasValue())
+return RetVal;
+  
+  LLVM_FALLTHROUGH;
+}
+case ConstructionContext::SimpleTemporaryObjectKind: {
+  const auto *TCC = cast(CC);
+  const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
+  Optional RetVal;
+  if (BTE) {
+RetVal = getObjectUnderConstruction(State, BTE, LCtx);
+if (RetVal.hasValue())
+  return RetVal;
+  }
+  
+  const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
+  if (MTE)
+RetVal = getObjectUnderConstruction(State, MTE, LCtx);
+
+  return RetVal;
+}
+case 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

In D77229#1967269 , @NoQ wrote:

> In D77229#1966888 , 
> @baloghadamsoftware wrote:
>
> > Not so strange, of course, they are destructed before the `postCall()` as 
> > they should be, but the question still remains: how to keep them alive for 
> > post-checking the call, but not forever.
>
>
> Nope, temporaries are destroyed at the end of the full-expression, which is 
> much later than `PostCall`.
>
> What code are you debugging?


OK, maybe I phrased it incorrectly. It is not destroyed, but not "live" 
anymore. Thus `isLive()` returns false. I am trying to debug 
`vector_insert_begin()` (line 954).




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > Did you mean this piece of code? It returns `_object{struct 
> > simple_iterator_base, S44016}`. Is this correct? If so, I will factor out 
> > this code and put it into a common function to be used by both this 
> > function and the original one.
> No, this one's for members, we've been talking about base classes.
Oh yes, I see it now. But which one then? Maybe line 585? Or the whole `switch` 
expression? Sorry, I am not sure I fully understand this piece of code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#1966888 , 
@baloghadamsoftware wrote:

> Not so strange, of course, they are destructed before the `postCall()` as 
> they should be, but the question still remains: how to keep them alive for 
> post-checking the call, but not forever.


Nope, temporaries are destroyed at the end of the full-expression, which is 
much later than `PostCall`.

What code are you debugging?




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

baloghadamsoftware wrote:
> Did you mean this piece of code? It returns `_object{struct 
> simple_iterator_base, S44016}`. Is this correct? If so, I will factor out 
> this code and put it into a common function to be used by both this function 
> and the original one.
No, this one's for members, we've been talking about base classes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#1966885 , 
@baloghadamsoftware wrote:

> OK, I can reach them from the `ConstructionContext` of the arguments. 
> However, they are temporaries and for some strange reason not alive which 
> means that `checkDeadSymbols()` removes the iterator positions associated to 
> them before the `postCall()` of the call itself. How to correctly prolong 
> their lives until the end of the `postCall()`?


Not so strange, of course, they are destructed before the `postCall()` as they 
should be, but the question still remains: how to keep them alive for 
post-checking the call, but not forever.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#1966711 , 
@baloghadamsoftware wrote:

> Any idea for `LazyCompoundVal` parameters of functions not inlined?


OK, I can reach them from the `ConstructionContext` of the arguments. However, 
they are temporaries and for some strange reason not alive which means that 
`checkDeadSymbols()` removes the iterator positions associated to them before 
the `postCall()` of the call itself. How to correctly prolong their lives until 
the end of the `postCall()`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

Any idea for `LazyCompoundVal` parameters of functions not inlined?




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:231-235
+if (dyn_cast_or_null(LCtx->getParentMap().getParent(E))) 
{
+  MemRegionManager  = getSValBuilder().getRegionManager();
+  return std::make_pair(
+  State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
+}

Did you mean this piece of code? It returns `_object{struct 
simple_iterator_base, S44016}`. Is this correct? If so, I will factor out this 
code and put it into a common function to be used by both this function and the 
original one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 255641.
baloghadamsoftware added a comment.

Experimenting...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -28,7 +28,7 @@
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
-void begin(const std::vector ) {
+/*void begin(const std::vector ) {
   auto i = v.begin();
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // expected-warning{{TRUE}}
@@ -1773,7 +1773,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i3)); // expected-warning{{$i1 + 2}} FIXME: Should be $i1 + 1
   // clang_analyzer_express(clang_analyzer_iterator_position(i5)); FIXME: expect warning $i1 + 1
   clang_analyzer_express(clang_analyzer_iterator_position(i4)); // expected-warning{{$FL.end()}}
-}
+  }*/
 
 struct simple_iterator_base {
   simple_iterator_base();
@@ -1812,7 +1812,7 @@
   }
 }
 
-void iter_diff(std::vector ) {
+/*void iter_diff(std::vector ) {
   auto i0 = V.begin(), i1 = V.end();
   ptrdiff_t len = i1 - i0; // no-crash
 }
@@ -1880,3 +1880,4 @@
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 }
+*/
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -109,6 +109,96 @@
   return LValue;
 }
 
+Optional ExprEngine::retrieveFromConstructionContext(
+ProgramStateRef State, const LocationContext *LCtx,
+const ConstructionContext *CC) const {
+  if (CC) {
+switch (CC->getKind()) {
+case ConstructionContext::CXX17ElidedCopyVariableKind:
+case ConstructionContext::SimpleVariableKind: {
+  const auto *DSCC = cast(CC);
+  const auto *DS = DSCC->getDeclStmt();
+  return getObjectUnderConstruction(State, DS, LCtx);
+}
+case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
+case ConstructionContext::SimpleConstructorInitializerKind: {
+  const auto *ICC = cast(CC);
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}
+case ConstructionContext::SimpleReturnedValueKind:
+case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
+  const StackFrameContext *SFC = LCtx->getStackFrame();
+  if (const LocationContext *CallerLCtx = SFC->getParent()) {
+auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
+   .getAs();
+if (!RTC) {
+  // We were unable to find the correct construction context for the
+  // call in the parent stack frame. This is equivalent to not being
+  // able to find construction context at all.
+  break;
+}
+if (isa(CallerLCtx)) {
+  // Unwrap block invocation contexts. They're mostly part of
+  // the current stack frame.
+  CallerLCtx = CallerLCtx->getParent();
+  assert(!isa(CallerLCtx));
+}
+return retrieveFromConstructionContext(
+  State, CallerLCtx, RTC->getConstructionContext());
+  }
+  break;
+}
+case ConstructionContext::ElidedTemporaryObjectKind: {
+  assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
+  const auto *TCC = cast(CC);
+  Optional RetVal = retrieveFromConstructionContext(
+  State, LCtx, TCC->getConstructionContextAfterElision());
+  if (RetVal.hasValue())
+return RetVal;
+  
+  LLVM_FALLTHROUGH;
+}
+case ConstructionContext::SimpleTemporaryObjectKind: {
+  const auto *TCC = cast(CC);
+  const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
+  Optional RetVal;
+  if (BTE) {
+RetVal = getObjectUnderConstruction(State, BTE, LCtx);
+if (RetVal.hasValue())
+  return RetVal;
+  }
+  
+  const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
+  if (MTE)
+RetVal = getObjectUnderConstruction(State, MTE, LCtx);
+
+  return RetVal;
+}
+case 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:127
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > This does not find anything in the map for base initializers.
> Yup, that's because they're not handled through the 
> objects-under-construction map. See `ExprEngine::handleConstructor()` - the 
> switch on constructor kinds.
> 
> That said, i don't think you need to look it up from the map; just re-compute 
> the region instead exactly how `ExprEngine::handleConstructor()` does it.
(this should be easier because it'll be exactly one line of code here, assuming 
the common part is factored out from `handleConstructor()`)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:127
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}

baloghadamsoftware wrote:
> This does not find anything in the map for base initializers.
Yup, that's because they're not handled through the objects-under-construction 
map. See `ExprEngine::handleConstructor()` - the switch on constructor kinds.

That said, i don't think you need to look it up from the map; just re-compute 
the region instead exactly how `ExprEngine::handleConstructor()` does it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Another problem: parameters can also be `LazyCompoundVal`s, but 
`CallEvent::getParameterLocation()` does not handle non-inlined calls.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D77229#1963485 , @NoQ wrote:

> Uh-oh, it's annoying indeed that you can't easily obtain the current CFG 
> element from inside a `CallEvent`.
>
> Given that every `CallEvent` is in fact associated with a `CFGElement` (but 
> not every `CallEvent` is associated with an `Expr` or have a `Decl`), can we 
> modify the `CallEvent` structure to store a `CFGElementRef` //instead// of 
> `llvm::PointerUnion Origin;`? That might be some 
> work but it should increase our sanity by a lot.


Yes, that seems a good idea. For now I solved this problem, but storing 
something instead of searching for it linearily sound better in regards of 
performance. However my current problem is worse, I have no idea at all, how to 
solve it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Uh-oh, it's annoying indeed that you can't easily obtain the current CFG 
element from inside a `CallEvent`.

Given that every `CallEvent` is in fact associated with a `CFGElement` (but not 
every `CallEvent` is associated with an `Expr` or have a `Decl`), can we modify 
the `CallEvent` structure to store a `CFGElementRef` //instead// of 
`llvm::PointerUnion Origin;`? That might be some 
work but it should increase our sanity by a lot.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 3 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:127
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}

This does not find anything in the map for base initializers.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:230
   const auto *Init = ICC->getCXXCtorInitializer();
   assert(Init->isAnyMemberInitializer());
   const CXXMethodDecl *CurCtor = cast(LCtx->getDecl());

This assertion seems to be unfounded. What to do with base initializers? 
However, in our case it seems that this function should never be executed 
because to the objects under construction are already in the map. However, I do 
not know how to handle base initializers.



Comment at: clang/test/Analysis/iterator-modeling.cpp:1791
 
 struct simple_derived_iterator: public simple_iterator_base {
   int& operator*();

We are crashing here (assertion) on calling the base initializer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 255279.
baloghadamsoftware added a comment.

More progress...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -28,7 +28,7 @@
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
-void begin(const std::vector ) {
+/*void begin(const std::vector ) {
   auto i = v.begin();
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // expected-warning{{TRUE}}
@@ -1773,7 +1773,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i3)); // expected-warning{{$i1 + 2}} FIXME: Should be $i1 + 1
   // clang_analyzer_express(clang_analyzer_iterator_position(i5)); FIXME: expect warning $i1 + 1
   clang_analyzer_express(clang_analyzer_iterator_position(i4)); // expected-warning{{$FL.end()}}
-}
+}*/
 
 struct simple_iterator_base {
   simple_iterator_base();
@@ -1812,7 +1812,7 @@
   }
 }
 
-void iter_diff(std::vector ) {
+/*void iter_diff(std::vector ) {
   auto i0 = V.begin(), i1 = V.end();
   ptrdiff_t len = i1 - i0; // no-crash
 }
@@ -1880,3 +1880,4 @@
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 }
+*/
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -109,6 +109,96 @@
   return LValue;
 }
 
+Optional ExprEngine::retrieveFromConstructionContext(
+ProgramStateRef State, const LocationContext *LCtx,
+const ConstructionContext *CC) const {
+  if (CC) {
+switch (CC->getKind()) {
+case ConstructionContext::CXX17ElidedCopyVariableKind:
+case ConstructionContext::SimpleVariableKind: {
+  const auto *DSCC = cast(CC);
+  const auto *DS = DSCC->getDeclStmt();
+  return getObjectUnderConstruction(State, DS, LCtx);
+}
+case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
+case ConstructionContext::SimpleConstructorInitializerKind: {
+  const auto *ICC = cast(CC);
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}
+case ConstructionContext::SimpleReturnedValueKind:
+case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
+  const StackFrameContext *SFC = LCtx->getStackFrame();
+  if (const LocationContext *CallerLCtx = SFC->getParent()) {
+auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
+   .getAs();
+if (!RTC) {
+  // We were unable to find the correct construction context for the
+  // call in the parent stack frame. This is equivalent to not being
+  // able to find construction context at all.
+  break;
+}
+if (isa(CallerLCtx)) {
+  // Unwrap block invocation contexts. They're mostly part of
+  // the current stack frame.
+  CallerLCtx = CallerLCtx->getParent();
+  assert(!isa(CallerLCtx));
+}
+return retrieveFromConstructionContext(
+  State, CallerLCtx, RTC->getConstructionContext());
+  }
+  break;
+}
+case ConstructionContext::ElidedTemporaryObjectKind: {
+  assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
+  const auto *TCC = cast(CC);
+  Optional RetVal = retrieveFromConstructionContext(
+  State, LCtx, TCC->getConstructionContextAfterElision());
+  if (RetVal.hasValue())
+return RetVal;
+  
+  LLVM_FALLTHROUGH;
+}
+case ConstructionContext::SimpleTemporaryObjectKind: {
+  const auto *TCC = cast(CC);
+  const CXXBindTemporaryExpr *BTE = TCC->getCXXBindTemporaryExpr();
+  Optional RetVal;
+  if (BTE) {
+RetVal = getObjectUnderConstruction(State, BTE, LCtx);
+if (RetVal.hasValue())
+  return RetVal;
+  }
+  
+  const MaterializeTemporaryExpr *MTE = TCC->getMaterializedTemporaryExpr();
+  if (MTE)
+RetVal = getObjectUnderConstruction(State, MTE, LCtx);
+
+  return RetVal;
+}
+case 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Output of

  /home/edmbalo/llvm-project/build/bin/clang -cc1 -internal-isystem 
/mnt/ssd/edmbalo/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc 
-analyze -analyzer-constraints=range -setup-static-analyzer -std=c++11 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection
 -analyzer-config aggressive-binary-operation-simplification=true 
-analyzer-config c++-container-inlining=false 
/home/edmbalo/llvm-project/clang/test/Analysis/iterator-modeling.cpp

is the following:

  Container PostCall
  Original RetVal: lazyCompoundVal{0x55563691e490,i0}
Retrieving Original
  Updated RetVal: 
  Generic PostCall
  Original RetVal: lazyCompoundVal{0x55563691e490,i0}
Retrieving Original
  Updated RetVal: 
  Container PostCall
  Original RetVal: 
lazyCompoundVal{0x555636921ba0,temp_object{std::list::const_iterator, 
S48444}}
Retrieving Original
  Updated RetVal: _object{std::list::const_iterator, S48444}
  Generic PostCall
  Original RetVal: 
lazyCompoundVal{0x555636921ba0,temp_object{std::list::const_iterator, 
S48444}}
Retrieving Original
  Updated RetVal: _object{std::list::const_iterator, S48444}
  Container PostCall
  Original RetVal: lazyCompoundVal{0x555636922d28,i1}
  No Stack Frame!
No construction context!!!
  Generic PostCall
  Original RetVal: lazyCompoundVal{0x555636922d28,i1}
  No Stack Frame!
No construction context!!!
  Container PostCall
  Original RetVal: lazyCompoundVal{0x555636925348,i2}
Retrieving Original
  Updated RetVal: 
  Generic PostCall
  Original RetVal: lazyCompoundVal{0x555636925348,i2}
Retrieving Original
  Updated RetVal: 
  Container PostCall
  Original RetVal: conj_$3{long, LC1, S45775, #1}
  Updated RetVal: conj_$3{long, LC1, S45775, #1}
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: conj_$11{long, LC1, S48640, #1}
  Updated RetVal: conj_$11{long, LC1, S48640, #1}
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: 0 S64b
  Updated RetVal: 0 S64b
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Bind Old: lazyCompoundVal{0x555636925348,i1}
  Bind New: _object{std::list::const_iterator, S49796}
  Container PostCall
  Original RetVal: 
lazyCompoundVal{0x555636921918,temp_object{std::list::const_iterator, 
S49796}}
Retrieving Original
Handling New
  Updated RetVal: _object{std::list::const_iterator, S49796}
  Generic PostCall
  Original RetVal: 
lazyCompoundVal{0x555636921918,temp_object{std::list::const_iterator, 
S49796}}
Retrieving Original
Handling New
  Updated RetVal: _object{std::list::const_iterator, S49796}
  Container PostCall
  Original RetVal: lazyCompoundVal{0x55563692ea10,i3}
  No Stack Frame!
No construction context!!!
  Generic PostCall
  Original RetVal: lazyCompoundVal{0x55563692ea10,i3}
  No Stack Frame!
No construction context!!!
  Container PostCall
  Original RetVal: 1 S64b
  Updated RetVal: 1 S64b
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: 0 S64b
  Updated RetVal: 0 S64b
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: 1 S64b
  Updated RetVal: 1 S64b
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: conj_$3{long, LC1, S45775, #1}
  Updated RetVal: conj_$3{long, LC1, S45775, #1}
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: 0 S64b
  Updated RetVal: 0 S64b
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  Container PostCall
  Original RetVal: conj_$11{long, LC1, S48640, #1}
  Updated RetVal: conj_$11{long, LC1, S48640, #1}
  Container PostCall
  Original RetVal: Unknown
  Updated RetVal: Unknown
  /home/edmbalo/llvm-project/clang/test/Analysis/iterator-modeling.cpp:900:3: 
warning: Not a symbol
clang_analyzer_denote(clang_analyzer_iterator_position(i1), "$i1");
^~
  /home/edmbalo/llvm-project/clang/test/Analysis/iterator-modeling.cpp:904:3: 
warning: TRUE
clang_analyzer_eval(clang_analyzer_iterator_validity(i0)); 
//expected-warning{{TRUE}}
^
  /home/edmbalo/llvm-project/clang/test/Analysis/iterator-modeling.cpp:905:3: 
warning: FALSE
clang_analyzer_eval(clang_analyzer_iterator_validity(i1)); 
//expected-warning{{TRUE}}
^
  /home/edmbalo/llvm-project/clang/test/Analysis/iterator-modeling.cpp:906:3: 
warning: TRUE
clang_analyzer_eval(clang_analyzer_iterator_validity(i2)); 
//expected-warning{{TRUE}}
^
  

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 254765.
baloghadamsoftware added a comment.

Lots of things seem to work now. However, I have a question: what to do with 
the return values of non-inlined calls? It may also be a `LazyCompoundVal`, but 
non-inlined calls do not have `StackFrameContext`. How to retrieve the 
`ConstructionContext` in such cases?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -28,7 +28,7 @@
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
-void begin(const std::vector ) {
+/*void begin(const std::vector ) {
   auto i = v.begin();
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // expected-warning{{TRUE}}
@@ -888,7 +888,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); // expected-warning{{$L.begin() + 1}}
   // clang_analyzer_express(clang_analyzer_iterator_position(i3)); FIXME: expect warning $L.begin()
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$L.end()}}
-}
+}*/
 
 template  Iter return_any_iterator(const Iter );
 
@@ -911,7 +911,7 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$L.end()}}
 }
 
-void list_insert_ahead_of_end(std::list , int n) {
+/*void list_insert_ahead_of_end(std::list , int n) {
   auto i0 = L.cbegin(), i1 = --L.cend(), i2 = L.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(L), "$L.begin()");
@@ -1880,3 +1880,4 @@
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 }
+*/
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -109,6 +109,96 @@
   return LValue;
 }
 
+Optional ExprEngine::retrieveFromConstructionContext(
+ProgramStateRef State, const LocationContext *LCtx,
+const ConstructionContext *CC) const {
+  if (CC) {
+switch (CC->getKind()) {
+case ConstructionContext::CXX17ElidedCopyVariableKind:
+case ConstructionContext::SimpleVariableKind: {
+  const auto *DSCC = cast(CC);
+  const auto *DS = DSCC->getDeclStmt();
+  return getObjectUnderConstruction(State, DS, LCtx);
+}
+case ConstructionContext::CXX17ElidedCopyConstructorInitializerKind:
+case ConstructionContext::SimpleConstructorInitializerKind: {
+  const auto *ICC = cast(CC);
+  const auto *Init = ICC->getCXXCtorInitializer();
+  return getObjectUnderConstruction(State, Init, LCtx);
+}
+case ConstructionContext::SimpleReturnedValueKind:
+case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
+  const StackFrameContext *SFC = LCtx->getStackFrame();
+  if (const LocationContext *CallerLCtx = SFC->getParent()) {
+auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
+   .getAs();
+if (!RTC) {
+  // We were unable to find the correct construction context for the
+  // call in the parent stack frame. This is equivalent to not being
+  // able to find construction context at all.
+  break;
+}
+if (isa(CallerLCtx)) {
+  // Unwrap block invocation contexts. They're mostly part of
+  // the current stack frame.
+  CallerLCtx = CallerLCtx->getParent();
+  assert(!isa(CallerLCtx));
+}
+return retrieveFromConstructionContext(
+  State, CallerLCtx, RTC->getConstructionContext());
+  }
+  break;
+}
+case ConstructionContext::ElidedTemporaryObjectKind: {
+  assert(AMgr.getAnalyzerOptions().ShouldElideConstructors);
+  const auto *TCC = cast(CC);
+  Optional RetVal = retrieveFromConstructionContext(
+  State, LCtx, TCC->getConstructionContextAfterElision());
+  if (RetVal.hasValue())
+return RetVal;
+  
+  LLVM_FALLTHROUGH;
+}
+case ConstructionContext::SimpleTemporaryObjectKind: {
+  const auto *TCC = cast(CC);
+  const CXXBindTemporaryExpr *BTE = 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
 
+Optional ExprEngine::retrieveFromConstructionContext(
+ProgramStateRef State, const LocationContext *LCtx,

Maybe we should merge we should merge this function into 
`handleConstructionContext()`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:436
+  /// context from where the region of its return value can be retrieved.
+  const ConstructionContext *getConstructionContext(unsigned BlockCount) const;
+  

Maybe indicating wheter the call returns a C++ record type should be done via 
using and Optional here (to modernize the api)?



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:342
+if (const auto *CC = Call.getConstructionContext(C.blockCount())) {
+  llvm::errs()<<"  Bingo!\n";
+  auto  = C.getExprEngine();

Debug comment left here :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The assert points out that the object under construction is already tracked and 
you're trying to add it twice. You don't want to add it tho, you just want to 
find the region.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: NoQ.
baloghadamsoftware added a project: clang.
Herald added subscribers: ASDenysPetrov, martong, steakhal, Charusso, 
gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, 
szepet, xazax.hun, whisperity.
Herald added a reviewer: Szelethus.
baloghadamsoftware added a comment.

Testing this patch on `test/Analysis/iterator-modeling.cpp` crashes with the 
following output:

  Handling operator++()
Return Value: lazyCompoundVal{0x55655390c9d8,i1}
Bingo!
  State->get(Key): 
  Key.getItem().getKind(): 0
  clang: 
/home/edmbalo/llvm-project/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:472: 
static clang::ento::ProgramStateRef 
clang::ento::ExprEngine::addObjectUnderConstruction(clang::ento::ProgramStateRef,
 const clang::ConstructionContextItem&, const clang::LocationContext*, 
clang::ento::SVal): Assertion `!State->get(Key) || 
Key.getItem().getKind() == ConstructionContextItem::TemporaryDestructorKind' 
failed.

...

What could be the problem here?


Since accessing the region of LazyCompoundVals is an undocumented and 
unreliable feature, try to find the region of the return value directly, 
skipping both the LazyCompoundVal and its later materialization.

This patch is work in progress.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77229

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -464,9 +464,12 @@
   ConstructedObjectKey Key(Item, LC->getStackFrame());
   // FIXME: Currently the state might already contain the marker due to
   // incorrect handling of temporaries bound to default parameters.
+  if (State->get(Key))
+llvm::errs()<<"State->get(Key): "<<*State->get(Key)<<"\n";
+  llvm::errs()<<"Key.getItem().getKind(): "set(Key, V);
 }
 
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -257,6 +257,24 @@
   return VR;
 }
 
+const ConstructionContext
+*CallEvent::getConstructionContext(unsigned BlockCount) const {
+  const StackFrameContext *StackFrame = getCalleeStackFrame(BlockCount);
+  if (!StackFrame)
+return nullptr;
+
+  const CFGBlock *Block = StackFrame->getCallSiteBlock();
+  if (!Block)
+return nullptr;
+
+  const auto RecCall =
+(*Block)[StackFrame->getIndex()].getAs();
+  if (!RecCall)
+return nullptr;
+
+  return RecCall->getConstructionContext();
+}
+
 /// Returns true if a type is a pointer-to-const or reference-to-const
 /// with no further indirection.
 static bool isPointerToConst(QualType Ty) {
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -301,61 +301,74 @@
 IteratorModeling::handleOverloadedOperator(CheckerContext ,
const CallEvent ,
OverloadedOperatorKind Op) const {
-if (isSimpleComparisonOperator(Op)) {
-  const auto *OrigExpr = Call.getOriginExpr();
-  if (!OrigExpr)
-return;
+  const auto *OrigExpr = Call.getOriginExpr();
+  if (!OrigExpr)
+return;
 
-  if (const auto *InstCall = dyn_cast()) {
-handleComparison(C, OrigExpr, Call.getReturnValue(),
- InstCall->getCXXThisVal(), Call.getArgSVal(0), Op);
-return;
-  }
+  if (isSimpleComparisonOperator(Op)) {
+const auto *OrigExpr = Call.getOriginExpr();
+if (!OrigExpr)
+  return;
 
-  handleComparison(C, OrigExpr, Call.getReturnValue(), Call.getArgSVal(0),
- Call.getArgSVal(1), Op);
+if (const auto *InstCall = dyn_cast()) {
+  handleComparison(C, OrigExpr, Call.getReturnValue(),
+   InstCall->getCXXThisVal(), Call.getArgSVal(0), Op);
   return;
-} else if (isRandomIncrOrDecrOperator(Op)) {
-  const auto *OrigExpr = Call.getOriginExpr();
-  if (!OrigExpr)
-return;
+}
 
-  if (const auto *InstCall = dyn_cast()) {
-if (Call.getNumArgs() 

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Testing this patch on `test/Analysis/iterator-modeling.cpp` crashes with the 
following output:

  Handling operator++()
Return Value: lazyCompoundVal{0x55655390c9d8,i1}
Bingo!
  State->get(Key): 
  Key.getItem().getKind(): 0
  clang: 
/home/edmbalo/llvm-project/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:472: 
static clang::ento::ProgramStateRef 
clang::ento::ExprEngine::addObjectUnderConstruction(clang::ento::ProgramStateRef,
 const clang::ConstructionContextItem&, const clang::LocationContext*, 
clang::ento::SVal): Assertion `!State->get(Key) || 
Key.getItem().getKind() == ConstructionContextItem::TemporaryDestructorKind' 
failed.

...

What could be the problem here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits