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,6 +183,10 @@ AnalysisManager &getAnalysisManager() override { return AMgr; } + AnalysisDeclContextManager &getAnalysisDeclContextManager() { + return AMgr.getAnalysisDeclContextManager(); + } + CheckerManager &getCheckerManager() const { return *AMgr.getCheckerManager(); } @@ -387,9 +394,9 @@ 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(); } 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,15 @@ } 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(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 =================================================================== --- test/Analysis/symbol-reaper.cpp +++ test/Analysis/symbol-reaper.cpp @@ -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@-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@-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 =================================================================== --- unittests/StaticAnalyzer/SymbolReaperTest.cpp +++ 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 + 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("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(D, "x"); + const auto *VD = findDeclByName(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 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 +} // namespace ento +} // namespace clang