[PATCH] D19311: [analyzer] Self Assignment Checker

[Devin Coughlin via cfe-commits]

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