Szelethus updated this revision to Diff 155915.
Szelethus added a comment.
Rebased to the latest trunk.
https://reviews.llvm.org/D49228
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
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<int *>(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<struct RecordDynTypedVoidPointerTest::RecordType *>(vptr)->x'}}
+ int y; // expected-note{{uninitialized field 'this->static_cast<struct RecordDynTypedVoidPointerTest::RecordType *>(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<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->x'}}
+ int y; // expected-note{{uninitialized field 'this->static_cast<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->y'}}
+ void *vptr; // expected-note{{uninitialized pointee 'this->static_cast<struct NestedNonVoidDynTypedVoidPointerTest::RecordType *>(vptr)->static_cast<char *>(vptr)'}}
};
void *vptr;
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -58,21 +58,52 @@
/// Note that this class is immutable, and new fields may only be added through
/// constructor calls.
class FieldChainInfo {
+ using FieldChainImpl = llvm::ImmutableListImpl<const FieldRegion *>;
+
+ // 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<A *>(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<QualType, const FieldChainImpl *>;
+ using CastBacksListImpl = llvm::ImmutableListImpl<CastBack>;
+
+public:
+ // We'll expose these types for a nicer looking factory instantionation.
using FieldChain = llvm::ImmutableList<const FieldRegion *>;
+ using CastBackList = llvm::ImmutableList<CastBack>;
+private:
FieldChain Chain;
+ CastBackList CastBacks;
const bool IsDereferenced = false;
public:
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<const FieldRegion *> *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<const FieldRegion *> 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<B*>(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<const FieldRegion *> *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<const FieldRegion *> *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() ? "->" : ".");
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
