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@-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
