NoQ updated this revision to Diff 181419.
NoQ marked an inline comment as done.
NoQ added a comment.
Prettify the unittest a bit, especially the `ASTMatcher` part of it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56632/new/
https://reviews.llvm.org/D56632
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/SymbolManager.cpp
test/Analysis/diagnostics/dtors.cpp
test/Analysis/symbol-reaper.cpp
unittests/StaticAnalyzer/CMakeLists.txt
unittests/StaticAnalyzer/SymbolReaperTest.cpp
Index: unittests/StaticAnalyzer/SymbolReaperTest.cpp
===================================================================
--- /dev/null
+++ unittests/StaticAnalyzer/SymbolReaperTest.cpp
@@ -0,0 +1,121 @@
+//===- unittests/StaticAnalyzer/SymbolReaperTest.cpp ----------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+using namespace ast_matchers;
+
+// A re-usable consumer that constructs ExprEngine out of CompilerInvocation.
+// TODO: Actually re-use it when we write our second test.
+class ExprEngineConsumer : public ASTConsumer {
+protected:
+ CompilerInstance &C;
+
+private:
+ // We need to construct all of these in order to construct ExprEngine.
+ CheckerManager ChkMgr;
+ cross_tu::CrossTranslationUnitContext CTU;
+ PathDiagnosticConsumers Consumers;
+ AnalysisManager AMgr;
+ SetOfConstDecls VisitedCallees;
+ FunctionSummariesTy FS;
+
+protected:
+ ExprEngine Eng;
+
+ // Find a declaration in the current AST by name. This has nothing to do
+ // with ExprEngine but turns out to be handy.
+ // TODO: There's probably a better place for it.
+ template <typename T>
+ const T *findDeclByName(const Decl *Where, StringRef Name) {
+ auto Matcher = decl(hasDescendant(namedDecl(hasName(Name)).bind("d")));
+ auto Matches = match(Matcher, *Where, Eng.getContext());
+ assert(Matches.size() == 1 && "Ambiguous name!");
+ for (BoundNodes &M : Matches)
+ return M.getNodeAs<T>("d");
+ llvm_unreachable("Unable to find declaration!");
+ }
+
+public:
+ ExprEngineConsumer(CompilerInstance &C)
+ : C(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C),
+ Consumers(),
+ AMgr(C.getASTContext(), C.getDiagnostics(), Consumers,
+ CreateRegionStoreManager, CreateRangeConstraintManager, &ChkMgr,
+ *C.getAnalyzerOpts()),
+ VisitedCallees(), FS(),
+ Eng(CTU, AMgr, &VisitedCallees, &FS, ExprEngine::Inline_Regular) {}
+};
+
+class SuperRegionLivenessConsumer : public ExprEngineConsumer {
+ void performTest(const Decl *D) {
+ const auto *FD = findDeclByName<FieldDecl>(D, "x");
+ const auto *VD = findDeclByName<VarDecl>(D, "s");
+ assert(FD && VD);
+
+ // The variable must belong to a stack frame,
+ // otherwise SymbolReaper would think it's a global.
+ const StackFrameContext *SFC =
+ Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+ // Create regions for 's' and 's.x'.
+ const VarRegion *VR = Eng.getRegionManager().getVarRegion(VD, SFC);
+ const FieldRegion *FR = Eng.getRegionManager().getFieldRegion(FD, VR);
+
+ // Pass a null location context to the SymbolReaper so that
+ // it was thinking that the variable is dead.
+ SymbolReaper SymReaper((StackFrameContext *)nullptr, (Stmt *)nullptr,
+ Eng.getSymbolManager(), Eng.getStoreManager());
+
+ SymReaper.markLive(FR);
+ EXPECT_TRUE(SymReaper.isLiveRegion(VR));
+ }
+
+public:
+ SuperRegionLivenessConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+ ~SuperRegionLivenessConsumer() override {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ for (auto D: DG)
+ performTest(D);
+ return true;
+ }
+};
+
+class SuperRegionLivenessAction: public ASTFrontendAction {
+public:
+ SuperRegionLivenessAction() {}
+ std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+ StringRef File) override {
+ auto Consumer = llvm::make_unique<SuperRegionLivenessConsumer>(Compiler);
+ return Consumer;
+ }
+};
+
+// Test that marking s.x as live would also make s live.
+TEST(SymbolReaper, SuperRegionLiveness) {
+ EXPECT_TRUE(tooling::runToolOnCode(new SuperRegionLivenessAction,
+ "void foo() { struct S { int x; } s; }"));
+}
+
+} // namespace
+}
+}
Index: unittests/StaticAnalyzer/CMakeLists.txt
===================================================================
--- unittests/StaticAnalyzer/CMakeLists.txt
+++ unittests/StaticAnalyzer/CMakeLists.txt
@@ -5,6 +5,7 @@
add_clang_unittest(StaticAnalysisTests
AnalyzerOptionsTest.cpp
RegisterCustomCheckersTest.cpp
+ SymbolReaperTest.cpp
)
target_link_libraries(StaticAnalysisTests
Index: test/Analysis/symbol-reaper.cpp
===================================================================
--- /dev/null
+++ test/Analysis/symbol-reaper.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnOnDeadSymbol(int);
+
+namespace test_dead_region_with_live_subregion_in_environment {
+int glob;
+
+struct A {
+ int x;
+
+ void foo() {
+ // Ugh, maybe just let clang_analyzer_eval() work within callees already?
+ // The glob variable shouldn't keep our symbol alive because
+ // 'x != 0' is concrete 'true'.
+ glob = (x != 0);
+ }
+};
+
+void test_A(A a) {
+ if (a.x == 0)
+ return;
+
+ clang_analyzer_warnOnDeadSymbol(a.x);
+
+ // What we're testing is that a.x is alive until foo() exits.
+ a.foo(); // no-warning // (i.e., no 'SYMBOL DEAD' yet)
+
+ // Let's see if constraints on a.x were known within foo().
+ clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}
+
+struct B {
+ A a;
+ int y;
+};
+
+A &noop(A &a) {
+ // This function ensures that the 'b' expression within its argument
+ // would be cleaned up before its call, so that only 'b.a' remains
+ // in the Environment.
+ return a;
+}
+
+
+void test_B(B b) {
+ if (b.a.x == 0)
+ return;
+
+ clang_analyzer_warnOnDeadSymbol(b.a.x);
+
+ // What we're testing is that a.x is alive until foo() exits.
+ noop(b.a).foo(); // no-warning // (i.e., no 'SYMBOL DEAD' yet)
+
+ // Let's see if constraints on a.x were known within foo().
+ clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}
+} // namespace test_dead_region_with_live_subregion_in_environment
Index: test/Analysis/diagnostics/dtors.cpp
===================================================================
--- test/Analysis/diagnostics/dtors.cpp
+++ test/Analysis/diagnostics/dtors.cpp
@@ -1,9 +1,11 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus -verify %s
-
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus -analyzer-output=text -verify %s
namespace no_crash_on_delete_dtor {
-// We were crashing when producing diagnostics for this code.
+// We were crashing when producing diagnostics for this code, but not for the
+// report that it currently emits. Instead, Static Analyzer was thinking that
+// p.get()->foo() is a null dereference because it was dropping
+// constraints over x too early and took a different branch next time
+// we call .get().
struct S {
void foo();
~S();
@@ -14,12 +16,15 @@
S *s;
smart_ptr(S *);
S *get() {
- return (x || 0) ? nullptr : s;
+ return (x || 0) ? nullptr : s; // expected-note{{Left side of '||' is false}}
+ // expected-note@-1{{'?' condition is false}}
+ // expected-warning@-2{{Use of memory after it is freed}}
+ // expected-note@-3{{Use of memory after it is freed}}
}
};
void bar(smart_ptr p) {
- delete p.get();
- p.get()->foo();
+ delete p.get(); // expected-note{{Memory is released}}
+ p.get()->foo(); // expected-note{{Calling 'smart_ptr::get'}}
}
} // namespace no_crash_on_delete_dtor
Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -405,7 +405,7 @@
}
void SymbolReaper::markLive(const MemRegion *region) {
- RegionRoots.insert(region);
+ RegionRoots.insert(region->getBaseRegion());
markElementIndicesLive(region);
}
@@ -426,11 +426,11 @@
}
bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
+ MR = MR->getBaseRegion();
+
if (RegionRoots.count(MR))
return true;
- MR = MR->getBaseRegion();
-
if (const auto *SR = dyn_cast<SymbolicRegion>(MR))
return isLive(SR->getSymbol());
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -198,7 +198,9 @@
mgr.getConstraintManagerCreator(), G.getAllocator(),
this),
SymMgr(StateMgr.getSymbolManager()),
- svalBuilder(StateMgr.getSValBuilder()), ObjCNoRet(mgr.getASTContext()),
+ MRMgr(StateMgr.getRegionManager()),
+ svalBuilder(StateMgr.getSValBuilder()),
+ ObjCNoRet(mgr.getASTContext()),
BR(mgr, *this),
VisitedCallees(VisitedCalleesIn), HowToInline(HowToInlineIn) {
unsigned TrimInterval = mgr.options.GraphTrimInterval;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -131,6 +131,9 @@
/// SymMgr - Object that manages the symbol information.
SymbolManager &SymMgr;
+ /// MRMgr - MemRegionManager object that creates memory regions.
+ MemRegionManager &MRMgr;
+
/// svalBuilder - SValBuilder object that creates SVals from expressions.
SValBuilder &svalBuilder;
@@ -180,10 +183,16 @@
AnalysisManager &getAnalysisManager() override { return AMgr; }
+ AnalysisDeclContextManager &getAnalysisDeclContextManager() {
+ return AMgr.getAnalysisDeclContextManager();
+ }
+
CheckerManager &getCheckerManager() const {
return *AMgr.getCheckerManager();
}
+ MemRegionManager &getRegionManager() { return MRMgr; }
+
SValBuilder &getSValBuilder() { return svalBuilder; }
BugReporter &getBugReporter() { return BR; }
@@ -387,7 +396,6 @@
return StateMgr.getBasicVals();
}
- // FIXME: Remove when we migrate over to just using ValueManager.
SymbolManager &getSymbolManager() { return SymMgr; }
const SymbolManager &getSymbolManager() const { return SymMgr; }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
