r351499 - [analyzer] Make sure base-region and its sub-regions are either all alive or all dead.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 17 16:08:56 PST 2019
Author: dergachev
Date: Thu Jan 17 16:08:56 2019
New Revision: 351499
URL: http://llvm.org/viewvc/llvm-project?rev=351499&view=rev
Log:
[analyzer] Make sure base-region and its sub-regions are either all alive or all dead.
SymbolReaper now realizes that our liveness analysis isn't sharp enough
to discriminate between liveness of, say, variables and their fields.
Surprisingly, this didn't quite work before: having a variable live only
through Environment (eg., calling a C++ method on a local variable
as the last action ever performed on that variable) would not keep the
region value symbol of a field of that variable alive.
It would have been broken in the opposite direction as well, but both
Environment and RegionStore use the scanReachableSymbols mechanism for finding
live symbols regions within their values, and due to that they accidentally
end up marking the whole chain of super-regions as live when at least one
sub-region is known to be live.
It is now a direct responsibility of SymbolReaper to maintain this invariant,
and a unit test was added in order to make sure it stays that way.
Differential Revision: https://reviews.llvm.org/D56632
rdar://problem/46914108
Added:
cfe/trunk/test/Analysis/symbol-reaper.cpp
cfe/trunk/unittests/StaticAnalyzer/SymbolReaperTest.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
cfe/trunk/test/Analysis/diagnostics/dtors.cpp
cfe/trunk/unittests/StaticAnalyzer/CMakeLists.txt
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=351499&r1=351498&r2=351499&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h Thu Jan 17 16:08:56 2019
@@ -131,6 +131,9 @@ private:
/// 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,6 +183,10 @@ public:
AnalysisManager &getAnalysisManager() override { return AMgr; }
+ AnalysisDeclContextManager &getAnalysisDeclContextManager() {
+ return AMgr.getAnalysisDeclContextManager();
+ }
+
CheckerManager &getCheckerManager() const {
return *AMgr.getCheckerManager();
}
@@ -387,9 +394,9 @@ public:
return StateMgr.getBasicVals();
}
- // FIXME: Remove when we migrate over to just using ValueManager.
SymbolManager &getSymbolManager() { return SymMgr; }
- const SymbolManager &getSymbolManager() const { return SymMgr; }
+ MemRegionManager &getRegionManager() { return MRMgr; }
+
// Functions for external checking of whether we have unfinished work
bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=351499&r1=351498&r2=351499&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Thu Jan 17 16:08:56 2019
@@ -198,7 +198,9 @@ ExprEngine::ExprEngine(cross_tu::CrossTr
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;
Modified: cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp?rev=351499&r1=351498&r2=351499&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp Thu Jan 17 16:08:56 2019
@@ -405,7 +405,7 @@ void SymbolReaper::markLive(SymbolRef sy
}
void SymbolReaper::markLive(const MemRegion *region) {
- RegionRoots.insert(region);
+ RegionRoots.insert(region->getBaseRegion());
markElementIndicesLive(region);
}
@@ -426,11 +426,15 @@ void SymbolReaper::markInUse(SymbolRef s
}
bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
+ // TODO: For now, liveness of a memory region is equivalent to liveness of its
+ // base region. In fact we can do a bit better: say, if a particular FieldDecl
+ // is not used later in the path, we can diagnose a leak of a value within
+ // that field earlier than, say, the variable that contains the field dies.
+ MR = MR->getBaseRegion();
+
if (RegionRoots.count(MR))
return true;
- MR = MR->getBaseRegion();
-
if (const auto *SR = dyn_cast<SymbolicRegion>(MR))
return isLive(SR->getSymbol());
Modified: cfe/trunk/test/Analysis/diagnostics/dtors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/dtors.cpp?rev=351499&r1=351498&r2=351499&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/diagnostics/dtors.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/dtors.cpp Thu Jan 17 16:08:56 2019
@@ -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 @@ struct smart_ptr {
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 at -1{{'?' condition is false}}
+ // expected-warning at -2{{Use of memory after it is freed}}
+ // expected-note at -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
Added: cfe/trunk/test/Analysis/symbol-reaper.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/symbol-reaper.cpp?rev=351499&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/symbol-reaper.cpp (added)
+++ cfe/trunk/test/Analysis/symbol-reaper.cpp Thu Jan 17 16:08:56 2019
@@ -0,0 +1,60 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,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() {
+ // FIXME: 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 at -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 b.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 at -1{{SYMBOL DEAD}}
+}
+} // namespace test_dead_region_with_live_subregion_in_environment
Modified: cfe/trunk/unittests/StaticAnalyzer/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/StaticAnalyzer/CMakeLists.txt?rev=351499&r1=351498&r2=351499&view=diff
==============================================================================
--- cfe/trunk/unittests/StaticAnalyzer/CMakeLists.txt (original)
+++ cfe/trunk/unittests/StaticAnalyzer/CMakeLists.txt Thu Jan 17 16:08:56 2019
@@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(StaticAnalysisTests
AnalyzerOptionsTest.cpp
RegisterCustomCheckersTest.cpp
+ SymbolReaperTest.cpp
)
target_link_libraries(StaticAnalysisTests
Added: cfe/trunk/unittests/StaticAnalyzer/SymbolReaperTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/StaticAnalyzer/SymbolReaperTest.cpp?rev=351499&view=auto
==============================================================================
--- cfe/trunk/unittests/StaticAnalyzer/SymbolReaperTest.cpp (added)
+++ cfe/trunk/unittests/StaticAnalyzer/SymbolReaperTest.cpp Thu Jan 17 16:08:56 2019
@@ -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!");
+ const T *Node = selectFirst<T>("d", Matches);
+ assert(Node && "Name not found!");
+ return Node;
+ }
+
+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 (const 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
+} // namespace ento
+} // namespace clang
More information about the cfe-commits
mailing list