From: Richard Trieu Date: Fri, 20 Sep 2013 03:03:06 +0000 (+0000) Subject: Modify the uninitialized field visitor to detect uninitialized use across the X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ef8f90caa14d85cffba2fea4b6bd425c3b22eea7;p=clang Modify the uninitialized field visitor to detect uninitialized use across the fields in the class. This allows a better checking of member intiailizers and in class initializers in regards to initialization ordering. For instance, this code will now produce warnings: class A { int x; int y; A() : x(y) {} // y is initialized after x, warn here A(int): y(x) {} // default initialization of leaves x uninitialized, warn here }; Several test cases were updated with -Wno-uninitialized to silence this warning. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@191068 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 9a6e9d0ff0..8486ff33c5 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1370,6 +1370,8 @@ def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">, def warn_reference_field_is_uninit : Warning< "reference %0 is not yet bound to a value when used here">, InGroup; +def note_uninit_in_this_constructor : Note< + "during field initialization in this constructor">; def warn_static_self_reference_in_init : Warning< "static variable %0 is suspiciously used within its own initialization">, InGroup; diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index d4f9cc4380..34374aa99e 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -2081,20 +2081,37 @@ namespace { class UninitializedFieldVisitor : public EvaluatedExprVisitor { Sema &S; + // If VD is null, this visitor will only update the Decls set. ValueDecl *VD; bool isReferenceType; + // List of Decls to generate a warning on. + llvm::SmallPtrSet &Decls; + bool WarnOnSelfReference; + // If non-null, add a note to the warning pointing back to the constructor. + const CXXConstructorDecl *Constructor; public: typedef EvaluatedExprVisitor Inherited; - UninitializedFieldVisitor(Sema &S, ValueDecl *VD) : Inherited(S.Context), - S(S) { - if (IndirectFieldDecl *IFD = dyn_cast(VD)) - this->VD = IFD->getAnonField(); - else - this->VD = VD; - isReferenceType = this->VD->getType()->isReferenceType(); + UninitializedFieldVisitor(Sema &S, ValueDecl *VD, + llvm::SmallPtrSet &Decls, + bool WarnOnSelfReference, + const CXXConstructorDecl *Constructor) + : Inherited(S.Context), S(S), VD(VD), isReferenceType(false), Decls(Decls), + WarnOnSelfReference(WarnOnSelfReference), Constructor(Constructor) { + // When VD is null, this visitor is used to detect initialization of other + // fields. + if (VD) { + if (IndirectFieldDecl *IFD = dyn_cast(VD)) + this->VD = IFD->getAnonField(); + else + this->VD = VD; + isReferenceType = this->VD->getType()->isReferenceType(); + } } void HandleMemberExpr(MemberExpr *ME, bool CheckReferenceOnly) { + if (!VD) + return; + if (CheckReferenceOnly && !isReferenceType) return; @@ -2122,15 +2139,38 @@ namespace { if (!isa(Base)) return; - if (VD == FieldME->getMemberDecl()) { + ValueDecl* FoundVD = FieldME->getMemberDecl(); + + if (VD == FoundVD) { + if (!WarnOnSelfReference) + return; + unsigned diag = isReferenceType ? diag::warn_reference_field_is_uninit : diag::warn_field_is_uninit; S.Diag(FieldME->getExprLoc(), diag) << VD; + if (Constructor) + S.Diag(Constructor->getLocation(), + diag::note_uninit_in_this_constructor); + return; + } + + if (CheckReferenceOnly) + return; + + if (Decls.count(FoundVD)) { + S.Diag(FieldME->getExprLoc(), diag::warn_field_is_uninit) << FoundVD; + if (Constructor) + S.Diag(Constructor->getLocation(), + diag::note_uninit_in_this_constructor); + } } void HandleValue(Expr *E) { + if (!VD) + return; + E = E->IgnoreParens(); if (MemberExpr *ME = dyn_cast(E)) { @@ -2180,7 +2220,7 @@ namespace { } void VisitCXXConstructExpr(CXXConstructExpr *E) { - if (E->getNumArgs() == 1) + if (E->getConstructor()->isCopyConstructor()) if (ImplicitCastExpr* ICE = dyn_cast(E->getArg(0))) if (ICE->getCastKind() == CK_NoOp) if (MemberExpr *ME = dyn_cast(ICE->getSubExpr())) @@ -2196,11 +2236,27 @@ namespace { Inherited::VisitCXXMemberCallExpr(E); } + + void VisitBinaryOperator(BinaryOperator *E) { + // If a field assignment is detected, remove the field from the + // uninitiailized field set. + if (E->getOpcode() == BO_Assign) + if (MemberExpr *ME = dyn_cast(E->getLHS())) + if (FieldDecl *FD = dyn_cast(ME->getMemberDecl())) + Decls.erase(FD); + + Inherited::VisitBinaryOperator(E); + } }; - static void CheckInitExprContainsUninitializedFields(Sema &S, Expr *E, - ValueDecl *VD) { + static void CheckInitExprContainsUninitializedFields( + Sema &S, Expr *E, ValueDecl *VD, llvm::SmallPtrSet &Decls, + bool WarnOnSelfReference, const CXXConstructorDecl *Constructor = 0) { + if (Decls.size() == 0 && !WarnOnSelfReference) + return; + if (E) - UninitializedFieldVisitor(S, VD).Visit(E); + UninitializedFieldVisitor(S, VD, Decls, WarnOnSelfReference, Constructor) + .Visit(E); } } // namespace @@ -2252,11 +2308,6 @@ Sema::ActOnCXXInClassMemberInitializer(Decl *D, SourceLocation InitLoc, InitExpr = Init.release(); - if (getDiagnostics().getDiagnosticLevel(diag::warn_field_is_uninit, InitLoc) - != DiagnosticsEngine::Ignored) { - CheckInitExprContainsUninitializedFields(*this, InitExpr, FD); - } - FD->setInClassInitializer(InitExpr); } @@ -3702,15 +3753,9 @@ bool CheckRedundantUnionInit(Sema &S, // Diagnose value-uses of fields to initialize themselves, e.g. // foo(foo) // where foo is not also a parameter to the constructor. +// Also diagnose across field uninitialized use such as +// x(y), y(x) // TODO: implement -Wuninitialized and fold this into that framework. -// FIXME: Warn about the case when other fields are used before being -// initialized. For example, let this field be the i'th field. When -// initializing the i'th field, throw a warning if any of the >= i'th -// fields are used, as they are not yet initialized. -// Right now we are only handling the case where the i'th field uses -// itself in its initializer. -// Also need to take into account that some fields may be initialized by -// in-class initializers, see C++11 [class.base.init]p9. static void DiagnoseUnitializedFields( Sema &SemaRef, const CXXConstructorDecl *Constructor) { @@ -3720,19 +3765,72 @@ static void DiagnoseUnitializedFields( return; } - for (CXXConstructorDecl::init_const_iterator - FieldInit = Constructor->init_begin(), + const CXXRecordDecl *RD = Constructor->getParent(); + + // Holds fields that are uninitialized. + llvm::SmallPtrSet UninitializedFields; + + for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end(); + I != E; ++I) { + if (FieldDecl *FD = dyn_cast(*I)) { + UninitializedFields.insert(FD); + } else if (IndirectFieldDecl *IFD = dyn_cast(*I)) { + UninitializedFields.insert(IFD->getAnonField()); + } + } + + // Fields already checked when processing the in class initializers. + llvm::SmallPtrSet + InClassUninitializedFields = UninitializedFields; + + for (CXXConstructorDecl::init_const_iterator FieldInit = + Constructor->init_begin(), FieldInitEnd = Constructor->init_end(); FieldInit != FieldInitEnd; ++FieldInit) { - FieldDecl *FD = (*FieldInit)->getAnyMember(); - if (!FD) - continue; + FieldDecl *Field = (*FieldInit)->getAnyMember(); Expr *InitExpr = (*FieldInit)->getInit(); - if (!isa(InitExpr)) { - CheckInitExprContainsUninitializedFields(SemaRef, InitExpr, FD); + if (!Field) { + CheckInitExprContainsUninitializedFields( + SemaRef, InitExpr, 0, UninitializedFields, + false/*WarnOnSelfReference*/); + continue; } + + if (CXXDefaultInitExpr *Default = dyn_cast(InitExpr)) { + // This field is initialized with an in-class initailzer. Remove the + // fields already checked to prevent duplicate warnings. + llvm::SmallPtrSet DiffSet = UninitializedFields; + for (llvm::SmallPtrSet::iterator + I = InClassUninitializedFields.begin(), + E = InClassUninitializedFields.end(); + I != E; ++I) { + DiffSet.erase(*I); + } + CheckInitExprContainsUninitializedFields( + SemaRef, Default->getExpr(), Field, DiffSet, + DiffSet.count(Field), Constructor); + + // Update the unitialized field sets. + CheckInitExprContainsUninitializedFields( + SemaRef, Default->getExpr(), 0, UninitializedFields, + false); + CheckInitExprContainsUninitializedFields( + SemaRef, Default->getExpr(), 0, InClassUninitializedFields, + false); + } else { + CheckInitExprContainsUninitializedFields( + SemaRef, InitExpr, Field, UninitializedFields, + UninitializedFields.count(Field)); + if (Expr* InClassInit = Field->getInClassInitializer()) { + CheckInitExprContainsUninitializedFields( + SemaRef, InClassInit, 0, InClassUninitializedFields, + false); + } + } + UninitializedFields.erase(Field); + InClassUninitializedFields.erase(Field); } } @@ -8057,6 +8155,56 @@ void Sema::ActOnFinishDelayedMemberInitializers(Decl *D) { // Check that any explicitly-defaulted methods have exception specifications // compatible with their implicit exception specifications. CheckDelayedExplicitlyDefaultedMemberExceptionSpecs(); + + // Once all the member initializers are processed, perform checks to see if + // any unintialized use is happeneing. + if (getDiagnostics().getDiagnosticLevel(diag::warn_field_is_uninit, + D->getLocation()) + == DiagnosticsEngine::Ignored) + return; + + CXXRecordDecl *RD = dyn_cast(D); + if (!RD) return; + + // Holds fields that are uninitialized. + llvm::SmallPtrSet UninitializedFields; + + // In the beginning, every field is uninitialized. + for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end(); + I != E; ++I) { + if (FieldDecl *FD = dyn_cast(*I)) { + UninitializedFields.insert(FD); + } else if (IndirectFieldDecl *IFD = dyn_cast(*I)) { + UninitializedFields.insert(IFD->getAnonField()); + } + } + + for (DeclContext::decl_iterator I = RD->decls_begin(), E = RD->decls_end(); + I != E; ++I) { + FieldDecl *FD = dyn_cast(*I); + if (!FD) + if (IndirectFieldDecl *IFD = dyn_cast(*I)) + FD = IFD->getAnonField(); + + if (!FD) + continue; + + Expr *InitExpr = FD->getInClassInitializer(); + if (!InitExpr) { + // Uninitialized reference types will give an error. + // Record types with an initializer are default initialized. + QualType FieldType = FD->getType(); + if (FieldType->isReferenceType() || FieldType->isRecordType()) + UninitializedFields.erase(FD); + continue; + } + + CheckInitExprContainsUninitializedFields( + *this, InitExpr, FD, UninitializedFields, + UninitializedFields.count(FD)/*WarnOnSelfReference*/); + + UninitializedFields.erase(FD); + } } namespace { diff --git a/test/CXX/dcl.decl/dcl.init/dcl.init.aggr/p7.cpp b/test/CXX/dcl.decl/dcl.init/dcl.init.aggr/p7.cpp index d1fbe766d5..22667ac14b 100644 --- a/test/CXX/dcl.decl/dcl.init/dcl.init.aggr/p7.cpp +++ b/test/CXX/dcl.decl/dcl.init/dcl.init.aggr/p7.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -std=c++1y %s -verify +// RUN: %clang_cc1 -Wno-uninitialized -std=c++1y %s -verify // expected-no-diagnostics diff --git a/test/Parser/cxx-ambig-init-templ.cpp b/test/Parser/cxx-ambig-init-templ.cpp index 88ab61c3ab..ac79f77e15 100644 --- a/test/Parser/cxx-ambig-init-templ.cpp +++ b/test/Parser/cxx-ambig-init-templ.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -std=c++11 -verify %s +// RUN: %clang_cc1 -Wno-uninitialized -std=c++11 -verify %s template struct c { c(int) = delete; typedef void val; operator int() const; }; diff --git a/test/SemaCXX/constexpr-value-init.cpp b/test/SemaCXX/constexpr-value-init.cpp index d137bd88db..e5b7db50eb 100644 --- a/test/SemaCXX/constexpr-value-init.cpp +++ b/test/SemaCXX/constexpr-value-init.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -verify +// RUN: %clang_cc1 %s -Wno-uninitialized -std=c++11 -fsyntax-only -verify struct A { constexpr A() : a(b + 1), b(a + 1) {} // expected-note {{outside its lifetime}} diff --git a/test/SemaCXX/cxx0x-class.cpp b/test/SemaCXX/cxx0x-class.cpp index 074591e706..2b1338f97f 100644 --- a/test/SemaCXX/cxx0x-class.cpp +++ b/test/SemaCXX/cxx0x-class.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wno-error=static-float-init %s +// RUN: %clang_cc1 -Wno-uninitialized -fsyntax-only -verify -std=c++11 -Wno-error=static-float-init %s int vs = 0; diff --git a/test/SemaCXX/uninitialized.cpp b/test/SemaCXX/uninitialized.cpp index 31895c6e02..2fb0482cac 100644 --- a/test/SemaCXX/uninitialized.cpp +++ b/test/SemaCXX/uninitialized.cpp @@ -340,7 +340,10 @@ namespace { }; struct E { - int a, b, c; + int b = 1; + int c = 1; + int a; // This field needs to be last to prevent the cross field + // uninitialized warning. E(char (*)[1]) : a(a ? b : c) {} // expected-warning {{field 'a' is uninitialized when used here}} E(char (*)[2]) : a(b ? a : a) {} // expected-warning 2{{field 'a' is uninitialized when used here}} E(char (*)[3]) : a(b ? (a) : c) {} // expected-warning {{field 'a' is uninitialized when used here}} @@ -459,7 +462,7 @@ namespace in_class_initializers { }; struct U { - U() : a(b + 1), b(a + 1) {} // FIXME: Warn here. + U() : a(b + 1), b(a + 1) {} // expected-warning{{field 'b' is uninitialized when used here}} int a = 42; // Note: because a and b are in the member initializer list, these initializers are ignored. int b = 1; }; @@ -561,3 +564,107 @@ namespace record_fields { A a9 = normal(a9); // expected-warning {{uninitialized}} }; } + +namespace cross_field_warnings { + struct A { + int a, b; + A() {} + A(char (*)[1]) : b(a) {} // expected-warning{{field 'a' is uninitialized when used here}} + A(char (*)[2]) : a(b) {} // expected-warning{{field 'b' is uninitialized when used here}} + }; + + struct B { + int a = b; // expected-warning{{field 'b' is uninitialized when used here}} + int b; + }; + + struct C { + int a; + int b = a; // expected-warning{{field 'a' is uninitialized when used here}} + C(char (*)[1]) : a(5) {} + C(char (*)[2]) {} + }; + + struct D { + int a; + int &b; + int &c = a; + int d = b; + D() : b(a) {} + }; + + struct E { + int a; + int get(); + static int num(); + E() {} + E(int) {} + }; + + struct F { + int a; + E e; + int b; + F(char (*)[1]) : a(e.get()) {} // expected-warning{{field 'e' is uninitialized when used here}} + F(char (*)[2]) : a(e.num()) {} + F(char (*)[3]) : e(a) {} // expected-warning{{field 'a' is uninitialized when used here}} + F(char (*)[4]) : a(4), e(a) {} + F(char (*)[5]) : e(b) {} // expected-warning{{field 'b' is uninitialized when used here}} + F(char (*)[6]) : e(b), b(4) {} // expected-warning{{field 'b' is uninitialized when used here}} + }; + + struct G { + G(const A&) {}; + }; + + struct H { + A a1; + G g; + A a2; + H() : g(a1) {} + H(int) : g(a2) {} + }; + + struct I { + I(int*) {} + }; + + struct J : public I { + int *a; + int *b; + int c; + J() : I((a = new int(5))), b(a), c(*a) {} + }; + + struct K { + int a = (b = 5); + int b = b + 5; + }; + + struct L { + int a = (b = 5); + int b = b + 5; // expected-warning{{field 'b' is uninitialized when used here}} + L() : a(5) {} // expected-note{{during field initialization in this constructor}} + }; + + struct M { }; + + struct N : public M { + int a; + int b; + N() : b(a) { } // expected-warning{{field 'a' is uninitialized when used here}} + }; + + struct O { + int x = 42; + int get() { return x; } + }; + + struct P { + O o; + int x = o.get(); + P() : x(o.get()) { } + }; + +} + diff --git a/test/SemaCXX/warn-unused-private-field.cpp b/test/SemaCXX/warn-unused-private-field.cpp index 661442db9e..932a7dcea1 100644 --- a/test/SemaCXX/warn-unused-private-field.cpp +++ b/test/SemaCXX/warn-unused-private-field.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -Wunused-private-field -Wused-but-marked-unused -verify -std=c++11 %s +// RUN: %clang_cc1 -fsyntax-only -Wunused-private-field -Wused-but-marked-unused -Wno-uninitialized -verify -std=c++11 %s class NotFullyDefined { public: