NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, 
rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet, 
mgorny.

This is a follow-up to D56042 <https://reviews.llvm.org/D56042>.

When a memory region is live, all its sub-regions and super-regions are 
automatically live (and vice versa), so it is only necessary to track liveness 
of base regions. This is exactly how we imagined this to work, but it turns out 
that it didn't.

The reason why it works correctly most of the time because the reachable symbol 
scanner is automatically marks all parent regions as reachable - and therefore 
they are marked as live by adding all of them to region roots every time a 
sub-region is marked as live. However, enumerating all *child* regions this way 
is problematic (there may be infinitely many of them).

In the test from D56042 <https://reviews.llvm.org/D56042>, the symbol for `p.x` 
dies because when `.get()` is called for the last time only `p` is kept alive 
by the Environment, but not `p.x`. Due to that,  `reg_$0<p.x>` is believed to 
be dead - recall that `SymbolRegionValue` is kept alive by its parent region, 
and additionally notice that there are no explicit bindings anywhere else to 
keep it alive (because `SymbolRegionValue` is simply assumed to be there as 
long as the parent region is alive, rather than bound explicitly).

Now, when the `RegionRoots` test fails, `isLiveRegion()` falls through to see 
if the region is "lexically" alive. Here it correctly jumps to the base region 
`p` and looks if live variables analysis thinks it's alive. It doesn't! Because 
there's no need to compute expression 'p' anymore anywhere in the program.

What `isLiveRegion()` should have done is look up the base region in 
`RegionRoots`. But it doesn't. Hence the patch.

The newly added `test_A` demonstrates the problem even more clearly: having the 
symbol for `a.x` die before the call to `a.foo()` is definitely incorrect.

The other test, `test_B`, is an attempt to figure out whether the problem is 
also there "in the opposite direction". That is, when `b.a` is marked live, is 
`b` marked live automatically? Otherwise the lookup in `RegionRoots` would 
still fail.

The answer is yes, it does work correctly, because `scanReachableSymbols` 
always scans the whole super-region chain of every region. Which means that 
every time the Environment or the Store marks a region as live, all of its 
super-regions are added to `RegionRoots`. However, i would still like to add 
conversion to the base region into `markLive()`, because this behavior is 
something that should be guaranteed by `SymbolReaper` rather implied by callers 
manually, even if the callers have to do that anyway.

So for now the change in `markLive()` does not affect functionality at all, but 
it will be important when checkers use the `checkLiveSymbols()` functionality 
more aggressively. Additionally it slightly decreases the size of the 
`RegionRoots` map for faster lookups but adds an extra time overhead for 
marking something as live (need to ascend to the base region). I didn't try to 
figure out whether it gives a net gain in performance.

For that reason the unit test as well. Also a few convenient getters were added 
to `ExprEngine` in order to make the test more concise.


Repository:
  rC Clang

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,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<cross_tu::CrossTranslationUnitContext> CTU;
+  std::unique_ptr<CheckerManager> ChkMgr;
+  std::unique_ptr<PathDiagnosticConsumers> Consumers;
+  std::unique_ptr<AnalysisManager> AMgr;
+  std::unique_ptr<SetOfConstDecls> VisitedCallees;
+  std::unique_ptr<FunctionSummariesTy> FS;
+  std::unique_ptr<ExprEngine> 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<FieldDecl>("fd");
+      VD = M.getNodeAs<VarDecl>("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<CheckerManager>(ACtx, *C.getAnalyzerOpts());
+    CTU = llvm::make_unique<cross_tu::CrossTranslationUnitContext>(C);
+    Consumers = llvm::make_unique<PathDiagnosticConsumers>();
+    AMgr = llvm::make_unique<AnalysisManager>(
+        ACtx, C.getDiagnostics(), *Consumers, CreateRegionStoreManager,
+        CreateRangeConstraintManager, &*ChkMgr, *C.getAnalyzerOpts());
+    VisitedCallees = llvm::make_unique<SetOfConstDecls>();
+    FS = llvm::make_unique<FunctionSummariesTy>();
+    Eng = llvm::make_unique<ExprEngine>(*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<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

Reply via email to