[PATCH] Make UninitializedValues detect the use of uninitialized fields in a constructor.
Richard Smith
richard at metafoo.co.uk
Fri Dec 6 12:49:51 PST 2013
This looks really great. Let's start getting bits of this checked in!
I've marked a few pieces of this patch that I'd like to be checked in as separate pieces (to avoid the big monolithic change here). In addition to those, can you factor out the `CXXDefaultInitExpr` handling code into a separate patch, too?
Additionally, now we've basically converged on the functional side, there's a couple of coding standard issues that should be sorted out prior to commit. Variable names should be capitalized (don't worry about pre-existing variables in this file, it's currently a big mess with regard to coding style issues), and you have a "* " in `ClassifyRefs::VisitMemberExpr`.
After getting the side-pieces committed, we can circle back to the bulk of this patch. There's only really one substantive comment that I have there (and I think it's something you said you're already working on): it's hard to tell if `CXXThisExpr` is being handled appropriately in all cases, and I'd like for it to be tracked with a `ClassifyRefs`-based approach too (so if a `CXXThisExpr` escapes we can conservatively-correctly mark all fields as initialized). I don't know what you think about this -- do you think the patch is already handling this sufficiently reliably that it'd be OK to commit without this, and fix it up later? I know you've tested this against a lot of code already, but cases like passing `this` through an `ObjCMessageExpr` or a `CXXConstructExpr` (which may not have come up in your testing) don't look like they're handled here.
================
Comment at: lib/Analysis/UninitializedValues.cpp:391
@@ +390,3 @@
+ ? cast<DeclRefExpr>(E)->getDecl()
+ : isa<MemberExpr>(E) ? cast<MemberExpr>(E)->getMemberDecl() : 0;
+ if (!D || !isTracked(D))
----------------
Do you need the `isa` check here? This should only be called on `DeclRefExpr` and `MemberExpr`, as far as I can tell, so we should assert if something else gets passed in (or, ideally, not compile).
================
Comment at: lib/Analysis/UninitializedValues.cpp:413-437
@@ -361,12 +412,27 @@
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;
}
- FindVarResult Var = findVar(E, DC);
- if (const DeclRefExpr *DRE = Var.getDeclRefExpr())
- Classification[DRE] = std::max(Classification[DRE], C);
+ 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->getLHS(), Ignore);
+ classify(BO->getRHS(), C);
+ return;
+ default:
+ return;
+ }
+ }
+
----------------
The improvements here are great, but they're orthogonal to handling class members. Please factor them out into a separate patch.
================
Comment at: lib/Analysis/UninitializedValues.cpp:393-394
@@ -392,4 +461,2 @@
classify(BO->getLHS(), Use);
- else if (BO->getOpcode() == BO_Assign)
- classify(BO->getLHS(), Ignore);
}
----------------
I'm concerned about this removal. The old model was:
* Visiting the `DeclRefExpr` on the LHS of an assignment does not initialize the variable
* Visiting the assignment expression itself initializes the variable on its LHS
The new model seems to be:
* Visiting the `DeclRefExpr` or `MemberExpr` on the LHS of an assignment initializes the variable or field
* Visiting the assignment expression itself *also* initializes the variable
I think this may be subtly incorrect. If we have:
int x;
x = x + 1;
... and the CFG happens to emit the LHS and then the RHS, we won't find the uninitialized use, because we'll think that `x` is already initialized.
================
Comment at: lib/Analysis/UninitializedValues.cpp:476-480
@@ -404,5 +475,7 @@
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.
+ // If the address of a value is passed to a function, we treat the value as
+ // initialized.
for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
----------------
This comment could describe what's going on here better. Usually if an address is taken, we treat the value as initialized; the thing we should call out here is that the value is *not* treated as initialized if passed to a const pointer parameter. Maybe just change the first comment to "If a value is passed by const pointer or const reference to a function, ..."?
================
Comment at: lib/Analysis/UninitializedValues.cpp:495-500
@@ -412,1 +494,8 @@
+ }
+ if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
+ if (const MemberExpr *ME =
+ dyn_cast<MemberExpr>(MCE->getImplicitObjectArgument())) {
+ classify(ME, Use);
+ }
+ }
}
----------------
What happens here:
int f();
struct S {
int a, b;
int init() { a = b = 0; return f(); }
};
struct T {
int k;
S s;
T() { s.init(); } // ok? warning?
T(int) : k(s.init()) {} // ok? warning?
};
I think both of these are fine: because `T::s` has trivial initialization, its lifetime begins when storage for it is obtained (per **[basic.life]p1**).
================
Comment at: lib/Analysis/UninitializedValues.cpp:519-523
@@ +518,7 @@
+ case CK_FunctionToPointerDecay:
+ if (const MemberExpr *ME = dyn_cast<MemberExpr>(CE->getSubExpr())) {
+ if (isa<CXXMethodDecl>(ME->getMemberDecl()) &&
+ cast<CXXMethodDecl>(ME->getMemberDecl())->isStatic())
+ classify(ME->getBase(), Ignore);
+ }
+ break;
----------------
Is this necessary? The `VisitMemberExpr` check seems like it should do the right thing here.
================
Comment at: lib/Analysis/UninitializedValues.cpp:541-544
@@ +540,6 @@
+ // If it's not a reference, it will be classified appropriately by a cast
+ if (LME == ME && LME->getMemberDecl()->getType()->isReferenceType())
+ classify(LME, Use);
+ else if (LME != ME)
+ classify(LME, isFieldOrNonStaticMethod(ME->getMemberDecl()) ? Use : Ignore);
+}
----------------
This doesn't look quite right (though I think the issue is in `findLeftmostMemberExpr`). Consider:
this->member.static_member.non_static_member
When we visit this, I think we'll mark 'member' as `Use`. Suggestion: teach `findLeftmostMemberExpr` not to walk through `MemberExpr`s for static data members.
================
Comment at: lib/Analysis/UninitializedValues.cpp:547-550
@@ -425,2 +546,6 @@
+
+void ClassifyRefs::VisitCXXConstructExpr(CXXConstructExpr *CE) {
+ if (CE->getConstructor()->isCopyOrMoveConstructor())
+ classify(CE->getArg(0), Use);
}
----------------
I think you should also handle the arguments the same way that `VisitCallExpr` does.
struct A { A(const int&); };
struct B {
int x, y;
B() { A a(x); y = x; } // should warn on this
};
I think we won't warn here, because our usual handling for conservatively marking the `x` in `a(x)` as Ignore won't fire.
(Even if this is a copy or move constructor, it could still have additional arguments, so you should do this in that case too.)
That said, this is also orthogonal to handling of fields, and fixing this in a separate patch would be preferred.
================
Comment at: lib/Analysis/UninitializedValues.cpp:487-493
@@ +486,9 @@
+ 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);
+ }
+ }
----------------
This is orthogonal to field handling; please break this out into a separate patch.
================
Comment at: lib/Analysis/UninitializedValues.cpp:993
@@ +992,3 @@
+ else if (Optional<CFGInitializer> ci = I->getAs<CFGInitializer>()) {
+ tf.setReportConstructor(isInClassInitializer(ctor, ++initIdx));
+ FieldDecl *fd = ci->getInitializer()->getMember();
----------------
This counting approach seems rather complex to me, and I think we can get the same effect with something much simpler: remove the 'ReportConstructor' code entirely, and when you come to issue a diagnostic for an uninitialized use of a field, check whether the source location of the use is within the source range of the constructor. If not, emit the additional note pointing out which constructor it was.
================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1672
@@ -1651,1 +1671,3 @@
+ .setAlwaysAdd(Stmt::MemberExprClass)
+ .setAlwaysAdd(Stmt::CXXDefaultInitExprClass)
.setAlwaysAdd(Stmt::ImplicitCastExprClass)
----------------
I don't think you need this any more.
http://llvm-reviews.chandlerc.com/D2181
More information about the cfe-commits
mailing list