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; } 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: 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(MR)) return isLive(SR->getSymbol()); 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: 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: 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: unittests/StaticAnalyzer/SymbolReaperTest.cpp =================================================================== --- /dev/null +++ unittests/StaticAnalyzer/SymbolReaperTest.cpp @@ -0,0 +1,119 @@ +//===- 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; + +class SuperRegionLivenessConsumer : public ASTConsumer { + CompilerInstance &C; + + // We need to construct all of these in order to construct ExprEngine. + // TODO: Re-use this when we add more tests? + std::unique_ptr CTU; + std::unique_ptr ChkMgr; + std::unique_ptr Consumers; + std::unique_ptr AMgr; + std::unique_ptr VisitedCallees; + std::unique_ptr FS; + std::unique_ptr Eng; + + void performTest(const Decl *D) { + // Obtain our AST entities. + const FieldDecl *FD = nullptr; + const VarDecl *VD = nullptr; + auto Matcher = functionDecl( + hasBody(compoundStmt( + has(declStmt( + has(varDecl().bind("vd")), + has(recordDecl(has(fieldDecl().bind("fd"))))))))); + auto Matches = match(Matcher, *D, Eng->getContext()); + for (BoundNodes &M: Matches) { + assert(!FD && !VD); + FD = M.getNodeAs("fd"); + VD = M.getNodeAs("vd"); + } + 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 region for 's'. + const VarRegion *VR = Eng->getRegionManager().getVarRegion(VD, SFC); + // Create region for 's.x'. + 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) : C(C) {} + ~SuperRegionLivenessConsumer() override {} + + void Initialize(ASTContext &ACtx) override { + // Construct ExprEngine. + // TODO: Re-use this when we add more tests? + ChkMgr = llvm::make_unique(ACtx, *C.getAnalyzerOpts()); + CTU = llvm::make_unique(C); + Consumers = llvm::make_unique(); + AMgr = llvm::make_unique( + ACtx, C.getDiagnostics(), *Consumers, CreateRegionStoreManager, + CreateRangeConstraintManager, &*ChkMgr, *C.getAnalyzerOpts()); + VisitedCallees = llvm::make_unique(); + FS = llvm::make_unique(); + Eng = llvm::make_unique(*CTU, *AMgr, &*VisitedCallees, &*FS, + ExprEngine::Inline_Regular); + } + + bool HandleTopLevelDecl(DeclGroupRef DG) override { + for (auto D: DG) + performTest(D); + return true; + } +}; + +class SuperRegionLivenessAction: public ASTFrontendAction { +public: + SuperRegionLivenessAction() {} + std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler, + StringRef File) override { + auto Consumer = llvm::make_unique(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 +} +}