[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
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
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
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))
___
cfe-commits mailing list
cfe-c
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
FPar wrote: @efriedma-quic Thank you for being patient. I do not have write permissions, can you please merge this for me? https://github.com/llvm/llvm-project/pull/100785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
https://github.com/FPar created https://github.com/llvm/llvm-project/pull/100785
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.
>From a94862e02a205deef577a288c9a4521141ddd041 Mon Sep 17 00:00:00 2001
From: Fabian Parzefall
Date: Fri, 26 Jul 2024 10:46:22 -0700
Subject: [PATCH] [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 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
FPar wrote:
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.
```c++
//--- 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;
```
```c++
//--- 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; }
```
```c++
//--- 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() {}
```
https://github.com/llvm/llvm-project/pull/100785
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
FPar wrote: Yes, that sums it up well. I will see to adding a patch clarifying this in the source. Thank you! https://github.com/llvm/llvm-project/pull/100785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
https://github.com/FPar updated https://github.com/llvm/llvm-project/pull/100785
>From 0f43c9933022ec825bfe3ede5c1622900eb86691 Mon Sep 17 00:00:00 2001
From: Fabian Parzefall
Date: Fri, 26 Jul 2024 10:46:22 -0700
Subject: [PATCH] [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 9b5227772125b2..195573e14ffe3a 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
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
Date: Fri, 26 Jul 2024 10:46:22 -0700
Subject: [PATCH] [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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
FPar wrote: @efriedma-quic Have you been able to take another look? https://github.com/llvm/llvm-project/pull/100785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Check inline defs when emitting speculative vtable (PR #100785)
https://github.com/FPar updated https://github.com/llvm/llvm-project/pull/100785
>From 0f43c9933022ec825bfe3ede5c1622900eb86691 Mon Sep 17 00:00:00 2001
From: Fabian Parzefall
Date: Fri, 26 Jul 2024 10:46:22 -0700
Subject: [PATCH] [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 9b5227772125b2..195573e14ffe3a 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
