[clang] Check inline defs when emitting speculative vtable

[pzfl]

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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Fabian Parzefall (FPar)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/100785.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+4-1)
• (modified) clang/test/CodeGenCXX/vtable-available-externally.cpp (+18-7)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index cd76f8406e7b7..c940089c93361 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 a57eb39edfe10..ab105260bc75a 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

[efriedma-quic]

Can you give a self-contained example that shows why this is a problem? (I mean, I can imagine there could be some interaction that causes issues, but I'm not sure what that interaction is, in this context.)

[pzfl]

Yes, certainly!

The example below demonstrates a case where the available_externally vtable mismatches the actual definition, and holds a reference to a symbol that does not exist in the final program. You can see the mismatch by compiling with -O1 -flto=thin. The generated LLVM IR should be identical to when ~D is declared as virtual inline ~D();, and the test above mine https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/vtable-available-externally.cpp#L242 asserts that the vtable should not be emitted in that case.

I cannot demonstrate and end-to-end miscompilation here. We have an interaction that tripped over this, where we insert a reference in a.cpp to that destructor that does not exist based on the available_externally vtable, which results in a linking error.

//--- h.h

struct A {
  virtual void foo() = 0;
  virtual void bar() = 0;

  virtual ~A() = default;
};

struct B {
  virtual ~B() = default;
};

struct C : B, A {
  void bar() override {}
};

struct D : C {
  void foo() override;

  // If the dtor declaration contains the inline specifier as below, clang does
  // not emit the vtable available_externally. Since D is defined inline outside
  // the class, the two declaration should have the exact same effect.
  //
  // virtual inline ~D();
  virtual ~D();
};

// D is defined inline, but not declared inline. Because clang does not check
// for the definition, it misses it when checking for unused virtual inline
// functions.
inline D::~D() = default;

//--- a.cpp

#include "h.h"

// This call instantiates the vtable of D in this TU as available_externally.
// The third and fourth entries in that vtable @_ZTV1D are the destructors
// @_ZN1DD1Ev and @_ZN1DD0Ev. Since the key function `foo` is not defined in
// this TU, the vtable should be external. The destructor is defined as inline
// in this TU, but not used in this TU. The symbol might not exist in the final
// program. The emitted available_externally definition however references these
// inline destructors.
//
// @_ZTV1D = available_externally dso_local unnamed_addr constant { [6 x ptr],
//               [6 x ptr] } { [6 x ptr] [ptr null, ptr @_ZTI1D, ptr @_ZN1DD1Ev,
//                                                                   ----------
//               ptr @_ZN1DD0Ev, ptr @_ZN1C3barEv, ptr @_ZN1D3fooEv], [6 x ptr]
//               [ptr inttoptr (i64 -8 to ptr), ptr @_ZTI1D,
//               ptr @_ZThn8_N1D3fooEv, ptr @_ZThn8_N1C3barEv,
//               ptr @_ZThn8_N1DD1Ev, ptr @_ZThn8_N1DD0Ev] }, align 8
D *e() { return new D; }

//--- b.cpp

#include "h.h"

// Define the key function here to get the actual vtable to be emitted in this
// TU. The compiler implements the complete object destructor of D (@_ZN1DD1Ev)
// through the base object destructor of C (@_ZN1CD2Ev). @_ZN1DD1Ev does not
// exist in this TU, and the available_externally vtable definition of D in
// a.cpp contains a reference to a non-existing symbol.
//
// @_ZTV1D = dso_local unnamed_addr constant { [6 x ptr], [6 x ptr] } {
//               [6 x ptr] [ptr null, ptr @_ZTI1D, ptr @_ZN1CD2Ev,
//                                                     ----------
//               ptr @_ZN1DD0Ev, ptr @_ZN1C3barEv, ptr @_ZN1D3fooEv], [6 x ptr]
//               [ptr inttoptr (i64 -8 to ptr), ptr @_ZTI1D,
//               ptr @_ZThn8_N1D3fooEv, ptr @_ZThn8_N1C3barEv,
//               ptr @_ZThn8_N1DD1Ev, ptr @_ZThn8_N1DD0Ev] }, align 8
void D::foo() {}

[efriedma-quic]

LGTM... but please consider submitting a patch to expand the explanation in ItaniumCXXABI::canSpeculativelyEmitVTableAsBaseClass.

If I'm following correctly, the function in question is, in fact, odr-used in the translation unit in question: constructing an object is an odr-use all virtual members. So we could emit it if we wanted to. But we currently don't because we emit inline functions lazily, and the lazy emission doesn't account for vtables. If we don't emit it, we can't reliably refer to it.

This applies equally to a function where the initial declaration is inline, and a function where the initial declaration is not inline, but a later definition is; this fix makes sure we treat both situations equivalently.

It might be worth trying to sort out this situation at some point; the inability to emit these functions seems like this makes devirtualization significantly weaker.

[pzfl]

Yes, that sums it up well. I will see to adding a patch clarifying this in the source. Thank you!

[pzfl]

@efriedma-quic Thank you for being patient. I do not have write permissions, can you please merge this for me?