dcoughlin added a comment.
Thanks for the patch! This looks like it will catch some nasty bugs.
My comments (inline) are mostly nits. But two are more substantial: (1) you
should add a path note where the self-assignment assumption is made explaining
the assumption to the user and (2) the potential cost of the eager case split
could be high -- can it be mitigated by changing the inlining mode?
================
Comment at: lib/StaticAnalyzer/Checkers/SelfAssignmentChecker.cpp:13
@@ +12,3 @@
+// where the parameter refers to the same location where the this pointer
+// points to.
+//
----------------
An interesting part of this checker is that it doesn't actually perform any
checks itself. Instead, it helps other checkers finds bugs by causing the
analyzer to explore copy and move assignment operators twice: once for when
'this' aliases with the parameter and once for when it may not.
I think it is worth mentioning this explicitly in the comment because it
unusual. Most checkers don't work this way.
================
Comment at: lib/StaticAnalyzer/Checkers/SelfAssignmentChecker.cpp:26
@@ +25,3 @@
+
+class SelfAssignmentChecker : public Checker<check::BeginFunction> {
+public:
----------------
What do you think about renaming this class to something like
"CXXSelfAssignmentChecker"? I think name is a bit confusing because in
Objective-C there is a notion of assigning to `self` (which is the ObjC
equivalent of `this`).
================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:435
@@ -434,1 +434,3 @@
return false;
+ if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+ if(MD->isCopyAssignmentOperator()||MD->isMoveAssignmentOperator())
----------------
It would be good to add a comment explaining why we re-analyze these methods at
the top level.
================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:436
@@ +435,3 @@
+ if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+ if(MD->isCopyAssignmentOperator()||MD->isMoveAssignmentOperator())
+ return false;
----------------
The LLVM style is to put spaces after `if` and around `||`.
http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses
================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:450
@@ -445,5 +449,3 @@
// inlining when reanalyzing an already inlined function.
- if (Visited.count(D)) {
- assert(isa<ObjCMethodDecl>(D) &&
- "We are only reanalyzing ObjCMethods.");
+ if (Visited.count(D)&&isa<ObjCMethodDecl>(D)) {
const ObjCMethodDecl *ObjCM = cast<ObjCMethodDecl>(D);
----------------
One thing I am concerned about is that this will introduce an eager case split,
which can be quite expensive. We could potentially mitigate this cost by
changing `getInliningModeForFunction()` to return `ExprEngine::Inline_Minimal`
for the copy and move assignment operators when analyzing them at the top level
after they have already been inlined elsewhere. Inline_Minimal would prevent
most inlining in these methods and thus reduce cost, but would also reduce
coverage (leading to false negatives). Do you have a sense of how often
self-assignment issues arise in inlined calls vs. in the operators themselves?
(Also the spacing around `&&` doesn't match LLVM style here.)
================
Comment at: test/Analysis/self-assign-unused.cpp:21
@@ +20,3 @@
+
+String& String::operator=(const String &rhs) {
+ free(str);
----------------
I think it is important to add a path note saying something like "Assuming
'*this' is equal to 'rhs'".
To do this, you can add a new BugReporterVisitor subclass to
BugReporterVisitors.h that walks a error path backwards to add the note. There
is an example of how to do this in NonLocalizedStringBRVisitor in
LocalizationChecker.cpp.
Unlike the localization checker, however, the path note for the self assignment
checker will be added to errors generated by *other* checkers, so you should
register the new bug report visitor for all reports in
GRBugReporter::generatePathDiagnostic() (similar to NilReceiverBRVisitor).
================
Comment at: test/Analysis/self-assign-unused.cpp:22
@@ +21,3 @@
+String& String::operator=(const String &rhs) {
+ free(str);
+ str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}
----------------
I think it would be good to add;
```
clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
expected-warning{{UNKNOWN}}
```
here to explicitly test for the case split. You'll need to add a prototype
`void clang_analyzer_eval(int);` to use this.
================
Comment at: test/Analysis/self-assign-unused.cpp:26
@@ +25,3 @@
+}
+
+String::operator const char*() const {
----------------
It would be good to also test the move assignment operator.
================
Comment at: test/Analysis/self-assign-used.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze
-analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection %s -verify
+
----------------
Can you combine these into one test file? This way someone updating the tests
will know there is only one place to look.
http://reviews.llvm.org/D19311
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
