[analyzer] Fix false positive in use-after-move checker

[malavikasamak]

Summary

The analyzer issues a warning when the destructor is invoked on the moved-from object explicitly. This is a false positive, as this invoking a destructor on a moved-from object is safe.

Example:
``
A* a = new A;
A b = std::move(*a);
a->~A();
``
The patch addresses this false warning.

[xazax.hun]

Thanks!

This patch looks good to me. While the documentation of CXXDestructorCall will say that it is only used to represent implicit dtor calls, I wonder if the documentation should be made more explicit saying that it will NOT be generated for an explicit dtor call. @NoQ do you think this is OK, or should CXXDestructorCall also be created for explicit calls?

[NoQ]

Aha thanks! Interesting that CallEvent was supposed to be a higher-level abstraction but in this case it attracts unnecessary attention to the syntax whereas the purely syntactic Decl does the job perfectly.

clang/test/Analysis/use-after-move.cpp

917

Technically this code is still provably bad because it demonstrates double destructor invocation (one manual, one automatic) which screws with C++ "object lifecycle" rules. So we most likely want to emit a warning here ultimately, it's just not the responibility of this checker to warn. This might be worth pointing out in comments to the test, to let people know why exactly we don't expect the warning. So that when they see the test fail, they knew whether it's a good thing or a bad thing.

The other examples also have a memory leak (and if you simply add a delete operator, it'll again cause double destruction) which probably also deserves a warning but the corresponding checker is disabled in this test. Might also be worth pointing out.

[NoQ]

@NoQ do you think this is OK, or should CXXDestructorCall also be created for explicit calls?

That's a good question, another example of this problem is CXXAllocatorCall that covers memory allocation with new syntax:

Class *foo = new Class;

but not explicit invocations like

void *foo = Class::operator new(sizeof(Class));

In this other case it's very natural to have CXXAllocatorCall only cover the first case because it has a lot of accessor methods that don't make any sense at all in the second case. In fact, the analysis conducted by the engine to model the first case is completely different from the second case, with all the construction context shenanigans and intermediate hidden variables, so it makes sense for a path-sensitive data structure to discriminate between those cases.

In case of destructors though, maybe it's actually the right call, as destructor invocations aren't all that different from each other.

[malavikasamak]

This update:

1. Fixes the memory leak in the test case.
2. Comments about invoking the destructor twice in the test cases.

[malavikasamak]

Moved the comment to the top of the test.

[NoQ]

LGTM!

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

if (CtorDec && CtorDec->isCopyOrMoveConstructor()) {
       return;
     }
   }
 
   const auto IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
 
-  // Calling a destructor on a moved object is fine.
-  if (isa<CXXDestructorCall>(IC))
-    return;
-
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisRegion)
     return;
 
   // The remaining part is check only for method call on a moved-from object.
   const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
   if (!MethodDecl)
     return;
 
+  // Calling a destructor on a moved object is fine.
+  if (isa<CXXDestructorDecl>(MethodDecl))
+    return;
+
   // We want to investigate the whole object, not only sub-object of a parent
   // class in which the encountered method defined.
   ThisRegion = ThisRegion->getMostDerivedObjectRegion();
 
   if (isStateResetMethod(MethodDecl)) {
     State = removeFromState(State, ThisRegion);
     C.addTransition(State);
     return;

clang/test/Analysis/use-after-move.cpp

void checkMoreLoopZombies4(bool flag) {
   while (flag) {
     Empty e{};
     for (; coin();)
       e; // expected-warning {{expression result unused}}
     Empty f = std::move(e); // no-warning
   }
 }
 
+void checkExplicitDestructorCalls() {
+  {
+     B* b = new B;
+     B a = std::move(*b);
+     b->~B(); // no-warning 
+     delete b;
+  }
+  // The below code segments invoke the destructor twice (explicit and 
+  // implicit). While this is not a desired code behavior, it is 
+  // not the use-after-move checker's responsibility to issue such a warning.
+  {
+    B a, b;
+    new (&a) B(reinterpret_cast<B &&>(b));
+    (&b)->~B(); // no-warning
+  }
+  {
+    B b;
+    B a  = std::move(b);
+    b.~B(); // no-warning 
+  }
+}
+
 struct MoveOnlyWithDestructor {
   MoveOnlyWithDestructor();
   ~MoveOnlyWithDestructor();
   MoveOnlyWithDestructor(const MoveOnlyWithDestructor &m) = delete;
   MoveOnlyWithDestructor(MoveOnlyWithDestructor &&m);
 };
 
 MoveOnlyWithDestructor foo() {