[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa3f4d17a1a53: [Analyzer] Use note tags to track container 
begin and and changes (authored by baloghadamsoftware).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Lovely, thank you!


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Test added.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto j = i - 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

baloghadamsoftware wrote:
> NoQ wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > Hmm, i think you should instead look at `ContReg`, i.e. whether 
> > > > > > it's a non-anonymous `VarRegion` or a `FieldRegion` or something 
> > > > > > like that (in other patches as well). It would work more often and 
> > > > > > it'll transparently handle references.
> > > > > Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
> > > > > `getMostDerivedRegion()` does not help.)
> > > > You mean the first checking form `SymbolicRegion`, then get its symbol, 
> > > > check for `SymbolRegionValue`, then get its `TypedValueRegion`, check 
> > > > for `DeclRegion` and use its `Decl`? This sound waaay more complicated 
> > > > and less readable. I am not sure which are the side cases: is it always 
> > > > `SymbolicRegion`? Is the `Symbol` of `SymbolicRegion` always a 
> > > > `SymbolRegionValue`? Is ithe `TypedValueRegion` (the return value of 
> > > > its `getRegion()`) always a `DeclRegion`?
> > > > Unfortunately it is a `SymRegion`
> > > 
> > > Emm, that's rarely the case. Only if it's a reference passed into a 
> > > top-level function as a parameter.
> > (or to another unknown location) (please learn what `SymbolicRegion` is, it 
> > is very important for your work)
> > 
> > I guess you should do both then, because when the analyzer is able to 
> > resolve the reference it's better in this case to point out what is the 
> > actual container that's being modified.
> Is it OK now?
Looks good, can we also have tests for this case? I.e., anything where the 
container isn't passed by reference to an unknown location.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

NoQ wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > Hmm, i think you should instead look at `ContReg`, i.e. whether it's 
> > > > > a non-anonymous `VarRegion` or a `FieldRegion` or something like that 
> > > > > (in other patches as well). It would work more often and it'll 
> > > > > transparently handle references.
> > > > Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
> > > > `getMostDerivedRegion()` does not help.)
> > > You mean the first checking form `SymbolicRegion`, then get its symbol, 
> > > check for `SymbolRegionValue`, then get its `TypedValueRegion`, check for 
> > > `DeclRegion` and use its `Decl`? This sound waaay more complicated and 
> > > less readable. I am not sure which are the side cases: is it always 
> > > `SymbolicRegion`? Is the `Symbol` of `SymbolicRegion` always a 
> > > `SymbolRegionValue`? Is ithe `TypedValueRegion` (the return value of its 
> > > `getRegion()`) always a `DeclRegion`?
> > > Unfortunately it is a `SymRegion`
> > 
> > Emm, that's rarely the case. Only if it's a reference passed into a 
> > top-level function as a parameter.
> (or to another unknown location) (please learn what `SymbolicRegion` is, it 
> is very important for your work)
> 
> I guess you should do both then, because when the analyzer is able to resolve 
> the reference it's better in this case to point out what is the actual 
> container that's being modified.
Is it OK now?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Updated according to a comment.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto j = i - 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

NoQ wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
> > > > non-anonymous `VarRegion` or a `FieldRegion` or something like that (in 
> > > > other patches as well). It would work more often and it'll 
> > > > transparently handle references.
> > > Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
> > > `getMostDerivedRegion()` does not help.)
> > You mean the first checking form `SymbolicRegion`, then get its symbol, 
> > check for `SymbolRegionValue`, then get its `TypedValueRegion`, check for 
> > `DeclRegion` and use its `Decl`? This sound waaay more complicated and less 
> > readable. I am not sure which are the side cases: is it always 
> > `SymbolicRegion`? Is the `Symbol` of `SymbolicRegion` always a 
> > `SymbolRegionValue`? Is ithe `TypedValueRegion` (the return value of its 
> > `getRegion()`) always a `DeclRegion`?
> > Unfortunately it is a `SymRegion`
> 
> Emm, that's rarely the case. Only if it's a reference passed into a top-level 
> function as a parameter.
(or to another unknown location) (please learn what `SymbolicRegion` is, it is 
very important for your work)

I guess you should do both then, because when the analyzer is able to resolve 
the reference it's better in this case to point out what is the actual 
container that's being modified.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
> > > non-anonymous `VarRegion` or a `FieldRegion` or something like that (in 
> > > other patches as well). It would work more often and it'll transparently 
> > > handle references.
> > Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
> > `getMostDerivedRegion()` does not help.)
> You mean the first checking form `SymbolicRegion`, then get its symbol, check 
> for `SymbolRegionValue`, then get its `TypedValueRegion`, check for 
> `DeclRegion` and use its `Decl`? This sound waaay more complicated and less 
> readable. I am not sure which are the side cases: is it always 
> `SymbolicRegion`? Is the `Symbol` of `SymbolicRegion` always a 
> `SymbolRegionValue`? Is ithe `TypedValueRegion` (the return value of its 
> `getRegion()`) always a `DeclRegion`?
> Unfortunately it is a `SymRegion`

Emm, that's rarely the case. Only if it's a reference passed into a top-level 
function as a parameter.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

baloghadamsoftware wrote:
> NoQ wrote:
> > Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
> > non-anonymous `VarRegion` or a `FieldRegion` or something like that (in 
> > other patches as well). It would work more often and it'll transparently 
> > handle references.
> Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
> `getMostDerivedRegion()` does not help.)
You mean the first checking form `SymbolicRegion`, then get its symbol, check 
for `SymbolRegionValue`, then get its `TypedValueRegion`, check for 
`DeclRegion` and use its `Decl`? This sound waaay more complicated and less 
readable. I am not sure which are the side cases: is it always 
`SymbolicRegion`? Is the `Symbol` of `SymbolicRegion` always a 
`SymbolRegionValue`? Is ithe `TypedValueRegion` (the return value of its 
`getRegion()`) always a `DeclRegion`?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

NoQ wrote:
> Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
> non-anonymous `VarRegion` or a `FieldRegion` or something like that (in other 
> patches as well). It would work more often and it'll transparently handle 
> references.
Unfortunately it is a `SymRegion` so it does not work :-( (Even using 
`getMostDerivedRegion()` does not help.)


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
non-anonymous `VarRegion` or a `FieldRegion` or something like that (in other 
patches as well). It would work more often and it'll transparently handle 
references.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

baloghadamsoftware added a comment.

I wonder whether I should change "to/from the left" to "towards/from the front" 
and "to/from the right" to "towards/from the right"?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Minor style change.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto j = i - 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

The scope of the patch is small and is well tested. I suggest to move the 
discussion about debug messages and `NoteTag`s to the next revision. LGTM!


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

baloghadamsoftware wrote:
> Szelethus wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > You'll need to check whether the container is actually of 
> > > > > > > > interest to the bug report. We don't want notes to be added 
> > > > > > > > about changes to irrelevant containers.
> > > > > > > > 
> > > > > > > > You can use a combination of "Report `BR` was emitted by one of 
> > > > > > > > the iterator checkers" and "The memory region of the container 
> > > > > > > > is marked as interesting" (while also actually marking it as 
> > > > > > > > interesting in the checker).
> > > > > > > > 
> > > > > > > > Ideally we should instead make a new generic storage inside the 
> > > > > > > > `BugReport` object, in order to pass down the interesting 
> > > > > > > > information from the call site of `emitReport` ("Hi, i'm an 
> > > > > > > > iterator checker who emitted this report and i'm interested in 
> > > > > > > > changes made to the size of this container").
> > > > > > > Are you sure in this? I already wondered how it works so I added 
> > > > > > > a test that checks one container and changes another one and 
> > > > > > > there were no note tags displayed for the one we did not check 
> > > > > > > but change. See the last test.
> > > > > > That's because you didn't do
> > > > > > ```lang=c++
> > > > > >   V2.cbegin();
> > > > > >   V2.cend();
> > > > > > ```
> > > > > > in the beginning.
> > > > > A similar conversation sparked up recently in between @boga95, 
> > > > > @steakhal and me regarding reporting taintedness. Bug reports are 
> > > > > fine up to the point where (in reverse) the first propagation 
> > > > > happens, but finding out which value tainted the one that caused the 
> > > > > report isn't handled at the moment. One idea was to mark the initial 
> > > > > (again, in reverse) value as interesting, create a `NoteTag` at the 
> > > > > point of propagation, where we should know which value was the cause 
> > > > > of the spread, mark that interesting as well, etc.
> > > > > 
> > > > > If `NoteTag`s only emit a message when the concerning value is 
> > > > > interesting, this should theoretically solve that problem. I guess 
> > > > > you could say that we're propagating interestingness in reverse.
> > > > > 
> > > > > I'm not immediately sure if this idea was ever mentioned or 
> > > > > implemented here.
> > > > Yes, that's the intended solution to such problems. 
> > > > `trackExpressionValue` works similarly, just with assignments instead 
> > > > of taint propagations. And in both cases note tags are a much more 
> > > > straightforward solution to the problem.
> > > Yes, you are right. My problem now is that how to mark interesting when 
> > > debugging? I I filter for interesting containers only, I lose my ability 
> > > to debug. Should I create a debug function just for marking a container 
> > > as interesting. Or is there such function already?
> > I'm not immediately sure how interetingness ties into debugging, what 
> > specific scenario are you thinking about?
> In the test of the modeling checker we use debug checkers. They should be 
> able to mark the container interesting to be able to test the not tags. I 
> managed to solve problem, even in a somewhat unorthodox way.
The core issue with NoteTag it does not know about interestingness and nor 
about MemRegion. I believe everything based on MemRegions already and when you 
emit the report you know exactly which MemRegion raised an error. So I think 
first we need to solve that the NoteTags only report on given MemRegions and 
those regions of course mega-interesting: we do not need to keep around the 
interestingness then.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Back to the version where we do not check for interestingness.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto j = i - 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Apologies -- I'd definitely prefer to address the debug related changes in a 
separate pack, similarly to D74541 .


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus added a comment.

In D73720#1874014 , 
@baloghadamsoftware wrote:

> In case of multiple container-related bugs only mark the container 
> interesting on the correct bug path. Also a typo fixed.


~~Uh-oh, I'm not sure why this would ever be an issue? Interestingness is a 
property of a given `BugReport`, not the `BugReporter` class. How can this 
interfere with one another?~~

Okay I see what the issue is. `DebugContainerModeling` normally doesn't emit a 
report, only adds notes on interesting containers. Though it still makes me 
wonder whether this is the right approach.

First, I think changes to the `DebugContainerModeling` seems to spawn a 
different discussion, and separating it to a different patch might make for a 
satisfying splitting point. The rest of the patch seems to be ready to land in 
my opinion. Also, this functionality seems to be duplicated in 
`DebugIteratorModeling` in D74541 , is this 
intended?

Second, that issue may be more appropriately solved by introducing a new debug 
interestingness kind (D65723 ), and just make 
the parameters of `clang_analyzer_express` interesting in the debug sense. If 
we did that, `DebugContainerModeling` could ask whether the symbol is 
debug-interesting instead of the `SourceRange` hackery. WDYT?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Minor update: ignore parentheses and casts when taking the name of the 
expression.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector ) {
   auto i = V.begin();
   auto j = 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

In case of multiple container-related bugs only mark the container interesting 
on the correct bug path. Also a typo fixed.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin_by_2(const std::vector ) {
@@ -246,11 +257,13 @@
 void decr_by_2_copy_begin(const std::vector 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Nicolás Alvarez via Phabricator via cfe-commits]

nicolas17 added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:484
+const NoteTag *ChangeTag =
+  getChangeTag(C, "shrinked from the right by 1 position", ContReg, ContE);
 // For vector-like and deque-like containers invalidate the last and the

Past tense is "shrank", not "shrinked".


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM, I like everything here, you worded the notes very nicely and the test 
cases seems to cover everything I could find! Please wait for @NoQ's approval, 
since he's the ranking member of  among the `NoteTag` users.

In D73720#1871975 , 
@baloghadamsoftware wrote:

> In D73720#1871955 , @Szelethus wrote:
>
> > Do we have a test where 2 containers are present but only one of them 
> > should be marked as interesting?
>
>
> Yes, of course we have, that was the starting point of the discussion. 
> However, I try to make the tests orthogonal, so this test is in 
> `container-modeling.cpp` where we do not dereference it, but print its begin 
> or end.


Yea, right, silly me.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

baloghadamsoftware added a comment.

In D73720#1871955 , @Szelethus wrote:

> Do we have a test where 2 containers are present but only one of them should 
> be marked as interesting?


Yes, of course we have, that was the starting point of the discussion. However, 
I try to make the tests orthogonal, so this test is in `container-modeling.cpp` 
where we do not dereference it, but print its begin or end.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus added a comment.

Do we have a test where 2 containers are present but only one of them should be 
marked as interesting?

  void deref_end_after_pop_back(std::vector , std::vector ) {
const auto i = --V.end();
const auto i2 = --V2.end();
  
V.pop_back();  // expected-note{{Container 'V' shrinked from the right by 1 
position}}
V2.pop_back(); // no-note
  
*i; // expected-warning{{Past-the-end iterator dereferenced}}
// expected-note@-1{{Past-the-end iterator dereferenced}}
  }




Comment at: clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp:95-103
+const NoteTag *InterestingTag =
+  C.getNoteTag([Cont](BugReport ) -> std::string {
+  auto *PSBR = dyn_cast();
+  if (PSBR) {
+PSBR->markInteresting(Cont);
+  }
+  return "";

Aha, makes sense, when calling `clang_analyzer_container_end(V)`, we want to 
make the analyzer emit more information about `V` so its obviously interesting.



Comment at: clang/test/Analysis/container-modeling.cpp:35
 
 

 ///

I hate to be that guy, but this is quite ugly :). How about the handsome
```
//===--===//
// Container assignment tests.
//===--===//
```
But I don't insist, especially within the scope of this patch.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

First real checker `IteratorRange` now marks container as interesting to 
benefit from container note tags. The other two iterator checkers will be 
updated after a second phase of container note tags which also cares for 
reassignment (upon move) and invalidation of iterators.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/container-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
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -32,6 +32,7 @@
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
+  // expected-note@-1{{Past-the-end iterator dereferenced}}
 }
 
 // Prefix increment - operator++()
@@ -59,6 +60,7 @@
 void incr_end(const std::vector ) {
   auto i = V.end();
   ++i; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Postfix increment - operator++(int)
@@ -86,6 +88,7 @@
 void end_incr(const std::vector ) {
   auto i = V.end();
   i++; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+   // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Prefix decrement - operator--()
@@ -93,6 +96,7 @@
 void decr_begin(const std::vector ) {
   auto i = V.begin();
   --i; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_behind_begin(const std::vector ) {
@@ -120,6 +124,7 @@
 void begin_decr(const std::vector ) {
   auto i = V.begin();
   i--; // expected-warning{{Iterator decremented ahead of its valid range}}
+   // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void behind_begin_decr(const std::vector ) {
@@ -168,11 +173,13 @@
 void incr_by_2_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_end(const std::vector ) {
   auto i = V.end();
   i += 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Addition - operator+(int)
@@ -201,11 +208,13 @@
 void incr_by_2_copy_ahead_of_end(const std::vector ) {
   auto i = --V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 void incr_by_2_copy_end(const std::vector ) {
   auto i = V.end();
   auto j = i + 2; // expected-warning{{Iterator incremented behind the past-the-end iterator}}
+  // expected-note@-1{{Iterator incremented behind the past-the-end iterator}}
 }
 
 // Subtraction assignment - operator-=(int)
@@ -213,11 +222,13 @@
 void decr_by_2_begin(const std::vector ) {
   auto i = V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented ahead of its valid range}}
 }
 
 void decr_by_2_behind_begin(const std::vector ) {
   auto i = ++V.begin();
   i -= 2; // expected-warning{{Iterator decremented ahead of its valid range}}
+  // expected-note@-1{{Iterator decremented 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

Szelethus wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > You'll need to check whether the container is actually of 
> > > > > > > interest to the bug report. We don't want notes to be added about 
> > > > > > > changes to irrelevant containers.
> > > > > > > 
> > > > > > > You can use a combination of "Report `BR` was emitted by one of 
> > > > > > > the iterator checkers" and "The memory region of the container is 
> > > > > > > marked as interesting" (while also actually marking it as 
> > > > > > > interesting in the checker).
> > > > > > > 
> > > > > > > Ideally we should instead make a new generic storage inside the 
> > > > > > > `BugReport` object, in order to pass down the interesting 
> > > > > > > information from the call site of `emitReport` ("Hi, i'm an 
> > > > > > > iterator checker who emitted this report and i'm interested in 
> > > > > > > changes made to the size of this container").
> > > > > > Are you sure in this? I already wondered how it works so I added a 
> > > > > > test that checks one container and changes another one and there 
> > > > > > were no note tags displayed for the one we did not check but 
> > > > > > change. See the last test.
> > > > > That's because you didn't do
> > > > > ```lang=c++
> > > > >   V2.cbegin();
> > > > >   V2.cend();
> > > > > ```
> > > > > in the beginning.
> > > > A similar conversation sparked up recently in between @boga95, 
> > > > @steakhal and me regarding reporting taintedness. Bug reports are fine 
> > > > up to the point where (in reverse) the first propagation happens, but 
> > > > finding out which value tainted the one that caused the report isn't 
> > > > handled at the moment. One idea was to mark the initial (again, in 
> > > > reverse) value as interesting, create a `NoteTag` at the point of 
> > > > propagation, where we should know which value was the cause of the 
> > > > spread, mark that interesting as well, etc.
> > > > 
> > > > If `NoteTag`s only emit a message when the concerning value is 
> > > > interesting, this should theoretically solve that problem. I guess you 
> > > > could say that we're propagating interestingness in reverse.
> > > > 
> > > > I'm not immediately sure if this idea was ever mentioned or implemented 
> > > > here.
> > > Yes, that's the intended solution to such problems. 
> > > `trackExpressionValue` works similarly, just with assignments instead of 
> > > taint propagations. And in both cases note tags are a much more 
> > > straightforward solution to the problem.
> > Yes, you are right. My problem now is that how to mark interesting when 
> > debugging? I I filter for interesting containers only, I lose my ability to 
> > debug. Should I create a debug function just for marking a container as 
> > interesting. Or is there such function already?
> I'm not immediately sure how interetingness ties into debugging, what 
> specific scenario are you thinking about?
In the test of the modeling checker we use debug checkers. They should be able 
to mark the container interesting to be able to test the not tags. I managed to 
solve problem, even in a somewhat unorthodox way.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

Only track the right container. Furthermore, minor updates according to the 
comments.


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
  clang/test/Analysis/container-modeling.cpp

Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -analyzer-output=text -verify
 
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -analyzer-output=text -verify
 
 // RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true %s 2>&1 | FileCheck %s
 
@@ -20,14 +20,16 @@
   V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
-  clang_analyzer_express(clang_analyzer_container_begin(V)); //expected-warning{{$V.begin()}}
+  clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
 }
 
 void end(const std::vector ) {
   V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
-  clang_analyzer_express(clang_analyzer_container_end(V)); //expected-warning{{$V.end()}}
+  clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
+   // expected-note@-1{{$V.end()}}
 }
 
 
@@ -48,8 +50,10 @@
   long B2 = clang_analyzer_container_begin(V2);
   long E2 = clang_analyzer_container_end(V2);
   V1 = std::move(V2);
-  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); //expected-warning{{TRUE}}
-  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); //expected-warning{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); // expected-warning{{TRUE}}
+ // expected-note@-1{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); // expected-warning{{TRUE}}
+   // expected-note@-1{{TRUE}}
 }
 
 
@@ -63,6 +67,8 @@
 /// Design decision: extends containers to the ->RIGHT-> (i.e. the
 /// past-the-end position of the container is incremented).
 
+void clang_analyzer_dump(void*);
+
 void push_back(std::vector , int n) {
   V.cbegin();
   V.cend();
@@ -70,10 +76,13 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.push_back(n);
+  V.push_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+  // expected-note@-1{{Container 'V' extended to the right by 1 position}}
 
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
+   // expected-note@-1{{$V.end() + 1}}
 }
 
 /// emplace_back()
@@ -88,10 +97,14 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.emplace_back(n);
+  V.emplace_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+ // expected-note@-1{{Container 'V' extended to the right by 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

baloghadamsoftware wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > You'll need to check whether the container is actually of interest 
> > > > > > to the bug report. We don't want notes to be added about changes to 
> > > > > > irrelevant containers.
> > > > > > 
> > > > > > You can use a combination of "Report `BR` was emitted by one of the 
> > > > > > iterator checkers" and "The memory region of the container is 
> > > > > > marked as interesting" (while also actually marking it as 
> > > > > > interesting in the checker).
> > > > > > 
> > > > > > Ideally we should instead make a new generic storage inside the 
> > > > > > `BugReport` object, in order to pass down the interesting 
> > > > > > information from the call site of `emitReport` ("Hi, i'm an 
> > > > > > iterator checker who emitted this report and i'm interested in 
> > > > > > changes made to the size of this container").
> > > > > Are you sure in this? I already wondered how it works so I added a 
> > > > > test that checks one container and changes another one and there were 
> > > > > no note tags displayed for the one we did not check but change. See 
> > > > > the last test.
> > > > That's because you didn't do
> > > > ```lang=c++
> > > >   V2.cbegin();
> > > >   V2.cend();
> > > > ```
> > > > in the beginning.
> > > A similar conversation sparked up recently in between @boga95, @steakhal 
> > > and me regarding reporting taintedness. Bug reports are fine up to the 
> > > point where (in reverse) the first propagation happens, but finding out 
> > > which value tainted the one that caused the report isn't handled at the 
> > > moment. One idea was to mark the initial (again, in reverse) value as 
> > > interesting, create a `NoteTag` at the point of propagation, where we 
> > > should know which value was the cause of the spread, mark that 
> > > interesting as well, etc.
> > > 
> > > If `NoteTag`s only emit a message when the concerning value is 
> > > interesting, this should theoretically solve that problem. I guess you 
> > > could say that we're propagating interestingness in reverse.
> > > 
> > > I'm not immediately sure if this idea was ever mentioned or implemented 
> > > here.
> > Yes, that's the intended solution to such problems. `trackExpressionValue` 
> > works similarly, just with assignments instead of taint propagations. And 
> > in both cases note tags are a much more straightforward solution to the 
> > problem.
> Yes, you are right. My problem now is that how to mark interesting when 
> debugging? I I filter for interesting containers only, I lose my ability to 
> debug. Should I create a debug function just for marking a container as 
> interesting. Or is there such function already?
I'm not immediately sure how interetingness ties into debugging, what specific 
scenario are you thinking about?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > You'll need to check whether the container is actually of interest to 
> > > > > the bug report. We don't want notes to be added about changes to 
> > > > > irrelevant containers.
> > > > > 
> > > > > You can use a combination of "Report `BR` was emitted by one of the 
> > > > > iterator checkers" and "The memory region of the container is marked 
> > > > > as interesting" (while also actually marking it as interesting in the 
> > > > > checker).
> > > > > 
> > > > > Ideally we should instead make a new generic storage inside the 
> > > > > `BugReport` object, in order to pass down the interesting information 
> > > > > from the call site of `emitReport` ("Hi, i'm an iterator checker who 
> > > > > emitted this report and i'm interested in changes made to the size of 
> > > > > this container").
> > > > Are you sure in this? I already wondered how it works so I added a test 
> > > > that checks one container and changes another one and there were no 
> > > > note tags displayed for the one we did not check but change. See the 
> > > > last test.
> > > That's because you didn't do
> > > ```lang=c++
> > >   V2.cbegin();
> > >   V2.cend();
> > > ```
> > > in the beginning.
> > A similar conversation sparked up recently in between @boga95, @steakhal 
> > and me regarding reporting taintedness. Bug reports are fine up to the 
> > point where (in reverse) the first propagation happens, but finding out 
> > which value tainted the one that caused the report isn't handled at the 
> > moment. One idea was to mark the initial (again, in reverse) value as 
> > interesting, create a `NoteTag` at the point of propagation, where we 
> > should know which value was the cause of the spread, mark that interesting 
> > as well, etc.
> > 
> > If `NoteTag`s only emit a message when the concerning value is interesting, 
> > this should theoretically solve that problem. I guess you could say that 
> > we're propagating interestingness in reverse.
> > 
> > I'm not immediately sure if this idea was ever mentioned or implemented 
> > here.
> Yes, that's the intended solution to such problems. `trackExpressionValue` 
> works similarly, just with assignments instead of taint propagations. And in 
> both cases note tags are a much more straightforward solution to the problem.
Yes, you are right. My problem now is that how to mark interesting when 
debugging? I I filter for interesting containers only, I lose my ability to 
debug. Should I create a debug function just for marking a container as 
interesting. Or is there such function already?


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

Szelethus wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > You'll need to check whether the container is actually of interest to 
> > > > the bug report. We don't want notes to be added about changes to 
> > > > irrelevant containers.
> > > > 
> > > > You can use a combination of "Report `BR` was emitted by one of the 
> > > > iterator checkers" and "The memory region of the container is marked as 
> > > > interesting" (while also actually marking it as interesting in the 
> > > > checker).
> > > > 
> > > > Ideally we should instead make a new generic storage inside the 
> > > > `BugReport` object, in order to pass down the interesting information 
> > > > from the call site of `emitReport` ("Hi, i'm an iterator checker who 
> > > > emitted this report and i'm interested in changes made to the size of 
> > > > this container").
> > > Are you sure in this? I already wondered how it works so I added a test 
> > > that checks one container and changes another one and there were no note 
> > > tags displayed for the one we did not check but change. See the last test.
> > That's because you didn't do
> > ```lang=c++
> >   V2.cbegin();
> >   V2.cend();
> > ```
> > in the beginning.
> A similar conversation sparked up recently in between @boga95, @steakhal and 
> me regarding reporting taintedness. Bug reports are fine up to the point 
> where (in reverse) the first propagation happens, but finding out which value 
> tainted the one that caused the report isn't handled at the moment. One idea 
> was to mark the initial (again, in reverse) value as interesting, create a 
> `NoteTag` at the point of propagation, where we should know which value was 
> the cause of the spread, mark that interesting as well, etc.
> 
> If `NoteTag`s only emit a message when the concerning value is interesting, 
> this should theoretically solve that problem. I guess you could say that 
> we're propagating interestingness in reverse.
> 
> I'm not immediately sure if this idea was ever mentioned or implemented here.
Yes, that's the intended solution to such problems. `trackExpressionValue` 
works similarly, just with assignments instead of taint propagations. And in 
both cases note tags are a much more straightforward solution to the problem.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

Szelethus added subscribers: steakhal, boga95.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > You'll need to check whether the container is actually of interest to the 
> > > bug report. We don't want notes to be added about changes to irrelevant 
> > > containers.
> > > 
> > > You can use a combination of "Report `BR` was emitted by one of the 
> > > iterator checkers" and "The memory region of the container is marked as 
> > > interesting" (while also actually marking it as interesting in the 
> > > checker).
> > > 
> > > Ideally we should instead make a new generic storage inside the 
> > > `BugReport` object, in order to pass down the interesting information 
> > > from the call site of `emitReport` ("Hi, i'm an iterator checker who 
> > > emitted this report and i'm interested in changes made to the size of 
> > > this container").
> > Are you sure in this? I already wondered how it works so I added a test 
> > that checks one container and changes another one and there were no note 
> > tags displayed for the one we did not check but change. See the last test.
> That's because you didn't do
> ```lang=c++
>   V2.cbegin();
>   V2.cend();
> ```
> in the beginning.
A similar conversation sparked up recently in between @boga95, @steakhal and me 
regarding reporting taintedness. Bug reports are fine up to the point where (in 
reverse) the first propagation happens, but finding out which value tainted the 
one that caused the report isn't handled at the moment. One idea was to mark 
the initial (again, in reverse) value as interesting, create a `NoteTag` at the 
point of propagation, where we should know which value was the cause of the 
spread, mark that interesting as well, etc.

If `NoteTag`s only emit a message when the concerning value is interesting, 
this should theoretically solve that problem. I guess you could say that we're 
propagating interestingness in reverse.

I'm not immediately sure if this idea was ever mentioned or implemented here.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:58
 public:
   ContainerModeling() {}
 

While we're at it [part 2], can we make this `= default`? :)



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:68-69
+  SVal) const;
+  typedef void (ContainerModeling::*TwoItParamFn)(CheckerContext &, SVal, SVal,
+  SVal) const;
 

Prefer `using`.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

baloghadamsoftware wrote:
> NoQ wrote:
> > You'll need to check whether the container is actually of interest to the 
> > bug report. We don't want notes to be added about changes to irrelevant 
> > containers.
> > 
> > You can use a combination of "Report `BR` was emitted by one of the 
> > iterator checkers" and "The memory region of the container is marked as 
> > interesting" (while also actually marking it as interesting in the checker).
> > 
> > Ideally we should instead make a new generic storage inside the `BugReport` 
> > object, in order to pass down the interesting information from the call 
> > site of `emitReport` ("Hi, i'm an iterator checker who emitted this report 
> > and i'm interested in changes made to the size of this container").
> Are you sure in this? I already wondered how it works so I added a test that 
> checks one container and changes another one and there were no note tags 
> displayed for the one we did not check but change. See the last test.
That's because you didn't do
```lang=c++
  V2.cbegin();
  V2.cend();
```
in the beginning.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

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

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

`const SVal &` -> `SVal`


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

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/test/Analysis/container-modeling.cpp

Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -analyzer-output=text -verify
 
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -analyzer-output=text -verify
 
 // RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true %s 2>&1 | FileCheck %s
 
@@ -20,14 +20,16 @@
   V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
-  clang_analyzer_express(clang_analyzer_container_begin(V)); //expected-warning{{$V.begin()}}
+  clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
 }
 
 void end(const std::vector ) {
   V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
-  clang_analyzer_express(clang_analyzer_container_end(V)); //expected-warning{{$V.end()}}
+  clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
+   // expected-note@-1{{$V.end()}}
 }
 
 
@@ -48,8 +50,10 @@
   long B2 = clang_analyzer_container_begin(V2);
   long E2 = clang_analyzer_container_end(V2);
   V1 = std::move(V2);
-  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); //expected-warning{{TRUE}}
-  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); //expected-warning{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); // expected-warning{{TRUE}}
+ // expected-note@-1{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); // expected-warning{{TRUE}}
+   // expected-note@-1{{TRUE}}
 }
 
 
@@ -70,10 +74,13 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.push_back(n);
+  V.push_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+  // expected-note@-1{{Container 'V' extended to the right by 1 position}}
 
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
+   // expected-note@-1{{$V.end() + 1}}
 }
 
 /// emplace_back()
@@ -88,10 +95,14 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.emplace_back(n);
+  V.emplace_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+ // expected-note@-1{{Container 'V' extended to the right by 1 position}}
+
 
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
+   // 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

NoQ wrote:
> You'll need to check whether the container is actually of interest to the bug 
> report. We don't want notes to be added about changes to irrelevant 
> containers.
> 
> You can use a combination of "Report `BR` was emitted by one of the iterator 
> checkers" and "The memory region of the container is marked as interesting" 
> (while also actually marking it as interesting in the checker).
> 
> Ideally we should instead make a new generic storage inside the `BugReport` 
> object, in order to pass down the interesting information from the call site 
> of `emitReport` ("Hi, i'm an iterator checker who emitted this report and i'm 
> interested in changes made to the size of this container").
Are you sure in this? I already wondered how it works so I added a test that 
checks one container and changes another one and there were no note tags 
displayed for the one we did not check but change. See the last test.


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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Also, yay, thanks for using the new API!




Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:41
 const SVal  = UndefinedVal()) const;
-  void handleAssign(CheckerContext , const SVal ) const;
-  void handleClear(CheckerContext , const SVal ) const;
-  void handlePushBack(CheckerContext , const SVal ) const;
-  void handlePopBack(CheckerContext , const SVal ) const;
-  void handlePushFront(CheckerContext , const SVal ) const;
-  void handlePopFront(CheckerContext , const SVal ) const;
+  void handleAssign(CheckerContext , const SVal ,
+   const Expr *ContE) const;

While we're at it, let's not pass `SVal`s by reference? They're small 
value-types by design.


Repository:
  rC Clang

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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

You'll need to check whether the container is actually of interest to the bug 
report. We don't want notes to be added about changes to irrelevant containers.

You can use a combination of "Report `BR` was emitted by one of the iterator 
checkers" and "The memory region of the container is marked as interesting" 
(while also actually marking it as interesting in the checker).

Ideally we should instead make a new generic storage inside the `BugReport` 
object, in order to pass down the interesting information from the call site of 
`emitReport` ("Hi, i'm an iterator checker who emitted this report and i'm 
interested in changes made to the size of this container").


Repository:
  rC Clang

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

https://reviews.llvm.org/D73720



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

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[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: Charusso, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, rnkovacs, szepet, xazax.hun.

Container operations such as `push_back()`, `pop_front()` etc. increment and 
decrement the abstract begin and end symbols of containers. This patch 
introduces note tags to `ContainerModeling` to track these changes. This helps 
the user to better identify the source of errors related to containers and 
iterators.


Repository:
  rC Clang

https://reviews.llvm.org/D73720

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/test/Analysis/container-modeling.cpp

Index: clang/test/Analysis/container-modeling.cpp
===
--- clang/test/Analysis/container-modeling.cpp
+++ clang/test/Analysis/container-modeling.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -analyzer-output=text -verify
 
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -analyzer-output=text -verify
 
 // RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true %s 2>&1 | FileCheck %s
 
@@ -20,14 +20,16 @@
   V.begin();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
-  clang_analyzer_express(clang_analyzer_container_begin(V)); //expected-warning{{$V.begin()}}
+  clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
 }
 
 void end(const std::vector ) {
   V.end();
 
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
-  clang_analyzer_express(clang_analyzer_container_end(V)); //expected-warning{{$V.end()}}
+  clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end()}}
+   // expected-note@-1{{$V.end()}}
 }
 
 
@@ -48,8 +50,10 @@
   long B2 = clang_analyzer_container_begin(V2);
   long E2 = clang_analyzer_container_end(V2);
   V1 = std::move(V2);
-  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); //expected-warning{{TRUE}}
-  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); //expected-warning{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_begin(V1) == B2); // expected-warning{{TRUE}}
+ // expected-note@-1{{TRUE}}
+  clang_analyzer_eval(clang_analyzer_container_end(V2) == E2); // expected-warning{{TRUE}}
+   // expected-note@-1{{TRUE}}
 }
 
 
@@ -70,10 +74,13 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.push_back(n);
+  V.push_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+  // expected-note@-1{{Container 'V' extended to the right by 1 position}}
 
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
+ // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
+   // expected-note@-1{{$V.end() + 1}}
 }
 
 /// emplace_back()
@@ -88,10 +95,14 @@
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.emplace_back(n);
+  V.emplace_back(n); // expected-note{{Container 'V' extended to the right by 1 position}}
+ //