[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)

[Fabian Parzefall via cfe-commits]

https://github.com/FPar updated https://github.com/llvm/llvm-project/pull/100785

>From 13f60e1835c2a5876fa873fc5b4087f942a90316 Mon Sep 17 00:00:00 2001
From: Fabian Parzefall <parzefall at meta.com>
Date: Fri, 26 Jul 2024 10:46:22 -0700
Subject: [PATCH 1/2] [clang] Check inline defs when emitting speculative
 vtable

Clang should only emit an available_externally vtable when there are no
unused virtual inline functions. Currently, if such such a function is
declared without inline inside the class, but is defined inline outside
the class, Clang might emit the vtable as available_externally. This
happens because Clang only considers the declarations of vtable entries,
but not the definitions. This patch addresses this by inspecting the
definitions in addition to the declarations.
---
 clang/lib/CodeGen/ItaniumCXXABI.cpp           |  5 +++-
 .../vtable-available-externally.cpp           | 25 +++++++++++++------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 0cde8a192eda08..c9aed292d4e5d1 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -442,7 +442,10 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
          continue;
 
        const CXXMethodDecl *Method = VtableComponent.getFunctionDecl();
-       if (!Method->getCanonicalDecl()->isInlined())
+       const FunctionDecl *FD = Method->getDefinition();
+       const bool IsInlined =
+           Method->getCanonicalDecl()->isInlined() || (FD && FD->isInlined());
+       if (!IsInlined)
          continue;
 
        StringRef Name = CGM.getMangledName(VtableComponent.getGlobalDecl());
diff --git a/clang/test/CodeGenCXX/vtable-available-externally.cpp b/clang/test/CodeGenCXX/vtable-available-externally.cpp
index a57eb39edfe102..ab105260bc75aa 100644
--- a/clang/test/CodeGenCXX/vtable-available-externally.cpp
+++ b/clang/test/CodeGenCXX/vtable-available-externally.cpp
@@ -250,28 +250,39 @@ struct C : A {
   virtual void car();
 };
 
+// Inline definition outside body, so we can't emit vtable available_externally
+// (see previous).
+// CHECK-TEST10-DAG: @_ZTVN6Test101FE = external unnamed_addr constant
+struct F : A {
+  void foo();
+  virtual void cat();         // inline outside body
+};
+inline void F::cat() {}
+
 // no key function, vtable will be generated everywhere it will be used
 // CHECK-TEST10-DAG: @_ZTVN6Test101EE = linkonce_odr unnamed_addr constant
 // CHECK-FORCE-EMIT-DAG: @_ZTVN6Test101EE = linkonce_odr unnamed_addr constant
 
 struct E : A {};
 
-void g(A& a) {
+void h(A& a) {
   a.foo();
   a.bar();
 }
 
-void f() {
+void g() {
   A a;
-  g(a);
+  h(a);
   B b;
-  g(b);
+  h(b);
   C c;
-  g(c);
+  h(c);
   D d;
-  g(d);
+  h(d);
   E e;
-  g(e);
+  h(e);
+  F f;
+  h(f);
 }
 
 }  // Test10

>From 4325b94c21695ed8e17eb2cc32dd399243c9bcb7 Mon Sep 17 00:00:00 2001
From: Fabian Parzefall <parzefall at meta.com>
Date: Wed, 4 Sep 2024 10:10:17 -0700
Subject: [PATCH 2/2] [clang][NFC] Expand comment on speculative vtable
 emissions

This expands the explanation on why we currently cannot emit speculative
vtables, if there are any unused virtual inline functions (that are not
emitted) in the module.
---
 clang/lib/CodeGen/ItaniumCXXABI.cpp | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index c9aed292d4e5d1..fb1eb72d9f3404 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2282,8 +2282,18 @@ bool ItaniumCXXABI::canSpeculativelyEmitVTableAsBaseClass(
   if (CGM.getCodeGenOpts().ForceEmitVTables)
     return true;
 
-  // If we don't have any not emitted inline virtual function then we are safe
-  // to emit an available_externally copy of vtable.
+  // A speculative vtable can only be generated if all virtual inline functions
+  // defined by this class are emitted. The vtable in the final program contains
+  // for each virtual inline function not used in the current TU a function that
+  // is equivalent to the unused function. The function in the actual vtable
+  // does not have to be declared under the same symbol (e.g., a virtual
+  // destructor that can be substituted with its base class's destructor). Since
+  // inline functions are emitted lazily and this emissions does not account for
+  // speculative emission of a vtable, we might generate a speculative vtable
+  // with references to inline functions that are not emitted under that name.
+  // This can lead to problems when devirtualizing a call to such a function,
+  // that result in linking errors. Hence, if there are any unused virtual
+  // inline function, we cannot emit the speculative vtable.
   // FIXME we can still emit a copy of the vtable if we
   // can emit definition of the inline functions.
   if (hasAnyUnusedVirtualInlineFunction(RD))