================
@@ -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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
