[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1359
+  return cache(S,
+   SVB.evalCast(Op, S->getType(), S->getOperand()->getType()));
+}

steakhal wrote:
> Hoist this common subexpression.
Ok.



Comment at: clang/test/Analysis/svalbuilder-casts.cpp:9-10
+
+// Test that the SValBuilder is able to look up and use a constraint for an
+// operand of a SymbolCast, when the operand is constrained to a const value.
+

steakhal wrote:
> Please ass a test case where these events happen in the execution path:
> 1) Have an unconstrained variable, such as a fn parameter `x`.
> 2) Produce a Symbolic cast of `x`, let's call it `s`.
> 3) Constrain `x` to some value range, let's say it must be positive.
> 4) Verify that the value of the Symbolic cast `s` is also constrained to be 
> positive.
Here is the test you'd like.
```
void testA(int x) {
  short s = x;
  assert(x > 0);
  clang_analyzer_eval(s > 0); // expected-warning{{UNKNOWN}} should be TRUE
}
```
However, this will not pass, because `x` is constrained with a **range**, and 
in the `Simplifier` we do constant folding, i.e. the range must collapse to a 
single value.
This test will pass with the child patch (created by Denys).



Comment at: clang/test/Analysis/svalbuilder-casts.cpp:44-45
+  // which is not truncated to short as zero. Thus the branch is infisible.
+  short s = x;
+  if (!s) {
+if (x == 65537 || x == 131073)

steakhal wrote:
> It took me a while to get that the `short` variable has nothing to do with 
> the test.
> I would recommend extending the previous example with the //short variable// 
> to demonstrate that the same thing (narrowing cast) can occur by either an 
> explicit cast or some implicit casts like a variable 
> initialization/assignment.
Ok, I added a hunk for the implicit cast.



Comment at: clang/test/Analysis/svalbuilder-casts.cpp:46-49
+if (x == 65537 || x == 131073)
+  clang_analyzer_warnIfReached(); // no-warning
+else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}

steakhal wrote:
> ```lang=C++
> clang_analyzer_eval(x == 1);   // expected-warning {{UNKNOWN}}
> clang_analyzer_eval(x == -1);  // expected-warning {{UNKNOWN}}
> clang_analyzer_eval(x == 0);   // expected-warning {{FALSE}}
> clang_analyzer_eval(x == 65537);   // expected-warning {{FALSE}}
> clang_analyzer_eval(x == -65537);  // expected-warning {{FALSE}}
> clang_analyzer_eval(x == 131073);  // expected-warning {{FALSE}}
> clang_analyzer_eval(x == -131073); // expected-warning {{FALSE}}
> ```
Thanks, I've added these cases.

> clang_analyzer_eval(x == 1);   // expected-warning {{UNKNOWN}}
> clang_analyzer_eval(x == -1);  // expected-warning {{UNKNOWN}}

Actually, `1` and `-1` does not cast to `0` as `short`, so these should be 
`FALSE`. Updated like so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126481

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

[PATCH] D126215: [analyzer] Deprecate `-analyzer-store region` flag

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

Still looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126215

___
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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Denys, I've created a very simple patch that makes the SValBuilder to be able 
to look up and use a constraint for an operand of a SymbolCast. That change 
passes 2 of your test cases, thus I made that a parent patch.




Comment at: clang/test/Analysis/symbol-integral-cast.cpp:12-36
+void test1(int x) {
+  // Even if two lower bytes of `x` equal to zero, it doesn't mean that
+  // the entire `x` is zero. We are not able to know the exact value of x.
+  // It can be one of  65536 possible values like [0, 65536, 131072, ...]
+  // and so on. To avoid huge range sets we still assume `x` in the range
+  // [INT_MIN, INT_MAX].
+  if (!(short)x) {

These two tests are redundant because they are handled by the Parent patch I've 
just created. https://reviews.llvm.org/D126481


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] D126481: [analyzer] Handle SymbolCast in SValBuilder

[Gabor Marton via Phabricator via cfe-commits]

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

Make the SimpleSValBuilder to be able to look up and use a constraint
for an operand of a SymbolCast, when the operand is constrained to a
const value.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126481

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/svalbuilder-casts.cpp


Index: clang/test/Analysis/svalbuilder-casts.cpp
===
--- /dev/null
+++ clang/test/Analysis/svalbuilder-casts.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+// Test that the SValBuilder is able to look up and use a constraint for an
+// operand of a SymbolCast, when the operand is constrained to a const value.
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void test(int x) {
+  // Constrain a SymSymExpr to a constant value.
+  assert(x * x == 1);
+  // It is expected to be able to get the constraint for the operand of the
+  // cast.
+  clang_analyzer_eval((char)(x * x) == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval((long)(x * x) == 1); // expected-warning{{TRUE}}
+}
+
+void test1(int x) {
+  // Even if two lower bytes of `x` equal to zero, it doesn't mean that
+  // the entire `x` is zero. We are not able to know the exact value of x.
+  // It can be one of  65536 possible values like [0, 65536, 131072, ...]
+  // and so on. To avoid huge range sets we still assume `x` in the range
+  // [INT_MIN, INT_MAX].
+  if (!(short)x) {
+if (!x)
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  }
+}
+
+void test2(int x) {
+  // If two lower bytes of `x` equal to zero, and we know x to be 65537,
+  // which is not truncated to short as zero. Thus the branch is infisible.
+  short s = x;
+  if (!s) {
+if (x == 65537 || x == 131073)
+  clang_analyzer_warnIfReached(); // no-warning
+else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1278,8 +1278,6 @@
   return SVB.makeSymbolVal(S);
 }
 
-// TODO: Support SymbolCast.
-
 SVal VisitSymIntExpr(const SymIntExpr *S) {
   auto I = Cached.find(S);
   if (I != Cached.end())
@@ -1349,7 +1347,17 @@
   S, SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType()));
 }
 
-// FIXME add VisitSymbolCast
+SVal VisitSymbolCast(const SymbolCast *S) {
+  auto I = Cached.find(S);
+  if (I != Cached.end())
+return I->second;
+  SVal Op = getConstOrVisit(S->getOperand());
+  if (isUnchanged(S->getOperand(), Op))
+return skip(S);
+
+  return cache(S,
+   SVB.evalCast(Op, S->getType(), S->getOperand()->getType()));
+}
 
 SVal VisitUnarySymExpr(const UnarySymExpr *S) {
   auto I = Cached.find(S);


Index: clang/test/Analysis/svalbuilder-casts.cpp
===
--- /dev/null
+++ clang/test/Analysis/svalbuilder-casts.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+// Test that the SValBuilder is able to look up and use a constraint for an
+// operand of a SymbolCast, when the operand is constrained to a const value.
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void test(int x) {
+  // Constrain a SymSymExpr to a constant value.
+  assert(x * x == 1);
+  // It is expected to be able to get the constraint for the operand of the
+  // cast.
+  clang_analyzer_eval((char)(x * x) == 1); // expected-warning{{TRUE}}
+  

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice work, looks promising! Once this 
https://reviews.llvm.org/D125400?vs=431909=431924#inline-1206369 is 
addressed I will accept.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:2273
+// here) may indicate failure. For this reason we do not enforce the errno
+// check (can cause false positive).
 addToFunctionSummaryMap(

You mean the enforced errno check could cause false positive?



Comment at: clang/test/Analysis/errno-stdlibraryfunctions.c:20-24
+void test2() {
+  if (access("path", 0) == -1) {
+if (errno != 0) { }
+  }
+}

steakhal wrote:
> 
+1 for @steakhal's recommendation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D125986: [clang][ASTImporter] Add support for import of UsingPackDecl.

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM! Thanks! And sorry for the delay in the review, please ping me next time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125986

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

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/test/Analysis/unary-sym-expr.c:35
+return;
+  clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
+}

steakhal wrote:
> martong wrote:
> > steakhal wrote:
> > > Does it work if you swap x and y?
> > No, that does not. And the reason of that is we cannot handle commutativity 
> > (yet).
> We could still test it, and put there a FIXME.
Ok, added a fixme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton 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 rGcd5783d3e82b: [analyzer][solver] Handle UnarySymExpr in 
SMTConv (authored by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/test/Analysis/unary-sym-expr-z3-refutation.c
  clang/test/Analysis/z3-crosscheck.c


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded 
from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// REQUIRES: z3
+
+void k(long L) {
+  int g = L;
+  int h = g + 1;
+  int j;
+  j += -h < 0; // should not crash
+  // expected-warning@-1{{garbage}}
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -446,6 +446,28 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+
+  QualType OperandTy;
+  llvm::SMTExprRef OperandExp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , 
hasComparison);
+  llvm::SMTExprRef UnaryExp =
+  fromUnOp(Solver, USE->getOpcode(), OperandExp);
+
+  // Currently, without the `support-symbolic-integer-casts=true` option,
+  // we do not emit `SymbolCast`s for implicit casts.
+  // One such implicit cast is missing if the operand of the unary operator
+  // has a different type than the unary itself.
+  if (Ctx.getTypeSize(OperandTy) != Ctx.getTypeSize(Sym->getType())) {
+if (hasComparison)
+  *hasComparison = false;
+return getCastExpr(Solver, Ctx, UnaryExp, OperandTy, Sym->getType());
+  }
+  return UnaryExp;
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton 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 rG88abc50398eb: [analyzer][solver] Handle UnarySymExpr in 
RangeConstraintSolver (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D125395?vs=429208=432251#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate.c
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/test/Analysis/unary-sym-expr-no-crash.c
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- clang/test/Analysis/unary-sym-expr.c
+++ clang/test/Analysis/unary-sym-expr.c
@@ -36,3 +36,12 @@
   // FIXME Commutativity is not supported yet.
   clang_analyzer_eval(-(y + x) == -3); // expected-warning{{UNKNOWN}}
 }
+
+int test_fp(int flag) {
+  int value;
+  if (flag == 0)
+value = 1;
+  if (-flag == 0)
+return value; // no-warning
+  return 42;
+}
Index: clang/test/Analysis/unary-sym-expr-no-crash.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-no-crash.c
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config support-symbolic-integer-casts=false \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// expected-no-diagnostics
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+void crash(int b, long c) {
+  b = c;
+  if (b > 0)
+if(-b) // should not crash here
+  ;
+}
Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -10,11 +12,8 @@
 #define INT_MAX (UINT_MAX & (UINT_MAX >> 1))
 #define INT_MIN (UINT_MAX & ~(UINT_MAX >> 1))
 
-extern void __assert_fail (__const char *__assertion, __const char *__file,
-unsigned int __line, __const char *__function)
- __attribute__ ((__noreturn__));
-#define assert(expr) \
-  ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
 
 void assert_in_range(int x) {
   assert(x <= ((int)INT_MAX / 4));
@@ -33,69 +32,60 @@
 
 void equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m != n)
-return;
+  assert(m == n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n == m); // expected-warning{{TRUE}}
 }
 
 void non_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m == n)
-return;
+  assert(m != n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n != m); // expected-warning{{TRUE}}
 }
 
 void less_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m < n)
-return;
+  assert(m >= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n <= m); // expected-warning{{TRUE}}
 }
 
 void less(int m, int n) {
   assert_in_range_2(m, n);
-  if (m <= n)
-return;
+  assert(m > n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n < m); // expected-warning{{TRUE}}
 }
 
 void greater_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m > n)
-return;
+  assert(m <= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n >= m); // expected-warning{{TRUE}}
 }
 
 void greater(int m, int n) {
   assert_in_range_2(m, n);
-  if (m >= n)
-return;
+  assert(m < n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n > m); // expected-warning{{TRUE}}
 }
 
 void negate_positive_range(int m, int n) {
-  if (m - n <= 0)
-return;
+  assert(m - n > 0);
   clang_analyzer_eval(n - m < 0); // expected-warning{{TRUE}}
   clang_analyzer_eval(n - m > INT_MIN); // expected-warning{{TRUE}}
-  clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{FALSE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void negate_int_min(int m, int n) {
-  if (m - n != INT_MIN)
-return;
+  assert(m - n == INT_MIN);
   clang_analyzer_eval(n - m == INT_MIN); // 

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
martong marked 2 inline comments as done.
Closed by commit rGb5b2aec1ff51: [analyzer] Add UnarySymExpr (authored by 
martong).

Changed prior to commit:
  https://reviews.llvm.org/D125318?vs=429207=432250#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Symbols.def
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr.c
@@ -0,0 +1,38 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+int test(int x, int y) {
+
+  clang_analyzer_dump(-x);   // expected-warning{{-reg_$0}}
+  clang_analyzer_dump(~x);   // expected-warning{{~reg_$0}}
+  int z = x + y;
+  clang_analyzer_dump(-z);   // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-(x + y)); // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-x + y);   // expected-warning{{(-reg_$0) + (reg_$1)}}
+
+  if (-x == 0) {
+clang_analyzer_eval(-x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(-x > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(-x < 0);  // expected-warning{{FALSE}}
+  }
+  if (~y == 0) {
+clang_analyzer_eval(~y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(~y > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(~y < 0);  // expected-warning{{FALSE}}
+  }
+  (void)(x);
+  return 42;
+}
+
+void test_svalbuilder_simplification(int x, int y) {
+  if (x + y != 3)
+return;
+  clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
+  // FIXME Commutativity is not supported yet.
+  clang_analyzer_eval(-(y + x) == -3); // expected-warning{{UNKNOWN}}
+}
Index: clang/test/Analysis/expr-inspection.cpp
===
--- clang/test/Analysis/expr-inspection.cpp
+++ clang/test/Analysis/expr-inspection.cpp
@@ -18,6 +18,8 @@
   clang_analyzer_express(x); // expected-warning{{Unable to express}}
 
   clang_analyzer_denote(x, "$x");
+  clang_analyzer_express(-x); // expected-warning{{-$x}}
+
   clang_analyzer_denote(y, "$y");
   clang_analyzer_express(x + y); // expected-warning{{$x + $y}}
 
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -72,6 +72,7 @@
 void test_4(int x, int y) {
   int z;
   static int stat;
+  clang_analyzer_explain(-x);// expected-warning-re^\- \(argument 'x'\)$
   clang_analyzer_explain(x + 1); // expected-warning-re^\(argument 'x'\) \+ 1$
   clang_analyzer_explain(1 + y); // expected-warning-re^\(argument 'y'\) \+ 1$
   clang_analyzer_explain(x + y); // expected-warning-re^\(argument 'x'\) \+ \(argument 'y'\)$
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -70,6 +70,16 @@
   os << ')';
 }
 
+void UnarySymExpr::dumpToStream(raw_ostream ) const {
+  os << UnaryOperator::getOpcodeStr(Op);
+  bool Binary = isa(Operand);
+  if (Binary)
+os << '(';
+  Operand->dumpToStream(os);
+  if (Binary)
+os << ')';
+}
+
 void SymbolConjured::dumpToStream(raw_ostream ) const {
   os << getKindStr() << getSymbolID() << '{' << T << ", LC" << LCtx->getID();
   if (S)
@@ -134,6 +144,9 @@
 case SymExpr::SymbolCastKind:
   itr.push_back(cast(SE)->getOperand());
   return;
+case SymExpr::UnarySymExprKind:
+  itr.push_back(cast(SE)->getOperand());
+  return;
 case SymExpr::SymIntExprKind:
   itr.push_back(cast(SE)->getLHS());
   return;
@@ -306,6 +319,22 @@
   return cast(data);
 }
 
+const UnarySymExpr *SymbolManager::getUnarySymExpr(const SymExpr *Operand,
+   UnaryOperator::Opcode Opc,
+   QualType T) 

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:118
+  // Make internal constraint solver entities friends so they can access the
+  // overconstrained related functions. We want to keep this API inaccessible
+  // for Checkers.

steakhal wrote:
> 
Fixed, thx.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126406

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

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rGca3d962548b9: [analyzer] Return from reAssume if State is 
posteriorly overconstrained (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D126406?vs=432225=432245#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126406

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/runtime-regression.c

Index: clang/test/Analysis/runtime-regression.c
===
--- /dev/null
+++ clang/test/Analysis/runtime-regression.c
@@ -0,0 +1,55 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.ArrayBoundV2 \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+// This test is here to check if there is no significant run-time regression
+// related to the assume machinery. The analysis should finish in less than 10
+// seconds.
+
+// expected-no-diagnostics
+
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;
+typedef unsigned long uint64_t;
+
+int filter_slice_word(int sat_linesize, int sigma, int radius, uint64_t *sat,
+  uint64_t *square_sat, int width, int height,
+  int src_linesize, int dst_linesize, const uint16_t *src,
+  uint16_t *dst, int jobnr, int nb_jobs) {
+  const int starty = height * jobnr / nb_jobs;
+  const int endy = height * (jobnr + 1) / nb_jobs;
+
+  for (int y = starty; y < endy; y++) {
+
+int lower_y = y - radius < 0 ? 0 : y - radius;
+int higher_y = y + radius + 1 > height ? height : y + radius + 1;
+int dist_y = higher_y - lower_y;
+
+for (int x = 0; x < width; x++) {
+
+  int lower_x = x - radius < 0 ? 0 : x - radius;
+  int higher_x = x + radius + 1 > width ? width : x + radius + 1;
+  int count = dist_y * (higher_x - lower_x);
+
+  // The below hunk caused significant regression in run-time.
+#if 1
+  uint64_t sum = sat[higher_y * sat_linesize + higher_x] -
+ sat[higher_y * sat_linesize + lower_x] -
+ sat[lower_y * sat_linesize + higher_x] +
+ sat[lower_y * sat_linesize + lower_x];
+  uint64_t square_sum = square_sat[higher_y * sat_linesize + higher_x] -
+square_sat[higher_y * sat_linesize + lower_x] -
+square_sat[lower_y * sat_linesize + higher_x] +
+square_sat[lower_y * sat_linesize + lower_x];
+  uint64_t mean = sum / count;
+  uint64_t var = (square_sum - sum * sum / count) / count;
+  dst[y * dst_linesize + x] =
+  (sigma * mean + var * src[y * src_linesize + x]) / (sigma + var);
+#endif
+
+}
+  }
+  return 0;
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2530,10 +2530,19 @@
   return State;
 }
 
+// We must declare reAssume in clang::ento, otherwise we could not declare that
+// as a friend in ProgramState. More precisely, the call of reAssume would be
+// ambiguous (one in the global namespace and an other which is declared in
+// ProgramState is in clang::ento).
+namespace clang {
+namespace ento {
 // Re-evaluate an SVal with top-level `State->assume` logic.
 LLVM_NODISCARD ProgramStateRef reAssume(ProgramStateRef State,
 const RangeSet *Constraint,
 SVal TheValue) {
+  assert(State);
+  if (State->isPosteriorlyOverconstrained())
+return nullptr;
   if (!Constraint)
 return State;
 
@@ -2556,6 +2565,8 @@
   return State->assumeInclusiveRange(DefinedVal, Constraint->getMinValue(),
  Constraint->getMaxValue(), true);
 }
+} // namespace ento
+} // namespace clang
 
 // Iterate over all symbols and try to simplify them. Once a symbol is
 // simplified then we check if we can merge the simplified symbol's equivalence
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -55,6 +55,8 @@
   }
 };
 
+class RangeSet;
+
 /// \class ProgramState
 /// ProgramState - This class encapsulates:
 ///
@@ -79,7 +81,6 @@
   friend class ExplodedGraph;
   friend class ExplodedNode;
   friend class 

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:127
+
+public:
   bool isPosteriorlyOverconstrained() const {

steakhal wrote:
> This shouldnt be the way.
> Consider fwd declaring and making it friend instead.
> I really dont want to expose this api.
Okay, I've made it a friend.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2547
   if (Constraint->encodesFalseRange())
 return State->assume(DefinedVal, false);
 

steakhal wrote:
> martong wrote:
> > I am wondering, that maybe it would be better to check for 
> > `isPosteriorlyOverconstrained` here. Because only `State->assume` can 
> > return such States, and by checking it here, we could spare some 
> > instructions.
> Play with it.
Yeah, I will, but I don't know how much time that would take to assess. So, in 
the meantime let's proceed with this version.



Comment at: clang/test/Analysis/runtime-regression.c:7
+// This test is here to check if there is no significant run-time regression
+// related to the assume machinery. This is an automatically reduced code. The
+// analysis should finish in less than 10 seconds.

steakhal wrote:
> Sadly, I had to manually track this down xD.
Ok, I removed the sentence.



Comment at: clang/test/Analysis/runtime-regression.c:8
+// related to the assume machinery. This is an automatically reduced code. The
+// analysis should finish in less than 10 seconds.
+

steakhal wrote:
> Maybe the test infra has something to specify a timeout.
I could not find how to set a timeout in the test file. There is a lit cli 
option that might be used, however, that would affect all other test cases. 
https://llvm.org/docs/CommandGuide/lit.html#cmdoption-lit-timeout



Comment at: clang/test/Analysis/runtime-regression.c:12
+
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;

steakhal wrote:
> Pin the tartget triple.
Ok, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126406

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

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 432225.
martong marked 5 inline comments as done.
martong added a comment.

- Make reAssume friend, pin the target in the test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126406

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/runtime-regression.c

Index: clang/test/Analysis/runtime-regression.c
===
--- /dev/null
+++ clang/test/Analysis/runtime-regression.c
@@ -0,0 +1,55 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.ArrayBoundV2 \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify
+
+// This test is here to check if there is no significant run-time regression
+// related to the assume machinery. The analysis should finish in less than 10
+// seconds.
+
+// expected-no-diagnostics
+
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;
+typedef unsigned long uint64_t;
+
+int filter_slice_word(int sat_linesize, int sigma, int radius, uint64_t *sat,
+  uint64_t *square_sat, int width, int height,
+  int src_linesize, int dst_linesize, const uint16_t *src,
+  uint16_t *dst, int jobnr, int nb_jobs) {
+  const int starty = height * jobnr / nb_jobs;
+  const int endy = height * (jobnr + 1) / nb_jobs;
+
+  for (int y = starty; y < endy; y++) {
+
+int lower_y = y - radius < 0 ? 0 : y - radius;
+int higher_y = y + radius + 1 > height ? height : y + radius + 1;
+int dist_y = higher_y - lower_y;
+
+for (int x = 0; x < width; x++) {
+
+  int lower_x = x - radius < 0 ? 0 : x - radius;
+  int higher_x = x + radius + 1 > width ? width : x + radius + 1;
+  int count = dist_y * (higher_x - lower_x);
+
+  // The below hunk caused significant regression in run-time.
+#if 1
+  uint64_t sum = sat[higher_y * sat_linesize + higher_x] -
+ sat[higher_y * sat_linesize + lower_x] -
+ sat[lower_y * sat_linesize + higher_x] +
+ sat[lower_y * sat_linesize + lower_x];
+  uint64_t square_sum = square_sat[higher_y * sat_linesize + higher_x] -
+square_sat[higher_y * sat_linesize + lower_x] -
+square_sat[lower_y * sat_linesize + higher_x] +
+square_sat[lower_y * sat_linesize + lower_x];
+  uint64_t mean = sum / count;
+  uint64_t var = (square_sum - sum * sum / count) / count;
+  dst[y * dst_linesize + x] =
+  (sigma * mean + var * src[y * src_linesize + x]) / (sigma + var);
+#endif
+
+}
+  }
+  return 0;
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2530,10 +2530,19 @@
   return State;
 }
 
+// We must declare reAssume in clang::ento, otherwise we could not declare that
+// as a friend in ProgramState. More precisely, the call of reAssume would be
+// ambiguous (one in the global namespace and an other which is declared in
+// ProgramState is in clang::ento).
+namespace clang {
+namespace ento {
 // Re-evaluate an SVal with top-level `State->assume` logic.
 LLVM_NODISCARD ProgramStateRef reAssume(ProgramStateRef State,
 const RangeSet *Constraint,
 SVal TheValue) {
+  assert(State);
+  if (State->isPosteriorlyOverconstrained())
+return nullptr;
   if (!Constraint)
 return State;
 
@@ -2556,6 +2565,8 @@
   return State->assumeInclusiveRange(DefinedVal, Constraint->getMinValue(),
  Constraint->getMaxValue(), true);
 }
+} // namespace ento
+} // namespace clang
 
 // Iterate over all symbols and try to simplify them. Once a symbol is
 // simplified then we check if we can merge the simplified symbol's equivalence
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -55,6 +55,8 @@
   }
 };
 
+class RangeSet;
+
 /// \class ProgramState
 /// ProgramState - This class encapsulates:
 ///
@@ -79,7 +81,6 @@
   friend class ExplodedGraph;
   friend class ExplodedNode;
   friend class NodeBuilder;
-  friend class ConstraintManager;
 
   ProgramStateManager *stateMgr;
   Environment Env;   // Maps a Stmt to its current SVal.
@@ -113,6 +114,16 @@
   // posteriorly over-constrained. These parents are 

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2547
   if (Constraint->encodesFalseRange())
 return State->assume(DefinedVal, false);
 

I am wondering, that maybe it would be better to check for 
`isPosteriorlyOverconstrained` here. Because only `State->assume` can return 
such States, and by checking it here, we could spare some instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126406

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

[PATCH] D126406: [analyzer] Return from reAssume if State is posteriorly overconstrained

[Gabor Marton via Phabricator via cfe-commits]

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

Depends on D124758 . That patch introduced 
serious regression in the run-time in
some special cases. This fixes that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126406

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/runtime-regression.c


Index: clang/test/Analysis/runtime-regression.c
===
--- /dev/null
+++ clang/test/Analysis/runtime-regression.c
@@ -0,0 +1,54 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.ArrayBoundV2 \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -verify
+
+// This test is here to check if there is no significant run-time regression
+// related to the assume machinery. This is an automatically reduced code. The
+// analysis should finish in less than 10 seconds.
+
+// expected-no-diagnostics
+
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;
+typedef unsigned long uint64_t;
+
+int filter_slice_word(int sat_linesize, int sigma, int radius, uint64_t *sat,
+  uint64_t *square_sat, int width, int height,
+  int src_linesize, int dst_linesize, const uint16_t *src,
+  uint16_t *dst, int jobnr, int nb_jobs) {
+  const int starty = height * jobnr / nb_jobs;
+  const int endy = height * (jobnr + 1) / nb_jobs;
+
+  for (int y = starty; y < endy; y++) {
+
+int lower_y = y - radius < 0 ? 0 : y - radius;
+int higher_y = y + radius + 1 > height ? height : y + radius + 1;
+int dist_y = higher_y - lower_y;
+
+for (int x = 0; x < width; x++) {
+
+  int lower_x = x - radius < 0 ? 0 : x - radius;
+  int higher_x = x + radius + 1 > width ? width : x + radius + 1;
+  int count = dist_y * (higher_x - lower_x);
+
+  // The below hunk caused significant regression in run-time.
+#if 1
+  uint64_t sum = sat[higher_y * sat_linesize + higher_x] -
+ sat[higher_y * sat_linesize + lower_x] -
+ sat[lower_y * sat_linesize + higher_x] +
+ sat[lower_y * sat_linesize + lower_x];
+  uint64_t square_sum = square_sat[higher_y * sat_linesize + higher_x] -
+square_sat[higher_y * sat_linesize + lower_x] -
+square_sat[lower_y * sat_linesize + higher_x] +
+square_sat[lower_y * sat_linesize + lower_x];
+  uint64_t mean = sum / count;
+  uint64_t var = (square_sum - sum * sum / count) / count;
+  dst[y * dst_linesize + x] =
+  (sigma * mean + var * src[y * src_linesize + x]) / (sigma + var);
+#endif
+
+}
+  }
+  return 0;
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2534,6 +2534,9 @@
 LLVM_NODISCARD ProgramStateRef reAssume(ProgramStateRef State,
 const RangeSet *Constraint,
 SVal TheValue) {
+  assert(State);
+  if (State->isPosteriorlyOverconstrained())
+return nullptr;
   if (!Constraint)
 return State;
 
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -123,11 +123,12 @@
   void setStore(const StoreRef );
 
   ProgramStateRef cloneAsPosteriorlyOverconstrained() const;
+
+public:
   bool isPosteriorlyOverconstrained() const {
 return PosteriorlyOverconstrained;
   }
 
-public:
   /// This ctor is used when creating the first ProgramState object.
   ProgramState(ProgramStateManager *mgr, const Environment& env,
   StoreRef st, GenericDataMap gdm);


Index: clang/test/Analysis/runtime-regression.c
===
--- /dev/null
+++ clang/test/Analysis/runtime-regression.c
@@ -0,0 +1,54 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.ArrayBoundV2 \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -verify
+
+// This test is here to check if there is no significant run-time regression
+// related to the 

[PATCH] D124758: [analyzer] Implement assume in terms of assumeDual

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> This change in itself reduced the run-time of the analysis to 16 seconds, on 
> my machine. However, the repetition of States should still be addressed. I am 
> going to upload the upper patch for a starter.

Sorry, in that 16s, I measured also the rebuild and linkage of the Clang 
binary. The time is actually way better, 2.8s, which is quite close to the 
original values we had before this change. So, perhaps it is not even needed to 
bother with the above mentioned cache mechanism.

   time ./bin/clang --analyze -Xclang 
-analyzer-checker=core,alpha.security.ArrayBoundV2,debug.ExprInspection test.c
  test.c:14:3: warning: 1 [debug.ExprInspection]
clang_analyzer_numTimesReached(); // 1 times
^~~~
  test.c:16:5: warning: 253 [debug.ExprInspection]
  clang_analyzer_numTimesReached(); // 285 times
  ^~~~
  test.c:21:5: warning: 805 [debug.ExprInspection]
  clang_analyzer_numTimesReached(); // 1128 times
  ^~~~
  test.c:24:7: warning: 487 [debug.ExprInspection]
clang_analyzer_numTimesReached(); // 560 times
^~~~
  4 warnings generated.
  ./bin/clang --analyze -Xclang  test.c  2.74s user 0.07s system 99% cpu 2.811 
total


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124758

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

[PATCH] D124758: [analyzer] Implement assume in terms of assumeDual

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks Balazs for the report.

Here is my analysis. Looks like during the recursive simplification, `reAssume` 
 produces `State`s that had been created by a previous `reAssume`. Before this 
change, to stop the recursion it was enough to to check if the `OldState` 
equals to the actual `State` in `reAssume`. Now, with this change, each 
`assume` call evaluates both the true and the false branches, thus it is not 
necessary that the subsequent `reAssume` could detect an already "visited" 
State.
So, the obvious solution would be to have a `State` cache in the `reAssume` 
machinery, though, implementation details are not clear yet.

There is another really important thing. We should not continue with `reAssume` 
if the `State` is `posteriorlyOverConstrained`.

   LLVM_NODISCARD ProgramStateRef reAssume(ProgramStateRef State,
   const RangeSet *Constraint,
   SVal TheValue) {
  +  if (State->isPosteriorlyOverconstrained())
  +return nullptr;
 if (!Constraint)
   return State;

This change in itself reduced the run-time of the analysis to 16 seconds, on my 
machine. However, the repetition of States should still be addressed. I am 
going to upload the upper patch for a starter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124758

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 432019.
martong marked an inline comment as done.
martong added a comment.

- Compare the size of the types instead of the type pointers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/test/Analysis/unary-sym-expr-z3-refutation.c
  clang/test/Analysis/z3-crosscheck.c


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded 
from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// REQUIRES: z3
+
+void k(long L) {
+  int g = L;
+  int h = g + 1;
+  int j;
+  j += -h < 0; // should not crash
+  // expected-warning@-1{{garbage}}
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -446,6 +446,28 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+
+  QualType OperandTy;
+  llvm::SMTExprRef OperandExp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , 
hasComparison);
+  llvm::SMTExprRef UnaryExp =
+  fromUnOp(Solver, USE->getOpcode(), OperandExp);
+
+  // Currently, without the `support-symbolic-integer-casts=true` option,
+  // we do not emit `SymbolCast`s for implicit casts.
+  // One such implicit cast is missing if the operand of the unary operator
+  // has a different type than the unary itself.
+  if (Ctx.getTypeSize(OperandTy) != Ctx.getTypeSize(Sym->getType())) {
+if (hasComparison)
+  *hasComparison = false;
+return getCastExpr(Solver, Ctx, UnaryExp, OperandTy, Sym->getType());
+  }
+  return UnaryExp;
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true 

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:463
+  // has a different type than the unary itself.
+  if (OperandTy != Sym->getType()) {
+if (hasComparison)

steakhal wrote:
> Could this fail spuriously due to a constness mismatch, since you compare 
> QualTypes instead of types?
> What happens if one of these refers to typedef? 
> Shouldn't we compare the canonical type pointers instead? Or this works out 
> just fine in practice. I might over-worry this :D
> 
> ALternatively, we could emit this cast only if the bitwidths are different. 
> (which caused the sort difference, thus the crash)
> Could this fail spuriously due to a constness mismatch, since you compare 
> QualTypes instead of types?
No, I don't think so. The underlying `fromCast` would simply return the same 
`SMTExprRef` if the bitwidths are the same.

> What happens if one of these refers to typedef? 
The same as in case of the qualifier mismatch.

> Shouldn't we compare the canonical type pointers instead? Or this works out 
> just fine in practice. I might over-worry this :D
> 
> ALternatively, we could emit this cast only if the bitwidths are different. 
> (which caused the sort difference, thus the crash)
Good point. The code would be more direct and efficient like that, I am going 
to update.




Comment at: clang/test/Analysis/unary-sym-expr-z3-refutation.c:28
+void k(long L) {
+  int g = L;
+  int h = g + 1;

steakhal wrote:
> So this is the implicit cast that we don't emit.
Yes.


Actually, without emitting SymbolCasts: `h` is evaluated as `$L + 1`. 
However, the expression's type is `int`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 431970.
martong added a comment.

- Fix Assertion `*Solver->getSort(LHS) == *Solver->getSort(RHS) && "AST's must 
have the same sort!"' failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/test/Analysis/unary-sym-expr-z3-refutation.c
  clang/test/Analysis/z3-crosscheck.c


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded 
from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config crosscheck-with-z3=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// REQUIRES: z3
+
+void k(long L) {
+  int g = L;
+  int h = g + 1;
+  int j;
+  j += -h < 0; // should not crash
+  // expected-warning@-1{{garbage}}
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -446,6 +446,28 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+
+  QualType OperandTy;
+  llvm::SMTExprRef OperandExp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , 
hasComparison);
+  llvm::SMTExprRef UnaryExp =
+  fromUnOp(Solver, USE->getOpcode(), OperandExp);
+
+  // Currently, without the `support-symbolic-integer-casts=true` option,
+  // we do not emit `SymbolCast`s for implicit casts.
+  // One such implicit cast is missing if the operand of the unary operator
+  // has a different type than the unary itself.
+  if (OperandTy != Sym->getType()) {
+if (hasComparison)
+  *hasComparison = false;
+return getCastExpr(Solver, Ctx, UnaryExp, OperandTy, Sym->getType());
+  }
+  return UnaryExp;
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/test/Analysis/unary-sym-expr-z3-refutation.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-z3-refutation.c
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton 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 rGf75bc5bfc8f8: [analyzer] Fix symbol simplification assertion 
failure (authored by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126281

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-assertion.c


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_warnIfReached();
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2508,12 +2508,6 @@
   SymbolSet ClsMembers = getClassMembers(State);
   assert(ClsMembers.contains(Old));
 
-  // We don't remove `Old`'s Sym->Class relation for two reasons:
-  // 1) This way constraints for the old symbol can still be found via it's
-  // equivalence class that it used to be the member of.
-  // 2) Performance and resource reasons. We can spare one removal and thus one
-  // additional tree in the forest of `ClassMap`.
-
   // Remove `Old`'s Class->Sym relation.
   SymbolSet::Factory  = getMembersFactory(State);
   ClassMembersTy::Factory  = State->get_context();
@@ -2527,6 +2521,12 @@
   ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers);
   State = State->set(ClassMembersMap);
 
+  // Remove `Old`'s Sym->Class relation.
+  ClassMapTy Classes = State->get();
+  ClassMapTy::Factory  = State->get_context();
+  Classes = CMF.remove(Classes, Old);
+  State = State->set(Classes);
+
   return State;
 }
 


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_warnIfReached();
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2508,12 +2508,6 @@
   SymbolSet ClsMembers = getClassMembers(State);
   assert(ClsMembers.contains(Old));
 
-  // We don't remove `Old`'s Sym->Class relation for two reasons:
-  // 1) This way constraints for the old symbol can still be found via it's
-  // equivalence class that it used to be the member of.
-  // 2) Performance and resource reasons. We can spare one removal and thus one
-  // additional tree in the forest of `ClassMap`.
-
   // Remove `Old`'s Class->Sym relation.
   SymbolSet::Factory  = getMembersFactory(State);
   ClassMembersTy::Factory  = State->get_context();
@@ -2527,6 +2521,12 @@
   ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers);
   State = State->set(ClassMembersMap);
 
+  // Remove `Old`'s Sym->Class relation.
+  ClassMapTy Classes = State->get();
+  ClassMapTy::Factory  = State->get_context();
+  Classes = CMF.remove(Classes, Old);
+  State = State->set(Classes);
+
   return State;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/test/Analysis/symbol-simplification-assertion.c:27
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+}

steakhal wrote:
> Is this statement still reachable? Demonstrate it by using the 
> `clang_analyzer_warnIfReached()`.
Ok, I've added that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126281

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

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 431635.
martong marked an inline comment as done.
martong added a comment.

- Add clang_analyzer_warnIfReached() to the test and remove unused debug 
function decls


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126281

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-assertion.c


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_warnIfReached();
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2508,12 +2508,6 @@
   SymbolSet ClsMembers = getClassMembers(State);
   assert(ClsMembers.contains(Old));
 
-  // We don't remove `Old`'s Sym->Class relation for two reasons:
-  // 1) This way constraints for the old symbol can still be found via it's
-  // equivalence class that it used to be the member of.
-  // 2) Performance and resource reasons. We can spare one removal and thus one
-  // additional tree in the forest of `ClassMap`.
-
   // Remove `Old`'s Class->Sym relation.
   SymbolSet::Factory  = getMembersFactory(State);
   ClassMembersTy::Factory  = State->get_context();
@@ -2527,6 +2521,12 @@
   ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers);
   State = State->set(ClassMembersMap);
 
+  // Remove `Old`'s Sym->Class relation.
+  ClassMapTy Classes = State->get();
+  ClassMapTy::Factory  = State->get_context();
+  Classes = CMF.remove(Classes, Old);
+  State = State->set(Classes);
+
   return State;
 }
 


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_warnIfReached();
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2508,12 +2508,6 @@
   SymbolSet ClsMembers = getClassMembers(State);
   assert(ClsMembers.contains(Old));
 
-  // We don't remove `Old`'s Sym->Class relation for two reasons:
-  // 1) This way constraints for the old symbol can still be found via it's
-  // equivalence class that it used to be the member of.
-  // 2) Performance and resource reasons. We can spare one removal and thus one
-  // additional tree in the forest of `ClassMap`.
-
   // Remove `Old`'s Class->Sym relation.
   SymbolSet::Factory  = getMembersFactory(State);
   ClassMembersTy::Factory  = State->get_context();
@@ -2527,6 +2521,12 @@
   ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers);
   State = State->set(ClassMembersMap);
 
+  // Remove `Old`'s Sym->Class relation.
+  ClassMapTy Classes = State->get();
+  ClassMapTy::Factory  = State->get_context();
+  Classes = CMF.remove(Classes, Old);
+  State = State->set(Classes);
+
   return State;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126123: [analyzer][NFC] MemRegion::getRegion() never returns null

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D126123#3531150 , @steakhal wrote:

> In D126123#3531112 , @martong wrote:
>
>> Is it documented with `getRegion`?  Could we decorate that with 
>> `returns-nonnull` 
>> https://clang.llvm.org/docs/AttributeReference.html#returns-nonnull ?
>
> Ah, I thought one of my changes addressing this was already on review.
> It turns out it was not the case. Now I uploaded it for review: D126198 
> 

Ok, could you please change the title of this Differential Revision? Something 
is still not fitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126123

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

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Point 2) above has negligible performance impact, empirical measurements do 
> not show any noticeable difference in the run-time.

Attaching the measurement results that show the run-time difference is not 
noticeable, or rather it is within the margin of error.

F23176609: image.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126281

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

[PATCH] D126215: [analyzer] Deprecate `-analyzer-store region` flag

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126215

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

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/test/Analysis/symbol-simplification-assertion.c:15-18
+void clang_analyzer_printState();
+void clang_analyzer_dump(long);
+void clang_analyzer_eval(int);
+

TODO remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126281

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

[PATCH] D126281: [analyzer] Fix symbol simplification assertion failure

[Gabor Marton via Phabricator via cfe-commits]

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

Fixes https://github.com/llvm/llvm-project/issues/55546

The assertion mentioned in the issue is triggered because an
inconsistency is formed in the Sym->Class and Class->Sym relations. A
simpler but similar inconsistency is demonstrated here:
https://reviews.llvm.org/D114887 .

Previously in `removeMember`, we didn't remove the old symbol's
Sym->Class relation. Back then, we explained it with the following two
bullet points:

> 1. This way constraints for the old symbol can still be found via it's
>
> equivalence class that it used to be the member of.
>
> 2. Performance and resource reasons. We can spare one removal and thus one
>
> additional tree in the forest of `ClassMap`.

This patch do remove the old symbol's Sym->Class relation in order to
keep the Sym->Class relation consistent with the Class->Sym relations.
Point 2) above has negligible performance impact, empirical measurements
do not show any noticeable difference in the run-time. Point 1) above
seems to be a not well justified statement. This is because we cannot
create a new symbol that would be equal to the old symbol after the
simplification had happened. The reason for this is that the SValBuilder
uses the available constant constraints for each sub-symbol.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126281

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-assertion.c


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+// expected-no-diagnostics
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_printState();
+void clang_analyzer_dump(long);
+void clang_analyzer_eval(int);
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + b != c);
+  assert(a == 0); // no-assertion
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -2508,12 +2508,6 @@
   SymbolSet ClsMembers = getClassMembers(State);
   assert(ClsMembers.contains(Old));
 
-  // We don't remove `Old`'s Sym->Class relation for two reasons:
-  // 1) This way constraints for the old symbol can still be found via it's
-  // equivalence class that it used to be the member of.
-  // 2) Performance and resource reasons. We can spare one removal and thus one
-  // additional tree in the forest of `ClassMap`.
-
   // Remove `Old`'s Class->Sym relation.
   SymbolSet::Factory  = getMembersFactory(State);
   ClassMembersTy::Factory  = State->get_context();
@@ -2527,6 +2521,12 @@
   ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers);
   State = State->set(ClassMembersMap);
 
+  // Remove `Old`'s Sym->Class relation.
+  ClassMapTy Classes = State->get();
+  ClassMapTy::Factory  = State->get_context();
+  Classes = CMF.remove(Classes, Old);
+  State = State->set(Classes);
+
   return State;
 }
 


Index: clang/test/Analysis/symbol-simplification-assertion.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-assertion.c
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=true \
+// RUN:   -verify
+
+// Here we test that no assertion is fired during symbol simplification.
+// Related issue: https://github.com/llvm/llvm-project/issues/55546
+
+// expected-no-diagnostics
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void clang_analyzer_printState();
+void clang_analyzer_dump(long);
+void clang_analyzer_eval(int);
+
+int a, b, c;
+long L, L1;
+void test() {
+  assert(a + L + 1 + b != c);
+  assert(L == a);
+  assert(c == 0);
+  L1 = 0;
+  assert(a + L1 + 1 + 

[PATCH] D126215: [analyzer] Deprecate `-analyzer-store region` flag

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> We should support deprecated analyzer flags for at least one release. In this 
> case I'm planning to drop this flag in clang-17

Should we emit a warning for the user about the deprecation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126215

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

[PATCH] D126216: [analyzer][NFC] Remove unused RegionStoreFeatures

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126216

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

[PATCH] D126198: [analyzer][NFCi] Annotate major nonnull returning functions

[Gabor Marton via Phabricator via cfe-commits]

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

Looks good, with minor revisions.




Comment at: clang/include/clang/Analysis/AnalysisDeclContext.h:233
+  : Kind(k), Ctx(ctx), Parent(parent), ID(ID) {
+assert(Ctx);
+  }

To be consistent with the other hunks, where you assert on the parameter, not 
on the member.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:919
 public:
+  // TODO what does this return?
   virtual const ValueDecl *getDecl() const = 0;





Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1021
 assert(!cast(SReg)->getStackFrame()->inTopFrame());
+assert(OriginExpr);
   }





Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1091
+  : DeclRegion(sReg, FieldRegionKind), FD(fd) {
+assert(FD);
+  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126198

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

[PATCH] D126123: [analyzer][NFC] MemRegion::getRegion() never returns null

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Is it documented with `getRegion`?  Could we decorate that with 
`returns-nonnull` 
https://clang.llvm.org/docs/AttributeReference.html#returns-nonnull ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126123

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

[PATCH] D126130: [analyzer][NFC] Remove unused SVal::hasConjuredSymbol

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Remove unused SVal::hasConjuredSymbol

The title seems to be off with the changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126130

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

[PATCH] D126128: [analyzer][NFC] Inline loc::ConcreteInt::evalBinOp

[Gabor Marton via Phabricator via cfe-commits]

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

Looks, good, but it was a struggle to follow if you did the inlining right or 
not. TBH, someone else should also check it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126128

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

[PATCH] D126127: [analyzer][NFC] Relocate unary transfer functions

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> This is an initial step of removing the SimpleSValBuilder abstraction. The 
> SValBuilder alone should be enough.

Perhaps this should be part of another patch stack?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126127

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

[PATCH] D126126: [analyzer][NFC] Inline and simplify nonloc::ConcreteInt functions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:90
-SVal SimpleSValBuilder::evalMinus(NonLoc val) {
-  switch (val.getSubKind()) {
-  case nonloc::ConcreteIntKind:

I'd rather keep the `switch` because in D125318 we are going to extend with 
`nonloc::SymbolValKind` so, soon it will no longer be  a standalone case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126126

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

[PATCH] D125954: [analyzer][NFC] Factor out the copy-paste code repetition of assumeDual and assumeInclusiveRangeDual

[Gabor Marton 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 rG96fba640cf58: [analyzer][NFC] Factor out the copy-paste code 
repetition of assumeDual and… (authored by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125954

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


Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -42,12 +42,14 @@
   return {};
 }
 
+template 
 ConstraintManager::ProgramStatePair
-ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
-  ProgramStateRef StTrue = assumeInternal(State, Cond, true);
+ConstraintManager::assumeDualImpl(ProgramStateRef ,
+  AssumeFunction ) {
+  ProgramStateRef StTrue = Assume(true);
 
   if (!StTrue) {
-ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+ProgramStateRef StFalse = Assume(false);
 if (LLVM_UNLIKELY(!StFalse)) { // both infeasible
   ProgramStateRef StInfeasible = 
State->cloneAsPosteriorlyOverconstrained();
   assert(StInfeasible->isPosteriorlyOverconstrained());
@@ -63,7 +65,7 @@
 return ProgramStatePair(nullptr, StFalse);
   }
 
-  ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+  ProgramStateRef StFalse = Assume(false);
   if (!StFalse) {
 return ProgramStatePair(StTrue, nullptr);
   }
@@ -71,36 +73,22 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInternal(State, Cond, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
+}
+
 ConstraintManager::ProgramStatePair
 ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc 
Value,
 const llvm::APSInt ,
 const llvm::APSInt ) {
-  ProgramStateRef StInRange =
-  assumeInclusiveRangeInternal(State, Value, From, To, true);
-  if (!StInRange) {
-ProgramStateRef StOutOfRange =
-assumeInclusiveRangeInternal(State, Value, From, To, false);
-if (LLVM_UNLIKELY(!StOutOfRange)) { // both infeasible
-  ProgramStateRef StInfeasible = 
State->cloneAsPosteriorlyOverconstrained();
-  assert(StInfeasible->isPosteriorlyOverconstrained());
-  // Checkers might rely on the API contract that both returned states
-  // cannot be null. Thus, we return StInfeasible for both branches because
-  // it might happen that a Checker uncoditionally uses one of them if the
-  // other is a nullptr. This may also happen with the non-dual and
-  // adjacent `assume(true)` and `assume(false)` calls. By implementing
-  // assume in therms of assumeDual, we can keep our API contract there as
-  // well.
-  return ProgramStatePair(StInfeasible, StInfeasible);
-}
-  }
-
-  ProgramStateRef StOutOfRange =
-  assumeInclusiveRangeInternal(State, Value, From, To, false);
-  if (!StOutOfRange) {
-return ProgramStatePair(StInRange, nullptr);
-  }
-
-  return ProgramStatePair(StInRange, StOutOfRange);
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInclusiveRangeInternal(State, Value, From, To, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
 }
 
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -162,6 +162,10 @@
   /// Returns whether or not a symbol is known to be null ("true"), known to be
   /// non-null ("false"), or may be either ("underconstrained").
   virtual ConditionTruthVal checkNull(ProgramStateRef State, SymbolRef Sym);
+
+  template 
+  ProgramStatePair assumeDualImpl(ProgramStateRef ,
+  AssumeFunction );
 };
 
 std::unique_ptr


Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -42,12 +42,14 @@
   return {};
 }
 
+template 
 ConstraintManager::ProgramStatePair
-ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
-  ProgramStateRef StTrue = assumeInternal(State, Cond, true);

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton 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 rG32f189b0d9a8: [analyzer] Implement assumeInclusiveRange in 
terms of assumeInclusiveRangeDual (authored by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125892

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
  clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -91,7 +91,7 @@
   } // end switch
 }
 
-ProgramStateRef SimpleConstraintManager::assumeInclusiveRange(
+ProgramStateRef SimpleConstraintManager::assumeInclusiveRangeInternal(
 ProgramStateRef State, NonLoc Value, const llvm::APSInt ,
 const llvm::APSInt , bool InRange) {
 
Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -71,8 +71,49 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt ) {
+  ProgramStateRef StInRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, true);
+  if (!StInRange) {
+ProgramStateRef StOutOfRange =
+assumeInclusiveRangeInternal(State, Value, From, To, false);
+if (LLVM_UNLIKELY(!StOutOfRange)) { // both infeasible
+  ProgramStateRef StInfeasible = State->cloneAsPosteriorlyOverconstrained();
+  assert(StInfeasible->isPosteriorlyOverconstrained());
+  // Checkers might rely on the API contract that both returned states
+  // cannot be null. Thus, we return StInfeasible for both branches because
+  // it might happen that a Checker uncoditionally uses one of them if the
+  // other is a nullptr. This may also happen with the non-dual and
+  // adjacent `assume(true)` and `assume(false)` calls. By implementing
+  // assume in therms of assumeDual, we can keep our API contract there as
+  // well.
+  return ProgramStatePair(StInfeasible, StInfeasible);
+}
+  }
+
+  ProgramStateRef StOutOfRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, false);
+  if (!StOutOfRange) {
+return ProgramStatePair(StInRange, nullptr);
+  }
+
+  return ProgramStatePair(StInRange, StOutOfRange);
+}
+
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
   DefinedSVal Cond, bool Assumption) {
   ConstraintManager::ProgramStatePair R = assumeDual(State, Cond);
   return Assumption ? R.first : R.second;
 }
+
+ProgramStateRef
+ConstraintManager::assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt , bool InBound) {
+  ConstraintManager::ProgramStatePair R =
+  assumeInclusiveRangeDual(State, Value, From, To);
+  return InBound ? R.first : R.second;
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
@@ -34,16 +34,6 @@
   // Implementation for interface from ConstraintManager.
   //===--===//
 
-  /// Ensures that the DefinedSVal conditional is expressed as a NonLoc by
-  /// creating boolean casts to handle Loc's.
-  ProgramStateRef assumeInternal(ProgramStateRef State, DefinedSVal Cond,
- bool Assumption) override;
-
-  ProgramStateRef assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
-   const llvm::APSInt ,
-   const llvm::APSInt ,
-   bool InRange) override;
-
 protected:
   //===--===//
   // Interface that subclasses must implement.
@@ -74,6 +64,17 @@
   // Internal implementation.
   //===--===//
 
+  /// Ensures that the DefinedSVal conditional is expressed 

[PATCH] D126067: [analyzer] Drop the unused 'analyzer-opt-analyze-nested-blocks' cc1 flag

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Uhh, ohh, don't forget to reflect this change in the ReleaseNotes.rst, so urers 
will be notified!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126067

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

[PATCH] D126067: [analyzer] Drop the unused 'analyzer-opt-analyze-nested-blocks' cc1 flag

[Gabor Marton via Phabricator via cfe-commits]

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

> However, this arises a couple of burning questions.
>
> Should the cc1 frontend still accept this flag - to keep tools/users passing 
> this flag directly to cc1 (which is unsupported, unadvertised) working.
> If we should remain backward compatible, how long?
> How can we get rid of deprecated and obsolete flags at some point?

The answer might be similar to what we do in case of a checker rename (or when 
we move out from alpha). Such a rename have the same problems, but we have not 
made much efforts to combat these problem. Users had to comply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126067

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

[PATCH] D125986: [clang][ASTImporter] Add support for import of UsingPackDecl.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:4861
+  ToUsingPack->setLexicalDeclContext(LexicalDC);
+  LexicalDC->addDeclInternal(ToUsingPack);
+

Why don't we use `addDeclToContexts`?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:925
+  classTemplateSpecializationDecl(hasDescendant(usingPackDecl(;
+}
+

Should we also check if the DeclContext and the Extensions are properly set?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125986

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

There are Z3 refutation related assertions on open-source projects once the 
patch stack is applied. Interestingly it happens in `fromBinOp`.

  clang-14: 
../../git/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:96:
 static const llvm::SMTExpr* 
clang::ento::SMTConv::fromBinOp(llvm::SMTSolverRef&, const llvm::SMTExpr* 
const&, clang::BinaryOperator::Opcode, const llvm::SMTExpr* const&, bool): 
Assertion `*Solver->getSort(LHS) == *Solver->getSort(RHS) && "AST's must have 
the same sort!"' failed.

This is the minimal reproducer:

  int b;
  long c();
  void d(int e, long *f) { *f = e; }
  void k() {
long a = c();
int g = a, h = g - 2, i = -h;
_Bool j;
d(i, );
j &= b == 0;
  }

I'd like to delay the commit of the patch stack until we can fix that, since Z3 
refutation is essential for most of our (internal) users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

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

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D125892#3524453 , @steakhal wrote:

> Now I see, the summary confused me.
>
>> This includes the refactoring of the common assumle*Dual logic into the
>> function template assumeDualImpl.
>
> I felt like this is a refactoring change, but it was not. It's about fixing 
> the behavior by using the `cloneAsPosteriorlyOverconstrained()`.
> Please reword the summary.

Ok, done.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h:42-46
+  ProgramStateRef assumeInclusiveRangeInternal(ProgramStateRef State,
+   NonLoc Value,
+   const llvm::APSInt ,
+   const llvm::APSInt ,
+   bool InRange) override;

steakhal wrote:
> Why is this in the `public:` section, if the name of it suggests its a detail?
Good point. I moved them under `protected:`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125892

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

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 430637.
martong marked an inline comment as done.
martong edited the summary of this revision.
martong added a comment.

- Move SimpleConstraintManager's assume*Internal functions to be protected


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125892

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
  clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -91,7 +91,7 @@
   } // end switch
 }
 
-ProgramStateRef SimpleConstraintManager::assumeInclusiveRange(
+ProgramStateRef SimpleConstraintManager::assumeInclusiveRangeInternal(
 ProgramStateRef State, NonLoc Value, const llvm::APSInt ,
 const llvm::APSInt , bool InRange) {
 
Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -71,8 +71,49 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt ) {
+  ProgramStateRef StInRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, true);
+  if (!StInRange) {
+ProgramStateRef StOutOfRange =
+assumeInclusiveRangeInternal(State, Value, From, To, false);
+if (LLVM_UNLIKELY(!StOutOfRange)) { // both infeasible
+  ProgramStateRef StInfeasible = State->cloneAsPosteriorlyOverconstrained();
+  assert(StInfeasible->isPosteriorlyOverconstrained());
+  // Checkers might rely on the API contract that both returned states
+  // cannot be null. Thus, we return StInfeasible for both branches because
+  // it might happen that a Checker uncoditionally uses one of them if the
+  // other is a nullptr. This may also happen with the non-dual and
+  // adjacent `assume(true)` and `assume(false)` calls. By implementing
+  // assume in therms of assumeDual, we can keep our API contract there as
+  // well.
+  return ProgramStatePair(StInfeasible, StInfeasible);
+}
+  }
+
+  ProgramStateRef StOutOfRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, false);
+  if (!StOutOfRange) {
+return ProgramStatePair(StInRange, nullptr);
+  }
+
+  return ProgramStatePair(StInRange, StOutOfRange);
+}
+
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
   DefinedSVal Cond, bool Assumption) {
   ConstraintManager::ProgramStatePair R = assumeDual(State, Cond);
   return Assumption ? R.first : R.second;
 }
+
+ProgramStateRef
+ConstraintManager::assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt , bool InBound) {
+  ConstraintManager::ProgramStatePair R =
+  assumeInclusiveRangeDual(State, Value, From, To);
+  return InBound ? R.first : R.second;
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
@@ -34,16 +34,6 @@
   // Implementation for interface from ConstraintManager.
   //===--===//
 
-  /// Ensures that the DefinedSVal conditional is expressed as a NonLoc by
-  /// creating boolean casts to handle Loc's.
-  ProgramStateRef assumeInternal(ProgramStateRef State, DefinedSVal Cond,
- bool Assumption) override;
-
-  ProgramStateRef assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
-   const llvm::APSInt ,
-   const llvm::APSInt ,
-   bool InRange) override;
-
 protected:
   //===--===//
   // Interface that subclasses must implement.
@@ -74,6 +64,17 @@
   // Internal implementation.
   //===--===//
 
+  /// Ensures that the DefinedSVal conditional is expressed as a NonLoc by
+  /// creating 

[PATCH] D125920: [analyzer][NFC] Remove the unused LocAsInteger::getPersistentLoc()

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM. Are there any other `getPersistentLoc` function definitions in other 
SVals? Might they be also unused?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125920

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

[PATCH] D125954: [analyzer][NFC] Factor out the copy-paste code repetition of assumeDual and assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

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

Depends on D125892 . There might be 
efficiency and performance
implications by using a lambda. Thus, I am going to conduct measurements
to see if there is any noticeable impact.
I've been thinking about two more alternatives:

1. Make `assumeDualImpl` a variadic template and (perfect) forward the 
arguments for the used `assume` function.
2. Use a macros.

I have concerns though, whether these alternatives would deteriorate the
readability of the code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125954

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


Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -42,12 +42,14 @@
   return {};
 }
 
+template 
 ConstraintManager::ProgramStatePair
-ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
-  ProgramStateRef StTrue = assumeInternal(State, Cond, true);
+ConstraintManager::assumeDualImpl(ProgramStateRef ,
+  AssumeFunction ) {
+  ProgramStateRef StTrue = Assume(true);
 
   if (!StTrue) {
-ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+ProgramStateRef StFalse = Assume(false);
 if (LLVM_UNLIKELY(!StFalse)) { // both infeasible
   ProgramStateRef StInfeasible = 
State->cloneAsPosteriorlyOverconstrained();
   assert(StInfeasible->isPosteriorlyOverconstrained());
@@ -63,7 +65,7 @@
 return ProgramStatePair(nullptr, StFalse);
   }
 
-  ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+  ProgramStateRef StFalse = Assume(false);
   if (!StFalse) {
 return ProgramStatePair(StTrue, nullptr);
   }
@@ -71,36 +73,22 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInternal(State, Cond, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
+}
+
 ConstraintManager::ProgramStatePair
 ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc 
Value,
 const llvm::APSInt ,
 const llvm::APSInt ) {
-  ProgramStateRef StInRange =
-  assumeInclusiveRangeInternal(State, Value, From, To, true);
-  if (!StInRange) {
-ProgramStateRef StOutOfRange =
-assumeInclusiveRangeInternal(State, Value, From, To, false);
-if (LLVM_UNLIKELY(!StOutOfRange)) { // both infeasible
-  ProgramStateRef StInfeasible = 
State->cloneAsPosteriorlyOverconstrained();
-  assert(StInfeasible->isPosteriorlyOverconstrained());
-  // Checkers might rely on the API contract that both returned states
-  // cannot be null. Thus, we return StInfeasible for both branches because
-  // it might happen that a Checker uncoditionally uses one of them if the
-  // other is a nullptr. This may also happen with the non-dual and
-  // adjacent `assume(true)` and `assume(false)` calls. By implementing
-  // assume in therms of assumeDual, we can keep our API contract there as
-  // well.
-  return ProgramStatePair(StInfeasible, StInfeasible);
-}
-  }
-
-  ProgramStateRef StOutOfRange =
-  assumeInclusiveRangeInternal(State, Value, From, To, false);
-  if (!StOutOfRange) {
-return ProgramStatePair(StInRange, nullptr);
-  }
-
-  return ProgramStatePair(StInRange, StOutOfRange);
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInclusiveRangeInternal(State, Value, From, To, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
 }
 
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -162,6 +162,10 @@
   /// Returns whether or not a symbol is known to be null ("true"), known to be
   /// non-null ("false"), or may be either ("underconstrained").
   virtual ConditionTruthVal checkNull(ProgramStateRef State, SymbolRef Sym);
+
+  template 
+  ProgramStatePair assumeDualImpl(ProgramStateRef ,
+  

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 430611.
martong added a comment.

- Split into two patches, this first patch will be just the no-brainer 
copy-paste from assumeDual implementaton.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125892

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
  clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -91,7 +91,7 @@
   } // end switch
 }
 
-ProgramStateRef SimpleConstraintManager::assumeInclusiveRange(
+ProgramStateRef SimpleConstraintManager::assumeInclusiveRangeInternal(
 ProgramStateRef State, NonLoc Value, const llvm::APSInt ,
 const llvm::APSInt , bool InRange) {
 
Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -71,8 +71,49 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt ) {
+  ProgramStateRef StInRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, true);
+  if (!StInRange) {
+ProgramStateRef StOutOfRange =
+assumeInclusiveRangeInternal(State, Value, From, To, false);
+if (LLVM_UNLIKELY(!StOutOfRange)) { // both infeasible
+  ProgramStateRef StInfeasible = State->cloneAsPosteriorlyOverconstrained();
+  assert(StInfeasible->isPosteriorlyOverconstrained());
+  // Checkers might rely on the API contract that both returned states
+  // cannot be null. Thus, we return StInfeasible for both branches because
+  // it might happen that a Checker uncoditionally uses one of them if the
+  // other is a nullptr. This may also happen with the non-dual and
+  // adjacent `assume(true)` and `assume(false)` calls. By implementing
+  // assume in therms of assumeDual, we can keep our API contract there as
+  // well.
+  return ProgramStatePair(StInfeasible, StInfeasible);
+}
+  }
+
+  ProgramStateRef StOutOfRange =
+  assumeInclusiveRangeInternal(State, Value, From, To, false);
+  if (!StOutOfRange) {
+return ProgramStatePair(StInRange, nullptr);
+  }
+
+  return ProgramStatePair(StInRange, StOutOfRange);
+}
+
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
   DefinedSVal Cond, bool Assumption) {
   ConstraintManager::ProgramStatePair R = assumeDual(State, Cond);
   return Assumption ? R.first : R.second;
 }
+
+ProgramStateRef
+ConstraintManager::assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt , bool InBound) {
+  ConstraintManager::ProgramStatePair R =
+  assumeInclusiveRangeDual(State, Value, From, To);
+  return InBound ? R.first : R.second;
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
@@ -39,10 +39,11 @@
   ProgramStateRef assumeInternal(ProgramStateRef State, DefinedSVal Cond,
  bool Assumption) override;
 
-  ProgramStateRef assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
-   const llvm::APSInt ,
-   const llvm::APSInt ,
-   bool InRange) override;
+  ProgramStateRef assumeInclusiveRangeInternal(ProgramStateRef State,
+   NonLoc Value,
+   const llvm::APSInt ,
+   const llvm::APSInt ,
+   bool InRange) override;
 
 protected:
   //===--===//
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ 

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D125892#3523296 , @steakhal wrote:

> Okay, it took me a while to get what's going on.
> Do you have anything in mind to express it by slightly less indirection, 
> references, templates and well, virtual functions xD

Okay, I see your point. Therefore, I am splitting up this patch into two:

1. The no-brainer copy-paste from `assumeDual` implementaton.
2. An NFC that tries to eliminate the copy paste coding. Efficiency and 
performance discussions should go there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125892

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

[PATCH] D125871: [analyzer] Delete alpha.deadcode.UnreachableCode checker

[Gabor Marton via Phabricator via cfe-commits]

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

In D125871#3521967 , @steakhal wrote:

> Could you please give a few examples of these FPs for the record?

Out of thin air I could come up with the following one below. Seems like `try` 
is not handled, but it is not attached to the fact of being non full-path. 
Maybe I was in too much rush, perhaps we should leave this checker here.
F23106602: image.png 

Besides, there is an annoying true positive which should be suppressed IMHO.
F23106640: image.png 




Comment at: clang/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp:53-54
-
-  if (Eng.hasWorkRemaining())
-return;
-

steakhal wrote:
> I thought this check guards this checker to be meaningful. @martong
To be honest, I missed this. So, at least the checker does not report, when the 
budge is out, i.e. when we definitely know for sure that we could not explore 
the whole graph. 

On the other hand, even if there is no work remaining for the engine, we still 
cannot be sure that all theoretical program paths have been covered can we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125871

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

[PATCH] D125892: [analyzer] Implement assumeInclusiveRange in terms of assumeInclusiveRangeDual

[Gabor Marton via Phabricator via cfe-commits]

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

This includes the refactoring of the common assumle*Dual logic into the
function template `assumeDualImpl`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125892

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
  clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -91,7 +91,7 @@
   } // end switch
 }
 
-ProgramStateRef SimpleConstraintManager::assumeInclusiveRange(
+ProgramStateRef SimpleConstraintManager::assumeInclusiveRangeInternal(
 ProgramStateRef State, NonLoc Value, const llvm::APSInt ,
 const llvm::APSInt , bool InRange) {
 
Index: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp
@@ -42,12 +42,14 @@
   return {};
 }
 
+template 
 ConstraintManager::ProgramStatePair
-ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
-  ProgramStateRef StTrue = assumeInternal(State, Cond, true);
+ConstraintManager::assumeDualImpl(ProgramStateRef ,
+  AssumeFunction ) {
+  ProgramStateRef StTrue = Assume(true);
 
   if (!StTrue) {
-ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+ProgramStateRef StFalse = Assume(false);
 if (LLVM_UNLIKELY(!StFalse)) { // both infeasible
   ProgramStateRef StInfeasible = State->cloneAsPosteriorlyOverconstrained();
   assert(StInfeasible->isPosteriorlyOverconstrained());
@@ -63,7 +65,7 @@
 return ProgramStatePair(nullptr, StFalse);
   }
 
-  ProgramStateRef StFalse = assumeInternal(State, Cond, false);
+  ProgramStateRef StFalse = Assume(false);
   if (!StFalse) {
 return ProgramStatePair(StTrue, nullptr);
   }
@@ -71,8 +73,35 @@
   return ProgramStatePair(StTrue, StFalse);
 }
 
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInternal(State, Cond, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
+}
+
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeInclusiveRangeDual(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt ) {
+  auto AssumeFun = [&](bool Assumption) {
+return assumeInclusiveRangeInternal(State, Value, From, To, Assumption);
+  };
+  return assumeDualImpl(State, AssumeFun);
+}
+
 ProgramStateRef ConstraintManager::assume(ProgramStateRef State,
   DefinedSVal Cond, bool Assumption) {
   ConstraintManager::ProgramStatePair R = assumeDual(State, Cond);
   return Assumption ? R.first : R.second;
 }
+
+ProgramStateRef
+ConstraintManager::assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
+const llvm::APSInt ,
+const llvm::APSInt , bool InBound) {
+  ConstraintManager::ProgramStatePair R =
+  assumeInclusiveRangeDual(State, Value, From, To);
+  return InBound ? R.first : R.second;
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
@@ -39,10 +39,11 @@
   ProgramStateRef assumeInternal(ProgramStateRef State, DefinedSVal Cond,
  bool Assumption) override;
 
-  ProgramStateRef assumeInclusiveRange(ProgramStateRef State, NonLoc Value,
-   const llvm::APSInt ,
-   const llvm::APSInt ,
-   bool InRange) override;
+  ProgramStateRef assumeInclusiveRangeInternal(ProgramStateRef State,
+   NonLoc Value,
+   const llvm::APSInt ,
+  

[PATCH] D125871: [analyzer] Delete alpha.deadcode.UnreachableCode checker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:847
 
-let ParentPackage = DeadCodeAlpha in {
-

TODO, update the ReleseNotes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125871

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

[PATCH] D125871: [analyzer] Delete alpha.deadcode.UnreachableCode checker

[Gabor Marton via Phabricator via cfe-commits]

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

This checker is fundamentally flawed, because this problem requires
all-path data-flow analysis.
This checker relies on the paths that have been visited during the
exploration of the exploded graph. There are no guarantees that the
symbolic execution will explore all paths. What's more, in practice,
most of the time it does not. Thus, we see way too many annoying false
positives.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125871

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  clang/test/Analysis/malloc-annotations.c
  clang/test/Analysis/malloc-annotations.cpp
  clang/test/Analysis/malloc.c
  clang/test/Analysis/malloc.cpp
  clang/test/Analysis/qt_malloc.cpp
  clang/test/Analysis/unreachable-code-path.c
  clang/www/analyzer/alpha_checks.html

Index: clang/www/analyzer/alpha_checks.html
===
--- clang/www/analyzer/alpha_checks.html
+++ clang/www/analyzer/alpha_checks.html
@@ -447,48 +447,6 @@
 
 
 
-
-
-Dead Code Alpha Checkers
-
-
-Name, DescriptionExample
-
-
-
-alpha.deadcode.UnreachableCode
-(C, C++, ObjC)
-Check unreachable code.
-
-
-// C
-int test() {
-  int x = 1;
-  while(x);
-  return x; // warn
-}
-
-
-// C++
-void test() {
-  int a = 2;
-
-  while (a > 1)
-a--;
-
-  if (a > 1)
-a++; // warn
-}
-
-
-// Objective-C
-void test(id x) {
-  return;
-  [x retain]; // warn
-}
-
-
-
 
 LLVM Checkers
 
Index: clang/test/Analysis/unreachable-code-path.c
===
--- clang/test/Analysis/unreachable-code-path.c
+++ /dev/null
@@ -1,226 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,deadcode.DeadStores,alpha.deadcode.UnreachableCode -verify -analyzer-opt-analyze-nested-blocks -Wno-unused-value %s
-
-extern void foo(int a);
-
-// The first few tests are non-path specific - we should be able to find them
-
-void test(unsigned a) {
-  switch (a) {
-a += 5; // expected-warning{{never executed}}
-  case 2:
-a *= 10;
-  case 3:
-a %= 2;
-  }
-  foo(a);
-}
-
-void test2(unsigned a) {
- help:
-  if (a > 0)
-return;
-  if (a == 0)
-return;
-  foo(a); // expected-warning{{never executed}}
-  goto help;
-}
-
-void test3(unsigned a) {
-  while(1);
-  if (a > 5) { // expected-warning{{never executed}}
-return;
-  }
-}
-
-// These next tests are path-sensitive
-
-void test4(void) {
-  int a = 5;
-
-  while (a > 1)
-a -= 2;
-
-  if (a > 1) {
-a = a + 56; // expected-warning{{never executed}}
-  }
-
-  foo(a);
-}
-
-extern void bar(char c);
-
-void test5(const char *c) {
-  foo(c[0]);
-
-  if (!c) {
-bar(1); // expected-warning{{never executed}}
-  }
-}
-
-// These next tests are false positives and should not generate warnings
-
-void test6(const char *c) {
-  if (c) return;
-  if (!c) return;
-  __builtin_unreachable(); // no-warning
-  __builtin_assume(0); // no-warning
-}
-
-// Compile-time constant false positives
-#define CONSTANT 0
-enum test_enum { Off, On };
-void test7(void) {
-  if (CONSTANT)
-return; // no-warning
-
-  if (sizeof(int))
-return; // no-warning
-
-  if (Off)
-return; // no-warning
-}
-
-void test8(void) {
-  static unsigned a = 0;
-
-  if (a)
-a = 123; // no-warning
-
-  a = 5;
-}
-
-// Check for bugs where multiple statements are reported
-void test9(unsigned a) {
-  switch (a) {
-if (a) // expected-warning{{never executed}}
-  foo(a + 5); // no-warning
-else  // no-warning
-  foo(a); // no-warning
-case 1:
-case 2:
-  break;
-default:
-  break;
-  }
-}
-
-// Tests from flow-sensitive version
-void test10(void) {
-  goto c;
-  d:
-  goto e; // expected-warning {{never executed}}
-  c: ;
-  int i;
-  return;
-  goto b; // expected-warning {{never executed}}
-  goto a; // expected-warning {{never executed}}
-  b:
-  i = 1; // no-warning
-  a:
-  i = 2;  // no-warning
-  goto f;
-  e:
-  goto d;
-  f: ;
-}
-
-// test11: we can actually end up in the default case, even if it is not
-// obvious: there might be something wrong with the given argument.
-enum foobar { FOO, BAR };
-extern void error(void);
-void test11(enum foobar fb) {
-  switch (fb) {
-case FOO:
-  break;
-case BAR:
-  break;
-default:
-  error(); // no-warning
-  return;
-  error(); // expected-warning {{never executed}}
-  }
-}
-
-void inlined(int condition) {
-  if (condition) {
-foo(5); 

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rG56b9b97c1ef5: [clang][analyzer][ctu] Make CTU a two phase 
analysis (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D123773?vs=430047=430273#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-small-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ctu-implicit.c
  clang/test/Analysis/ctu-main.c
  clang/test/Analysis/ctu-main.cpp
  clang/test/Analysis/ctu-on-demand-parsing.c
  clang/test/Analysis/ctu-on-demand-parsing.cpp
  clang/test/Analysis/ctu-onego-existingdef.cpp
  clang/test/Analysis/ctu-onego-indirect.cpp
  clang/test/Analysis/ctu-onego-small.cpp
  clang/test/Analysis/ctu-onego-toplevel.cpp

Index: clang/test/Analysis/ctu-onego-toplevel.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-toplevel.cpp
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-toplevel-other.cpp.ast %S/Inputs/ctu-onego-toplevel-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -verify=ctu %s
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -analyzer-display-progress \
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+
+// CallGraph: c->b
+// topological sort: c, b
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// because that happens in another TU.
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus
+// that would not be visited as toplevel without special care.
+
+// `c` is analyzed as toplevel and during that the other TU is loaded:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} c(int){{.*}}CTU loaded AST file
+// next, `b` is analyzed as toplevel:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} b(int)
+
+void b(int x);
+void other(int y);
+void c(int y) {
+  other(y);
+  return;
+  // The below call is here to form the proper CallGraph, but will not be
+  // analyzed.
+  b(1);
+}
+
+void b(int x) {
+  if (x == 0)
+(void)(1 / x);
+// ctu-warning@-1{{Division by zero}}
+// We receive the above warning only if `b` is analyzed as top-level.
+}
Index: clang/test/Analysis/ctu-onego-small.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-small.cpp
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \

[PATCH] D123685: [clang][ASTImporter] Add isNewDecl

[Gabor Marton 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 rG25ac078a961d: [clang][ASTImporter] Add isNewDecl (authored 
by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123685

Files:
  clang/include/clang/AST/ASTImporterSharedState.h
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7698,6 +7698,38 @@
   EXPECT_TRUE(ToX->getInClassInitializer());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  void other() {
+bar();
+  }
+  )",
+  Lang_CXX11);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  )",
+  Lang_CXX11);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("other")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_CXX11);
+  ASSERT_TRUE(ToOther);
+
+  auto *ToBar = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("bar")));
+
+  EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
+  EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -285,6 +285,7 @@
   ToD = CreateFun(std::forward(args)...);
   // Keep track of imported Decls.
   Importer.RegisterImportedDecl(FromD, ToD);
+  Importer.SharedState->markAsNewDecl(ToD);
   InitializeImportedDecl(FromD, ToD);
   return false; // A new Decl is created.
 }
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -38,6 +38,9 @@
   /// never cleared (like ImportedFromDecls).
   llvm::DenseMap ImportErrors;
 
+  /// Set of the newly created declarations.
+  llvm::DenseSet NewDecls;
+
   // FIXME put ImportedFromDecls here!
   // And from that point we can better encapsulate the lookup table.
 
@@ -73,6 +76,10 @@
   void setImportDeclError(Decl *To, ImportError Error) {
 ImportErrors[To] = Error;
   }
+
+  bool isNewDecl(const Decl *ToD) const { return NewDecls.count(ToD); }
+
+  void markAsNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };
 
 } // namespace clang


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7698,6 +7698,38 @@
   EXPECT_TRUE(ToX->getInClassInitializer());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  void other() {
+bar();
+  }
+  )",
+  Lang_CXX11);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  )",
+  Lang_CXX11);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("other")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_CXX11);
+  ASSERT_TRUE(ToOther);
+
+  auto *ToBar = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("bar")));
+
+  EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
+  EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -285,6 +285,7 @@
   ToD = CreateFun(std::forward(args)...);
   // Keep track of imported Decls.
   Importer.RegisterImportedDecl(FromD, ToD);
+  Importer.SharedState->markAsNewDecl(ToD);
   InitializeImportedDecl(FromD, ToD);
   return false; // A new Decl is created.
 }
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -38,6 +38,9 @@
   /// never cleared (like ImportedFromDecls).
   llvm::DenseMap ImportErrors;
 
+  /// Set of the newly created declarations.
+  llvm::DenseSet NewDecls;
+
   // FIXME put ImportedFromDecls here!
   // And 

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/docs/ReleaseNotes.rst:352
+  reports are lost compared to single-TU analysis, the lost reports are highly
+  likely to be false positives.
 

xazax.hun wrote:
> I wonder if you also want to mention that this still finds most of the CTU 
> findings in most cases.
Yes, I do, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

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

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 430047.
martong added a comment.

- Update ReleaseNotes.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-small-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ctu-implicit.c
  clang/test/Analysis/ctu-main.c
  clang/test/Analysis/ctu-main.cpp
  clang/test/Analysis/ctu-on-demand-parsing.c
  clang/test/Analysis/ctu-on-demand-parsing.cpp
  clang/test/Analysis/ctu-onego-existingdef.cpp
  clang/test/Analysis/ctu-onego-indirect.cpp
  clang/test/Analysis/ctu-onego-small.cpp
  clang/test/Analysis/ctu-onego-toplevel.cpp

Index: clang/test/Analysis/ctu-onego-toplevel.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-toplevel.cpp
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-toplevel-other.cpp.ast %S/Inputs/ctu-onego-toplevel-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -verify=ctu %s
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -analyzer-display-progress \
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+
+// CallGraph: c->b
+// topological sort: c, b
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// because that happens in another TU.
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus
+// that would not be visited as toplevel without special care.
+
+// `c` is analyzed as toplevel and during that the other TU is loaded:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} c(int){{.*}}CTU loaded AST file
+// next, `b` is analyzed as toplevel:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} b(int)
+
+void b(int x);
+void other(int y);
+void c(int y) {
+  other(y);
+  return;
+  // The below call is here to form the proper CallGraph, but will not be
+  // analyzed.
+  b(1);
+}
+
+void b(int x) {
+  if (x == 0)
+(void)(1 / x);
+// ctu-warning@-1{{Division by zero}}
+// We receive the above warning only if `b` is analyzed as top-level.
+}
Index: clang/test/Analysis/ctu-onego-small.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-small.cpp
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-small-other.cpp.ast %S/Inputs/ctu-onego-small-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// Small function defined in another TU.
+int bar();
+
+// 

[PATCH] D125771: [clang-tidy] Add a useful note about -std=c++11-or-later

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/check_clang_tidy.py:31
+  -std=c++11-or-later:
+This flag will cause multiple runs withing the same check_clang_tidy
+execution. Make sure you don't have shared state across these runs.

typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125771

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

[PATCH] D123784: [clang][analyzer][ctu] Traverse the ctu CallEnter nodes in reverse order

[Gabor Marton via Phabricator via cfe-commits]

martong abandoned this revision.
martong marked an inline comment as done.
martong added a comment.

Abandoning because by using a `bool` in `ctuBifurcate`, the CTUWorklist will 
not have more than one elements during the first phase. Details: 
https://reviews.llvm.org/D123773?id=426037#inline-1206493


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123784

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

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 430023.
martong marked an inline comment as done.
martong added a comment.

- Change ctuBifurcate to use bool in GDM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

Files:
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-small-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ctu-implicit.c
  clang/test/Analysis/ctu-main.c
  clang/test/Analysis/ctu-main.cpp
  clang/test/Analysis/ctu-on-demand-parsing.c
  clang/test/Analysis/ctu-on-demand-parsing.cpp
  clang/test/Analysis/ctu-onego-existingdef.cpp
  clang/test/Analysis/ctu-onego-indirect.cpp
  clang/test/Analysis/ctu-onego-small.cpp
  clang/test/Analysis/ctu-onego-toplevel.cpp

Index: clang/test/Analysis/ctu-onego-toplevel.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-toplevel.cpp
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-toplevel-other.cpp.ast %S/Inputs/ctu-onego-toplevel-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -verify=ctu %s
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -analyzer-display-progress \
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+
+// CallGraph: c->b
+// topological sort: c, b
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// because that happens in another TU.
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus
+// that would not be visited as toplevel without special care.
+
+// `c` is analyzed as toplevel and during that the other TU is loaded:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} c(int){{.*}}CTU loaded AST file
+// next, `b` is analyzed as toplevel:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} b(int)
+
+void b(int x);
+void other(int y);
+void c(int y) {
+  other(y);
+  return;
+  // The below call is here to form the proper CallGraph, but will not be
+  // analyzed.
+  b(1);
+}
+
+void b(int x) {
+  if (x == 0)
+(void)(1 / x);
+// ctu-warning@-1{{Division by zero}}
+// We receive the above warning only if `b` is analyzed as top-level.
+}
Index: clang/test/Analysis/ctu-onego-small.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-small.cpp
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-small-other.cpp.ast %S/Inputs/ctu-onego-small-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// Small function defined in 

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.

xazax.hun wrote:
> martong wrote:
> > xazax.hun wrote:
> > > xazax.hun wrote:
> > > > martong wrote:
> > > > > xazax.hun wrote:
> > > > > > So if we see the same foreign function called in multiple contexts, 
> > > > > > we will only queue one of the contexts for the CTU. Is this the 
> > > > > > intended design? 
> > > > > > 
> > > > > > So if I see:
> > > > > > ```
> > > > > > foreign(true);
> > > > > > foreign(false);
> > > > > > ```
> > > > > > 
> > > > > > The new CTU will only evaluate `foreign(true)` but not 
> > > > > > `foreign(false)`. 
> > > > > This is intentional.
> > > > > ```
> > > > > foreign(true);
> > > > > foreign(false);
> > > > > ```
> > > > > The new CTU will evaluate the following paths in the exploded graph:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // conservative evaluated
> > > > > foreign(true); // inlined
> > > > > foreign(false); // inlined
> > > > > ```
> > > > > The point is to keep bifurcation to a minimum and avoid the 
> > > > > exponential blow up.
> > > > > So, we will never have a path like this:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // inlined
> > > > > ```
> > > > > 
> > > > > Actually, this is the same strategy that we use during the dynamic 
> > > > > dispatch of C++ virtual calls. See `DynamicDispatchBifurcationMap`.
> > > > > 
> > > > > The conservative evaluation happens in the first phase, the inlining 
> > > > > in the second phase (assuming the phase1 inlining option is set to 
> > > > > none).
> > > > > The new CTU will evaluate the following paths in the exploded graph:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // conservative evaluated
> > > > > foreign(true); // inlined
> > > > > foreign(false); // inlined
> > > > > ```
> > > > 
> > > > When we encounter `foreign(true)`, we would add the decl to 
> > > > `CTUDispatchBifurcationSet`. So the second time we see a call to the 
> > > > function `foreign(false);`, we will just do the conservative evaluation 
> > > > and will not add the call to the CTU worklist. So how will 
> > > > `foreign(false);` be inlined in the second pass? Do I miss something? 
> > > > 
> > > Oh, I think I now understand. Do you expect `foreign(false);` to be 
> > > inlined after we return from `foreign(true);` in the second pass? Sorry 
> > > for the confusion, in that case it looks good to me.
> > > Oh, I think I now understand. Do you expect `foreign(false);` to be 
> > > inlined after we return from `foreign(true);` in the second pass?
> > 
> > Yes, that's right.
> Actually, in this case I wonder whether we really need a set of declarations. 
> Wouldn't a boolean be sufficient for each path?
> 
> So in case of:
> ```
> foreign1(true);
> foreign2(false);
> ```
> 
> Why would we want to add `foreign2` to the CTU worklist? More specifically, 
> why is it important whether a foreign call is actually the same declaration 
> that we saw earlier on the path? Isn't being foreign the only important 
> information in this case?
Good point. 

By using the set of declarations we might have a path where `foreign1` is 
conservatively evaluated and then `foreign2` is inlined. I was having a hard 
time to find any use-cases where this would be useful, but could not find one. 

On the other hand, by using a `bool`, as you suggest, no such superfluous path 
would exist. Plus, the dependent child patch (D123784) is becoming unnecessary 
because the CTUWorklist will have at most one element during the first phase.

Besides, I've made measurements comparing the `bool` and the `set` 
implementation. The results are quite similar. Both have lost the same amount 
of bugreports compared to the single-tu analysis and the average length of the 
bugpaths are also similar. {F23090628}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

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

[PATCH] D125708: [analyzer][NFC] Remove unused default SVal constructors

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125708

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

[PATCH] D125707: [analyzer][NFC] Remove unused friend SVal declarations

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125707

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

[PATCH] D125706: [analyzer][NFC] Use idiomatic classof instead of isKind

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125706

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

[PATCH] D125709: [analyzer][Casting] Support isa, cast, dyn_cast of SVals

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> I'm not sure, shall I add tests?

Yes, please. Unit tests for `dyn_cast` and `isa` should be easy. However, I am 
not sure how to test `cast` for the failure cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125709

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

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:435
+  assert(Cond);
+  State = State->assume(*Cond, true);
+  return errno_modeling::setErrnoValue(State, C.getLocationContext(),

balazske wrote:
> martong wrote:
> > Please check if `State` can be nullptr.
> I think here it is never null. A relation is created between a new conjured 
> symbol and zero, this can never fail, or not? (The `ErrnoSVal` should be here 
> a newly created symbol. If the `Tag` is not used it may be the same symbol 
> that was previously bound to the expression if `EvalCallAsPure` is used.)
Okay, I see your reasoning. However, we might never know how the constraint 
solver reasons about it, thus we should not rely on that. As a contrived 
example, it might check recursively that the LHS of the BinOp you just conjured 
and it might recognize that the State is infeasible.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:603
 
-Summary (ConstraintSet &, StringRef Note = "") {
-  Cases.push_back(SummaryCase(std::move(CS), Note));
+Summary (ConstraintSet &, const ErrnoConstraintKind ,
+  StringRef Note = "") {

balazske wrote:
> martong wrote:
> > Would it make sense to have a `ErrnoIrrelevant` as the default value for 
> > `ErrnoC`?
> It would be a bit more convenient but in the current design it is not 
> possible to pass the member variable as default value. `ErrnoIrrelevant` is 
> really used in less than half of the cases.
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D125400#3516164 , @balazske wrote:

> @martong Do you mean that a "describe" function is needed for the return 
> value constraint of the function and for the errno constraint type? Then a 
> note tag can contain what the function is assumed to return on success and 
> what is allowed (or not) to do with `errno`. For example: "Assuming that 
> 'mkdir' is successful, it returns zero in this case and value of 'errno' is 
> unspecified after the call".

Yes, that sounds good. We have the virtual `describe` function in the base 
constraint class that could be used to explain what values we expect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D125400#3508955 , @balazske wrote:

> Function `mkdir` is modeled incorrectly by the checker. According to the man 
> page it can return 0 or -1 only (-1 is error) but the checker allows 
> non-negative value at success. So the shown bug report is incorrect (it can 
> be only -1 if not 0 and then check of `errno` is allowed). Anyway the note 
> tags should be added to every function.

Okay, could you please correct the summary then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D125547#3515259 , @steakhal wrote:

> Something is messed up with the diff. You refer to `fromUnOp()` but it's not 
> defined anywhere.

No. There is no mess up here, the diff is correct. `fromUnOp` had been 
implemented way before. I missed that when I created the initial patch. So, we 
just have to call that preexisting function in `getSymExpr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429633.
martong marked 2 inline comments as done.
martong added a comment.

- Use existing fromUnOp
- pass nullptr as FromTy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/test/Analysis/z3-crosscheck.c


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded 
from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -446,6 +446,14 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+  llvm::SMTExprRef Exp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), nullptr, hasComparison);
+  return fromUnOp(Solver, USE->getOpcode(), Exp);
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -446,6 +446,14 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+  llvm::SMTExprRef Exp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), nullptr, hasComparison);
+  return fromUnOp(Solver, USE->getOpcode(), Exp);
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:258
 
+  static inline llvm::SMTExprRef fromUnary(llvm::SMTSolverRef ,
+   ASTContext ,

steakhal wrote:
> I would prefer the `fromUnOp` similarly to how `fromBinOp` is called.
Yeah, very good point, I just realized that `fromUnOp` is already implemented!



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:467-469
+  QualType FromTy;
+  llvm::SMTExprRef Exp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , hasComparison);

steakhal wrote:
> If you discard the `FromTy`, you could have passed a nullptr there.
Ok, Changed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125547

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

[PATCH] D125524: [BoundV2] ArrayBoundV2 checks if the extent is tainted

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:208
 if (state_exceedsUpperBound && state_withinUpperBound) {
-  SVal ByteOffset = rawOffset.getByteOffset();
-  if (isTainted(state, ByteOffset)) {
+  if (isTainted(state, *upperboundToCheck)) {
 reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted,

steakhal wrote:
> martong wrote:
> > Could you please explain why we change `rawOffset` to `*upperBoundToCheck`? 
> > And perhaps the same explanation could infiltrate into the checker's code 
> > itself as a comment to `upperbound`.
> In the test attached you can see that the extent is tainted, not the offset.
> Thus checking the offset for taint won't suffice.
> The bug condition should depend on the calculation itself, which is basically 
> what is done here.
Okay makes sense, but then please update the comment
```
// If we are under constrained and the index variables are tainted, report.
```
to mention the extent as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125524

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

[PATCH] D125547: [analyzer][solver] Handle UnarySymExpr in SMTConv

[Gabor Marton via Phabricator via cfe-commits]

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

Dependent patch adds UnarySymExpr, now I'd like to handle that for SMT
conversions like refutation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125547

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/test/Analysis/z3-crosscheck.c


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded 
from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -255,6 +255,20 @@
 llvm_unreachable("Unimplemented opcode");
   }
 
+  static inline llvm::SMTExprRef fromUnary(llvm::SMTSolverRef ,
+   ASTContext ,
+   const llvm::SMTExprRef ,
+   const UnaryOperator::Opcode Op) {
+switch (Op) {
+case UO_Minus:
+  return Solver->mkBVNeg(Exp);
+case UO_Not:
+  return Solver->mkBVNot(Exp);
+default:;
+}
+llvm_unreachable("Unimplemented opcode");
+  }
+
   /// Construct an SMTSolverRef from a QualType FromTy to a QualType ToTy,
   /// and their bit widths.
   static inline llvm::SMTExprRef fromCast(llvm::SMTSolverRef ,
@@ -446,6 +460,17 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+
+  QualType FromTy;
+  llvm::SMTExprRef Exp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , hasComparison);
+
+  return fromUnary(Solver, Ctx, Exp, USE->getOpcode());
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);


Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -14,6 +14,20 @@
   return 0;
 }
 
+int unary(int x, long l)
+{
+  int *z = 0;
+  int y = l;
+  if ((x & 1) && ((x & 1) ^ 1))
+if (-y)
+#ifdef NO_CROSSCHECK
+return *z; // expected-warning {{Dereference of null pointer (loaded from variable 'z')}}
+#else
+return *z; // no-warning
+#endif
+  return 0;
+}
+
 void g(int d);
 
 void f(int *a, int *b) {
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
@@ -255,6 +255,20 @@
 llvm_unreachable("Unimplemented opcode");
   }
 
+  static inline llvm::SMTExprRef fromUnary(llvm::SMTSolverRef ,
+   ASTContext ,
+   const llvm::SMTExprRef ,
+   const UnaryOperator::Opcode Op) {
+switch (Op) {
+case UO_Minus:
+  return Solver->mkBVNeg(Exp);
+case UO_Not:
+  return Solver->mkBVNot(Exp);
+default:;
+}
+llvm_unreachable("Unimplemented opcode");
+  }
+
   /// Construct an SMTSolverRef from a QualType FromTy to a QualType ToTy,
   /// and their bit widths.
   static inline llvm::SMTExprRef fromCast(llvm::SMTSolverRef ,
@@ -446,6 +460,17 @@
   return getCastExpr(Solver, Ctx, Exp, FromTy, Sym->getType());
 }
 
+if (const UnarySymExpr *USE = dyn_cast(Sym)) {
+  if (RetTy)
+*RetTy = Sym->getType();
+
+  QualType FromTy;
+  llvm::SMTExprRef Exp =
+  getSymExpr(Solver, Ctx, USE->getOperand(), , hasComparison);
+
+  return fromUnary(Solver, Ctx, Exp, USE->getOpcode());
+}
+
 if (const BinarySymExpr *BSE = dyn_cast(Sym)) {
   llvm::SMTExprRef Exp =
   getSymBinExpr(Solver, Ctx, BSE, hasComparison, RetTy);
___
cfe-commits mailing 

[PATCH] D123685: [clang][ASTImporter] Add isNewDecl

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429212.
martong added a comment.

- setNewDecl -> markAsNewDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123685

Files:
  clang/include/clang/AST/ASTImporterSharedState.h
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7686,6 +7686,38 @@
   EXPECT_TRUE(ToX->getInClassInitializer());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  void other() {
+bar();
+  }
+  )",
+  Lang_CXX11);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  )",
+  Lang_CXX11);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("other")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_CXX11);
+  ASSERT_TRUE(ToOther);
+
+  auto *ToBar = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("bar")));
+
+  EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
+  EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -285,6 +285,7 @@
   ToD = CreateFun(std::forward(args)...);
   // Keep track of imported Decls.
   Importer.RegisterImportedDecl(FromD, ToD);
+  Importer.SharedState->markAsNewDecl(ToD);
   InitializeImportedDecl(FromD, ToD);
   return false; // A new Decl is created.
 }
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -39,6 +39,9 @@
   /// never cleared (like ImportedFromDecls).
   llvm::DenseMap ImportErrors;
 
+  /// Set of the newly created declarations.
+  llvm::DenseSet NewDecls;
+
   // FIXME put ImportedFromDecls here!
   // And from that point we can better encapsulate the lookup table.
 
@@ -74,6 +77,10 @@
   void setImportDeclError(Decl *To, ImportError Error) {
 ImportErrors[To] = Error;
   }
+
+  bool isNewDecl(const Decl *ToD) const { return NewDecls.count(ToD); }
+
+  void markAsNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };
 
 } // namespace clang


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7686,6 +7686,38 @@
   EXPECT_TRUE(ToX->getInClassInitializer());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  void other() {
+bar();
+  }
+  )",
+  Lang_CXX11);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  int bar() {
+return 0;
+  }
+  )",
+  Lang_CXX11);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("other")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_CXX11);
+  ASSERT_TRUE(ToOther);
+
+  auto *ToBar = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("bar")));
+
+  EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
+  EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -285,6 +285,7 @@
   ToD = CreateFun(std::forward(args)...);
   // Keep track of imported Decls.
   Importer.RegisterImportedDecl(FromD, ToD);
+  Importer.SharedState->markAsNewDecl(ToD);
   InitializeImportedDecl(FromD, ToD);
   return false; // A new Decl is created.
 }
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -39,6 +39,9 @@
   /// never cleared (like ImportedFromDecls).
   llvm::DenseMap ImportErrors;
 
+  /// Set of the newly created declarations.
+  llvm::DenseSet NewDecls;
+
   // FIXME put ImportedFromDecls here!
   // And from that point we can better encapsulate the lookup table.
 
@@ -74,6 +77,10 @@
   void setImportDeclError(Decl *To, 

[PATCH] D125524: [BoundV2] ArrayBoundV2 checks if the extent is tainted

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:208
 if (state_exceedsUpperBound && state_withinUpperBound) {
-  SVal ByteOffset = rawOffset.getByteOffset();
-  if (isTainted(state, ByteOffset)) {
+  if (isTainted(state, *upperboundToCheck)) {
 reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted,

Could you please explain why we change `rawOffset` to `*upperBoundToCheck`? And 
perhaps the same explanation could infiltrate into the checker's code itself as 
a comment to `upperbound`.



Comment at: clang/test/Analysis/taint-diagnostic-visitor.c:46-48
+  int *p = (int *)malloc(x + conj); // Generic taint checker forbids tainted 
allocation.
+  // expected-warning@-1 {{Untrusted data is used to specify the buffer size}}
+  // expected-note@-2{{Untrusted data is used to specify the buffer size}}

Could we get rid of the seemingly unrelated malloc taint report by using an 
array on the stack?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125524

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

[PATCH] D125532: [analyzer] Introduce clang_analyzer_dumpSvalType introspection function

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:268
+  QualType Ty = C.getSVal(Arg).getType(C.getASTContext());
+  reportBug(Ty.getAsString(), C);
+}

Would it make sense to call `Ty->dump(OS)` to a stream and then report the 
string of the stream?
That way we could see some more information. Or perhaps that could be done in 
another introspection function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125532

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

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/unary-sym-expr.c:35
+return;
+  clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
+}

steakhal wrote:
> Does it work if you swap x and y?
No, that does not. And the reason of that is we cannot handle commutativity 
(yet).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

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

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429208.
martong added a comment.

- Add a test case for casts related crash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate.c
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/test/Analysis/unary-sym-expr-no-crash.c
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- clang/test/Analysis/unary-sym-expr.c
+++ clang/test/Analysis/unary-sym-expr.c
@@ -34,3 +34,12 @@
 return;
   clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
 }
+
+int test_fp(int flag) {
+  int value;
+  if (flag == 0)
+value = 1;
+  if (-flag == 0)
+return value; // no-warning
+  return 42;
+}
Index: clang/test/Analysis/unary-sym-expr-no-crash.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr-no-crash.c
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config support-symbolic-integer-casts=false \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
+
+// expected-no-diagnostics
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+void crash(int b, long c) {
+  b = c;
+  if (b > 0)
+if(-b) // should not crash here
+  ;
+}
Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -10,11 +12,8 @@
 #define INT_MAX (UINT_MAX & (UINT_MAX >> 1))
 #define INT_MIN (UINT_MAX & ~(UINT_MAX >> 1))
 
-extern void __assert_fail (__const char *__assertion, __const char *__file,
-unsigned int __line, __const char *__function)
- __attribute__ ((__noreturn__));
-#define assert(expr) \
-  ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
 
 void assert_in_range(int x) {
   assert(x <= ((int)INT_MAX / 4));
@@ -33,69 +32,60 @@
 
 void equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m != n)
-return;
+  assert(m == n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n == m); // expected-warning{{TRUE}}
 }
 
 void non_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m == n)
-return;
+  assert(m != n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n != m); // expected-warning{{TRUE}}
 }
 
 void less_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m < n)
-return;
+  assert(m >= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n <= m); // expected-warning{{TRUE}}
 }
 
 void less(int m, int n) {
   assert_in_range_2(m, n);
-  if (m <= n)
-return;
+  assert(m > n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n < m); // expected-warning{{TRUE}}
 }
 
 void greater_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m > n)
-return;
+  assert(m <= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n >= m); // expected-warning{{TRUE}}
 }
 
 void greater(int m, int n) {
   assert_in_range_2(m, n);
-  if (m >= n)
-return;
+  assert(m < n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n > m); // expected-warning{{TRUE}}
 }
 
 void negate_positive_range(int m, int n) {
-  if (m - n <= 0)
-return;
+  assert(m - n > 0);
   clang_analyzer_eval(n - m < 0); // expected-warning{{TRUE}}
   clang_analyzer_eval(n - m > INT_MIN); // expected-warning{{TRUE}}
-  clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{FALSE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void negate_int_min(int m, int n) {
-  if (m - n != INT_MIN)
-return;
+  assert(m - n == INT_MIN);
   clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{TRUE}}
 }
 
 void negate_mixed(int m, int n) {
-  if (m -n > INT_MIN && m - n <= 0)
-return;
+  assert(m - n > 0 || m - n == INT_MIN);
   clang_analyzer_eval(n - m <= 0); // expected-warning{{TRUE}}
 }
 
@@ -106,54 +96,59 @@
   

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429207.
martong added a comment.

- Revert "Add type parameter to evalMinus and evalComplement"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Symbols.def
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+int test(int x, int y) {
+
+  clang_analyzer_dump(-x);   // expected-warning{{-reg_$0}}
+  clang_analyzer_dump(~x);   // expected-warning{{~reg_$0}}
+  int z = x + y;
+  clang_analyzer_dump(-z);   // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-(x + y)); // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-x + y);   // expected-warning{{(-reg_$0) + (reg_$1)}}
+
+  if (-x == 0) {
+clang_analyzer_eval(-x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(-x > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(-x < 0);  // expected-warning{{FALSE}}
+  }
+  if (~y == 0) {
+clang_analyzer_eval(~y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(~y > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(~y < 0);  // expected-warning{{FALSE}}
+  }
+  (void)(x);
+  return 42;
+}
+
+void test_svalbuilder_simplification(int x, int y) {
+  if (x + y != 3)
+return;
+  clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
+}
Index: clang/test/Analysis/expr-inspection.cpp
===
--- clang/test/Analysis/expr-inspection.cpp
+++ clang/test/Analysis/expr-inspection.cpp
@@ -18,6 +18,8 @@
   clang_analyzer_express(x); // expected-warning{{Unable to express}}
 
   clang_analyzer_denote(x, "$x");
+  clang_analyzer_express(-x); // expected-warning{{-$x}}
+
   clang_analyzer_denote(y, "$y");
   clang_analyzer_express(x + y); // expected-warning{{$x + $y}}
 
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -72,6 +72,7 @@
 void test_4(int x, int y) {
   int z;
   static int stat;
+  clang_analyzer_explain(-x);// expected-warning-re^\- \(argument 'x'\)$
   clang_analyzer_explain(x + 1); // expected-warning-re^\(argument 'x'\) \+ 1$
   clang_analyzer_explain(1 + y); // expected-warning-re^\(argument 'y'\) \+ 1$
   clang_analyzer_explain(x + y); // expected-warning-re^\(argument 'x'\) \+ \(argument 'y'\)$
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -70,6 +70,16 @@
   os << ')';
 }
 
+void UnarySymExpr::dumpToStream(raw_ostream ) const {
+  os << UnaryOperator::getOpcodeStr(Op);
+  bool Binary = isa(Operand);
+  if (Binary)
+os << '(';
+  Operand->dumpToStream(os);
+  if (Binary)
+os << ')';
+}
+
 void SymbolConjured::dumpToStream(raw_ostream ) const {
   os << getKindStr() << getSymbolID() << '{' << T << ", LC" << LCtx->getID();
   if (S)
@@ -134,6 +144,9 @@
 case SymExpr::SymbolCastKind:
   itr.push_back(cast(SE)->getOperand());
   return;
+case SymExpr::UnarySymExprKind:
+  itr.push_back(cast(SE)->getOperand());
+  return;
 case SymExpr::SymIntExprKind:
   itr.push_back(cast(SE)->getLHS());
   return;
@@ -306,6 +319,22 @@
   return cast(data);
 }
 
+const UnarySymExpr *SymbolManager::getUnarySymExpr(const SymExpr *Operand,
+   UnaryOperator::Opcode Opc,
+   QualType T) {
+  llvm::FoldingSetNodeID ID;
+  UnarySymExpr::Profile(ID, Operand, Opc, T);
+  void *InsertPos;
+  SymExpr *data = DataSet.FindNodeOrInsertPos(ID, InsertPos);
+  if (!data) {
+data = (UnarySymExpr *)BPAlloc.Allocate();
+new (data) UnarySymExpr(Operand, Opc, T);
+DataSet.InsertNode(data, InsertPos);
+  }
+

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:107
+return makeNonLoc(X.castAs().getSymbol(), UO_Not,
+  X.getType(Context));
   default:

steakhal wrote:
> martong wrote:
> > steakhal wrote:
> > > I'm not sure if we should rely on `SVal::getType()`. I think the calling 
> > > context should pass the type along here.
> > Good point. One example where the type of the SVal is different than the 
> > type of the UnaryExpr is here:
> > ```
> > void test_x(int x, long y) {
> >   x = y;  // $x has `long` type
> >   -x; // the UnaryExpr `-x` has `int` type
> > }
> > ```
> > Note that if we use `support-symbolic-integer-casts=true` then there is no 
> > such type mismatch because there is in implicit cast modeled at `x = y`, 
> > thus $x type is correctly `int`.
> > 
> > Anyway, I am going to update evalMinus and evalComplement to take an 
> > additional QualType parameter.
> Please add this test with both scenarios with 
> `support-symbolic-integer-casts`. D125532 will help in demonstrating this.
Ahh, unfortunately, if we use the type of the AST's `UnaryExpr` instead of the 
operand `SVal` then we will end up crashing the constraint solver, because of 
the missing SymbolCast below:
```
void crash(int b, long c) {
  b = c;
  if (b > 0)// $b  is long
if(-b) // should not crash here // $-b is int
  ;
  // SymbolicRangeInferrer
  // takes the negated range of $-b (with type int)
  // getRangeForNegatedSub
  // and takes the range of $b (with type long) and tries to intersect them.
  // RangeSet VisitSymExpr(SymbolRef Sym)
}
```
This manifests in the child patch, because that's where we add the support of 
UnarySymExpr to the solver, so I add a test case there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

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

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429172.
martong marked 7 inline comments as done.
martong added a comment.

- Change -mul to -pct
- Change default values to ctu-max-nodes-pct = 50 and ctu-max-nodes-min = 1
- Rename isCTUInFirtstPhase to isSecondPhaseCTU and invert the condition
- Check that the AST file is loaded in existingdef test file
- Set CTUWlist as nullptr in single-TU mode
- Add bar to ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt
- Changes in comments of test files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

Files:
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-indirect-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-small-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-small-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp
  
clang/test/Analysis/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ctu-implicit.c
  clang/test/Analysis/ctu-main.c
  clang/test/Analysis/ctu-main.cpp
  clang/test/Analysis/ctu-on-demand-parsing.c
  clang/test/Analysis/ctu-on-demand-parsing.cpp
  clang/test/Analysis/ctu-onego-existingdef.cpp
  clang/test/Analysis/ctu-onego-indirect.cpp
  clang/test/Analysis/ctu-onego-small.cpp
  clang/test/Analysis/ctu-onego-toplevel.cpp

Index: clang/test/Analysis/ctu-onego-toplevel.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-toplevel.cpp
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-onego-toplevel-other.cpp.ast %S/Inputs/ctu-onego-toplevel-other.cpp
+// RUN: cp %S/Inputs/ctu-onego-toplevel-other.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -verify=ctu %s
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-pc-linux-gnu \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -analyzer-config ctu-phase1-inlining=none \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -analyzer-display-progress \
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+
+// CallGraph: c->b
+// topological sort: c, b
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// because that happens in another TU.
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus
+// that would not be visited as toplevel without special care.
+
+// `c` is analyzed as toplevel and during that the other TU is loaded:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} c(int){{.*}}CTU loaded AST file
+// next, `b` is analyzed as toplevel:
+// CHECK: ANALYZE (Path,  Inline_Regular): {{.*}} b(int)
+
+void b(int x);
+void other(int y);
+void c(int y) {
+  other(y);
+  return;
+  // The below call is here to form the proper CallGraph, but will not be
+  // analyzed.
+  b(1);
+}
+
+void b(int x) {
+  if (x == 0)
+(void)(1 / x);
+// ctu-warning@-1{{Division by zero}}
+// We receive the above warning only if `b` is analyzed as top-level.
+}
Index: clang/test/Analysis/ctu-onego-small.cpp
===
--- /dev/null
+++ clang/test/Analysis/ctu-onego-small.cpp
@@ -0,0 +1,51 @@
+// RUN: rm 

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

martong marked 15 inline comments as done.
martong added inline comments.



Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };

whisperity wrote:
> (The naming of this function feels a bit odd. `markAsNewDecl` or just 
> `markNewDecl`?)
Good point, `markAsNewDecl` sounds better. I'll update this in the parent patch 
https://reviews.llvm.org/D123685 though (and rebase this).



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413-419
+"We count the nodes for a normal single tu analysis. We multiply that "
+"number with this config value then we divide the result by 100 to get "
+"the maximum number of nodes that the analyzer can generate while "
+"exploring a top level function in CTU mode (for each exploded graph)."
+"For example, 100 means that CTU will explore maximum as much nodes that "
+"we explored during the single tu mode, 200 means it will explore twice as 
"
+"much, 50 means it will explore maximum 50% more.", 100)

whisperity wrote:
> whisperity wrote:
> > 
> Couldn't this description here be simplified to say something along the lines 
> of //"the percentage of single-TU analysed nodes that the CTU analysis is 
> allowed to visit"//? Is the calculation method needed from the user's 
> perspective? The examples talk about percentage too.
Thanks, this is so true, very good point. I've changed it. And also changed the 
name to suffix `-pct` instead of `-mul` to reflect this.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+

whisperity wrote:
> whisperity wrote:
> > Is this configuration inherent to the static analyser, and not the 
> > //CrossTU// library?
> (Documentation for the options are missing.)
Yes, this is only for the analyzer.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+

martong wrote:
> whisperity wrote:
> > whisperity wrote:
> > > Is this configuration inherent to the static analyser, and not the 
> > > //CrossTU// library?
> > (Documentation for the options are missing.)
> Yes, this is only for the analyzer.
This is the direct representation of the analyzer option `ctu-phase1-inlining` 
and there is a bulky documentation for that in `AnalyzerOptions.def`. I'd 
rather not repeat that documentation here.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:814-816
+  /// Returns true if the CTU analysis is running its first phase.
+  /// Returns true in single TU (non-CTU) mode!
+  bool isCTUInFirtstPhase() { return Engine.getCTUWorkList(); }

whisperity wrote:
> How and why is this needed? Could you call it `isSingleTUOr1stPhaseCTU` 
> instead?
> 
> Rather, if this is used as a distinction input in conditionals, could you 
> invert the branches and have a function `isSecondPhaseCTU`, and do the 
> inverted logic where this function is consumed?
Yep, very good point, thanks again. Changed it with the inverted branch version.



Comment at: 
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt:1
+11:c:@F@other# ctu-onego-other.cpp.ast

whisperity wrote:
> Why is there only 1 symbol in this file, when the file above contains two 
> function definitions?
Good catch. I've added the other function. Besides, I had the wrong filename 
provided, it should have been `ctu-onego-existingdef-other.cpp.ast`, the 
`existingdef` was missing. Fixed that too, plus added and extra `FileCheck` to 
the `existingdef` test that detects if the ast file is not loaded.



Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:30
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus

whisperity wrote:
> One-go? And what does that refer to? Is "Onego" analysis the one this patch 
> is introducing?
Yes. I've updated the summary of the patch to make this clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

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

[PATCH] D123685: [clang][ASTImporter] Add isNewDecl

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };

`markAsNewDecl` sounds better, I'll update before commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123685

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

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 429152.
martong marked an inline comment as done.
martong added a comment.

- Add type parameter to evalMinus and evalComplement
- Correct dumpToStream in case of binary sub expressions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Symbols.def
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- /dev/null
+++ clang/test/Analysis/unary-sym-expr.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+int test(int x, int y) {
+
+  clang_analyzer_dump(-x);   // expected-warning{{-reg_$0}}
+  clang_analyzer_dump(~x);   // expected-warning{{~reg_$0}}
+  int z = x + y;
+  clang_analyzer_dump(-z);   // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-(x + y)); // expected-warning{{-((reg_$0) + (reg_$1))}}
+  clang_analyzer_dump(-x + y);   // expected-warning{{(-reg_$0) + (reg_$1)}}
+
+  if (-x == 0) {
+clang_analyzer_eval(-x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(-x > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(-x < 0);  // expected-warning{{FALSE}}
+  }
+  if (~y == 0) {
+clang_analyzer_eval(~y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(~y > 0);  // expected-warning{{FALSE}}
+clang_analyzer_eval(~y < 0);  // expected-warning{{FALSE}}
+  }
+  (void)(x);
+  return 42;
+}
+
+void test_svalbuilder_simplification(int x, int y) {
+  if (x + y != 3)
+return;
+  clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
+}
Index: clang/test/Analysis/expr-inspection.cpp
===
--- clang/test/Analysis/expr-inspection.cpp
+++ clang/test/Analysis/expr-inspection.cpp
@@ -18,6 +18,8 @@
   clang_analyzer_express(x); // expected-warning{{Unable to express}}
 
   clang_analyzer_denote(x, "$x");
+  clang_analyzer_express(-x); // expected-warning{{-$x}}
+
   clang_analyzer_denote(y, "$y");
   clang_analyzer_express(x + y); // expected-warning{{$x + $y}}
 
Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -72,6 +72,7 @@
 void test_4(int x, int y) {
   int z;
   static int stat;
+  clang_analyzer_explain(-x);// expected-warning-re^\- \(argument 'x'\)$
   clang_analyzer_explain(x + 1); // expected-warning-re^\(argument 'x'\) \+ 1$
   clang_analyzer_explain(1 + y); // expected-warning-re^\(argument 'y'\) \+ 1$
   clang_analyzer_explain(x + y); // expected-warning-re^\(argument 'x'\) \+ \(argument 'y'\)$
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -70,6 +70,16 @@
   os << ')';
 }
 
+void UnarySymExpr::dumpToStream(raw_ostream ) const {
+  os << UnaryOperator::getOpcodeStr(Op);
+  bool Binary = isa(Operand);
+  if (Binary)
+os << '(';
+  Operand->dumpToStream(os);
+  if (Binary)
+os << ')';
+}
+
 void SymbolConjured::dumpToStream(raw_ostream ) const {
   os << getKindStr() << getSymbolID() << '{' << T << ", LC" << LCtx->getID();
   if (S)
@@ -134,6 +144,9 @@
 case SymExpr::SymbolCastKind:
   itr.push_back(cast(SE)->getOperand());
   return;
+case SymExpr::UnarySymExprKind:
+  itr.push_back(cast(SE)->getOperand());
+  return;
 case SymExpr::SymIntExprKind:
   itr.push_back(cast(SE)->getLHS());
   return;
@@ -306,6 +319,22 @@
   return cast(data);
 }
 
+const UnarySymExpr *SymbolManager::getUnarySymExpr(const SymExpr *Operand,
+   UnaryOperator::Opcode Opc,
+   QualType T) {
+  llvm::FoldingSetNodeID ID;
+  

[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:107
+return makeNonLoc(X.castAs().getSymbol(), UO_Not,
+  X.getType(Context));
   default:

steakhal wrote:
> I'm not sure if we should rely on `SVal::getType()`. I think the calling 
> context should pass the type along here.
Good point. One example where the type of the SVal is different than the type 
of the UnaryExpr is here:
```
void test_x(int x, long y) {
  x = y;  // $x has `long` type
  -x; // the UnaryExpr `-x` has `int` type
}
```
Note that if we use `support-symbolic-integer-casts=true` then there is no such 
type mismatch because there is in implicit cast modeled at `x = y`, thus $x 
type is correctly `int`.

Anyway, I am going to update evalMinus and evalComplement to take an additional 
QualType parameter.



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:73-76
+void UnarySymExpr::dumpToStream(raw_ostream ) const {
+  os << UnaryOperator::getOpcodeStr(Op);
+  Operand->dumpToStream(os);
+}

Would be better to add parenthesis around for compound expressions. So 
expressions like -x where x = a + b should look like `-($a + $b)` instead of 
the wrong format `-$a + $b`. I am going to update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318

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

[PATCH] D125463: [analyzer][NFC] Tighten some of the SValBuilder return types

[Gabor Marton via Phabricator via cfe-commits]

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

Seems like it compiles, so LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125463

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

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 428932.
martong marked 6 inline comments as done.
martong added a comment.

- Address steakhal review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate.c
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- clang/test/Analysis/unary-sym-expr.c
+++ clang/test/Analysis/unary-sym-expr.c
@@ -28,3 +28,12 @@
 return;
   clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
 }
+
+int test_fp(int flag) {
+  int value;
+  if (flag == 0)
+value = 1;
+  if (-flag == 0)
+return value; // no-warning
+  return 42;
+}
Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -10,11 +12,8 @@
 #define INT_MAX (UINT_MAX & (UINT_MAX >> 1))
 #define INT_MIN (UINT_MAX & ~(UINT_MAX >> 1))
 
-extern void __assert_fail (__const char *__assertion, __const char *__file,
-unsigned int __line, __const char *__function)
- __attribute__ ((__noreturn__));
-#define assert(expr) \
-  ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
 
 void assert_in_range(int x) {
   assert(x <= ((int)INT_MAX / 4));
@@ -33,69 +32,60 @@
 
 void equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m != n)
-return;
+  assert(m == n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n == m); // expected-warning{{TRUE}}
 }
 
 void non_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m == n)
-return;
+  assert(m != n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n != m); // expected-warning{{TRUE}}
 }
 
 void less_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m < n)
-return;
+  assert(m >= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n <= m); // expected-warning{{TRUE}}
 }
 
 void less(int m, int n) {
   assert_in_range_2(m, n);
-  if (m <= n)
-return;
+  assert(m > n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n < m); // expected-warning{{TRUE}}
 }
 
 void greater_or_equal(int m, int n) {
   assert_in_range_2(m, n);
-  if (m > n)
-return;
+  assert(m <= n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n >= m); // expected-warning{{TRUE}}
 }
 
 void greater(int m, int n) {
   assert_in_range_2(m, n);
-  if (m >= n)
-return;
+  assert(m < n);
   assert_in_wide_range(m - n);
   clang_analyzer_eval(n > m); // expected-warning{{TRUE}}
 }
 
 void negate_positive_range(int m, int n) {
-  if (m - n <= 0)
-return;
+  assert(m - n > 0);
   clang_analyzer_eval(n - m < 0); // expected-warning{{TRUE}}
   clang_analyzer_eval(n - m > INT_MIN); // expected-warning{{TRUE}}
-  clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{FALSE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void negate_int_min(int m, int n) {
-  if (m - n != INT_MIN)
-return;
+  assert(m - n == INT_MIN);
   clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{TRUE}}
 }
 
 void negate_mixed(int m, int n) {
-  if (m -n > INT_MIN && m - n <= 0)
-return;
+  assert(m - n > 0 || m - n == INT_MIN);
   clang_analyzer_eval(n - m <= 0); // expected-warning{{TRUE}}
 }
 
@@ -106,54 +96,59 @@
   clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void effective_range_2(int m, int n) {
   assert(m - n <= 0);
   assert(n - m <= 0);
-  clang_analyzer_eval(m - n == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
-  clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  clang_analyzer_eval(m - n == 0 || m - n == INT_MIN); // expected-warning{{TRUE}}
 }
 
 void negate_unsigned_min(unsigned m, unsigned n) {
-  if (m - n == UINT_MIN) {
-clang_analyzer_eval(n - m == UINT_MIN); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m != UINT_MIN); // expected-warning{{FALSE}}
-clang_analyzer_eval(n - m > UINT_MIN);  // expected-warning{{FALSE}}
-clang_analyzer_eval(n - m < UINT_MIN);  // expected-warning{{FALSE}}
-  }
+  assert(m - n == UINT_MIN);
+  

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

martong marked 13 inline comments as done.
martong added a comment.

In D125395#3506854 , @steakhal wrote:

> Great content!
> I've got a long list of nits though. Nothing personal :D

No worries, thank you for being assiduous.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1454-1456
+  if (USE->getOpcode() == UO_Minus) {
+// Just get the operand when we negate a symbol that is already 
negated.
+// -(-a) == a, ~(~a) == a

steakhal wrote:
> The comment says that it would work with both `-` and `~`.
> Why do you check against only `-`?
Unfortunately, the complement operation is not implemented on the RangeSet. So, 
yes, in this sense the comment is misleading, I've changed it.



Comment at: clang/test/Analysis/constraint_manager_negate.c:27-29
+  clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a >= INT_MIN); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a == INT_MIN); // expected-warning{{FALSE}}

steakhal wrote:
> You can also query if the bound is sharp, by asking the same question but 
> with a slightly different value and expect `UNKNOWN`.
> ```
>   clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
>   clang_analyzer_eval(a < -1); // expected-warning{{UNKNOWN}}
> ```
> 
> I also believe that `a >= INT_MIN` is `TRUE` for all `a` anyway, thus it can 
> be omitted.
> Checking against equality won't help much either, because we had no equality 
> check before, thus it should result in `FALSE` unconditionally.
Okay, I changed it to something very obvious:
```
  clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
  clang_analyzer_eval(a > INT_MIN); // expected-warning{{TRUE}}
```



Comment at: clang/test/Analysis/constraint_manager_negate.c:46
+return;
+  clang_analyzer_eval(-a == INT_MIN); // expected-warning{{TRUE}}
+}

steakhal wrote:
> Let's also assert `a == INT_MIN` just for the symmetry.
That would be superfluous in my opinion, since I have changed the `if` to an
```
  assert(a == INT_MIN);
```



Comment at: clang/test/Analysis/constraint_manager_negate.c:76
+void negate_unsigned_min(unsigned a) {
+  if (a == UINT_MIN) {
+clang_analyzer_eval(-a == UINT_MIN); // expected-warning{{TRUE}}

steakhal wrote:
> Well, this is the third form of expressing constraints.
> 1) Using asserts.
> 2) Use ifs but with early returns.
> 3) Use ifs but embed the code into the true branch.
> 
> Please, settle on one method.
Ok, I changed to `assert` everywhere.



Comment at: clang/test/Analysis/constraint_manager_negate.c:87-88
+  if (a == 4u) {
+clang_analyzer_eval(-a == 4u); // expected-warning{{FALSE}}
+clang_analyzer_eval(-a != 4u); // expected-warning{{TRUE}}
+  }

steakhal wrote:
> Add an equality comparison which results in `TRUE`.
Sure.



Comment at: clang/test/Analysis/constraint_manager_negate.c:100-105
+void negate_unsigned_mid2(unsigned a) {
+  if (a < UINT_MID && a > UINT_MIN) {
+clang_analyzer_eval(-a > UINT_MID); // expected-warning{{TRUE}}
+clang_analyzer_eval(-a < UINT_MID); // expected-warning{{FALSE}}
+  }
+}

steakhal wrote:
> I believe the eval query can be sharper than this.
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

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

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

The bugreports look promising. However, I think we desperately need a note that 
describes which function has set the `errno_modeling` state. Below I'd expect 
the following notes for the highlighted function call.

- assuming return value of `mkdir` is in range [0, INT_MAX]
- errno is not set

F23040017: image.png 

I suppose, you'd need to add some changes for these both in the ErrnoChecker 
and here as well.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:435
+  assert(Cond);
+  State = State->assume(*Cond, true);
+  return errno_modeling::setErrnoValue(State, C.getLocationContext(),

Please check if `State` can be nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
 
+  const bool Foreign = false; // From CTU.
+

xazax.hun wrote:
> martong wrote:
> > martong wrote:
> > > xazax.hun wrote:
> > > > I feel that we use different terms for the imported declarations. 
> > > > Sometimes we call them `new`, sometimes `imported`, sometimes 
> > > > `foreign`. In case all of these means the same thing, it would be nice 
> > > > to standardize on a single way of naming. If there is a subtle 
> > > > difference between them, let's document that in a comment. It would be 
> > > > nice if we did not need the comment after the declaration but it would 
> > > > be obvious from the variable name.
> > > Yes, I agree that this should deserver some more explanation. Maybe right 
> > > above this declaration?
> > > 
> > > So, `new` means that a declaration is **created** newly by the 
> > > ASTImporter.
> > > `imported` means it has been imported, but not necessarily `new`. Think 
> > > about this case, we import `foo`'s definition.
> > > ```
> > > // to.cpp
> > > void bar() {} // from a.h
> > > // from.cpp
> > > void bar() {} // from a.h
> > > void foo() {
> > >   bar();
> > > }
> > > ```
> > > Then `foo` will be `new` and `imported`, `bar` will be `imported` and not 
> > > `new`.  
> > > `foreign` basically means `imported` and `new`.
> > I've just added an explanatory comment for this field.
> Foreign means new and imported. But is there a way for a declaration to be 
> new and not to be imported? If no, in that case it feels like new and foreign 
> are actually the same and we should standardize on a single name.
Yeah, you are right, `new` and `foreign` are the same in this sense. However, I 
think the term `foreign` is more expressive in the sense that is suggests that 
the definition is coming from another translation unit.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.

xazax.hun wrote:
> xazax.hun wrote:
> > martong wrote:
> > > xazax.hun wrote:
> > > > So if we see the same foreign function called in multiple contexts, we 
> > > > will only queue one of the contexts for the CTU. Is this the intended 
> > > > design? 
> > > > 
> > > > So if I see:
> > > > ```
> > > > foreign(true);
> > > > foreign(false);
> > > > ```
> > > > 
> > > > The new CTU will only evaluate `foreign(true)` but not 
> > > > `foreign(false)`. 
> > > This is intentional.
> > > ```
> > > foreign(true);
> > > foreign(false);
> > > ```
> > > The new CTU will evaluate the following paths in the exploded graph:
> > > ```
> > > foreign(true); // conservative evaluated
> > > foreign(false); // conservative evaluated
> > > foreign(true); // inlined
> > > foreign(false); // inlined
> > > ```
> > > The point is to keep bifurcation to a minimum and avoid the exponential 
> > > blow up.
> > > So, we will never have a path like this:
> > > ```
> > > foreign(true); // conservative evaluated
> > > foreign(false); // inlined
> > > ```
> > > 
> > > Actually, this is the same strategy that we use during the dynamic 
> > > dispatch of C++ virtual calls. See `DynamicDispatchBifurcationMap`.
> > > 
> > > The conservative evaluation happens in the first phase, the inlining in 
> > > the second phase (assuming the phase1 inlining option is set to none).
> > > The new CTU will evaluate the following paths in the exploded graph:
> > > ```
> > > foreign(true); // conservative evaluated
> > > foreign(false); // conservative evaluated
> > > foreign(true); // inlined
> > > foreign(false); // inlined
> > > ```
> > 
> > When we encounter `foreign(true)`, we would add the decl to 
> > `CTUDispatchBifurcationSet`. So the second time we see a call to the 
> > function `foreign(false);`, we will just do the conservative evaluation and 
> > will not add the call to the CTU worklist. So how will `foreign(false);` be 
> > inlined in the second pass? Do I miss something? 
> > 
> Oh, I think I now understand. Do you expect `foreign(false);` to be inlined 
> after we return from `foreign(true);` in the second pass? Sorry for the 
> confusion, in that case it looks good to me.
> Oh, I think I now understand. Do you expect `foreign(false);` to be inlined 
> after we return from `foreign(true);` in the second pass?

Yes, that's right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

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

[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice work! Could you pleas add some lit tests that describe an errno related 
bugreport for a standard lib function?




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:381
 
+  class ErrnoConstraintKind {
+  public:

This class serves as a base class, it is not really a `Kind` class which is 
usually just an enum.
I'd prefer to call this `ErrnoConstraintBase` or `BasicErrnoConstraint`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:383
+  public:
+virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
+  const Summary ,

This class is a polymorphic base class, since we have this virtual function 
here. Would make sense to make destructor virtual as well, even though delete 
is not called explicitly on the base class (as I see).



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:397
+
+  class SingleValueErrnoConstraint : public ErrnoConstraintKind {
+uint64_t const ErrnoValue;

Please document the subclasses.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:603
 
-Summary (ConstraintSet &, StringRef Note = "") {
-  Cases.push_back(SummaryCase(std::move(CS), Note));
+Summary (ConstraintSet &, const ErrnoConstraintKind ,
+  StringRef Note = "") {

Would it make sense to have a `ErrnoIrrelevant` as the default value for 
`ErrnoC`?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:719-722
+  NoErrnoConstraint ErrnoIrrelevant;
+  SuccessErrnoConstraint ErrnoMustNotBeChecked;
+  ZeroRelatedErrnoConstraint ErrnoNEZeroIrrelevant{
+  clang::BinaryOperatorKind::BO_NE, errno_modeling::Errno_Irrelevant};

Could you please add some more documentation to these variables? And they could 
be `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400

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

[PATCH] D125360: [analyzer] Add taint to the BoolAssignmentChecker

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125360

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

[PATCH] D124674: [analyzer] Indicate if a parent state is infeasible

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp:46
+ConstraintManager::ProgramStatePair
+ConstraintManager::assumeDual(ProgramStateRef State, DefinedSVal Cond) {
+  ProgramStateRef StTrue = assume(State, Cond, true);

TODO We should have a very similar implementation for 
`assumeInclusiveRangeDual`! (Not that high prio, that is used only by the 
BoolAssignmentChecker).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124674

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

[PATCH] D125340: [clang][NFC][AST] rename the ImportError to ASTImportError

[Gabor Marton via Phabricator via cfe-commits]

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

Yeah, okay, this patch makes sense now that I've seen a clash with python's 
ImportError  . I've checked 
lldb c++ files and `ImportError` is not used. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125340

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

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Fixes https://github.com/llvm/llvm-project/issues/55241


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

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

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 428683.
martong added a comment.

- Add false positive test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125395

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate.c
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/test/Analysis/unary-sym-expr.c

Index: clang/test/Analysis/unary-sym-expr.c
===
--- clang/test/Analysis/unary-sym-expr.c
+++ clang/test/Analysis/unary-sym-expr.c
@@ -28,3 +28,12 @@
 return;
   clang_analyzer_eval(-(x + y) == -3); // expected-warning{{TRUE}}
 }
+
+int test_fp(int flag) {
+  int value;
+  if (flag == 0)
+value = 1;
+  if (-flag == 0)
+return value; // no-warning
+  return 42;
+}
Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -87,6 +89,7 @@
   clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{FALSE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void negate_int_min(int m, int n) {
   if (m - n != INT_MIN)
 return;
@@ -106,11 +109,11 @@
   clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void effective_range_2(int m, int n) {
   assert(m - n <= 0);
   assert(n - m <= 0);
-  clang_analyzer_eval(m - n == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
-  clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  clang_analyzer_eval(m - n == 0 || m - n == INT_MIN); // expected-warning{{TRUE}}
 }
 
 void negate_unsigned_min(unsigned m, unsigned n) {
@@ -122,6 +125,15 @@
   }
 }
 
+_Static_assert(7u - 3u != 3u - 7u, "");
+void negate_unsigned_4(unsigned m, unsigned n) {
+  if (m - n == 4u) {
+clang_analyzer_eval(n - m == 4u); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != 4u); // expected-warning{{TRUE}}
+  }
+}
+
+_Static_assert(UINT_MID == -UINT_MID, "");
 void negate_unsigned_mid(unsigned m, unsigned n) {
   if (m - n == UINT_MID) {
 clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}}
@@ -136,6 +148,8 @@
   }
 }
 
+_Static_assert(1u - 2u == UINT_MAX, "");
+_Static_assert(2u - 1u == 1, "");
 void negate_unsigned_max(unsigned m, unsigned n) {
   if (m - n == UINT_MAX) {
 clang_analyzer_eval(n - m == 1); // expected-warning{{TRUE}}
@@ -152,8 +166,8 @@
 
 // The next code is a repro for the bug PR41588
 void negated_unsigned_range(unsigned x, unsigned y) {
-  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
-  clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(y - x != 0); // expected-warning{{UNKNOWN}}
   // expected no assertion on the next line
-  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{UNKNOWN}}
 }
Index: clang/test/Analysis/constraint_manager_negate.c
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_negate.c
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+void exit(int);
+
+#define UINT_MIN (0U)
+#define UINT_MAX (~UINT_MIN)
+#define UINT_MID (UINT_MAX / 2 + 1)
+#define INT_MAX (UINT_MAX & (UINT_MAX >> 1))
+#define INT_MIN (UINT_MAX & ~(UINT_MAX >> 1))
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+unsigned int __line, __const char *__function)
+ __attribute__ ((__noreturn__));
+#define assert(expr) \
+  ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+void negate_positive_range(int a) {
+  if (-a <= 0)
+return;
+  // -a: [1, INT_MAX]
+  // a: [INT_MIN, -1]
+  clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a >= INT_MIN); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a == INT_MIN); // expected-warning{{FALSE}}
+}
+
+void negate_positive_range2(int a) {
+  if (a <= 0)
+return;
+  // a: [1, INT_MAX]
+  // -a: [INT_MIN, -1]
+  

[PATCH] D125395: [analyzer][solver] Handle UnarySymExpr in RangeConstraintSolver

[Gabor Marton via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125395

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

Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -87,6 +89,7 @@
   clang_analyzer_eval(n - m == INT_MIN); // expected-warning{{FALSE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void negate_int_min(int m, int n) {
   if (m - n != INT_MIN)
 return;
@@ -106,11 +109,11 @@
   clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}}
 }
 
+_Static_assert(INT_MIN == -INT_MIN, "");
 void effective_range_2(int m, int n) {
   assert(m - n <= 0);
   assert(n - m <= 0);
-  clang_analyzer_eval(m - n == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
-  clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  clang_analyzer_eval(m - n == 0 || m - n == INT_MIN); // expected-warning{{TRUE}}
 }
 
 void negate_unsigned_min(unsigned m, unsigned n) {
@@ -122,6 +125,15 @@
   }
 }
 
+_Static_assert(7u - 3u != 3u - 7u, "");
+void negate_unsigned_4(unsigned m, unsigned n) {
+  if (m - n == 4u) {
+clang_analyzer_eval(n - m == 4u); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != 4u); // expected-warning{{TRUE}}
+  }
+}
+
+_Static_assert(UINT_MID == -UINT_MID, "");
 void negate_unsigned_mid(unsigned m, unsigned n) {
   if (m - n == UINT_MID) {
 clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}}
@@ -136,6 +148,8 @@
   }
 }
 
+_Static_assert(1u - 2u == UINT_MAX, "");
+_Static_assert(2u - 1u == 1, "");
 void negate_unsigned_max(unsigned m, unsigned n) {
   if (m - n == UINT_MAX) {
 clang_analyzer_eval(n - m == 1); // expected-warning{{TRUE}}
@@ -152,8 +166,8 @@
 
 // The next code is a repro for the bug PR41588
 void negated_unsigned_range(unsigned x, unsigned y) {
-  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
-  clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(y - x != 0); // expected-warning{{UNKNOWN}}
   // expected no assertion on the next line
-  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{UNKNOWN}}
 }
Index: clang/test/Analysis/constraint_manager_negate.c
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_negate.c
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+
+void exit(int);
+
+#define UINT_MIN (0U)
+#define UINT_MAX (~UINT_MIN)
+#define UINT_MID (UINT_MAX / 2 + 1)
+#define INT_MAX (UINT_MAX & (UINT_MAX >> 1))
+#define INT_MIN (UINT_MAX & ~(UINT_MAX >> 1))
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+unsigned int __line, __const char *__function)
+ __attribute__ ((__noreturn__));
+#define assert(expr) \
+  ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+void negate_positive_range(int a) {
+  if (-a <= 0)
+return;
+  // -a: [1, INT_MAX]
+  // a: [INT_MIN, -1]
+  clang_analyzer_eval(a < 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a >= INT_MIN); // expected-warning{{TRUE}}
+  clang_analyzer_eval(a == INT_MIN); // expected-warning{{FALSE}}
+}
+
+void negate_positive_range2(int a) {
+  if (a <= 0)
+return;
+  // a: [1, INT_MAX]
+  // -a: [INT_MIN, -1]
+  clang_analyzer_eval(-a < 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(-a >= INT_MIN); // expected-warning{{TRUE}}
+  clang_analyzer_eval(-a == INT_MIN); // expected-warning{{FALSE}}
+}
+
+_Static_assert(INT_MIN == -INT_MIN, "");

[PATCH] D125379: [analyzer][solver] Do not negate unsigned ranges

[Gabor Marton via Phabricator via cfe-commits]

martong abandoned this revision.
martong added inline comments.



Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:125-130
 void negate_unsigned_mid(unsigned m, unsigned n) {
   if (m - n == UINT_MID) {
-clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
   }
 }

Actually, this test case was correct, because UINT_MID is a special value and 
for that
```
_Static_assert(UINT_MID == -UINT_MID, "");
```
holds.
So, this patch is meaningless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125379

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

[PATCH] D125379: [analyzer][solver] Do not negate unsigned ranges

[Gabor Marton via Phabricator via cfe-commits]

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

This is a bugfix. Simply put, 2u - 1u != 2u - 1u. See the static
assertion in the test file. The fix simply ban the negation of unsigned
expressions. This way the we are getting a little bit more conservatie,
but at least we do not infer wrong values.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125379

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


Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -122,31 +122,37 @@
   }
 }
 
+_Static_assert(12u - 1u != 1u - 12u, "Good modulo arithmetic");
 void negate_unsigned_mid(unsigned m, unsigned n) {
   if (m - n == UINT_MID) {
-clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
   }
 }
 
 void negate_unsigned_mid2(unsigned m, unsigned n) {
   if (m - n < UINT_MID && m - n > UINT_MIN) {
-clang_analyzer_eval(n - m > UINT_MID); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m < UINT_MID); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m > UINT_MID); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
+clang_analyzer_eval(n - m < UINT_MID); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
   }
 }
 
+
+_Static_assert(1u - 2u == UINT_MAX, "Good modulo arithmetic");
+_Static_assert(2u - 1u == 1, "Good modulo arithmetic");
 void negate_unsigned_max(unsigned m, unsigned n) {
   if (m - n == UINT_MAX) {
-clang_analyzer_eval(n - m == 1); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m != 1); // expected-warning{{FALSE}}
+// FIXME only the TRUE case should appear. But it is better to be
+// conservative than faulty.
+clang_analyzer_eval(n - m == 1); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != 1); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
   }
 }
-
 void negate_unsigned_one(unsigned m, unsigned n) {
   if (m - n == 1) {
-clang_analyzer_eval(n - m == UINT_MAX); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m < UINT_MAX);  // expected-warning{{FALSE}}
+// FIXME only the TRUE case should appear.
+clang_analyzer_eval(n - m == UINT_MAX); // expected-warning{{TRUE}} 
expected-warning{{FALSE}}
+clang_analyzer_eval(n - m < UINT_MAX);  // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
   }
 }
 
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -1454,8 +1454,7 @@
 QualType T = Sym->getType();
 
 // Do not negate unsigned ranges
-if (!T->isUnsignedIntegerOrEnumerationType() &&
-!T->isSignedIntegerOrEnumerationType())
+if (T->isUnsignedIntegerOrEnumerationType())
   return llvm::None;
 
 SymbolManager  = State->getSymbolManager();


Index: clang/test/Analysis/constraint_manager_negate_difference.c
===
--- clang/test/Analysis/constraint_manager_negate_difference.c
+++ clang/test/Analysis/constraint_manager_negate_difference.c
@@ -122,31 +122,37 @@
   }
 }
 
+_Static_assert(12u - 1u != 1u - 12u, "Good modulo arithmetic");
 void negate_unsigned_mid(unsigned m, unsigned n) {
   if (m - n == UINT_MID) {
-clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m == UINT_MID); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(n - m != UINT_MID); // expected-warning{{FALSE}} expected-warning{{TRUE}}
   }
 }
 
 void negate_unsigned_mid2(unsigned m, unsigned n) {
   if (m - n < UINT_MID && m - n > UINT_MIN) {
-clang_analyzer_eval(n - m > UINT_MID); // expected-warning{{TRUE}}
-clang_analyzer_eval(n - m < UINT_MID); // expected-warning{{FALSE}}
+clang_analyzer_eval(n - m > UINT_MID); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+

[PATCH] D125340: [clang][NFC][AST] rename the ImportError to ASTImportError

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

What are the benefits of this renaming? I mean is there a name clash? Do we 
have another kind of "import" in Clang or in some of the dependent projects, 
don't we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125340

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