[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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

Rebased, no tricks with `LazyCompoundVal`s are used anymore.


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

https://reviews.llvm.org/D75677

Files:
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/iterator-range.cpp

Index: clang/test/Analysis/iterator-range.cpp
===
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -957,3 +957,4 @@
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
   // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
+
Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -116,7 +116,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto j = i++; // expected-note 2{{Iterator 'i' incremented by 1}}
+  auto j = i++; // expected-note{{Iterator 'i' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning-re {{$v.begin() + 1{{$
// expected-note@-1{{$v.begin() + 1}}
@@ -129,7 +129,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto j = i--; // expected-note 2{{Iterator 'i' decremented by 1}}
+  auto j = i--; // expected-note{{Iterator 'i' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning-re {{$v.end() - 1{{$
// expected-note@-1{{$v.end() - 1}}
@@ -223,7 +223,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 + 2; // expected-note 4{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 + 2; // expected-note 2{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -231,8 +231,6 @@
 // expected-note@-1{{$v.begin()}}
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re{{$v.begin() + 2{{$
 // expected-note@-1{{$v.begin() + 2}}
-  clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$v.begin() + 2}}
-// expected-note@-1{{$v.begin() + 2}}
 }
 
 void plus_negative(const std::vector ) {
@@ -240,7 +238,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 + (-2); // expected-note 3{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 + (-2); // expected-note 2{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -255,7 +253,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 + 0; // expected-note 2{{Iterator 'i1' unchanged}}
+  auto i2 = i1 + 0; // expected-note{{Iterator 'i1' unchanged}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -268,7 +266,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 - 2;  // expected-note 3{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 - 2; // expected-note 2{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -283,7 +281,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 - (-2); // expected-note 3{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 - (-2); // expected-note 2{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -298,7 +296,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 - 0; // expected-note 2{{Iterator 'i1' unchanged}}
+  auto i2 = i1 - 0; // expected-note{{Iterator 'i1' unchanged}}
 
   

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > baloghadamsoftware wrote:
> > > > > > > > > > NoQ wrote:
> > > > > > > > > > > I'm opposed to this code for the same reason that i'm 
> > > > > > > > > > > opposed to it in the debug checker. Parent region is an 
> > > > > > > > > > > undocumented implementation detail of `RegionStore`. It 
> > > > > > > > > > > is supposed to be immaterial to the user. You should not 
> > > > > > > > > > > rely on its exact value.
> > > > > > > > > > > 
> > > > > > > > > > > @baloghadamsoftware Can we eliminate all such code from 
> > > > > > > > > > > iterator checkers and instead identify all iterators by 
> > > > > > > > > > > regions in which they're stored? Does my improved C++ 
> > > > > > > > > > > support help with this anyhow whenever it kicks in?
> > > > > > > > > > How to find the region where it is stored? I am open to 
> > > > > > > > > > find better solutions, but it was the only one I could find 
> > > > > > > > > > so far. If we ignore `LazyCompoundVal` then everything 
> > > > > > > > > > falls apart, we can remove all the iterator-related 
> > > > > > > > > > checkers.
> > > > > > > > > It's the region from which you loaded it. If you obtained it 
> > > > > > > > > as `Call.getArgSVal()` then it's the parameter region for the 
> > > > > > > > > call. If you obtained it as `Call.getReturnValue()` then it's 
> > > > > > > > > the target region can be obtained by looking at the 
> > > > > > > > > //construction context// for the call.
> > > > > > > > `LazyCompoundVal` and friends seem to be a constantly emerging 
> > > > > > > > headache for almost everyone. For how long I've spent in the 
> > > > > > > > analyzer, and have religiously studied conversations and your 
> > > > > > > > workbook about it, I still feel anxious whenever it comes up.
> > > > > > > > 
> > > > > > > > It would be great to have this documented in the code sometime.
> > > > > > > Do you mean `CallEvent::getParameterLocation()` for arguments? 
> > > > > > > What is the //construction context// for the call? How can it be 
> > > > > > > obtained?
> > > > > > I do not know exactly how many place `LazyCompoundVal`  appears, 
> > > > > > but one place for sure is in 
> > > > > > `MaterializeTemporaryExpr::getSubExpr()`. What to use there instead?
> > > > > I also get it in the `Val` parameter of `checkBind()`.
> > > > Now I spent a whole day in vain. You probably mean 
> > > > `ExprEngine::getObjectUnderConstruction()`, (which takes 
> > > > `ConstructionContextItem` as its argument) but it turned out that there 
> > > > are no objects under construction in `checkPostCall()`. (Stack dump 
> > > > says `constructing_objects` as `null`.) It seems that the //only 
> > > > working solution// is the current one. I am not opposed to find better 
> > > > working solutions, but we cannot spend months to completely rewrite 
> > > > parts of the analyzer for such a simple patch. And note tags are 
> > > > definitely needed for iterator checkers.
> > > "A whole day"? "One simple patch"? Give me a break.
> > > 
> > > We've been discussing this problem since your very first implementation 
> > > of the iterator checker dozens of patches ago, and i spent //six months// 
> > > of my full time work trying to make this part of the analyzer operate in 
> > > an obvious, principled manner, even made a dev meeting talk about it in 
> > > order to explain how it works. And all you do is keep insisting that your 
> > > solution is "working" even though literally nobody understands how, even 
> > > you.
> > > 
> > > Out of all the contributors who bring patches to me every day, only 
> > > @Szelethus is actively addressing the technical debt. This is not "one 
> > > simple patch". This has to stop at some point, and i expect you, being a 
> > > fairly senior contributor at this point, to put at least a slight effort 
> > > into good engineering practices, apply the necessary amount of critical 
> > > thinking, take basic responsibility for your code.
> > > 
> > > I don't mind if you address this issue immediately after this patch if 
> > > you urgently need this patch landed. But i wouldn't like this to go on 
> > > forever.
> > > 
> > > > dump says `constructing_objects` as `null`
> > > 
> > > You shouldn't be spending the whole day before noticing it. Whenever 
> > > something isn't working, the first thing you do should be dump the 
> > > ExplodedGraph and 

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > Szelethus wrote:
> > > > > > > NoQ wrote:
> > > > > > > > baloghadamsoftware wrote:
> > > > > > > > > NoQ wrote:
> > > > > > > > > > I'm opposed to this code for the same reason that i'm 
> > > > > > > > > > opposed to it in the debug checker. Parent region is an 
> > > > > > > > > > undocumented implementation detail of `RegionStore`. It is 
> > > > > > > > > > supposed to be immaterial to the user. You should not rely 
> > > > > > > > > > on its exact value.
> > > > > > > > > > 
> > > > > > > > > > @baloghadamsoftware Can we eliminate all such code from 
> > > > > > > > > > iterator checkers and instead identify all iterators by 
> > > > > > > > > > regions in which they're stored? Does my improved C++ 
> > > > > > > > > > support help with this anyhow whenever it kicks in?
> > > > > > > > > How to find the region where it is stored? I am open to find 
> > > > > > > > > better solutions, but it was the only one I could find so 
> > > > > > > > > far. If we ignore `LazyCompoundVal` then everything falls 
> > > > > > > > > apart, we can remove all the iterator-related checkers.
> > > > > > > > It's the region from which you loaded it. If you obtained it as 
> > > > > > > > `Call.getArgSVal()` then it's the parameter region for the 
> > > > > > > > call. If you obtained it as `Call.getReturnValue()` then it's 
> > > > > > > > the target region can be obtained by looking at the 
> > > > > > > > //construction context// for the call.
> > > > > > > `LazyCompoundVal` and friends seem to be a constantly emerging 
> > > > > > > headache for almost everyone. For how long I've spent in the 
> > > > > > > analyzer, and have religiously studied conversations and your 
> > > > > > > workbook about it, I still feel anxious whenever it comes up.
> > > > > > > 
> > > > > > > It would be great to have this documented in the code sometime.
> > > > > > Do you mean `CallEvent::getParameterLocation()` for arguments? What 
> > > > > > is the //construction context// for the call? How can it be 
> > > > > > obtained?
> > > > > I do not know exactly how many place `LazyCompoundVal`  appears, but 
> > > > > one place for sure is in `MaterializeTemporaryExpr::getSubExpr()`. 
> > > > > What to use there instead?
> > > > I also get it in the `Val` parameter of `checkBind()`.
> > > Now I spent a whole day in vain. You probably mean 
> > > `ExprEngine::getObjectUnderConstruction()`, (which takes 
> > > `ConstructionContextItem` as its argument) but it turned out that there 
> > > are no objects under construction in `checkPostCall()`. (Stack dump says 
> > > `constructing_objects` as `null`.) It seems that the //only working 
> > > solution// is the current one. I am not opposed to find better working 
> > > solutions, but we cannot spend months to completely rewrite parts of the 
> > > analyzer for such a simple patch. And note tags are definitely needed for 
> > > iterator checkers.
> > "A whole day"? "One simple patch"? Give me a break.
> > 
> > We've been discussing this problem since your very first implementation of 
> > the iterator checker dozens of patches ago, and i spent //six months// of 
> > my full time work trying to make this part of the analyzer operate in an 
> > obvious, principled manner, even made a dev meeting talk about it in order 
> > to explain how it works. And all you do is keep insisting that your 
> > solution is "working" even though literally nobody understands how, even 
> > you.
> > 
> > Out of all the contributors who bring patches to me every day, only 
> > @Szelethus is actively addressing the technical debt. This is not "one 
> > simple patch". This has to stop at some point, and i expect you, being a 
> > fairly senior contributor at this point, to put at least a slight effort 
> > into good engineering practices, apply the necessary amount of critical 
> > thinking, take basic responsibility for your code.
> > 
> > I don't mind if you address this issue immediately after this patch if you 
> > urgently need this patch landed. But i wouldn't like this to go on forever.
> > 
> > > dump says `constructing_objects` as `null`
> > 
> > You shouldn't be spending the whole day before noticing it. Whenever 
> > something isn't working, the first thing you do should be dump the 
> > ExplodedGraph and look at it. You would have noticed it right away.
> > 
> > Was the object never there or was it removed too early? If there are no 
> > objects under construction tracked but you need them tracked, make them 
> > tracked for as long as you need. That's the 

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > Szelethus wrote:
> > > > > > NoQ wrote:
> > > > > > > baloghadamsoftware wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > I'm opposed to this code for the same reason that i'm opposed 
> > > > > > > > > to it in the debug checker. Parent region is an undocumented 
> > > > > > > > > implementation detail of `RegionStore`. It is supposed to be 
> > > > > > > > > immaterial to the user. You should not rely on its exact 
> > > > > > > > > value.
> > > > > > > > > 
> > > > > > > > > @baloghadamsoftware Can we eliminate all such code from 
> > > > > > > > > iterator checkers and instead identify all iterators by 
> > > > > > > > > regions in which they're stored? Does my improved C++ support 
> > > > > > > > > help with this anyhow whenever it kicks in?
> > > > > > > > How to find the region where it is stored? I am open to find 
> > > > > > > > better solutions, but it was the only one I could find so far. 
> > > > > > > > If we ignore `LazyCompoundVal` then everything falls apart, we 
> > > > > > > > can remove all the iterator-related checkers.
> > > > > > > It's the region from which you loaded it. If you obtained it as 
> > > > > > > `Call.getArgSVal()` then it's the parameter region for the call. 
> > > > > > > If you obtained it as `Call.getReturnValue()` then it's the 
> > > > > > > target region can be obtained by looking at the //construction 
> > > > > > > context// for the call.
> > > > > > `LazyCompoundVal` and friends seem to be a constantly emerging 
> > > > > > headache for almost everyone. For how long I've spent in the 
> > > > > > analyzer, and have religiously studied conversations and your 
> > > > > > workbook about it, I still feel anxious whenever it comes up.
> > > > > > 
> > > > > > It would be great to have this documented in the code sometime.
> > > > > Do you mean `CallEvent::getParameterLocation()` for arguments? What 
> > > > > is the //construction context// for the call? How can it be obtained?
> > > > I do not know exactly how many place `LazyCompoundVal`  appears, but 
> > > > one place for sure is in `MaterializeTemporaryExpr::getSubExpr()`. What 
> > > > to use there instead?
> > > I also get it in the `Val` parameter of `checkBind()`.
> > Now I spent a whole day in vain. You probably mean 
> > `ExprEngine::getObjectUnderConstruction()`, (which takes 
> > `ConstructionContextItem` as its argument) but it turned out that there are 
> > no objects under construction in `checkPostCall()`. (Stack dump says 
> > `constructing_objects` as `null`.) It seems that the //only working 
> > solution// is the current one. I am not opposed to find better working 
> > solutions, but we cannot spend months to completely rewrite parts of the 
> > analyzer for such a simple patch. And note tags are definitely needed for 
> > iterator checkers.
> "A whole day"? "One simple patch"? Give me a break.
> 
> We've been discussing this problem since your very first implementation of 
> the iterator checker dozens of patches ago, and i spent //six months// of my 
> full time work trying to make this part of the analyzer operate in an 
> obvious, principled manner, even made a dev meeting talk about it in order to 
> explain how it works. And all you do is keep insisting that your solution is 
> "working" even though literally nobody understands how, even you.
> 
> Out of all the contributors who bring patches to me every day, only 
> @Szelethus is actively addressing the technical debt. This is not "one simple 
> patch". This has to stop at some point, and i expect you, being a fairly 
> senior contributor at this point, to put at least a slight effort into good 
> engineering practices, apply the necessary amount of critical thinking, take 
> basic responsibility for your code.
> 
> I don't mind if you address this issue immediately after this patch if you 
> urgently need this patch landed. But i wouldn't like this to go on forever.
> 
> > dump says `constructing_objects` as `null`
> 
> You shouldn't be spending the whole day before noticing it. Whenever 
> something isn't working, the first thing you do should be dump the 
> ExplodedGraph and look at it. You would have noticed it right away.
> 
> Was the object never there or was it removed too early? If there are no 
> objects under construction tracked but you need them tracked, make them 
> tracked for as long as you need. That's the whole point of the 
> objects-under-construction map.
Sorry, @NoQ, I wrote this comment before starting the WIP patch. I agree that 

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > I'm opposed to this code for the same reason that i'm opposed 
> > > > > > > > to it in the debug checker. Parent region is an undocumented 
> > > > > > > > implementation detail of `RegionStore`. It is supposed to be 
> > > > > > > > immaterial to the user. You should not rely on its exact value.
> > > > > > > > 
> > > > > > > > @baloghadamsoftware Can we eliminate all such code from 
> > > > > > > > iterator checkers and instead identify all iterators by regions 
> > > > > > > > in which they're stored? Does my improved C++ support help with 
> > > > > > > > this anyhow whenever it kicks in?
> > > > > > > How to find the region where it is stored? I am open to find 
> > > > > > > better solutions, but it was the only one I could find so far. If 
> > > > > > > we ignore `LazyCompoundVal` then everything falls apart, we can 
> > > > > > > remove all the iterator-related checkers.
> > > > > > It's the region from which you loaded it. If you obtained it as 
> > > > > > `Call.getArgSVal()` then it's the parameter region for the call. If 
> > > > > > you obtained it as `Call.getReturnValue()` then it's the target 
> > > > > > region can be obtained by looking at the //construction context// 
> > > > > > for the call.
> > > > > `LazyCompoundVal` and friends seem to be a constantly emerging 
> > > > > headache for almost everyone. For how long I've spent in the 
> > > > > analyzer, and have religiously studied conversations and your 
> > > > > workbook about it, I still feel anxious whenever it comes up.
> > > > > 
> > > > > It would be great to have this documented in the code sometime.
> > > > Do you mean `CallEvent::getParameterLocation()` for arguments? What is 
> > > > the //construction context// for the call? How can it be obtained?
> > > I do not know exactly how many place `LazyCompoundVal`  appears, but one 
> > > place for sure is in `MaterializeTemporaryExpr::getSubExpr()`. What to 
> > > use there instead?
> > I also get it in the `Val` parameter of `checkBind()`.
> Now I spent a whole day in vain. You probably mean 
> `ExprEngine::getObjectUnderConstruction()`, (which takes 
> `ConstructionContextItem` as its argument) but it turned out that there are 
> no objects under construction in `checkPostCall()`. (Stack dump says 
> `constructing_objects` as `null`.) It seems that the //only working 
> solution// is the current one. I am not opposed to find better working 
> solutions, but we cannot spend months to completely rewrite parts of the 
> analyzer for such a simple patch. And note tags are definitely needed for 
> iterator checkers.
"A whole day"? "One simple patch"? Give me a break.

We've been discussing this problem since your very first implementation of the 
iterator checker dozens of patches ago, and i spent //six months// of my full 
time work trying to make this part of the analyzer operate in an obvious, 
principled manner, even made a dev meeting talk about it in order to explain 
how it works. And all you do is keep insisting that your solution is "working" 
even though literally nobody understands how, even you.

Out of all the contributors who bring patches to me every day, only @Szelethus 
is actively addressing the technical debt. This is not "one simple patch". This 
has to stop at some point, and i expect you, being a fairly senior contributor 
at this point, to put at least a slight effort into good engineering practices, 
apply the necessary amount of critical thinking, take basic responsibility for 
your code.

I don't mind if you address this issue immediately after this patch if you 
urgently need this patch landed. But i wouldn't like this to go on forever.

> dump says `constructing_objects` as `null`

You shouldn't be spending the whole day before noticing it. Whenever something 
isn't working, the first thing you do should be dump the ExplodedGraph and look 
at it. You would have noticed it right away.

Was the object never there or was it removed too early? If there are no objects 
under construction tracked but you need them tracked, make them tracked for as 
long as you need. That's the whole point of the objects-under-construction map.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > I'm opposed to this code for the same reason that i'm opposed to 
> > > > > > > it in the debug checker. Parent region is an undocumented 
> > > > > > > implementation detail of `RegionStore`. It is supposed to be 
> > > > > > > immaterial to the user. You should not rely on its exact value.
> > > > > > > 
> > > > > > > @baloghadamsoftware Can we eliminate all such code from iterator 
> > > > > > > checkers and instead identify all iterators by regions in which 
> > > > > > > they're stored? Does my improved C++ support help with this 
> > > > > > > anyhow whenever it kicks in?
> > > > > > How to find the region where it is stored? I am open to find better 
> > > > > > solutions, but it was the only one I could find so far. If we 
> > > > > > ignore `LazyCompoundVal` then everything falls apart, we can remove 
> > > > > > all the iterator-related checkers.
> > > > > It's the region from which you loaded it. If you obtained it as 
> > > > > `Call.getArgSVal()` then it's the parameter region for the call. If 
> > > > > you obtained it as `Call.getReturnValue()` then it's the target 
> > > > > region can be obtained by looking at the //construction context// for 
> > > > > the call.
> > > > `LazyCompoundVal` and friends seem to be a constantly emerging headache 
> > > > for almost everyone. For how long I've spent in the analyzer, and have 
> > > > religiously studied conversations and your workbook about it, I still 
> > > > feel anxious whenever it comes up.
> > > > 
> > > > It would be great to have this documented in the code sometime.
> > > Do you mean `CallEvent::getParameterLocation()` for arguments? What is 
> > > the //construction context// for the call? How can it be obtained?
> > I do not know exactly how many place `LazyCompoundVal`  appears, but one 
> > place for sure is in `MaterializeTemporaryExpr::getSubExpr()`. What to use 
> > there instead?
> I also get it in the `Val` parameter of `checkBind()`.
Now I spent a whole day in vain. You probably mean 
`ExprEngine::getObjectUnderConstruction()`, (which takes 
`ConstructionContextItem` as its argument) but it turned out that there are no 
objects under construction in `checkPostCall()`. (Stack dump says 
`constructing_objects` as `null`.) It seems that the //only working solution// 
is the current one. I am not opposed to find better working solutions, but we 
cannot spend months to completely rewrite parts of the analyzer for such a 
simple patch. And note tags are definitely needed for iterator checkers.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > I'm opposed to this code for the same reason that i'm opposed to it 
> > > > > > in the debug checker. Parent region is an undocumented 
> > > > > > implementation detail of `RegionStore`. It is supposed to be 
> > > > > > immaterial to the user. You should not rely on its exact value.
> > > > > > 
> > > > > > @baloghadamsoftware Can we eliminate all such code from iterator 
> > > > > > checkers and instead identify all iterators by regions in which 
> > > > > > they're stored? Does my improved C++ support help with this anyhow 
> > > > > > whenever it kicks in?
> > > > > How to find the region where it is stored? I am open to find better 
> > > > > solutions, but it was the only one I could find so far. If we ignore 
> > > > > `LazyCompoundVal` then everything falls apart, we can remove all the 
> > > > > iterator-related checkers.
> > > > It's the region from which you loaded it. If you obtained it as 
> > > > `Call.getArgSVal()` then it's the parameter region for the call. If you 
> > > > obtained it as `Call.getReturnValue()` then it's the target region can 
> > > > be obtained by looking at the //construction context// for the call.
> > > `LazyCompoundVal` and friends seem to be a constantly emerging headache 
> > > for almost everyone. For how long I've spent in the analyzer, and have 
> > > religiously studied conversations and your workbook about it, I still 
> > > feel anxious whenever it comes up.
> > > 
> > > It would be great to have this documented in the code sometime.
> > Do you mean `CallEvent::getParameterLocation()` for arguments? What is the 
> > //construction context// for the call? How can it be obtained?
> I do not know exactly how many place `LazyCompoundVal`  appears, but one 
> place for sure is in `MaterializeTemporaryExpr::getSubExpr()`. What to use 
> there instead?
I also get it in the `Val` parameter of `checkBind()`.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > I'm opposed to this code for the same reason that i'm opposed to it 
> > > > > in the debug checker. Parent region is an undocumented implementation 
> > > > > detail of `RegionStore`. It is supposed to be immaterial to the user. 
> > > > > You should not rely on its exact value.
> > > > > 
> > > > > @baloghadamsoftware Can we eliminate all such code from iterator 
> > > > > checkers and instead identify all iterators by regions in which 
> > > > > they're stored? Does my improved C++ support help with this anyhow 
> > > > > whenever it kicks in?
> > > > How to find the region where it is stored? I am open to find better 
> > > > solutions, but it was the only one I could find so far. If we ignore 
> > > > `LazyCompoundVal` then everything falls apart, we can remove all the 
> > > > iterator-related checkers.
> > > It's the region from which you loaded it. If you obtained it as 
> > > `Call.getArgSVal()` then it's the parameter region for the call. If you 
> > > obtained it as `Call.getReturnValue()` then it's the target region can be 
> > > obtained by looking at the //construction context// for the call.
> > `LazyCompoundVal` and friends seem to be a constantly emerging headache for 
> > almost everyone. For how long I've spent in the analyzer, and have 
> > religiously studied conversations and your workbook about it, I still feel 
> > anxious whenever it comes up.
> > 
> > It would be great to have this documented in the code sometime.
> Do you mean `CallEvent::getParameterLocation()` for arguments? What is the 
> //construction context// for the call? How can it be obtained?
I do not know exactly how many place `LazyCompoundVal`  appears, but one place 
for sure is in `MaterializeTemporaryExpr::getSubExpr()`. What to use there 
instead?


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

Szelethus wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > I'm opposed to this code for the same reason that i'm opposed to it in 
> > > > the debug checker. Parent region is an undocumented implementation 
> > > > detail of `RegionStore`. It is supposed to be immaterial to the user. 
> > > > You should not rely on its exact value.
> > > > 
> > > > @baloghadamsoftware Can we eliminate all such code from iterator 
> > > > checkers and instead identify all iterators by regions in which they're 
> > > > stored? Does my improved C++ support help with this anyhow whenever it 
> > > > kicks in?
> > > How to find the region where it is stored? I am open to find better 
> > > solutions, but it was the only one I could find so far. If we ignore 
> > > `LazyCompoundVal` then everything falls apart, we can remove all the 
> > > iterator-related checkers.
> > It's the region from which you loaded it. If you obtained it as 
> > `Call.getArgSVal()` then it's the parameter region for the call. If you 
> > obtained it as `Call.getReturnValue()` then it's the target region can be 
> > obtained by looking at the //construction context// for the call.
> `LazyCompoundVal` and friends seem to be a constantly emerging headache for 
> almost everyone. For how long I've spent in the analyzer, and have 
> religiously studied conversations and your workbook about it, I still feel 
> anxious whenever it comes up.
> 
> It would be great to have this documented in the code sometime.
Do you mean `CallEvent::getParameterLocation()` for arguments? What is the 
//construction context// for the call? How can it be obtained?


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > I'm opposed to this code for the same reason that i'm opposed to it in 
> > > the debug checker. Parent region is an undocumented implementation detail 
> > > of `RegionStore`. It is supposed to be immaterial to the user. You should 
> > > not rely on its exact value.
> > > 
> > > @baloghadamsoftware Can we eliminate all such code from iterator checkers 
> > > and instead identify all iterators by regions in which they're stored? 
> > > Does my improved C++ support help with this anyhow whenever it kicks in?
> > How to find the region where it is stored? I am open to find better 
> > solutions, but it was the only one I could find so far. If we ignore 
> > `LazyCompoundVal` then everything falls apart, we can remove all the 
> > iterator-related checkers.
> It's the region from which you loaded it. If you obtained it as 
> `Call.getArgSVal()` then it's the parameter region for the call. If you 
> obtained it as `Call.getReturnValue()` then it's the target region can be 
> obtained by looking at the //construction context// for the call.
`LazyCompoundVal` and friends seem to be a constantly emerging headache for 
almost everyone. For how long I've spent in the analyzer, and have religiously 
studied conversations and your workbook about it, I still feel anxious whenever 
it comes up.

It would be great to have this documented in the code sometime.



Comment at: clang/test/Analysis/iterator-modelling.cpp:169
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // 
expected-warning{{TRUE}}
 // 
expected-note@-1{{TRUE}}

baloghadamsoftware wrote:
> Szelethus wrote:
> > Interestingness won't be propagated here because 
> > `clang_analyzer_iterator_container(i2) == ` is interesting, not 
> > `clang_analyzer_iterator_container(i2)`, correct?
> Currently only `clang_analyzer_express()` marks its argument as interesting. 
> This could be extended in the future, however the argument of 
> `clang_analyzer_eval()` is usually the result of the comparison, not the 
> symbolic comparison itself so the sides of the comparison are not reachable 
> at that point.
Fair enough.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.
Herald added a subscriber: ASDenysPetrov.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > I'm opposed to this code for the same reason that i'm opposed to it in the 
> > debug checker. Parent region is an undocumented implementation detail of 
> > `RegionStore`. It is supposed to be immaterial to the user. You should not 
> > rely on its exact value.
> > 
> > @baloghadamsoftware Can we eliminate all such code from iterator checkers 
> > and instead identify all iterators by regions in which they're stored? Does 
> > my improved C++ support help with this anyhow whenever it kicks in?
> How to find the region where it is stored? I am open to find better 
> solutions, but it was the only one I could find so far. If we ignore 
> `LazyCompoundVal` then everything falls apart, we can remove all the 
> iterator-related checkers.
It's the region from which you loaded it. If you obtained it as 
`Call.getArgSVal()` then it's the parameter region for the call. If you 
obtained it as `Call.getReturnValue()` then it's the target region can be 
obtained by looking at the //construction context// for the call.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.
Herald added a subscriber: DenisDvlp.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

NoQ wrote:
> I'm opposed to this code for the same reason that i'm opposed to it in the 
> debug checker. Parent region is an undocumented implementation detail of 
> `RegionStore`. It is supposed to be immaterial to the user. You should not 
> rely on its exact value.
> 
> @baloghadamsoftware Can we eliminate all such code from iterator checkers and 
> instead identify all iterators by regions in which they're stored? Does my 
> improved C++ support help with this anyhow whenever it kicks in?
How to find the region where it is stored? I am open to find better solutions, 
but it was the only one I could find so far. If we ignore `LazyCompoundVal` 
then everything falls apart, we can remove all the iterator-related checkers.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:107-108
+  BR.markInteresting(Iter);
+  if (const auto  = Iter.getAs()) {
+BR.markInteresting(LCV->getRegion());
+  }

What region would it be in the common scenario? Will it be the argument region 
or the region from which the value was copied into the argument region? Can we 
say explicitly "let's just take the argument region" because it's clearly the 
right thing to do?



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

I'm opposed to this code for the same reason that i'm opposed to it in the 
debug checker. Parent region is an undocumented implementation detail of 
`RegionStore`. It is supposed to be immaterial to the user. You should not rely 
on its exact value.

@baloghadamsoftware Can we eliminate all such code from iterator checkers and 
instead identify all iterators by regions in which they're stored? Does my 
improved C++ support help with this anyhow whenever it kicks in?



Comment at: clang/test/Analysis/iterator-modelling.cpp:69
 
-  auto j = ++i;
+  auto j = ++i; // expected-note 2{{Iterator 'i' incremented by 1}}
 

Let's also add a test for a simple copy, i.e. `auto j = i;`. Does it say 
`Iterator 'i' copied`? What about `moved`?



Comment at: clang/test/Analysis/iterator-range.cpp:33
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}

This line ultimately needs a note as well, i.e. `Iterator points to end of 
container 'V'` or something like that.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:91-111
   SVal V = C.getSVal(CE->getArg(0));
   const auto *Pos = getIteratorPosition(State, V);
+  SVal Field = Default;
+
   if (Pos) {
-State = State->BindExpr(CE, C.getLocationContext(), get(Pos));
-  } else {
-State = State->BindExpr(CE, C.getLocationContext(), Default);
+Field = get(Pos);
   }

Szelethus wrote:
> Now that we have multiple `SVal`s around, can we rename `V`? Also, I would 
> appreciate some comments. As I understand it, `ExprInspectionChecker` now 
> marks the arguments as interesting, so if we write this:
> ```lang=cpp
> clang_analyzer_express(clang_analyzer_iterator_position(i2));
> ```
> `clang_analyzer_iterator_position(i2)` will be interesting, and this function 
> propagates this interestingness to `i2`, correct?
Yes.



Comment at: clang/test/Analysis/iterator-modelling.cpp:169
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // 
expected-warning{{TRUE}}
 // 
expected-note@-1{{TRUE}}

Szelethus wrote:
> Interestingness won't be propagated here because 
> `clang_analyzer_iterator_container(i2) == ` is interesting, not 
> `clang_analyzer_iterator_container(i2)`, correct?
Currently only `clang_analyzer_express()` marks its argument as interesting. 
This could be extended in the future, however the argument of 
`clang_analyzer_eval()` is usually the result of the comparison, not the 
symbolic comparison itself so the sides of the comparison are not reachable at 
that point.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/test/Analysis/iterator-modelling.cpp:169
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // 
expected-warning{{TRUE}}
 // 
expected-note@-1{{TRUE}}

Interestingness won't be propagated here because 
`clang_analyzer_iterator_container(i2) == ` is interesting, not 
`clang_analyzer_iterator_container(i2)`, correct?


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Its very interesting to see how the idea of interestingness propagation through 
`NoteTag`s works in practice -- I feel like many other checkers will need to 
adopt something like this, and its great that we have the first use case 
presented here. The patch from afar looks good, I only left some nits to ease 
the readability for the uninitiated.




Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:91-111
   SVal V = C.getSVal(CE->getArg(0));
   const auto *Pos = getIteratorPosition(State, V);
+  SVal Field = Default;
+
   if (Pos) {
-State = State->BindExpr(CE, C.getLocationContext(), get(Pos));
-  } else {
-State = State->BindExpr(CE, C.getLocationContext(), Default);
+Field = get(Pos);
   }

Now that we have multiple `SVal`s around, can we rename `V`? Also, I would 
appreciate some comments. As I understand it, `ExprInspectionChecker` now marks 
the arguments as interesting, so if we write this:
```lang=cpp
clang_analyzer_express(clang_analyzer_iterator_position(i2));
```
`clang_analyzer_iterator_position(i2)` will be interesting, and this function 
propagates this interestingness to `i2`, correct?



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:103
+  C.getNoteTag([V, Field](PathSensitiveBugReport ) -> std::string {
+auto *PSBR = cast();
+if (PSBR->isInteresting(Field)) {

We won't need this anymore :)



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:432
 
-  reportBug(*Str, C);
+  reportBug(*Str, C, Optional(ArgVal));
 }

Why the need for the `Optional` stuff here?



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:108-109
   SVal It2 = UndefinedVal()) const;
+  const NoteTag *getInterestingnessPropagationTag(CheckerContext , SVal It1,
+  SVal It2) const;
   void printState(raw_ostream , ProgramStateRef State, const char *NL,

The usage of this function is very non-obvious, plenty of comments would be 
appreciated here as well.


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

https://reviews.llvm.org/D75677



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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

Rebased.


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

https://reviews.llvm.org/D75677

Files:
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/test/Analysis/iterator-modelling.cpp

Index: clang/test/Analysis/iterator-modelling.cpp
===
--- clang/test/Analysis/iterator-modelling.cpp
+++ clang/test/Analysis/iterator-modelling.cpp
@@ -81,7 +81,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto j = i++; // expected-note 2{{Iterator 'i' incremented by 1}}
+  auto j = i++; // expected-note{{Iterator 'i' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.begin() + 1}}
//expected-note@-1{{$v.begin() + 1}}
@@ -94,7 +94,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto j = i--; // expected-note 2{{Iterator 'i' decremented by 1}}
+  auto j = i--; // expected-note{{Iterator 'i' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.end() - 1}}
//expected-note@-1{{$v.end() - 1}}
@@ -164,7 +164,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 + 2; // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 + 2; // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -177,7 +177,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 + (-2); // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 + (-2); // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -190,7 +190,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 - 2;  // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 - 2; // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -203,7 +203,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 - (-2); // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 - (-2); // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -217,7 +217,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i1;  // expected-note 2{{Iterator 'i1' incremented by 1}}
+  ++i1;  // expected-note{{Iterator 'i1' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin() + 1}}
 //expected-note@-1{{$v.begin() + 1}}
@@ -231,7 +231,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i2;  // expected-note 2{{Iterator 'i2' incremented by 1}}
+  ++i2;  // expected-note{{Iterator 'i2' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin()}}
 //expected-note@-1{{$v.begin()}}
@@ -245,7 +245,7 @@
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
   auto i2 = i1;
-  --i1;  // expected-note 2{{Iterator 'i1' decremented by 1}}
+  --i1;  // expected-note{{Iterator 'i1' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.end() - 1}}
 //expected-note@-1{{$v.end() - 1}}
@@ -259,7 +259,7 @@
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
   auto i2 = i1;
-  --i2;  // expected-note 2{{Iterator 'i2' decremented by 1}}
+  --i2;  // expected-note{{Iterator 'i2' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.end()}}
 

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

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

Updated according to the comments to D75514 .


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

https://reviews.llvm.org/D75677

Files:
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/test/Analysis/iterator-modelling.cpp

Index: clang/test/Analysis/iterator-modelling.cpp
===
--- clang/test/Analysis/iterator-modelling.cpp
+++ clang/test/Analysis/iterator-modelling.cpp
@@ -81,7 +81,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto j = i++; // expected-note 2{{Iterator 'i' incremented by 1}}
+  auto j = i++; // expected-note{{Iterator 'i' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.begin() + 1}}
//expected-note@-1{{$v.begin() + 1}}
@@ -94,7 +94,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto j = i--; // expected-note 2{{Iterator 'i' decremented by 1}}
+  auto j = i--; // expected-note{{Iterator 'i' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.end() - 1}}
//expected-note@-1{{$v.end() - 1}}
@@ -164,7 +164,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 + 2; // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 + 2; // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -177,7 +177,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 + (-2); // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 + (-2); // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -190,7 +190,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 - 2;  // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 - 2; // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -203,7 +203,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 - (-2); // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 - (-2); // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -217,7 +217,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i1;  // expected-note 2{{Iterator 'i1' incremented by 1}}
+  ++i1;  // expected-note{{Iterator 'i1' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin() + 1}}
 //expected-note@-1{{$v.begin() + 1}}
@@ -231,7 +231,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i2;  // expected-note 2{{Iterator 'i2' incremented by 1}}
+  ++i2;  // expected-note{{Iterator 'i2' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin()}}
 //expected-note@-1{{$v.begin()}}
@@ -245,7 +245,7 @@
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
   auto i2 = i1;
-  --i1;  // expected-note 2{{Iterator 'i1' decremented by 1}}
+  --i1;  // expected-note{{Iterator 'i1' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.end() - 1}}
 //expected-note@-1{{$v.end() - 1}}
@@ -259,7 +259,7 @@
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
   auto i2 = i1;
-  --i2;  // expected-note 2{{Iterator 'i2' decremented by 1}}
+  --i2;  // expected-note{{Iterator 'i2' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.end()}}

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

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

baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus.
baloghadamsoftware added a project: clang.
Herald added subscribers: martong, steakhal, Charusso, gamesh411, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity.
baloghadamsoftware added a parent revision: D74541: [Analyzer] Use note tags to 
track iterator increments and decrements.

If an error happens which is related to some iterators the Iterator Modeling 
checker adds note tags to all the iterator operations along the bug path. This 
may be disturbing if there are other iterators beside the ones which are 
affected by the bug. This patch restricts the note tags to only the affected 
iterators and adjust the debug checkers to be able to test this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75677

Files:
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/test/Analysis/iterator-modelling.cpp

Index: clang/test/Analysis/iterator-modelling.cpp
===
--- clang/test/Analysis/iterator-modelling.cpp
+++ clang/test/Analysis/iterator-modelling.cpp
@@ -81,7 +81,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto j = i++; // expected-note 2{{Iterator 'i' incremented by 1}}
+  auto j = i++; // expected-note{{Iterator 'i' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.begin() + 1}}
//expected-note@-1{{$v.begin() + 1}}
@@ -94,7 +94,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto j = i--; // expected-note 2{{Iterator 'i' decremented by 1}}
+  auto j = i--; // expected-note{{Iterator 'i' decremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.end() - 1}}
//expected-note@-1{{$v.end() - 1}}
@@ -164,7 +164,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 + 2; // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 + 2; // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -177,7 +177,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 + (-2); // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 + (-2); // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -190,7 +190,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
-  auto i2 = i1 - 2;  // expected-note 2{{Iterator 'i1' decremented by 2}}
+  auto i2 = i1 - 2; // expected-note{{Iterator 'i1' decremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -203,7 +203,7 @@
 
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
-  auto i2 = i1 - (-2); // expected-note 2{{Iterator 'i1' incremented by 2}}
+  auto i2 = i1 - (-2); // expected-note{{Iterator 'i1' incremented by 2}}
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == ); // expected-warning{{TRUE}}
 // expected-note@-1{{TRUE}}
@@ -217,7 +217,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i1;  // expected-note 2{{Iterator 'i1' incremented by 1}}
+  ++i1;  // expected-note{{Iterator 'i1' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin() + 1}}
 //expected-note@-1{{$v.begin() + 1}}
@@ -231,7 +231,7 @@
   clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
 
   auto i2 = i1;
-  ++i2;  // expected-note 2{{Iterator 'i2' incremented by 1}}
+  ++i2;  // expected-note{{Iterator 'i2' incremented by 1}}
 
   clang_analyzer_express(clang_analyzer_iterator_position(i1)); //expected-warning{{$v.begin()}}
 //expected-note@-1{{$v.begin()}}
@@ -245,7 +245,7 @@
   clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
 
   auto i2 = i1;
-  --i1;  // expected-note 2{{Iterator 'i1' decremented by 1}}
+