subject:"\[PATCH\] D62883\: \[analyzer\] Track conditions of terminator statements on which the reported node depends on"

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-21 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 205954.
Szelethus added a comment.

- Collect only `ExplodedNode`s. Lesson learned!
- Add a `TODO:` in `trackExpressionValue` about maybe tracking conditions to 
all bug locations, rather than only for tracked variables.


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

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,287 @@
+// RUN: %clang_analyze_cc1 %s -verify -DTRACKING_CONDITIONS \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+//
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace global_variable_invalidation {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+  // expected-note@-3{{Value assigned to 'bar'}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace global_variable_invalidation
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo())
+#ifdef TRACKING_CONDITIONS
+// expected-note@-2{{Calling 'foo'}}
+// expected-note@-3{{Returning from 'foo'}}
+// expected-note@-4{{'flag' initialized here}}
+#endif // TRACKING_CONDITIONS
+// expected-note@-6{{Assuming 'flag' is not equal to 0}}
+// expected-note@-7{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+  operator bool()

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-20 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

Oh, wait, no, loops.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-20 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

In D62883#1552969 , @xazax.hun wrote:

> In D62883#1552870 , @Szelethus wrote:
>
> > - Uniqueing already tracked conditions as an (`Expr`, `ExplodedNode`) pair 
> > instead of on `Expr`
>
>
> I would be surprised to see the same ExploderNode multiple time a lot.  Also, 
> ExploderNode will have a program point, so having both an ExploderNode and an 
> Expr feels redundant to me.


(`Expr`, `LocationContext`) sounds right.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-20 Thread Gábor Horváth via Phabricator via cfe-commits

xazax.hun added a comment.

In D62883#1552870 , @Szelethus wrote:

> - Now using `CFGBlock::getTerminatorConditionExpr()`
> - Uniqueing already tracked conditions as an (`Expr`, `ExplodedNode`) pair 
> instead of on `Expr`


I would be surprised to see the same ExploderNode multiple time a lot.  Also, 
ExploderNode will have a program point, so having both an ExploderNode and an 
Expr feels redundant to me.

> - Add a `TODO:` about caching
> - Add plenty of new test cases for good measure (examples from my latest mail 
> http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html)




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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-20 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 205916.
Szelethus marked an inline comment as done.
Szelethus added a comment.

- Now using `CFGBlock::getTerminatorConditionExpr()`
- Uniqueing already tracked conditions as an (`Expr`, `ExplodedNode`) pair 
instead of on `Expr`
- Add a `TODO:` about caching
- Add plenty of new test cases for good measure (examples from my latest mail 
http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html)


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

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,287 @@
+// RUN: %clang_analyze_cc1 %s -verify -DTRACKING_CONDITIONS \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+//
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace global_variable_invalidation {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+  // expected-note@-3{{Value assigned to 'bar'}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace global_variable_invalidation
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo())
+#ifdef TRACKING_CONDITIONS
+// expected-note@-2{{Calling 'foo'}}
+// expected-note@-3{{Returning from 'foo'}}
+// expected-note@-4{{'flag' initialized here}}
+#endif // TRACKING_CONDITIONS
+// expected-note@-6{{Assuming 'flag' is not equal to 0}}
+// expected-note@-7{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+//

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-17 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1640-1642
+  CFGBlock *OriginB = GetRelevantBlock(Origin);
+  if (!OriginB || !NB)
+return nullptr;

Szelethus wrote:
> NoQ wrote:
> > `// TODO: This can be cached.`
> Like, caching the `CFGBlock`s for given `ExplodedNode`s?
I mean, on this line it's the same block every time (per instance of a visitor).


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-17 Thread Kristóf Umann via Phabricator via cfe-commits

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



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1609-1613
+  if (B->rbegin()->getKind() != CFGElement::Kind::Statement)
+return nullptr;
+
+  // This should be the condition of the terminator block.
+  const Stmt *S = B->rbegin()->castAs().getStmt();

NoQ wrote:
> A bit clearner:
> 
> ```lang=c++
> auto StmtElem = B->rbegin().getAs();
> if (!StmtElem)
>   return nullptr;
> 
> const Stmt *S = StmtElem->getStmt();
> ```
> 
> Also how about `CFGBlock::getTerminatorCondition()`?
I peeked at it's implementation, and it seems to be incorrect.

Referencing @xazax.hun's inline:
> I vaguely recall some problem with the results of getTerminatorStmt for 
> logical operators like &&. I believe what we really want is the last 
> expression we evaluated in the basic block which will be the last Stmt of the 
> basic block. So if we can settle with the last stmt we can get rid of this 
> code.

And this is what `CFGBlock::getTerminatorCondition()` does:
```lang=c++
Stmt *Terminator = getTerminatorStmt();
if (!Terminator)
  return nullptr;
 
Expr *E = nullptr;

switch (Terminator->getStmtClass()) {
  // ...
  case Stmt::BinaryOperatorClass: // '&&' and '||'
E = cast(Terminator)->getLHS();
  // 
}
return E;
```
But nevertheless, maybe it'd be good to fix this in there and use it here.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1640-1642
+  CFGBlock *OriginB = GetRelevantBlock(Origin);
+  if (!OriginB || !NB)
+return nullptr;

NoQ wrote:
> `// TODO: This can be cached.`
Like, caching the `CFGBlock`s for given `ExplodedNode`s?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1646
+if (const Expr *Condition = getTerminatorCondition(NB))
+  if (BR.addTrackedCondition(Condition))
+bugreporter::trackExpressionValue(

NoQ wrote:
> All right, i still don't understand this caching based on condition 
> expression.
> 
> You mean, like, if we're encountering the same condition multiple times (say, 
> in a loop), we should only track it once? Why? Like, its values (which are 
> the thing we'll really be tracking) may be different (say, on different loop 
> iterations).
Yup, can't argue with that, I'll revise this.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-17 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1609-1613
+  if (B->rbegin()->getKind() != CFGElement::Kind::Statement)
+return nullptr;
+
+  // This should be the condition of the terminator block.
+  const Stmt *S = B->rbegin()->castAs().getStmt();

A bit clearner:

```lang=c++
auto StmtElem = B->rbegin().getAs();
if (!StmtElem)
  return nullptr;

const Stmt *S = StmtElem->getStmt();
```

Also how about `CFGBlock::getTerminatorCondition()`?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1640-1642
+  CFGBlock *OriginB = GetRelevantBlock(Origin);
+  if (!OriginB || !NB)
+return nullptr;

`// TODO: This can be cached.`



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1646
+if (const Expr *Condition = getTerminatorCondition(NB))
+  if (BR.addTrackedCondition(Condition))
+bugreporter::trackExpressionValue(

All right, i still don't understand this caching based on condition expression.

You mean, like, if we're encountering the same condition multiple times (say, 
in a loop), we should only track it once? Why? Like, its values (which are the 
thing we'll really be tracking) may be different (say, on different loop 
iterations).


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-17 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso accepted this revision.
Charusso added a comment.

My opinion based on the Summary, which is the kind of the opposite what we 
have. No invalidation happens, so no swap happens, so it is kind of misleading 
like writing out "Returning something" where we cannot be sure what we return 
("resulting in new notes being placed at for the call to foo() on line 14 and a 
note about flag being invalidated on line 5.").
Please update the Summary according to the changes in the patch.

Also please note that, we are very pessimistic and everything is uninteresting 
(like returning unknown values), you only could `markInteresting()`.

In D62883#1545347 , @Szelethus wrote:

> I like to think that most of the unimportant notes can be easily categorized, 
> as seen above. With some, admittedly crude heuristics, we could cut these 
> down greatly, and leave some breathing room for fine-tuning it.

I am not against that direction, just you throw oil on fire, and we already 
have a huge fire (D62978 ). I believe with 
that we could see more bad-patterns and we could answer "In which range do we 
track?".
I hope you find the answer, and thanks for working on that to solve the problem!

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

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D62883#1545343 , @Charusso wrote:

> In D62883#1545341 , @Szelethus wrote:
>
> > In D62883#1545324 , @Charusso 
> > wrote:
> >
> > > As @NoQ pointed out we have some problem with that function. We are 
> > > tracking *values* without using the `redecl()`-chain (see in 
> > > https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or 
> > > without tracking conditions. You have solved(?) the latter, but the 
> > > former is equally important to solve.
> >
> >
> > I'm a little confused, which is the "former" and "latter" you're referring 
> > to?
>
>
> I believe you have solved the condition tracking as you move in-place what is 
> going on.

I'm still not sure I follow :) What are the two distinct things to be solved 
here?

>>> I believe this patch should be on by default, but it requires to fix the 
>>> "In which range do we track?" problem with D62978 
>>> .
>> 
>> I disagree with this. The reason why I'd like to make this an off-by-default 
>> option is to implement my followup improvements incrementally (not only 
>> that, but a whole family of conditions is going to be added), and allow us 
>> to observe the changes those make in relation to this patch -- besides, I 
>> don't really see this patch changing even if I manage to fix those issues. 
>> Would you like to see a change being made to this specific patch?
> 
> As you moving stuff around it just bypasses the usefulness of the mentioned 
> two visitors, which is a problem. Also you have mentioned some very crazy bad 
> side-effects which is yes, related to in which range do we operate.

Could you please elaborate? The particular thing I'm missing, I guess, is the 
need to get those issues fixed before this patch, and making this 
on-by-default. If anything, enabling a flag like this could really demonstrate 
changes on those problems. I also like to think that tracking a condition 
(especially a condition of a control dependency of yet another condition) is a 
drastically different case that tracking a "regular" variable (in particular, I 
think more aggressive pruning could be used), and such a change would 
definitely be out of scope for this patch.

I like to think that most of the unimportant notes can be easily categorized, 
as seen above. With some, admittedly crude heuristics, we could cut these down 
greatly, and leave some breathing room for fine-tuning it.

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

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204980.
Szelethus added a comment.

Add another `RUN:` line to see more clearly the effects of this patch.


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

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,164 @@
+// RUN: %clang_analyze_cc1 %s -verify -DTRACKING_CONDITIONS \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+//
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+  // expected-note@-3{{Value assigned to 'bar'}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo())
+#ifdef TRACKING_CONDITIONS
+// expected-note@-2{{Calling 'foo'}}
+// expected-note@-3{{Returning from 'foo'}}
+// expected-note@-4{{'flag' initialized here}}
+#endif // TRACKING_CONDITIONS
+// expected-note@-6{{Assuming 'flag' is not equal to 0}}
+// expected-note@-7{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+  operator bool() const { return coin(); }
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+};
+
+void test() {
+

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso added a comment.

In D62883#1545341 , @Szelethus wrote:

> In D62883#1545324 , @Charusso wrote:
>
> > As @NoQ pointed out we have some problem with that function. We are 
> > tracking *values* without using the `redecl()`-chain (see in 
> > https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
> > tracking conditions. You have solved(?) the latter, but the former is 
> > equally important to solve.
>
>
> I'm a little confused, which is the "former" and "latter" you're referring to?


I believe you have solved the condition tracking as you move in-place what is 
going on.

>> I believe this patch should be on by default, but it requires to fix the "In 
>> which range do we track?" problem with D62978 
>> .
> 
> I disagree with this. The reason why I'd like to make this an off-by-default 
> option is to implement my followup improvements incrementally (not only that, 
> but a whole family of conditions is going to be added), and allow us to 
> observe the changes those make in relation to this patch -- besides, I don't 
> really see this patch changing even if I manage to fix those issues. Would 
> you like to see a change being made to this specific patch?

As you moving stuff around it just bypasses the usefulness of the mentioned two 
visitors, which is a problem. Also you have mentioned some very crazy bad 
side-effects which is yes, related to in which range do we operate.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

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

In D62883#1545324 , @Charusso wrote:

> Hey! It is a cool patch as the smallest the scope the better to understand 
> what is going on. I like the direction, but we saw with @NoQ, we have to 
> think about "In which range do we track?".
>
> In D62883#1545282 , @Szelethus wrote:
>
> > - Hide the current implementation behind the off-by-default analyzer config 
> > `"track-conditions"` Do you think that this patch looks good enough now 
> > that it's off by default? Maybe it'd be easier to develop/review followup 
> > changes separately.
>
>
> It would be helpful to see what is exactly new, what trackExpressionValue() 
> cannot provide. Could you adjust the test case with a second run, where you 
> switch off that new feature, please? (I highly recommend that for future 
> developments also.)


You can see how this would look like by checking the history of this revision: 
https://reviews.llvm.org/D62883?vs=204554=204973=ignore-most#toc. 
Adding a second `RUN:` line might be a good idea though.

> In D62883#1544609 , @Szelethus wrote:
> 
>> In D62883#1544302 , @NoQ wrote:
>>
>> > On the other hand, all of these problems seem to be examples of the 
>> > problem of D62978 . Might it be that it's 
>> > the only thing that we're missing? Like, we need to formulate a rule 
>> > that'll give us an understanding of when do we track the value past the 
>> > point where it has been constrained to true or false - do we care about 
>> > the origins of the value or do we care about its constraints only? In case 
>> > of `flag` in the test examples, we clearly do. In case of these bools that 
>> > come from boolean conversions and assertions and such, we clearly don't. 
>> > What's the difference?
>> >
>> > How about we track the condition value past its collapse point only if it 
>> > involves an overwrite of a variable (or other region) from which the value 
>> > is loaded? Like, if there are no overwrites, stop at the collapse point. 
>> > If there are overwrites, stop at the oldest overwrite point (i.e., the 
>> > value was loaded from 'x' which was previously overwritten by the value of 
>> > value 'y' which was previously overwritten by the initial value of 
>> > variable 'z' => stop at the overwrite point of 'y').
>> >
>> > (then, again, not sure what happens if more nested stack frames are around)
>>
>>
>> For now, I'd like to restrict my efforts to simply "we should track this"  
>> or "we shouldn't track this". I think simply not tracking cases where the 
>> condition is a single variable assignment is a good initial approach. Let 
>> how the tracking is done be a worry for another day. Also, I'm a little 
>> confused, isn't this comment about how D62978 
>>  could be solved?
> 
> 
> As @NoQ pointed out we have some problem with that function. We are tracking 
> *values* without using the `redecl()`-chain (see in 
> https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
> tracking conditions. You have solved(?) the latter, but the former is equally 
> important to solve.

I'm a little confused, which is the "formet" and "latter" you're referring to?

> As I see that is the current situation:
> 
> - ReturnVisitor: give us too much unrelated information as we go out of scope.
> - FindLastStoreBRVisitor: do not use `redecls()` so gets confused with all 
> the hackish SVal conversions.
> - TrackControlDependencyCondBRVisitor: swaps notes which is questionable if I 
> want to see what the above two kindly provides for me, or I would remove them 
> instead.
> 
>   I believe this patch should be on by default, but it requires to fix the 
> "In which range do we track?" problem with D62978 
> .

I disagree with this. The reason why I'd like to make this an off-by-default 
option is to implement my followup improvements incrementally (not only that, 
but a whole family of conditions is going to be added), and allow us to observe 
the changes those make in relation to this patch -- besides, I don't really see 
this patch changing even if I manage to fix those issues. Would you like to see 
a change being made to this specific patch?


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

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

Hey! It is a cool patch as the smallest the scope the better to understand what 
is going on. I like the direction, but we saw with @NoQ, we have to think about 
"In which range do we track?".

In D62883#1545282 , @Szelethus wrote:

> - Hide the current implementation behind the off-by-default analyzer config 
> `"track-conditions"` Do you think that this patch looks good enough now that 
> it's off by default? Maybe it'd be easier to develop/review followup changes 
> separately.


It would be helpful to see what is exactly new, what trackExpressionValue() 
cannot provide. Could you adjust the test case with a second run, where you 
switch off that new feature, please? (I highly recommend that for future 
developments also.)

In D62883#1544609 , @Szelethus wrote:

> In D62883#1544302 , @NoQ wrote:
>
> > On the other hand, all of these problems seem to be examples of the problem 
> > of D62978 . Might it be that it's the only 
> > thing that we're missing? Like, we need to formulate a rule that'll give us 
> > an understanding of when do we track the value past the point where it has 
> > been constrained to true or false - do we care about the origins of the 
> > value or do we care about its constraints only? In case of `flag` in the 
> > test examples, we clearly do. In case of these bools that come from boolean 
> > conversions and assertions and such, we clearly don't. What's the 
> > difference?
> >
> > How about we track the condition value past its collapse point only if it 
> > involves an overwrite of a variable (or other region) from which the value 
> > is loaded? Like, if there are no overwrites, stop at the collapse point. If 
> > there are overwrites, stop at the oldest overwrite point (i.e., the value 
> > was loaded from 'x' which was previously overwritten by the value of value 
> > 'y' which was previously overwritten by the initial value of variable 'z' 
> > => stop at the overwrite point of 'y').
> >
> > (then, again, not sure what happens if more nested stack frames are around)
>
>
> For now, I'd like to restrict my efforts to simply "we should track this"  or 
> "we shouldn't track this". I think simply not tracking cases where the 
> condition is a single variable assignment is a good initial approach. Let how 
> the tracking is done be a worry for another day. Also, I'm a little confused, 
> isn't this comment about how D62978  could 
> be solved?


As @NoQ pointed out we have some problem with that function. We are tracking 
*values* without using the `redecl()`-chain (see in 
https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
tracking conditions. You have solved(?) the latter, but the former is equally 
important to solve.

As I see that is the current situation:

- ReturnVisitor: give us too much unrelated information as we go out of scope.
- FindLastStoreBRVisitor: do not use `redecls()` so gets confused with all the 
hackish SVal conversions.
- TrackControlDependencyCondBRVisitor: swaps notes which is questionable if I 
want to see what the above two kindly provides for me, or I would remove them 
instead.

I believe this patch should be on by default, but it requires to fix the "In 
which range do we track?" problem with D62978 .


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus marked 8 inline comments as done.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1850
+
+return const_cast(Node->getLocationContext()
+->getAnalysisDeclContext()->getCFGStmtMap()->getBlock(S));

xazax.hun wrote:
> Why do we need a ptr to non-const CFGBlock?
`llvm::IDFCalculator` takes the blocks as a non-const pointer.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204973.
Szelethus added a comment.

- Rebase on the rest of the patches
- Make `TrackControlDependencyCondBRVisitor` local to `BugReporterVisitors.cpp`
- Hide the current implementation behind the off-by-default analyzer config 
`"track-conditions"`
- Add two more testcases I'd like to improve on in followup patches

Do you think that this patch looks good enough now that it's off by default? 
Maybe it'd be easier to develop/review followup changes separately.


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

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+ // expected-note@-1 {{Value assigned to 'bar'}}
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin(); // expected-note {{Returning value}}
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo()) // expected-note {{Calling 'foo'}}
+// expected-note@-1{{Returning from 'foo'}}
+// expected-note@-2{{'flag' initialized here}}
+// expected-note@-3{{Assuming 'flag' is not equal to 0}}
+// expected-note@-4{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+  operator bool() const { return coin(); } // expected-note {{Returning value}}
+};
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (ConvertsToBool()) // expected-note {{Calling 'ConvertsToBool::operator bool'}}
+// expected-note@-1{{Returning from 'ConvertsToBool::operator bool'}}
+//

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet TrackedConditions;
+

xazax.hun wrote:
> Do we need this? I wonder if marking the result of the condition as 
> interesting is sufficient.
Later on, when I'm covering the "should-not-have-happened" case, it's possible 
that a condition would be added that wasn't even explored by the analyzer, so 
this is kinda necessary.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D62883#1542802 , @Szelethus wrote:

> If you hover your mouse over the icon, you can also read a comment of mine. 
> When you open a report, in the right corner of the source code you'll find a 
> dropdown menu, allowing you to see the report without this patch applied:
>  F9217110: image.png 


I added another run that displays notes at each condition that this new visitor 
tracks.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D62883#1544283 , @NoQ wrote:

> However, this heuristic breaks when the same code appears in an inlined stack 
> frame. Because given the context, we need to prove that this check makes 
> sense *in this context*. I strongly suspect that all of the heuristics that 
> you're talking about depend significantly on the surrounding call stacks.

I don't follow you here, unfortunately, could you please elaborate? Also, I'll 
come out with this right now: I never really understood these contexts, is 
there a handy doc or a paper I could take a look at?

> Like, i suspect it so strongly that i'm ready to admit that i've no idea what 
> we're doing anymore, and it makes sense to take a pen and a sheet of paper 
> and try to write down the tracking rules, and then come up with an easily 
> editable and extensible architecture for tweaking those rules as we discover 
> more and more of them.

That sounds great. The should-not-have-happened and the must-have-happened 
cases will all rely on this (since in both cases, at least for now, we'd like 
to use the extra information to track conditions), so maybe it's due to refresh 
`trackExpressionValue`. It would also make publishing and reviewing my works a 
lot easier.

In D62883#1544302 , @NoQ wrote:

> On the other hand, all of these problems seem to be examples of the problem 
> of D62978 . Might it be that it's the only 
> thing that we're missing? Like, we need to formulate a rule that'll give us 
> an understanding of when do we track the value past the point where it has 
> been constrained to true or false - do we care about the origins of the value 
> or do we care about its constraints only? In case of `flag` in the test 
> examples, we clearly do. In case of these bools that come from boolean 
> conversions and assertions and such, we clearly don't. What's the difference?
>
> How about we track the condition value past its collapse point only if it 
> involves an overwrite of a variable (or other region) from which the value is 
> loaded? Like, if there are no overwrites, stop at the collapse point. If 
> there are overwrites, stop at the oldest overwrite point (i.e., the value was 
> loaded from 'x' which was previously overwritten by the value of value 'y' 
> which was previously overwritten by the initial value of variable 'z' => stop 
> at the overwrite point of 'y').
>
> (then, again, not sure what happens if more nested stack frames are around)

For now, I'd like to restrict my efforts to simply "we should track this"  or 
"we shouldn't track this". I think simply not tracking cases where the 
condition is a single variable assignment is a good initial approach. Let how 
the tracking is done be a worry for another day. Also, I'm a little confused, 
isn't this comment about how D62978  could be 
solved?

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

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

(then, again, not sure what happens if more nested stack frames are around)


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

How about we track the condition value past its collapse point only if it 
involves an overwrite of a variable (or other region) from which the value is 
loaded? Like, if there are no overwrites, stop at the collapse point. If there 
are overwrites, stop at the oldest overwrite point (i.e., the value was loaded 
from 'x' which was previously overwritten by the value of value 'y' which was 
previously overwritten by the initial value of variable 'z' => stop at the 
overwrite point of 'y').


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:80-81
+
+  if (bar) // expected-note   {{Taking true branch}}
+   // expected-note@-1{{Assuming 'bar' is not equal to 0}}
+if (flag) // expected-note   {{Taking true branch}}

These notes should be in the opposite order. It doesn't matter for the test 
case, but it improves readability of the test case :)


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

In D62883#1542802 , @Szelethus wrote:

> Some conclusions:
>
> - Cases where the condition is also a variable initialization tend to expand 
> the bug path greatly. This isn't always bad, but should be noted. In general, 
> unless we have a good heuristic to figure out whether there is meaningful 
> information on the right hand side of the initialization, I think we just 
> shouldn't track these. A good example for this is this one 
> .
>  The 37th event contains pretty much every information we need, it's obvious 
> that the optional could either be None or non-None, since it's in a 
> condition. dyn_cast is a prime example too: in this case 
> ,
>  note 9 is all we really need.
> - We should probably believe that `operator bool()` is implemented correctly, 
> and shouldn't track the value all the way there (at least, when we're only 
> tracking the condition). Example 
> 
> - We shouldn't ever track assert-like conditions. Example 
> 
>  (note 38-41)
> - When the report didn't suffer from any of the above issues, I found the 
> extra notes to be helpful! :D

On the other hand, all of these problems seem to be examples of the problem of 
D62978 . Might it be that it's the only thing 
that we're missing? Like, we need to formulate a rule that'll give us an 
understanding of when do we track the value past the point where it has been 
constrained to true or false - do we care about the origins of the value or do 
we care about its constraints only? In case of `flag` in the test examples, we 
clearly do. In case of these bools that come from boolean conversions and 
assertions and such, we clearly don't. What's the difference?

Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:66
+void foo() {
+  // TODO: It makes no sense at all for bar to have been assigned here.
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}

It's invalidation of globals by `coin()` which potentially writes to `bar`.

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

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

> I am not sure about assuming operator bool being correct. I think we first 
> could think about other tricks to limit the tracking (see my idea above) and 
> if we fail I would only add such rules as a last resort.

I think this depends greatly on the stack frame layout. I seem to be perfectly 
fine with not tracking the value within `foo()` in

  if (foo()) {
...
  }

whenever `foo()` returns a `bool`. Simply because "why else would you add a 
check if it always returns the same value?".

However, this heuristic breaks when the same code appears in an inlined stack 
frame. Because given the context, we need to prove that this check makes sense 
*in this context*.

I strongly suspect that all of the heuristics that you're talking about depend 
significantly on the surrounding call stacks. Like, i suspect it so strongly 
that i'm ready to admit that i've no idea what we're doing anymore, and it 
makes sense to take a pen and a sheet of paper and try to write down the 
tracking rules, and then come up with an easily editable and extensible 
architecture for tweaking those rules as we discover more and more of them.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I don't understand why the visibility had to be changed (especially without 
explanation), and unless there's a strong reason for it, I strongly disagree 
with it.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Gábor Horváth via Phabricator via cfe-commits

xazax.hun added a comment.

I looked at bug path with the Optional. I did not debug into the analyzer but 
my intuition is the following: `DefChainEnd` is interesting. `TerminatedPaths` 
is the control dependency of `DefChainEnd`. And there are probably other things 
that are the control and/or value dependency of `TerminatedPaths` . One trick 
we could try is to limit the number of dependencies we track. For example, we 
could try what happens if we only track the immediate control dependencies of 
the original interesting region/symbol and does not add more control dependency 
recursively.

In case we find limiting the number of dependencies useful, I could imagine a 
user-facing flag like `report-verbosity`. The default one could include only 
the notes we find useful after limiting the tracking. A detailed option could 
add all the notes from the transitive tracking of dependencies. A verbose 
option could remove path pruning as well. This is just random brainstorming :) 
First, we need to so whether non-transitive tracking it is actually improving 
the situation.

I am not sure about assuming `operator bool` being correct. I think we first 
could think about other tricks to limit the tracking (see my idea above) and if 
we fail I would only add such rules as a last resort.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet TrackedConditions;
+

Do we need this? I wonder if marking the result of the condition as interesting 
is sufficient.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1850
+
+return const_cast(Node->getLocationContext()
+->getAnalysisDeclContext()->getCFGStmtMap()->getBlock(S));

Why do we need a ptr to non-const CFGBlock?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1895
+
+  if (ControlDepTree.isControlDependency(OriginB, NB))
+if (const Expr *Condition = getTerminatorCondition(NB))

Maybe this is the worng place to comment, but `isControlDependency` sounds a 
bit weird. How about `isControlDependenent`?


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

//*sloowly catches up >.<*//


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-13 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I took a look at the results. You can take a look at selected ones here:

http://cc.elte.hu:15001/Default/#report-hash==LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions=Confirmed=False%20positive=Intentional=New=Reopened=Unresolved=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions=on

Due to the lack of better categorization, I used the following (so this has in 
fact no relation to whether the report is correct or not):

- Intentional reports (green x) are the ones where the bugreport got 
unquestionably worse.
- Confirmed reports (red check mark) got definitely better
- False positives (grey line) are in between.

If you hover your mouse over the icon, you can also read a comment of mine. 
When you open a report, in the right corner of the source code you'll find a 
dropdown menu, allowing you to see the report without this patch applied:
F9217110: image.png 
There, you can observe the original bug report without this patch.

Some conclusions:

- Cases where the condition is also a variable initialization tend to expand 
the bug path greatly. This isn't always bad, but should be noted. In general, 
unless we have a good heuristic to figure out whether there is meaningful 
information on the right hand side of the initialization, I think we just 
shouldn't track these. A good example for this is this one 
.
 The 37th event contains pretty much every information we need, it's obvious 
that the optional could either be None or non-None, since it's in a condition. 
dyn_cast is a prime example too: in this case 
,
 note 9 is all we really need.
- We should probably believe that `operator bool()` is implemented correctly, 
and shouldn't track the value all the way there (at least, when we're only 
tracking the condition). Example 

- We shouldn't ever track assert-like conditions. Example 

 (note 38-41)
- When the report didn't suffer from any of the above issues, I found the extra 
notes to be helpful! :D

I'm doing another analysis to help a bit on evaluation with the following 
modification:

  diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  index 9c40ebead56..4ac4b873675 100644
  --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  @@ -1892,12 +1892,28 @@ TrackControlDependencyCondBRVisitor::VisitNode(const 
ExplodedNode *N,
 if (!OriginB || !NB)
   return nullptr;
   
  -  if (ControlDepTree.isControlDependency(OriginB, NB))
  -if (const Expr *Condition = getTerminatorCondition(NB))
  -  if (BR.addTrackedCondition(Condition))
  +  if (ControlDepTree.isControlDependency(OriginB, NB)) {
  +if (const Expr *Condition = getTerminatorCondition(NB)) {
  +  if (BR.addTrackedCondition(Condition)) {
   bugreporter::trackExpressionValue(
   N, Condition, BR, /*EnableNullFPSuppression=*/false);
   
  +if (BRC.getAnalyzerOptions().AnalysisDiagOpt == PD_NONE)
  +  return nullptr;
  +
  +std::string ConditionText = Lexer::getSourceText(
  +CharSourceRange::getTokenRange(Condition->getSourceRange()),
  +   BRC.getSourceManager(),
  +   
BRC.getASTContext().getLangOpts());
  +
  +return std::make_shared(
  +PathDiagnosticLocation::createEnd(
  +Condition, BRC.getSourceManager(), N->getLocationContext()),
  +"Tracking condition " + ConditionText);
  +  }
  +}
  +  }
  +
 return nullptr;
   }


CHANGES SINCE LAST ACTION

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-13 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I marked reports, confusingly, "Confirmed" if the extra notes were meaningful, 
"Intentional" if they were meaningless, and "False positive" if it's in between.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-13 Thread Kristóf Umann via Phabricator via cfe-commits

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



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:34
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}

NoQ wrote:
> Why?
I'll remove it in the next diff *pinky swear*.


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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-13 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204554.
Szelethus added a comment.

- Resolved some reviewer comments
- Added a `BugReport` level set to avoid tracking the same condition (which 
would result in an almost infinite loop)

Aaaand I have some results to show: http://cc.elte.hu:15001/Default/#

Sort by "Storage date", and look for "LLVM/Clang/Clang-tools-extra BEFORE 
tracking conditions" and "LLVM/Clang/Clang-tools-extra AFTER tracking 
conditions". I didn't have much time to draw conclusions yet, but it's clear 
that the amount of extra notes are intolerable.


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

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Analysis/Inputs/expected-plists/unix-fns.c.plist
  
clang/test/Analysis/diagnostics/Inputs/expected-plists/undef-value-param.m.plist
  clang/test/Analysis/diagnostics/no-store-func-path-notes.m
  clang/test/Analysis/diagnostics/undef-value-param.m
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}
+// expected-note@-1{{Assuming 'flag' is 0}}
+x = new int;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Taking true branch}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}
+// expected-note@-1{{Assuming 'flag' is 0}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Taking true branch}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+  // TODO: It makes no sense at all for bar to have been assigned here.
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+ // expected-note@-1 {{Value assigned to 'bar'}}
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (bar) // expected-note   {{Taking true branch}}
+   // expected-note@-1{{Assuming 'bar' is not equal to 0}}
+if (flag) // expected-note   {{Taking true branch}}
+  // expected-note@-1{{Assuming 'flag' is not equal to 0}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
Index: clang/test/Analysis/diagnostics/undef-value-param.m
===
--- clang/test/Analysis/diagnostics/undef-value-param.m
+++ clang/test/Analysis/diagnostics/undef-value-param.m
@@ -52,7 +52,7 @@
 
 static void CreateRef(SCDynamicStoreRef *storeRef, unsigned x) {
 unsigned err = 0;
-SCDynamicStoreRef ref = anotherCreateRef(, x);
+SCDynamicStoreRef ref = anotherCreateRef(, x); // expected-note{{Value assigned to 'err'}}
 if (err) { 
//expected-note@-1{{Assuming 'err' is not equal to

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-10 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

In D62883#1536894 , @Szelethus wrote:

> I'm not sure how long it'll take for me to figure out what's wrong, but these 
> numbers are so ridiculous, I suspect a programming error rather then an 
> algorithmic issue.
>
> edit: I seem to have found a solution, I'll share more later when I get to 
> evaluate this :)


At a glance, it might be that `bugreporter::trackExpressionValue()` is called 
on every node within the block, rather than only once in a lifetime of the 
visitor.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-10 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I implemented the fixes you commented on, but during evaluation it turned out 
that my visitor eats ram for breakfast, and then goes for seconds. I mean, like 
5-30x the normal memory consumption, and the same for analysis speed. I counted 
the number of concurrent instances (incrementing a static counter in ctor, 
decrementing in dtor), and it seems to go up into the hundreds of thousands, 
even millions before restarting the counter. I killed the process before 
crippling the server.

I'm not sure how long it'll take for me to figure out what's wrong, but these 
numbers are so ridiculous, I suspect a programming error rather then an 
algorithmic issue.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-05 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

> I think we should make clear that this visitor only operates within one 
> function and does not track controll dependencies across functions.

Hmm, maybe it's worth investigating whether if we pop the call stack, does the 
bug report improve if we update the visitor to build control dependencies for 
the recent function, and update the origin block to be the one where the 
function call was located.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-05 Thread Gábor Horváth via Phabricator via cfe-commits

xazax.hun requested changes to this revision.
xazax.hun added inline comments.
This revision now requires changes to proceed.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:176
+  const ExplodedNode *Origin;
+  CFGControlDependencyTree ControlDepTree;
+  llvm::SmallSet VisitedBlocks;

I think we should make clear that this visitor only operates within one 
function and does not track controll dependencies across functions. 



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:680
 
+} // end of anonymous namespace
+

What was the reason of adding these begin/ends? I prefer to keep such 
refactorings separate, also it might be controversial whether this change is 
desired.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1863-1864
+
+case Stmt::ObjCForCollectionStmtClass:
+  return cast(S)->getCollection();
+

NoQ wrote:
> Szelethus wrote:
> > @NoQ Any feelings on this?
> This makes some sense in the long run but i think you should give up here for 
> now. Unlike `CXXForRangeStmt`, its Objective-C counterpart doesn't mock up 
> the AST that would have made it look like a regular for-loop, so there just 
> isn't an AST node that would represent the part of it that corresponds to 
> "`__begin != __end`".
> 
> Even in the C++ case, i'm not sure your current behavior would make much 
> sense. We should probably delegate this work to a checker that knows how 
> collections work and what makes them empty or have a certain size, something 
> similar to what the `IteratorChecker` seems to be becoming :)
> 
> Should we give mark the report as invalid when we give up here? I've no idea, 
> i guess i'll have to gather more empirical evidence on that.
I vaguely recall some problem with the results of getTerminatorStmt for logical 
operators like &&. I believe what we really want is the last expression we 
evaluated in the basic block which will be the last Stmt of the basic block. So 
if we can settle with the last stmt we can get rid of this code. 



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1899-1901
+  CFGBlock *NB = GetRelevantBlock(N);
+  if (!VisitedBlocks.insert(NB).second)
+return nullptr;

NoQ wrote:
> I didn't really understand this part.
I guess detecting control depdendency only makes sense for the last statement 
of the basic block (which is the first one we see in the visitor order). But 
instead of maintaining a state with the visited nodes we could also check if 
the statement is the last one of its basic block. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-04 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

Yay, this thing really works!

> Now, whether this change is any good is practically impossible to tell 
> without evaluation on production code, so I'll get back with that once I 
> gather some data.

Yes. This patch deserves some data on how much have reports grown in length 
(i.e., how many have grown in length and how much did they do so). Ideally we 
should also have a look at how many of the new notes were truly useful. Eg., 
you can start with the old report, understand it (or fail to do so), then take 
the new report. If you get this feeling that's like "ugh, if i've seen this 
note before i would have figured this out much sooner!", then it's a good note. 
If the bug report was obvious to begin with and the note didn't add much, it's 
probably not a good note. But it's not necessarily bad as long as the report 
remains readable.

Additionally, it's likely that some reports will in fact get suppressed due to 
inline defensive checks suppressions kicking in over condition values. That'd 
be a lot of fun to see as well.

Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1863-1864
+
+case Stmt::ObjCForCollectionStmtClass:
+  return cast(S)->getCollection();
+

Szelethus wrote:
> @NoQ Any feelings on this?
This makes some sense in the long run but i think you should give up here for 
now. Unlike `CXXForRangeStmt`, its Objective-C counterpart doesn't mock up the 
AST that would have made it look like a regular for-loop, so there just isn't 
an AST node that would represent the part of it that corresponds to "`__begin 
!= __end`".

Even in the C++ case, i'm not sure your current behavior would make much sense. 
We should probably delegate this work to a checker that knows how collections 
work and what makes them empty or have a certain size, something similar to 
what the `IteratorChecker` seems to be becoming :)

Should we give mark the report as invalid when we give up here? I've no idea, i 
guess i'll have to gather more empirical evidence on that.

Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1894-1897
+  // If N is originaring from a different function than the node of interest,
+  // we can't reason about control dependencies.
+  if (>getCFG() != >getCFG())
+return nullptr;

What if they're in the same function but in different stack frames, i.e. due to 
recursion? I think you should just compare stack frame contexts here.

Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1899-1901
+  CFGBlock *NB = GetRelevantBlock(N);
+  if (!VisitedBlocks.insert(NB).second)
+return nullptr;

I didn't really understand this part.

Comment at: clang/test/Analysis/diagnostics/no-store-func-path-notes.m:26
+// expected-note@-1{{Returning from 
'initVar:param:'}}
+// expected-note@-2{{Passing the value 0 
via 2nd parameter 'param'}}
+// expected-note@-3{{'out' initialized to 
1}}

I suspect we'll have to play with the wording a little bit. The current 
`trackExpressionValue()` was worded for the scenario in which there's only one 
value that we're tracking. So it keeps saying things like "THE VALUE" as if 
we're focused on one value. In reality, however, "the value 0" isn't the same 
value as the "undefined or garbage value" that the report is about.

I think we should discriminate between these values somehow. Eg., "Passing the 
//condition// value 0 via 2nd parameter 'param'". We have this information when 
we're attaching the visitor, so we can keep passing it around.

Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:34
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}

Why?

Repository:
  rC Clang

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

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-04 Thread Kristóf Umann via Phabricator via cfe-commits

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

For the test files where plist files have changed: (note to self: there has 
**got to be** a better way to show this off)

F9062068: track_cond_undef_value_param_before.html 

F9062011: track_cond_undef_value_param_after.html 


F9062176: track_cond_unix_fns_before.html 
F9062279: track_cond_unix_fns_after.html 

F9062777: track_cond_retain_release_m_objc_1_before.html 

F9062772: track_cond_retain_release_m_objc_1_after.html 


(same for the .*objcpp file)
F9062784: track_cond_retain_release_m_objc_2_before.html 

F9062797: track_cond_retain_release_m_objc_2_after.html 


F9062927: track_cond_edges_new_before.html 
F9062929: track_cond_edges_new_after.html 

F9063089: track_cond_cxx_for_range_before.html 

F9063092: track_cond_cxx_for_range_after.html 





Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1863-1864
+
+case Stmt::ObjCForCollectionStmtClass:
+  return cast(S)->getCollection();
+

@NoQ Any feelings on this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-04 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus created this revision.
Szelethus added reviewers: NoQ, dcoughlin, a.sidorin, baloghadamsoftware, 
xazax.hun, Charusso, rnkovacs.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, szepet, whisperity.
Szelethus added a parent revision: D62619: [analyzer][Dominators] Add a control 
dependency tree builder + a new debug checker.

This patch implements the idea discussed on the mailing list 
, in fact, the 
included testfile contains the functions `example_1` and `example_2` exactly 
how it's described there.

The idea is to, as the title says, to track the value of the condition of the 
terminator statement on which the reported node depends on:

  01 int flag;
  02 bool coin();
  03 
  04 void foo() {
  05   flag = coin(); // no note
  06 }
  07 
  08 int main() {
  09   int *x = 0; // x initialized to 0
  10   flag = 1;
  11   foo();
  12   if (flag) // assumed false
  13 x = new int;
  14   foo();
  15 
  16   if (flag) // assumed true
  17 *x = 5; // warn
  18 }

We emit a warning at statement 17. The new BugReporter visitor figures out that 
statement 16 is in fact a control dependency if the reported node, and uses 
`trackExpressionValue()` to track it's condition, in this case, `flag`, 
resulting in new notes being placed at for the call to `foo()` on line 14 and a 
note about `flag` being invalidated on line 5.

Now, whether this change is any good is practically impossible to tell without 
evaluation on production code, so I'll get back with that once I gather some 
data.


Repository:
  rC Clang

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Analysis/Inputs/expected-plists/unix-fns.c.plist
  
clang/test/Analysis/diagnostics/Inputs/expected-plists/undef-value-param.m.plist
  clang/test/Analysis/diagnostics/no-store-func-path-notes.m
  clang/test/Analysis/diagnostics/undef-value-param.m
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note 2{{Value assigned to 'flag'}}
+}
+
+void example_1() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}
+// expected-note@-1{{Assuming 'flag' is 0}}
+x = new int;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Taking true branch}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+
+void example_2() {
+  int *x = 0;
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Taking false branch}}
+// expected-note@-1{{Assuming 'flag' is 0}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Taking true branch}}
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
Index: clang/test/Analysis/diagnostics/undef-value-param.m
===
--- clang/test/Analysis/diagnostics/undef-value-param.m
+++ clang/test/Analysis/diagnostics/undef-value-param.m
@@ -52,7 +52,7 @@
 
 static void CreateRef(SCDynamicStoreRef *storeRef, unsigned x) {
 unsigned err = 0;
-SCDynamicStoreRef ref = anotherCreateRef(, x);
+SCDynamicStoreRef ref = anotherCreateRef(, x); // expected-note{{Value assigned to 'err'}}
 if (err) { 
//expected-note@-1{{Assuming 'err' is not equal to 0}}
//expected-note@-2{{Taking true branch}}
Index: clang/test/Analysis/diagnostics/no-store-func-path-notes.m

38 matches

Mail list logo