[PATCH] Make UninitializedValues detect the use of uninitialized fields in a constructor.
Enrico Pertoso
epertoso at google.com
Mon Nov 18 11:27:26 PST 2013
Thanks for the review.
Yes, I've run it over a big code base. I didn't spot false positives, but I still think there are too many false negatives.
================
Comment at: lib/Analysis/UninitializedValues.cpp:53-54
@@ +52,4 @@
+ declaringClass = dyn_cast<RecordDecl>(declaringClass->getDeclContext());
+ if (!cd || (cd->getParent() != declaringClass))
+ return false;
+ return true;
----------------
Richard Smith wrote:
> Micro-optimization: move the `if (!cd)` test to before the loop.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:306
@@ +305,3 @@
+private:
+ Wrapped& wrapped;
+ typedef StmtVisitor<CXXDefaultInitExprVisitor<Wrapped> > Base;
----------------
Richard Smith wrote:
> " &", not "& ", please.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:348
@@ +347,3 @@
+ return 0;
+ if (FieldDecl *fd = cast<FieldDecl>(me->getMemberDecl()))
+ if (!fd->isAnonymousStructOrUnion())
----------------
Richard Smith wrote:
> `if` is redundant: fd can't be null here. Please combine this with the above check to use a `dyn_cast`:
>
> FieldDecl *fd = dyn_cast<FieldDecl>(me->getMemberDecl());
> if (!fd)
> return 0;
> // ...
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:421
@@ -342,1 +420,3 @@
+ : isa<MemberExpr>(E) ? cast<MemberExpr>(E)->getMemberDecl() : 0;
+ if (D && !isTracked(D))
return Ignore;
----------------
Richard Smith wrote:
> I don't think it matters, but `!D || !isTracked(D)` makes more sense to me.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:532-536
@@ -412,1 +531,7 @@
+ }
+ if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE))
+ if (const MemberExpr *ME =
+ dyn_cast<MemberExpr>(MCE->getImplicitObjectArgument())) {
+ classify(ME, Use);
+ }
}
----------------
Richard Smith wrote:
> Please add braces around this.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:571
@@ +570,3 @@
+ if (ME == findLeftmostMemberExpr(ME) &&
+ ME->getMemberDecl()->getType()->isReferenceType()) {
+ classify(ME, Use);
----------------
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?
================
Comment at: lib/Analysis/UninitializedValues.cpp:603
@@ +602,3 @@
+ bool reportConstructor;
+ const CXXConstructorDecl* constructor;
+ bool isUnionConstructor;
----------------
Richard Smith wrote:
> " *", not "* ".
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:879-880
@@ -666,1 +878,4 @@
}
+ if (const CXXConstructorDecl *CD =
+ dyn_cast<CXXConstructorDecl>(ac.getDecl())) {
+ // If "this" is passed to a function without the const qualifier, assume
----------------
Richard Smith wrote:
> `if (constructor)` ?
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:939
@@ +938,3 @@
+ else
+ vals[cast<FieldDecl>(e->getMemberDecl())] = Initialized;
+ break;
----------------
Richard Smith wrote:
> Is the `cast` here correct? `findLeftmostMemberExpr` might return a `MemberExpr` that references a static member (for an expression like `this->static_member`), and that's a `VarDecl`, not a `FieldDecl`.
Yes. ##classification.get(e)## calls ##ClassifyRefs::isTracked##, which will be false for such a ##VarDecl##.
================
Comment at: lib/Analysis/UninitializedValues.cpp:1050
@@ +1049,3 @@
+ else if (Optional<CFGInitializer> ci = I->getAs<CFGInitializer>()) {
+ FieldDecl* fd = ci->getInitializer()->getMember();
+ if (fd && isTrackedField(fd, constructor)) {
----------------
Richard Smith wrote:
> " *", not "* ".
Done.
================
Comment at: test/SemaCXX/uninitialized.cpp:444
@@ +443,3 @@
+ J() : a(1) {
+ float f = b;
+ b = -f;
----------------
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.
================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1241
@@ -1226,2 +1240,3 @@
const MappedType &V = i->second;
+ const Decl* d = i->first;
----------------
Richard Smith wrote:
> " *"
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:97
@@ +96,3 @@
+ : dyn_cast<FieldDecl>(*I);
+ if (fd && isTrackedField(fd, &dc))
+ map[fd] = count++;
----------------
Richard Smith wrote:
> The `isTrackedField` check here seems redundant. All fields you will see here are tracked.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:452
@@ +451,3 @@
+ dyn_cast<BinaryConditionalOperator>(E)) {
+ classify(BCO->getCommon(), C);
+ classify(BCO->getFalseExpr(), C);
----------------
Richard Smith wrote:
> I think this should be classified as `Use`. If we have:
>
> (x ?: y) = 0;
>
> `x` is used before it's initialized. Maybe this doesn't matter, because we'll separately have an lvalue-to-rvalue conversion for `x`?
>
> Can you change the above `ConditionalOperator` code to instead handle `AbstractConditionalOperator` and fold the two checks together?
Done.
But we have to explicitly classify the common expr as ##Use##, because an ##OpaqueValueExpr## will appear between the lvalue-to-rvalue cast and ##x##.
================
Comment at: lib/Analysis/UninitializedValues.cpp:502-503
@@ -401,1 +501,4 @@
classify(UO->getSubExpr(), Use);
+ else if (UO->getOpcode() == UO_AddrOf)
+ classify(UO->getSubExpr(), Init);
+}
----------------
Richard Smith wrote:
> Why is this necessary? This would seem to fall under the general rule that "if we can't classify it, it's initialization".
Done. It wasn't, indeed.
================
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:
> 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.
================
Comment at: lib/Analysis/UninitializedValues.cpp:1040-1042
@@ -759,2 +1039,5 @@
// Apply the transfer function.
- TransferFunctions tf(vals, cfg, block, ac, classification, handler);
+ const CXXConstructorDecl *constructor =
+ dyn_cast<CXXConstructorDecl>(ac.getDecl());
+ bool isUnionConstructor = constructor && constructor->getParent()->isUnion();
+ TransferFunctions tf(vals, cfg, block, ac, classification, handler,
----------------
Richard Smith wrote:
> Do you need to pass these in? `TransferFunctions` can work this out for itself.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:604
@@ -440,1 +603,3 @@
+ const CXXConstructorDecl* constructor;
+ bool isUnionConstructor;
----------------
Richard Smith wrote:
> Do you really need a member for this? Seems trivial to reconstruct.
Done.
================
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:
> 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).
================
Comment at: lib/Analysis/UninitializedValues.cpp:1004-1011
@@ -737,1 +1003,10 @@
+void TransferFunctions::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *die) {
+ if (!die->getExpr())
+ return;
+ CXXDefaultInitExprVisitor<TransferFunctions> V(*this);
+ reportConstructor = true;
+ V.Visit((Stmt *)die->getExpr());
+ reportConstructor = false;
+}
+
----------------
Richard Smith wrote:
> Again, I don't think this does the right thing if `this` appears in a default initializer for an object other than `*this`. Elsewhere we handle this by visiting the `CXXDefaultInitExpr` while holding an override for what `this` binds to. You could do something similar here.
Please see my comment in ##ClassifyRefs::VisitCXXDefaultInitExpr##.
================
Comment at: lib/Analysis/UninitializedValues.cpp:55
@@ +54,3 @@
+ return false;
+ return true;
+}
----------------
Richard Smith wrote:
> What if the field is of a class type with no data members?
>
> struct X {};
> struct Y {
> X x, y;
> Y() : y(x) {} // should probably not warn, but presumably will
> };
It doesn't, ##x## is default initialized.
================
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:
> 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?
================
Comment at: lib/Analysis/UninitializedValues.cpp:805-809
@@ -614,1 +804,7 @@
+ const CXXRecordDecl *RD = constructor->getParent();
+ for (CXXRecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
+ I != E; ++I) {
+ if (isTrackedField(*I, constructor))
+ vals[*I] = v;
+ }
}
----------------
Richard Smith wrote:
> What about indirect fields from anonymous structs or unions?
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:41
@@ -39,3 +40,3 @@
QualType ty = vd->getType();
return ty->isScalarType() || ty->isVectorType();
}
----------------
Richard Smith wrote:
> You seem to have no problem dealing with fields of non-scalar, non-vector types. I expect you could do the same for variables.
Maybe. I'd prefer to run a few test before.
================
Comment at: lib/Analysis/UninitializedValues.cpp:547
@@ -423,2 +546,3 @@
}
+ return;
}
----------------
Richard Smith wrote:
> This looks like it would do the wrong thing if we had a c-style cast that only performed an lvalue-to-rvalue conversion or decay. Push the `return` into the `void` check?
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:529
@@ +528,3 @@
+ Ex = stripCasts(DC->getParentASTContext(), UO->getSubExpr());
+ classify(Ex, (*I)->getType().isConstQualified() ? Ignore : Init);
+ }
----------------
Richard Smith wrote:
> Did you mean to use `IsPointerToConst` here?
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:555
@@ +554,3 @@
+ case CK_ArrayToPointerDecay:
+ classify(CE->getSubExpr(), Ignore);
+ break;
----------------
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?
================
Comment at: lib/Analysis/UninitializedValues.cpp:842-856
@@ -645,1 +841,17 @@
+void TransferFunctions::VisitCXXOperatorCallExpr(CXXOperatorCallExpr *oce) {
+ if (!constructor)
+ return;
+
+ if (oce->getOperator() == OO_Equal) {
+ const CXXMethodDecl *Callee = cast<CXXMethodDecl>(oce->getCalleeDecl());
+ const Expr *lhs =
+ stripCasts(constructor->getParentASTContext(), oce->getArgs()[0]);
+ if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(lhs)) {
+ if (isa<CXXThisExpr>(UO->getSubExpr()) &&
+ constructor->getParent() == Callee->getParent())
+ setAllFields(Initialized);
+ }
+ }
+}
+
----------------
Richard Smith wrote:
> Handle this in `VisitCallExpr` instead. This approach will do the wrong thing for a call to an operator marked `returns_twice` or `analyzer_noreturn`, or for an explicit call to the assignment operator, like `this->operator=(foo);`
Done.
An explicit call to the assignment operator would have been handled by ##VisitCXXMemberCallExpr##, but I've also moved that code in ##VisitCallExpr##.
I added the tests for both: ##*this = foo## and ##this->operator=(foo)## in ##uninitialized.cpp##.
================
Comment at: lib/Analysis/UninitializedValues.cpp:894-907
@@ -667,1 +893,16 @@
+
+void TransferFunctions::VisitCXXMemberCallExpr(CXXMemberCallExpr *mce) {
+ if (!constructor)
+ return;
+ // After a call to a member function within the constructor, it's possible
+ // that some or all of the fields have been initialized. Just mark them
+ // conservatively as such.
+ if (isa<CXXThisExpr>(mce->getImplicitObjectArgument())) {
+ // Make sure it's not an inherited method.
+ if (constructor->getParent() ==
+ cast<CXXMethodDecl>(mce->getCalleeDecl())->getParent()) {
+ setAllFields(Initialized);
+ return;
+ }
+ }
}
----------------
Richard Smith wrote:
> Please either fold this into VisitCallExpr or call VisitCallExpr if you don't handle the call here.
Done.
================
Comment at: lib/Analysis/UninitializedValues.cpp:953-954
@@ +952,4 @@
+ // We treat this as if it escaped analysis.
+ if (findThisExpr(BO->getLHS()))
+ setAllFields(Initialized);
+ }
----------------
Richard Smith wrote:
> I'm unconvinced that this and the `CXXThisExpr`-as-function-argument checks are sufficient; `CXXThisExpr` could be used to indirectly initialize the object through various other forms of expression. I think you should make the classification code also classify all the `CXXThisExprs`, and treat all fields as initialized if we see a `CXXThisExpr` that is classified as `Init`.
I'll try to do it. I need a little more time.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:7959-7960
@@ -8173,3 +7958,4 @@
- DiagnoseUninitializedFields(*this, Constructor);
+ sema::AnalysisBasedWarnings::Policy P = AnalysisWarnings.getDefaultPolicy();
+ AnalysisWarnings.IssueWarnings(P, FunctionScopes.back(), Constructor, 0);
}
----------------
Richard Smith wrote:
> A comment explaining this would be useful.
Done.
http://llvm-reviews.chandlerc.com/D2181
More information about the cfe-commits
mailing list