[PATCH] D100955: [-Wcalled-once] Do not run analysis on Obj-C++

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D100955#4054569 , @thakis wrote:

> Can you say a few words on _why_ this is emitted in only Objective-C? What's 
> missing for Objective-C++? I was surprised that this warning fired in a .m 
> file but not in a .mm file.

Technically there is nothing that prevents us from supporting Objective-C++, 
but the warning implementation will need to handle it correctly. Currently, it 
doesn't support lambda expressions and C++ exceptions from the top of my head. 
Probably there are more language constructs that can cause incorrect warnings. 
Long story short, it needs additional logic and testing.

Cheers!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100955

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

[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

@aaron.ballman a gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110

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

[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Finally I had a chance to come back to this patch.
@aaron.ballman what do you think about it?  I tried to address your notes and 
implemented both features under one attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110

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

[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 367788.
vsavchenko added a comment.
Herald added a subscriber: manas.

Join 'suppress' and 'analyzer_suppress' attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/test/Analysis/suppression-attr.m
  clang/test/SemaCXX/attr-suppress.cpp
  clang/test/SemaCXX/suppress.cpp
  clang/test/SemaObjC/attr-suppress.m

Index: clang/test/SemaObjC/attr-suppress.m
===
--- /dev/null
+++ clang/test/SemaObjC/attr-suppress.m
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks %s -verify
+
+#define SUPPRESS1 __attribute__((suppress))
+#define SUPPRESS2(...) __attribute__((suppress(__VA_ARGS__)))
+
+SUPPRESS1 int global = 42;
+
+SUPPRESS1 void foo() {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  SUPPRESS1 int *p;
+
+  SUPPRESS1 int a = 0; // no-warning
+  SUPPRESS2()
+  int b = 1; // no-warning
+  SUPPRESS2("a")
+  int c = a + b; // no-warning
+  SUPPRESS2("a", "b") { b = c - a; } // no-warning
+
+  SUPPRESS2("a", "b")
+  if (b == 10)
+a += 4;  // no-warning
+  SUPPRESS1 while (1) {} // no-warning
+  SUPPRESS1 switch (a) { // no-warning
+  default:
+c -= 10;
+  }
+
+  // GNU-style attributes and C++11 attributes apply to different things when
+  // written like this.  GNU  attribute gets attached to the declaration, while
+  // C++11 attribute ends up on the type.
+  int SUPPRESS2("r") z;
+  SUPPRESS2(foo)
+  float f;
+  // expected-error@-2 {{'suppress' attribute requires a string}}
+}
+
+union SUPPRESS2("type.1") U {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+  int i;
+  float f;
+};
+
+SUPPRESS1 @interface Test {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+}
+@property SUPPRESS2("prop") int *prop;
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+- (void)bar:(int)x SUPPRESS1;
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+@end
Index: clang/test/SemaCXX/suppress.cpp
===
--- clang/test/SemaCXX/suppress.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
-
-[[gsl::suppress("globally")]];
-
-namespace N {
-  [[gsl::suppress("in-a-namespace")]];
-}
-
-[[gsl::suppress("readability-identifier-naming")]]
-void f_() {
-  int *p;
-  [[gsl::suppress("type", "bounds")]] {
-p = reinterpret_cast(7);
-  }
-
-  [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
-  [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
-  int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
-  [[gsl::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
-}
-
-union [[gsl::suppress("type.1")]] U {
-  int i;
-  float f;
-};
Index: clang/test/SemaCXX/attr-suppress.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-suppress.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[gsl::suppress("globally")]];
+
+namespace N {
+[[gsl::suppress("in-a-namespace")]];
+}
+
+[[gsl::suppress("readability-identifier-naming")]] void f_() {
+  int *p;
+  [[gsl::suppress("type", "bounds")]] {
+p = reinterpret_cast(7);
+  }
+
+  [[gsl::suppress]] int x;   // expected-error {{'suppress' attribute takes at least 1 argument}}
+  [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+  int [[gsl::suppress("r")]] z;  // expected-error {{'suppress' attribute cannot be applied to types}}
+  [[gsl::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+  int i;
+  float f;
+};
+
+[[clang::suppress]];
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+
+namespace N {
+[[clang::suppress("in-a-namespace")]];
+// expected-error@-1 {{'suppress' attribute only applies to variables and statements}}
+} // namespace N
+
+[[clang::suppress]] int global = 42;
+
+[[clang::suppress]] void foo() {
+  // expected-error@-1 {{'suppress' attribute only applies to variables and 

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Now it looks good!  Thanks again!


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

https://reviews.llvm.org/D107366

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

[PATCH] D107636: [analyzer][solver] Compute adjustment for unsupported symbols as well

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D107636#2931302 , @steakhal wrote:

> Seems reasonable to me. Let's wait for someone else as well.

Sure, NP.

> This is a really elegant patch, I should tell!

Thanks!  I guess my take on this, that this path to the solver just got 
forgotten and that's what produced this inconsistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107636

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

[PATCH] D107636: [analyzer][solver] Compute adjustment for unsupported symbols as well

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
ASDenysPetrov, manas, RedDocMD.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When the solver sees conditions like `a +/- INT cmp INT`, it disassembles
such expressions and uses the first integer constant as a so-called 
"Adjustment".
So it's easier later on to reason about the range of the symbol (`a`
in this case).

However, conditions of form `a +/- INT` are not treated the same way,
and we might end up with contradictory constraints.

This patch resolves this issue and extracts "Adjustment" for the
latter case as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107636

Files:
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/infeasible_paths.c


Index: clang/test/Analysis/infeasible_paths.c
===
--- /dev/null
+++ clang/test/Analysis/infeasible_paths.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify 
%s
+
+// expected-no-diagnostics
+
+void clang_analyzer_warnIfReached();
+
+void unsupportedSymAssumption_1(int a) {
+  int b = a + 1;
+  if (b + 1)
+return;
+  // At this point, b == -1, a == -2
+  // and this condition is always true.
+  if (b < 1)
+return;
+  clang_analyzer_warnIfReached(); // no-warning
+}
+
+void unsupportedSymAssumption_2(int a) {
+  int b = a - 1;
+  if (!b)
+return;
+  if (b)
+return;
+  clang_analyzer_warnIfReached(); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -137,10 +137,13 @@
 
   // Reverse the operation and add directly to state.
   const llvm::APSInt  = BVF.getValue(0, T);
+  llvm::APSInt Adjustment = Zero;
+  computeAdjustment(Sym, Adjustment);
+
   if (Assumption)
-return assumeSymNE(State, Sym, Zero, Zero);
-  else
-return assumeSymEQ(State, Sym, Zero, Zero);
+return assumeSymNE(State, Sym, Zero, Adjustment);
+
+  return assumeSymEQ(State, Sym, Zero, Adjustment);
 }
 
 ProgramStateRef RangedConstraintManager::assumeSymRel(ProgramStateRef State,


Index: clang/test/Analysis/infeasible_paths.c
===
--- /dev/null
+++ clang/test/Analysis/infeasible_paths.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+void clang_analyzer_warnIfReached();
+
+void unsupportedSymAssumption_1(int a) {
+  int b = a + 1;
+  if (b + 1)
+return;
+  // At this point, b == -1, a == -2
+  // and this condition is always true.
+  if (b < 1)
+return;
+  clang_analyzer_warnIfReached(); // no-warning
+}
+
+void unsupportedSymAssumption_2(int a) {
+  int b = a - 1;
+  if (!b)
+return;
+  if (b)
+return;
+  clang_analyzer_warnIfReached(); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -137,10 +137,13 @@
 
   // Reverse the operation and add directly to state.
   const llvm::APSInt  = BVF.getValue(0, T);
+  llvm::APSInt Adjustment = Zero;
+  computeAdjustment(Sym, Adjustment);
+
   if (Assumption)
-return assumeSymNE(State, Sym, Zero, Zero);
-  else
-return assumeSymEQ(State, Sym, Zero, Zero);
+return assumeSymNE(State, Sym, Zero, Adjustment);
+
+  return assumeSymEQ(State, Sym, Zero, Adjustment);
 }
 
 ProgramStateRef RangedConstraintManager::assumeSymRel(ProgramStateRef State,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:1373
+  var array = [];
+  for (var i = lower; i <= upper; ++i) {
+  array.push(i);

ASDenysPetrov wrote:
> NoQ wrote:
> > Shouldn't it be `i < upper`?
> `[lower, upper).` Sure! Thanks!
And this bug is probably because of this!


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

https://reviews.llvm.org/D107366

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

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Now it captures (and makes bold) one extra arrow from the previous note.
Correct example attached!
F18364580: report-e4b488.html 


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

https://reviews.llvm.org/D107366

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

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Oh, wait!  I found a bug!


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

https://reviews.llvm.org/D107366

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

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko accepted this revision.
vsavchenko added a comment.

In D107366#2926662 , @ASDenysPetrov 
wrote:

> In D107366#2926343 , @NoQ wrote:
>
>> Works in Firefox on macOS as well!
>
> Great!
> @vsavchenko , any suggestions?

LGTM!  Thanks fo taking care of this!


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

https://reviews.llvm.org/D107366

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

[PATCH] D107366: [analyzer] Adjust JS code of analyzer's HTML report for IE support.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Can you please also attach an HTML file just to verify that it works?




Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:1293
+var classListAdd = function(el, theClass) {
+  if(!el.className.baseVal)
+el.className += " " + theClass;

nit: space please



Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:1396
 var arrow = document.querySelector("#arrow" + index);
-arrow.classList.add("selected");
+if(arrow) {
+  classListAdd(arrow, "selected")

nit: space please


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

https://reviews.llvm.org/D107366

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

[PATCH] D106823: [analyzer][solver] Iterate to a fixpoint during symbol simplification with constants

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Looking great!
I have a couple of nit picks and I kind of want to check that it doesn't affect 
the performance on a different set of projects as well.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2112-2113
+  assert(ClsMembers.contains(Old));
+  assert(ClsMembers.getHeight() > 1 &&
+ "Class should have at least two members");
+

Maybe after removing you can check that `ClsMembers` is not empty?
I just don't like relying on `getHeight` because it looks like an 
implementation detail and shouldn't be used.  We use it only in one place in 
the whole codebase (in equivalence classes :) ), but since we can re-write this 
assertion to have a simpler condition, I think that it should be preferred.



Comment at: 
clang/test/Analysis/symbol-simplification-fixpoint-iteration-unreachable-code.cpp:20
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_printState();
+

Do we need to print states in this test?



Comment at: 
clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp:32
+  // CHECK-NEXT:   "equivalence_classes": [
+  // CHECK-NEXT: [ "(reg_$0) != (reg_$2)" ],
+  // CHECK-NEXT: [ "reg_$0", "reg_$2" ]

Same question here



Comment at: 
clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp:36
+  // CHECK-NEXT:  "equivalence_classes": [
+  // CHECK-NEXT:[ "(reg_$0) != (reg_$3)" ],
+  // CHECK-NEXT:[ "reg_$0", "reg_$3" ],

OK, I understand the next two classes.
But how did we produce this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106823

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

[PATCH] D104647: [analyzer] Support SVal::getType for pointer-to-member values

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:85
   const NamedDecl *D;
+  QualType T;
   llvm::ImmutableList L;

NoQ wrote:
> What prevents you from obtaining the type from `D`? I guess `NamedDecl` 
> itself doesn't provide a `getType()` on its own but what specific sub-classes 
> can `D` be that don't have a type?
Because it's not a declaration of `pointer-to-member`, but a declaration of the 
actual member.  It can be `nullptr` while the type is always known.
I'd prefer not to construct a pointer-to-member type myself.  It's the work for 
Sema, not for the analyzer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104647

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

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/utils/analyzer/Dockerfile:15-22
 RUN apt-get update && apt-get install -y \
 git=1:2.17.1* \
 gettext=0.19.8.1* \
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
-cmake=3.20.5* \
-ninja-build=1.8.2-1
+cmake=3.21.1* \
+ninja-build=1.8.2-1 \

Changes to `clang/utils/analyzer/*` don't belong to this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

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

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

That looks interesting!
Can you please add tests, though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107339

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

[PATCH] D92928: [analyzer] Highlight arrows for currently selected event

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e02f58780ab: [analyzer] Highlight arrows for currently 
selected event (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92928

Files:
  clang/lib/Rewrite/HTMLRewrite.cpp
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/test/Analysis/html_diagnostics/control-arrows.cpp

Index: clang/test/Analysis/html_diagnostics/control-arrows.cpp
===
--- clang/test/Analysis/html_diagnostics/control-arrows.cpp
+++ clang/test/Analysis/html_diagnostics/control-arrows.cpp
@@ -16,10 +16,13 @@
 
 // CHECK:  
-// CHECK-NOT:  
+// CHECK-COUNT-9:  
+// CHECK-NOT:  
 // CHECK:
 // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: const arrowIndices = [ 9,8,6,5,3,2,0 ]
+// CHECK-NEXT: 
 //
 // Except for arrows we still want to have grey bubbles with control notes.
 // CHECK:   {
+  using Base = std::vector;
+
+public:
+  ArrowMap(unsigned Size) : Base(Size, 0) {}
+  unsigned getTotalNumberOfArrows() const { return at(0); }
+};
+
+llvm::raw_ostream <<(llvm::raw_ostream , const ArrowMap ) {
+  OS << "[ ";
+  llvm::interleave(Indices, OS, ",");
+  return OS << " ]";
+}
+
 } // namespace
 
 void ento::createHTMLDiagnosticConsumer(
@@ -761,6 +779,7 @@
   unsigned NumberOfArrows = 0;
   // Stores the count of the regular piece indices.
   std::map IndexMap;
+  ArrowMap ArrowIndices(TotalRegularPieces + 1);
 
   // Stores the different ranges where we have reported something.
   std::vector PopUpRanges;
@@ -779,13 +798,30 @@
 } else if (isArrowPiece(Piece)) {
   NumberOfArrows = ProcessControlFlowPiece(
   R, FID, cast(Piece), NumberOfArrows);
+  ArrowIndices[NumRegularPieces] = NumberOfArrows;
 
 } else {
   HandlePiece(R, FID, Piece, PopUpRanges, NumRegularPieces,
   TotalRegularPieces);
   --NumRegularPieces;
+  ArrowIndices[NumRegularPieces] = ArrowIndices[NumRegularPieces + 1];
 }
   }
+  ArrowIndices[0] = NumberOfArrows;
+
+  // At this point ArrowIndices represent the following data structure:
+  //   [a_0, a_1, ..., a_N]
+  // where N is the number of events in the path.
+  //
+  // Then for every event with index i \in [0, N - 1], we can say that
+  // arrows with indices \in [a_(i+1), a_i) correspond to that event.
+  // We can say that because arrows with these indices appeared in the
+  // path in between the i-th and the (i+1)-th events.
+  assert(ArrowIndices.back() == 0 &&
+ "No arrows should be after the last event");
+  // This assertion also guarantees that all indices in are <= NumberOfArrows.
+  assert(llvm::is_sorted(ArrowIndices, std::greater()) &&
+ "Incorrect arrow indices map");
 
   // Secondary indexing if we are having multiple pop-ups between two notes.
   // (e.g. [(13) 'a' is 'true'];  [(13.1) 'b' is 'false'];  [(13.2) 'c' is...)
@@ -819,7 +855,7 @@
   html::EscapeText(R, FID);
   html::AddLineNumbers(R, FID);
 
-  addArrowSVGs(R, FID, NumberOfArrows);
+  addArrowSVGs(R, FID, ArrowIndices);
 
   // If we have a preprocessor, relex the file and syntax highlight.
   // We might not have a preprocessor if we come from a deserialized AST file,
@@ -1088,7 +1124,7 @@
 }
 
 void HTMLDiagnostics::addArrowSVGs(Rewriter , FileID BugFileID,
-   unsigned NumberOfArrows) {
+   const ArrowMap ) {
   std::string S;
   llvm::raw_string_ostream OS(S);
 
@@ -1103,27 +1139,52 @@
   pointer-events: none;
   overflow: visible
   }
+  .arrow {
+  stroke-opacity: 0.2;
+  stroke-width: 1;
+  marker-end: url(#arrowhead);
+  }
+
+  .arrow.selected {
+  stroke-opacity: 0.6;
+  stroke-width: 2;
+  marker-end: url(#arrowheadSelected);
+  }
+
+  .arrowhead {
+  orient: auto;
+  stroke: none;
+  opacity: 0.6;
+  fill: blue;
+  }
 
 http://www.w3.org/2000/svg;>
   
-
+
+  
+
+
   
 
   
-  
+  
 )<<<";
 
-  for (unsigned Index : llvm::seq(0u, NumberOfArrows)) {
-OS << "\n";
+  for (unsigned Index : llvm::seq(0u, ArrowIndices.getTotalNumberOfArrows())) {
+OS << "\n";
   }
 
   OS << R"<<<(
   
 
-)<<<";
+
+const arrowIndices = )<<<";
+
+  OS << ArrowIndices << "\n\n";
 
   R.InsertTextBefore(R.getSourceMgr().getLocForStartOfFile(BugFileID),
  OS.str());
@@ -1220,7 +1281,7 @@
 });
 
 var findNum = function() {
-var s = document.querySelector(".selected");
+var s = document.querySelector(".msg.selected");
 if (!s || s.id == "EndPath") {
 return 0;
 }
@@ -1235,6 +1296,7 @@
 el.classList.add("selected");
 window.scrollBy(0, el.getBoundingClientRect().top -
 (window.innerHeight / 2));
+highlightArrowsForSelectedEvent();
 }
 
 

[PATCH] D92639: [analyzer] Add control flow arrows to the analyzer's HTML reports

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG97bcafa28deb: [analyzer] Add control flow arrows to the 
analyzers HTML reports (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92639

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/lib/Rewrite/HTMLRewrite.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/test/Analysis/html_diagnostics/control-arrows.cpp

Index: clang/test/Analysis/html_diagnostics/control-arrows.cpp
===
--- /dev/null
+++ clang/test/Analysis/html_diagnostics/control-arrows.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -fR %t
+// RUN: mkdir %t
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:-analyzer-output=html -o %t -verify %s
+// RUN: cat %t/report-*.html | FileCheck %s
+
+int dereference(int *x) {
+  return *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+int foobar(bool cond, int *x) {
+  if (cond)
+x = 0;
+  return dereference(x);
+}
+
+// CHECK:  
+// CHECK-NOT:  
+// CHECK:
+// CHECK-NEXT: 
+//
+// Except for arrows we still want to have grey bubbles with control notes.
+// CHECK:  2
+// CHECK-SAME:   Taking true branch
Index: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
@@ -27,6 +27,7 @@
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
@@ -77,60 +78,78 @@
   void FlushDiagnosticsImpl(std::vector ,
 FilesMade *filesMade) override;
 
-  StringRef getName() const override {
-return "HTMLDiagnostics";
-  }
+  StringRef getName() const override { return "HTMLDiagnostics"; }
 
   bool supportsCrossFileDiagnostics() const override {
 return SupportsCrossFileDiagnostics;
   }
 
-  unsigned ProcessMacroPiece(raw_ostream ,
- const PathDiagnosticMacroPiece& P,
+  unsigned ProcessMacroPiece(raw_ostream , const PathDiagnosticMacroPiece ,
  unsigned num);
 
+  unsigned ProcessControlFlowPiece(Rewriter , FileID BugFileID,
+   const PathDiagnosticControlFlowPiece ,
+   unsigned Number);
+
   void HandlePiece(Rewriter , FileID BugFileID, const PathDiagnosticPiece ,
const std::vector , unsigned num,
unsigned max);
 
-  void HighlightRange(Rewriter& R, FileID BugFileID, SourceRange Range,
+  void HighlightRange(Rewriter , FileID BugFileID, SourceRange Range,
   const char *HighlightStart = "",
   const char *HighlightEnd = "");
 
-  void ReportDiag(const PathDiagnostic& D,
-  FilesMade *filesMade);
+  void ReportDiag(const PathDiagnostic , FilesMade *filesMade);
 
   // Generate the full HTML report
-  std::string GenerateHTML(const PathDiagnostic& D, Rewriter ,
-   const SourceManager& SMgr, const PathPieces& path,
+  std::string GenerateHTML(const PathDiagnostic , Rewriter ,
+   const SourceManager , const PathPieces ,
const char *declName);
 
   // Add HTML header/footers to file specified by FID
-  void FinalizeHTML(const PathDiagnostic& D, Rewriter ,
-const SourceManager& SMgr, const PathPieces& path,
+  void FinalizeHTML(const PathDiagnostic , Rewriter ,
+const SourceManager , const PathPieces ,
 FileID FID, const FileEntry *Entry, const char *declName);
 
   // Rewrite the file specified by FID with HTML formatting.
-  void RewriteFile(Rewriter , const PathPieces& path, FileID FID);
+  void RewriteFile(Rewriter , const PathPieces , FileID FID);
 
+  PathGenerationScheme getGenerationScheme() const override {
+return Everything;
+  }
 
 private:
+  void addArrowSVGs(Rewriter , FileID BugFileID, unsigned NumberOfArrows);
+
   /// \return Javascript for displaying shortcuts help;
   StringRef showHelpJavascript();
 
   /// \return Javascript for navigating the HTML report using j/k keys.
   StringRef generateKeyboardNavigationJavascript();
 
+  /// \return Javascript for drawing control-flow arrows.
+  StringRef generateArrowDrawingJavascript();
+
   /// \return JavaScript for an option to only show relevant lines.
-  std::string showRelevantLinesJavascript(
-

[PATCH] D92928: [analyzer] Highlight arrows for currently selected event

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D92928#2913552 , @ASDenysPetrov 
wrote:

> Or, we can find another symbiotic way. You can make changes the way without 
> painfull part of thinking about IE. And I will prepare the next patch 
> adjusting it. Thus, revisions would be smaller. That's would be easier for 
> you to test all the things before the load. I will take a charge for IE part 
> on my own and prepare a new revision.

That actually can work!  Thanks!

> Let's load this patch and I will prepare the adjustment for IE in pursuit.

Ah, OK.  Sure, let's do it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92928

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

[PATCH] D107026: [Clang] Add support for attribute 'escape'

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Awesome, I have nothing to add at this point!
Let's still wait for @aaron.ballman to check it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107026

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

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:381
+const MemRegion *ThisRegion = DC->getCXXThisVal().getAsRegion();
+assert(ThisRegion && "We do not support explicit calls to destructor");
+const auto *InnerPtrVal = State->get(ThisRegion);

RedDocMD wrote:
> vsavchenko wrote:
> > And if it happens we are going to crash with assertion failure?
> Assuming assertions are enabled, that is.
We should never crash or fail on valid C++ code.  We can abandon everything, 
forbid checker to report anything on a function that has something that we 
don't know how to handle properly, but never fail the overall analysis process 
because of that. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105821

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

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:143
+llvm::ArrayRef ValidNames(STD_PTR_NAMES);
+return llvm::is_contained(ValidNames, Name);
   }

And why can't we pass `STD_PTR_NAMES` directly to `llvm::is_contained`?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:378-387
+llvm::errs() << "Found destructor call\n";
+State = DC->invalidateRegions(C.blockCount(), State);
+const MemRegion *ThisRegion = DC->getCXXThisVal().getAsRegion();
+assert(ThisRegion && "We do not support explicit calls to destructor");
+const auto *InnerPtrVal = State->get(ThisRegion);
+State = State->remove(ThisRegion);
+if (InnerPtrVal)

I suggest to add a ton of comments with the reasoning behind these actions.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:381
+const MemRegion *ThisRegion = DC->getCXXThisVal().getAsRegion();
+assert(ThisRegion && "We do not support explicit calls to destructor");
+const auto *InnerPtrVal = State->get(ThisRegion);

And if it happens we are going to crash with assertion failure?



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:421-422
   const auto *TrackingExpr = Call.getArgExpr(0);
-  assert(TrackingExpr->getType()->isPointerType() &&
- "Adding a non pointer value to TrackedRegionMap");
+  if (TrackingExpr->getType()->isPointerType())
+return false;
   auto ArgVal = Call.getArgSVal(0);

Okay, I'm either missing something or this condition is missing `!` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105821

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

[PATCH] D107026: [Clang] Add support for attribute 'escape'

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Great job!  It looks good, but I have a couple of minor tweaks.

I see that the "applies to pointer arguments only" warning is not tested for 
`noescape`, but I still find it to be a good practice to write a test with a 
bunch of cases with attributes applied in wrong places.

Additionally, I believe that `escape` and `noescape` convey the same 
information, and it is totally okay to have two attributes because the user has 
to have a choice to say either "I do escape it" or "I definitely do not escape 
it" when the default is unspecified at all.  But I do think that we should add 
a check disallowing both of these attributes to be used at the same time for 
the same parameter, since they directly contradict one another.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107026

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

[PATCH] D106739: [analyzer] Add option to SATest.py for extra checkers

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Looks good!  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106739

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

[PATCH] D106285: [Analyzer][solver] Fix inconsistent equivalence class data

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Oh, I didn't accept it?  Sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106285

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

[PATCH] D106416: [analyzer] Fix build dependency issues for SATest

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Awesome!  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106416

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Can we please land the fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103440#2891915 , @manas wrote:

> Here is the proof of correctness of the algorithm using Z3: 
> https://gist.github.com/weirdsmiley/ad6a9dbf3370e96d29f9e90068931d25

There is a couple of fundamental problems there that you need to resolve.  I 
left my comments there.




Comment at: clang/test/Analysis/constant-folding.c:260
+
+  // Overflows on both ends
+  if (a >= 5 && a <= UINT_MAX - 5 && b <= 10) {

This looks like an overflow on one end (we couldn't have them on both ends with 
addition and unsigned integers) and wrapping around surpassing the beginning of 
the range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440

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

[PATCH] D92928: [analyzer] Highlight arrows for currently selected event

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I want to say that I really appreciate the effort you put into finding all the 
workarounds for IE, but it makes adding new features here incredibly painful 
because IE doesn't seem to support anything.  And the majority of developers 
(on Linux and on MacOS) have literally no way to test it.  What we gain from 
supporting IE for non-existing users, we lose in the ability to actually 
improve this code!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92928

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

[PATCH] D92928: [analyzer] Highlight arrows for currently selected event

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Maybe I'm missing something, but do we really need to care about IE?  The last 
version was released in 2013, and even Microsoft itself stops supporting IE.  
Why should we care?  Is there anyone who uses old deprecated browser that is 
not maintained?  `classList` thing was here for almost 4 years and no one 
seemed to care.  Am I missing something here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92928

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko accepted this revision.
vsavchenko added a comment.

Great, LGTM!
But let's wait for @xazax.hun anyways


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:252
+static bool isStdFunctionCall(const CallEvent ) {
+  return Call.getDecl() && Call.getDecl() ->getDeclContext()->isStdNamespace();
+}

nit: there's an extra space here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:252-255
+  const auto *Decl = Call.getDecl();
+  if (!Decl)
+return false;
+  return Decl->getDeclContext()->isStdNamespace();

Can we use a one-liner that I suggested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Also, I tested this fix on a set of open-source projects and I don't see any 
problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D106285: [Analyzer][solver] Fix inconsistent equivalence class data

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Ah, I see now!  I think we could've put together a somewhat easier test knowing 
what's wrong, but it's not important at all.
Thanks for addressing this issue!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106285

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

[PATCH] D106136: [Analyzer][solver] Fix equivalence class invariant violation in removeDeadBindings

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D106136#2889610 , @martong wrote:

> In D106136#2883707 , @vsavchenko 
> wrote:
>
>> In D106136#2883100 , @martong 
>> wrote:
>>
>>> Thanks for taking your time to take a look. And I accept your statements. 
>>> Nevertheless, let me explain my motivation.
>>>
>>> I thought that a class is trivial if it has only one member. And it seemed 
>>> perfectly logical to not store equivalence classes with one member. I.e `a` 
>>> equals to `a` does not hold any meaningful information it just takes 
>>> precious memory. When we add a new constraint we take careful steps to 
>>> avoid adding a new class with one member. However, when remove dead kicks 
>>> in, suddenly we end up having classes stored with one member, which is a 
>>> somewhat inconsistent design IMHO. Perhaps some better documentation could 
>>> have helped.
>>
>> Representative symbol gives its equivalence class an ID.  We use this ID for 
>> everything, for comparing and for mapping.  Since we live in a persistent 
>> world, we can't just change this ID at some point, it will basically mean 
>> that we want to replace a class with another class.  So, all maps where this 
>> class participated (constraints, class, members, disequality) should remove 
>> the old class and add the new class.  This is a huge burden.  You need to 
>> carefully do all this, and bloat existing data structures.
>>
>> Trivial class is an optimization for the vast majority of symbols that never 
>> get into any classes.  We still need to associate constraints with those.  
>> This is where implicit Symbol to Class conversion comes in handy.
>>
>> Now let's imagine that something else is merged into it.  We are obliged to 
>> officially map the symbol to its class, and the class to its members.  When 
>> this happens, it goes into the data structure FOREVER.  And when in the 
>> future, with only one member, instead of keeping something that we already 
>> have from other versions of the data structure, you decide to remove the 
>> item from the class map, you actually use more memory when it was there!  
>> This is how persistent data structures work, different versions of the same 
>> data structure share data.  And I'm not even talking here about the fact 
>> that you now need to replace the class with one ID with the class with 
>> another ID in every relationship.
>>
>> You can probably measure memory footprint in your example and see it for 
>> yourself.
>
> Yeah, makes sense. Thanks for taking more time for further explanations. 
> Still, IMHO, the definition of a **trivial class** needs a clear written 
> documentation in the code itself, so other developers can easier understand 
> and maintain this code. I am going to create an NFC patch that summarizes 
> this discussion in a comment attached to the `isTrivial` function.

That's a great idea!  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106136

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

[PATCH] D106296: [analyzer] Fix for faulty namespace test in SmartPtrModelling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:254-257
+  const auto *Decl = Call.getDecl();
+  if (!Decl)
+return false;
+  if (!Decl->getDeclContext()->isStdNamespace())

I think you can have a separate function:
```
bool isStdFunctionCall(const CallEvent ) {
  return Call.getDecl() && Call.getDecl()->getDeclContext()->isStdNamespace();
}
```
and then use it like this:
```
if (Call.getNumArgs() != 2 || !isStdFunctionCall(Call))
  return false;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106296

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

[PATCH] D106136: [Analyzer][solver] Fix equivalence class invariant violation in removeDeadBindings

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D106136#2883100 , @martong wrote:

> Thanks for taking your time to take a look. And I accept your statements. 
> Nevertheless, let me explain my motivation.
>
> I thought that a class is trivial if it has only one member. And it seemed 
> perfectly logical to not store equivalence classes with one member. I.e `a` 
> equals to `a` does not hold any meaningful information it just takes precious 
> memory. When we add a new constraint we take careful steps to avoid adding a 
> new class with one member. However, when remove dead kicks in, suddenly we 
> end up having classes stored with one member, which is a somewhat 
> inconsistent design IMHO. Perhaps some better documentation could have helped.

Representative symbol gives its equivalence class an ID.  We use this ID for 
everything, for comparing and for mapping.  Since we live in a persistent 
world, we can't just change this ID at some point, it will basically mean that 
we want to replace a class with another class.  So, all maps where this class 
participated (constraints, class, members, disequality) should remove the old 
class and add the new class.  This is a huge burden.  You need to carefully do 
all this, and bloat existing data structures.

Trivial class is an optimization for the vast majority of symbols that never 
get into any classes.  We still need to associate constraints with those.  This 
is where implicit Symbol to Class conversion comes in handy.

Now let's imagine that something else is merged into it.  We are obliged to 
officially map the symbol to its class, and the class to its members.  When 
this happens, it goes into the data structure FOREVER.  And when in the future, 
with only one member, instead of keeping something that we already have from 
other versions of the data structure, you decide to remove the item from the 
class map, you actually use more memory when it was there!  This is how 
persistent data structures work, different versions of the same data structure 
share data.  And I'm not even talking here about the fact that you now need to 
replace the class with one ID with the class with another ID in every 
relationship.

You can probably measure memory footprint in your example and see it for 
yourself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106136

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

[PATCH] D106136: [Analyzer][solver] Fix equivalence class invariant violation in removeDeadBindings

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Thanks for working on it, but it is a quite large change that I don't get the 
motivation for (it doesn't even fix the recently found bug).

Summary explains what the patch does, but for why it is done, it talks about an 
invariant that is not specified anywhere in the code.

When I was implementing it, trivial class is a symbol that was never ever equal 
to anything else.  The moment we make merge something into it, it stops being 
trivial, forever!  Representative is an implementation detail and shouldn't be 
required to represent the actual information.  What I'm saying is that 
representative can be long gone, but the class can still hold.  If we use 
representative symbol for something other than its pointer or type after it's 
gone, the mistake is using representative in the first place, not the 
motivation to make it more complex and change representatives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106136

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

[PATCH] D106102: [analyzer][solver] Introduce reasoning for not equal to operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1223-1225
+template <>
+RangeSet SymbolicRangeInferrer::VisitBinaryOperator(Range LHS, Range 
RHS,
+   QualType T) {

I think it should be a specialization for another `VisitBinaryOperator`.
In the switch, you can see that we give range sets for `LHS` and `RHS`, so how 
does it work?
There is a function in between (also `VisitBinaryOperator`) that creates simple 
ranges out of range sets and ask to visit binary operator for those.  You can 
specialize it instead since we can simply check for empty intersection of range 
sets.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+  // In all other cases, the resulting range cannot be deduced.
+  return RangeFactory.getEmptySet();
+}

Empty range set means "This situation is IMPOSSIBLE".  Is that what you want 
here?



Comment at: clang/test/Analysis/constant-folding.c:470-504
+  // Checks when ranges are not overlapping
+  if (a <= 10 && b >= 20) {
+clang_analyzer_eval((a != b) != 0); // expected-warning{{TRUE}}
+  }
+
+  if (c <= INT_MIN + 10 && d >= INT_MAX - 10) {
+clang_analyzer_eval((c != d) == 0); // expected-warning{{FALSE}}

Did you try it in debugger, do we get inside of your function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106102

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1908-1912
+// Handle SymbolCast before actual assignment.
+std::tie(Sym, NewConstraint) =
+modifySymbolAndConstraints(Sym, NewConstraint);
+if (!State)
+  return nullptr;

ASDenysPetrov wrote:
> vsavchenko wrote:
> > That's not using `ConstraintAssignor`, you simply put your implementation 
> > in here.  That won't do!
> OK, please tell me how to use it correctly in my case.
Can you read the comments first and then ask me if you have any specific 
questions?


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1908-1912
+// Handle SymbolCast before actual assignment.
+std::tie(Sym, NewConstraint) =
+modifySymbolAndConstraints(Sym, NewConstraint);
+if (!State)
+  return nullptr;

That's not using `ConstraintAssignor`, you simply put your implementation in 
here.  That won't do!


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

https://reviews.llvm.org/D103096

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

[PATCH] D106063: [Analyzer][solver] Remove unused functions

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Yes, let's do this!  Thanks for addressing it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106063

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2877818 , @ASDenysPetrov 
wrote:

> Made `ignoreCast` non-virtual.
> P.S.  IMO, this change is not something that can be taken as a pattern, 
> though.

It is already a pattern in other type hierarchies.
Virtual functions are only good, when they can have multiple implementations.  
`ignoreCasts` by its name can have only one implementation and couldn't be 
virtual.  That's it!  It is more useable now, and less confusing for its users. 
 The fact that its definition lives in some other cpp file doesn't change it.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+SymbolRef Sym = Operand;
+while (isa(Sym))
+  Sym = cast(Sym)->Operand;
+return Sym;

ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > ASDenysPetrov wrote:
> > > > > vsavchenko wrote:
> > > > > > ASDenysPetrov wrote:
> > > > > > > vsavchenko wrote:
> > > > > > > > 
> > > > > > > Do you think the recursive call is better than the loop? But, I 
> > > > > > > guess, I see your point. You option could be safer if we had 
> > > > > > > another implementation of the virtual method. Or you think such 
> > > > > > > alike cast symbol is possible in the future? Well, for now 
> > > > > > > `ignoreCasts` doesn't make sense to any other `Expr` successors.
> > > > > > Oh, wait, why is it even virtual?  I don't think that it should be 
> > > > > > virtual.
> > > > > > Are similar functions in `Expr` virtual?
> > > > > > And I think that this implementation should live in `SymExpr` 
> > > > > > directly.  Then it would look like:
> > > > > > ```
> > > > > > if (const SymbolCast *ThisAsCast = dyn_cast(this)) {
> > > > > >   return ThisAsCast->ignoreCasts();
> > > > > > }
> > > > > > return this;
> > > > > > ```
> > > > > Yes, `SymExpr` is an abstract class. And because of limitations and 
> > > > > dependency of inheritance we are not able to know the implementaion 
> > > > > of `SymbolCast`. Unfortunately, this is not a CRTP.
> > > > Re-read my comment, please.
> > > > Oh, wait, why is it even virtual?
> > > `ignoreCasts` is a virtual function because I haven't found any other way 
> > > to implement it better.
> > > > I don't think that it should be virtual.
> > > Unfortunately, this is not a CRTP to avoid dynamic linking.
> > > > Are similar functions in Expr virtual?
> > > `SymExpr` is an abstract class. I'm not sure about similarity but 
> > > `SymExpr` has such virtual methods:
> > >   - computeComplexity
> > >   - getType 
> > >   - getOriginRegion
> > > > And I think that this implementation should live in SymExpr directly.
> > > It's impossible due to `SymExpr` implementation design. `SymExpr` knows 
> > > nothing about implementation details of `SymbolCast` to invoke 
> > > `ignoreCasts()`.
> > > 
> > a) `Expr` is also an abstract class
> > b) I put the implementation right there in the comment above.  I don't see 
> > any reasons not to use it.
> > c) I don't buy it about "impossible" and "implementation design" because 
> > you can always declare function in one place and define it in the other.
> I think I achieved of what you've been concerned.
This function should be removed then.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:3002-3004
+  std::tie(St, Sym, New) = modifySymbolAndConstraints(St, Sym, New);
+  if (!St)
+return nullptr;

No, please, remove duplication by putting it inside of the constraint assignor. 
 It is designed specifically so we don't duplicate code around `assumeSymXX` 
functions.


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+SymbolRef Sym = Operand;
+while (isa(Sym))
+  Sym = cast(Sym)->Operand;
+return Sym;

ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > ASDenysPetrov wrote:
> > > > > vsavchenko wrote:
> > > > > > 
> > > > > Do you think the recursive call is better than the loop? But, I 
> > > > > guess, I see your point. You option could be safer if we had another 
> > > > > implementation of the virtual method. Or you think such alike cast 
> > > > > symbol is possible in the future? Well, for now `ignoreCasts` doesn't 
> > > > > make sense to any other `Expr` successors.
> > > > Oh, wait, why is it even virtual?  I don't think that it should be 
> > > > virtual.
> > > > Are similar functions in `Expr` virtual?
> > > > And I think that this implementation should live in `SymExpr` directly. 
> > > >  Then it would look like:
> > > > ```
> > > > if (const SymbolCast *ThisAsCast = dyn_cast(this)) {
> > > >   return ThisAsCast->ignoreCasts();
> > > > }
> > > > return this;
> > > > ```
> > > Yes, `SymExpr` is an abstract class. And because of limitations and 
> > > dependency of inheritance we are not able to know the implementaion of 
> > > `SymbolCast`. Unfortunately, this is not a CRTP.
> > Re-read my comment, please.
> > Oh, wait, why is it even virtual?
> `ignoreCasts` is a virtual function because I haven't found any other way to 
> implement it better.
> > I don't think that it should be virtual.
> Unfortunately, this is not a CRTP to avoid dynamic linking.
> > Are similar functions in Expr virtual?
> `SymExpr` is an abstract class. I'm not sure about similarity but `SymExpr` 
> has such virtual methods:
>   - computeComplexity
>   - getType 
>   - getOriginRegion
> > And I think that this implementation should live in SymExpr directly.
> It's impossible due to `SymExpr` implementation design. `SymExpr` knows 
> nothing about implementation details of `SymbolCast` to invoke 
> `ignoreCasts()`.
> 
a) `Expr` is also an abstract class
b) I put the implementation right there in the comment above.  I don't see any 
reasons not to use it.
c) I don't buy it about "impossible" and "implementation design" because you 
can always declare function in one place and define it in the other.


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

>   // 1. `VisitSymbolCast`.
>   // Get a range for main `reg_$0` - [-2147483648, 2147483647]
>   // Cast main range to `short` - [-2147483648, 2147483647] -> [-32768, 
> 32767].
>   // Now we get a valid range for further bifurcation - [-32768, 32767].

That's a great example, thanks for putting it together.  I can see your point 
now!

Please, rebase your change and make use of `ConstraintAssignor`, and rework 
`VisitSymbolCast`.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+SymbolRef Sym = Operand;
+while (isa(Sym))
+  Sym = cast(Sym)->Operand;
+return Sym;

ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > 
> > > Do you think the recursive call is better than the loop? But, I guess, I 
> > > see your point. You option could be safer if we had another 
> > > implementation of the virtual method. Or you think such alike cast symbol 
> > > is possible in the future? Well, for now `ignoreCasts` doesn't make sense 
> > > to any other `Expr` successors.
> > Oh, wait, why is it even virtual?  I don't think that it should be virtual.
> > Are similar functions in `Expr` virtual?
> > And I think that this implementation should live in `SymExpr` directly.  
> > Then it would look like:
> > ```
> > if (const SymbolCast *ThisAsCast = dyn_cast(this)) {
> >   return ThisAsCast->ignoreCasts();
> > }
> > return this;
> > ```
> Yes, `SymExpr` is an abstract class. And because of limitations and 
> dependency of inheritance we are not able to know the implementaion of 
> `SymbolCast`. Unfortunately, this is not a CRTP.
Re-read my comment, please.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+if (!Opts.ShouldSupportSymbolicIntegerCasts)
+  return VisitSymExpr(Sym);
+

ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > ASDenysPetrov wrote:
> > > > > vsavchenko wrote:
> > > > > > Why do you use `VisitSymExpr` here?  You want to interrupt all 
> > > > > > `Visits or... I'm not sure I fully understand.
> > > > > Here we want to delegate the reasoning to another handler as we don't 
> > > > > support non-integral cast yet.
> > > > You are not delegating it here.  `Visit` includes a runtime dispatch 
> > > > that calls a correct `VisitTYPE` method.  Here you call `VisitSymExpr` 
> > > > directly, which is one of the `VisitTYPE` methods.  No dispatch will 
> > > > happen, and since we use `VisitSymExpr` as the last resort (it's the 
> > > > most base class, if we got there, we don't actually support the 
> > > > expression), you interrupt the `Visit` and go directly to "the last 
> > > > resort".
> > > > 
> > > > See the problem now?
> > > OK. I reject this idea before. If we call `Visit` inside 
> > > `VisitSymbolCast`, we will go into recursive loop, because it will return 
> > > us back to `VisitSymbolCast` as we have passed `Sym` as is. (This is 
> > > theoretically, I didn't check in practice.) Or I'm missing smth?
> > > I choosed `VisitSymExpr` here because all kinds of `SymbolCast` were 
> > > previously handled here. So I decided to pass all unsupproted forms of 
> > > casts there.
> > Did I suggest to `Visit(Sym)`?  Of course it is going to end up in a loop!  
> > Why isn't it `Visit(Sym->getOperand())` here?  Before we started producing 
> > casts, casts were transparent.  This logic would fit perfectly with that.
> > were transparent.
> Not exactly. There are still some cases when symbols are not equal to there 
> roots(aka Operands). Such cases were handled by `VisitSymExpr` which uses 
> `infer(Sym->getType());` instead of getOperand`. So this needs a sort of 
> think twice. Also I see a problem with `EquivalenceClass`'es. Consider next:
> ```
> int x, y;
> if(x == y)
>   if ((char)x == 2)
> if(y == 259)
>   // Here we shall update `(char)x` and find this branch infeasible.
> ```
> Also such cases like:
> ```
> if(x == (short)y)
>   // What we should do(store) with(in) `EquivalenceClass`es.
> ```
> Currently, I have an obscure vision of the solution.
> There are still some cases when symbols are not equal to there roots(aka 
> Operands)
Right now we don't have casts, this is what we do currently.  However faulty it 
is, it is the existing solution and we should respect that.

>  Also I see a problem with EquivalenceClass'es.
Because of the current situation with casts (or more precisely with their 
lack), `EquivalenceClass`es do not get merged for symbols with different types. 
 It is as simple as that.
You can find similar tests in `equality_tracking.c`.


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

https://reviews.llvm.org/D103096

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

[PATCH] D105693: [analyzer][solver][NFC] Refactor how we detect (dis)equalities

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG60bd8cbc0c84: [analyzer][solver][NFC] Refactor how we detect 
(dis)equalities (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105693

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/equality_tracking.c

Index: clang/test/Analysis/equality_tracking.c
===
--- clang/test/Analysis/equality_tracking.c
+++ clang/test/Analysis/equality_tracking.c
@@ -219,3 +219,17 @@
   if (c < 0)
 ;
 }
+
+void implyDisequalityFromGT(int a, int b) {
+  if (a > b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
+
+void implyDisequalityFromLT(int a, int b) {
+  if (a < b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -684,58 +684,30 @@
 //   Equality/diseqiality abstraction
 //===--===//
 
-/// A small helper structure representing symbolic equality.
+/// A small helper function for detecting symbolic (dis)equality.
 ///
 /// Equality check can have different forms (like a == b or a - b) and this
 /// class encapsulates those away if the only thing the user wants to check -
-/// whether it's equality/diseqiality or not and have an easy access to the
-/// compared symbols.
-struct EqualityInfo {
-public:
-  SymbolRef Left, Right;
-  // true for equality and false for disequality.
-  bool IsEquality = true;
-
-  void invert() { IsEquality = !IsEquality; }
-  /// Extract equality information from the given symbol and the constants.
-  ///
-  /// This function assumes the following expression Sym + Adjustment != Int.
-  /// It is a default because the most widespread case of the equality check
-  /// is (A == B) + 0 != 0.
-  static Optional extract(SymbolRef Sym, const llvm::APSInt ,
-const llvm::APSInt ) {
-// As of now, the only equality form supported is Sym + 0 != 0.
-if (!Int.isNullValue() || !Adjustment.isNullValue())
-  return llvm::None;
-
-return extract(Sym);
-  }
-  /// Extract equality information from the given symbol.
-  static Optional extract(SymbolRef Sym) {
-return EqualityExtractor().Visit(Sym);
+/// whether it's equality/diseqiality or not.
+///
+/// \returns true if assuming this Sym to be true means equality of operands
+///  false if it means disequality of operands
+///  None otherwise
+Optional meansEquality(const SymSymExpr *Sym) {
+  switch (Sym->getOpcode()) {
+  case BO_Sub:
+// This case is: A - B != 0 -> disequality check.
+return false;
+  case BO_EQ:
+// This case is: A == B != 0 -> equality check.
+return true;
+  case BO_NE:
+// This case is: A != B != 0 -> diseqiality check.
+return false;
+  default:
+return llvm::None;
   }
-
-private:
-  class EqualityExtractor
-  : public SymExprVisitor> {
-  public:
-Optional VisitSymSymExpr(const SymSymExpr *Sym) const {
-  switch (Sym->getOpcode()) {
-  case BO_Sub:
-// This case is: A - B != 0 -> disequality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  case BO_EQ:
-// This case is: A == B != 0 -> equality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), true};
-  case BO_NE:
-// This case is: A != B != 0 -> diseqiality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  default:
-return llvm::None;
-  }
-}
-  };
-};
+}
 
 //===--===//
 //Intersection functions
@@ -866,7 +838,13 @@
   }
 
   RangeSet VisitSymSymExpr(const SymSymExpr *Sym) {
-return VisitBinaryOperator(Sym);
+return intersect(
+RangeFactory,
+// If Sym is (dis)equality, we might have some information
+// on that in our equality classes data structure.
+getRangeForEqualities(Sym),
+// And we should always check what we can get from the operands.
+VisitBinaryOperator(Sym));
   }
 
 private:
@@ -907,9 +885,6 @@
 // calculate the effective range set by intersecting the range set
 // for A - B and the negated range set of B - A.
 getRangeForNegatedSub(Sym),
-// If Sym is (dis)equality, we might have some 

[PATCH] D105692: [analyzer][solver][NFC] Introduce ConstraintAssignor

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf26deb4e6ba7: [analyzer][solver][NFC] Introduce 
ConstraintAssignor (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105692

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -669,6 +669,17 @@
   return getConstraint(State, EquivalenceClass::find(State, Sym));
 }
 
+LLVM_NODISCARD ProgramStateRef setConstraint(ProgramStateRef State,
+ EquivalenceClass Class,
+ RangeSet Constraint) {
+  return State->set(Class, Constraint);
+}
+
+LLVM_NODISCARD ProgramStateRef setConstraints(ProgramStateRef State,
+  ConstraintRangeTy Constraints) {
+  return State->set(Constraints);
+}
+
 //===--===//
 //   Equality/diseqiality abstraction
 //===--===//
@@ -1373,6 +1384,182 @@
   return {RangeFactory, ValueFactory.getValue(Min), ValueFactory.getValue(Max)};
 }
 
+//===--===//
+// Constraint assignment logic
+//===--===//
+
+/// ConstraintAssignorBase is a small utility class that unifies visitor
+/// for ranges with a visitor for constraints (rangeset/range/constant).
+///
+/// It is designed to have one derived class, but generally it can have more.
+/// Derived class can control which types we handle by defining methods of the
+/// following form:
+///
+///   bool handle${SYMBOL}To${CONSTRAINT}(const SYMBOL *Sym,
+///   CONSTRAINT Constraint);
+///
+/// where SYMBOL is the type of the symbol (e.g. SymSymExpr, SymbolCast, etc.)
+///   CONSTRAINT is the type of constraint (RangeSet/Range/Const)
+///   return value signifies whether we should try other handle methods
+///  (i.e. false would mean to stop right after calling this method)
+template  class ConstraintAssignorBase {
+public:
+  using Const = const llvm::APSInt &;
+
+#define DISPATCH(CLASS) return assign##CLASS##Impl(cast(Sym), Constraint)
+
+#define ASSIGN(CLASS, TO, SYM, CONSTRAINT) \
+  if (!static_cast(this)->assign##CLASS##To##TO(SYM, CONSTRAINT))   \
+  return false
+
+  void assign(SymbolRef Sym, RangeSet Constraint) {
+assignImpl(Sym, Constraint);
+  }
+
+  bool assignImpl(SymbolRef Sym, RangeSet Constraint) {
+switch (Sym->getKind()) {
+#define SYMBOL(Id, Parent) \
+  case SymExpr::Id##Kind:  \
+DISPATCH(Id);
+#include "clang/StaticAnalyzer/Core/PathSensitive/Symbols.def"
+}
+llvm_unreachable("Unknown SymExpr kind!");
+  }
+
+#define DEFAULT_ASSIGN(Id) \
+  bool assign##Id##To##RangeSet(const Id *Sym, RangeSet Constraint) {  \
+return true;   \
+  }\
+  bool assign##Id##To##Range(const Id *Sym, Range Constraint) { return true; } \
+  bool assign##Id##To##Const(const Id *Sym, Const Constraint) { return true; }
+
+  // When we dispatch for constraint types, we first try to check
+  // if the new constraint is the constant and try the corresponding
+  // assignor methods.  If it didn't interrupt, we can proceed to the
+  // range, and finally to the range set.
+#define CONSTRAINT_DISPATCH(Id)\
+  if (const llvm::APSInt *Const = Constraint.getConcreteValue()) { \
+ASSIGN(Id, Const, Sym, *Const);\
+  }\
+  if (Constraint.size() == 1) {\
+ASSIGN(Id, Range, Sym, *Constraint.begin());   \
+  }\
+  ASSIGN(Id, RangeSet, Sym, Constraint)
+
+  // Our internal assign method first tries to call assignor methods for all
+  // constraint types that apply.  And if not interrupted, continues with its
+  // parent class.
+#define SYMBOL(Id, Parent)   

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

OK, thanks for putting a summary.  I now got a good idea why you need both.
At the same time, take a look at D105692 .  
I'm about to land it and I think it's going to be useful for you.


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

https://reviews.llvm.org/D103096

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

[PATCH] D105693: [analyzer][solver][NFC] Refactor how we detect (dis)equalities

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:687
 
 /// A small helper structure representing symbolic equality.
 ///

martong wrote:
> This is no longer a `structure`.
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105693

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

[PATCH] D105693: [analyzer][solver][NFC] Refactor how we detect (dis)equalities

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 358341.
vsavchenko marked an inline comment as done.
vsavchenko added a comment.

Fix comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105693

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/equality_tracking.c

Index: clang/test/Analysis/equality_tracking.c
===
--- clang/test/Analysis/equality_tracking.c
+++ clang/test/Analysis/equality_tracking.c
@@ -219,3 +219,17 @@
   if (c < 0)
 ;
 }
+
+void implyDisequalityFromGT(int a, int b) {
+  if (a > b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
+
+void implyDisequalityFromLT(int a, int b) {
+  if (a < b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -684,58 +684,30 @@
 //   Equality/diseqiality abstraction
 //===--===//
 
-/// A small helper structure representing symbolic equality.
+/// A small helper function for detecting symbolic (dis)equality.
 ///
 /// Equality check can have different forms (like a == b or a - b) and this
 /// class encapsulates those away if the only thing the user wants to check -
-/// whether it's equality/diseqiality or not and have an easy access to the
-/// compared symbols.
-struct EqualityInfo {
-public:
-  SymbolRef Left, Right;
-  // true for equality and false for disequality.
-  bool IsEquality = true;
-
-  void invert() { IsEquality = !IsEquality; }
-  /// Extract equality information from the given symbol and the constants.
-  ///
-  /// This function assumes the following expression Sym + Adjustment != Int.
-  /// It is a default because the most widespread case of the equality check
-  /// is (A == B) + 0 != 0.
-  static Optional extract(SymbolRef Sym, const llvm::APSInt ,
-const llvm::APSInt ) {
-// As of now, the only equality form supported is Sym + 0 != 0.
-if (!Int.isNullValue() || !Adjustment.isNullValue())
-  return llvm::None;
-
-return extract(Sym);
-  }
-  /// Extract equality information from the given symbol.
-  static Optional extract(SymbolRef Sym) {
-return EqualityExtractor().Visit(Sym);
+/// whether it's equality/diseqiality or not.
+///
+/// \returns true if assuming this Sym to be true means equality of operands
+///  false if it means disequality of operands
+///  None otherwise
+Optional meansEquality(const SymSymExpr *Sym) {
+  switch (Sym->getOpcode()) {
+  case BO_Sub:
+// This case is: A - B != 0 -> disequality check.
+return false;
+  case BO_EQ:
+// This case is: A == B != 0 -> equality check.
+return true;
+  case BO_NE:
+// This case is: A != B != 0 -> diseqiality check.
+return false;
+  default:
+return llvm::None;
   }
-
-private:
-  class EqualityExtractor
-  : public SymExprVisitor> {
-  public:
-Optional VisitSymSymExpr(const SymSymExpr *Sym) const {
-  switch (Sym->getOpcode()) {
-  case BO_Sub:
-// This case is: A - B != 0 -> disequality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  case BO_EQ:
-// This case is: A == B != 0 -> equality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), true};
-  case BO_NE:
-// This case is: A != B != 0 -> diseqiality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  default:
-return llvm::None;
-  }
-}
-  };
-};
+}
 
 //===--===//
 //Intersection functions
@@ -866,7 +838,13 @@
   }
 
   RangeSet VisitSymSymExpr(const SymSymExpr *Sym) {
-return VisitBinaryOperator(Sym);
+return intersect(
+RangeFactory,
+// If Sym is (dis)equality, we might have some information
+// on that in our equality classes data structure.
+getRangeForEqualities(Sym),
+// And we should always check what we can get from the operands.
+VisitBinaryOperator(Sym));
   }
 
 private:
@@ -907,9 +885,6 @@
 // calculate the effective range set by intersecting the range set
 // for A - B and the negated range set of B - A.
 getRangeForNegatedSub(Sym),
-// If Sym is (dis)equality, we might have some information on that
-// in our equality classes data structure.
-getRangeForEqualities(Sym),

[PATCH] D105692: [analyzer][solver][NFC] Introduce ConstraintAssignor

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 358336.
vsavchenko added a comment.

Fix comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105692

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -669,6 +669,17 @@
   return getConstraint(State, EquivalenceClass::find(State, Sym));
 }
 
+LLVM_NODISCARD ProgramStateRef setConstraint(ProgramStateRef State,
+ EquivalenceClass Class,
+ RangeSet Constraint) {
+  return State->set(Class, Constraint);
+}
+
+LLVM_NODISCARD ProgramStateRef setConstraints(ProgramStateRef State,
+  ConstraintRangeTy Constraints) {
+  return State->set(Constraints);
+}
+
 //===--===//
 //   Equality/diseqiality abstraction
 //===--===//
@@ -1373,6 +1384,182 @@
   return {RangeFactory, ValueFactory.getValue(Min), ValueFactory.getValue(Max)};
 }
 
+//===--===//
+// Constraint assignment logic
+//===--===//
+
+/// ConstraintAssignorBase is a small utility class that unifies visitor
+/// for ranges with a visitor for constraints (rangeset/range/constant).
+///
+/// It is designed to have one derived class, but generally it can have more.
+/// Derived class can control which types we handle by defining methods of the
+/// following form:
+///
+///   bool handle${SYMBOL}To${CONSTRAINT}(const SYMBOL *Sym,
+///   CONSTRAINT Constraint);
+///
+/// where SYMBOL is the type of the symbol (e.g. SymSymExpr, SymbolCast, etc.)
+///   CONSTRAINT is the type of constraint (RangeSet/Range/Const)
+///   return value signifies whether we should try other handle methods
+///  (i.e. false would mean to stop right after calling this method)
+template  class ConstraintAssignorBase {
+public:
+  using Const = const llvm::APSInt &;
+
+#define DISPATCH(CLASS) return assign##CLASS##Impl(cast(Sym), Constraint)
+
+#define ASSIGN(CLASS, TO, SYM, CONSTRAINT) \
+  if (!static_cast(this)->assign##CLASS##To##TO(SYM, CONSTRAINT))   \
+  return false
+
+  void assign(SymbolRef Sym, RangeSet Constraint) {
+assignImpl(Sym, Constraint);
+  }
+
+  bool assignImpl(SymbolRef Sym, RangeSet Constraint) {
+switch (Sym->getKind()) {
+#define SYMBOL(Id, Parent) \
+  case SymExpr::Id##Kind:  \
+DISPATCH(Id);
+#include "clang/StaticAnalyzer/Core/PathSensitive/Symbols.def"
+}
+llvm_unreachable("Unknown SymExpr kind!");
+  }
+
+#define DEFAULT_ASSIGN(Id) \
+  bool assign##Id##To##RangeSet(const Id *Sym, RangeSet Constraint) {  \
+return true;   \
+  }\
+  bool assign##Id##To##Range(const Id *Sym, Range Constraint) { return true; } \
+  bool assign##Id##To##Const(const Id *Sym, Const Constraint) { return true; }
+
+  // When we dispatch for constraint types, we first try to check
+  // if the new constraint is the constant and try the corresponding
+  // assignor methods.  If it didn't interrupt, we can proceed to the
+  // range, and finally to the range set.
+#define CONSTRAINT_DISPATCH(Id)\
+  if (const llvm::APSInt *Const = Constraint.getConcreteValue()) { \
+ASSIGN(Id, Const, Sym, *Const);\
+  }\
+  if (Constraint.size() == 1) {\
+ASSIGN(Id, Range, Sym, *Constraint.begin());   \
+  }\
+  ASSIGN(Id, RangeSet, Sym, Constraint)
+
+  // Our internal assign method first tries to call assignor methods for all
+  // constraint types that apply.  And if not interrupted, continues with its
+  // parent class.
+#define SYMBOL(Id, Parent) \
+  bool assign##Id##Impl(const Id *Sym, RangeSet Constraint) {  \
+CONSTRAINT_DISPATCH(Id);

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+if (!Opts.ShouldSupportSymbolicIntegerCasts)
+  return VisitSymExpr(Sym);
+

ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits 
> > > > or... I'm not sure I fully understand.
> > > Here we want to delegate the reasoning to another handler as we don't 
> > > support non-integral cast yet.
> > You are not delegating it here.  `Visit` includes a runtime dispatch that 
> > calls a correct `VisitTYPE` method.  Here you call `VisitSymExpr` directly, 
> > which is one of the `VisitTYPE` methods.  No dispatch will happen, and 
> > since we use `VisitSymExpr` as the last resort (it's the most base class, 
> > if we got there, we don't actually support the expression), you interrupt 
> > the `Visit` and go directly to "the last resort".
> > 
> > See the problem now?
> OK. I reject this idea before. If we call `Visit` inside `VisitSymbolCast`, 
> we will go into recursive loop, because it will return us back to 
> `VisitSymbolCast` as we have passed `Sym` as is. (This is theoretically, I 
> didn't check in practice.) Or I'm missing smth?
> I choosed `VisitSymExpr` here because all kinds of `SymbolCast` were 
> previously handled here. So I decided to pass all unsupproted forms of casts 
> there.
Did I suggest to `Visit(Sym)`?  Of course it is going to end up in a loop!  
Why isn't it `Visit(Sym->getOperand())` here?  Before we started producing 
casts, casts were transparent.  This logic would fit perfectly with that.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1216
+  if (!T->isIntegralOrEnumerationType())
+return VisitSymExpr(Sym);
+

vsavchenko wrote:
> Same goes here.
And here, since we couldn't really reason about it, we usually return 
`infer(T)`.


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+SymbolRef Sym = Operand;
+while (isa(Sym))
+  Sym = cast(Sym)->Operand;
+return Sym;

ASDenysPetrov wrote:
> vsavchenko wrote:
> > 
> Do you think the recursive call is better than the loop? But, I guess, I see 
> your point. You option could be safer if we had another implementation of the 
> virtual method. Or you think such alike cast symbol is possible in the 
> future? Well, for now `ignoreCasts` doesn't make sense to any other `Expr` 
> successors.
Oh, wait, why is it even virtual?  I don't think that it should be virtual.
Are similar functions in `Expr` virtual?
And I think that this implementation should live in `SymExpr` directly.  Then 
it would look like:
```
if (const SymbolCast *ThisAsCast = dyn_cast(this)) {
  return ThisAsCast->ignoreCasts();
}
return this;
```


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I'll allocate some time to get into your summary, but for now here are my 
concerns about `SymbolRangeInferrer` and `VisitSymbolCast`.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+if (!Opts.ShouldSupportSymbolicIntegerCasts)
+  return VisitSymExpr(Sym);
+

ASDenysPetrov wrote:
> vsavchenko wrote:
> > Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits 
> > or... I'm not sure I fully understand.
> Here we want to delegate the reasoning to another handler as we don't support 
> non-integral cast yet.
You are not delegating it here.  `Visit` includes a runtime dispatch that calls 
a correct `VisitTYPE` method.  Here you call `VisitSymExpr` directly, which is 
one of the `VisitTYPE` methods.  No dispatch will happen, and since we use 
`VisitSymExpr` as the last resort (it's the most base class, if we got there, 
we don't actually support the expression), you interrupt the `Visit` and go 
directly to "the last resort".

See the problem now?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+  // Find the first constraint and exit the loop.
+  RSPtr = getConstraint(State, S);
+}

ASDenysPetrov wrote:
> vsavchenko wrote:
> > Why do you get associated constraint directly without consulting with what 
> > `SymbolRangeInferrer` can tell you about it?
> What do you mean? I didn't get. Could you give an example?
`getConstraint` returns whatever constraint we have stored directly in the 
constraint map.  That's the main source of information for ranges, but not the 
only one.

Here is the  of things that you skip, when you do `getConstraint` here:
  * we can understand that something is equality/disequality check and find the 
corresponding info in Equivalence Classes data structure
  * we can see that the expression has the form `A - B` and we can find 
constraint for `B - A`
  * we can see that the expression is comparison `A op B` and check what other 
comparison info we have on `A` and `B` (your own change)
  * we can see that the expression is of form `A op B` and check if we know 
something about `A` and `B`, and produce a reasonable constraint out of this 
information

In order to use the right information, you should use `infer` that will 
actually do all other things as well.  That's how `SymbolRangeInferrer` is 
designed, to be **recursive**.

Speaking of **recursiveness**.  All these loops and manually checking for types 
of the cast's operand is against this pattern.  Recursive visitors should call 
`Visit` for children nodes (like `RecursiveASTVisitor`).  In other words, if 
`f(x)` is a visit function, it should be defined like this:
```
f(x) = g(f(x->operand_1), f(x->operand_2), ... , f(x->operand_N))
```
or if we talk about your case specifically:
```
f(x: SymbolCast) = h(f(x->Operand))
```
and the `h` function should transform the range set returned by `f(x->Operand)` 
into a range set appropriate for `x`.

NOTE: `h` can also intersect different ranges


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

https://reviews.llvm.org/D103096

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

[PATCH] D105421: [analyzer] Handle << operator for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko accepted this revision.
vsavchenko added a comment.

In D105421#2873458 , @RedDocMD wrote:

> Removed stupid mistakes

No need for self deprecation here!  Thanks for addressing these!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1af97c9d0b02: [analyzer] LoopUnrolling: fix crash when a 
loop counter is captured in a lambda… (authored by AbbasSabra, committed by 
vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

Files:
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/test/Analysis/loop-unrolling.cpp

Index: clang/test/Analysis/loop-unrolling.cpp
===
--- clang/test/Analysis/loop-unrolling.cpp
+++ clang/test/Analysis/loop-unrolling.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++11 -analyzer-config exploration_strategy=unexplored_first_queue %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++11 -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify -std=c++14 -DDFS=1 %s
 
 void clang_analyzer_numTimesReached();
 void clang_analyzer_warnIfReached();
@@ -511,3 +511,39 @@
 clang_analyzer_numTimesReached(); // expected-warning {{4}}
   }
 }
+
+void capture_by_value_as_loop_counter() {
+  int out = 0;
+  auto l = [i = out]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_by_ref_as_loop_counter() {
+  int out = 0;
+  auto l = [ = out]() {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
+
+void capture_implicitly_by_value_as_loop_counter() {
+  int i = 0;
+  auto l = [=]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{10}}
+}
+  };
+}
+
+void capture_implicitly_by_ref_as_loop_counter() {
+  int i = 0;
+  auto l = [&]() mutable {
+for (i = 0; i < 10; ++i) {
+  clang_analyzer_numTimesReached(); // expected-warning {{4}}
+}
+  };
+}
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -79,14 +79,17 @@
   return State;
 }
 
-static internal::Matcher simpleCondition(StringRef BindName) {
-  return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
-  hasOperatorName("<="), hasOperatorName(">="),
-  hasOperatorName("!=")),
-hasEitherOperand(ignoringParenImpCasts(declRefExpr(
-to(varDecl(hasType(isInteger())).bind(BindName),
-hasEitherOperand(ignoringParenImpCasts(
-integerLiteral().bind("boundNum"
+static internal::Matcher simpleCondition(StringRef BindName,
+   StringRef RefName) {
+  return binaryOperator(
+ anyOf(hasOperatorName("<"), hasOperatorName(">"),
+   hasOperatorName("<="), hasOperatorName(">="),
+   hasOperatorName("!=")),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName)))
+ .bind(RefName))),
+ hasEitherOperand(
+ ignoringParenImpCasts(integerLiteral().bind("boundNum"
   .bind("conditionOperator");
 }
 
@@ -138,7 +141,7 @@
 
 static internal::Matcher forLoopMatcher() {
   return forStmt(
- hasCondition(simpleCondition("initVarName")),
+ hasCondition(simpleCondition("initVarName", "initVarRef")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
  hasLoopInit(
  anyOf(declStmt(hasSingleDecl(
@@ -156,17 +159,52 @@
  hasUnaryOperand(declRefExpr(
  to(varDecl(allOf(equalsBoundNode("initVarName"),
   hasType(isInteger(),
- unless(hasBody(hasSuspiciousStmt("initVarName".bind("forLoop");
+ unless(hasBody(hasSuspiciousStmt("initVarName"
+  .bind("forLoop");
 }
 
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
-  // Global variables assumed as escaped variables.
+static bool isCapturedByReference(ExplodedNode *N, const DeclRefExpr *DR) {
+
+  // Get the 

[PATCH] D105421: [analyzer] Handle << operator for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:87
+static bool hasStdClassWithName(const CXXRecordDecl *RD,
+const ArrayRef ) {
+  if (!RD || !RD->getDeclContext()->isStdNamespace())

`ArrayRef` is already a ref (and it says it in its name), so you should remove 
it there.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:103
+static bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  return hasStdClassWithName(RD, {STD_PTR_NAMES, 3});
+}

`ArrayRef` has an implicit constructor from C-arrays, putting size here is 
unnecessary.
And the reason why you were not able to do this is **type mismatch**.  
`STD_PTR_NAMES` is array of `llvm::StringLiteral`, but you chose 
`ArrayRef` as the type of the argument.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:195
 
+const SmallVector BasicOstreamName = {"basic_ostream"};
+

RedDocMD wrote:
> vsavchenko wrote:
> > Same here + don't call it "Name" (singular).  It is a) an array and b) in 
> > the future, we might add more things to it, so we shouldn't need to rename 
> > it everywhere.
> Well, we won't have to rename but we will still have to change the array 
> length everywhere if more names are added.
No, we don't have to do that.  See the reason in the other comment.
Whenever you need to explicitly hardcode size for a stack-allocated array, you 
are doing something wrong.  Stack-allocated everything (VLAs aside) is required 
to have a fixed size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867441 , @ASDenysPetrov 
wrote:

> @vsavchenko
>
>> Why did you write it this way!?
>
> I want the map contains only valid constraints at any time, so we can easely 
> get them without traversing with all variants intersecting with each other. 
> I'm gonna move `updateExistingConstraints ` logic to `VisitSymbolCast`. I 
> think your suggestion can even improve the feature and cover some more cases. 
> I'll add more tests in the next update. Thanks!

`[-10, 10]` is also valid, right?  You can't keep things at their best all the 
time.  And if you want all constraints directly in the map then what's all this 
logic in `VisitSymbolCast`?  That's why I keep asking why do you need both 
parts of this solution and didn't get any answer so far.
I'm hands down for the incremental approach and adding small-to-medium size 
improvements on top of each other.  That makes my life as a reviewer easier :) 
That's said, I don't want to commit to a big solution, where the author doesn't 
want to explain why there are two parts of the solution instead of one.

I want you to tell me why the code that's in `VisitSymbolCast` does what it 
does. And the same about `updateExistingConstraints`.  Also I want to hear a 
solid reason why it's split this way and why we need both of them.

You should understand that I'm not peaking on you personally.  The review 
process takes a lot of my time too.  I want to make it easier for both of us.  
When the reviewer understand what you are going for, it is much easier for them 
to help you in refining your solution.  This patch is very big, but the summary 
doesn't cover the main part: the approach.  And you leave me here dragging it 
out of you.


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

https://reviews.llvm.org/D103096

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

[PATCH] D105692: [analyzer][solver][NFC] Introduce ConstraintAssignor

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1388
+//===--===//
+//New constraint handler
+//===--===//

martong wrote:
> `New` now, but it might be old after a while.
Whoops, this is how I first named the class.  It is not new handler, but new 
constraint.  Because of this ambiguity I actually renamed it 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105692

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867136 , @ASDenysPetrov 
wrote:

> @vsavchenko
>
>> I still want to hear a good explanation why is it done this way.  Here `c` 
>> is mapped to `(char)x`, and we have `[-10, 10]` directly associated with it, 
>> but we also have `(short)x` associated with `[8, 8]`.  Why can't 
>> `VisitSymbolCast` look up constraints for `(short)x` it already looks up for 
>> constraints for different casts already.
>
> Hm, you've confused me. I'll make some debugging and report.

It should not be about debugging, it's your code!  Why did you write it this 
way!?


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867104 , @ASDenysPetrov 
wrote:

> Generally, with this patch we kinda have several constraints for each cast of 
> a single symbol. And we shall care for all of that constraints and timely 
> update them (if possible).
> For instance, we have `int x` and meet casts of this symbol in code:
>
>   int x;
>   (char)x; // we can reason about the 1st byte
>   (short)x; // we can reason about the 2 lowest bytes
>   (ushort)x; // we can reason about the 2 lowest bytes (in this case we may 
> not store for unsigned separately, as we already stored 2 bytes for signed)
>
> That's like we have a knowledge of a lower //part// of the integer. And every 
> time we have a new constraints, for example, for `(short)x;` (aka 2 bytes) 
> then we have to update all the constraints that have two bytes or lower 
> (`(char)x`in this case) as well to make them consistent.

What we do in `Inferrer` is that we try to look at many sources of information 
and `intersect` their ranges.  And I repeat my question again in a bit 
different form, why can't it look up constraints for `(char)x` and for 
`(short)x` and intersect them?
You should admit you never really address this question.  Why can't 
`VisitSymolCast` do everything?


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867021 , @ASDenysPetrov 
wrote:

> In D103096#2866730 , @vsavchenko 
> wrote:
>
>> In D103096#2866704 , 
>> @ASDenysPetrov wrote:
>>
>>> @vsavchenko
>>
>> That's not the question I'm asking.  Why do you need to set constraints for 
>> other symbolic expressions, when `SymbolicInferrer` can look them up on its 
>> own?  Which cases will fail if we remove that part altogether?
>
> I see. Here is what fails in case if we don't update other constraints:
>
>   void test(int x) {
> if ((char)x > -10 && (char)x < 10) {
>   if ((short)x == 8) {
> // If you remove updateExistingConstraints,
> // then `c` won't be 8. It would be [-10, 10] instead.
> char c = x;
> if (c != 8)
>   clang_analyzer_warnIfReached(); // should no-warning, but fail
>   }
> }
>   }

OK, it's something! Good!
I still want to hear a good explanation why is it done this way.  Here `c` is 
mapped to `(char)x`, and we have `[-10, 10]` directly associated with it, but 
we also have `(short)x` associated with `[8, 8]`.  Why can't `VisitSymbolCast` 
look up constraints for `(short)x` it already looks up for constraints for 
different casts already.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+if (!Opts.ShouldSupportSymbolicIntegerCasts)
+  return VisitSymExpr(Sym);
+

Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits or... 
I'm not sure I fully understand.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1215
+  QualType T = RootSym->getType();
+  if (!T->isIntegralOrEnumerationType())
+return VisitSymExpr(Sym);

Can we get a test for that?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1216
+  if (!T->isIntegralOrEnumerationType())
+return VisitSymExpr(Sym);
+

Same goes here.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+  // Find the first constraint and exit the loop.
+  RSPtr = getConstraint(State, S);
+}

Why do you get associated constraint directly without consulting with what 
`SymbolRangeInferrer` can tell you about it?


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

https://reviews.llvm.org/D103096

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

[PATCH] D105693: [analyzer][solver][NFC] Refactor how we detect (dis)equalities

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
ASDenysPetrov, manas, RedDocMD.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch simplifies the way we deal with (dis)equalities.
Due to the symmetry between constraint handler and range inferrer,
we can have very similar implementations of logic handling
questions about (dis)equality and assumptions involving (dis)equality.

It also helps us to remove one more visitor, and removes uncertainty
that we got all the right places to put `trackNE` and `trackEQ`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105693

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/equality_tracking.c

Index: clang/test/Analysis/equality_tracking.c
===
--- clang/test/Analysis/equality_tracking.c
+++ clang/test/Analysis/equality_tracking.c
@@ -219,3 +219,17 @@
   if (c < 0)
 ;
 }
+
+void implyDisequalityFromGT(int a, int b) {
+  if (a > b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
+
+void implyDisequalityFromLT(int a, int b) {
+  if (a < b) {
+clang_analyzer_eval(a == b); // expected-warning{{FALSE}}
+clang_analyzer_eval(a != b); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -688,54 +688,26 @@
 ///
 /// Equality check can have different forms (like a == b or a - b) and this
 /// class encapsulates those away if the only thing the user wants to check -
-/// whether it's equality/diseqiality or not and have an easy access to the
-/// compared symbols.
-struct EqualityInfo {
-public:
-  SymbolRef Left, Right;
-  // true for equality and false for disequality.
-  bool IsEquality = true;
-
-  void invert() { IsEquality = !IsEquality; }
-  /// Extract equality information from the given symbol and the constants.
-  ///
-  /// This function assumes the following expression Sym + Adjustment != Int.
-  /// It is a default because the most widespread case of the equality check
-  /// is (A == B) + 0 != 0.
-  static Optional extract(SymbolRef Sym, const llvm::APSInt ,
-const llvm::APSInt ) {
-// As of now, the only equality form supported is Sym + 0 != 0.
-if (!Int.isNullValue() || !Adjustment.isNullValue())
-  return llvm::None;
-
-return extract(Sym);
-  }
-  /// Extract equality information from the given symbol.
-  static Optional extract(SymbolRef Sym) {
-return EqualityExtractor().Visit(Sym);
+/// whether it's equality/diseqiality or not.
+///
+/// \returns true if assuming this Sym to be true means equality of operands
+///  false if it means disequality of operands
+///  None otherwise
+Optional meansEquality(const SymSymExpr *Sym) {
+  switch (Sym->getOpcode()) {
+  case BO_Sub:
+// This case is: A - B != 0 -> disequality check.
+return false;
+  case BO_EQ:
+// This case is: A == B != 0 -> equality check.
+return true;
+  case BO_NE:
+// This case is: A != B != 0 -> diseqiality check.
+return false;
+  default:
+return llvm::None;
   }
-
-private:
-  class EqualityExtractor
-  : public SymExprVisitor> {
-  public:
-Optional VisitSymSymExpr(const SymSymExpr *Sym) const {
-  switch (Sym->getOpcode()) {
-  case BO_Sub:
-// This case is: A - B != 0 -> disequality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  case BO_EQ:
-// This case is: A == B != 0 -> equality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), true};
-  case BO_NE:
-// This case is: A != B != 0 -> diseqiality check.
-return EqualityInfo{Sym->getLHS(), Sym->getRHS(), false};
-  default:
-return llvm::None;
-  }
-}
-  };
-};
+}
 
 //===--===//
 //Intersection functions
@@ -866,7 +838,13 @@
   }
 
   RangeSet VisitSymSymExpr(const SymSymExpr *Sym) {
-return VisitBinaryOperator(Sym);
+return intersect(
+RangeFactory,
+// If Sym is (dis)equality, we might have some information
+// on that in our equality classes data structure.
+getRangeForEqualities(Sym),
+// And we should always check what we can get from the operands.
+VisitBinaryOperator(Sym));
   }
 
 private:
@@ -907,9 +885,6 @@
 // calculate the effective range set by 

[PATCH] D105692: [analyzer][solver][NFC] Introduce ConstraintAssignor

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
ASDenysPetrov, manas, RedDocMD.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The new component is a symmetric response to SymbolicRangeInferrer.
While the latter is the unified component, which answers all the
questions what does the solver knows about a particular symbolic
expression, assignor associates new constraints (aka "assumes")
with symbolic expressions and can imply additional knowledge that
the solver can extract and use later on.

- Why do we need it and why is SymbolicRangeInferrer not enough?

As it is noted before, the inferrer only helps us to get the most
precise range information based on the existing knowledge and on the
mathematical foundations of different operations that symbolic
expressions actually represent.  It doesn't introduce new constraints.

The assignor, on the other hand, can impose constraints on other
symbols using the same domain knowledge.

- But for some expressions, SymbolicRangeInferrer looks into constraints for 
similar expressions, why can't we do that for all the cases?

That's correct!  But in order to do something like this, we should
have a finite number of possible "similar expressions".

Let's say we are asked about `$a - $b` and we know something about
`$b - $a`.  The inferrer can invert this expression and check
constraints for `$b - $a`.  This is simple!
But let's say we are asked about `$a` and we know that `$a * $b != 0`.
In this situation, we can imply that `$a != 0`, but the inferrer shouldn't
try every possible symbolic expression `X` to check if `$a * X` or
`X * $a` is constrained to non-zero.

With the assignor mechanism, we can catch this implication right at
the moment we associate `$a * $b` with non-zero range, and set similar
constraints for `$a` and `$b` as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105692

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -669,6 +669,17 @@
   return getConstraint(State, EquivalenceClass::find(State, Sym));
 }
 
+LLVM_NODISCARD ProgramStateRef setConstraint(ProgramStateRef State,
+ EquivalenceClass Class,
+ RangeSet Constraint) {
+  return State->set(Class, Constraint);
+}
+
+LLVM_NODISCARD ProgramStateRef setConstraints(ProgramStateRef State,
+  ConstraintRangeTy Constraints) {
+  return State->set(Constraints);
+}
+
 //===--===//
 //   Equality/diseqiality abstraction
 //===--===//
@@ -1373,6 +1384,182 @@
   return {RangeFactory, ValueFactory.getValue(Min), ValueFactory.getValue(Max)};
 }
 
+//===--===//
+//New constraint handler
+//===--===//
+
+/// ConstraintAssignorBase is a small utility class that unifies visitor
+/// for ranges with a visitor for constraints (rangeset/range/constant).
+///
+/// It is designed to have one derived class, but generally it cna have more.
+/// Derived class can control which types we handle by defining methods of the
+/// following form:
+///
+///   bool handle${SYMBOL}To${CONSTRAINT}(const SYMBOL *Sym,
+///   CONSTRAINT Constraint);
+///
+/// where SYMBOL is the type of the symbol (e.g. SymSymExpr, SymbolCast, etc.)
+///   CONSTRAINT is the type of constraint (RangeSet/Range/Const)
+///   return value signifies whether we should try other handle methods
+///  (i.e. false would mean to stop right after calling this method)
+template  class ConstraintAssignorBase {
+public:
+  using Const = const llvm::APSInt &;
+
+#define DISPATCH(CLASS) return assign##CLASS##Impl(cast(Sym), Constraint)
+
+#define ASSIGN(CLASS, TO, SYM, CONSTRAINT) \
+  if (!static_cast(this)->assign##CLASS##To##TO(SYM, CONSTRAINT))   \
+  return false
+
+  void assign(SymbolRef Sym, RangeSet Constraint) {
+assignImpl(Sym, Constraint);
+  }
+
+  bool assignImpl(SymbolRef Sym, RangeSet Constraint) {
+switch (Sym->getKind()) {
+#define SYMBOL(Id, Parent) \
+  case SymExpr::Id##Kind:

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2866704 , @ASDenysPetrov 
wrote:

> @vsavchenko

That's not the question I'm asking.  Why do you need to set constraints for 
other symbolic expressions, when `SymbolicInferrer` can look them up on its 
own?  Which cases will fail if we remove that part altogether?


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Can you please explain why you do the same thing in two different ways?




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+SymbolRef Sym = Operand;
+while (isa(Sym))
+  Sym = cast(Sym)->Operand;
+return Sym;




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

https://reviews.llvm.org/D103096

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

[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Great!  Thanks for addressing all of the comments!




Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+  return FD->getType()->isReferenceType();
+} else {
+  assert(false && "Unknown captured variable");
+}

AbbasSabra wrote:
> vsavchenko wrote:
> > AbbasSabra wrote:
> > > vsavchenko wrote:
> > > > But actually, it's a bit different with this one. I don't know exact 
> > > > details and rules how clang sema fills in the class for lambda.
> > > > According to [[ https://en.cppreference.com/w/cpp/language/lambda | 
> > > > cppreference ]]:
> > > > > For the entities that are captured by reference (with the default 
> > > > > capture [&] or when using the character &, e.g. [, , ]), it is 
> > > > > unspecified if additional data members are declared in the closure 
> > > > > type
> > > > 
> > > > It can be pretty much specified in clang, that's true, but it looks 
> > > > like in `DeadStoreChecker` we have a very similar situation and we do 
> > > > not assume that captured variable always have a corresponding field.
> > > Yes, I based this on the fact that DeadStoreChecker considers it 
> > > possible, but I have never faced a case where it does not have a 
> > > corresponding field.
> > It still would be good to have some proof that it is indeed like this or 
> > simply fallback into returning true (which you already do when in doubt).
> As I said, I believe it cannot happen but I assumed based on the other 
> checker that there is something I don't know.
> I think based on !!getCaptureFields!! the implementation we are iterating 
> over all captures. For that algorithm to work number of captures should be 
> equal to number of fields
> ```
> void CXXRecordDecl::getCaptureFields(
>llvm::DenseMap ,
>FieldDecl *) const {
>   Captures.clear();
>   ThisCapture = nullptr;
> 
>   LambdaDefinitionData  = getLambdaData();
>   RecordDecl::field_iterator Field = field_begin();
>   for (const LambdaCapture *C = Lambda.Captures, *CEnd = C + 
> Lambda.NumCaptures;
>C != CEnd; ++C, ++Field) {
> if (C->capturesThis())
>   ThisCapture = *Field;
> else if (C->capturesVariable())
>   Captures[C->getCapturedVar()] = *Field;
>   }
>   assert(Field == field_end());
> }
> ```
> 
> I dropped the defensive code
OK, sounds reasonable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273

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

[PATCH] D105421: [analyzer] Handle << operator for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Great, thanks for addressing my comments! I still have a couple of minor 
suggestions though.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:99-100
+
+const SmallVector StdPtrNames = {"shared_ptr", "unique_ptr",
+   "weak_ptr"};
+

If it's a compile-time constant, let's make sure it is.
nit: I prefer global scope constants to be CAPITALIZED, so it's easier to spot 
them in the code.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:195
 
+const SmallVector BasicOstreamName = {"basic_ostream"};
+

Same here + don't call it "Name" (singular).  It is a) an array and b) in the 
future, we might add more things to it, so we shouldn't need to rename it 
everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421

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

[PATCH] D105421: [analyzer] Handle << operator for std::unique_ptr

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Good job, great to see how you are going through the whole list of methods!

As for the patch, some tests and COMMENTS will be nice.  Also, I think that for 
a checker that models quite a lot of functions and methods, we need to follow 
the pattern:

  if (isMethodA(...)) {
return handleMethodA(...);
  }
  if (isMethodB(...)) {
return handleMethodB(...);
  }
  ...

however small implementations for those methods are.  It will give fast insight 
into which methods are actually supported.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:86
+bool hasStdClassWithName(const CXXRecordDecl *RD,
+ const SmallVectorImpl ) {
+  if (!RD || !RD->getDeclContext()->isStdNamespace())

When you change those vectors of names to global arrays, we can change it to 
`ArrayRef` to be more idiomatic LLVM code.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:111
+  return hasStdClassWithName(
+  RD, SmallVector{"shared_ptr", "unique_ptr", "weak_ptr"});
+}

This is a compile-time constant, I don't think we should construct it every 
time in runtime.  Global array of three names is totally fine.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:194
+  const auto *RD = E->getType()->getAsCXXRecordDecl();
+  return hasStdClassWithName(RD, SmallVector{"basic_ostream"});
+}

Same goes here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421

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

[PATCH] D105447: [analyzer] Allow cmake options to be passed to satest container

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Awesome, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105447

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

[PATCH] D105552: [analyzer][NFC] NoStoreFuncVisitor: Compact parameters for region pretty printing into a class

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Honestly, I don't really see how this is better.
IMO `Printer` is something that prints, it should be everything that it does.  
It can accept different parameters tweaking how it prints in its constructor, 
but if it is a region printer, you should give it a region, and it should print 
it.  It's not a one-use thing.

Here it looks like you aggregate all the information about the region inside of 
this class, and it knows how to print it.  And it looks like this is actually a 
primary reason here, you want a good way to aggregate all these million 
parameters into one single object.  A better name (and purpose) for abstraction 
like this would be to actually make all these parameters to make sense 
together.  So that the visitor produces them together and then we process them 
together.  And one nice property of such aggregated result would be ability to 
print it.

Another problem that I see is that even for such a small class, I have no idea 
how to use it.  It has too many parameters and it doesn't tell me what is the 
relationship between them.  My guess is that in this form this class will never 
be reused outside of `NoStoreFuncVisitor`, then why do we need it?

At this point, I don't see how this abstraction adds any meaningful layering.  
Meaningful abstraction defines a language for a problem that we are trying to 
solve, so we can use it naturally when we talk.

Sorry about being pretty harsh here.  I do think that `NoStoreFuncVisitor` 
needs refactoring and better abstractions, I just don't think that this is the 
way to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105552

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2801-2802
+  // Get a root symbol in case of SymbolCast.
+  while (isa(Sym))
+Sym = cast(Sym)->getOperand();
+

ASDenysPetrov wrote:
> vsavchenko wrote:
> > This looks like a pattern and we should probably make into a method of 
> > `SymbolCast`
> I did it :) but refused. It will just turn into:
> ```
>   if (isa(Sym))
> Sym = cast(Sym)->getRootOperand();
> ```
> It looks pretty the same and brings no benefit IMO, does it?
> Every time I used `getRootOperand` I also needed some additional traverse 
> through the types te get some another information, so I couldn't avoid the 
> `while` loop there. So I decided not to introduce a new method in 
> `SymbolCast`.
Aha, I see your point.  I guess we can take it into `SymExpr` and call it not 
`getRootOperand`, which won't tell much to a person reading the name, but 
something like `ignoreCasts`.  It will fit well with `Expr::IgnoreCasts`, 
`Expr::IgnoreParens`, etc.


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

https://reviews.llvm.org/D103096

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

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

This is a very complicated patch, I think we'll have to iterate on it quite a 
lot.
Additionally, we have to be sure that this doesn't crash our performance.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1131
 
+class NominalTypeList {
+  CanQualType Types[4];

Comments on:
* why do we need it?
* why does it have four types?
* why do we not care about signed/unsigned types?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2797-2799
+ProgramStateRef
+RangeConstraintManager::updateExistingConstraints(ProgramStateRef State,
+  SymbolRef Sym, RangeSet R) {

OK, but I still don't understand one thing.
Here you go over all "smaller" types and artificially create constraints for 
them, and at the same time in `VisitSymbolCast` you do the opposite operation?  
Why?  Shouldn't the map have constraints for smaller types already because of 
this action?  Why do we need to do both?




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2801-2802
+  // Get a root symbol in case of SymbolCast.
+  while (isa(Sym))
+Sym = cast(Sym)->getOperand();
+

This looks like a pattern and we should probably make into a method of 
`SymbolCast`


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

https://reviews.llvm.org/D103096

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

[PATCH] D105436: [analyzer][solver] Use all sources of constraints

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:908
+// into subexpressions of Sym.
+Visit(Sym));
   }

martong wrote:
> vsavchenko wrote:
> > martong wrote:
> > > Alright. So, this is correct because `Visit` boils down finally to either 
> > > `infer(Sym->getType)` or to `VisitBinaryOperator`. And both of them do a 
> > > correct over-approximation of the ranges. Please confirm. 
> > > 
> > > First, I was a bit concerned b/c it is not immediate and not documented 
> > > here. And it is easy to think by the first look that this might be faulty 
> > > if we take the approximation of one operand of a binop that might not be 
> > > true for the whole binop expression. Again, that is not the case because 
> > > we approximate only in case of such ops where we can do a correct 
> > > over-approximation (i.e. `|`, `&` and `%`). 
> > > 
> > > My point is, I'd like to see more explanatory comments here.
> > I'm sorry, but I don't really understand your point here.
> > 
> > Everything that this solver provides is conservative ranges, from whatever 
> > source it comes.  If you intersect two conservative ranges, you get a 
> > conservative range.
> > It doesn't matter what we do in `Visit` as long as it is correct.  If 
> > `Visit` is incorrect then the previous version of this code that gave 
> > preference to some sources over the other ones was also incorrect.
> Thanks for your reply.  So, with other words, I didn't see why it is 
> immediate that a range for a sub-expression is a good approximation for the 
> whole expression. Maybe it's just me, but that's not obvious until one checks 
> that what exactly happens in `Visit`.
Oh, I mean, it's not correct.  Symbolic expressions are N-ary operators, and if 
we know constraints for at least some of these N operands, we can provide a 
conservative range for the whole symbol using some knowledge of the operator.  
It doesn't say anywhere that we use a range for a sub-expression as an 
approximation for the whole range.

Actually I want to move some of these other sources inside of `Visit` as well 
because they trigger only to very specific kinds of symbolic expressions (e.g. 
binary minus, equality/disequality, comparisons).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105436

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

[PATCH] D105436: [analyzer][solver] Use all sources of constraints

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:908
+// into subexpressions of Sym.
+Visit(Sym));
   }

martong wrote:
> Alright. So, this is correct because `Visit` boils down finally to either 
> `infer(Sym->getType)` or to `VisitBinaryOperator`. And both of them do a 
> correct over-approximation of the ranges. Please confirm. 
> 
> First, I was a bit concerned b/c it is not immediate and not documented here. 
> And it is easy to think by the first look that this might be faulty if we 
> take the approximation of one operand of a binop that might not be true for 
> the whole binop expression. Again, that is not the case because we 
> approximate only in case of such ops where we can do a correct 
> over-approximation (i.e. `|`, `&` and `%`). 
> 
> My point is, I'd like to see more explanatory comments here.
I'm sorry, but I don't really understand your point here.

Everything that this solver provides is conservative ranges, from whatever 
source it comes.  If you intersect two conservative ranges, you get a 
conservative range.
It doesn't matter what we do in `Visit` as long as it is correct.  If `Visit` 
is incorrect then the previous version of this code that gave preference to 
some sources over the other ones was also incorrect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105436

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

[PATCH] D105436: [analyzer][solver] Use all sources of constraints

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6017cb31bb35: [analyzer][solver] Use all sources of 
constraints (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105436

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constant-folding.c


Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -179,6 +179,36 @@
   }
 }
 
+unsigned reset();
+
+void testCombinedSources(unsigned a, unsigned b) {
+  if (b >= 10 && (a | b) <= 30) {
+// Check that we can merge constraints from (a | b), a, and b.
+// Because of the order of assumptions, we already know that (a | b) is 
[10, 30].
+clang_analyzer_eval((a | b) >= 10 && (a | b) <= 30); // 
expected-warning{{TRUE}}
+  }
+
+  a = reset();
+  b = reset();
+
+  if ((a | b) <= 30 && b >= 10) {
+// Check that we can merge constraints from (a | b), a, and b.
+// At this point, we know that (a | b) is [0, 30], but the knowledge
+// of b >= 10 added later can help us to refine it and change it to [10, 
30].
+clang_analyzer_eval(10 <= (a | b) && (a | b) <= 30); // 
expected-warning{{TRUE}}
+  }
+
+  a = reset();
+  b = reset();
+
+  unsigned c = (a | b) & (a != b);
+  if (c <= 40 && a == b) {
+// Even though we have a directo constraint for c [0, 40],
+// we can get a more precise range by looking at the expression itself.
+clang_analyzer_eval(c == 0); // expected-warning{{TRUE}}
+  }
+}
+
 void testRemainderRules(unsigned int a, unsigned int b, int c, int d) {
   // Check that we know that remainder of zero divided by any number is still 
0.
   clang_analyzer_eval((0 % c) == 0); // expected-warning{{TRUE}}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -884,26 +884,28 @@
   }
 
   RangeSet infer(SymbolRef Sym) {
-if (Optional ConstraintBasedRange = intersect(
-RangeFactory, getConstraint(State, Sym),
-// If Sym is a difference of symbols A - B, then maybe we have 
range
-// set stored for B - A.
-//
-// If we have range set stored for both A - B and B - A then
-// calculate the effective range set by intersecting the range set
-// for A - B and the negated range set of B - A.
-getRangeForNegatedSub(Sym), getRangeForEqualities(Sym))) {
-  return *ConstraintBasedRange;
-}
-
-// If Sym is a comparison expression (except <=>),
-// find any other comparisons with the same operands.
-// See function description.
-if (Optional CmpRangeSet = getRangeForComparisonSymbol(Sym)) {
-  return *CmpRangeSet;
-}
-
-return Visit(Sym);
+return intersect(
+RangeFactory,
+// Of course, we should take the constraint directly associated with
+// this symbol into consideration.
+getConstraint(State, Sym),
+// If Sym is a difference of symbols A - B, then maybe we have range
+// set stored for B - A.
+//
+// If we have range set stored for both A - B and B - A then
+// calculate the effective range set by intersecting the range set
+// for A - B and the negated range set of B - A.
+getRangeForNegatedSub(Sym),
+// If Sym is (dis)equality, we might have some information on that
+// in our equality classes data structure.
+getRangeForEqualities(Sym),
+// If Sym is a comparison expression (except <=>),
+// find any other comparisons with the same operands.
+// See function description.
+getRangeForComparisonSymbol(Sym),
+// Apart from the Sym itself, we can infer quite a lot if we look
+// into subexpressions of Sym.
+Visit(Sym));
   }
 
   RangeSet infer(EquivalenceClass Class) {


Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -179,6 +179,36 @@
   }
 }
 
+unsigned reset();
+
+void testCombinedSources(unsigned a, unsigned b) {
+  if (b >= 10 && (a | b) <= 30) {
+// Check that we can merge constraints from (a | b), a, and b.
+// Because of the order of assumptions, we already know that (a | b) is [10, 30].
+clang_analyzer_eval((a | b) >= 10 && (a | b) <= 30); // expected-warning{{TRUE}}
+  }
+
+  a = reset();
+  b = reset();
+
+  if ((a | b) <= 30 && b >= 10) {
+// Check that we can merge constraints from (a | b), a, and b.
+// At this point, we 

[PATCH] D105447: [analyzer] Allow cmake options to be passed to satest container

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/utils/analyzer/entrypoint.py:12
 settings, rest = parse_arguments()
+cmake_opts = list(filter(lambda cmd: cmd[:2]=='-D', rest))
 if settings.wait:

I think we should still use `argparse` for stuff like this, and we don't want 
any of these flags to sneak into `rest`.
https://stackoverflow.com/a/31141568/11582326


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105447

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

[PATCH] D105436: [analyzer][solver] Use all sources of constraints

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I compared issues produced by this patch to the issues produced before that on 
all projects from `clang/utils/analyzer/projects`, and didn't find any 
difference.

Performance measurements also show the we are within the same margins.
F17774659: photo_2021-07-05 19.39.43.jpeg 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105436

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

[PATCH] D105436: [analyzer][solver] Use all sources of constraints

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
ASDenysPetrov, manas, RedDocMD.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Prior to this patch, we always gave priority to constraints that we
actually know about symbols in question.  However, these can get
outdated and we can get better results if we look at all possible
sources of knowledge, including sub-expressions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105436

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constant-folding.c


Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -179,6 +179,36 @@
   }
 }
 
+unsigned reset();
+
+void testCombinedSources(unsigned a, unsigned b) {
+  if (b >= 10 && (a | b) <= 30) {
+// Check that we can merge constraints from (a | b), a, and b.
+// Because of the order of assumptions, we already know that (a | b) is 
[10, 30].
+clang_analyzer_eval((a | b) >= 10 && (a | b) <= 30); // 
expected-warning{{TRUE}}
+  }
+
+  a = reset();
+  b = reset();
+
+  if ((a | b) <= 30 && b >= 10) {
+// Check that we can merge constraints from (a | b), a, and b.
+// At this point, we know that (a | b) is [0, 30], but the knowledge
+// of b >= 10 added later can help us to refine it and change it to [10, 
30].
+clang_analyzer_eval(10 <= (a | b) && (a | b) <= 30); // 
expected-warning{{TRUE}}
+  }
+
+  a = reset();
+  b = reset();
+
+  unsigned c = (a | b) & (a != b);
+  if (c <= 40 && a == b) {
+// Even though we have a directo constraint for c [0, 40],
+// we can get a more precise range by looking at the expression itself.
+clang_analyzer_eval(c == 0); // expected-warning{{TRUE}}
+  }
+}
+
 void testRemainderRules(unsigned int a, unsigned int b, int c, int d) {
   // Check that we know that remainder of zero divided by any number is still 
0.
   clang_analyzer_eval((0 % c) == 0); // expected-warning{{TRUE}}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -884,26 +884,28 @@
   }
 
   RangeSet infer(SymbolRef Sym) {
-if (Optional ConstraintBasedRange = intersect(
-RangeFactory, getConstraint(State, Sym),
-// If Sym is a difference of symbols A - B, then maybe we have 
range
-// set stored for B - A.
-//
-// If we have range set stored for both A - B and B - A then
-// calculate the effective range set by intersecting the range set
-// for A - B and the negated range set of B - A.
-getRangeForNegatedSub(Sym), getRangeForEqualities(Sym))) {
-  return *ConstraintBasedRange;
-}
-
-// If Sym is a comparison expression (except <=>),
-// find any other comparisons with the same operands.
-// See function description.
-if (Optional CmpRangeSet = getRangeForComparisonSymbol(Sym)) {
-  return *CmpRangeSet;
-}
-
-return Visit(Sym);
+return intersect(
+RangeFactory,
+// Of course, we should take the constraint directly associated with
+// this symbol into consideration.
+getConstraint(State, Sym),
+// If Sym is a difference of symbols A - B, then maybe we have range
+// set stored for B - A.
+//
+// If we have range set stored for both A - B and B - A then
+// calculate the effective range set by intersecting the range set
+// for A - B and the negated range set of B - A.
+getRangeForNegatedSub(Sym),
+// If Sym is (dis)equality, we might have some information on that
+// in our equality classes data structure.
+getRangeForEqualities(Sym),
+// If Sym is a comparison expression (except <=>),
+// find any other comparisons with the same operands.
+// See function description.
+getRangeForComparisonSymbol(Sym),
+// Apart from the Sym itself, we can infer quite a lot if we look
+// into subexpressions of Sym.
+Visit(Sym));
   }
 
   RangeSet infer(EquivalenceClass Class) {


Index: clang/test/Analysis/constant-folding.c
===
--- clang/test/Analysis/constant-folding.c
+++ clang/test/Analysis/constant-folding.c
@@ -179,6 +179,36 @@
   }
 }
 
+unsigned reset();
+
+void testCombinedSources(unsigned a, unsigned b) {
+  if (b >= 10 && (a | b) <= 30) {
+// Check that we can merge constraints 

[PATCH] D105273: [analyzer] Introduce range-based reasoning for subtraction operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1485
+  if (ResultType.isUnsigned() ||
+  ((LHS.From() >= 0 && RHS.From() < 0) &&
+   (LHS.To() >= 0 && RHS.To() < 0)) ||

manas wrote:
> @vsavchenko one thing crossed my mind is that, shouldn't I compare `From` and 
> `To` values with  `llvm::APSInt Zero = 
> ValueFactory.getAPSIntType(T).getZeroValue()` instead of literal `0`?
Yes!  Thanks for noticing this (the same goes to the other patch as well)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105273

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

[PATCH] D105340: [analyzer] Produce SymbolCast symbols for integral types in SValBuilder::evalCast

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:318
+ANALYZER_OPTION(bool, ShouldHandleIntegralCastForRanges,
+"handle-integral-cast-for-ranges",
+"Handle truncations, promotions and conversions for ranges of "

BTW, mb it should be less specific?  Something like 
`ShouldSupportSymbolicIntegerCasts`?
BTW 2, do you even plan on supporting symbolic casts in other cases?


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

https://reviews.llvm.org/D105340

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

[PATCH] D105340: [analyzer] Produce SymbolCast symbols for integral types in SValBuilder::evalCast

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Also, even though the test is very extensive it is pretty lopsided at the same 
time.  C-style cast is only one case out of the myriad of all explicit and, 
more importantly, implicit casts.


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

https://reviews.llvm.org/D105340

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

[PATCH] D105273: [analyzer] Introduce range-based reasoning for subtraction operator

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Hey Manas!  Great job, you put this together real quick!




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1490
+  // the resulting range should be [Min, Max].
+  if (ResultType.isUnsigned()) {
+return {RangeFactory, ValueFactory.getValue(Min),

Maybe you can include this as yet another condition in the next `if` statement? 
 Their bodies are identical.



Comment at: clang/test/Analysis/constant-folding.c:399
+// (a - b) = [0, 5] U [UINT_MAX - 9, UINT_MAX]
+clang_analyzer_eval((a - b) != 6); // expected-warning{{TRUE}}
+clang_analyzer_eval((a - b) != UINT_MAX - 10); // expected-warning{{TRUE}}

Maybe you can check `(a - b) > 5 && (a - b) < UINT_MAX - 9` to cover the whole 
range?



Comment at: clang/test/Analysis/constant-folding.c:405
+
+  if (c >= INT_MAX - 5 && d >= INT_MAX - 5) {
+// (c - d) = [-5, 5]

This is also Min and Max overflowing on the positive side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105273

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

[PATCH] D105340: [analyzer] Produce SymbolCast symbols for integral types in SValBuilder::evalCast

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/test/Analysis/produce-symbolcast.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection 
-analyzer-config handle-integral-cast-for-ranges=true -verify %s
+

ASDenysPetrov wrote:
> vsavchenko wrote:
> > This test is failing on my desktop, when I downloaded your patch:
> > ```
> > error: 'warning' diagnostics expected but not seen: 
> >   File .../produce-symbolcast.cpp Line 67: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 79: (long long) ((unsigned long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 122: (unsigned long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 134: (unsigned long long) ((unsigned 
> > long) (reg_$0))
> >   File .../produce-symbolcast.cpp Line 192: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 204: (long long) ((unsigned long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 247: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 259: (unsigned long long) ((unsigned 
> > long) (reg_$0))
> >   File .../produce-symbolcast.cpp Line 317: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 329: (long long) ((unsigned long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 372: (unsigned long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 384: (unsigned long long) ((unsigned 
> > long) (reg_$0))
> >   File .../produce-symbolcast.cpp Line 448: (long long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 454: (long long) ((unsigned long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 492: (unsigned long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 497: (unsigned long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 503: (unsigned long long) 
> > (reg_$0)
> >   File .../produce-symbolcast.cpp Line 509: (unsigned long long) ((unsigned 
> > long) (reg_$0))
> >   File .../produce-symbolcast.cpp Line 562: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 567: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 574: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 579: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 617: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 622: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 629: (unsigned long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 634: (unsigned long long) ((unsigned 
> > long) (reg_$0))
> >   File .../produce-symbolcast.cpp Line 937: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 949: (long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 992: (unsigned long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1004: (unsigned long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1062: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1067: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1074: (long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1078: (long long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1129: (unsigned long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1133: (unsigned long long) 
> > (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1187: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1192: (long) (reg_$0)
> >   File .../produce-symbolcast.cpp Line 1199: (long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1204: (long long) ((unsigned long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1242: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 1247: (unsigned long) (reg_$0 > x>)
> >   File .../produce-symbolcast.cpp Line 1254: (unsigned long long) ((long) 
> > (reg_$0))
> >   File .../produce-symbolcast.cpp Line 1259: (unsigned long long) 
> > ((unsigned long) (reg_$0))
> > error: 'warning' diagnostics seen but not expected: 
> >   File .../produce-symbolcast.cpp Line 67: (long) ((unsigned int) 
> > (reg_$0)) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 79: (long long) (reg_$0) 
> > [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 122: (unsigned long) ((unsigned int) 
> > (reg_$0)) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 134: (unsigned long long) 
> > (reg_$0) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 192: (long) ((unsigned int) 
> > (reg_$0)) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 204: (long long) (reg_$0) 
> > [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 247: (unsigned long) ((unsigned int) 
> > (reg_$0)) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 259: (unsigned long long) 
> > (reg_$0) [debug.ExprInspection]
> >   File .../produce-symbolcast.cpp Line 317: (long) ((unsigned int) 
> > (reg_$0)) [debug.ExprInspection]

[PATCH] D105340: [analyzer] Produce SymbolCast symbols for integral types in SValBuilder::evalCast

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/test/Analysis/range_casts.c:125-126
   if (index - 1UL == 0L) // Was not reached prior fix.
-clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+// Tempopary regression in scope of implementing integral cast.
+// This will be restored as soon as all commits are loaded.
+clang_analyzer_warnIfReached(); // no-warning

The main purpose of the new option is exactly to prevent "temporary 
regressions" so that everything works just as it did before


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105340

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

[PATCH] D105340: [analyzer] Produce SymbolCast symbols for integral types in SValBuilder::evalCast

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Hey, thanks for starting on splitting into more pieces!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:96
QualType OriginalTy);
+  SVal simplifySymbolCast(nonloc::SymbolVal V, QualType CastTy);
 

What does it do and what should I give it?



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:510-546
-// Handles casts of type CK_IntegralCast.
-// At the moment, this function will redirect to evalCast, except when the 
range
-// of the original value is known to be greater than the max of the target 
type.
-SVal SValBuilder::evalIntegralCast(ProgramStateRef state, SVal val,
-   QualType castTy, QualType originalTy) {
-  // No truncations if target type is big enough.
-  if (getContext().getTypeSize(castTy) >= getContext().getTypeSize(originalTy))

I'd like to see the motivation about why this code is removed.
My main concern is this:
  * If removing `evalIntegralCast` is essential for this feature and is not an 
NFC: it should also obey the new analyzer option.
  * If it is NFC, and we can safely remove this function no matter what the 
value of the option is, it should be a separate patch.



Comment at: clang/test/Analysis/bool-assignment.c:46-50
 #ifdef ANALYZER_CM_Z3
 BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}}
 #else
-BOOL x = y; // no-warning
+BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}}
 #endif

If Z3 and not Z3 are the same now, we can simply merge two cases and remove 
preprocessor directive.



Comment at: clang/test/Analysis/produce-symbolcast.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection 
-analyzer-config handle-integral-cast-for-ranges=true -verify %s
+

This test is failing on my desktop, when I downloaded your patch:
```
error: 'warning' diagnostics expected but not seen: 
  File .../produce-symbolcast.cpp Line 67: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 79: (long long) ((unsigned long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 122: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 134: (unsigned long long) ((unsigned 
long) (reg_$0))
  File .../produce-symbolcast.cpp Line 192: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 204: (long long) ((unsigned long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 247: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 259: (unsigned long long) ((unsigned 
long) (reg_$0))
  File .../produce-symbolcast.cpp Line 317: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 329: (long long) ((unsigned long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 372: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 384: (unsigned long long) ((unsigned 
long) (reg_$0))
  File .../produce-symbolcast.cpp Line 448: (long long) (reg_$0)
  File .../produce-symbolcast.cpp Line 454: (long long) ((unsigned long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 492: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 497: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 503: (unsigned long long) (reg_$0)
  File .../produce-symbolcast.cpp Line 509: (unsigned long long) ((unsigned 
long) (reg_$0))
  File .../produce-symbolcast.cpp Line 562: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 567: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 574: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 579: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 617: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 622: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 629: (unsigned long long) ((long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 634: (unsigned long long) ((unsigned 
long) (reg_$0))
  File .../produce-symbolcast.cpp Line 937: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 949: (long long) ((long) (reg_$0))
  File .../produce-symbolcast.cpp Line 992: (unsigned long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1004: (unsigned long long) ((long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 1062: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1067: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1074: (long long) ((long) (reg_$0))
  File .../produce-symbolcast.cpp Line 1078: (long long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1129: (unsigned long long) ((long) 
(reg_$0))
  File .../produce-symbolcast.cpp Line 1133: (unsigned long long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1187: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1192: (long) (reg_$0)
  File .../produce-symbolcast.cpp Line 1199: (long long) ((long) (reg_$0))
  File .../produce-symbolcast.cpp Line 1204: (long long) ((unsigned long) 
(reg_$0))
  File 

[PATCH] D105167: [analyzer] Fix HTML report deduplication.

[Valeriy Savchenko via Phabricator via cfe-commits]

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

This is incredible!  Thanks for addressing it!  I've encountered this many 
times.




Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:277
+
+  std::stringstream filename;
+  filename << "report-";

steakhal wrote:
> I don't frequently see `stringstream`s in the codebase.
> Why don't you use the llvm alternative?
> ```
> std::string s;
> llvm::raw_string_ostream os(s);
> ```
nit: whatever stream you choose, can you please start it with a capital letter? 




Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:286-287
+  // but the stable report filename is still more verbose.
+  // We should rename the option ("verbose" filename?) but it will break
+  // people's workflows.
+  if (DiagOpts.ShouldWriteStableReportFilename) {

Can we make a mirror for this option and mark the other one as deprecated?


Repository:
  rC Clang

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

https://reviews.llvm.org/D105167

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

[PATCH] D104550: [analyzer] Implement getType for SVal

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D104550#2849561 , @DavidSpickett 
wrote:

> @vsavchenko One of the added tests is failing on our 32 bit Armv7 Thumb bot: 
> https://lab.llvm.org/buildbot/#/builders/170/builds/61
>
>   
> /home/tcwg-buildslave/worker/clang-thumbv7-full-2stage/llvm/clang/unittests/StaticAnalyzer/SValTest.cpp:169:
>  Failure
>   Expected equality of these values:
> Context.UnsignedLongTy
>   Which is: unsigned long
> A.getType(Context)
>   Which is: unsigned int
>   [  FAILED  ] SValTest.GetLocAsIntType (22 ms)
>   [--] 1 test from SValTest (22 ms total)
>
> A 32/64 bit issue?

Hi @DavidSpickett , thanks for looking into this!
This patch was almost instantly followed by 
https://github.com/llvm/llvm-project/commit/b2842298cebf420ecb3750bf309021a7f37870c1
 which fixed the issue.  Please, let me know, if you still see it after that 
commit!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104550

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

[PATCH] D105017: [analyzer] LValueToRValueBitCasts should evaluate to an r-value

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko accepted this revision.
vsavchenko added a comment.

Oh, wow, that's awesome! Thanks!


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

https://reviews.llvm.org/D105017

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

[PATCH] D105101: [analyzer] Make CheckerManager::hasPathSensitiveCheckers() complete again

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Great!  Thanks for the detailed explanation in your Summary.
I agree that there should be a runtime check, but it doesn't seem viable as 
long as `Checker.h` is organized the way it is organized now.




Comment at: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:40
+int Unused[]{0, (Result |= !Callbacks.empty())...};
+static_cast(Unused);
+return Result;

Maybe we can use `LLVM_ATTRIBUTE_UNUSED` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105101

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

[PATCH] D104550: [analyzer] Implement getType for SVal

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG159024ce2315: [analyzer] Implement getType for SVal 
(authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104550

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/SValTest.cpp

Index: clang/unittests/StaticAnalyzer/SValTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/SValTest.cpp
@@ -0,0 +1,366 @@
+//===- unittests/StaticAnalyzer/SvalTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+// getType() tests include whole bunch of type comparisons,
+// so when something is wrong, it's good to have gtest telling us
+// what are those types.
+LLVM_ATTRIBUTE_UNUSED std::ostream <<(std::ostream ,
+   const QualType ) {
+  return OS << T.getAsString();
+}
+
+LLVM_ATTRIBUTE_UNUSED std::ostream <<(std::ostream ,
+   const CanQualType ) {
+  return OS << QualType{T};
+}
+
+namespace ento {
+namespace {
+
+//===--===//
+//   Testing framework implementation
+//===--===//
+
+/// A simple map from variable names to symbolic values used to init them.
+using SVals = llvm::StringMap;
+
+/// SValCollector is the barebone of all tests.
+///
+/// It is implemented as a checker and reacts to binds, so we find
+/// symbolic values of interest, and to end analysis, where we actually
+/// can test whatever we gathered.
+class SValCollector : public Checker {
+public:
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext ) const {
+// Skip instantly if we finished testing.
+// Also, we care only for binds happening in variable initializations.
+if (Tested || !isa(S))
+  return;
+
+if (const auto *VR = llvm::dyn_cast_or_null(Loc.getAsRegion())) {
+  CollectedSVals[VR->getDescriptiveName(false)] = Val;
+}
+  }
+
+  void checkEndAnalysis(ExplodedGraph , BugReporter ,
+ExprEngine ) const {
+if (!Tested) {
+  test(Engine, Engine.getContext());
+  Tested = true;
+  CollectedSVals.clear();
+}
+  }
+
+  /// Helper function for tests to access bound symbolic values.
+  SVal getByName(StringRef Name) const { return CollectedSVals[Name]; }
+
+private:
+  /// Entry point for tests.
+  virtual void test(ExprEngine , const ASTContext ) const = 0;
+
+  mutable bool Tested = false;
+  mutable SVals CollectedSVals;
+};
+
+// SVAL_TEST is a combined way of providing a short code snippet and
+// to test some programmatic predicates on symbolic values produced by the
+// engine for the actual code.
+//
+// Each test has a NAME.  One can think of it as a name for normal gtests.
+//
+// Each test should provide a CODE snippet.  Code snippets might contain any
+// valid C/C++, but have ONLY ONE defined function.  There are no requirements
+// about function's name or parameters.  It can even be a class method.  The
+// body of the function must contain a set of variable declarations.  Each
+// variable declaration gets bound to a symbolic value, so for the following
+// example:
+//
+// int x = ;
+//
+// `x` will be bound to whatever symbolic value the engine produced for .
+// LIVENESS and REASSIGNMENTS don't affect this binding.
+//
+// 

[PATCH] D104647: [analyzer] Support SVal::getType for pointer-to-member values

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 355154.
vsavchenko added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104647

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/unittests/StaticAnalyzer/SValTest.cpp

Index: clang/unittests/StaticAnalyzer/SValTest.cpp
===
--- clang/unittests/StaticAnalyzer/SValTest.cpp
+++ clang/unittests/StaticAnalyzer/SValTest.cpp
@@ -361,6 +361,41 @@
   EXPECT_EQ(Context.VoidPtrTy, B.getType(Context));
 }
 
+SVAL_TEST(GetMemberPtrType, R"(
+struct A {
+  int a;
+  struct {
+int b;
+  };
+};
+void foo(int A::*x) {
+  int A::*a = ::a;
+  int A::*b = ::b;
+  int A::*c = x;
+}
+)") {
+  SVal A = getByName("a");
+  ASSERT_FALSE(A.getType(Context).isNull());
+  const auto *AMemberPtrTy = dyn_cast(A.getType(Context));
+  ASSERT_NE(AMemberPtrTy, nullptr);
+  EXPECT_EQ(Context.IntTy, AMemberPtrTy->getPointeeType());
+  const auto *ARecordType = dyn_cast(AMemberPtrTy->getClass());
+  ASSERT_NE(ARecordType, nullptr);
+  EXPECT_EQ("A", ARecordType->getDecl()->getName());
+
+  SVal B = getByName("b");
+  ASSERT_FALSE(B.getType(Context).isNull());
+  const auto *BMemberPtrTy = dyn_cast(B.getType(Context));
+  ASSERT_NE(BMemberPtrTy, nullptr);
+  EXPECT_EQ(Context.IntTy, BMemberPtrTy->getPointeeType());
+  const auto *BRecordType = dyn_cast(BMemberPtrTy->getClass());
+  ASSERT_NE(BRecordType, nullptr);
+  EXPECT_EQ("A", BRecordType->getDecl()->getName());
+
+  SVal C = getByName("c");
+  EXPECT_TRUE(C.isUnknown());
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -180,6 +180,9 @@
   QualType VisitNonLocSymbolVal(nonloc::SymbolVal SV) {
 return Visit(SV.getSymbol());
   }
+  QualType VisitNonLocPointerToMember(nonloc::PointerToMember PTM) {
+return PTM.getType();
+  }
   QualType VisitSymbolicRegion(const SymbolicRegion *SR) {
 return Visit(SR->getSymbol());
   }
@@ -209,21 +212,21 @@
 }
 
 bool nonloc::PointerToMember::isNullMemberPointer() const {
-  return getPTMData().isNull();
+  return getPTMData() == nullptr;
 }
 
 const NamedDecl *nonloc::PointerToMember::getDecl() const {
-  const auto PTMD = this->getPTMData();
-  if (PTMD.isNull())
+  if (!getPTMData())
 return nullptr;
 
-  const NamedDecl *ND = nullptr;
-  if (PTMD.is())
-ND = PTMD.get();
-  else
-ND = PTMD.get()->getDeclaratorDecl();
+  return getPTMData()->getDeclaratorDecl();
+}
 
-  return ND;
+QualType nonloc::PointerToMember::getType() const {
+  if (!getPTMData())
+return {};
+
+  return getPTMData()->getType();
 }
 
 //===--===//
@@ -239,17 +242,11 @@
 }
 
 nonloc::PointerToMember::iterator nonloc::PointerToMember::begin() const {
-  const PTMDataType PTMD = getPTMData();
-  if (PTMD.is())
-return {};
-  return PTMD.get()->begin();
+  return getPTMData()->begin();
 }
 
 nonloc::PointerToMember::iterator nonloc::PointerToMember::end() const {
-  const PTMDataType PTMD = getPTMData();
-  if (PTMD.is())
-return {};
-  return PTMD.get()->end();
+  return getPTMData()->end();
 }
 
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -236,7 +236,7 @@
   return nonloc::SymbolVal(sym);
 }
 
-DefinedSVal SValBuilder::getMemberPointer(const NamedDecl *ND) {
+DefinedSVal SValBuilder::getMemberPointer(const NamedDecl *ND, QualType T) {
   assert(!ND || isa(ND) || isa(ND) ||
  isa(ND));
 
@@ -250,7 +250,8 @@
   return getFunctionPointer(MD);
   }
 
-  return nonloc::PointerToMember(ND);
+  return nonloc::PointerToMember(
+  ND ? getBasicValueFactory().getPointerToMemberData(ND, T) : nullptr);
 }
 
 DefinedSVal SValBuilder::getFunctionPointer(const FunctionDecl *func) {
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -516,7 +516,7 @@
 continue;
   }
   case CK_NullToMemberPointer: {
-SVal V = 

[PATCH] D104550: [analyzer] Implement getType for SVal

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 355152.
vsavchenko added a comment.

Add one more note to `getType` docstring


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104550

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/SValTest.cpp

Index: clang/unittests/StaticAnalyzer/SValTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/SValTest.cpp
@@ -0,0 +1,366 @@
+//===- unittests/StaticAnalyzer/SvalTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+// getType() tests include whole bunch of type comparisons,
+// so when something is wrong, it's good to have gtest telling us
+// what are those types.
+LLVM_ATTRIBUTE_UNUSED std::ostream <<(std::ostream ,
+   const QualType ) {
+  return OS << T.getAsString();
+}
+
+LLVM_ATTRIBUTE_UNUSED std::ostream <<(std::ostream ,
+   const CanQualType ) {
+  return OS << QualType{T};
+}
+
+namespace ento {
+namespace {
+
+//===--===//
+//   Testing framework implementation
+//===--===//
+
+/// A simple map from variable names to symbolic values used to init them.
+using SVals = llvm::StringMap;
+
+/// SValCollector is the barebone of all tests.
+///
+/// It is implemented as a checker and reacts to binds, so we find
+/// symbolic values of interest, and to end analysis, where we actually
+/// can test whatever we gathered.
+class SValCollector : public Checker {
+public:
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext ) const {
+// Skip instantly if we finished testing.
+// Also, we care only for binds happening in variable initializations.
+if (Tested || !isa(S))
+  return;
+
+if (const auto *VR = llvm::dyn_cast_or_null(Loc.getAsRegion())) {
+  CollectedSVals[VR->getDescriptiveName(false)] = Val;
+}
+  }
+
+  void checkEndAnalysis(ExplodedGraph , BugReporter ,
+ExprEngine ) const {
+if (!Tested) {
+  test(Engine, Engine.getContext());
+  Tested = true;
+  CollectedSVals.clear();
+}
+  }
+
+  /// Helper function for tests to access bound symbolic values.
+  SVal getByName(StringRef Name) const { return CollectedSVals[Name]; }
+
+private:
+  /// Entry point for tests.
+  virtual void test(ExprEngine , const ASTContext ) const = 0;
+
+  mutable bool Tested = false;
+  mutable SVals CollectedSVals;
+};
+
+// SVAL_TEST is a combined way of providing a short code snippet and
+// to test some programmatic predicates on symbolic values produced by the
+// engine for the actual code.
+//
+// Each test has a NAME.  One can think of it as a name for normal gtests.
+//
+// Each test should provide a CODE snippet.  Code snippets might contain any
+// valid C/C++, but have ONLY ONE defined function.  There are no requirements
+// about function's name or parameters.  It can even be a class method.  The
+// body of the function must contain a set of variable declarations.  Each
+// variable declaration gets bound to a symbolic value, so for the following
+// example:
+//
+// int x = ;
+//
+// `x` will be bound to whatever symbolic value the engine produced for .
+// LIVENESS and REASSIGNMENTS don't affect this binding.
+//
+// During the test the actual values can be accessed via `getByName` function,
+// and, for the `x`-bound value, 

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103967#2844140 , @thakis wrote:

> Thanks. Looks like it flakily fails on mac too every now and then: 
> http://45.33.8.238/mac/33005/step_7.txt
>
> Maybe you print something in nondeterministic iteration order?

Yes, I think that the problem is deeper.  `ClassMembers` is 
`llvm::ImmutableSet` (i.e. sorted set), and it uses `SymbolRef` (e.g. pointer 
values) for sorting.  So, it does look like the order can be nondeterministic.
However, when we print constraints, we should see similar problems, but we 
don't 樂


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103967

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

[PATCH] D105005: [analyzer][solver][NFC] Simplify function signatures

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8474bb13c327: [analyzer][solver][NFC] Simplify function 
signatures (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105005

Files:
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -549,14 +549,13 @@
  SymbolRef Sym);
 
   /// Merge classes for the given symbols and return a new state.
-  LLVM_NODISCARD static inline ProgramStateRef
-  merge(BasicValueFactory , RangeSet::Factory , ProgramStateRef State,
-SymbolRef First, SymbolRef Second);
+  LLVM_NODISCARD static inline ProgramStateRef merge(RangeSet::Factory ,
+ ProgramStateRef State,
+ SymbolRef First,
+ SymbolRef Second);
   // Merge this class with the given class and return a new state.
-  LLVM_NODISCARD inline ProgramStateRef merge(BasicValueFactory ,
-  RangeSet::Factory ,
-  ProgramStateRef State,
-  EquivalenceClass Other);
+  LLVM_NODISCARD inline ProgramStateRef
+  merge(RangeSet::Factory , ProgramStateRef State, EquivalenceClass Other);
 
   /// Return a set of class members for the given state.
   LLVM_NODISCARD inline SymbolSet getClassMembers(ProgramStateRef State) const;
@@ -567,15 +566,14 @@
  SymbolReaper ) const;
 
   LLVM_NODISCARD static inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, SymbolRef First, SymbolRef Second);
+  markDisequal(RangeSet::Factory , ProgramStateRef State, SymbolRef First,
+   SymbolRef Second);
   LLVM_NODISCARD static inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass First,
-   EquivalenceClass Second);
+  markDisequal(RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass First, EquivalenceClass Second);
   LLVM_NODISCARD inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass Other) const;
+  markDisequal(RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass Other) const;
   LLVM_NODISCARD static inline ClassSet
   getDisequalClasses(ProgramStateRef State, SymbolRef Sym);
   LLVM_NODISCARD inline ClassSet
@@ -641,15 +639,13 @@
   }
   static inline SymbolSet::Factory (ProgramStateRef State);
 
-  inline ProgramStateRef mergeImpl(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, SymbolSet Members,
-   EquivalenceClass Other,
+  inline ProgramStateRef mergeImpl(RangeSet::Factory , ProgramStateRef State,
+   SymbolSet Members, EquivalenceClass Other,
SymbolSet OtherMembers);
   static inline bool
   addToDisequalityInfo(DisequalityMapTy , ConstraintRangeTy ,
-   BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass First,
-   EquivalenceClass Second);
+   RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass First, EquivalenceClass Second);
 
   /// This is a unique identifier of the class.
   uintptr_t ID;
@@ -740,8 +736,7 @@
 //===--===//
 
 template 
-LLVM_NODISCARD inline RangeSet intersect(BasicValueFactory ,
- RangeSet::Factory , RangeSet Head,
+LLVM_NODISCARD inline RangeSet intersect(RangeSet::Factory , RangeSet Head,
  SecondTy Second, RestTy... Tail);
 
 template  struct IntersectionTraits;
@@ -764,15 +759,14 @@
 };
 
 template 
-LLVM_NODISCARD inline EndTy intersect(BasicValueFactory ,
-  RangeSet::Factory , EndTy End) {
+LLVM_NODISCARD inline EndTy intersect(RangeSet::Factory , EndTy End) {
   // If the list contains only RangeSet or Optional, simply return
   // that range set.
   return End;
 }
 
 LLVM_NODISCARD LLVM_ATTRIBUTE_UNUSED inline Optional

[PATCH] D105005: [analyzer][solver][NFC] Simplify function signatures

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko created this revision.
vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, 
ASDenysPetrov, manas, RedDocMD.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Since RangeSet::Factory actually contains BasicValueFactory, we can
remove value factory from many function signatures inside the solver.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105005

Files:
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -549,14 +549,13 @@
  SymbolRef Sym);
 
   /// Merge classes for the given symbols and return a new state.
-  LLVM_NODISCARD static inline ProgramStateRef
-  merge(BasicValueFactory , RangeSet::Factory , ProgramStateRef State,
-SymbolRef First, SymbolRef Second);
+  LLVM_NODISCARD static inline ProgramStateRef merge(RangeSet::Factory ,
+ ProgramStateRef State,
+ SymbolRef First,
+ SymbolRef Second);
   // Merge this class with the given class and return a new state.
-  LLVM_NODISCARD inline ProgramStateRef merge(BasicValueFactory ,
-  RangeSet::Factory ,
-  ProgramStateRef State,
-  EquivalenceClass Other);
+  LLVM_NODISCARD inline ProgramStateRef
+  merge(RangeSet::Factory , ProgramStateRef State, EquivalenceClass Other);
 
   /// Return a set of class members for the given state.
   LLVM_NODISCARD inline SymbolSet getClassMembers(ProgramStateRef State) const;
@@ -567,15 +566,14 @@
  SymbolReaper ) const;
 
   LLVM_NODISCARD static inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, SymbolRef First, SymbolRef Second);
+  markDisequal(RangeSet::Factory , ProgramStateRef State, SymbolRef First,
+   SymbolRef Second);
   LLVM_NODISCARD static inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass First,
-   EquivalenceClass Second);
+  markDisequal(RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass First, EquivalenceClass Second);
   LLVM_NODISCARD inline ProgramStateRef
-  markDisequal(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass Other) const;
+  markDisequal(RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass Other) const;
   LLVM_NODISCARD static inline ClassSet
   getDisequalClasses(ProgramStateRef State, SymbolRef Sym);
   LLVM_NODISCARD inline ClassSet
@@ -636,15 +634,13 @@
   }
   static inline SymbolSet::Factory (ProgramStateRef State);
 
-  inline ProgramStateRef mergeImpl(BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, SymbolSet Members,
-   EquivalenceClass Other,
+  inline ProgramStateRef mergeImpl(RangeSet::Factory , ProgramStateRef State,
+   SymbolSet Members, EquivalenceClass Other,
SymbolSet OtherMembers);
   static inline bool
   addToDisequalityInfo(DisequalityMapTy , ConstraintRangeTy ,
-   BasicValueFactory , RangeSet::Factory ,
-   ProgramStateRef State, EquivalenceClass First,
-   EquivalenceClass Second);
+   RangeSet::Factory , ProgramStateRef State,
+   EquivalenceClass First, EquivalenceClass Second);
 
   /// This is a unique identifier of the class.
   uintptr_t ID;
@@ -735,8 +731,7 @@
 //===--===//
 
 template 
-LLVM_NODISCARD inline RangeSet intersect(BasicValueFactory ,
- RangeSet::Factory , RangeSet Head,
+LLVM_NODISCARD inline RangeSet intersect(RangeSet::Factory , RangeSet Head,
  SecondTy Second, RestTy... Tail);
 
 template  struct IntersectionTraits;
@@ -759,15 +754,14 @@
 };
 
 template 
-LLVM_NODISCARD inline EndTy intersect(BasicValueFactory ,
-  RangeSet::Factory , EndTy End) {
+LLVM_NODISCARD inline EndTy 

[PATCH] D104716: [analyzer] Fix assertion failure on code with transparent unions

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd646157146cc: [analyzer] Fix assertion failure on code with 
transparent unions (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104716

Files:
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/test/Analysis/transparent_union_bug.c

Index: clang/test/Analysis/transparent_union_bug.c
===
--- /dev/null
+++ clang/test/Analysis/transparent_union_bug.c
@@ -0,0 +1,40 @@
+// RUN: %clang_analyze_cc1 -analyze -triple x86_64-apple-darwin10 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_warnIfReached();
+
+typedef struct {
+  int value;
+} Struct;
+
+typedef union {
+  Struct *ptr;
+  long num;
+} __attribute__((transparent_union)) Alias;
+
+void foo(Struct *x);
+void foo(Alias y) {
+  if (y.ptr == 0) {
+// no-crash
+  }
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+void foobar(long z);
+void foobar(Alias z) {
+  if (z.num != 42) {
+// no-crash
+  }
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+
+void foobaz(Alias x) {
+  if (x.ptr == 0) {
+// no-crash
+  }
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+void bar(Struct arg) {
+  foo();
+  foobar(42);
+  foobaz();
+}
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -47,6 +47,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/PointerIntPair.h"
@@ -466,6 +467,42 @@
   llvm_unreachable("unknown callable kind");
 }
 
+static bool isTransparentUnion(QualType T) {
+  const RecordType *UT = T->getAsUnionType();
+  return UT && UT->getDecl()->hasAttr();
+}
+
+// In some cases, symbolic cases should be transformed before we associate
+// them with parameters.  This function incapsulates such cases.
+static SVal processArgument(SVal Value, const Expr *ArgumentExpr,
+const ParmVarDecl *Parameter, SValBuilder ) {
+  QualType ParamType = Parameter->getType();
+  QualType ArgumentType = ArgumentExpr->getType();
+
+  // Transparent unions allow users to easily convert values of union field
+  // types into union-typed objects.
+  //
+  // Also, more importantly, they allow users to define functions with different
+  // different parameter types, substituting types matching transparent union
+  // field types with the union type itself.
+  //
+  // Here, we check specifically for latter cases and prevent binding
+  // field-typed values to union-typed regions.
+  if (isTransparentUnion(ParamType) &&
+  // Let's check that we indeed trying to bind different types.
+  !isTransparentUnion(ArgumentType)) {
+BasicValueFactory  = SVB.getBasicValueFactory();
+
+llvm::ImmutableList CompoundSVals = BVF.getEmptySValList();
+CompoundSVals = BVF.prependSVal(Value, CompoundSVals);
+
+// Wrap it with compound value.
+return SVB.makeCompoundVal(ParamType, CompoundSVals);
+  }
+
+  return Value;
+}
+
 static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
  CallEvent::BindingsTy ,
  SValBuilder ,
@@ -490,10 +527,12 @@
 // determined in compile-time but not represented as arg-expressions,
 // which makes getArgSVal() fail and return UnknownVal.
 SVal ArgVal = Call.getArgSVal(Idx);
+const Expr *ArgExpr = Call.getArgExpr(Idx);
 if (!ArgVal.isUnknown()) {
   Loc ParamLoc = SVB.makeLoc(
   MRMgr.getParamVarRegion(Call.getOriginExpr(), Idx, CalleeCtx));
-  Bindings.push_back(std::make_pair(ParamLoc, ArgVal));
+  Bindings.push_back(
+  std::make_pair(ParamLoc, processArgument(ArgVal, ArgExpr, *I, SVB)));
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Awesome, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103967

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

[PATCH] D104917: [Analyzer] Extend exploded-graph-rewriter to support eq and diseq classes

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

LGTM, but I'm not really an expert in `exploded-graph-rewriter`.  I think @NoQ 
should take a look.




Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:299
+self.equivalence_classes = (
+GenericMap({i: ", ".join(eq_classes[i]) for i in range(0, 
len(eq_classes))})
+if eq_classes

I believe that `enumerate` is more idiomatic for Python in situations like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104917

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