[analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors.

[NoQ]

Summary

When a class forgets to initialize a field in the constructor, and then gets copied around, a warning is emitted that the value assigned to a specific field is undefined. When the copy/move constructor is implicit (not written out in the code) but not trivial (is not a trivial memory copy, eg. because members have an explicit copy constructor), the body of such constructor is auto-generated in the AST. In this case the checker's warning message is squeezed at the top of the class declaration, and it gets hard to guess which field is at fault.

Fix the warning message to include the name of the field.

Such warnings have become fairly popular in our temporary constructor evaluation.

This warning should probably be extended to the trivial copy case, but that's a separate story.

[NoQ]

See also rC326165.

[a.sidorin]

lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp

60 ↗

(On Diff #136014)

SmallString<128>?

98 ↗

(On Diff #136014)

May be it is better to print qualified name? Just name of member can be misleading if base classes have members with same names.

[NoQ]

Use SmallString.

Add some test for base-class implicit constructors to see how it currently works (not ideal but good enough).

lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp

60 ↗

(On Diff #136014)

*recalls how it's done* Ok!

98 ↗

(On Diff #136014)

I don't think this can happen in an implicit constructor. We'd jump to the base-class constructor in this case.

[dcoughlin]

LGTM. I have a suggestion for a slight modification of the diagnostic text inline.

test/Analysis/implicit-ctor-undef-value.cpp

12 ↗

(On Diff #136145)

I think it would be good to mention that this assignment is happening in an implicit constructor, since that is probably not obvious to most users.

How about: "Value assigned to field 'y' in implicit constructor is garbage or undefined".

[NoQ]

Mention the implicit constructor in the warning message.

cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp

if (const FunctionDecl *EnclosingFunctionDecl =
     if (C.getCalleeName(EnclosingFunctionDecl) == "swap")
       return;
 
   ExplodedNode *N = C.generateErrorNode();
 
   if (!N)
     return;
 
-  const char *str = "Assigned value is garbage or undefined";
-
+  static const char *const DefaultMsg =
+      "Assigned value is garbage or undefined";
   if (!BT)
-    BT.reset(new BuiltinBug(this, str));
+    BT.reset(new BuiltinBug(this, DefaultMsg));
 
   // Generate a report for this bug.
+  llvm::SmallString<128> Str;
+  llvm::raw_svector_ostream OS(Str);
+
   const Expr *ex = nullptr;
 
   while (StoreE) {
     if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
-      str = "The expression is an uninitialized value. "
+      OS << "The expression is an uninitialized value. "
             "The computed value will also be garbage";
 
       ex = U->getSubExpr();
       break;
     }
 
     if (const BinaryOperator *B = dyn_cast<BinaryOperator>(StoreE)) {
       if (B->isCompoundAssignmentOp()) {
         if (C.getSVal(B->getLHS()).isUndef()) {
-          str = "The left expression of the compound assignment is an "
+          OS << "The left expression of the compound assignment is an "
                 "uninitialized value. The computed value will also be garbage";
           ex = B->getLHS();
           break;
         }
       }
 
       ex = B->getRHS();
       break;
     }
 
     if (const DeclStmt *DS = dyn_cast<DeclStmt>(StoreE)) {
       const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl());
       ex = VD->getInit();
     }
 
+    if (const auto *CD =
+            dyn_cast<CXXConstructorDecl>(C.getStackFrame()->getDecl())) {
+      if (CD->isImplicit()) {
+        for (auto I : CD->inits()) {
+          if (I->getInit()->IgnoreImpCasts() == StoreE) {
+            OS << "Value assigned to field '" << I->getMember()->getName()
+               << "' in implicit constructor is garbage or undefined";
             break;
           }
+        }
+      }
+    }
+
+    break;
+  }
+
+  if (OS.str().empty())
+    OS << DefaultMsg;
 
-  auto R = llvm::make_unique<BugReport>(*BT, str, N);
+  auto R = llvm::make_unique<BugReport>(*BT, OS.str(), N);
   if (ex) {
     R->addRange(ex->getSourceRange());
     bugreporter::trackNullOrUndefValue(N, ex, *R);
   }
   C.emitReport(std::move(R));
 }
 
 void ento::registerUndefinedAssignmentChecker(CheckerManager &mgr) {
   mgr.registerChecker<UndefinedAssignmentChecker>();
 }

cfe/trunk/test/Analysis/implicit-ctor-undef-value.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=text -verify %s
+
+namespace implicit_constructor {
+struct S {
+public:
+  S() {}
+  S(const S &) {}
+};
+
+// Warning is in a weird position because the body of the constructor is
+// missing. Specify which field is being assigned.
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+          // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+  int x, y;
+  S s;
+
+public:
+  C(): x(0) {}
+};
+
+void test() {
+  C c1;
+  C c2(c1); // expected-note{{Calling implicit copy constructor for 'C'}}
+}
+} // end namespace implicit_constructor
+
+
+namespace explicit_constructor {
+class C {
+  int x, y;
+
+public:
+  C(): x(0) {}
+  // It is not necessary to specify which field is being assigned to.
+  C(const C &c):
+    x(c.x),
+    y(c.y) // expected-warning{{Assigned value is garbage or undefined}}
+           // expected-note@-1{{Assigned value is garbage or undefined}}
+  {}
+};
+
+void test() {
+  C c1;
+  C c2(c1); // expected-note{{Calling copy constructor for 'C'}}
+}
+} // end namespace explicit_constructor
+
+
+namespace base_class_constructor {
+struct S {
+public:
+  S() {}
+  S(const S &) {}
+};
+
+class C { // expected-warning{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+          // expected-note@-1{{Value assigned to field 'y' in implicit constructor is garbage or undefined}}
+  int x, y;
+  S s;
+
+public:
+  C(): x(0) {}
+};
+
+class D: public C {
+public:
+  D(): C() {}
+};
+
+void test() {
+  D d1;
+  D d2(d1); // expected-note   {{Calling implicit copy constructor for 'D'}}
+            // expected-note@-1{{Calling implicit copy constructor for 'C'}}
+}
+} // end namespace base_class_constructor