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 patch aims to fix derefencing, which has been debated for months now.
Instead of working with `SVal`s, the function now relies on `TypedValueRegion`.
Since this has been discussed since the inception of the checker, I'd very much
prefer finding a permanent solution for this before moving forward.
Repository:
rC Clang
https://reviews.llvm.org/D51057
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.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
@@ -203,18 +203,14 @@
CyclicPointerTest1();
}
-// TODO: Currently, the checker ends up in an infinite loop for the following
-// test case.
-/*
struct CyclicPointerTest2 {
- int **pptr;
+ int **pptr; // no-crash
CyclicPointerTest2() : pptr(reinterpret_cast<int **>(&pptr)) {}
};
void fCyclicPointerTest2() {
CyclicPointerTest2();
}
-*/
//===----------------------------------------------------------------------===//
// Void pointer tests.
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -95,11 +95,13 @@
/// known, and thus FD can not be analyzed.
static bool isVoidPointer(QualType T);
-/// Dereferences \p V and returns the value and dynamic type of the pointee, as
-/// well as wether \p FR needs to be casted back to that type. If for whatever
-/// reason dereferencing fails, returns with None.
-static llvm::Optional<std::tuple<SVal, QualType, bool>>
-dereference(ProgramStateRef State, const FieldRegion *FR);
+using DereferenceInfo = std::pair<const TypedValueRegion *, bool>;
+
+/// Dereferences \p FR and returns with the pointee's region, and whether it
+/// needs to be casted back to it's location type. If for whatever reason
+/// dereferencing fails, returns with None.
+static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
+ const FieldRegion *FR);
//===----------------------------------------------------------------------===//
// Methods for FindUninitializedFields.
@@ -110,9 +112,7 @@
bool FindUninitializedFields::isPointerOrReferenceUninit(
const FieldRegion *FR, FieldChainInfo LocalChain) {
- assert((FR->getDecl()->getType()->isAnyPointerType() ||
- FR->getDecl()->getType()->isReferenceType() ||
- FR->getDecl()->getType()->isBlockPointerType()) &&
+ assert(isLocType(FR->getDecl()->getType()) &&
"This method only checks pointer/reference objects!");
SVal V = State->getSVal(FR);
@@ -123,6 +123,8 @@
}
if (V.isUndef()) {
+ assert(!FR->getDecl()->getType()->isReferenceType() &&
+ "References must be initialized!");
return addFieldToUninits(
LocalChain.add(LocField(FR, /*IsDereferenced*/ false)));
}
@@ -134,54 +136,46 @@
// At this point the pointer itself is initialized and points to a valid
// location, we'll now check the pointee.
- llvm::Optional<std::tuple<SVal, QualType, bool>> DerefInfo =
- dereference(State, FR);
+ llvm::Optional<DereferenceInfo> DerefInfo = dereference(State, FR);
if (!DerefInfo) {
IsAnyFieldInitialized = true;
return false;
}
- V = std::get<0>(*DerefInfo);
- QualType DynT = std::get<1>(*DerefInfo);
- bool NeedsCastBack = std::get<2>(*DerefInfo);
+ const TypedValueRegion *R = DerefInfo->first;
+ const bool NeedsCastBack = DerefInfo->second;
- // If FR is a pointer pointing to a non-primitive type.
- if (Optional<nonloc::LazyCompoundVal> RecordV =
- V.getAs<nonloc::LazyCompoundVal>()) {
+ QualType DynT = R->getLocationType();
+ QualType PointeeT = DynT->getPointeeType();
- const TypedValueRegion *R = RecordV->getRegion();
+ if (PointeeT->isStructureOrClassType()) {
+ if (NeedsCastBack)
+ return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
+ return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
+ }
- if (DynT->getPointeeType()->isStructureOrClassType()) {
+ if (PointeeT->isUnionType()) {
+ if (isUnionUninit(R)) {
if (NeedsCastBack)
- return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
- return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
- }
-
- if (DynT->getPointeeType()->isUnionType()) {
- if (isUnionUninit(R)) {
- if (NeedsCastBack)
- return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
- return addFieldToUninits(LocalChain.add(LocField(FR)));
- } else {
- IsAnyFieldInitialized = true;
- return false;
- }
- }
-
- if (DynT->getPointeeType()->isArrayType()) {
+ return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
+ return addFieldToUninits(LocalChain.add(LocField(FR)));
+ } else {
IsAnyFieldInitialized = true;
return false;
}
+ }
- llvm_unreachable("All cases are handled!");
+ if (PointeeT->isArrayType()) {
+ IsAnyFieldInitialized = true;
+ return false;
}
- assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isAnyPointerType() ||
- DynT->isReferenceType()) &&
+ assert((isPrimitiveType(PointeeT) || isLocType(PointeeT)) &&
"At this point FR must either have a primitive dynamic type, or it "
"must be a null, undefined, unknown or concrete pointer!");
- if (isPrimitiveUninit(V)) {
+ const SVal &PointeeV = State->getSVal(loc::MemRegionVal(R));
+ if (isPrimitiveUninit(PointeeV)) {
if (NeedsCastBack)
return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
return addFieldToUninits(LocalChain.add(LocField(FR)));
@@ -204,47 +198,54 @@
return false;
}
-static llvm::Optional<std::tuple<SVal, QualType, bool>>
-dereference(ProgramStateRef State, const FieldRegion *FR) {
-
- DynamicTypeInfo DynTInfo;
- QualType DynT;
+static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
+ const FieldRegion *FR) {
// 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());
SVal V = State->getSVal(FR);
assert(V.getAs<loc::MemRegionVal>() && "V must be loc::MemRegionVal!");
+ auto Tmp = V.getAs<loc::MemRegionVal>();
+
+ // 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 (Tmp->getRegion()->getSymbolicBase()) {
+ return None;
+ }
+
+ // The region we'd like to acquire.
+ const auto *R = Tmp->getRegionAs<TypedValueRegion>();
+ assert(R);
+
+ DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, R);
+ if (!DynTInfo.isValid()) {
+ return None;
+ }
+
+ QualType DynT = DynTInfo.getType();
// If V is multiple pointer value, we'll dereference it again (e.g.: int** ->
// int*).
- // TODO: Dereference according to the dynamic type to avoid infinite loop for
- // these kind of fields:
- // int **ptr = reinterpret_cast<int **>(&ptr);
- while (auto Tmp = V.getAs<loc::MemRegionVal>()) {
- // 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 (Tmp->getRegion()->getSymbolicBase()) {
- return None;
- }
+ Tmp = State->getSVal(*Tmp, DynT).getAs<loc::MemRegionVal>();
- DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
- if (!DynTInfo.isValid()) {
- return None;
- }
-
- DynT = DynTInfo.getType();
+ // In order to ensure that this loop terminates, we're also checking the
+ // dynamic type of FR, since type hierarchy is finite.
+ while (Tmp && isLocType(DynT->getPointeeType())) {
- if (isVoidPointer(DynT)) {
+ if (Tmp->getRegion()->getSymbolicBase())
return None;
- }
- V = State->getSVal(*Tmp, DynT);
+ DynT = DynT->getPointeeType();
+ R = Tmp->getRegionAs<TypedValueRegion>();
+ assert(R && "R must be a TypedValueRegion!");
+
+ Tmp = State->getSVal(*Tmp, DynT).getAs<loc::MemRegionVal>();
}
- return std::make_tuple(V, DynT, NeedsCastBack);
+ return std::make_pair(R, NeedsCastBack);
}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -292,8 +292,7 @@
continue;
}
- if (T->isAnyPointerType() || T->isReferenceType() ||
- T->isBlockPointerType()) {
+ if (isLocType(T)) {
if (isPointerOrReferenceUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
@@ -258,6 +258,11 @@
return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
}
+inline bool isLocType(const QualType &T) {
+ return T->isAnyPointerType() || T->isReferenceType() ||
+ T->isBlockPointerType();
+}
+
// Template method definitions.
template <class FieldNodeT>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
