This revision was automatically updated to reflect the committed changes. Closed by commit rL342215: [analyzer][UninitializedObjectChecker] Updated comments (authored by Szelethus, committed by ). Herald added a subscriber: llvm-commits.
Changed prior to commit: https://reviews.llvm.org/D51417?vs=163061&id=165446#toc Repository: rL LLVM https://reviews.llvm.org/D51417 Files: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp @@ -1,4 +1,4 @@ -//===----- UninitializedPointer.cpp ------------------------------*- C++ -*-==// +//===----- UninitializedPointee.cpp ------------------------------*- C++ -*-==// // // The LLVM Compiler Infrastructure // @@ -90,9 +90,8 @@ // 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 @@ // 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 @@ 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; Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -94,7 +94,9 @@ }; /// 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 @@ } if (isDereferencableType(T)) { - if (isPointerOrReferenceUninit(FR, LocalChain)) + if (isDereferencableUninit(FR, LocalChain)) ContainsUninitField = true; continue; } @@ -314,7 +316,8 @@ 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 @@ 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 @@ /// 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 @@ void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) { auto Chk = Mgr.registerChecker<UninitializedObjectChecker>(); + Chk->IsPedantic = Mgr.getAnalyzerOptions().getBooleanOption( "Pedantic", /*DefaultVal*/ false, Chk); Chk->ShouldConvertNotesToWarnings = Mgr.getAnalyzerOptions().getBooleanOption( Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -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 @@ 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 @@ /// 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,14 +124,19 @@ 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; bool isEmpty() const { return Chain.isEmpty(); } const FieldRegion *getUninitRegion() const; const FieldNode &getHead() { return Chain.getHead(); } + void printNoteMsg(llvm::raw_ostream &Out) const; }; @@ -161,19 +174,21 @@ 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 @@ // 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 @@ 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() ||
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits