[PATCH] Make UninitializedValues detect the use of uninitialized fields in a constructor.
Richard Smith
richard at metafoo.co.uk
Thu Nov 14 20:51:00 PST 2013
This looks really exciting, and a great direction for the -Wuninitialized warning. (I'm a little sad that we're losing the enabled-by-default warning here, but I can live with that...)
I think there are a few small pieces here that you could factor out and could be committed separately (handling of `.*` and `->*`, for instance).
Longer-term, I think it'd be useful to extend this to model subobjects of fields and base classes (for instance, we should catch using a field of base class B2 from an initializer for base class B1), but this is a big step in the right direction, and I certainly don't want any scope creep on this change.
Have you run this over a significant quantity of code? Does it find many bugs, and were there any false positives?
================
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;
----------------
Micro-optimization: move the `if (!cd)` test to before the loop.
================
Comment at: lib/Analysis/UninitializedValues.cpp:97
@@ +96,3 @@
+ : dyn_cast<FieldDecl>(*I);
+ if (fd && isTrackedField(fd, &dc))
+ map[fd] = count++;
----------------
The `isTrackedField` check here seems redundant. All fields you will see here are tracked.
================
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]);
+ }
+ }
----------------
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?
================
Comment at: lib/Analysis/UninitializedValues.cpp:306
@@ +305,3 @@
+private:
+ Wrapped& wrapped;
+ typedef StmtVisitor<CXXDefaultInitExprVisitor<Wrapped> > Base;
----------------
" &", not "& ", please.
================
Comment at: lib/Analysis/UninitializedValues.cpp:348
@@ +347,3 @@
+ return 0;
+ if (FieldDecl *fd = cast<FieldDecl>(me->getMemberDecl()))
+ if (!fd->isAnonymousStructOrUnion())
----------------
`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;
// ...
================
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;
----------------
I don't think it matters, but `!D || !isTracked(D)` makes more sense to me.
================
Comment at: lib/Analysis/UninitializedValues.cpp:452
@@ +451,3 @@
+ dyn_cast<BinaryConditionalOperator>(E)) {
+ classify(BCO->getCommon(), C);
+ classify(BCO->getFalseExpr(), C);
----------------
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?
================
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);
+}
----------------
Why is this necessary? This would seem to fall under the general rule that "if we can't classify it, it's initialization".
================
Comment at: lib/Analysis/UninitializedValues.cpp:41
@@ -39,3 +40,3 @@
QualType ty = vd->getType();
return ty->isScalarType() || ty->isVectorType();
}
----------------
You seem to have no problem dealing with fields of non-scalar, non-vector types. I expect you could do the same for variables.
================
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);
+ }
}
----------------
Please add braces around this.
================
Comment at: lib/Analysis/UninitializedValues.cpp:529
@@ +528,3 @@
+ Ex = stripCasts(DC->getParentASTContext(), UO->getSubExpr());
+ classify(Ex, (*I)->getType().isConstQualified() ? Ignore : Init);
+ }
----------------
Did you mean to use `IsPointerToConst` here?
================
Comment at: lib/Analysis/UninitializedValues.cpp:547
@@ -423,2 +546,3 @@
}
+ return;
}
----------------
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?
================
Comment at: lib/Analysis/UninitializedValues.cpp:555
@@ +554,3 @@
+ case CK_ArrayToPointerDecay:
+ classify(CE->getSubExpr(), Ignore);
+ break;
----------------
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.
================
Comment at: lib/Analysis/UninitializedValues.cpp:571
@@ +570,3 @@
+ if (ME == findLeftmostMemberExpr(ME) &&
+ ME->getMemberDecl()->getType()->isReferenceType()) {
+ classify(ME, Use);
----------------
Seems this case should apply if any of the `FieldDecls` in the chain are of reference type, not just if the last one is.
================
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());
+}
+
----------------
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.
================
Comment at: lib/Analysis/UninitializedValues.cpp:55
@@ +54,3 @@
+ return false;
+ return true;
+}
----------------
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
};
================
Comment at: lib/Analysis/UninitializedValues.cpp:603
@@ +602,3 @@
+ bool reportConstructor;
+ const CXXConstructorDecl* constructor;
+ bool isUnionConstructor;
----------------
" *", not "* ".
================
Comment at: lib/Analysis/UninitializedValues.cpp:604
@@ -440,1 +603,3 @@
+ const CXXConstructorDecl* constructor;
+ bool isUnionConstructor;
----------------
Do you really need a member for this? Seems trivial to reconstruct.
================
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);
----------------
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.
================
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;
+ }
}
----------------
What about indirect fields from anonymous structs or unions?
================
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);
+ }
+ }
+}
+
----------------
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);`
================
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;
+ }
+ }
}
----------------
Please either fold this into VisitCallExpr or call VisitCallExpr if you don't handle the call here.
================
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
----------------
`if (constructor)` ?
================
Comment at: lib/Analysis/UninitializedValues.cpp:939
@@ +938,3 @@
+ else
+ vals[cast<FieldDecl>(e->getMemberDecl())] = Initialized;
+ break;
----------------
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`.
================
Comment at: lib/Analysis/UninitializedValues.cpp:953-954
@@ +952,4 @@
+ // We treat this as if it escaped analysis.
+ if (findThisExpr(BO->getLHS()))
+ setAllFields(Initialized);
+ }
----------------
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`.
================
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;
+}
+
----------------
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.
================
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,
----------------
Do you need to pass these in? `TransferFunctions` can work this out for itself.
================
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)) {
----------------
" *", not "* ".
================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1241
@@ -1226,2 +1240,3 @@
const MappedType &V = i->second;
+ const Decl* d = i->first;
----------------
" *"
================
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);
}
----------------
A comment explaining this would be useful.
================
Comment at: test/SemaCXX/uninitialized.cpp:444
@@ +443,3 @@
+ J() : a(1) {
+ float f = b;
+ b = -f;
----------------
We should really warn on this. A FIXME for that would be nice.
http://llvm-reviews.chandlerc.com/D2181
More information about the cfe-commits
mailing list