[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

I've made some performance measurements on https://github.com/vim/vim project.
Since I'm using Windows I had no chance to do a lot of test on different 
projects, because of Unix-environment dependency.
I've tested the most weighty files running `clang --analyze -Xclang "-Iproto" 
-Xclang -analyzer-stats src/file.c` on each.
In this way I've tested 84 files. And here is a result table (Each row contains 
a result of testing an individual c/cpp-file).F11958819: 
performance_test_vim.xlsx 
Total results are:
test 1

| Before | After  | Delta |
| 770,9s | 760,8s | 1,31% |
|

test 2

| Before | After  | Delta |
| 791,4s | 771,8s | 2,48% |
|




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

https://reviews.llvm.org/D78933



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Folk,
please look at this patch. It has been hanging for a time here. We should 
finally make a decision about it.


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

https://reviews.llvm.org/D77062



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

[PATCH] D80117: [analyzer] Introduce reasoning about symbolic remainder operator

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko
I've made some assumptions.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:459-469
+if (Origin.From().isMinSignedValue()) {
+  // If mini is a minimal signed value, absolute value of it is greater
+  // than the maximal signed value.  In order to avoid these
+  // complications, we simply return the whole range.
+  return {ValueFactory.getMinValue(RangeType),
+  ValueFactory.getMaxValue(RangeType)};
+}

I think you should swap `if` statements. I'll explain.
Let's consider the input is an **uint8** range [42, 242] and you will return 
[0, 242] in the second `if`.
But if the input is an **uint8** range [128, 242] you will return [0, 255] in 
the first `if`, because 128 is an equivalent of -128(INT8_MIN) in binary 
representation so the condition in the first if would be true.
What is the great difference between [42, 242] and [128, 242] to have different 
results? Or you've just missed this case?

P.S. I think your function's name doesn't fit its body, since //absolute 
value// is always positive (without sign) from its definition, but you output 
range may have negative values. You'd better write an explanation above the 
function and rename it.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:481
+//   * Otherwise, From <= 0, To >= 0, and
+// AbsMax == max(abs(From), abs(To))
+llvm::APSInt AbsMax = std::max(-Origin.From(), Origin.To());

As for me, the last  //reason// fully covers previous special cases, so you can 
omit those ones, thus simplify the comment.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:659
+  //
+  // If we are dealing with unsigned case, we shouldn't move the lower bound.
+  if (Min.isSigned()) {

Extend the comment, please, why we should move bounds to zero at all.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:684
+
+  return {RangeFactory, ValueFactory.getValue(Min), 
ValueFactory.getValue(Max)};
+}

Is it OK to return this rangeset in case when one of operands(or both) is 
negative, since this rangeset can vary from specific implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80117



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Just a ping.


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

https://reviews.llvm.org/D77802



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ many thanks. I'll land it.


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

https://reviews.llvm.org/D77802



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGba92b274225f: [analyzer] Improved RangeSet::Negate support 
of unsigned ranges (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77802

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/RangeSetTest.cpp

Index: clang/unittests/StaticAnalyzer/RangeSetTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/RangeSetTest.cpp
@@ -0,0 +1,130 @@
+//===- unittests/StaticAnalyzer/RangeSetTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// TestCase contains to lists of ranges.
+// Original one has to be negated.
+// Expected one has to be compared to negated original range.
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;
+
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),
+expected(createRangeSetFromList(BVF, F, expectedList)) {}
+
+private:
+  RangeSet createRangeSetFromList(BasicValueFactory &BVF, RangeSet::Factory &F,
+  const std::initializer_list rangeList) {
+llvm::APSInt from(sizeof(T) * 8, std::is_unsigned::value);
+llvm::APSInt to = from;
+RangeSet rangeSet = F.getEmptySet();
+for (auto it = rangeList.begin(); it != rangeList.end(); it += 2) {
+  from = *it;
+  to = *(it + 1);
+  rangeSet = rangeSet.addRange(
+  F, RangeSet(F, BVF.getValue(from), BVF.getValue(to)));
+}
+return rangeSet;
+  }
+
+  void printNegate(const TestCase &TestCase) {
+TestCase.original.print(llvm::dbgs());
+llvm::dbgs() << " => ";
+TestCase.expected.print(llvm::dbgs());
+  }
+};
+
+class RangeSetTest : public testing::Test {
+protected:
+  // Init block
+  std::unique_ptr AST = tooling::buildASTFromCode("struct foo;");
+  ASTContext &context = AST->getASTContext();
+  llvm::BumpPtrAllocator alloc;
+  BasicValueFactory BVF{context, alloc};
+  RangeSet::Factory F;
+  // End init block
+
+  template  void checkNegate() {
+using type = T;
+
+// Use next values of the range {MIN, A, B, MID, C, D, MAX}.
+
+// MID is a value in the middle of the range
+// which unary minus does not affect on,
+// e.g. int8/int32(0), uint8(128), uint32(2147483648).
+
+constexpr type MIN = std::numeric_limits::min();
+constexpr type MAX = std::numeric_limits::max();
+constexpr type MID = std::is_signed::value
+ ? 0
+ : ~(static_cast(-1) / static_cast(2));
+constexpr type A = MID - static_cast(42 + 42);
+constexpr type B = MID - static_cast(42);
+constexpr type C = -B;
+constexpr type D = -A;
+
+static_assert(MIN < A && A < B && B < MID && MID < C && C < D && D < MAX,
+  "Values shall be in an ascending order");
+
+// Left {[x, y], [x, y]} is what shall be negated.
+// Right {[x, y], [x, y]} is what shall be compared to a negation result.
+TestCase cases[] = {
+{BVF, F, {MIN, A}, {MIN, MIN, D, MAX}},
+{BVF, F, {MIN, C}, {MIN, MIN, B, MAX}},
+{BVF, F, {MIN, MID}, {MIN, MIN, MID, MAX}},
+{BVF, F, {MIN, MAX}, {MIN, MAX}},
+{BVF, F, {A, D}, {A, D}},
+{BVF, F, {A, B}, {C, D}},
+{BVF, F, {MIN, A, D, MAX}, {MIN, A, D, MAX}},
+{BVF, F, {MIN, B, MID, D}, {MIN, MIN, A, MID, C, MAX}},
+{BVF, F, {MIN, MID, C, D}, {MIN, MIN, A, B, MID, MAX}},
+{BVF, F, {MIN, MID, C, MAX}, {MIN, B, MID, MAX}},
+{BVF, F, {A, MID, D, MAX}, {MIN + 1, A, MID, D}},
+{BVF, F, {A, A}, {D, D}},
+{BVF, F, {MID, MID}, {MID, MID}},
+{BVF, F, {MAX, MAX}, {MIN + 1, MIN + 1}},
+};
+
+for (const auto &c : cases) {
+  // Negate original and check with expected.
+  RangeSet negatedFromOriginal = c.original.Negate(BVF, F);
+  EXPECT_EQ(negatedFromOriginal, c.expected);
+  // Negate negated back and check with original.
+  RangeSet nega

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@xazax.hun, any thoughts?


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

https://reviews.llvm.org/D78933



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

[PATCH] D80444: [analyzer] Add support for IE of keyboard and mouse navigation in HTML report

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6bbaa62d26b6: [analyzer] Add support for IE of keyboard and 
mouse navigation in HTML report (authored by ASDenysPetrov).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80444

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


Index: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
@@ -1070,8 +1070,13 @@
 

[PATCH] D78442: Create a warning flag for 'warn_conv_*_not_used'

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

I suddenly discovered you unnoticed revision. I'd advise you to add more 
reviewers or subscribers here.


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

https://reviews.llvm.org/D78442



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 267185.
ASDenysPetrov added a comment.

Updated. Rebased with D79232 .
@vsavchenko, @NoQ, @xazax.hun  please look.


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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,213 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // ex

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@xazax.hun

> I think we should check it on some more projects. We saw vastly different 
> analyzer behavior on other projects in the past.

I completely agree with you. But, unfortunately, vim-proj is the only I could 
squeeze from that bunch.

> I think you should be able to run it on any cmake project that supports 
> windows by generating compilation database and using scan-build-py (in case 
> other methods did not work).

I'll appreciate if you'd give some of such projects. Another point is that I 
don't know how to get printed stats from the //scan-build//.

> But I wonder how would this compare to a solution that also handles 
> transitivity. We did not really look into the alternatives yet.

I've read an article about Octagons you gave me. The article covers much bigger 
and more complicated scope, then mine solution. For now I'm not really ready to 
enlarge it, as it needs a deep investigation. I think for now it can be 
accepted as an intermadiate proposal.


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

https://reviews.llvm.org/D78933



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 6 inline comments as done.
ASDenysPetrov added a comment.

Do not apologize for syntax, language or code style remarks. Them make code 
better and it's really important for me to know about my faults.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:80
+
+  static size_t IndexFromOp(BinaryOperatorKind OP) {
+return static_cast(OP - BO_LT);

vsavchenko wrote:
> I would prefer function names to comply with LLVM coding standards (start 
> with a verb and a lowercase letter
> https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
I'll add a **get **prefix. Saying about lower/uppercases, I'd say this is a 
mess inside this file. I've just decided to use an upper one.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:665
+
+auto SSE = dyn_cast(Sym);
+if (!SSE)

vsavchenko wrote:
> I believe that when we use auto we still try to be more verbose with it, so 
> in this case it should be smth like `const auto *SSE`
I'll do.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:689
+
+// Loop goes through the all columns except the last `UnknownX2`
+// We treat `UnknownX2` column separately at the end of the loop body.

vsavchenko wrote:
> Sorry for nitpicking, but it seems like the grammar is a bit off in the **the 
> all columns except the last** part
Do you mean to change to **all of the columns exept the last one 
('UnknownX2')**?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690
+// Loop goes through the all columns except the last `UnknownX2`
+// We treat `UnknownX2` column separately at the end of the loop body.
+for (size_t i = 0; i < CmpOpTable.GetCmpOpCount(); ++i) {

vsavchenko wrote:
> I think that **treat** is not the best option here. Maybe smith like 
> **process** will do?
I met a lot of **treat ** usage in terms of **handle/process** among the code, 
so just followed it.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:696
+  const SymSymExpr *SymSym = SymMgr.getSymSymExpr(LHS, QueriedOP, RHS, T);
+  const RangeSet *RangeSet = State->get(SymSym);
+

vsavchenko wrote:
> Maybe we can name this constant somehow differently to be more verbose? 
What do you think about **FoundRangeSet**?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:700-705
+  if (!RangeSet) {
+const BinaryOperatorKind ROP =
+BinaryOperator::reverseComparisonOp(QueriedOP);
+SymSym = SymMgr.getSymSymExpr(RHS, ROP, LHS, T);
+RangeSet = State->get(SymSym);
+  }

vsavchenko wrote:
> Please, correct me if I'm wrong, but I thought that we are iterating over 
> //all// comparison operators (excluding `<=>`), which means that if we don't 
> find it on this iteration for let's say `x < y` then we'll find it (or 
> already did) for `x > y`.  So, my question is - why do we have this clause 
> then?
> 
> And it confuses me that `RangeSet` now corresponds to a //reversed// 
> operator, while `QueriedOP` remains the same.
> 
> Maybe a good commentary explaining why we need it could help!
No-no. Please, look carefully.
At first we try to find `X op Y`, when **op** is a comparison operator. If we 
failed then try to find the reversed (flipped) version of the expression `Y 
reversed(op) X`.
Examples: `x > y and y < x`, `x != y and y != x`, `x <= y and y >= x`

We don't care about operand positions, we just want to find any of its previous 
relation, and which branch of it we are now in.


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

https://reviews.llvm.org/D78933



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 267587.
ASDenysPetrov added a comment.

Updated due to comments.


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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,213 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); 

[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 278445.
ASDenysPetrov marked an inline comment as done.
ASDenysPetrov added a comment.

Changed naming due to LLVM rules.
I decided not to change names everywhere to leave the patch more readable.


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

https://reviews.llvm.org/D77062

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/string.c

Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -363,6 +363,15 @@
 strcpy(x, y); // no-warning
 }
 
+// PR37503
+void *func_strcpy_no_assertion();
+char ***ptr_strcpy_no_assertion;
+void strcpy_no_assertion() {
+  *(unsigned char **)ptr_strcpy_no_assertion = (unsigned char *)(func_strcpy_no_assertion());
+  char c;
+  strcpy(**ptr_strcpy_no_assertion, &c); // no-assertion
+}
+
 //===--===
 // stpcpy()
 //===--===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -196,9 +196,8 @@
   void evalBzero(CheckerContext &C, const CallExpr *CE) const;
 
   // Utility methods
-  std::pair
-  static assumeZero(CheckerContext &C,
-ProgramStateRef state, SVal V, QualType Ty);
+  static std::pair
+  assumeZero(ProgramStateRef State, SVal V);
 
   static ProgramStateRef setCStringLength(ProgramStateRef state,
   const MemRegion *MR,
@@ -279,16 +278,18 @@
 // Individual checks and utility methods.
 //===--===//
 
-std::pair
-CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
-   QualType Ty) {
-  Optional val = V.getAs();
-  if (!val)
-return std::pair(state, state);
+std::pair
+CStringChecker::assumeZero(ProgramStateRef State, SVal V) {
+  auto States = std::make_pair(State, State);
 
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
-  return state->assume(svalBuilder.evalEQ(state, *val, zero));
+  Optional Val = V.getAs();
+  // FIXME: We should understand how LazyCompoundVal can be possible here,
+  // fix the root cause and get rid of this check.
+  if (Val && !V.getAs())
+// Returned pair shall be {null, non-null} so reorder states.
+std::tie(States.second, States.first) = State->assume(*Val);
+
+  return States;
 }
 
 ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
@@ -299,8 +300,7 @@
 return nullptr;
 
   ProgramStateRef stateNull, stateNonNull;
-  std::tie(stateNull, stateNonNull) =
-  assumeZero(C, State, l, Arg.Expression->getType());
+  std::tie(stateNull, stateNonNull) = assumeZero(State, l);
 
   if (stateNull && !stateNonNull) {
 if (Filter.CheckCStringNullArg) {
@@ -1071,8 +1071,7 @@
 CharVal = svalBuilder.evalCast(CharVal, Ctx.UnsignedCharTy, Ctx.IntTy);
 
 ProgramStateRef StateNullChar, StateNonNullChar;
-std::tie(StateNullChar, StateNonNullChar) =
-assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+std::tie(StateNullChar, StateNonNullChar) = assumeZero(State, CharVal);
 
 if (StateWholeReg && !StateNotWholeReg && StateNullChar &&
 !StateNonNullChar) {
@@ -1133,11 +1132,9 @@
   // See if the size argument is zero.
   const LocationContext *LCtx = C.getLocationContext();
   SVal sizeVal = state->getSVal(Size.Expression, LCtx);
-  QualType sizeTy = Size.Expression->getType();
 
   ProgramStateRef stateZeroSize, stateNonZeroSize;
-  std::tie(stateZeroSize, stateNonZeroSize) =
-  assumeZero(C, state, sizeVal, sizeTy);
+  std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(state, sizeVal);
 
   // Get the value of the Dest.
   SVal destVal = state->getSVal(Dest.Expression, LCtx);
@@ -1287,11 +1284,9 @@
 
   // See if the size argument is zero.
   SVal sizeVal = State->getSVal(Size.Expression, LCtx);
-  QualType sizeTy = Size.Expression->getType();
 
   ProgramStateRef stateZeroSize, stateNonZeroSize;
-  std::tie(stateZeroSize, stateNonZeroSize) =
-  assumeZero(C, State, sizeVal, sizeTy);
+  std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(State, sizeVal);
 
   // If the size can be zero, the result will be 0 in that case, and we don't
   // have to check either of the buffers.
@@ -1367,8 +1362,7 @@
 SVal maxlenVal = state->getSVal(maxlenExpr, LCtx);
 
 ProgramStateRef stateZeroSize, stateNonZeroSize;
-std::tie(stateZeroSize, stateNonZeroSize) =
-  assumeZero(C, state, maxlenVal, maxlenExpr->getType());
+std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(state, maxlenVal);
 

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ 
Yes, I have some thoughts. I am on revision master from [02.04.2020 10:29:20] 
(5 days ago). I've check these test and all passes on my PC.

As you can see in the log :

  scan-build: Removing directory 
'/c/src/llvm-project/out/gn/obj/clang/test/Analysis/scan-build/Output/plist_html_output.test.tmp.output_dir/2020-04-07-090114-54706-1'
 because it contains no reports.
  ^
  :7:1: note: possible intended match here
  scan-build: No bugs found.

That means that **Inputs/single_null_dereference.c** is somewhat differs from 
mine or clang behavior was changed since my revision.

> Do tests actually run on your machine when REQUIRES: shell is still in?

No they don't. They are marked as unsuported if **REQUIRES: shell** is in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ 
I've just pulled all from github and make a build. Then I ran the tests, and 
they successfully passed. 
Namely, **scan-build** analyzed inputs and create //stdout //with diagnostics. 
Then **filecheck **checked without fails. But in case of **buildbot **it didn't 
create any diagnostic //stdout//.

According to buildbot's logs I assume that smth is wrong on its side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D76768#1967129 , @Charusso wrote:

> I believe it is very strange on a Windows system to have multiple dots in a 
> file. The other issue could be the wildcard `/*/` in a path full of `\`s. The 
> LLVM `lit` (https://llvm.org/docs/CommandGuide/lit.html) has tons of 
> Windows-related shortcuts, which I have never seen being used, but could be 
> useful.


I've checked. There is no problem with dots and /*/. My command promt (Win10) 
handles them perfectly well. Mixing / and \ in paths is also acceptable and 
handles correctly.
For instance, Visual Studio creates file with multiple dots for projects as 
"Project1.vcxproj.filters". Also you can easely create the same directory.
Wildcard /*/ is a part of input syntax of unix utility **cat**, thus it works 
as well.

Is there any way to get to buildbot file system to compare test files with what 
is in the master branch? Particularly 
//C:\src\llvm-project\clang\test\Analysis\scan-build/Inputs/single_null_dereference.c//


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

> If the source was checked out incorrectly then most likely everything would 
> have been broken, not just our test. I'd rather suspect write permissions or 
> something, but then again, a lot of tests create directories and then write 
> into them. Dunno.

No, I mean the source itself could be different.
I don't think permissions is the case, because we saw the log when scan-build 
reported: //No bugs found.//.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@thakis

> 'perl' is not recognized as an internal or external command,

I think you got another problem, that Perl isn't just presented in your PATH. 
It is not the case for buildbot logs.

> perl wasn't required to run tests on Win for a long time. If we do want to 
> run these tests on Windows, could we make the config lit file check if perl 
> is available and set a feature and require that in the tests?

This is a good idea, to make tests more robust and friendly verbal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, baloghadamsoftware.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun.

This fixes https://bugs.llvm.org/show_bug.cgi?id=41588
RangeSet Negate function shall handle unsigned ranges as well as signed ones.
RangeSet getRangeForMinusSymbol function shall use wider variety of ranges, not 
only concrete value ranges.
RangeSet Intersect functions shall not produce assertions.

Changes:
Improved safety of RangeSet::Intersect function. Added isEmpty() check to 
prevent an assertion.
Added support of handling unsigned ranges to RangeSet::Negate and 
RangeSet::getRangeForMinusSymbol.
Extended RangeSet::getRangeForMinusSymbol to return not only range sets with 
single value [n,n], but with wide ranges [n,m].


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77802

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
@@ -110,3 +110,9 @@
   clang_analyzer_eval(m - n == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
   clang_analyzer_eval(n - m == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
 }
+
+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{{FALSE}} expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -155,11 +155,11 @@
 // or, alternatively, /removing/ all integers between Upper and Lower.
 RangeSet RangeSet::Intersect(BasicValueFactory &BV, Factory &F,
  llvm::APSInt Lower, llvm::APSInt Upper) const {
-  if (!pin(Lower, Upper))
-return F.getEmptySet();
-
   PrimRangeSet newRanges = F.getEmptySet();
 
+  if (isEmpty() || !pin(Lower, Upper))
+return newRanges;
+
   PrimRangeSet::iterator i = begin(), e = end();
   if (Lower <= Upper)
 IntersectInRange(BV, F, Lower, Upper, newRanges, i, e);
@@ -190,32 +190,54 @@
   return newRanges;
 }
 
-// Turn all [A, B] ranges to [-B, -A]. Ranges [MIN, B] are turned to range set
-// [MIN, MIN] U [-B, MAX], when MIN and MAX are the minimal and the maximal
-// signed values of the type.
+// Turn all [A, B] ranges to [-B, -A], when "-" is a C-like unary minus
+// operation under the values of the type.
+// Negate also restores disrupted ranges on bounds,
+// e.g. [MIN, B] => [MIN, MIN] U [-B, MAX] => [MIN, B]
 RangeSet RangeSet::Negate(BasicValueFactory &BV, Factory &F) const {
   PrimRangeSet newRanges = F.getEmptySet();
 
+  if (isEmpty())
+return newRanges;
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();
+  const llvm::APSInt &MIN = BV.getMinValue(sampleValue);
+  const llvm::APSInt &MAX = BV.getMaxValue(sampleValue);
+  bool hasNewRangesMinValue = false;
+
   for (iterator i = begin(), e = end(); i != e; ++i) {
-const llvm::APSInt &from = i->From(), &to = i->To();
-const llvm::APSInt &newTo = (from.isMinSignedValue() ?
- BV.getMaxValue(from) :
- BV.getValue(- from));
-if (to.isMaxSignedValue() && !newRanges.isEmpty() &&
-newRanges.begin()->From().isMinSignedValue()) {
-  assert(newRanges.begin()->To().isMinSignedValue() &&
- "Ranges should not overlap");
-  assert(!from.isMinSignedValue() && "Ranges should not overlap");
-  const llvm::APSInt &newFrom = newRanges.begin()->From();
-  newRanges =
-F.add(F.remove(newRanges, *newRanges.begin()), Range(newFrom, newTo));
-} else if (!to.isMinSignedValue()) {
-  const llvm::APSInt &newFrom = BV.getValue(- to);
-  newRanges = F.add(newRanges, Range(newFrom, newTo));
-}
-if (from.isMinSignedValue()) {
-  newRanges = F.add(newRanges, Range(BV.getMinValue(from),
- BV.getMinValue(from)));
+const llvm::APSInt &from = i->From();
+const llvm::APSInt &to = i->To();
+
+const bool isFromMinValue =
+isUnsigned ? from.isMinValue() : from.isMinSignedValue();
+const bool isToMinValue =
+isUnsigned ? to.isMinValue() : to.isMinSignedValue();
+
+// handle a special case for MIN value
+if (isFromMi

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@thakis
Let me explain what works for me.
This is in my **PATH**:

- C:\Perl\c\bin
- C:\Perl\perl\site\bin
- C:\Perl\perl\bin
- D:\llvm-project\buildn\bin

This is what in D:\llvm-project\buildn\bin (all this is a product of ninja+gcc):

- clang.exe
- FileCheck.exe
- scan-build.bat
- scan-build
- llvm-lit.py

No additional changes required (CMakeList.txt, bat editing, etc.).
Than I run command promt from arbitrary dir and run next: //llvm-lit.py 
D:/llvm-project/clang/test/Analysis/scan-build/exclude_directories.test//
Test passes. Done.
I hope it will help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ , 
sorry for the late response, was working on other patch.

> Why are we getting a compound value here?

We are getting **nonloc::SymbolVal** here, not a compound value, since //* 
*ptr// result is unknown after //*ptr = foo();// (look at the test sample).
The root cause was that we compared **nonloc** with Zero sval which was **loc** 
and we got an assertion as a result.
Here I've improved and simplified a bit and now it is handles OK without 
assertions and passes all the tests.
 What do you think, is it worth to be landed?


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

https://reviews.llvm.org/D77062



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

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@thakis,

> ^ This is not usually true. (But I think lit adds build/bin to the PATH for 
> tests, so it's possibly true for tests, which would be enough.)

AFAIK lit do not add anything to PATH. You should do it by yourself.

> If you run `del bin\*` followed by `make check-clang` (or `ninja check-clang` 
> or what have you), then I think bin/scan-build won't be built since it's not 
> a dependency of the check-clang target. It doesn't have to be a dependency on 
> non-Win, but on Win the execution flow is different due to the bat trampoline.

I did as you ask. Confirm your suggestion: scan-build have not appeared after 
`ninja check-clang`. All the tests except one passed. Log attached F11713079: 
log_ninja_check-clang.txt .
Intresting but scan-build tests haven't been run through `ninja check-clang`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76768



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@Szelethus, @NoQ
I've investigated graph.dot of the sample. F11723129: t37503.dot 

Here is a simplification:

1. SA thinks that `ptr` is a pointer with a structure 
`MemRegion->MemRegion->MemRegion->Element`
2. Then `*(unsigned char **)ptr = (unsigned char *)(func());` occures. Symbolic 
substitution happens to `ptr`.
3. After that SA thinks that `ptr` holds a symbolic value 
`MemRegion->MemRegion->Element` because of casts.
4. `**ptr` should lead us to `MemRegion->MemRegion->MemRegion` from C++ point 
of view, but dereferencing applies to substituted symbolic value from SA point 
of view and we finally get `MemRegion->MemRegion->Element`

As I see, this is not //treating the symptom//. This is exactly handling this 
particular case which is legal and may take place.

Another solution could be to check the first argument of `strcpy` for being 
actially a `char*` and show a warning otherwise.

Please, explain, what I could miss in my suggestions, because I'm less 
expertise than you, guys.


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

https://reviews.llvm.org/D77062



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal, thank you for your time!
@baloghadamsoftware

> Please add a bit more tests where we can see that the negated of unsigned 
> range of `[n..m]` is `UINT_MAX-m+1..UINT_MAX-n+1]`.

Yes. I am writing them. Thanks for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77802



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 257617.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.
Herald added a subscriber: mgorny.

Improved Negate function in terms of handling `[MIN,A]U[B,MAX] => 
[MIN,-B]U[-A,MAX]` (previously was `[MIN,MIN]U[MIN+1,-B]U[-A,MAX]`).
Added unit test for Negate function.


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

https://reviews.llvm.org/D77802

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/RangeSetTest.cpp

Index: clang/unittests/StaticAnalyzer/RangeSetTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/RangeSetTest.cpp
@@ -0,0 +1,138 @@
+//===- unittests/StaticAnalyzer/RangeSetTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// TestCase contains to lists of ranges
+// original one, has to be negated
+// expected one, has to be compared to negated original range
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;
+
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),
+expected(createRangeSetFromList(BVF, F, expectedList)) {}
+
+private:
+  RangeSet createRangeSetFromList(BasicValueFactory &BVF, RangeSet::Factory &F,
+  const std::initializer_list &rangeList) {
+llvm::APSInt from(sizeof(T) * 8, std::is_unsigned::value);
+llvm::APSInt to = from;
+RangeSet rangeSet = F.getEmptySet();
+for (auto it = rangeList.begin(); it != rangeList.end(); it += 2) {
+  from = *it;
+  to = *(it + 1);
+  rangeSet = rangeSet.addRange(
+  F, RangeSet(F, BVF.getValue(from), BVF.getValue(to)));
+}
+return rangeSet;
+  }
+};
+
+class RangeSetTest : public testing::Test {
+protected:
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;
+  FileManager FileMgr{FileSystemOpts};
+  SourceManager SM{Diag, FileMgr};
+  LangOptions LOpts;
+  IdentifierTable idents;
+  SelectorTable sels;
+  Builtin::Context builtins;
+  ASTContext context{LOpts, SM, idents, sels, builtins};
+  llvm::BumpPtrAllocator alloc;
+  BasicValueFactory BVF{context, alloc};
+  RangeSet::Factory F;
+  // end init block
+
+  template  void checkNegate() {
+using type = T;
+
+// use next values of the range {MIN, A, B, MID, C, D, MAX}
+
+// MID is a value in the middle of the range
+// which unary minus does not affect on
+// e.g. int8/int32(0), uint8(128), uint32(2147483648)
+
+constexpr type MIN = std::numeric_limits::min();
+constexpr type MAX = std::numeric_limits::max();
+constexpr type MID = std::is_signed::value
+ ? 0
+ : ~(static_cast(-1) / static_cast(2));
+constexpr type A = MID - static_cast(42 + 42);
+constexpr type B = MID - static_cast(42);
+constexpr type C = -B;
+constexpr type D = -A;
+
+static_assert(MIN < A && A < B && B < MID && MID < C && C < D && D < MAX,
+  "Values shall be in an ascending order");
+
+// left {[x, y], [x, y]} is what shall be negated
+// right {[x, y], [x, y]}  is what shall be compared to a negation result
+TestCase cases[] = {
+{BVF, F, {MIN, A}, {MIN, MIN, D, MAX}},
+{BVF, F, {MIN, C}, {MIN, MIN, B, MAX}},
+{BVF, F, {MIN, MID}, {MIN, MIN, MID, MAX}},
+{BVF, F, {MIN, MAX}, {MIN, MAX}},
+{BVF, F, {A, D}, {A, D}},
+{BVF, F, {A, B}, {C, D}},
+{BVF, F, {MIN, A, D, MAX}, {MIN, A, D, MAX}},
+{BVF, F, {MIN, B, MID, D}, {MIN, MIN, A, MID, C, MAX}},
+{BVF, F, {MIN, MID, C, D}, {MIN, MIN, A, B, MID, MAX}},
+{BVF, F, {MIN, MID, C, MAX}, {MIN, B, MID, MAX}},
+{BVF, F, {A, MID, D, MAX}, {MIN + 1, A, MID, D}},
+{BVF, F, {A, A}, {D, D}},
+{BVF, F, {MID, MID}, {MID, MID}},
+{BVF, F, {MAX, MAX}, {MIN + 1, MIN + 1}},
+};
+
+for (const auto &c : cases) {
+  // negate original and check with expected
+  RangeSet negatedFromOriginal = c.original.Negate(BVF, F);
+  

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@Szelethus

> This is the exploded graph for which code? The t37503.cpp file you uploaded 
> earlier doesn't have the functions/variables found here, nor does the test 
> case included in this patch.

This //html //corresponds to the test case in the patch. You can find it below 
in clang/test/Analysis/string.c. However `t37503.cpp` also contains the same 
snippet just with other names and it wouldn't change the graph.


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

https://reviews.llvm.org/D77062



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

[PATCH] D78289: [analyzer] Stability improvements for IteratorModeling functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: baloghadamsoftware, NoQ.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun.

Some functions paths may lead to crash.
Fixed ignoring check for nullptr before dereferencing.
Fixed using local variable outside the scope  through a pointer.
Fixed minor misspellings.

This patch covers a bug https://bugs.llvm.org/show_bug.cgi?id=41485


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78289

Files:
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp


Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,9 +422,14 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  if (!LPos || !RPos)
+return;
+
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
+if (!State)
+  return;
 auto &SymMgr = C.getSymbolManager();
 auto *LCtx = C.getLocationContext();
 RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
@@ -532,8 +537,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 


Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,9 +422,14 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  if (!LPos || !RPos)
+return;
+
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
+if (!State)
+  return;
 auto &SymMgr = C.getSymbolManager();
 auto *LCtx = C.getLocationContext();
 RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
@@ -532,8 +537,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 3 inline comments as done.
ASDenysPetrov added a comment.

@NoQ You concerns are absolutly justified. I agree with you. Let me tell what 
I'm thinking in inlines.

If you are interested in why the assertion happens, please, look D77062#1977613 





Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:273
   Optional val = V.getAs();
-  if (!val)
-return std::pair(state, state);
+  if (val && !V.getAs()) {
+ // return pair shall be {null, non-null} so reorder states

NoQ wrote:
> Basically `SVal::getAs<>` should not be used for discovering the type of the 
> value; only the exact representation that's used for the value of the type 
> that's already in your possession. Say, it's ok to use it to discriminate 
> between, say, compound value and lazy compound value. It's ok to use it to 
> discriminate between a concrete integer and a symbol. It's ok to use it do 
> discriminate between a known value and an unknown value. But if it's used for 
> discriminating between a compound value and a numeric symbol, i'm 99% sure 
> it's incorrect. You should already know the type from the AST before you even 
> obtain the value. It doesn't make sense to run the checker at all if the 
> function receives a structure. And if it doesn't receive the structure but 
> the run-time value is of structure type, then either the checker isn't 
> obtaining the value correctly or there's bug in path-sensitive analysis. 
> That's why i still believe you're only treating the symptoms. There's nothing 
> normal in the situation where "strcpy suddenly accepts a structure (or an 
> array) by value".
I'll remove `V.getAs()`. I mistakenly added this 
`V.getAs()` to keep code safe regardless of the 
checker intention and sanity. The point is that `state->assume(*val)` calls 
`ConstraintMgr->assumeDual` which finally calls 
`SimpleConstraintManager::assumeAux`. `assumeAux` handles all NonLoc kinds 
except **LazyCompoundValKind**and **CompoundValKind**(I missed to check it). In 
case of these two kinds it leads to `llvm_unreachable("'Assume' not implemented 
for this NonLoc");`. But for now I understand that I failed because 
**unreachable** means I might not have check for those kinds, since the 
function already takes on this work. 

However it is not an actual fix. The fix is `std::tie(states.second, 
states.first) = state->assume(*val);`. You can see the summary above.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2270-2278
   // Pro-actively check that argument types are safe to do arithmetic upon.
   // We do not want to crash if someone accidentally passes a structure
   // into, say, a C++ overload of any of these functions. We could not check
   // that for std::copy because they may have arguments of other types.
   for (auto I : CE->arguments()) {
 QualType T = I->getType();
 if (!T->isIntegralOrEnumerationType() && !T->isPointerType())

This check prevents passing **structures**. From this point we are sure to work 
with **pointers **and **integral **types in arguments:
E.g. `char *strcpy(int i1, int i2);`, `char *strcpy(char *c1, char *c2);`, 
`char *strcpy(Kind t1, Kind t2);`
This confirms @NoQ's supposition that checker narrows argument types.
P.S. Honestly I'd narrow this more aggressively to just a **char pointer 
**type, anyway it doesn't relate to the bug and I wouldn't add this to a single 
patch.


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

https://reviews.llvm.org/D77062



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 268424.
ASDenysPetrov added a comment.

Removed `V.getAs()` from `if`.


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

https://reviews.llvm.org/D77062

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/string.c


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -363,6 +363,14 @@
 strcpy(x, y); // no-warning
 }
 
+void *func_strcpy_no_assertion();
+char ***ptr_strcpy_no_assertion;
+void strcpy_no_assertion() {
+  *(unsigned char **)ptr_strcpy_no_assertion = (unsigned char 
*)(func_strcpy_no_assertion());
+  char c;
+  strcpy(**ptr_strcpy_no_assertion, &c); // no-assertion
+}
+
 //===--===
 // stpcpy()
 //===--===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -282,13 +282,15 @@
 std::pair
 CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
QualType Ty) {
+  auto states = std::make_pair(state, state);
+
   Optional val = V.getAs();
-  if (!val)
-return std::pair(state, state);
+  if (val) {
+// return pair shall be {null, non-null} so reorder states
+std::tie(states.second, states.first) = state->assume(*val);
+  }
 
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
-  return state->assume(svalBuilder.evalEQ(state, *val, zero));
+  return states;
 }
 
 ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -363,6 +363,14 @@
 strcpy(x, y); // no-warning
 }
 
+void *func_strcpy_no_assertion();
+char ***ptr_strcpy_no_assertion;
+void strcpy_no_assertion() {
+  *(unsigned char **)ptr_strcpy_no_assertion = (unsigned char *)(func_strcpy_no_assertion());
+  char c;
+  strcpy(**ptr_strcpy_no_assertion, &c); // no-assertion
+}
+
 //===--===
 // stpcpy()
 //===--===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -282,13 +282,15 @@
 std::pair
 CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
QualType Ty) {
+  auto states = std::make_pair(state, state);
+
   Optional val = V.getAs();
-  if (!val)
-return std::pair(state, state);
+  if (val) {
+// return pair shall be {null, non-null} so reorder states
+std::tie(states.second, states.first) = state->assume(*val);
+  }
 
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
-  return state->assume(svalBuilder.evalEQ(state, *val, zero));
+  return states;
 }
 
 ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: dcoughlin, NoQ, alexfh.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

Problem:
The issue is that UnknownVal is produced for an array element when it is used 
in expressions with unknown bounds and unknown index. Thus it doesn't bind in 
the list of Expressions and never be used twice then.

Solution:
Produce symbolic values for array elements instead of UnknownVal. This also 
enables to bind these values and use them later in the next expressions.

This fixes https://bugs.llvm.org/show_bug.cgi?id=9289


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81254

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/PR9289.cpp


Index: clang/test/Analysis/PR9289.cpp
===
--- /dev/null
+++ clang/test/Analysis/PR9289.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+int fun(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  if (a[index] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int fun2(const int **a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index][index2] < 2)
+var = 1;
+  if (a[index][index2] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int fun3(const int *a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  index = index2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined 
[core.uninitialized.Assign]}}
+  return ret;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1704,9 +1704,9 @@
   // FIXME: This is a hack, and doesn't do anything really intelligent yet.
   const RegionRawOffset &O = R->getAsArrayOffset();
 
-  // If we cannot reason about the offset, return an unknown value.
+  // If we cannot reason about the offset, return a symbolic value.
   if (!O.getRegion())
-return UnknownVal();
+return svalBuilder.getRegionValueSymbolVal(R);
 
   if (const TypedValueRegion *baseR =
 dyn_cast_or_null(O.getRegion())) {


Index: clang/test/Analysis/PR9289.cpp
===
--- /dev/null
+++ clang/test/Analysis/PR9289.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+int fun(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  if (a[index] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int fun2(const int **a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index][index2] < 2)
+var = 1;
+  if (a[index][index2] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int fun3(const int *a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  index = index2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1704,9 +1704,9 @@
   // FIXME: This is a hack, and doesn't do anything really intelligent yet.
   const RegionRawOffset &O = R->getAsArrayOffset();
 
-  // If we cannot reason about the offset, return an unknown value.
+  // If we cannot reason about the offset, return a symbolic value.
   if (!O.getRegion())
-return UnknownVal();
+return svalBuilder.getRegionValueSymbolVal(R);
 
   if (const TypedValueRegion *baseR =
 dyn_cast_or_null(O.getRegion())) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 269481.
ASDenysPetrov added a comment.

Added more tests changing an array element.


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

https://reviews.llvm.org/D81254

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/PR9289.cpp

Index: clang/test/Analysis/PR9289.cpp
===
--- /dev/null
+++ clang/test/Analysis/PR9289.cpp
@@ -0,0 +1,97 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+int index_sym(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  if (a[index] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int index_int(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[42] < 2)
+var = 1;
+  if (a[42] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int multi_ext_arr(const int **a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index][index2] < 2)
+var = 1;
+  if (a[index][index2] < 2)
+ret = var; // no warning about garbage value
+  return ret;
+}
+
+int index_sym_change(const int *a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  index = index2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
+
+int index_int_change(const int *a, int index, int index2) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  index = 42;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
+
+int element_sym_change(int *a, int index, int newValue) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  a[index] = newValue;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
+
+int element_int_change(int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  a[index] = 42;
+  if (a[index] < 2)
+ret = var; // no warning, branch does not execute
+  return ret;
+}
+
+int element_int_change2(int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  a[index] = 1;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
+
+int ptr_sym_change(int *a, int index, int *a2) {
+  int var;
+  int ret = 0;
+  if (a[index] < 2)
+var = 1;
+  a = a2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return ret;
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1704,9 +1704,9 @@
   // FIXME: This is a hack, and doesn't do anything really intelligent yet.
   const RegionRawOffset &O = R->getAsArrayOffset();
 
-  // If we cannot reason about the offset, return an unknown value.
+  // If we cannot reason about the offset, return a symbolic value.
   if (!O.getRegion())
-return UnknownVal();
+return svalBuilder.getRegionValueSymbolVal(R);
 
   if (const TypedValueRegion *baseR =
 dyn_cast_or_null(O.getRegion())) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1709
   if (!O.getRegion())
-return UnknownVal();
+return svalBuilder.getRegionValueSymbolVal(R);
 

NoQ wrote:
> As soon as contents of the array change during analysis, this becomes 
> incorrect: we cannot keep denoting a new value with the same old symbol.
I've updated the patch, adding more tests with the case you mentioned. Please, 
look at them, especially at functions  last four functions. Is this your case? 
If so, then I tested them and they passed successfully. And if I'm wrong, 
please, explain what I missed more detaily.


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

https://reviews.llvm.org/D81254



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ, what do you think about my explanation?


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

https://reviews.llvm.org/D77062



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko I've compiled some performance stats using //csa-testbench//. The 
result are within the margin of error.

| Project| Before  | After   | Delta  |
| libWebM| 0:00:32 | 0:00:34 | 6,25%  |
| Memcached  | 0:00:25 | 0:00:26 | 4,00%  |
| OpenSSL| 0:05:03 | 0:04:56 | -2,31% |
| PostgreSQL | 0:09:55 | 0:09:24 | -5,21% |
| protobuf   | 0:07:07 | 0:06:53 | -3,28% |
| Redis  | 0:02:50 | 0:02:59 | 5,29%  |
| SQLite | 0:08:24 | 0:08:48 | 4,76%  |
| TinyXML2   | 0:00:15 | 0:00:18 | 20,00% |
| TMUX   | 0:01:20 | 0:01:30 | 12,50% |
| twin   | 0:00:54 | 0:00:57 | 5,56%  |
| Vim| 0:03:36 | 0:03:46 | 4,63%  |
| Xerces | 0:04:41 | 0:05:01 | 7,12%  |
| TOTAL  | 0:45:02 | 0:45:32 | 1,11%  |


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

https://reviews.llvm.org/D78933



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@xazax.hun,
I've made performance measurements you concerned about. Could you look at it 
and make decision on this patch?


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

https://reviews.llvm.org/D78933



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.
Herald added subscribers: martong, steakhal.

@Charusso 
I think this patch may fix this bug https://bugs.llvm.org/show_bug.cgi?id=25284
Could you please verify and close it if so?
At least I couldn't reproduce it on the latest build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69599



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D78933#2090076 , @xazax.hun wrote:

> I would not call the results of the measurement within the margin of error 
> but the results do not look bad. Unless there is some objection from someone 
> else I am ok with committing this but please change the title of revision 
> before committing. When one says optimization we often think about 
> performance.  It should say something like reasoning about more comparison 
> operations.


Thanks. I'll change the name. 
I mentioned it as a "margin of error" because if you run tests on the same 
build twice you can easily get the same difference between two tests.


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

https://reviews.llvm.org/D78933



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

[PATCH] D77806: [analyzer] Do not report CFError null dereference for nonnull params

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko 
Seems you could fix this bug https://bugs.llvm.org/show_bug.cgi?id=24876
Could you check it, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77806



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

[PATCH] D78933: [analyzer] Reasoning about comparison expressions in RangeConstraintManager

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe1741e34e00b: [analyzer] Reasoning about comparison 
expressions in RangeConstraintManager (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,213 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expect

[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ , just one another ping, since it is near to be closed.


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

https://reviews.llvm.org/D77062



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

[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Hi @vsavchenko , sorry for the late review.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:367-378
+RangeSet RangeSet::Delete(BasicValueFactory &BV, Factory &F,
+  const llvm::APSInt &Point) const {
+  llvm::APSInt Upper = Point;
+  llvm::APSInt Lower = Point;
+
+  ++Upper;
+  --Lower;

Useful function. But I'd better rename it to `subtract` as we are working with 
sets (as a mathimatical collection). We should have a such one for the Ranges 
not only for Points.
We have `intersect`, `delete` aka `subtract`. And we also need to have 
functions `union` and `symmetricDifference` to cover full palette of common 
operations on sets.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734
   //expressions which we currently do not know how to negate.
-  const RangeSet *getRangeForMinusSymbol(ProgramStateRef State, SymbolRef Sym) 
{
+  Optional getRangeForInvertedSub(SymbolRef Sym) {
 if (const SymSymExpr *SSE = dyn_cast(Sym)) {

As for me, I'd call this like `getRangeForNegatedSymSymExpr`, since you do 
Negate operation inside.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:841-844
+  RangeSet getTrueRange(QualType T) {
+RangeSet TypeRange = infer(T);
+return assumeNonZero(TypeRange, T);
+  }

Don't you think this is too complicated for such a simple getter?
Maybe we can just construct the range using smth about `RangeSet(RangeFactory, 
++Zero, --Zero);` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381



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

[PATCH] D83286: [analyzer][solver] Track symbol disequalities

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko

> if a != b and b == C where C is a constant, a != C

Did you take into account that e.g. `a > b` also is a disequality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83286



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

[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko
Thank you.
Despite of all of my nits, LGTM!




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:367-378
+RangeSet RangeSet::Delete(BasicValueFactory &BV, Factory &F,
+  const llvm::APSInt &Point) const {
+  llvm::APSInt Upper = Point;
+  llvm::APSInt Lower = Point;
+
+  ++Upper;
+  --Lower;

vsavchenko wrote:
> ASDenysPetrov wrote:
> > Useful function. But I'd better rename it to `subtract` as we are working 
> > with sets (as a mathimatical collection). We should have a such one for the 
> > Ranges not only for Points.
> > We have `intersect`, `delete` aka `subtract`. And we also need to have 
> > functions `union` and `symmetricDifference` to cover full palette of common 
> > operations on sets.
> I agree that we should have a full set of functions.  I don't agree however, 
> that this function is a `subtract`.  Subtract is an operation on two sets and 
> here we have a set and a point.  One might argue that a point is just a very 
> simple set, that's true, but real `subtract` would be more complex in its 
> implementation.
> 
> Naming it `delete`, on the other hand, I was coming from a notion of deleting 
> points or neighbourhoods 
> (https://en.wikipedia.org/wiki/Neighbourhood_(mathematics)#Deleted_neighbourhood).
>One might argue that a point is just a very simple set
That's actually what I mean :) and for this particular case you may leave the 
implementation as is. And for me it still does what `subtract` does. But I'm 
OK,  I don't insist.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734
   //expressions which we currently do not know how to negate.
-  const RangeSet *getRangeForMinusSymbol(ProgramStateRef State, SymbolRef Sym) 
{
+  Optional getRangeForInvertedSub(SymbolRef Sym) {
 if (const SymSymExpr *SSE = dyn_cast(Sym)) {

vsavchenko wrote:
> ASDenysPetrov wrote:
> > As for me, I'd call this like `getRangeForNegatedSymSymExpr`, since you do 
> > Negate operation inside.
> I'm not super happy about my name either, but I feel like it describes it 
> better than the previous name and your version.  That function doesn't work 
> for any `SymSymExpr` and it doesn't simply negate whatever we gave it.  It 
> works specifically for symbolic subtractions and this is the information I 
> want to be reflected in the name.
Oh, I just assumed //...Sub// at the end as a //subexpression// but you mean 
//subtraction//. What I'm trying to say is that we can rename it like 
`getRangeFor...`//the expression which this function can handle//. E.g. 
`getRangeForNegatedSubtractionSymSymExpr`. My point is in a speaking name.

I think //invertion// is not something appropriate in terms of applying minus 
operator. I think invertion of zero should be something opposite but not a 
zero. Because when you would like to implement the function which turns [A, B] 
into [MIN, A)U(B, MAX], what would be the name of it? I think this is an 
//invertion//.

But this is not a big deal, it's just my thoughts.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:841-844
+  RangeSet getTrueRange(QualType T) {
+RangeSet TypeRange = infer(T);
+return assumeNonZero(TypeRange, T);
+  }

vsavchenko wrote:
> ASDenysPetrov wrote:
> > Don't you think this is too complicated for such a simple getter?
> > Maybe we can just construct the range using smth about 
> > `RangeSet(RangeFactory, ++Zero, --Zero);` ?
> It is more complex than a false range but there is a reason for it.
> 
> First of all, `RangeSet` can't have ranges where the end is greater than its 
> start.  Only `Intersect` can handle such ranges correctly.  Another thing is 
> that ranges like that mean `[MIN, --Zero], [++Zero, MAX]` and without a type 
> we can't really say what `MIN` and `MAX` are, so such constructor for 
> `RangeSet` simply cannot exist.
> 
> Another point is that we do need to have `[MIN, -1], [+1, MAX]` as opposed to 
> `[-1, -1], [+1, +1]` because of C language (it doesn't have boolean type), 
> and because of the cases like `a - b` where we know that `a != b`.
> 
> I hope that answers the question.
I just want this function has low complexity and be more lightweight as 
`getFalseRange`. And if we have any chance to simplify it (and you have all the 
data to get MIN and MAX), it'd be cool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381



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

[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko 
OK, let it be. )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381



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

[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734
   //expressions which we currently do not know how to negate.
-  const RangeSet *getRangeForMinusSymbol(ProgramStateRef State, SymbolRef Sym) 
{
+  Optional getRangeForInvertedSub(SymbolRef Sym) {
 if (const SymSymExpr *SSE = dyn_cast(Sym)) {

NoQ wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > ASDenysPetrov wrote:
> > > > > As for me, I'd call this like `getRangeForNegatedSymSymExpr`, since 
> > > > > you do Negate operation inside.
> > > > I'm not super happy about my name either, but I feel like it describes 
> > > > it better than the previous name and your version.  That function 
> > > > doesn't work for any `SymSymExpr` and it doesn't simply negate whatever 
> > > > we gave it.  It works specifically for symbolic subtractions and this 
> > > > is the information I want to be reflected in the name.
> > > Oh, I just assumed //...Sub// at the end as a //subexpression// but you 
> > > mean //subtraction//. What I'm trying to say is that we can rename it 
> > > like `getRangeFor...`//the expression which this function can handle//. 
> > > E.g. `getRangeForNegatedSubtractionSymSymExpr`. My point is in a speaking 
> > > name.
> > > 
> > > I think //invertion// is not something appropriate in terms of applying 
> > > minus operator. I think invertion of zero should be something opposite 
> > > but not a zero. Because when you would like to implement the function 
> > > which turns [A, B] into [MIN, A)U(B, MAX], what would be the name of it? 
> > > I think this is an //invertion//.
> > > 
> > > But this is not a big deal, it's just my thoughts.
> > My thought process here was that we are trying to get range for `A - B` and 
> > there is also information on `B - A`, so we can get something for `A - B` 
> > based on that.  So, it doesn't really matter what it does under the hood 
> > with ranges, it matters what its semantics are.  Here I called `B - A` //an 
> > inverted subtraction//.
> > I don't really know what would be the best name, but I thought that this 
> > one makes more sense.
> > Because when you would like to implement the function which turns [A, B] 
> > into [MIN, A)U(B, MAX], what would be the name of it? I think this is an 
> > //invertion//.
> 
> https://en.wikipedia.org/wiki/Complement_(set_theory)
> https://en.wikipedia.org/wiki/Inverse_function
> 
> "Negated subtraction" is definitely my favorite so far. "Mirrored" might be a 
> good layman term as well.
>https://en.wikipedia.org/wiki/Complement_(set_theory)
>https://en.wikipedia.org/wiki/Inverse_function
Thanks for the links, @NoQ. That's much definite now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381



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

[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

One more :-)


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

https://reviews.llvm.org/D77062



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

[PATCH] D82092: [analyzer] Handle `\l` symbol in Windows specific file paths in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, vsavchenko.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

Handle `\l` separately for Windows, because macros `__FILE__` produces Windows 
specific delimiters `\` and sometimes it happens when directory name starts 
from the letter `l`.

Fix: Use regex on Windows for replacing all `\l` (like `,\l`, `}\l`, `[\l`) 
except `\\l`, because `__FILE__` containes multiple `\` before `\l` and it 
helps to distinguish this case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82092

Files:
  
clang/test/Analysis/exploded-graph-rewriter/l_directory_from_l/win_file_macros.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ 
produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82103: [analyzer] Remove forbidden characters from a SourceLocation filename for a graph dump on Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: Charusso, NoQ.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

Windows forbidden file path characters are used in a field `file`, while 
creating a dump `dot` file using an argument -analyzer-dump-egraph. It 
specifically relates to angle brackets when using ``, 
``, `` values in filenames. It causes that script 
exploded-graph-rewriter.py incorrectly parses the dump.

Fix: Remove forbidden characters from filename for Windows platform, when 
creating graph dump file.

This patch shall follow after D82092 , since 
it has a fix for `assert`. `assert` is used in the test here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82103

Files:
  clang/include/clang/Basic/JsonSupport.h
  
clang/test/Analysis/exploded-graph-rewriter/l_directory_from_l/win_file_macros.cpp
  clang/test/Analysis/exploded-graph-rewriter/win_filepath.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py

Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?
 
 
 namespace clang {
@@ -98,7 +99,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32 
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), std::end(ForbiddenChars),
+   Char) != std::end(ForbiddenChars);
+});
+filename.erase(RemoveIt, filename.end());
+// Handle windows-specific path delimiters.
 std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 Out << "\"line\": " << PLoc.getLine()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82103: [analyzer] Remove forbidden characters from a SourceLocation filename for a graph dump on Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 271971.
ASDenysPetrov added a comment.

Fixed clang-format.


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

https://reviews.llvm.org/D82103

Files:
  clang/include/clang/Basic/JsonSupport.h
  
clang/test/Analysis/exploded-graph-rewriter/l_directory_from_l/win_file_macros.cpp
  clang/test/Analysis/exploded-graph-rewriter/win_filepath.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py

Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?
 
 namespace clang {
 
@@ -98,7 +98,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), std::end(ForbiddenChars),
+   Char) != std::end(ForbiddenChars);
+});
+filename.erase(RemoveIt, filename.end());
+// Handle windows-specific path delimiters.
 std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 Out << "\"line\": " << PLoc.getLine()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82092: [analyzer] Handle `\l` symbol in Windows specific file paths in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 271968.
ASDenysPetrov added a comment.

Fixed clang-format.


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

https://reviews.llvm.org/D82092

Files:
  
clang/test/Analysis/exploded-graph-rewriter/l_directory_from_l/win_file_macros.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ 
produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,15 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately for Windows, because macros __FILE__ produces
+# Windows specific delimiters `\` and sometimes it happens
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because __FILE__ containes multiple `\` before `\l`.
+node_label = re.sub(r'(?___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82092: [analyzer] Handle `\l` symbol in Windows specific file paths in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

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

In D82092#2100973 , @vsavchenko wrote:

> Even though it doesn't seem that necessary, I would still vote to comply with 
> `clang-format` in tests as well.


Yes, I just forgot to format it and was going to load it after.

> Additionally, I want to make a note that we are not really working hard on 
> keeping all the scripts and the analyzer itself to be fully functional on 
> Windows.

I think it depends on the platform you prefer, so that's natural. I also try to 
use Ubuntu on VM to check any differences.




Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:383
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,

NoQ wrote:
> NoQ wrote:
> > Behavior of this script should not depend on the host platform. Neither 
> > should it depend on the target platform if the graph is obtained via 
> > cross-compilation. It should only depend on the graph itself, as it's a 
> > simple dot file converter. The graph itself may, of course, depend on the 
> > target platform (maybe even on the host platform).
> > 
> > So it sounds like this fix should be on the C++ side.
> Like, i mean, i see no reason why it should be impossible to copy the 
> original dot file from one machine to another and still be able to run the 
> script. It's probably not a super-important use case but i don't see why 
> would we consciously break that.
>So it sounds like this fix should be on the C++ side.
The first I did was C++ side. BTW an explicit literal `"string\\literal"` can 
also cause this issue.
But then I found that I am not sure what the solution could be. How can we let 
the script skip this particular case without modifiing the string? How can we 
modify the string to keep it logically valid? The only solution I see is to 
make the replacement selection more accurate. 

>Like, i mean, i see no reason why it should be impossible to copy the original 
>dot file from one machine to another and still be able to run the script.
Correct! I'll keep the regex part for all platforms.

I've just checked the old code on Ubuntu using the sample. `char text[] = 
"string\\literal";` And it also fails as on Windows.
When I used the patch It passed. So this is not only a Windows related issue.


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

https://reviews.llvm.org/D82092



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

[PATCH] D82092: [analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 272007.
ASDenysPetrov added a comment.

Expanded the patch for all platforms.


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

https://reviews.llvm.org/D82092

Files:
  clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,13 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately because a string literal can be in code
+# like "string\\literal" with the `\l` inside.
+# Also on Windows macros __FILE__ produces specific delimiters `\`
+# and a directory or file may starts with the letter `l`.
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because the literal as a rule containes multiple `\` before `\l`.
+node_label = re.sub(r'(?', '>') \
- .replace('\\l', '') \
  .replace('|', '\\|')
+s = re.sub(r'(?', s)
 if self._gray_mode:
 s = re.sub(r'', '', s)
 s = re.sub(r'', '', s)
Index: clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
@@ -0,0 +1,26 @@
+// CAUTION: The name of this file should start with `l` for proper tests.
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter %t.dot | FileCheck %s
+// REQUIRES: asserts
+
+void test1() {
+  // Here __FILE__ macros produces a string with `\` delimiters on Windows
+  // and the name of the file starts with `l`.
+  char text[] = __FILE__;
+}
+
+void test2() {
+  // Here `\l` is in the middle of the literal.
+  char text[] = "string\\literal";
+}
+
+void test() {
+  test1();
+  test2();
+}
+
+// This test is passed if exploded_graph_rewriter handles dot file without 
errors.
+// CHECK: digraph "ExplodedGraph"


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,13 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately because a string literal can be in code
+# like "string\\literal" with the `\l` inside.
+# Also on Windows macros __FILE__ produces specific delimiters `\`
+# and a directory or file may starts with the letter `l`.
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because the literal as a rule containes multiple `\` before `\l`.
+node_label = re.sub(r'(?', '>') \
- .replace('\\l', '') \
  .replace('|', '\\|')
+s = re.sub(r'(?', s)
 if self._gray_mode:
 s = re.sub(r'', '', s)
 s = re.sub(r'', '', s)
Index: clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
==

[PATCH] D82092: [analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

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



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:432
  .replace('|', '\\|')
+s = re.sub(r'(?', s)
 if self._gray_mode:

This makes the graph data being look from {F12194502} to {F12194498}.


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

https://reviews.llvm.org/D82092



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

[PATCH] D82103: [analyzer] Remove forbidden characters from a SourceLocation filename for a graph dump on Windows

[Denys Petrov via Phabricator via cfe-commits]

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



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:383
+# when directory name starts with the letter `l`.
+if sys.platform == 'win32':
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,

NoQ wrote:
> I think this too doesn't need to be platform specific(?)
Oh, so sorry! This is a mess. It somehow got here from my another patch. I'll 
correct it. This should not be here.


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

https://reviews.llvm.org/D82103



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

[PATCH] D82092: [analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

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

Thanks for the quick review, guys!




Comment at: 
clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp:25
+
+// This test is passed if exploded_graph_rewriter handles dot file without 
errors.
+// CHECK: digraph "ExplodedGraph"

NoQ wrote:
> I still wouldn't mind testing the actual output.
I'll add more //checks// before the load.


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

https://reviews.llvm.org/D82092



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

[PATCH] D82103: [analyzer] Remove forbidden characters from a SourceLocation filename for a graph dump on Windows

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 272204.
ASDenysPetrov added a comment.

Removed accidentally included unrelated changes.


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

https://reviews.llvm.org/D82103

Files:
  clang/include/clang/Basic/JsonSupport.h
  clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp


Index: clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
@@ -0,0 +1,26 @@
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter %t.dot | FileCheck %s
+// REQUIRES: asserts
+// UNSUPPORTED: !windows
+
+// Angle brackets shall not be presented in the field `file`,
+// because exploded_graph_rewriter handles it as a file path
+// and such symbols are forbidden on Windows platform.
+
+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__))
+
+int test(int *array, int size) {
+  //Here `assert` helps to produce angle brackets.
+  assert(size);
+  return array[size];
+}
+
+//This test is passed if exploded_graph_rewriter handles dot file without 
errors.
+// CHECK: digraph "ExplodedGraph"
Index: clang/include/clang/Basic/JsonSupport.h
===
--- clang/include/clang/Basic/JsonSupport.h
+++ clang/include/clang/Basic/JsonSupport.h
@@ -13,7 +13,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
-
+#include 
 
 namespace clang {
 
@@ -98,7 +98,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), 
std::end(ForbiddenChars),
+   Char) != std::end(ForbiddenChars);
+});
+filename.erase(RemoveIt, filename.end());
+// Handle windows-specific path delimiters.
 std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 Out << "\"line\": " << PLoc.getLine()


Index: clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
@@ -0,0 +1,26 @@
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter %t.dot | FileCheck %s
+// REQUIRES: asserts
+// UNSUPPORTED: !windows
+
+// Angle brackets shall not be presented in the field `file`,
+// because exploded_graph_rewriter handles it as a file path
+// and such symbols are forbidden on Windows platform.
+
+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__))
+
+int test(int *array, int size) {
+  //Here `assert` helps to produce angle brackets.
+  assert(size);
+  return array[size];
+}
+
+//This test is passed if exploded_graph_rewriter handles dot file without errors.
+// CHECK: digraph "ExplodedGraph"
Index: clang/include/clang/Basic/JsonSupport.h
===
--- clang/include/clang/Basic/JsonSupport.h
+++ clang/include/clang/Basic/JsonSupport.h
@@ -13,7 +13,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
-
+#include 
 
 namespace clang {
 
@@ -98,7 +98,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), std::end(ForbiddenChars),
+   

[PATCH] D82092: [analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG01f9388d95ac: [analyzer] Handle `\l` symbol in string 
literals in exploded-graph-rewriter (authored by ASDenysPetrov).

Changed prior to commit:
  https://reviews.llvm.org/D82092?vs=272007&id=272379#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82092

Files:
  clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,13 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately because a string literal can be in code
+# like "string\\literal" with the `\l` inside.
+# Also on Windows macros __FILE__ produces specific delimiters `\`
+# and a directory or file may starts with the letter `l`.
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because the literal as a rule containes multiple `\` before `\l`.
+node_label = re.sub(r'(?', '>') \
- .replace('\\l', '') \
  .replace('|', '\\|')
+s = re.sub(r'(?', s)
 if self._gray_mode:
 s = re.sub(r'', '', s)
 s = re.sub(r'', '', s)
Index: clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
@@ -0,0 +1,28 @@
+// CAUTION: The name of this file should start with `l` for proper tests.
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter %t.dot | FileCheck %s
+// REQUIRES: asserts
+
+void test1() {
+  // Here __FILE__ macros produces a string with `\` delimiters on Windows
+  // and the name of the file starts with `l`.
+  char text[] = __FILE__;
+}
+
+void test2() {
+  // Here `\l` is in the middle of the literal.
+  char text[] = "string\\literal";
+}
+
+void test() {
+  test1();
+  test2();
+}
+
+// This test is passed if exploded_graph_rewriter handles dot file without 
errors.
+// CHECK: digraph "ExplodedGraph"
+// CHECK: 
clang\\test\\Analysis\\exploded-graph-rewriter\\l_name_starts_with_l.cpp";
+// CHECK: char text[] = "string\\literal";


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -368,8 +368,7 @@
 self.root_id = node_id
 # Note: when writing tests you don't need to escape everything,
 # even though in a valid dot file everything is escaped.
-node_label = result.group(2).replace('\\l', '') \
-.replace(' ', '') \
+node_label = result.group(2).replace(' ', '') \
 .replace('\\"', '"') \
 .replace('\\{', '{') \
 .replace('\\}', '}') \
@@ -378,6 +377,13 @@
 .replace('\\<', '<') \
 .replace('\\>', '>') \
 .rstrip(',')
+# Handle `\l` separately because a string literal can be in code
+# like "string\\literal" with the `\l` inside.
+# Also on Windows macros __FILE__ produces specific delimiters `\`
+# and a directory or file may starts with the letter `l`.
+# Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
+# because the literal as a rule containes multiple `\` before `\l`.
+node_label = r

[PATCH] D82103: [analyzer] Remove forbidden characters from a SourceLocation filename for a graph dump on Windows

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbe9c5818351b: [analyzer] Remove forbidden characters from a 
filename for a graph dump on… (authored by ASDenysPetrov).

Changed prior to commit:
  https://reviews.llvm.org/D82103?vs=272204&id=272427#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82103

Files:
  clang/include/clang/Basic/JsonSupport.h
  clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp


Index: clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
@@ -0,0 +1,20 @@
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter --verbose %t.dot 2>&1 | FileCheck %s
+// REQUIRES: asserts
+// UNSUPPORTED: !windows
+
+// Angle brackets shall not be presented in the field `file`,
+// because exploded_graph_rewriter handles it as a file path
+// and such symbols are forbidden on Windows platform.
+
+void test() {
+  // This produces angle brackets.
+  char text[] = __FILE__;
+}
+
+// This test is passed if exploded_graph_rewriter handles dot file without 
errors.
+// CHECK: DEBUG:root:Line: digraph "Exploded Graph"
+// CHECK: \"file\": \"scratch space\"
Index: clang/include/clang/Basic/JsonSupport.h
===
--- clang/include/clang/Basic/JsonSupport.h
+++ clang/include/clang/Basic/JsonSupport.h
@@ -13,7 +13,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
-
+#include 
 
 namespace clang {
 
@@ -98,7 +98,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), 
std::end(ForbiddenChars),
+   Char) != std::end(ForbiddenChars);
+});
+filename.erase(RemoveIt, filename.end());
+// Handle windows-specific path delimiters.
 std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 Out << "\"line\": " << PLoc.getLine()


Index: clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
===
--- /dev/null
+++ clang/test/Analysis/exploded-graph-rewriter/win_path_forbidden_chars.cpp
@@ -0,0 +1,20 @@
+// FIXME: Figure out how to use %clang_analyze_cc1 with our lit.local.cfg.
+// RUN: %clang_cc1 -analyze -triple x86_64-unknown-linux-gnu \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-dump-egraph=%t.dot %s
+// RUN: %exploded_graph_rewriter --verbose %t.dot 2>&1 | FileCheck %s
+// REQUIRES: asserts
+// UNSUPPORTED: !windows
+
+// Angle brackets shall not be presented in the field `file`,
+// because exploded_graph_rewriter handles it as a file path
+// and such symbols are forbidden on Windows platform.
+
+void test() {
+  // This produces angle brackets.
+  char text[] = __FILE__;
+}
+
+// This test is passed if exploded_graph_rewriter handles dot file without errors.
+// CHECK: DEBUG:root:Line: digraph "Exploded Graph"
+// CHECK: \"file\": \"scratch space\"
Index: clang/include/clang/Basic/JsonSupport.h
===
--- clang/include/clang/Basic/JsonSupport.h
+++ clang/include/clang/Basic/JsonSupport.h
@@ -13,7 +13,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
-
+#include 
 
 namespace clang {
 
@@ -98,7 +98,16 @@
 if (AddBraces)
   Out << "{ ";
 std::string filename(PLoc.getFilename());
-#ifdef _WIN32 // Handle windows-specific path delimiters.
+#ifdef _WIN32
+// Remove forbidden Windows path characters
+auto RemoveIt =
+std::remove_if(filename.begin(), filename.end(), [](auto Char) {
+  static const char ForbiddenChars[] = "<>*?\"|";
+  return std::find(std::begin(ForbiddenChars), std::end(ForbiddenChars),
+   Char) != std::end(ForbiddenChars);
+});
+filename.erase(RemoveIt, filename.end());
+// Handle windows-specific path delimiters.
 std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 Out << "\"line\": " << PLoc.getLine()
___

[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 272997.
ASDenysPetrov added a comment.

@NoQ thanks for the approval. But I'm afraid we should return the check for 
`!V.getAs()` back in `CStringChecker::assumeZero`.

Look, I paid attention to the fix for https://llvm.org/PR24951 and related 
commit rG480a0c00ca51d909a925120737b71289cbb79eda 
. I saw 
that @dcoughlin added a check for `V.getAs()` to fix 
and the old code used it implicitly. My patch returns this issue back. That's 
why we need to add the check for `LazyCompoundVal` as well.

Another big question is why `LazyCompoundVal` can be possible here at all. As I 
see according to the graph below the answer is somewhere deep in the core. Thus 
I'd propose to apply this patch in scope of fix https://llvm.org/PR37503.
F11723847: tmp63ucwe5i.html 

I updated the patch. I've also added a //FIXME// caveat.


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

https://reviews.llvm.org/D77062

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/string.c

Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -363,6 +363,14 @@
 strcpy(x, y); // no-warning
 }
 
+void *func_strcpy_no_assertion();
+char ***ptr_strcpy_no_assertion;
+void strcpy_no_assertion() {
+  *(unsigned char **)ptr_strcpy_no_assertion = (unsigned char *)(func_strcpy_no_assertion());
+  char c;
+  strcpy(**ptr_strcpy_no_assertion, &c); // no-assertion
+}
+
 //===--===
 // stpcpy()
 //===--===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -196,9 +196,8 @@
   void evalBzero(CheckerContext &C, const CallExpr *CE) const;
 
   // Utility methods
-  std::pair
-  static assumeZero(CheckerContext &C,
-ProgramStateRef state, SVal V, QualType Ty);
+  std::pair static assumeZero(
+  ProgramStateRef state, SVal V);
 
   static ProgramStateRef setCStringLength(ProgramStateRef state,
   const MemRegion *MR,
@@ -279,16 +278,18 @@
 // Individual checks and utility methods.
 //===--===//
 
-std::pair
-CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
-   QualType Ty) {
+std::pair
+CStringChecker::assumeZero(ProgramStateRef state, SVal V) {
+  auto states = std::make_pair(state, state);
+
   Optional val = V.getAs();
-  if (!val)
-return std::pair(state, state);
+  // FIXME: We should understand how LazyCompoundVal can be possible here,
+  // fix the root cause and get rid of this check.
+  if (val && !V.getAs())
+// Returned pair shall be {null, non-null} so reorder states.
+std::tie(states.second, states.first) = state->assume(*val);
 
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
-  return state->assume(svalBuilder.evalEQ(state, *val, zero));
+  return states;
 }
 
 ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
@@ -299,8 +300,7 @@
 return nullptr;
 
   ProgramStateRef stateNull, stateNonNull;
-  std::tie(stateNull, stateNonNull) =
-  assumeZero(C, State, l, Arg.Expression->getType());
+  std::tie(stateNull, stateNonNull) = assumeZero(State, l);
 
   if (stateNull && !stateNonNull) {
 if (Filter.CheckCStringNullArg) {
@@ -1071,8 +1071,7 @@
 CharVal = svalBuilder.evalCast(CharVal, Ctx.UnsignedCharTy, Ctx.IntTy);
 
 ProgramStateRef StateNullChar, StateNonNullChar;
-std::tie(StateNullChar, StateNonNullChar) =
-assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+std::tie(StateNullChar, StateNonNullChar) = assumeZero(State, CharVal);
 
 if (StateWholeReg && !StateNotWholeReg && StateNullChar &&
 !StateNonNullChar) {
@@ -1133,11 +1132,9 @@
   // See if the size argument is zero.
   const LocationContext *LCtx = C.getLocationContext();
   SVal sizeVal = state->getSVal(Size.Expression, LCtx);
-  QualType sizeTy = Size.Expression->getType();
 
   ProgramStateRef stateZeroSize, stateNonZeroSize;
-  std::tie(stateZeroSize, stateNonZeroSize) =
-  assumeZero(C, state, sizeVal, sizeTy);
+  std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(state, sizeVal);
 
   // Get the value of the Dest.
   SVal destVal = state->getSVal(Dest.Expression, LCtx);
@@ -1287,11 +1284,9 @@
 
   // See if the size argument is zero.
   SVal sizeVal = State->getSVal(Size.Expression, LCtx);
-  QualType sizeTy = Size.Expres

[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ gentle ping.


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

https://reviews.llvm.org/D77062



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

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ, thanks for the examples.

I didn't get the first one. How do you get to the "//In reality we don't 
know//", if we don't touch `a[index1]`:

  void test1(int *a, int index1, int index2) {
int x = a[index1];
a[index2] = 0;
int y = a[index1];
  
// In reality we don't know but after your patch
// we're confident that this is "TRUE".
clang_analyzer_eval(x == y);
  }

I worked on the second case. I found some possible way to resovle it:

  void foo(int *a); // Potentially modifies elements of 'a'.
  void fooRef(int *&a); // Potentially modifies elements of 'a'.
  
  void test2(int *a) {
// a - &SymRegion{reg_$6}
foo(a); // after this, CSA doesn't change a.
// a - &SymRegion{reg_$6}
fooRef(a); // after this, CSA changes a.
// a - &SymRegion{conj_$10{int *, LC1, S925, #1}}
  }

Here we need to make `foo` behave the same as `fooRef`. What do you think?


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

https://reviews.llvm.org/D81254



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@baloghadamsoftware
Please, review my changes. If you think they are OK, could you, please, commit 
the patch on by behalf as an author:
`git commit --amend --author="Denys Petrov "`


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

https://reviews.llvm.org/D77802



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 9 inline comments as done.
ASDenysPetrov added a comment.

@baloghadamsoftware

> However, please still extend the current tests

I've looked for some appropriate tests to extend, but haven't found. What would 
you advise to use instead of mine?
@steakhal
Thanks for the comments.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();

steakhal wrote:
> Should we take it as `const ref` to prevent copying?
getMinValue returns APSInt by value, so it wouldn't make sense.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573
   if (const RangeSet *negV = State->get(negSym)) {
-// Unsigned range set cannot be negated, unless it is [0, 0].
-if ((negV->getConcreteValue() &&
- (*negV->getConcreteValue() == 0)) ||
+if (T->isUnsignedIntegerOrEnumerationType() ||
 T->isSignedIntegerOrEnumerationType())
   return negV;

steakhal wrote:
> Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to 
> something similar?
I've looked for similar single function, but there is no appropriate one. I 
decided not to add a new one to make patch more focused.



Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:114-118
+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{{FALSE}} 
expected-warning{{TRUE}}
+}

steakhal wrote:
> What does this test case demonstrate? Could you elaborate on that?
> Why do you evaluate the `x - y != 0` here twice?
>Why do you evaluate the x - y != 0 here twice?
This is the root line which assertion occured on.
I'll add some comments.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;

steakhal wrote:
> According to the [LLVM coding 
> style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
>  we should use UpperCamelCase for variable names for new code.
> 
> Note that the Analyzer Core was written in a different style, we should 
> follow that there instead (at least IMO).
Here is a discussion [[ https://llvm.org/docs/Proposals/VariableNames.html | 
VariableNames]] I was guide. Seems like we can use camelBack for new code. Am I 
right? Also RangeSetTest.cpp already uses mixed naming as you can see.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),

steakhal wrote:
> AFAIK since `std::initializer_list` is just two pointers we should take it by 
> value.
I'm not sure about //should//, since initializer_list is a container and it'd 
be consistent if we handle it like any other compound object despite its 
implementation (IMO). But I can change it if you wish.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;

steakhal wrote:
> Generally, `new expressions` are a code smell. We should use something like 
> an `std::make_unique` to prevent memory leaks on exceptions.
> Though, I'm not sure if there is a similar function for 
> `llvm::IntrusiveRefCntPtr`s.
I'll make it more safe.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66
+  template  void checkNegate() {
+using type = T;
+

steakhal wrote:
> To be honest, I'm not sure if `type` is more descriptive than `T`.
It is useful for debug purposes, for instance you can change it to `using type 
= int;` to verify something. It also untie from template argument list and make 
the code more flexible.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:68
+
+// use next values of the range {MIN, A, B, MID, C, D, MAX}
+

steakhal wrote:
> AFAIK full sentences are required for comments.
> https://llvm.org/docs/CodingStandards.html#commenting
I'll correct it.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+  // c.original.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // c.expected.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedFromOriginal.print(llvm::dbgs());
+  // ll

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 258300.
ASDenysPetrov added a comment.

Refined Negate function. Divided by steps for clarity.
Made some changes in comments due to guideline.


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

https://reviews.llvm.org/D77802

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/RangeSetTest.cpp

Index: clang/unittests/StaticAnalyzer/RangeSetTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/RangeSetTest.cpp
@@ -0,0 +1,141 @@
+//===- unittests/StaticAnalyzer/RangeSetTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// TestCase contains to lists of ranges.
+// Original one has to be negated.
+// Expected one has to be compared to negated original range.
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;
+
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),
+expected(createRangeSetFromList(BVF, F, expectedList)) {}
+
+private:
+  RangeSet createRangeSetFromList(BasicValueFactory &BVF, RangeSet::Factory &F,
+  const std::initializer_list &rangeList) {
+llvm::APSInt from(sizeof(T) * 8, std::is_unsigned::value);
+llvm::APSInt to = from;
+RangeSet rangeSet = F.getEmptySet();
+for (auto it = rangeList.begin(); it != rangeList.end(); it += 2) {
+  from = *it;
+  to = *(it + 1);
+  rangeSet = rangeSet.addRange(
+  F, RangeSet(F, BVF.getValue(from), BVF.getValue(to)));
+}
+return rangeSet;
+  }
+};
+
+class RangeSetTest : public testing::Test {
+protected:
+  // Init block
+  IntrusiveRefCntPtr Diags{new DiagnosticIDs};
+  IntrusiveRefCntPtr DiagOpts{new DiagnosticOptions};
+  DiagnosticsEngine Diag{Diags, DiagOpts};
+  FileSystemOptions FileSystemOpts;
+  FileManager FileMgr{FileSystemOpts};
+  SourceManager SM{Diag, FileMgr};
+  LangOptions LOpts;
+  IdentifierTable idents;
+  SelectorTable sels;
+  Builtin::Context builtins;
+  ASTContext context{LOpts, SM, idents, sels, builtins};
+  llvm::BumpPtrAllocator alloc;
+  BasicValueFactory BVF{context, alloc};
+  RangeSet::Factory F;
+  // End init block
+
+  template  void checkNegate() {
+using type = T;
+
+// Use next values of the range {MIN, A, B, MID, C, D, MAX}.
+
+// MID is a value in the middle of the range
+// which unary minus does not affect on,
+// e.g. int8/int32(0), uint8(128), uint32(2147483648).
+
+constexpr type MIN = std::numeric_limits::min();
+constexpr type MAX = std::numeric_limits::max();
+constexpr type MID = std::is_signed::value
+ ? 0
+ : ~(static_cast(-1) / static_cast(2));
+constexpr type A = MID - static_cast(42 + 42);
+constexpr type B = MID - static_cast(42);
+constexpr type C = -B;
+constexpr type D = -A;
+
+static_assert(MIN < A && A < B && B < MID && MID < C && C < D && D < MAX,
+  "Values shall be in an ascending order");
+
+// Left {[x, y], [x, y]} is what shall be negated.
+// Right {[x, y], [x, y]} is what shall be compared to a negation result.
+TestCase cases[] = {
+{BVF, F, {MIN, A}, {MIN, MIN, D, MAX}},
+{BVF, F, {MIN, C}, {MIN, MIN, B, MAX}},
+{BVF, F, {MIN, MID}, {MIN, MIN, MID, MAX}},
+{BVF, F, {MIN, MAX}, {MIN, MAX}},
+{BVF, F, {A, D}, {A, D}},
+{BVF, F, {A, B}, {C, D}},
+{BVF, F, {MIN, A, D, MAX}, {MIN, A, D, MAX}},
+{BVF, F, {MIN, B, MID, D}, {MIN, MIN, A, MID, C, MAX}},
+{BVF, F, {MIN, MID, C, D}, {MIN, MIN, A, B, MID, MAX}},
+{BVF, F, {MIN, MID, C, MAX}, {MIN, B, MID, MAX}},
+{BVF, F, {A, MID, D, MAX}, {MIN + 1, A, MID, D}},
+{BVF, F, {A, A}, {D, D}},
+{BVF, F, {MID, MID}, {MID, MID}},
+{BVF, F, {MAX, MAX}, {MIN + 1, MIN + 1}},
+};
+
+for (const auto &c : cases) {
+  // Negate original and check with expected.
+  RangeSet negatedFromOriginal = c.original.Negate(BVF, F);
+  EXPECT_EQ(negatedFromOriginal, c.expected);
+  // Negate negated bac

[PATCH] D78289: [analyzer] Stability improvements for IteratorModeling functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@baloghadamsoftware
I know you have much expertise here. Do you have some thoughts of how I can 
test exactly these cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78289



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

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

ping


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

https://reviews.llvm.org/D77062



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();

ASDenysPetrov wrote:
> steakhal wrote:
> > Should we take it as `const ref` to prevent copying?
> getMinValue returns APSInt by value, so it wouldn't make sense.
My fault. Yes, you are right. I've missed &. I used to append & to the type, 
since it is more readable and actually is a part of type, but I won't debate 
with clang style guide :).


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

https://reviews.llvm.org/D77802



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

[PATCH] D78289: [analyzer] Stability improvements for IteratorModeling functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 259521.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

@baloghadamsoftware
I've added a test for unscoped SVal, but still can't come up with `if (!LPos || 
!RPos)`
Could you, please, suggest me something?


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

https://reviews.llvm.org/D78289

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


Index: clang/test/Analysis/iterator-range.cpp
===
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -810,6 +810,19 @@
   auto j = std::prev(i, 0); // no-warning
 }
 
+// std::prev() with int* for checking Loc value argument
+namespace std {
+template 
+T prev(T, int *);
+}
+
+void prev_loc_value(const std::vector &V, int o) {
+
+  auto i = return_any_iterator(V.begin());
+  int *offset = &o;
+  auto j = std::prev(i, offset); // no-warning
+}
+
 //
 // Structure member dereference operators
 //
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,9 +422,14 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  if (!LPos || !RPos)
+return;
+
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
+if (!State)
+  return;
 auto &SymMgr = C.getSymbolManager();
 auto *LCtx = C.getLocationContext();
 RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
@@ -532,8 +537,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 


Index: clang/test/Analysis/iterator-range.cpp
===
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -810,6 +810,19 @@
   auto j = std::prev(i, 0); // no-warning
 }
 
+// std::prev() with int* for checking Loc value argument
+namespace std {
+template 
+T prev(T, int *);
+}
+
+void prev_loc_value(const std::vector &V, int o) {
+
+  auto i = return_any_iterator(V.begin());
+  int *offset = &o;
+  auto j = std::prev(i, offset); // no-warning
+}
+
 //
 // Structure member dereference operators
 //
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,9 +422,14 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  if (!LPos || !RPos)
+return;
+
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
+if (!State)
+  return;
 auto &SymMgr = C.getSymbolManager();
 auto *LCtx = C.getLocationContext();
 RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
@@ -532,8 +537,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78289: [analyzer] Stability improvements for IteratorModeling functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 259561.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

I decided not to add changes of what I cannot cover by tests. So I removed them.

@NoQ @baloghadamsoftware
please, review my changes.


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

https://reviews.llvm.org/D78289

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


Index: clang/test/Analysis/iterator-range.cpp
===
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -810,6 +810,19 @@
   auto j = std::prev(i, 0); // no-warning
 }
 
+// std::prev() with int* for checking Loc value argument
+namespace std {
+template 
+T prev(T, int *);
+}
+
+void prev_loc_value(const std::vector &V, int o) {
+
+  auto i = return_any_iterator(V.begin());
+  int *offset = &o;
+  auto j = std::prev(i, offset); // no-warning
+}
+
 //
 // Structure member dereference operators
 //
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,7 +422,7 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
 auto &SymMgr = C.getSymbolManager();
@@ -532,8 +532,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 


Index: clang/test/Analysis/iterator-range.cpp
===
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -810,6 +810,19 @@
   auto j = std::prev(i, 0); // no-warning
 }
 
+// std::prev() with int* for checking Loc value argument
+namespace std {
+template 
+T prev(T, int *);
+}
+
+void prev_loc_value(const std::vector &V, int o) {
+
+  auto i = return_any_iterator(V.begin());
+  int *offset = &o;
+  auto j = std::prev(i, offset); // no-warning
+}
+
 //
 // Structure member dereference operators
 //
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -402,7 +402,7 @@
   if (!Cont)
 return;
 
-  // At least one of the iterators have recorded positions. If one of them has
+  // At least one of the iterators has recorded positions. If one of them does
   // not then create a new symbol for the offset.
   SymbolRef Sym;
   if (!LPos || !RPos) {
@@ -422,7 +422,7 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
-  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {
 auto &SymMgr = C.getSymbolManager();
@@ -532,8 +532,9 @@
 return;
 
   const auto *value = &RHS;
+  SVal val;
   if (auto loc = RHS.getAs()) {
-const auto val = State->getRawSVal(*loc);
+val = State->getRawSVal(*loc);
 value = &val;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@Szelethus, could you, please, make some suggestions about my investigation 
above?


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

https://reviews.llvm.org/D77062



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

[PATCH] D78933: RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, baloghadamsoftware, steakhal, xazax.hun.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, martong, Charusso, rnkovacs.

I got an idea how to make RangeConstraintManager​ more sofisticated.
I want you speak out, share your vision about this idea.

The idea came to me as a thing which resolves this bug PR13426 
.
Let's consider the next snippet:

  int foo(int y, int x) {
  int x;
  if (y == z) {
  x = 0;
  }
  if (y != z) {
  x = 1;
  }
  return x;
  }

Obviously that `x` will be initialized, but CSA reports next:

  warning: Undefined or garbage value returned to caller
[core.uninitialized.UndefReturn]
  return x;

It happenes because CSA does not take into account that `y == z` and `y != z` 
are just two **opposites**, as if it was:

  if (y == z) {
  x = 0;
  } else {
  x = 1;
  }

So my improvments is in handling case above and similar ones.
This patch covers next:

- Consider comparisons such as `x > y` and `y < x` as **reversed** copy and 
generates a //true// branch only. Applies to `< > <= >= == !=`.
- Consider comparisons such as `x > y` and `x < y`, `x <= y`, `x == y`,  `y > 
x`,  `y >= x`,  `y == x` as **opposites** and generates a //false// branch 
only. Applies to `< > <= >= == !=`.

As a result of processing an example below, we have F11810767: before.html 
 and F11810762: after.html 
 exploded graphs.

  void foo(int y, int x) {
  int z;
  if (x > y) {
  if (y > x) {
  z = 1;
  }
  if (y >= x) {
  z = 2;
  }
  if (y == x) {
  z = 3;
  }
  }
  else{
  if (y > x) {
  z = 1;
  }
  if (y >= x) {
  z = 2;
  }
  if (y == x) {
  z = 3;
  }
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,109 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+//reversed
+
+void comparison_reversed_gt(int x, int y) {
+  if (x > y)
+clang_analyzer_eval(y < x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y < x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_ge(int x, int y) {
+  if (x >= y)
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_lt(int x, int y) {
+  if (x < y)
+clang_analyzer_eval(y > x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y > x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_le(int x, int y) {
+  if (x <= y)
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_eq(int x, int y) {
+  if (x == y)
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_ne(int x, int y) {
+  if (x != y)
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y != x); // expected-warning{{FALSE}}
+}
+
+//opposite
+
+void comparison_opposite_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_ge(int x, int y) {
+  if (x >= y) {
+clang_analyzer_eval(x < y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x < y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x > y); // expected-warning{{FALSE}}
+clang_an

[PATCH] D78933: RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 260331.

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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+//reversed
+
+void comparison_reversed_gt(int x, int y) {
+  if (x > y)
+clang_analyzer_eval(y < x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y < x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_ge(int x, int y) {
+  if (x >= y)
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_lt(int x, int y) {
+  if (x < y)
+clang_analyzer_eval(y > x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y > x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_le(int x, int y) {
+  if (x <= y)
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_eq(int x, int y) {
+  if (x == y)
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+}
+
+void comparison_reversed_ne(int x, int y) {
+  if (x != y)
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  else
+clang_analyzer_eval(y != x); // expected-warning{{FALSE}}
+}
+
+//opposite
+
+void comparison_opposite_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_ge(int x, int y) {
+  if (x >= y) {
+clang_analyzer_eval(x < y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x < y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x > y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x > y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x); // expected-warning{{TRUE}}
+  }
+}
+
+void comparison_opposite_eq(int x, int y) {
+  if (x == y) {
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{FALSE}}
+  } else {
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 260604.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

@baloghadamsoftware 
I have added regression tests as you asked. You can now check them. Thanks.
@NoQ,
I kindly ask you to look at the changes, seems that it has a full set to be 
load-ready. I really need your feedback here, since the changes is in the core.


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

https://reviews.llvm.org/D77802

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/RangeSetTest.cpp

Index: clang/unittests/StaticAnalyzer/RangeSetTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/RangeSetTest.cpp
@@ -0,0 +1,141 @@
+//===- unittests/StaticAnalyzer/RangeSetTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// TestCase contains to lists of ranges.
+// Original one has to be negated.
+// Expected one has to be compared to negated original range.
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;
+
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),
+expected(createRangeSetFromList(BVF, F, expectedList)) {}
+
+private:
+  RangeSet createRangeSetFromList(BasicValueFactory &BVF, RangeSet::Factory &F,
+  const std::initializer_list &rangeList) {
+llvm::APSInt from(sizeof(T) * 8, std::is_unsigned::value);
+llvm::APSInt to = from;
+RangeSet rangeSet = F.getEmptySet();
+for (auto it = rangeList.begin(); it != rangeList.end(); it += 2) {
+  from = *it;
+  to = *(it + 1);
+  rangeSet = rangeSet.addRange(
+  F, RangeSet(F, BVF.getValue(from), BVF.getValue(to)));
+}
+return rangeSet;
+  }
+};
+
+class RangeSetTest : public testing::Test {
+protected:
+  // Init block
+  IntrusiveRefCntPtr Diags{new DiagnosticIDs};
+  IntrusiveRefCntPtr DiagOpts{new DiagnosticOptions};
+  DiagnosticsEngine Diag{Diags, DiagOpts};
+  FileSystemOptions FileSystemOpts;
+  FileManager FileMgr{FileSystemOpts};
+  SourceManager SM{Diag, FileMgr};
+  LangOptions LOpts;
+  IdentifierTable idents;
+  SelectorTable sels;
+  Builtin::Context builtins;
+  ASTContext context{LOpts, SM, idents, sels, builtins};
+  llvm::BumpPtrAllocator alloc;
+  BasicValueFactory BVF{context, alloc};
+  RangeSet::Factory F;
+  // End init block
+
+  template  void checkNegate() {
+using type = T;
+
+// Use next values of the range {MIN, A, B, MID, C, D, MAX}.
+
+// MID is a value in the middle of the range
+// which unary minus does not affect on,
+// e.g. int8/int32(0), uint8(128), uint32(2147483648).
+
+constexpr type MIN = std::numeric_limits::min();
+constexpr type MAX = std::numeric_limits::max();
+constexpr type MID = std::is_signed::value
+ ? 0
+ : ~(static_cast(-1) / static_cast(2));
+constexpr type A = MID - static_cast(42 + 42);
+constexpr type B = MID - static_cast(42);
+constexpr type C = -B;
+constexpr type D = -A;
+
+static_assert(MIN < A && A < B && B < MID && MID < C && C < D && D < MAX,
+  "Values shall be in an ascending order");
+
+// Left {[x, y], [x, y]} is what shall be negated.
+// Right {[x, y], [x, y]} is what shall be compared to a negation result.
+TestCase cases[] = {
+{BVF, F, {MIN, A}, {MIN, MIN, D, MAX}},
+{BVF, F, {MIN, C}, {MIN, MIN, B, MAX}},
+{BVF, F, {MIN, MID}, {MIN, MIN, MID, MAX}},
+{BVF, F, {MIN, MAX}, {MIN, MAX}},
+{BVF, F, {A, D}, {A, D}},
+{BVF, F, {A, B}, {C, D}},
+{BVF, F, {MIN, A, D, MAX}, {MIN, A, D, MAX}},
+{BVF, F, {MIN, B, MID, D}, {MIN, MIN, A, MID, C, MAX}},
+{BVF, F, {MIN, MID, C, D}, {MIN, MIN, A, B, MID, MAX}},
+{BVF, F, {MIN, MID, C, MAX}, {MIN, B, MID, MAX}},
+{BVF, F, {A, MID, D, MAX}, {MIN + 1, A, MID, D}},
+{BVF, F, {A, A}, {D, D}},
+{BVF, F, {MID, MID}, {MID, MID}},
+{BVF, F, {MAX, MAX}, {MIN + 1, MIN + 1}},
+};
+
+for (const auto &

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal 
What about unsigneds? Does it work for unsigned values as well?


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

https://reviews.llvm.org/D77792



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

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 261552.

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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,183 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+   

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 261556.

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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,183 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+   

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 261563.
ASDenysPetrov edited the summary of this revision.

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

https://reviews.llvm.org/D78933

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

Index: clang/test/Analysis/constraint_manager_conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/constraint_manager_conditions.cpp
@@ -0,0 +1,183 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void comparison_lt(int x, int y) {
+  if (x < y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_gt(int x, int y) {
+  if (x > y) {
+clang_analyzer_eval(x < y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{TRUE}}
+clang_analyzer_eval(y < x);  // expected-warning{{TRUE}}
+clang_analyzer_eval(x <= y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y >= x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x == y); // expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}}
+  } else {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x != y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y != x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  }
+}
+
+void comparison_le(int x, int y) {
+  if (x <= y) {
+clang_analyzer_eval(x < y);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y > x);  // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x > y);  // expected-warning{{FALSE}}
+clang_analyzer_eval(y < x);  // expected-warning{{FALSE}}
+clang_analyzer_eval(x <= y); // expected-warning{{TRUE}}
+clang_analyzer_eval(y >= x); // expected-warning{{TRUE}}
+clang_analyzer_eval(x >= y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y <= x); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(x == y); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+clang_analyzer_eval(y == x); // expe

[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D79232#2016065 , @NoQ wrote:

> @baloghadamsoftware @steakhal @ASDenysPetrov will you be able to plug D49074 
> /D50256 
> /D77792 
> /D77802 
> /D78933  
> into this so that to separate algebra from pattern-matching and ensure no 
> performance regressions? Or is something still missing?


Sure. I'll plug my changes D77802 /D78933 
 as soon as this patch is available in the 
master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79232



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 4 inline comments as done.
ASDenysPetrov added a comment.

Thank you for the feedback. I'll update the patch soon.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:261-262
+// remove adjacent ranges
+newRanges = F.remove(newRanges, *iter1);
+newRanges = F.remove(newRanges, *iter2);
+// add united range

NoQ wrote:
> I don't remember iterator invalidation rules for these containers and it 
> gives me anxiety. Given that these containers are immutable, i wouldn't be 
> surprised if iterators are indeed never invalidated (just keep pointing to 
> the old container) but it definitely deserves a comment.
Thanks for the notice. I'm not sure about iterator invalidation, so I'll just 
replace `*iter2` with `*newRanges.begin()`.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),

NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > AFAIK since `std::initializer_list` is just two pointers we should take 
> > > it by value.
> > I'm not sure about //should//, since initializer_list is a container and 
> > it'd be consistent if we handle it like any other compound object despite 
> > its implementation (IMO). But I can change it if you wish.
> > `initializer_list` is a container
> 
> No, it's a //view// :)
> No, it's a view :)
Yes, you are right. OK, I'll change it to value argument.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;

NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Generally, `new expressions` are a code smell. We should use something 
> > > like an `std::make_unique` to prevent memory leaks on exceptions.
> > > Though, I'm not sure if there is a similar function for 
> > > `llvm::IntrusiveRefCntPtr`s.
> > I'll make it more safe.
> I'd rather create ASTContext through tooling, like in other unittests.
I'll look into other tests and try to change to ASTContext.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+  // c.original.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // c.expected.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedFromOriginal.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedBackward.print(llvm::dbgs());

NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Should we keep this?
> > I'm not sure, but I'd better keep it, because it is useful for debugging.
> No-no, we don't do that here >.>
> 
> If you constructed an awesome debugging facility and you want to save it for 
> future generations, i'd recommend turning it into an actual facility, not 
> just comments. Like, maybe, an unused function that can be invoked for 
> debugging, or something like that.
OK, I'll move it to function.


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

https://reviews.llvm.org/D77802



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

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 261784.
ASDenysPetrov added a comment.

@NoQ updated the patch. Review, please.


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

https://reviews.llvm.org/D77802

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/constraint_manager_negate_difference.c
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/RangeSetTest.cpp

Index: clang/unittests/StaticAnalyzer/RangeSetTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/RangeSetTest.cpp
@@ -0,0 +1,130 @@
+//===- unittests/StaticAnalyzer/RangeSetTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+// TestCase contains to lists of ranges.
+// Original one has to be negated.
+// Expected one has to be compared to negated original range.
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;
+
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),
+expected(createRangeSetFromList(BVF, F, expectedList)) {}
+
+private:
+  RangeSet createRangeSetFromList(BasicValueFactory &BVF, RangeSet::Factory &F,
+  const std::initializer_list rangeList) {
+llvm::APSInt from(sizeof(T) * 8, std::is_unsigned::value);
+llvm::APSInt to = from;
+RangeSet rangeSet = F.getEmptySet();
+for (auto it = rangeList.begin(); it != rangeList.end(); it += 2) {
+  from = *it;
+  to = *(it + 1);
+  rangeSet = rangeSet.addRange(
+  F, RangeSet(F, BVF.getValue(from), BVF.getValue(to)));
+}
+return rangeSet;
+  }
+
+  void printNegate(const TestCase &TestCase) {
+TestCase.original.print(llvm::dbgs());
+llvm::dbgs() << " => ";
+TestCase.expected.print(llvm::dbgs());
+  }
+};
+
+class RangeSetTest : public testing::Test {
+protected:
+  // Init block
+  std::unique_ptr AST = tooling::buildASTFromCode("struct foo;");
+  ASTContext &context = AST->getASTContext();
+  llvm::BumpPtrAllocator alloc;
+  BasicValueFactory BVF{context, alloc};
+  RangeSet::Factory F;
+  // End init block
+
+  template  void checkNegate() {
+using type = T;
+
+// Use next values of the range {MIN, A, B, MID, C, D, MAX}.
+
+// MID is a value in the middle of the range
+// which unary minus does not affect on,
+// e.g. int8/int32(0), uint8(128), uint32(2147483648).
+
+constexpr type MIN = std::numeric_limits::min();
+constexpr type MAX = std::numeric_limits::max();
+constexpr type MID = std::is_signed::value
+ ? 0
+ : ~(static_cast(-1) / static_cast(2));
+constexpr type A = MID - static_cast(42 + 42);
+constexpr type B = MID - static_cast(42);
+constexpr type C = -B;
+constexpr type D = -A;
+
+static_assert(MIN < A && A < B && B < MID && MID < C && C < D && D < MAX,
+  "Values shall be in an ascending order");
+
+// Left {[x, y], [x, y]} is what shall be negated.
+// Right {[x, y], [x, y]} is what shall be compared to a negation result.
+TestCase cases[] = {
+{BVF, F, {MIN, A}, {MIN, MIN, D, MAX}},
+{BVF, F, {MIN, C}, {MIN, MIN, B, MAX}},
+{BVF, F, {MIN, MID}, {MIN, MIN, MID, MAX}},
+{BVF, F, {MIN, MAX}, {MIN, MAX}},
+{BVF, F, {A, D}, {A, D}},
+{BVF, F, {A, B}, {C, D}},
+{BVF, F, {MIN, A, D, MAX}, {MIN, A, D, MAX}},
+{BVF, F, {MIN, B, MID, D}, {MIN, MIN, A, MID, C, MAX}},
+{BVF, F, {MIN, MID, C, D}, {MIN, MIN, A, B, MID, MAX}},
+{BVF, F, {MIN, MID, C, MAX}, {MIN, B, MID, MAX}},
+{BVF, F, {A, MID, D, MAX}, {MIN + 1, A, MID, D}},
+{BVF, F, {A, A}, {D, D}},
+{BVF, F, {MID, MID}, {MID, MID}},
+{BVF, F, {MAX, MAX}, {MIN + 1, MIN + 1}},
+};
+
+for (const auto &c : cases) {
+  // Negate original and check with expected.
+  RangeSet negatedFromOriginal = c.original.Negate(BVF, F);
+  EXPECT_EQ(negatedFromOriginal, c.expected);
+  // Negate negated back and check with original.
+  RangeSet negatedBackward = negatedFromOriginal.Negate(BVF, F);
+  EXPECT_EQ(negatedBackward, c.original);
+}
+  }
+};
+

[PATCH] D85026: [analyzer] Minor refactoring of SVal::getSubKind function

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov created this revision.
ASDenysPetrov added reviewers: NoQ, vsavchenko, Eugene.Zelenko, krememek, 
steakhal.
ASDenysPetrov added a project: clang.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
dexonsmith, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
ASDenysPetrov requested review of this revision.

`BaseMask` occupies the lowest bits. Effect of applying the mask is neutralized 
by right shift operation, thus making it useless.
Also change mask init value from //hex// to //bin// since it's more natural for 
mask.

Fix: Remove a redundant bitwise operation.

P.S. It hurts my eyes everytime I see it. I gave up.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85026

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -80,7 +80,7 @@
 #define ABSTRACT_SVAL_WITH_KIND(Id, Parent) Id ## Kind,
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
   };
-  enum { BaseBits = 2, BaseMask = 0x3 };
+  enum { BaseBits = 2, BaseMask = 0b11 };
 
 protected:
   const void *Data = nullptr;
@@ -116,7 +116,7 @@
 
   unsigned getRawKind() const { return Kind; }
   BaseKind getBaseKind() const { return (BaseKind) (Kind & BaseMask); }
-  unsigned getSubKind() const { return (Kind & ~BaseMask) >> BaseBits; }
+  unsigned getSubKind() const { return Kind >> BaseBits; }
 
   // This method is required for using SVal in a FoldingSetNode.  It
   // extracts a unique signature for this SVal object.


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -80,7 +80,7 @@
 #define ABSTRACT_SVAL_WITH_KIND(Id, Parent) Id ## Kind,
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
   };
-  enum { BaseBits = 2, BaseMask = 0x3 };
+  enum { BaseBits = 2, BaseMask = 0b11 };
 
 protected:
   const void *Data = nullptr;
@@ -116,7 +116,7 @@
 
   unsigned getRawKind() const { return Kind; }
   BaseKind getBaseKind() const { return (BaseKind) (Kind & BaseMask); }
-  unsigned getSubKind() const { return (Kind & ~BaseMask) >> BaseBits; }
+  unsigned getSubKind() const { return Kind >> BaseBits; }
 
   // This method is required for using SVal in a FoldingSetNode.  It
   // extracts a unique signature for this SVal object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85034: [analyzer] Simplified functions SVal::getAsSymbolicExpression and similar ones

[Denys Petrov via Phabricator via cfe-commits]

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

Simplified functions `SVal::getAsSymbolicExpression`, `SVal::getAsSymExpr` and 
`SVal::getAsSymbol`. After revision I concluded that `getAsSymbolicExpression` 
and `getAsSymExpr` repeat functionality of `getAsSymbol`, thus they can be 
removed.

Fix: Removed functions SVal::getAsSymbolicExpression and SVal::getAsSymExpr.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85034

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -86,7 +86,7 @@
 return makeLocAsInteger(LI->getLoc(), castSize);
   }
 
-  if (const SymExpr *se = val.getAsSymbolicExpression()) {
+  if (SymbolRef se = val.getAsSymbol()) {
 QualType T = Context.getCanonicalType(se->getType());
 // If types are the same or both are integers, ignore the cast.
 // FIXME: Remove this hack when we support symbolic truncation/extension.
Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -57,7 +57,7 @@
   // SymIntExprs.
   if (!canReasonAbout(Cond)) {
 // Just add the constraint to the expression without trying to simplify.
-SymbolRef Sym = Cond.getAsSymExpr();
+SymbolRef Sym = Cond.getAsSymbol();
 assert(Sym);
 return assumeSymUnsupported(State, Sym, Assumption);
   }
@@ -101,7 +101,7 @@
 
   if (!canReasonAbout(Value)) {
 // Just add the constraint to the expression without trying to simplify.
-SymbolRef Sym = Value.getAsSymExpr();
+SymbolRef Sym = Value.getAsSymbol();
 assert(Sym);
 return assumeSymInclusiveRange(State, Sym, From, To, InRange);
   }
Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -116,8 +116,6 @@
   return nullptr;
 }
 
-// TODO: The next 3 functions have to be simplified.
-
 /// If this SVal wraps a symbol return that SymbolRef.
 /// Otherwise, return 0.
 ///
@@ -132,22 +130,6 @@
   return getAsLocSymbol(IncludeBaseRegions);
 }
 
-/// getAsSymbolicExpression - If this Sval wraps a symbolic expression then
-///  return that expression.  Otherwise return NULL.
-const SymExpr *SVal::getAsSymbolicExpression() const {
-  if (Optional X = getAs())
-return X->getSymbol();
-
-  return getAsSymbol();
-}
-
-const SymExpr* SVal::getAsSymExpr() const {
-  const SymExpr* Sym = getAsSymbol();
-  if (!Sym)
-Sym = getAsSymbolicExpression();
-  return Sym;
-}
-
 const MemRegion *SVal::getAsRegion() const {
   if (Optional X = getAs())
 return X->getRegion();
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -377,8 +377,8 @@
 SVal SValBuilder::makeSymExprValNN(BinaryOperator::Opcode Op,
NonLoc LHS, NonLoc RHS,
QualType ResultTy) {
-  const SymExpr *symLHS = LHS.getAsSymExpr();
-  const SymExpr *symRHS = RHS.getAsSymExpr();
+  SymbolRef symLHS = LHS.getAsSymbol();
+  SymbolRef symRHS = RHS.getAsSymbol();
 
   // TODO: When the Max Complexity is reached, we should conjure a symbol
   // instead of generating an Unknown value and propagate the taint info to it.
@@ -492,7 +492,7 @@
   if (getContext().getTypeSize(castTy) >= getContext().getTypeSize(originalTy))
 return evalCast(val, castTy, originalTy);
 
-  const SymExpr *se = val.getAsSymbolicExpression();
+  SymbolRef se = val.getAsSymbol();
   if (!se) // Let evalCast handle non symbolic expressions.
 return evalCast(val, castTy, originalTy);
 
Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp
===

[PATCH] D85034: [analyzer] Simplify functions SVal::getAsSymbolicExpression and similar ones

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko

> I again have to nitpick about the commit message, can you please change it to 
> "Simplify ..."?

Ready! Sorry, I remember, you've already told me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85034

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

[PATCH] D85026: [analyzer] Introduce minor refactoring of SVal::getSubKind function

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko:

> However, I'm going to complain about commit messages again 😅 I would prefer 
> having imperative mood in the message, something like "Refactor ..." or 
> "Introduce minor refactoring..."

Changed title.

Thank you, guys!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85026

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

[PATCH] D85034: [analyzer] Simplify functions SVal::getAsSymbolicExpression and similar ones

[Denys Petrov 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 rG86e1b73507f3: [analyzer] Simplify function 
SVal::getAsSymbolicExpression and similar ones (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85034

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -86,7 +86,7 @@
 return makeLocAsInteger(LI->getLoc(), castSize);
   }
 
-  if (const SymExpr *se = val.getAsSymbolicExpression()) {
+  if (SymbolRef se = val.getAsSymbol()) {
 QualType T = Context.getCanonicalType(se->getType());
 // If types are the same or both are integers, ignore the cast.
 // FIXME: Remove this hack when we support symbolic truncation/extension.
Index: clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
@@ -57,7 +57,7 @@
   // SymIntExprs.
   if (!canReasonAbout(Cond)) {
 // Just add the constraint to the expression without trying to simplify.
-SymbolRef Sym = Cond.getAsSymExpr();
+SymbolRef Sym = Cond.getAsSymbol();
 assert(Sym);
 return assumeSymUnsupported(State, Sym, Assumption);
   }
@@ -101,7 +101,7 @@
 
   if (!canReasonAbout(Value)) {
 // Just add the constraint to the expression without trying to simplify.
-SymbolRef Sym = Value.getAsSymExpr();
+SymbolRef Sym = Value.getAsSymbol();
 assert(Sym);
 return assumeSymInclusiveRange(State, Sym, From, To, InRange);
   }
Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -116,8 +116,6 @@
   return nullptr;
 }
 
-// TODO: The next 3 functions have to be simplified.
-
 /// If this SVal wraps a symbol return that SymbolRef.
 /// Otherwise, return 0.
 ///
@@ -132,22 +130,6 @@
   return getAsLocSymbol(IncludeBaseRegions);
 }
 
-/// getAsSymbolicExpression - If this Sval wraps a symbolic expression then
-///  return that expression.  Otherwise return NULL.
-const SymExpr *SVal::getAsSymbolicExpression() const {
-  if (Optional X = getAs())
-return X->getSymbol();
-
-  return getAsSymbol();
-}
-
-const SymExpr* SVal::getAsSymExpr() const {
-  const SymExpr* Sym = getAsSymbol();
-  if (!Sym)
-Sym = getAsSymbolicExpression();
-  return Sym;
-}
-
 const MemRegion *SVal::getAsRegion() const {
   if (Optional X = getAs())
 return X->getRegion();
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -377,8 +377,8 @@
 SVal SValBuilder::makeSymExprValNN(BinaryOperator::Opcode Op,
NonLoc LHS, NonLoc RHS,
QualType ResultTy) {
-  const SymExpr *symLHS = LHS.getAsSymExpr();
-  const SymExpr *symRHS = RHS.getAsSymExpr();
+  SymbolRef symLHS = LHS.getAsSymbol();
+  SymbolRef symRHS = RHS.getAsSymbol();
 
   // TODO: When the Max Complexity is reached, we should conjure a symbol
   // instead of generating an Unknown value and propagate the taint info to it.
@@ -492,7 +492,7 @@
   if (getContext().getTypeSize(castTy) >= getContext().getTypeSize(originalTy))
 return evalCast(val, castTy, originalTy);
 
-  const SymExpr *se = val.getAsSymbolicExpression();
+  SymbolRef se = val.getAsSymbol();
   if (!se) // Let evalCast handle non symbolic expressions.
 return evalCast(val, castTy, originalTy);
 
Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- clang/lib/StaticAnalyzer/Core/ProgramState.cpp
+++ clang/lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -582,9 +582,6 @@
   if (SymbolRef Sym = val.getAsSymbol())
 return scan(Sym);
 
-  if (const SymExpr *Sym = val.getAsSymbolicExpression())
-return scan(Sym);
-
   if (Optiona

[PATCH] D85026: [analyzer] Introduce minor refactoring of SVal::getSubKind function

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG21fa82d5c63c: [analyzer] Introduce minor refactoring of 
SVal::getSubKind function (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85026

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -80,7 +80,7 @@
 #define ABSTRACT_SVAL_WITH_KIND(Id, Parent) Id ## Kind,
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
   };
-  enum { BaseBits = 2, BaseMask = 0x3 };
+  enum { BaseBits = 2, BaseMask = 0b11 };
 
 protected:
   const void *Data = nullptr;
@@ -116,7 +116,7 @@
 
   unsigned getRawKind() const { return Kind; }
   BaseKind getBaseKind() const { return (BaseKind) (Kind & BaseMask); }
-  unsigned getSubKind() const { return (Kind & ~BaseMask) >> BaseBits; }
+  unsigned getSubKind() const { return Kind >> BaseBits; }
 
   // This method is required for using SVal in a FoldingSetNode.  It
   // extracts a unique signature for this SVal object.


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -80,7 +80,7 @@
 #define ABSTRACT_SVAL_WITH_KIND(Id, Parent) Id ## Kind,
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
   };
-  enum { BaseBits = 2, BaseMask = 0x3 };
+  enum { BaseBits = 2, BaseMask = 0b11 };
 
 protected:
   const void *Data = nullptr;
@@ -116,7 +116,7 @@
 
   unsigned getRawKind() const { return Kind; }
   BaseKind getBaseKind() const { return (BaseKind) (Kind & BaseMask); }
-  unsigned getSubKind() const { return (Kind & ~BaseMask) >> BaseBits; }
+  unsigned getSubKind() const { return Kind >> BaseBits; }
 
   // This method is required for using SVal in a FoldingSetNode.  It
   // extracts a unique signature for this SVal object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.
Herald added a subscriber: steakhal.

Ping!




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:199
   // Utility methods
-  std::pair
-  static assumeZero(CheckerContext &C,
-ProgramStateRef state, SVal V, QualType Ty);
+  std::pair static assumeZero(
+  ProgramStateRef state, SVal V);

vsavchenko wrote:
> Can you please put `static` before return type, it will be more consistent 
> with other static methods nearby.
Sure, I just didn't pay attention to that.


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

https://reviews.llvm.org/D77062

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

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

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

This checker finds STL thread primitives misuse.

- std::mutex::unlock without std::mutex::lock
- std::mutex::lock twice

In future:

- std::mutex::lock without std::mutex::unlock (discuss)
- std::recursive_mutex support

P.S. This is an alpha version of my first checker. Do not hesitate to express 
your complaints!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85431

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
  clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp

Index: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.ThreadPrimitives -verify %s
+
+namespace std {
+struct mutex {
+  void lock();
+  void unlock();
+};
+template 
+struct guard_lock {
+  T &t;
+  guard_lock(T &m) : t(m) {
+t.lock();
+  }
+  ~guard_lock() {
+t.unlock();
+  }
+};
+} // namespace std
+
+void correct_lock_unlock(std::mutex m) {
+  m.lock();
+  m.unlock();
+}
+
+void incorrect_lock_unlock(std::mutex m1, std::mutex m2) {
+  m1.lock();
+  m2.unlock(); // expected-warning{{TRUE}}
+}
+
+void incorrect_lock_unlock2(std::mutex m, bool b) {
+  m.lock();
+  if(b)
+m.unlock();
+}
+
+void unlock_without_lock(std::mutex m) {
+  m.unlock(); // expected-warning{{TRUE}}
+}
+
+void twice_lock(std::mutex m) {
+  m.lock();
+  m.lock(); // expected-warning{{TRUE}}
+}
+
+void twice_lock2(std::mutex m) {
+  while (true)
+m.lock(); // expected-warning{{TRUE}}
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,118 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This checker finds STL thread primitives misuse.
+// - std::mutex::unlock without std::mutex::lock
+// - std::mutex::lock twice
+//
+//===--===//
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class ThreadPrimitivesChecker
+: public Checker {
+public:
+  ThreadPrimitivesChecker();
+
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+private:
+  mutable std::unique_ptr BT;
+  CallDescription LockFunc, UnlockFunc;
+
+  std::pair FindMutexLockOrUnlock(const CallEvent &Call,
+  CheckerContext &C) const;
+  void reportBug(CheckerContext &C, const char Msg[]) const;
+};
+
+} // namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
+ThreadPrimitivesChecker::ThreadPrimitivesChecker()
+: LockFunc({"std","mutex","lock"}, 0, 0), UnlockFunc({"std","mutex","unlock"}, 0, 0) {}
+
+
+std::pair ThreadPrimitivesChecker::FindMutexLockOrUnlock(
+const CallEvent &Call, CheckerContext &C) const {
+
+  if (const auto *MCall = dyn_cast(&Call)) {
+const bool IsLockFunc = MCall->isCalled(LockFunc);
+const bool IsUnlockFunc = IsLockFunc ? false : MCall->isCalled(UnlockFunc);
+return {IsLockFunc, IsUnlockFunc};
+  }
+  return {false, false};
+}
+
+void ThreadPrimitivesChecker::checkPostCall(const CallEvent &Call,
+CheckerContext &C) const {
+  // Find mutex::lock or mutex::unlock functions.
+  bool IsLockFunc;
+  bool IsUnlockFunc;
+  std::tie(IsLockFunc, IsUnlockFunc) = FindMutexLockOrUnloc

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 283916.
ASDenysPetrov added a comment.

@vsavchenko 
Made changes due to your remarks. Thanks you.


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

https://reviews.llvm.org/D85431

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
  clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp

Index: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.ThreadPrimitives -verify %s
+
+namespace std {
+struct mutex {
+  void lock();
+  void unlock();
+};
+template 
+struct guard_lock {
+  T &t;
+  guard_lock(T &m) : t(m) {
+t.lock();
+  }
+  ~guard_lock() {
+t.unlock();
+  }
+};
+} // namespace std
+
+void correct_lock_unlock(std::mutex m) {
+  m.lock();
+  m.unlock();
+}
+
+void incorrect_lock_unlock(std::mutex m1, std::mutex m2) {
+  m1.lock();
+  m2.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void incorrect_lock_unlock2(std::mutex m, bool b) {
+  m.lock();
+  if (b)
+m.unlock();
+}
+
+void unlock_without_lock(std::mutex m) {
+  m.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void twice_lock(std::mutex m) {
+  m.lock();
+  m.lock(); // expected-warning{{lock more than once}}
+}
+
+void twice_lock2(std::mutex m) {
+  while (true)
+m.lock(); // expected-warning{{lock more than once}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,111 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This checker finds STL thread primitives misuse.
+// - std::mutex::unlock without std::mutex::lock
+// - std::mutex::lock twice
+//
+//===--===//
+#include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum MutexFunc {
+
+};
+
+class ThreadPrimitivesChecker : public Checker {
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+private:
+  BugType BT{this, "STL thread primitives misuse", "std::mutex misuse"};
+  CallDescription LockFunc{{"std", "mutex", "lock"}, 0, 0};
+  CallDescription UnlockFunc{{"std", "mutex", "unlock"}, 0, 0};
+
+  std::pair FindMutexLockOrUnlock(const CallEvent &Call,
+  CheckerContext &C) const;
+  void reportBug(CheckerContext &C, const char Msg[]) const;
+};
+
+} // namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
+std::pair
+ThreadPrimitivesChecker::FindMutexLockOrUnlock(const CallEvent &Call,
+   CheckerContext &C) const {
+
+  if (const auto *MCall = dyn_cast(&Call)) {
+const bool IsLockFunc = MCall->isCalled(LockFunc);
+const bool IsUnlockFunc = IsLockFunc ? false : MCall->isCalled(UnlockFunc);
+return {IsLockFunc, IsUnlockFunc};
+  }
+  return {false, false};
+}
+
+void ThreadPrimitivesChecker::checkPostCall(const CallEvent &Call,
+CheckerContext &C) const {
+  // Find mutex::lock or mutex::unlock functions.
+  bool IsLockFunc;
+  bool IsUnlockFunc;
+  std::tie(IsLockFunc, IsUnlockFunc) = FindMutexLockOrUnlock(Call, C);
+
+  if (!IsLockFunc && !IsUnlockFunc)
+return;
+
+  // We are sure about cast here, because mutex::lock/unlock met before.
+  const auto *MCall = cast(&Call);
+  SVal SymVal = MCall->getCXXThisVal();
+  const bool LockFound = C.getState()->contains(SymVal);
+
+  if (LockFound) {
+if (IsLockFunc) {
+  reportBug(C, "Call mutex::lock more than once.");
+} else if (IsUnlockFunc) {
+  ProgramStateRef State = C.getState()->remove(SymVal);
+  C.a

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:46
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+

vsavchenko wrote:
> vsavchenko wrote:
> > You should also cleanup and remove dead symbols from the set.
> Maybe `SymbolRef` is more suitable here?
>Maybe SymbolRef is more suitable here?
I'm afraid it's not. Sval keeps some data but this data 
See, getCXXThisVal returns SVal which is MemRegionVal, thus I can't cast it to 
SymbolVal and get SymbolRef.
But I would appreciate if you could tell me what other actionsI should do to 
get a correct SymbolRef.


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

https://reviews.llvm.org/D85431

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

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 284353.
ASDenysPetrov added a comment.

Aded enum FuncIdKind to define function Ids.


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

https://reviews.llvm.org/D85431

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
  clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp

Index: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.ThreadPrimitives -verify %s
+
+namespace std {
+struct mutex {
+  void lock();
+  void unlock();
+};
+template 
+struct guard_lock {
+  T &t;
+  guard_lock(T &m) : t(m) {
+t.lock();
+  }
+  ~guard_lock() {
+t.unlock();
+  }
+};
+} // namespace std
+
+void correct_lock_unlock(std::mutex m) {
+  m.lock();
+  std::mutex *m2 = &m;
+  m2->unlock();
+}
+
+void correct_lock_unlock2(std::mutex m) {
+  m.lock();
+  std::mutex &m2 = m;
+  m2.unlock();
+}
+
+void correct_lock_unlock3(std::mutex m) {
+  m.lock();
+  m.unlock();
+}
+
+void correct_lock_unlock4(std::mutex m) {
+  std::guard_lock l(m);
+}
+
+void incorrect_lock_unlock(std::mutex m1, std::mutex m2) {
+  m1.lock();
+  m2.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void incorrect_lock_unlock2(std::mutex m, bool b) {
+  m.lock();
+  if (b)
+m.unlock();
+}
+
+void unlock_without_lock(std::mutex m) {
+  m.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void twice_lock(std::mutex m) {
+  m.lock();
+  m.lock(); // expected-warning{{lock more than once}}
+}
+
+void twice_lock2(std::mutex m) {
+  while (true)
+m.lock(); // expected-warning{{lock more than once}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,113 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This checker finds STL thread primitives misuse.
+// - std::mutex::unlock without std::mutex::lock
+// - std::mutex::lock twice
+//
+//===--===//
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum FuncIdKind : uint8_t {
+  Undefined = 0,
+  Mutex_Lock,
+  Mutex_Unlock,
+};
+
+static const std::pair FuncMapping[]{
+{Mutex_Lock, {{"std", "mutex", "lock"}, 0, 0}},
+{Mutex_Unlock, {{"std", "mutex", "unlock"}, 0, 0}},
+};
+
+class ThreadPrimitivesChecker : public Checker {
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+private:
+  BugType BT{this, "STL thread primitives misuse", "std::mutex misuse"};
+
+  FuncIdKind FindMutexLockOrUnlock(const CallEvent &Call,
+   CheckerContext &C) const;
+  void reportBug(CheckerContext &C, const char Msg[]) const;
+};
+
+} // namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
+FuncIdKind
+ThreadPrimitivesChecker::FindMutexLockOrUnlock(const CallEvent &Call,
+   CheckerContext &C) const {
+  if (const auto *MCall = dyn_cast(&Call))
+for (auto &P : FuncMapping)
+  if (MCall->isCalled(P.second))
+return P.first;
+  return Undefined;
+}
+
+void ThreadPrimitivesChecker::checkPostCall(const CallEvent &Call,
+CheckerContext &C) const {
+  // Find mutex::lock or mutex::unlock functions.
+  FuncIdKind FuncId = FindMutexLockOrUnlock(Call, C);
+
+  if (FuncId == Undefined)
+return;
+
+  // We are sure about cast here, because mutex::lock/unlock met before.
+  const auto *MCall = cast(&Call);
+  SVal SymVal = MCall->getCXXThisVal();
+  SymVal.dump();
+  const bool LockFound = C.getState()->contains(SymVal);
+
+  switch (FuncId) {
+  case Mutex_Lock:
+if (LockFound) {
+  reportBug(C, "Call mutex::lock more than once.");
+} else {
+  ProgramState

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 284373.
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

Added recursive_mutex support.

Please, look at a test file on TODOs. I want to cover those cases in the 
future. Could you advise me something on it?


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

https://reviews.llvm.org/D85431

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
  clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp

Index: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.ThreadPrimitives -verify %s
+
+namespace std {
+struct mutex {
+  void lock();
+  void unlock();
+};
+struct recursive_mutex {
+  void lock();
+  void unlock();
+};
+template 
+struct guard_lock {
+  T &t;
+  guard_lock(T &m) : t(m) {
+t.lock();
+  }
+  ~guard_lock() {
+t.unlock();
+  }
+};
+} // namespace std
+
+void correct_lock_unlock(std::mutex m) {
+  m.lock();
+  std::mutex *m2 = &m;
+  m2->unlock();
+}
+
+void correct_lock_unlock2(std::mutex m) {
+  m.lock();
+  std::mutex &m2 = m;
+  m2.unlock();
+}
+
+void correct_lock_unlock3(std::mutex m) {
+  m.lock();
+  m.unlock();
+}
+
+void correct_lock_unlock4(std::mutex m) {
+  std::guard_lock l(m);
+}
+
+void incorrect_lock_unlock(std::mutex m1, std::mutex m2, std::recursive_mutex rm1, std::recursive_mutex rm2) {
+  m1.lock();
+  m2.unlock(); // expected-warning{{unlock without lock}}
+  rm1.lock();
+  rm2.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void lock_without_unlock(std::mutex m, bool b) {
+  m.lock();
+  if (b)
+m.unlock(); // TODO: Detect case when unlock is in one branch only.
+}
+
+// TODO: Detect lock without unlock at the end of the block.
+void lock_without_unlock(std::mutex m, std::recursive_mutex rm2) {
+  m.lock();
+  rm2.lock();
+}
+
+void unlock_without_lock(std::mutex m, std::recursive_mutex rm) {
+  m.unlock();  // expected-warning{{unlock without lock}}
+  rm.unlock(); // expected-warning{{unlock without lock}}
+}
+
+void twice_lock(std::mutex m, std::recursive_mutex rm) {
+  m.lock();
+  m.lock(); // expected-warning{{lock more than once}}
+
+  rm.lock();
+  rm.lock(); // OK
+}
+
+void twice_lock2(std::mutex m, std::recursive_mutex rm) {
+  while (true) {
+m.lock();  // expected-warning{{lock more than once}}
+rm.lock(); // OK
+  }
+}
Index: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp
@@ -0,0 +1,121 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This checker finds STL thread primitives misuse.
+// - std::mutex::unlock without std::mutex::lock
+// - std::mutex::lock twice
+//
+//===--===//
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum FuncIdKind : uint8_t {
+  Undefined = 0,
+  Mutex_Lock,
+  Mutex_Unlock,
+  Recursive_Mutex_Lock,
+  Recursive_Mutex_Unlock,
+};
+
+static const std::pair FuncMapping[]{
+{Mutex_Lock, {{"std", "mutex", "lock"}, 0, 0}},
+{Mutex_Unlock, {{"std", "mutex", "unlock"}, 0, 0}},
+{Recursive_Mutex_Lock, {{"std", "recursive_mutex", "lock"}, 0, 0}},
+{Recursive_Mutex_Unlock, {{"std", "recursive_mutex", "unlock"}, 0, 0}},
+};
+
+class ThreadPrimitivesChecker : public Checker {
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+private:
+  BugType BT{this, "STL thread primitives misuse", "std::mutex misuse"};
+
+  FuncIdKind FindMutexLockOrUnlock(const CallEvent &Call,
+   CheckerContext &C) const;
+  void reportBug(CheckerContext &C, const char Msg[]) const;
+};
+
+} // namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
+FuncIdKind
+ThreadPrimitivesC

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ just one more ping. You accepted it and then I just revised it again. 
Could you, please, take a minute and look at it.
I'd close it with a great pleasure. It's been hanging too long.


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

https://reviews.llvm.org/D77062

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

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ

> Umm, why don't you extend `PthreadLockChecker` instead?

First of all I wanted to try to go through all the steps, since it's my first 
checker. The second is that I considered `PthreadLockChecker` designed for 
C-functions. Another reason is that it better helps me to test the code when it 
is separated from all other.
When I feel more confident and it makes sence I'll try to merge this patch into 
PthreadLockChecker. For now it's more just an idea to discuss, to collect 
expertise.


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

https://reviews.llvm.org/D85431

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

[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@NoQ

> You're on the right track but your checker repeats `PthreadLockChecker` 
> word-by-word. Like, you can find answers to all your questions (eg., "how to 
> use `isLiveRegion`?") by reading that checker. C++ functions aren't any 
> different from C functions; that's an extremely minor difference that doesn't 
> justify developing a new checker from scratch.

Thanks. I've already looked through PthreadLockChecker. Yes, it's pretty 
similar. I'll keep working separately for a while though as I feel more 
comfortable making mistakes.




Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:34
+
+static const std::pair FuncMapping[]{
+{Mutex_Lock, {{"std", "mutex", "lock"}, 0, 0}},

NoQ wrote:
> `CallDescriptionMap` is the modern API for this stuff.
Thanks. I'll look.


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

https://reviews.llvm.org/D85431

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

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> If index1 and index2 has the same value, we should not be confident that the 
> x == y holds.

Thanks! Now I see. Shame on me =)

> We know where it points to (aka. the pointer's value is conjured), but we 
> don't know what the value if there.

Absolutely right. I looked at this patch after a while and don't currently see 
a proper solution.

> I feel like this problem should be handled by some sort of invalidation 
> mechanism.

Definitely it should be some criteria/mark/flag about the region invalidation. 
Maybe someone more confident could prompt us how to.




Comment at: clang/test/Analysis/PR9289.cpp:13-21
+int index_int(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[42] < 2)
+var = 1;
+  if (a[42] < 2)
+ret = var; // no warning about garbage value

steakhal wrote:
> Why do we test this?
> Here we can //reason// about the index.
> This patch preserves the current behavior relating to this.
This is a root case of the bug which this patch came from. I decided to vary it 
by using an explicit index.



Comment at: clang/test/Analysis/PR9289.cpp:36-40
+  if (a[index] < 2)
+var = 1;
+  index = index2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined 
[core.uninitialized.Assign]}}

steakhal wrote:
> I'm not sure why do you overwrite the `index` if you could simply index with 
> `index2` instead in the following expression. That would make no difference, 
> only make the test more readable IMO.
> Same comment for all the`*_change` test cases.
My intention is to retain `index` inside brackets. Just to check that `index` 
changed indeed.


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

https://reviews.llvm.org/D81254

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

[PATCH] D85984: [analyzer] Add a new checker alpha.cplusplus.CPlusPlus11Lock

[Denys Petrov via Phabricator via cfe-commits]

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

This checker is made above PthreadLockChecker and works the same. It is adapted 
for **std::mutex** primitive.

Next I want to add support **std::recursive_mutex**.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85984

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp

Index: clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
@@ -0,0 +1,119 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.CPlusPlus11Lock -verify %s
+
+namespace std {
+
+struct mutex {
+  void lock();
+  bool try_lock();
+  void unlock();
+};
+
+struct recursive_mutex {
+  void lock();
+  bool try_lock();
+  void unlock();
+};
+
+template 
+struct guard_lock {
+  T &t;
+  guard_lock(T &m) : t(m) {
+t.lock();
+  }
+  ~guard_lock() {
+t.unlock();
+  }
+};
+} // namespace std
+
+std::mutex m1;
+std::mutex m2;
+std::recursive_mutex rm1;
+std::recursive_mutex rm2;
+
+// ok
+
+void ok1() {
+  m1.lock(); // no-warning
+}
+
+void ok2() {
+  m1.unlock(); // no-warning
+}
+
+void ok3() {
+  m1.lock();   // no-warning
+  m1.unlock(); // no-warning
+}
+
+void ok4() {
+  m1.lock();   // no-warning
+  m1.unlock(); // no-warning
+  m1.lock();   // no-warning
+  m1.unlock(); // no-warning
+}
+
+void ok5() {
+  m1.lock();   // no-warning
+  m1.unlock(); // no-warning
+  m2.lock();   // no-warning
+  m2.unlock(); // no-warning
+}
+
+void ok6(void) {
+  m1.lock();   // no-warning
+  m2.lock();   // no-warning
+  m2.unlock(); // no-warning
+  m1.unlock(); // no-warning
+}
+
+void ok7(void) {
+  if (m1.try_lock()) // no-warning
+m1.unlock(); // no-warning
+}
+
+void ok8(void) {
+  m1.unlock();   // no-warning
+  if (m1.try_lock()) // no-warning
+m1.unlock(); // no-warning
+}
+
+void ok9(void) {
+  if (!m1.try_lock()) // no-warning
+m1.lock();// no-warning
+  m1.unlock();// no-warning
+}
+
+void ok10() {
+  std::guard_lock gl(m1);
+}
+
+// bad
+
+void bad1() {
+  m1.lock(); // no-warning
+  m1.lock(); // expected-warning{{This lock has already been acquired}}
+}
+
+void bad2() {
+  m1.lock();   // no-warning
+  m1.unlock(); // no-warning
+  m1.unlock(); // expected-warning {{This lock has already been unlocked}}
+}
+
+void bad3() {
+  m1.lock();   // no-warning
+  m2.lock();   // no-warning
+  m1.unlock(); // expected-warning {{This was not the most recently acquired lock. Possible lock order reversal}}
+  m2.unlock(); // no-warning
+}
+
+void bad5() {
+  while (true)
+m1.unlock(); // expected-warning {{This lock has already been unlocked}}
+}
+
+void bad6() {
+  while (true)
+m1.lock(); // expected-warning{{This lock has already been acquired}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -70,11 +70,17 @@
 class PthreadLockChecker : public Checker {
 public:
-  enum LockingSemantics { NotApplicable = 0, PthreadSemantics, XNUSemantics };
+  enum LockingSemantics {
+NotApplicable = 0,
+PthreadSemantics,
+XNUSemantics,
+CPlusPlusSemantics,
+  };
   enum CheckerKind {
 CK_PthreadLockChecker,
 CK_FuchsiaLockChecker,
 CK_C11LockChecker,
+CK_CPlusPlus11LockChecker,
 CK_NumCheckKinds
   };
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
@@ -83,7 +89,7 @@
 private:
   typedef void (PthreadLockChecker::*FnCheck)(const CallEvent &Call,
   CheckerContext &C,
-  CheckerKind checkkind) const;
+  CheckerKind CheckKind) const;
   CallDescriptionMap PThreadCallbacks = {
   // Init.
   {{"pthread_mutex_init", 2}, &PthreadLockChecker::InitAnyLock},
@@ -164,49 +170,82 @@
   {{"mtx_destroy", 1}, &PthreadLockChecker::DestroyPthreadLock},
   };
 
+  CallDescriptionMap CPlusPlus11Callbacks = {
+
+  // Acquire.
+  {{{"std", "mutex", "lock"}, 0, 0},
+   &PthreadLockChecker::AcquireCPlusPlus11Lock},
+  // TODO {{{"std", "recursive_mutex", "lock"}, 0, 0},
+  // &PthreadLockChecker::AcquireCPlusPlus11RecursiveLock},
+
+  // Try.
+  {{{"std", "mutex", "try_lock"}, 0, 0},
+   &PthreadLockChecker::TryCPlusPlus11Lock},
+  // TODO {{{"std", "recursive_m

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Hey, folk, welcome to https://reviews.llvm.org/D85984
I've moved the logic of this checker in `PthreadLockChecker`

Should this revision be //closed //or //rejected //somehow?


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

https://reviews.llvm.org/D81254

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

[PATCH] D79336: [analyzer] Generalize bitwise OR rules for ranges

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:77
+  assert(!isEmpty());
+  // NOTE: It's a shame that we can't implement 'getMaxValue' without scanning
+  //   the whole tree to get to the last element.

vsavchenko wrote:
> xazax.hun wrote:
> > Yeah, this is quite unfortunate. But you might end up calling this a lot 
> > for bitwise operations. I wonder if it is worth to solve this problem 
> > before commiting this patch. I do not insist though. 
> I was even thinking about fixing it myself, this shouldn't be very hard, Also 
> addition of `lower_bound` and `upper_bound` can really help implementing 
> `Includes` for `RangeSet`.
Be aware as D77792 also implements such a method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79336



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