[clang] [clang][analyzer] Don't warn about virtual calls in final class destructors (PR #178654)

[Artem Dergachev via cfe-commits]

================
@@ -123,6 +123,13 @@ void VirtualCallChecker::checkPreCall(const CallEvent &Call,
   if (!ObState)
     return;
 
+  // If the class whose constructor/destructor we're in is marked final,
+  // virtual dispatch is safe because there can be no derived classes.
+  const auto *LCtx = C.getLocationContext();
+  const auto *CurMethod = dyn_cast_or_null<CXXMethodDecl>(LCtx->getDecl());
+  if (CurMethod && CurMethod->getParent()->hasAttr<FinalAttr>())
----------------
haoNoQ wrote:

These are two different functions: `C.getLocationContext()->getDecl()` is the caller, `Call.getDecl()` is the callee.

It may actually be incorrect to check the caller function `C.getLocationContext()->getDecl()` because it's not necessarily our constructor/destructor. Could be an unrelated function invoked on a different object by our constructor/destructor, and that function may also be final in its own class.

Could also be, well, the constructor/destructor of our superclass. And in those constructors/destructors the vtable is actually already/still broken (https://godbolt.org/z/Enfoe95or). So in this situation we still want to emit the warning, and `C.getLocationContext()->getDecl()` would produce correct results.

(Also even if the entire class isn't `final`, the virtual method itself might still be `final`. And we don't actually care whether the constructor/destructor is final, we only care about the method. In this sense checking `C.getLocationContext()->getDecl()` is also somewhat counterproductive.)

And I'm actually not sure whether `Call.getDecl()` (aka `MD`) will show the base class method when we're in a base class constructor/destructor. I think it may be too smart for its own good. (Which may ultimately result in incorrect path simulation too, given that this checker isn't there to stop it. If that's a real problem, we'll ultimately need to address it by tearing down our dynamic type information early enough. Maybe it's already fine idk.) So I'm worried that it may look like it's calling the final override even when it's invoked from a base class constructor.

So I think this part may be incorrect either way, and it definitely needs to be covered by a few more tests to confirm that it's doing the right thing in presence of interprocedural analysis. (Try to call the method from inside the base class constructor/destructor, try to call the method after diving into an unrelated function.)

Given that the patch only reduces the amount of warnings we emit, I'm ok with landing it even with slight incorrectness. (@steakhal's version because it may actually be correct, plus check for the attribute on the method itself.) But looks like there's some interesting follow-up work here. And it's probably a good idea to add those tests anyway, even if we aren't immediately fixing them, just to have a rough idea how much we're missing.

https://github.com/llvm/llvm-project/pull/178654