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

[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<ObjCForCollectionStmt>(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 (&Origin->getCFG() != &N->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 at -1{{Returning from 'initVar:param:'}}
+                                    // expected-note at -2{{Passing the value 0 via 2nd parameter 'param'}}
+                                    // expected-note at -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