================
Comment at: lib/Analysis/UninitializedValues.cpp:571
@@ +570,3 @@
+ if (ME == findLeftmostMemberExpr(ME) &&
+ ME->getMemberDecl()->getType()->isReferenceType()) {
+ classify(ME, Use);
----------------
Richard Smith wrote:
> 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.
Thanks, you helped me diagnose a bigger problem here (record access wasn't
being counted as use in general).
I did this because when you have
lang=c++
struct A {
int &a;
int &b;
int n;
A() : a(b), n(b) {}
};
the use of ##b## in ##n(b)## will be detected because of the lvalue-to-rvalue
cast, but there's no such cast on references.
Each ##MemberExpr## is visited, so the code warns on your example. I've added a
test case.
================
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());
+}
+
----------------
Richard Smith wrote:
> 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).
In that case ##B{}## is a ##CXXConstructExpr##. You can't use aggregate
initialization for ##B## because it has a base class.
Unless there's a change in the aggregate definition after n3653.
================
Comment at: lib/Analysis/UninitializedValues.cpp:555
@@ +554,3 @@
+ case CK_ArrayToPointerDecay:
+ classify(CE->getSubExpr(), Ignore);
+ break;
----------------
Richard Smith wrote:
> 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.
Acknowledged.
================
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);
----------------
Richard Smith wrote:
> 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.
Actually, it prevents a false warning, but it's still not common. I'll get rid
of it.
And I've realized that this function could get into an infinite loop. Fixed
that.
================
Comment at: test/SemaCXX/uninitialized.cpp:444
@@ +443,3 @@
+ J() : a(1) {
+ float f = b;
+ b = -f;
----------------
Richard Smith wrote:
> 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.
Acknowledged.
================
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]);
+ }
+ }
----------------
Richard Smith wrote:
> 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).
The only difference I see is that, as there's no control flow information at
all, what's usually reported as sometimes uninitialized will be reported as
always uninitialized.
But I'll have a look at the ##CFGBuilder##.
================
Comment at: include/clang/Analysis/Analyses/UninitializedValues.h:51
@@ -49,1 +50,3 @@
+ /// If the use refers to a field, the use happens in this constructor.
+ const CXXConstructorDecl *Constructor;
----------------
Richard Smith wrote:
> Please extend the comment here: something like "If the use happens in a
> default initializer for a field, this is the constructor in which the default
> initializer was used."
Done.
================
Comment at: include/clang/Analysis/Analyses/UninitializedValues.h:73
@@ -67,2 +72,3 @@
const Expr *getUser() const { return User; }
+ const CXXConstructorDecl *getConstructor() const { return Constructor; }
----------------
Richard Smith wrote:
> Maybe `getConstructorForDefaultInit`?
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:529
@@ +528,3 @@
+ Ex = stripCasts(DC->getParentASTContext(), UO->getSubExpr());
+ classify(Ex, (*I)->getType().isConstQualified() ? Ignore : Init);
+ }
----------------
Enrico Pertoso wrote:
> Richard Smith wrote:
> > Did you mean to use `IsPointerToConst` here?
> Done.
I've changed this a bit to make sure that #IsPointerToConst()## is only called
if ##(*I)->isGLValue()## is false. This is to avoid a regression for cases like:
lang=c++
int foo(const int *&a) { a = &some_global_int_; }
void bar() {
const int *j;
foo(j); // don't classify the DeclRefExpr for j as Ignore here, just let it
escape
int a = *j;
}
http://llvm-reviews.chandlerc.com/D2181
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
