Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp +++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp @@ -58,9 +58,36 @@ /// Note that this class is immutable, and new fields may only be added through /// constructor calls. class FieldChainInfo { + using FieldChainImpl = llvm::ImmutableListImpl; + + // If the fieldchain has void pointers or nonloc::LocAsInteger objects, + // they have to be casted back in order to dereference them. + // + // struct B { + // void *ptr; + // B(void *ptr) : ptr(ptr) {} + // }; + // struct A { int x; } a; // say this isn't zero initialized + // + // B b(&a); // warning + // + // For cases like this, we can't report that 'this->ptr->a' is uninitialized, + // since void pointers can't be dereferenced, but rather the note message + // should say 'static_cast(this->ptr)->a' is uninitialized. + // + // We'll store which elements of the fieldchain needs to be casted back, + // and the type it needs to be casted to. + using CastBack = const std::pair; + using CastBacksListImpl = llvm::ImmutableListImpl; + +public: + // We'll expose these types for a nicer looking factory instantionation. using FieldChain = llvm::ImmutableList; + using CastBackList = llvm::ImmutableList; +private: FieldChain Chain; + CastBackList CastBacks; const bool IsDereferenced = false; @@ -68,11 +95,15 @@ FieldChainInfo() = default; FieldChainInfo(const FieldChainInfo &Other, const bool IsDereferenced) - : Chain(Other.Chain), IsDereferenced(IsDereferenced) {} + : Chain(Other.Chain), CastBacks(Other.CastBacks), + IsDereferenced(IsDereferenced) {} FieldChainInfo(const FieldChainInfo &Other, const FieldRegion *FR, const bool IsDereferenced = false); + FieldChainInfo(const FieldChainInfo &Other, const FieldRegion *FR, + const bool IsDereferenced, QualType CastBackType); + bool contains(const FieldRegion *FR) const { return Chain.contains(FR); } bool isPointer() const; @@ -88,8 +119,8 @@ /// elements in reverse order, and have no reverse iterators, we use a /// recursive function to print the fieldchain correctly. The last element in /// the chain is to be printed by `print`. - static void printTail(llvm::raw_ostream &Out, - const llvm::ImmutableListImpl *L); + static void printTail(llvm::raw_ostream &Out, const FieldChainImpl *L, + const CastBacksListImpl *C); friend struct FieldChainInfoComparator; }; @@ -201,7 +232,8 @@ // Static variable instantionations. -static llvm::ImmutableListFactory Factory; +static FieldChainInfo::FieldChain::Factory ChainFactory; +static FieldChainInfo::CastBackList::Factory CastBackFactory; // Utility function declarations. @@ -489,6 +521,9 @@ } QualType DynT = DynTInfo.getType(); + // If the static type of the field is a void pointer, we need to cast it back + // to the dynamic type before dereferencing. + bool NeedsCastBack = isVoidPointer(FR->getDecl()->getType()); if (isVoidPointer(DynT)) { IsAnyFieldInitialized = true; @@ -528,11 +563,18 @@ const TypedValueRegion *R = RecordV->getRegion(); - if (DynT->getPointeeType()->isStructureOrClassType()) + if (DynT->getPointeeType()->isStructureOrClassType()) { + if (NeedsCastBack) + return isNonUnionUninit( + R, {LocalChain, FR, /*IsDereferenced */ false, DynT}); return isNonUnionUninit(R, {LocalChain, FR}); + } if (DynT->getPointeeType()->isUnionType()) { if (isUnionUninit(R)) { + if (NeedsCastBack) + return addFieldToUninits( + {LocalChain, FR, /*IsDereferenced*/ true, DynT}); return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true}); } else { IsAnyFieldInitialized = true; @@ -553,8 +595,11 @@ "At this point FR must either have a primitive dynamic type, or it " "must be a null, undefined, unknown or concrete pointer!"); - if (isPrimitiveUninit(DerefdV)) + if (isPrimitiveUninit(DerefdV)) { + if (NeedsCastBack) + return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true, DynT}); return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true}); + } IsAnyFieldInitialized = true; return false; @@ -577,7 +622,16 @@ : FieldChainInfo(Other, IsDereferenced) { assert(!contains(FR) && "Can't add a field that is already a part of the " "fieldchain! Is this a cyclic reference?"); - Chain = Factory.add(FR, Other.Chain); + Chain = ChainFactory.add(FR, Other.Chain); +} + +FieldChainInfo::FieldChainInfo(const FieldChainInfo &Other, + const FieldRegion *FR, const bool IsDereferenced, + QualType CastBackType) + : FieldChainInfo(Other, FR, IsDereferenced) { + CastBacks = CastBackFactory.add( + std::make_pair(CastBackType, Chain.getInternalPointer()), + Other.CastBacks); } bool FieldChainInfo::isPointer() const { @@ -595,25 +649,6 @@ return (*Chain.begin())->getDecl(); } -// TODO: This function constructs an incorrect string if a void pointer is a -// part of the chain: -// -// struct B { int x; } -// -// struct A { -// void *vptr; -// A(void* vptr) : vptr(vptr) {} -// }; -// -// void f() { -// B b; -// A a(&b); -// } -// -// The note message will be "uninitialized field 'this->vptr->x'", even though -// void pointers can't be dereferenced. This should be changed to "uninitialized -// field 'static_cast(this->vptr)->x'". -// // TODO: This function constructs an incorrect fieldchain string in the // following case: // @@ -632,21 +667,46 @@ if (Chain.isEmpty()) return; - const llvm::ImmutableListImpl *L = - Chain.getInternalPointer(); - printTail(Out, L->getTail()); - Out << getVariableName(L->getHead()->getDecl()); + const FieldChainImpl *L = Chain.getInternalPointer(); + const CastBacksListImpl *C = CastBacks.getInternalPointer(); + const FieldDecl *Field = L->getHead()->getDecl(); + + // During the construction of a FieldChainInfo object, the CastBacks field + // will be filled in order with pointers to certain elements of the FieldChain + // that needs to be casted to another type. + // If L is the first element stored in CastBacks, we'll pass the rest of the + // CastBacks list to printTail. If not, we know that the first element of it + // will refer to another field in the fieldchain, unless no field needs to be + // casted back. + if (C && L == C->getHead().second) { + printTail(Out, L->getTail(), C->getTail()); + Out << "static_cast<" << C->getHead().first.getAsString() << ">(" + << getVariableName(Field) << ')'; + } else { + printTail(Out, L->getTail(), C); + Out << getVariableName(Field); + } } -void FieldChainInfo::printTail( - llvm::raw_ostream &Out, - const llvm::ImmutableListImpl *L) { - if (!L) +void FieldChainInfo::printTail(llvm::raw_ostream &Out, const FieldChainImpl *L, + const CastBacksListImpl *C) { + if (!L) { + assert(!C && "Entire fieldchain printed but some elements of CastBacks is " + "unused!"); return; + } + + if (C && L == C->getHead().second) + printTail(Out, L->getTail(), C->getTail()); + else + printTail(Out, L->getTail(), C); - printTail(Out, L->getTail()); const FieldDecl *Field = L->getHead()->getDecl(); - Out << getVariableName(Field); + if (C && L == C->getHead().second) + Out << "static_cast<" << C->getHead().first.getAsString() << ">(" + << getVariableName(Field) << ')'; + else + Out << getVariableName(Field); Out << (Field->getType()->isPointerType() ? "->" : "."); } Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp =================================================================== --- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp +++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp @@ -287,7 +287,7 @@ } struct IntDynTypedVoidPointerTest1 { - void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}} + void *vptr; // expected-note{{uninitialized pointee 'this->static_cast(vptr)'}} int dontGetFilteredByNonPedanticMode = 0; IntDynTypedVoidPointerTest1(void *vptr) : vptr(vptr) {} // expected-warning{{1 uninitialized field}} @@ -300,8 +300,8 @@ struct RecordDynTypedVoidPointerTest { struct RecordType { - int x; // expected-note{{uninitialized field 'this->vptr->x'}} - int y; // expected-note{{uninitialized field 'this->vptr->y'}} + int x; // expected-note{{uninitialized field 'this->static_cast(vptr)->x'}} + int y; // expected-note{{uninitialized field 'this->static_cast(vptr)->y'}} }; void *vptr; @@ -317,9 +317,9 @@ struct NestedNonVoidDynTypedVoidPointerTest { struct RecordType { - int x; // expected-note{{uninitialized field 'this->vptr->x'}} - int y; // expected-note{{uninitialized field 'this->vptr->y'}} - void *vptr; // expected-note{{uninitialized pointee 'this->vptr->vptr'}} + int x; // expected-note{{uninitialized field 'this->static_cast(vptr)->x'}} + int y; // expected-note{{uninitialized field 'this->static_cast(vptr)->y'}} + void *vptr; // expected-note{{uninitialized pointee 'this->static_cast(vptr)->static_cast(vptr)'}} }; void *vptr;