r342215 - [analyzer][UninitializedObjectChecker] Updated comments
Kristof Umann via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 14 02:07:40 PDT 2018
Author: szelethus
Date: Fri Sep 14 02:07:40 2018
New Revision: 342215
URL: http://llvm.org/viewvc/llvm-project?rev=342215&view=rev
Log:
[analyzer][UninitializedObjectChecker] Updated comments
Some of the comments are incorrect, imprecise, or simply nonexistent.
Since I have a better grasp on how the analyzer works, it makes sense
to update most of them in a single swoop.
I tried not to flood the code with comments too much, this amount
feels just right to me.
Differential Revision: https://reviews.llvm.org/D51417
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h?rev=342215&r1=342214&r2=342215&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h Fri Sep 14 02:07:40 2018
@@ -26,31 +26,36 @@
namespace clang {
namespace ento {
-/// Represent a single field. This is only an interface to abstract away special
-/// cases like pointers/references.
+/// A lightweight polymorphic wrapper around FieldRegion *. We'll use this
+/// interface to store addinitional information about fields. As described
+/// later, a list of these objects (i.e. "fieldchain") will be constructed and
+/// used for printing note messages should an uninitialized value be found.
class FieldNode {
protected:
const FieldRegion *FR;
+ /// FieldNodes are never meant to be created on the heap, see
+ /// FindUninitializedFields::addFieldToUninits().
/* non-virtual */ ~FieldNode() = default;
public:
FieldNode(const FieldRegion *FR) : FR(FR) {}
+ // We'll delete all of these special member functions to force the users of
+ // this interface to only store references to FieldNode objects in containers.
FieldNode() = delete;
FieldNode(const FieldNode &) = delete;
FieldNode(FieldNode &&) = delete;
FieldNode &operator=(const FieldNode &) = delete;
FieldNode &operator=(const FieldNode &&) = delete;
- /// Profile - Used to profile the contents of this object for inclusion in a
- /// FoldingSet.
void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(this); }
- // Helper method for uniqueing.
+ /// Helper method for uniqueing.
bool isSameRegion(const FieldRegion *OtherFR) const {
- // Special FieldNode descendants may wrap nullpointers -- we wouldn't like
- // to unique these objects.
+ // Special FieldNode descendants may wrap nullpointers (for example if they
+ // describe a special relationship between two elements of the fieldchain)
+ // -- we wouldn't like to unique these objects.
if (FR == nullptr)
return false;
@@ -63,19 +68,22 @@ public:
return FR->getDecl();
}
- // When a fieldchain is printed (a list of FieldNode objects), it will have
- // the following format:
- // <note message>'<prefix>this-><node><separator><node><separator>...<node>'
+ // When a fieldchain is printed, it will have the following format (without
+ // newline, indices are in order of insertion, from 1 to n):
+ //
+ // <note_message_n>'<prefix_n><prefix_n-1>...<prefix_1>
+ // this-><node_1><separator_1><node_2><separator_2>...<node_n>'
- /// If this is the last element of the fieldchain, this method will be called.
+ /// If this is the last element of the fieldchain, this method will print the
+ /// note message associated with it.
/// The note message should state something like "uninitialized field" or
/// "uninitialized pointee" etc.
virtual void printNoteMsg(llvm::raw_ostream &Out) const = 0;
- /// Print any prefixes before the fieldchain.
+ /// Print any prefixes before the fieldchain. Could contain casts, etc.
virtual void printPrefix(llvm::raw_ostream &Out) const = 0;
- /// Print the node. Should contain the name of the field stored in getRegion.
+ /// Print the node. Should contain the name of the field stored in FR.
virtual void printNode(llvm::raw_ostream &Out) const = 0;
/// Print the separator. For example, fields may be separated with '.' or
@@ -89,14 +97,14 @@ public:
/// even if Field is a captured lambda variable.
std::string getVariableName(const FieldDecl *Field);
-/// Represents a field chain. A field chain is a vector of fields where the
-/// first element of the chain is the object under checking (not stored), and
-/// every other element is a field, and the element that precedes it is the
-/// object that contains it.
+/// Represents a field chain. A field chain is a list of fields where the first
+/// element of the chain is the object under checking (not stored), and every
+/// other element is a field, and the element that precedes it is the object
+/// that contains it.
///
/// Note that this class is immutable (essentially a wrapper around an
-/// ImmutableList), and new elements can only be added by creating new
-/// FieldChainInfo objects through add().
+/// ImmutableList), new FieldChainInfo objects may be created by member
+/// functions such as add() and replaceHead().
class FieldChainInfo {
public:
using FieldChainImpl = llvm::ImmutableListImpl<const FieldNode &>;
@@ -116,7 +124,11 @@ public:
FieldChainInfo(FieldChain::Factory &F) : ChainFactory(F) {}
FieldChainInfo(const FieldChainInfo &Other) = default;
+ /// Constructs a new FieldChainInfo object with \p FN appended.
template <class FieldNodeT> FieldChainInfo add(const FieldNodeT &FN);
+
+ /// Constructs a new FieldChainInfo object with \p FN as the new head of the
+ /// list.
template <class FieldNodeT> FieldChainInfo replaceHead(const FieldNodeT &FN);
bool contains(const FieldRegion *FR) const;
@@ -124,6 +136,7 @@ public:
const FieldRegion *getUninitRegion() const;
const FieldNode &getHead() { return Chain.getHead(); }
+
void printNoteMsg(llvm::raw_ostream &Out) const;
};
@@ -161,19 +174,21 @@ public:
const UninitFieldMap &getUninitFields() { return UninitFields; }
/// Returns whether the analyzed region contains at least one initialized
- /// field.
+ /// field. Note that this includes subfields as well, not just direct ones,
+ /// and will return false if an uninitialized pointee is found with
+ /// CheckPointeeInitialization enabled.
bool isAnyFieldInitialized() { return IsAnyFieldInitialized; }
private:
- // For the purposes of this checker, we'll regard the object under checking as
- // a directed tree, where
+ // For the purposes of this checker, we'll regard the analyzed region as a
+ // directed tree, where
// * the root is the object under checking
// * every node is an object that is
// - a union
// - a non-union record
- // - a pointer/reference
+ // - dereferencable (see isDereferencableType())
// - an array
- // - of a primitive type, which we'll define later in a helper function.
+ // - of a primitive type (see isPrimitiveType())
// * the parent of each node is the object that contains it
// * every leaf is an array, a primitive object, a nullptr or an undefined
// pointer.
@@ -215,22 +230,20 @@ private:
// We'll traverse each node of the above graph with the appropiate one of
// these methods:
- /// This method checks a region of a union object, and returns true if no
- /// field is initialized within the region.
+ /// Checks the region of a union object, and returns true if no field is
+ /// initialized within the region.
bool isUnionUninit(const TypedValueRegion *R);
- /// This method checks a region of a non-union object, and returns true if
- /// an uninitialized field is found within the region.
+ /// Checks a region of a non-union object, and returns true if an
+ /// uninitialized field is found within the region.
bool isNonUnionUninit(const TypedValueRegion *R, FieldChainInfo LocalChain);
- /// This method checks a region of a pointer or reference object, and returns
- /// true if the ptr/ref object itself or any field within the pointee's region
- /// is uninitialized.
- bool isPointerOrReferenceUninit(const FieldRegion *FR,
- FieldChainInfo LocalChain);
-
- /// This method returns true if the value of a primitive object is
+ /// Checks a region of a pointer or reference object, and returns true if the
+ /// ptr/ref object itself or any field within the pointee's region is
/// uninitialized.
+ bool isDereferencableUninit(const FieldRegion *FR, FieldChainInfo LocalChain);
+
+ /// Returns true if the value of a primitive object is uninitialized.
bool isPrimitiveUninit(const SVal &V);
// Note that we don't have a method for arrays -- the elements of an array are
@@ -249,11 +262,8 @@ private:
bool addFieldToUninits(FieldChainInfo LocalChain);
};
-/// 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
-/// value is undefined or not, such as ints and doubles, can be analyzed with
-/// ease. This also helps ensuring that every special field type is handled
-/// correctly.
+/// Returns true if T is a primitive type. An object of a primitive type only
+/// needs to be analyzed as much as checking whether their value is undefined.
inline bool isPrimitiveType(const QualType &T) {
return T->isBuiltinType() || T->isEnumeralType() ||
T->isMemberPointerType() || T->isBlockPointerType() ||
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp?rev=342215&r1=342214&r2=342215&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp Fri Sep 14 02:07:40 2018
@@ -94,7 +94,9 @@ public:
};
/// Represents that the FieldNode that comes after this is declared in a base
-/// of the previous FieldNode.
+/// of the previous FieldNode. As such, this descendant doesn't wrap a
+/// FieldRegion, and is purely a tool to describe a relation between two other
+/// FieldRegion wrapping descendants.
class BaseClass final : public FieldNode {
const QualType BaseClassT;
@@ -296,7 +298,7 @@ bool FindUninitializedFields::isNonUnion
}
if (isDereferencableType(T)) {
- if (isPointerOrReferenceUninit(FR, LocalChain))
+ if (isDereferencableUninit(FR, LocalChain))
ContainsUninitField = true;
continue;
}
@@ -314,7 +316,8 @@ bool FindUninitializedFields::isNonUnion
llvm_unreachable("All cases are handled!");
}
- // Checking bases.
+ // Checking bases. The checker will regard inherited data members as direct
+ // fields.
const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
if (!CXXRD)
return ContainsUninitField;
@@ -361,6 +364,9 @@ bool FindUninitializedFields::isPrimitiv
const FieldRegion *FieldChainInfo::getUninitRegion() const {
assert(!Chain.isEmpty() && "Empty fieldchain!");
+
+ // ImmutableList::getHead() isn't a const method, hence the not too nice
+ // implementation.
return (*Chain.begin()).getRegion();
}
@@ -375,31 +381,11 @@ bool FieldChainInfo::contains(const Fiel
/// Prints every element except the last to `Out`. Since ImmutableLists store
/// 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`.
+/// the chain is to be printed by `FieldChainInfo::print`.
static void printTail(llvm::raw_ostream &Out,
const FieldChainInfo::FieldChainImpl *L);
-// 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:
+// FIXME: This function constructs an incorrect string in the following case:
//
// struct Base { int x; };
// struct D1 : Base {}; struct D2 : Base {};
@@ -515,6 +501,7 @@ std::string clang::ento::getVariableName
void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
auto Chk = Mgr.registerChecker<UninitializedObjectChecker>();
+
Chk->IsPedantic = Mgr.getAnalyzerOptions().getBooleanOption(
"Pedantic", /*DefaultVal*/ false, Chk);
Chk->ShouldConvertNotesToWarnings = Mgr.getAnalyzerOptions().getBooleanOption(
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp?rev=342215&r1=342214&r2=342215&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp Fri Sep 14 02:07:40 2018
@@ -1,4 +1,4 @@
-//===----- UninitializedPointer.cpp ------------------------------*- C++ -*-==//
+//===----- UninitializedPointee.cpp ------------------------------*- C++ -*-==//
//
// The LLVM Compiler Infrastructure
//
@@ -90,9 +90,8 @@ public:
// Utility function declarations.
-/// 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.
+/// Returns whether \p T can be (transitively) dereferenced to a void pointer
+/// type (void*, void**, ...).
static bool isVoidPointer(QualType T);
using DereferenceInfo = std::pair<const TypedValueRegion *, bool>;
@@ -107,9 +106,7 @@ static llvm::Optional<DereferenceInfo> d
// Methods for FindUninitializedFields.
//===----------------------------------------------------------------------===//
-// Note that pointers/references don't contain fields themselves, so in this
-// function we won't add anything to LocalChain.
-bool FindUninitializedFields::isPointerOrReferenceUninit(
+bool FindUninitializedFields::isDereferencableUninit(
const FieldRegion *FR, FieldChainInfo LocalChain) {
assert(isDereferencableType(FR->getDecl()->getType()) &&
@@ -222,12 +219,11 @@ static llvm::Optional<DereferenceInfo> d
while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
R = Tmp->getAs<TypedValueRegion>();
-
if (!R)
return None;
// We found a cyclic pointer, like int *ptr = (int *)&ptr.
- // TODO: Report these fields too.
+ // TODO: Should we report these fields too?
if (!VisitedRegions.insert(R).second)
return None;
More information about the cfe-commits
mailing list