Szelethus updated this revision to Diff 168592.
Szelethus added a comment.
Reimplemented with AST matchers.
@george.karpenkov I know you wanted to test this feature for a while, but sadly
I've been busy with the macro expansion related projects, I hope it's still
relevant.
https://reviews.llvm.org/D51866
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
Index: test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
===================================================================
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -0,0 +1,257 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN: -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN: -analyzer-config alpha.cplusplus.UninitializedObject:IgnoreGuardedFields=true \
+// RUN: -std=c++11 -verify %s
+
+//===----------------------------------------------------------------------===//
+// Helper functions for tests.
+//===----------------------------------------------------------------------===//
+void halt() __attribute__((__noreturn__));
+void assert(int b) {
+ if (!b)
+ halt();
+}
+
+int rand();
+
+//===----------------------------------------------------------------------===//
+// Tests for fields properly guarded by asserts.
+//===----------------------------------------------------------------------===//
+
+class NoUnguardedFieldsTest {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+private:
+ int Volume, Area;
+ Kind K;
+
+public:
+ NoUnguardedFieldsTest(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0;
+ break;
+ }
+ }
+
+ void operator-() {
+ assert(K == Kind::A);
+ (void)Area;
+ }
+
+ void operator+() {
+ assert(K == Kind::V);
+ (void)Volume;
+ }
+};
+
+void fNoUnguardedFieldsTest() {
+ NoUnguardedFieldsTest T1(NoUnguardedFieldsTest::Kind::A);
+ NoUnguardedFieldsTest T2(NoUnguardedFieldsTest::Kind::V);
+}
+
+class NoUnguardedFieldsWithUndefMethodTest {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+private:
+ int Volume, Area;
+ Kind K;
+
+public:
+ NoUnguardedFieldsWithUndefMethodTest(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0;
+ break;
+ }
+ }
+
+ void operator-() {
+ assert(K == Kind::A);
+ (void)Area;
+ }
+
+ void operator+() {
+ assert(K == Kind::V);
+ (void)Volume;
+ }
+
+ void methodWithoutDefinition();
+};
+
+void fNoUnguardedFieldsWithUndefMethodTest() {
+ NoUnguardedFieldsWithUndefMethodTest
+ T1(NoUnguardedFieldsWithUndefMethodTest::Kind::A);
+ NoUnguardedFieldsWithUndefMethodTest
+ T2(NoUnguardedFieldsWithUndefMethodTest::Kind::V);
+}
+
+class UnguardedFieldThroughMethodTest {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+private:
+ int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+ Kind K;
+
+public:
+ UnguardedFieldThroughMethodTest(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0; // expected-warning {{1 uninitialized field}}
+ break;
+ }
+ }
+
+ void operator-() {
+ assert(K == Kind::A);
+ (void)Area;
+ }
+
+ void operator+() {
+ (void)Volume;
+ }
+};
+
+void fUnguardedFieldThroughMethodTest() {
+ UnguardedFieldThroughMethodTest T1(UnguardedFieldThroughMethodTest::Kind::A);
+}
+
+class UnguardedPublicFieldsTest {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+ int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+ Kind K;
+
+public:
+ UnguardedPublicFieldsTest(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0; // expected-warning {{1 uninitialized field}}
+ break;
+ }
+ }
+
+ void operator-() {
+ assert(K == Kind::A);
+ (void)Area;
+ }
+
+ void operator+() {
+ assert(K == Kind::V);
+ (void)Volume;
+ }
+};
+
+void fUnguardedPublicFieldsTest() {
+ UnguardedPublicFieldsTest T1(UnguardedPublicFieldsTest::Kind::A);
+}
+
+//===----------------------------------------------------------------------===//
+// Highlights of some false negatives due to syntactic checking.
+//===----------------------------------------------------------------------===//
+
+class UnguardedFalseNegativeTest1 {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+private:
+ int Volume, Area;
+ Kind K;
+
+public:
+ UnguardedFalseNegativeTest1(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0;
+ break;
+ }
+ }
+
+ void operator-() {
+ if (rand())
+ assert(K == Kind::A);
+ (void)Area;
+ }
+
+ void operator+() {
+ if (rand())
+ assert(K == Kind::V);
+ (void)Volume;
+ }
+};
+
+void fUnguardedFalseNegativeTest1() {
+ UnguardedFalseNegativeTest1 T1(UnguardedFalseNegativeTest1::Kind::A);
+}
+
+class UnguardedFalseNegativeTest2 {
+public:
+ enum Kind {
+ V,
+ A
+ };
+
+private:
+ int Volume, Area;
+ Kind K;
+
+public:
+ UnguardedFalseNegativeTest2(Kind K) : K(K) {
+ switch (K) {
+ case V:
+ Volume = 0;
+ break;
+ case A:
+ Area = 0;
+ break;
+ }
+ }
+
+ void operator-() {
+ assert(rand());
+ (void)Area;
+ }
+
+ void operator+() {
+ assert(rand());
+ (void)Volume;
+ }
+};
+
+void fUnguardedFalseNegativeTest2() {
+ UnguardedFalseNegativeTest2 T1(UnguardedFalseNegativeTest2::Kind::A);
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -20,13 +20,15 @@
#include "../ClangSACheckers.h"
#include "UninitializedObject.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
using namespace clang;
using namespace clang::ento;
+using namespace clang::ast_matchers;
namespace {
@@ -113,6 +115,16 @@
/// \p Pattern.
static bool shouldIgnoreRecord(const RecordDecl *RD, StringRef Pattern);
+/// Checks _syntactically_ whether it is possible to access FD from the record
+/// that contains it without a preceding assert (even if that access happens
+/// inside a method). This is mainly used for records that act like unions, like
+/// having multiple bit fields, with only a fraction being properly initialized.
+/// If these fields are properly guarded with asserts, this method returns
+/// false.
+///
+/// Since this check is done syntactically, this method could be inaccurate.
+static bool hasUnguardedAccess(const FieldDecl *FD, ProgramStateRef State);
+
//===----------------------------------------------------------------------===//
// Methods for UninitializedObjectChecker.
//===----------------------------------------------------------------------===//
@@ -207,16 +219,19 @@
}
bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain) {
+ const FieldRegion *FR = Chain.getUninitRegion();
+
if (State->getStateManager().getContext().getSourceManager().isInSystemHeader(
- Chain.getUninitRegion()->getDecl()->getLocation()))
+ FR->getDecl()->getLocation()))
+ return false;
+
+ if (Opts.IgnoreGuardedFields && !hasUnguardedAccess(FR->getDecl(), State))
return false;
UninitFieldMap::mapped_type NoteMsgBuf;
llvm::raw_svector_ostream OS(NoteMsgBuf);
Chain.printNoteMsg(OS);
- return UninitFields
- .insert(std::make_pair(Chain.getUninitRegion(), std::move(NoteMsgBuf)))
- .second;
+ return UninitFields.insert(std::make_pair(FR, std::move(NoteMsgBuf))).second;
}
bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,
@@ -456,6 +471,58 @@
return false;
}
+static const Stmt *getMethodBody(const CXXMethodDecl *M) {
+ if (isa<CXXConstructorDecl>(M))
+ return nullptr;
+
+ if (!M->isDefined())
+ return nullptr;
+
+ return M->getDefinition()->getBody();
+}
+
+static bool hasUnguardedAccess(const FieldDecl *FD, ProgramStateRef State) {
+
+ if (FD->getAccess() == AccessSpecifier::AS_public)
+ return true;
+
+ const auto *Parent = dyn_cast<CXXRecordDecl>(FD->getParent());
+
+ if (!Parent)
+ return true;
+
+ Parent = Parent->getDefinition();
+ assert(Parent && "The record's definition must be avaible if an uninitialized"
+ " field of it was found!");
+
+ ASTContext &AC = State->getStateManager().getContext();
+
+ auto FieldAccessM = memberExpr(hasDeclaration(equalsNode(FD))).bind("access");
+ // TODO: Should we only regard asserts as guards, or maybe something else too?
+ auto GuardM = callExpr(callee(functionDecl(hasName("assert")))).bind("guard");
+
+ for (const CXXMethodDecl *M : Parent->methods()) {
+ const Stmt *MethodBody = getMethodBody(M);
+ if (!MethodBody)
+ continue;
+
+ auto Accesses = match(stmt(hasDescendant(FieldAccessM)), *MethodBody, AC);
+ if (Accesses.empty())
+ continue;
+ const auto *FirstAccess = Accesses[0].getNodeAs<MemberExpr>("access");
+
+ auto Guards = match(stmt(hasDescendant(GuardM)), *MethodBody, AC);
+ if (Guards.empty())
+ return true;
+ const auto *FirstGuard = Guards[0].getNodeAs<CallExpr>("guard");
+
+ if (FirstAccess->getBeginLoc() < FirstGuard->getBeginLoc())
+ return true;
+ }
+
+ return false;
+}
+
std::string clang::ento::getVariableName(const FieldDecl *Field) {
// If Field is a captured lambda variable, Field->getName() will return with
// an empty string. We can however acquire it's name from the lambda's
@@ -495,4 +562,6 @@
ChOpts.IgnoredRecordsWithFieldPattern =
AnOpts.getOptionAsString("IgnoreRecordsWithField",
/*DefaultVal*/ "", Chk);
+ ChOpts.IgnoreGuardedFields =
+ AnOpts.getBooleanOption("IgnoreGuardedFields", /*DefaultVal*/ false, Chk);
}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
@@ -35,18 +35,25 @@
// `-analyzer-config \
// alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true`.
//
+// TODO: With some clever heuristics, some pointers should be dereferenced
+// by default. For example, if the pointee is constructed within the
+// constructor call, it's reasonable to say that no external object
+// references it, and we wouldn't generate multiple report on the same
+// pointee.
+//
// - "IgnoreRecordsWithField" (string). If supplied, the checker will not
// analyze structures that have a field with a name or type name that
// matches the given pattern. Defaults to "".
//
// `-analyzer-config \
// alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField="[Tt]ag|[Kk]ind"`.
//
-// TODO: With some clever heuristics, some pointers should be dereferenced
-// by default. For example, if the pointee is constructed within the
-// constructor call, it's reasonable to say that no external object
-// references it, and we wouldn't generate multiple report on the same
-// pointee.
+// - "IgnoreGuardedFields" (boolean). If set to true, the checker will analyze
+// _syntactically_ whether the found uninitialized object is used without a
+// preceding assert call. Defaults to false.
+//
+// `-analyzer-config \
+// alpha.cplusplus.UninitializedObject:IgnoreGuardedFields=true`.
//
// Most of the following methods as well as the checker itself is defined in
// UninitializedObjectChecker.cpp.
@@ -69,6 +76,7 @@
bool ShouldConvertNotesToWarnings = false;
bool CheckPointeeInitialization = false;
std::string IgnoredRecordsWithFieldPattern;
+ bool IgnoreGuardedFields = false;
};
/// A lightweight polymorphic wrapper around FieldRegion *. We'll use this
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
