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

421–442

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

421–442

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

 
 #define DEBUG_LOGGING 0
 
 static bool isTrackedVar(const VarDecl *vd, const DeclContext *dc) {
   if (vd->isLocalVarDecl() && !vd->hasGlobalStorage() &&
       !vd->isExceptionVariable() &&
       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 (DRE && DRE->getDecl() == VD)
       return DRE;
   }
   return 0;
 }
 
 void ClassifyRefs::classify(const Expr *E, Class C) {
   // The result of a ?: could also be an lvalue.
   E = E->IgnoreParens();
-  if (const ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {
-    const Expr *TrueExpr = CO->getTrueExpr();
+  if (const AbstractConditionalOperator *ACO =
+          dyn_cast<AbstractConditionalOperator>(E)) {
+    if (isa<BinaryConditionalOperator>(ACO))
+      classify(cast<BinaryConditionalOperator>(ACO)->getCommon(), Use);
+    const Expr *TrueExpr = ACO->getTrueExpr();
     if (!isa<OpaqueValueExpr>(TrueExpr))
       classify(TrueExpr, C);
-    classify(CO->getFalseExpr(), C);
+    classify(ACO->getFalseExpr(), C);
     return;
   }
 
+  if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+    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 (DeclStmt::decl_iterator DI = DS->decl_begin(), DE = DS->decl_end();
        DI != DE; ++DI) {
     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) {
-  // 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.
-  for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
-       I != E; ++I)
-    if ((*I)->getType().isConstQualified() && (*I)->isGLValue())
-      classify(*I, Ignore);
+  // 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)->isGLValue()) {
+      if ((*I)->getType().isConstQualified())
+        classify((*I), Ignore);
+      continue;
+    } 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

test/SemaCXX/uninitialized.cpp

for (;;) {
     int k = __alignof__(k);
 
     int l = k ? l : l;  // expected-warning {{variable 'l' is uninitialized when used within its own initialization}}
     int m = 1 + (k ? m : m);  // expected-warning {{'m' is uninitialized when used within its own initialization}}
     int n = -n;  // expected-warning {{variable 'n' 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}}
+}
+
+namespace mem_ptr {
+struct A {
+  int x;
+  int y;
+};
+
+void foo() {
+  int A::* px = &A::x;
+  A a{ 1, 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}}
+}
+}
+
 // Test self-references with record types.
 class A {
   // Non-POD class.
   public:
     enum count { ONE, TWO, THREE };
     int num;
     static int count;
     int get() const { return num; }