[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1761
 
-  if (Method->isVirtual()) {
+  if (Method->isVirtual() && !Method->isConsteval()) {
     if (Method->isPure())
----------------
luken-google wrote:
> shafik wrote:
> > It is not clear to me if this is the core issue or not. Can you explain this a little better.
> Thanks for the feedback!
> 
> Sure, here's what I'm thinking. This code calls ItaniumVTableContext::getMethodVTableIndex(GlobalDecl) on line 1771 below. That method asserts in VTableBuilder.cpp:2281 because it tries to look up a vtable entry for the consteval method and fails to do so. Any consteval methods are prevented from gaining vtable entries by the logic in VTableContextBase::hasVtableSlot(const CXXMethodDecl *MD):
> 
> ```
> bool VTableContextBase::hasVtableSlot(const CXXMethodDecl *MD) {
>   return MD->isVirtual() && !MD->isConsteval();
> }
> ```
> 
> This call is peppered throughout the VTableBuilder code, but the relevant call for our purposes is in ItaniumVTableBuilder::AddMethods() in VTableBuilder.cpp:1492, which causes any consteval method to be skipped when the code is iterating through all virtual member functions and adding them to the vtable.
> 
> My fix mirrors the logic in VTableContextBase::hasVtableSlot here, because the code assumes that if the method is virtual it has a vtable index, which consteval functions don't. Writing this, I've convinced myself it's better to just call that method directly, so I've refactored the code to avoid the duplicate logic.
Thank you, that makes sense. Using the function call is much better, it avoids logic duplication and assures that if the logic is updated this section will stay in sync.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132874/new/

https://reviews.llvm.org/D132874