[PATCH] Make UninitializedValues detect the use of uninitialized fields in a constructor.

[Richard Smith]

================
Comment at: lib/Analysis/UninitializedValues.cpp:571
@@ +570,3 @@
+  if (ME == findLeftmostMemberExpr(ME) &&
+      ME->getMemberDecl()->getType()->isReferenceType()) {
+    classify(ME, Use);
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > Seems this case should apply if any of the `FieldDecls` in the chain are of reference type, not just if the last one is.
> I'm not sure I understand:
> 
>   lang=c++
>   struct A {
>     int &a;
>   };
>   struct B {
>     A &a;
>     B(int) {}
>   };
>   struct C {
>     B &ref;
>     B b;
>     C() : b(ref.a.a) {}
>     C(B &o) : b(o.a.a) {}
>   };
>   
> The only thing we know here is that "ref" is uninitialized.
> Do you have anything specific in mind?
Suppose we had:

  struct A { int n; };
  struct B { A &a; };
  struct C {
    int n;
    B b;
    C(A &a) : b{a}, n(b.a.n = 0) {}
  };

Here (because the `n` initializer runs before the `b` initializer), `b` is uninitialized in the `b.a.n = 0` expression. Because `b.a` is a reference, that expression is a use of `b`, not an initialization of it. So we should diagnose this.

================
Comment at: lib/Analysis/UninitializedValues.cpp:581-586
@@ -426,1 +580,8 @@
 
+void ClassifyRefs::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE) {
+  if (DIE->getExpr() == 0)
+    return;
+  CXXDefaultInitExprVisitor<ClassifyRefs> V(*this);
+  V.Visit((Stmt*) DIE->getExpr());
+}
+
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > This looks suspicious. If we're visiting a `CXXDefaultInitExpr` for an object other than `this`, and that initializer references `this`, we'll think that we have a reference to one of our subobjects, but we don't.
> ##isTrackedField## verifies that the specified field has the same parent of the constructor. A ##CXXDefaultInitExpr## appears either in a ##CtorInitializer## or within the context of (c++1y) aggregate initialization, in which case the record of the current constructor and that of any ##MemberExpr## in the aggregate can't be the same.
I don't think that is the case. For instance:

  struct A {
    int a;
    int b;
    int c;
    struct B;
    A();
  };
  struct A::B : A {
    int d = (c = 0);
  };
  A::A() : a(B{}.d), b(c) {} // B{} should not mark our `c` as initialized.

As I alluded to in another comment, I think we could handle this correctly by not walking into a `CXXDefaultInitExpr` in aggregate initialization (perhaps only walk into them when they appear at the top level in a `CtorInitializer`, and do that in the CFG builder).

================
Comment at: lib/Analysis/UninitializedValues.cpp:795-796
@@ +794,4 @@
+      E = UO->getSubExpr()->IgnoreParenCasts();
+    else if (const ArraySubscriptExpr *ASE = dyn_cast<ArraySubscriptExpr>(E))
+      E = ASE->getLHS()->IgnoreParenCasts();
+  } while (OE != E);
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > I assume you're trying to cope with `this[0].blah` here? This doesn't look quite right: it'll also find `this[-1].blah` (which might sometimes be valid) and will do the wrong thing on `0[this].blah`. Maybe just drop the array indexing support here -- it's a corner case and not worth the cost of getting all the details right.
> Actually no. This just detects things like
> 
>   lang=c++
>   struct X {
>     int x, y;
>     X(int f) {
>       reinterpret_cast<int*>(this)[0] = f;
>       y = x;
>     }
>   };
> 
> Warning on ##y = x## is wrong. On the other hand, I'm simply doing as if ##this## escaped analysis here, and yes, it won't work on ##0[reinterpret_cast<int*>(this)] = f##.
> It's still seems to be a corner case, though, plus it's not warning on member access via ##this[0]##.
> 
> Should I still drop this?
Unless you have real-world examples of bugs the extra accuracy catches here, I'd prefer to drop this for simplicity's sake.

================
Comment at: lib/Analysis/UninitializedValues.cpp:317-323
@@ +316,9 @@
+  void VisitStmt(Stmt* S) {
+    llvm::SmallVector<Stmt*, 12> children;
+    for (Stmt::child_range I = S->children(); I; ++I) {
+      children.push_back(*I);
+    }
+    for (std::size_t i = children.size(); i > 0; --i) {
+      Visit(children[i - 1]);
+    }
+  }
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > Why do you need the extra complexity of building a list and iterating backwards here, rather than just visiting the children inside the first loop?
> That's the way statements are appended to the CFG in the base case.
> But it doesn't seem to cause problems if I don't do it here (##classify## is always called when there's enough context).
> 
Hmm, does this do good things if there is control flow in the `CXXDefaultInitExpr`?

  struct S {
    enum Kind { Foo, Bar } K;
    int FooVal;
    int BarVal;
    int Value = K == Foo ? FooVal : BarVal;
    S() : K(Foo), FooVal(0) {} // warning about uninitialized use of BarVal?
  };

Maybe we should teach the CFG builder to inline `CXXDefaultInitExprs` when they appear as a `CXXCtorInitializer`? That should not break the CFG's invariants, and allow you to drop a lot (all?) of your special-case handling for them (since you don't actually want to walk into them when they occur within aggregate initialization).

================
Comment at: lib/Analysis/UninitializedValues.cpp:555
@@ +554,3 @@
+    case CK_ArrayToPointerDecay:
+      classify(CE->getSubExpr(), Ignore);
+      break;
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > This looks curious. Treating this case as `Init` seems appropriate to me: we don't track how the resulting pointer is used, and it might be used to initialize the object. Is the idea to ignore all array subobjects of class type in the analysis? That is:
> > 
> >   struct X {
> >     int a[3];
> >     int b;
> >   };
> >   struct Y {
> >     X x;
> >     Y() {
> >       x.a[0] = 1; // don't treat 'x' as initialized
> >       x.a[1] = x.b; // warning: 'x' used uninitialized
> >     }
> >   };
> > 
> > If so, this really deserves a comment.
> This is actually another case in which ##x## is treated as initialized because the default constructor is called, hence ##x## is immediately treated as initialized.
> The analysis could potentially be made smarter for aggregates, but I'm not sure this patch is the best place to do it.
> 
> This was intended for:
> 
>   lang=c++
>   void foo(int *a);
>   struct X {
>     int a[3];
>     int b;
>     X() {
>       foo(a);
>       b = a[1]; // do not warn on 'a' here.
>     }
>   };
> 
> But actually it's not necessary because the current code is not classifying ##ArraySubscriptExprs##.
> Should I try to deal with ##ArraySubscriptExpr## it in this same patch?
My preference would be to keep this patch as small as possible while adding the new constructor-handling functionality. I'd prefer for `ArraySubscriptExpr` support to be added separately if that's practical.

================
Comment at: test/SemaCXX/uninitialized.cpp:444
@@ +443,3 @@
+    J() : a(1) {
+      float f = b;
+      b = -f;
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > We should really warn on this. A FIXME for that would be nice.
> I don't think we should. ##J## is a union, and one of its fields (##a##) has been initialized.
> If you're really sure that a warning should still be issued here, it's actually easier to do so rather than not.
Well, it's undefined behavior to write to field `a` then read from field `b`. The "uninitialized" warning isn't really the right one to issue, though, so I think your approach in this patch is fine.


http://llvm-reviews.chandlerc.com/D2181