Make -Wuninitialized warn on pointer-to-member and comma operators.

[epertoso]

Summary

A few lines taken from in http://llvm-reviews.chandlerc.com/D2181 that are unrelated to the main goal of that patch.

isTrackedVar has been updated to also track records.
DeclRefExprs appearing on the left side of a comma operator are ignored, while those appearing on the right side are classified as Use.

[jordan_rose]

Does that comma change do the right thing for "(foo(a), b)"? (i.e. does it still warn about 'a' being uninitialized?)

[epertoso]

Yes, it does.

DeclRefExprs are visited only by TransferFunctions, while ClassifyRefs relies mostly on binary operators, casts and unary operators to determine whether a variable is used or initialized.

For statements like c = (a, a) we need to classify the DeclRefExpr on the LHS of the comma operator as Ignore, or TransferFunctions will treat is as an initialization.

In the case of (foo(a), b), the LHS is a CallExpr and ClassifyRefs::classify has no effect on it, while a is the sub-expression of an lvalue-to-rvalue cast, and will be correctly classified as Use.

I added two lines to the test.

[rsmith]

lib/Analysis/UninitializedValues.cpp

436–457

It doesn't look like there's a new test case for this change. Is this somehow covered by our existing tests?

[epertoso]

lib/Analysis/UninitializedValues.cpp

436–457

Added one.

[rsmith]

LGTM

[epertoso]

Hi Richard,

even if phabricator mixes me up with the svn user 'enrico', I cannot commit.

[epertoso]

Updated to diff against head, as it should have been submitted quite a while ago.

[epertoso]

Richard, would you mind having another look at this?
It changed slightly after your LGTM.

[klimek]

LG. I think this looks fine from the version richard lg'ed.

lib/Analysis/UninitializedValues.cpp

 //==- UninitializedValues.cpp - Find Uninitialized Values -------*- C++ --*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements uninitialized values analysis for source-level CFGs.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Analysis/Analyses/PostOrderCFGView.h"
 #include "clang/Analysis/Analyses/UninitializedValues.h"
 #include "clang/Analysis/AnalysisContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/DomainSpecific/ObjCNoReturn.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/PackedVector.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include <utility>
 
 using namespace clang;
 
 #define DEBUG_LOGGING 0
 
 static bool isTrackedVar(const VarDecl *vd, const DeclContext *dc) {
   if (vd->isLocalVarDecl() && !vd->hasGlobalStorage() &&
       !vd->isExceptionVariable() && !vd->isInitCapture() &&
       vd->getDeclContext() == dc) {
     QualType ty = vd->getType();
-    return ty->isScalarType() || ty->isVectorType();
+    return ty->isScalarType() || ty->isVectorType() || ty->isRecordType();
   }
   return false;
 }
 
 //------------------------------------------------------------------------====//
 // DeclToIndex: a mapping from Decls we track to value indices.
 //====------------------------------------------------------------------------//
 
if (!VD || !isTrackedVar(VD))
       return Ignore;
 
     return Init;
   }
 };
 }
 
 static const DeclRefExpr *getSelfInitExpr(VarDecl *VD) {
+  if (VD->getType()->isRecordType()) return nullptr;
   if (Expr *Init = VD->getInit()) {
     const DeclRefExpr *DRE
       = dyn_cast<DeclRefExpr>(stripCasts(VD->getASTContext(), Init));
     if (DRE && DRE->getDecl() == VD)
       return DRE;
   }
   return nullptr;
 }
if (const BinaryConditionalOperator *BCO =
     return;
   }
 
   if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(E)) {
     classify(OVE->getSourceExpr(), C);
     return;
   }
 
+  if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
+    if (VarDecl *VD = dyn_cast<VarDecl>(ME->getMemberDecl())) {
+      if (!VD->isStaticDataMember())
+        classify(ME->getBase(), C);
+    }
+    return;
+  }
+
   if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
-    if (BO->getOpcode() == BO_Comma)
+    switch (BO->getOpcode()) {
+    case BO_PtrMemD:
+    case BO_PtrMemI:
+      classify(BO->getLHS(), C);
+      return;
+    case BO_Comma:
       classify(BO->getRHS(), C);
       return;
+    default:
+      return;
+    }
   }
 
   FindVarResult Var = findVar(E, DC);
   if (const DeclRefExpr *DRE = Var.getDeclRefExpr())
     Classification[DRE] = std::max(Classification[DRE], C);
 }
 
 void ClassifyRefs::VisitDeclStmt(DeclStmt *DS) {
   for (auto *DI : DS->decls()) {
     VarDecl *VD = dyn_cast<VarDecl>(DI);
     if (VD && isTrackedVar(VD))
       if (const DeclRefExpr *DRE = getSelfInitExpr(VD))
         Classification[DRE] = SelfInit;
   }
 }
 
 void ClassifyRefs::VisitBinaryOperator(BinaryOperator *BO) {
   // Ignore the evaluation of a DeclRefExpr on the LHS of an assignment. If this
   // is not a compound-assignment, we will treat it as initializing the variable
   // when TransferFunctions visits it. A compound-assignment does not affect
   // whether a variable is uninitialized, and there's no point counting it as a
   // use.
   if (BO->isCompoundAssignmentOp())
     classify(BO->getLHS(), Use);
-  else if (BO->getOpcode() == BO_Assign)
+  else if (BO->getOpcode() == BO_Assign || BO->getOpcode() == BO_Comma)
     classify(BO->getLHS(), Ignore);
 }
 
 void ClassifyRefs::VisitUnaryOperator(UnaryOperator *UO) {
   // Increment and decrement are uses despite there being no lvalue-to-rvalue
   // conversion.
   if (UO->isIncrementDecrementOp())
     classify(UO->getSubExpr(), Use);
 }
 
+static bool isPointerToConst(const QualType &QT) {
+  return QT->isAnyPointerType() && QT->getPointeeType().isConstQualified();
+}
+
 void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
   // Classify arguments to std::move as used.
   if (CE->getNumArgs() == 1) {
     if (FunctionDecl *FD = CE->getDirectCallee()) {
       if (FD->isInStdNamespace() && FD->getIdentifier() &&
           FD->getIdentifier()->isStr("move")) {
+        // RecordTypes are handled in SemaDeclCXX.cpp.
+        if (!CE->getArg(0)->getType()->isRecordType())
           classify(CE->getArg(0), Use);
         return;
       }
     }
   }
 
-  // If a value is passed by const reference to a function, we should not assume
-  // that it is initialized by the call, and we conservatively do not assume
-  // that it is used.
+  // If a value is passed by const pointer or by const reference to a function,
+  // we should not assume that it is initialized by the call, and we
+  // conservatively do not assume that it is used.
   for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
-       I != E; ++I)
-    if ((*I)->getType().isConstQualified() && (*I)->isGLValue())
-      classify(*I, Ignore);
+       I != E; ++I) {
+    if ((*I)->isGLValue()) {
+      if ((*I)->getType().isConstQualified())
+        classify((*I), Ignore);
+    } else if (isPointerToConst((*I)->getType())) {
+      const Expr *Ex = stripCasts(DC->getParentASTContext(), *I);
+      const UnaryOperator *UO = dyn_cast<UnaryOperator>(Ex);
+      if (UO && UO->getOpcode() == UO_AddrOf)
+        Ex = UO->getSubExpr();
+      classify(Ex, Ignore);
+    }
+  }
 }
 
 void ClassifyRefs::VisitCastExpr(CastExpr *CE) {
   if (CE->getCastKind() == CK_LValueToRValue)
     classify(CE->getSubExpr(), Use);
   else if (CStyleCastExpr *CSE = dyn_cast<CStyleCastExpr>(CE)) {
     if (CSE->getType()->isVoidType()) {
       // Squelch any detected load of an uninitialized value if

lib/Sema/SemaDecl.cpp

public:
     void HandleDeclRefExpr(DeclRefExpr *DRE) {
       Decl* ReferenceDecl = DRE->getDecl();
       if (OrigDecl != ReferenceDecl) return;
       unsigned diag;
       if (isReferenceType) {
         diag = diag::warn_uninit_self_reference_in_reference_init;
       } else if (cast<VarDecl>(OrigDecl)->isStaticLocal()) {
         diag = diag::warn_static_self_reference_in_init;
-      } else {
+      } else if (isa<TranslationUnitDecl>(OrigDecl->getDeclContext()) ||
+                 isa<NamespaceDecl>(OrigDecl->getDeclContext()) ||
+                 DRE->getDecl()->getType()->isRecordType()) {
         diag = diag::warn_uninit_self_reference_in_init;
+      } else {
+        // Local variables will be handled by the CFG analysis.
+        return;
       }
 
       S.DiagRuntimeBehavior(DRE->getLocStart(), DRE,
                             S.PDiag(diag)
                               << DRE->getNameInfo().getName()
                               << OrigDecl->getLocation()
                               << DRE->getSourceRange());
     }

test/SemaCXX/uninitialized.cpp

void test_stuff () {
   int v = (u += v); // expected-warning {{variable 'v' is uninitialized when used within its own initialization}}
   int w = (w += 10); // expected-warning {{variable 'w' is uninitialized when used within its own initialization}}
   int x = x++; // expected-warning {{variable 'x' is uninitialized when used within its own initialization}}
   int y = ((s ? (y, v) : (77, y))++, sizeof(y)); // expected-warning {{variable 'y' is uninitialized when used within its own initialization}}
   int z = ++ref(z);                              // expected-warning {{variable 'z' is uninitialized when used within its own initialization}}
   int aa = (ref(aa) += 10); // expected-warning {{variable 'aa' is uninitialized when used within its own initialization}}
   int bb = bb ? x : y; // expected-warning {{variable 'bb' is uninitialized when used within its own initialization}}
 
-
   for (;;) {
     int a = a; // no-warning: used to signal intended lack of initialization.
     int b = b + 1; // expected-warning {{variable 'b' is uninitialized when used within its own initialization}}
     int c = (c + c); // expected-warning {{variable 'c' is uninitialized when used within its own initialization}}
     int d = ({ d + d ;}); // expected-warning {{variable 'd' is uninitialized when used within its own initialization}}
     int e = static_cast<long>(e) + 1; // expected-warning {{variable 'e' is uninitialized when used within its own initialization}}
     int f = foo(f); // expected-warning {{variable 'f' is uninitialized when used within its own initialization}}
 
for (;;) {
     int y = ((s ? (y, v) : (77, y))++, sizeof(y)); // expected-warning {{variable 'y' is uninitialized when used within its own initialization}}
     int z = ++ref(z);                              // expected-warning {{variable 'z' is uninitialized when used within its own initialization}}
     int aa = (ref(aa) += 10); // expected-warning {{variable 'aa' is uninitialized when used within its own initialization}}
     int bb = bb ? x : y; // expected-warning {{variable 'bb' is uninitialized when used within its own initialization}}
 
   }
 }
 
+void test_comma() {
+  int a;  // expected-note {{initialize the variable 'a' to silence this warning}}
+  int b = (a, a ?: 2);  // expected-warning {{variable 'a' is uninitialized when used here}}
+  int c = (a, a, b, c);  // expected-warning {{variable 'c' is uninitialized when used within its own initialization}}
+  int d;  // expected-note {{initialize the variable 'd' to silence this warning}}
+  int e = (foo(d), e, b); // expected-warning {{variable 'd' is uninitialized when used here}}
+  int f;  // expected-note {{initialize the variable 'f' to silence this warning}}
+  f = f + 1, 2;  // expected-warning {{variable 'f' is uninitialized when used here}}
+  int h;
+  int g = (h, g, 2);  // no-warning: h, g are evaluated but not used.
+}
+
+namespace member_ptr {
+struct A {
+  int x;
+  int y;
+  A(int x) : x{x} {}
+};
+
+void test_member_ptr() {
+  int A::* px = &A::x;
+  A a{a.*px}; // expected-warning {{variable 'a' is uninitialized when used within its own initialization}}
+  A b = b; // expected-warning {{variable 'b' is uninitialized when used within its own initialization}}
+}
+}
+
+namespace const_ptr {
+void foo(int *a);
+void bar(const int *a);
+void foobar(const int **a);
+
+void test_const_ptr() {
+  int a;
+  int b;  // expected-note {{initialize the variable 'b' to silence this warning}}
+  foo(&a);
+  bar(&b);
+  b = a + b; // expected-warning {{variable 'b' is uninitialized when used here}}
+  int *ptr;  //expected-note {{initialize the variable 'ptr' to silence this warning}}
+  const int *ptr2;
+  foo(ptr); // expected-warning {{variable 'ptr' is uninitialized when used here}}
+  foobar(&ptr2);
+}
+}
+
 // Also test similar constructs in a field's initializer.
 struct S {
   int x;
   int y;
   const int z = 5;
   void *ptr;
 
   S(bool (*)[1]) : x(x) {} // expected-warning {{field 'x' is uninitialized when used here}}