Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
This diff fixes a long debated issues with pointers/references not being
dereferenced according to their dynamic type. This also fixed an issue where
regions that were pointed to by void pointers were not analyzed.
Note how I also added a test case about inheritance, clearly showing that it
doesn't find an uninitialized field that it should. Because base class handling
is still being discussed, I'm planning to fix that issue in a different patch.
The reason why it's still in this patch is that it might be closely tied to
these changes too.
Thumbs up to @NoQ for setting me on the right track!
<http://lists.llvm.org/pipermail/cfe-dev/2018-June/058271.html>
Repository:
rC Clang
https://reviews.llvm.org/D49199
Files:
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-inheritance.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
@@ -191,7 +191,7 @@
struct CyclicPointerTest {
int *ptr;
- CyclicPointerTest() : ptr(reinterpret_cast<int*>(&ptr)) {}
+ CyclicPointerTest() : ptr(reinterpret_cast<int *>(&ptr)) {}
};
void fCyclicPointerTest() {
@@ -280,13 +280,62 @@
void *vptr; // no-crash
CyclicVoidPointerTest() : vptr(&vptr) {}
-
};
void fCyclicVoidPointerTest() {
CyclicVoidPointerTest();
}
+struct IntDynTypedVoidPointerTest1 {
+ void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}}
+ int dontGetFilteredByNonPedanticMode = 0;
+
+ IntDynTypedVoidPointerTest1(void *vptr) : vptr(vptr) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fIntDynTypedVoidPointerTest1() {
+ int a;
+ IntDynTypedVoidPointerTest1 tmp(&a);
+}
+
+struct RecordDynTypedVoidPointerTest {
+ struct RecordType {
+ int x; // expected-note{{uninitialized field 'this->vptr->x'}}
+ int y; // expected-note{{uninitialized field 'this->vptr->y'}}
+ };
+
+ void *vptr;
+ int dontGetFilteredByNonPedanticMode = 0;
+
+ RecordDynTypedVoidPointerTest(void *vptr) : vptr(vptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fRecordDynTypedVoidPointerTest() {
+ RecordDynTypedVoidPointerTest::RecordType a;
+ RecordDynTypedVoidPointerTest tmp(&a);
+}
+
+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'}}
+ };
+
+ void *vptr;
+ int dontGetFilteredByNonPedanticMode = 0;
+
+ NestedNonVoidDynTypedVoidPointerTest(void *vptr, void *c) : vptr(vptr) {
+ static_cast<RecordType *>(vptr)->vptr = c; // expected-warning{{3 uninitialized fields}}
+ }
+};
+
+void fNestedNonVoidDynTypedVoidPointerTest() {
+ NestedNonVoidDynTypedVoidPointerTest::RecordType a;
+ char c;
+ NestedNonVoidDynTypedVoidPointerTest tmp(&a, &c);
+}
+
//===----------------------------------------------------------------------===//
// Multipointer tests.
//===----------------------------------------------------------------------===//
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===================================================================
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -773,3 +773,26 @@
void fVirtualDiamondInheritanceTest3() {
VirtualDiamondInheritanceTest3();
}
+
+//===----------------------------------------------------------------------===//
+// Dynamic type test.
+//===----------------------------------------------------------------------===//
+
+struct DynTBase {};
+struct DynTDerived : DynTBase {
+ // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
+ int x; // no-note
+};
+
+struct DynamicTypeTest {
+ DynTBase *bptr;
+ int i = 0;
+
+ // TODO: we'd expect the warning: {{1 uninitialized field}}
+ DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+};
+
+void f() {
+ DynTDerived d;
+ DynamicTypeTest t(&d);
+};
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -30,7 +30,7 @@
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include <algorithm>
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
using namespace clang;
using namespace clang::ento;
@@ -210,10 +210,10 @@
/// constructor.
bool isCalledByConstructor(const CheckerContext &Context);
-/// Returns whether FD can be (transitively) dereferenced to a void pointer type
+/// Returns whether T can be (transitively) dereferenced to a void pointer type
/// (void*, void**, ...). The type of the region behind a void pointer isn't
/// known, and thus FD can not be analyzed.
-bool isVoidPointer(const FieldDecl *FD);
+bool isVoidPointer(QualType T);
/// Returns true if T is a primitive type. We defined this type so that for
/// objects that we'd only like analyze as much as checking whether their
@@ -452,75 +452,99 @@
SVal V = State->getSVal(FR);
- if (V.isUnknown() || V.isZeroConstant()) {
+ if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) {
IsAnyFieldInitialized = true;
return false;
}
if (V.isUndef()) {
return addFieldToUninits({LocalChain, FR});
}
- const FieldDecl *FD = FR->getDecl();
+ assert(V.getAs<loc::MemRegionVal>() &&
+ "At this point V must be loc::MemRegionVal!");
+ auto L = V.castAs<loc::MemRegionVal>();
- // TODO: The dynamic type of a void pointer may be retrieved with
- // `getDynamicTypeInfo`.
- if (isVoidPointer(FD)) {
+ // We can't reason about symbolic regions, assume its initialized.
+ // Note that this also avoids a potential infinite recursion, because
+ // constructors for list-like classes are checked without being called, and
+ // the Static Analyzer will construct a symbolic region for Node *next; or
+ // similar code snippets.
+ if (L.getRegion()->getSymbolicBase()) {
IsAnyFieldInitialized = true;
return false;
}
- assert(V.getAs<Loc>() && "V should be Loc at this point!");
+ DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion());
+ if (!DynTInfo.isValid()) {
+ IsAnyFieldInitialized = true;
+ return false;
+ }
- // At this point the pointer itself is initialized and points to a valid
- // location, we'll now check the pointee.
- SVal DerefdV = State->getSVal(V.castAs<Loc>());
+ QualType DynT = DynTInfo.getType();
- // TODO: Dereferencing should be done according to the dynamic type.
- while (Optional<Loc> L = DerefdV.getAs<Loc>()) {
- DerefdV = State->getSVal(*L);
+ if (isVoidPointer(DynT)) {
+ IsAnyFieldInitialized = true;
+ return false;
}
- // If V is a pointer pointing to a record type.
- if (Optional<nonloc::LazyCompoundVal> RecordV =
- DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+ // At this point the pointer itself is initialized and points to a valid
+ // location, we'll now check the pointee.
+ SVal DerefdV = State->getSVal(V.castAs<Loc>(), DynT);
- const TypedValueRegion *R = RecordV->getRegion();
+ // If DerefdV is still a pointer value, we'll dereference it again (e.g.:
+ // int** -> int*).
+ while (auto Tmp = DerefdV.getAs<loc::MemRegionVal>()) {
+ if (Tmp->getRegion()->getSymbolicBase()) {
+ IsAnyFieldInitialized = true;
+ return false;
+ }
- // We can't reason about symbolic regions, assume its initialized.
- // Note that this also avoids a potential infinite recursion, because
- // constructors for list-like classes are checked without being called, and
- // the Static Analyzer will construct a symbolic region for Node *next; or
- // similar code snippets.
- if (R->getSymbolicBase()) {
+ DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
+ if (!DynTInfo.isValid()) {
IsAnyFieldInitialized = true;
return false;
}
- const QualType T = R->getValueType();
+ DynT = DynTInfo.getType();
+ if (isVoidPointer(DynT)) {
+ IsAnyFieldInitialized = true;
+ return false;
+ }
- if (T->isStructureOrClassType())
+ DerefdV = State->getSVal(*Tmp, DynT);
+ }
+
+ // If FR is a pointer pointing to a non-primitive type.
+ if (Optional<nonloc::LazyCompoundVal> RecordV =
+ DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+
+ const TypedValueRegion *R = RecordV->getRegion();
+
+ if (DynT->getPointeeType()->isStructureOrClassType())
return isNonUnionUninit(R, {LocalChain, FR});
- if (T->isUnionType()) {
+ if (DynT->getPointeeType()->isUnionType()) {
if (isUnionUninit(R)) {
return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
} else {
IsAnyFieldInitialized = true;
return false;
}
}
- if (T->isArrayType()) {
+ if (DynT->getPointeeType()->isArrayType()) {
IsAnyFieldInitialized = true;
return false;
}
llvm_unreachable("All cases are handled!");
}
- // TODO: If possible, it should be asserted that the DerefdV at this point is
- // primitive.
+ assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isPointerType() ||
+ DynT->isReferenceType()) &&
+ "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))
return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
@@ -564,6 +588,25 @@
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:
//
@@ -622,9 +665,7 @@
namespace {
-bool isVoidPointer(const FieldDecl *FD) {
- QualType T = FD->getType();
-
+bool isVoidPointer(QualType T) {
while (!T.isNull()) {
if (T->isVoidPointerType())
return true;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
