From 6c0b2ce1be3b454fc209fe46aa15e1b60c974bbf Mon Sep 17 00:00:00 2001
From: Artem Dergachev <artem.dergachev@gmail.com>
Date: Mon, 3 Dec 2018 22:44:16 +0000
Subject: [PATCH] [analyzer] MoveChecker: NFC: Remove the workaround for the
 "zombie symbols" bug.

The checker had extra code to clean up memory regions that were sticking around
in the checker without ever being cleaned up due to the bug that was fixed in
r347953. Because of that, if a region was moved from, then became dead,
and then reincarnated, there were false positives.

Why regions are even allowed to reincarnate is a separate story. Luckily, this
only happens for local regions that don't produce symbols when loaded from.

No functional change intended. The newly added test demonstrates that even
though no cleanup is necessary upon destructor calls, the early return
cannot be removed. It was not failing before the patch.

Differential Revision: https://reviews.llvm.org/D54372

llvm-svn: 348208
---
 clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp | 51 ++++-------------------
 clang/test/Analysis/use-after-move.cpp            | 12 ++++++
 2 files changed, 19 insertions(+), 44 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
index 33a89f9..f96e41c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -43,7 +43,7 @@ public:
 };
 
 class MoveChecker
-    : public Checker<check::PreCall, check::PostCall, check::EndFunction,
+    : public Checker<check::PreCall, check::PostCall,
                      check::DeadSymbols, check::RegionChanges> {
 public:
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
@@ -217,42 +217,6 @@ ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
   return nullptr;
 }
 
-// Removing the function parameters' MemRegion from the state. This is needed
-// for PODs where the trivial destructor does not even created nor executed.
-void MoveChecker::checkEndFunction(const ReturnStmt *RS,
-                                   CheckerContext &C) const {
-  auto State = C.getState();
-  TrackedRegionMapTy Objects = State->get<TrackedRegionMap>();
-  if (Objects.isEmpty())
-    return;
-
-  auto LC = C.getLocationContext();
-
-  const auto LD = dyn_cast_or_null<FunctionDecl>(LC->getDecl());
-  if (!LD)
-    return;
-  llvm::SmallSet<const MemRegion *, 8> InvalidRegions;
-
-  for (auto Param : LD->parameters()) {
-    auto Type = Param->getType().getTypePtrOrNull();
-    if (!Type)
-      continue;
-    if (!Type->isPointerType() && !Type->isReferenceType()) {
-      InvalidRegions.insert(State->getLValue(Param, LC).getAsRegion());
-    }
-  }
-
-  if (InvalidRegions.empty())
-    return;
-
-  for (const auto &E : State->get<TrackedRegionMap>()) {
-    if (InvalidRegions.count(E.first->getBaseRegion()))
-      State = State->remove<TrackedRegionMap>(E.first);
-  }
-
-  C.addTransition(State);
-}
-
 void MoveChecker::checkPostCall(const CallEvent &Call,
                                 CheckerContext &C) const {
   const auto *AFC = dyn_cast<AnyFunctionCall>(&Call);
@@ -382,20 +346,19 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
   const auto IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
-  // In case of destructor call we do not track the object anymore.
-  const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
-  if (!ThisRegion)
+
+  // Calling a destructor on a moved object is fine.
+  if (isa<CXXDestructorCall>(IC))
     return;
 
-  if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) {
-    State = removeFromState(State, ThisRegion);
-    C.addTransition(State);
+  const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
     return;
-  }
 
   const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
   if (!MethodDecl)
     return;
+
   // Checking assignment operators.
   bool OperatorEq = MethodDecl->isOverloadedOperator() &&
                     MethodDecl->getOverloadedOperator() == OO_Equal;
diff --git a/clang/test/Analysis/use-after-move.cpp b/clang/test/Analysis/use-after-move.cpp
index f39a5ec..d825596 100644
--- a/clang/test/Analysis/use-after-move.cpp
+++ b/clang/test/Analysis/use-after-move.cpp
@@ -714,3 +714,15 @@ void checkLoopZombies() {
     Empty f = std::move(e); // no-warning
   }
 }
+
+struct MoveOnlyWithDestructor {
+  MoveOnlyWithDestructor();
+  ~MoveOnlyWithDestructor();
+  MoveOnlyWithDestructor(const MoveOnlyWithDestructor &m) = delete;
+  MoveOnlyWithDestructor(MoveOnlyWithDestructor &&m);
+};
+
+MoveOnlyWithDestructor foo() {
+  MoveOnlyWithDestructor m;
+  return m;
+}
-- 
2.7.4