[PATCH] D34972: [CodeGen] Propagate dllexport to thunks

[Shoaib Meenai via cfe-commits]

I had initially also thought that thunks wouldn't need to be exported, since
you should always have one of two scenarios:

1. The class doesn't have a key function, in which case both its vtable and
   its thunks will just be emitted wherever they're needed.
2. The class has a key function, in which case both its vtable and its thunks
   will be present in the library with the key function, and the thunks are
   only referenced from the vtable.

The situation I ran into which required thunks to be exported was something
like the following (this is from memory, so some details might be a bit off):

* B virtually inherits from A and has a key function, so its vtable and thunks
  are localized to the library with the key function.
* C inherits from B and doesn't have a key function.
* The constructor vtable for B-in-C is emitted in some other library (due to
  C's lack of a key function), and it tries to reference B's thunks, causing
  link failures.

I can add this scenario to the commit message if it makes the motivation
clearer.

It should be noted that the thunks were exported before r298330, which only
changed dllexport behavior, so dllimport behavior for thunks should still be
the same as it was before. I can look into what that behavior is and make it
explicit in this diff as well.

From: David Majnemer <david.majnemer at gmail.com>
Date: Wednesday, July 5, 2017 at 11:15 AM
To: Shoaib Meenai <smeenai at fb.com>, Shoaib Meenai via Phabricator <reviews at reviews.llvm.org>, "compnerd at compnerd.org" <compnerd at compnerd.org>, "reviews+D34972+public+8a22767368a5b974 at reviews.llvm.org" <reviews+D34972+public+8a22767368a5b974 at reviews.llvm.org>, "rnk at google.com" <rnk at google.com>
Cc: "cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>, "zturner at google.com" <zturner at google.com>
Subject: Re: [PATCH] D34972: [CodeGen] Propagate dllexport to thunks

If the thinks are not imported, why would it make sense to export them?

Maybe devirtualization could trigger this?

On Wed, Jul 5, 2017 at 1:17 PM Shoaib Meenai <smeenai at fb.com> wrote:
The thunks are (as far as I know, at least) only referenced from vtables, so
there's no opportunity to perform indirect calls via the IAT for them. The
vtable would just end up referencing the linker-generated import thunk
instead.

I can play around with marking the thunks as dllimport if the function being
thunked is dllimport, but I was curious if you had any specific scenarios in
mind where you thought it would make a difference.

From: cfe-commits <cfe-commits-bounces at lists.llvm.org> on behalf of David Majnemer via cfe-commits <cfe-commits at lists.llvm.org>
Reply-To: David Majnemer <david.majnemer at gmail.com>
Date: Tuesday, July 4, 2017 at 8:42 AM
To: Shoaib Meenai via Phabricator <reviews at reviews.llvm.org>, "compnerd at compnerd.org" <compnerd at compnerd.org>, "reviews+D34972+public+8a22767368a5b974 at reviews.llvm.org" <reviews+D34972+public+8a22767368a5b974 at reviews.llvm.org>, "rnk at google.com" <rnk at google.com>
Cc: "zturner at google.com" <zturner at google.com>, "cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>
Subject: Re: [PATCH] D34972: [CodeGen] Propagate dllexport to thunks

What about the import side?

On Mon, Jul 3, 2017 at 10:37 PM Shoaib Meenai via Phabricator via cfe-commits <cfe-commits at lists.llvm.org> wrote:
smeenai created this revision.

Under Windows Itanium, we need to export virtual and non-virtual thunks
if the functions being thunked are exported. These thunks would
previously inherit their dllexport attribute from the declaration, but
r298330 changed declarations to not have dllexport attributes. We
therefore need to add the dllexport attribute to the definition
ourselves now. This is consistent with MinGW GCC's behavior.

This redoes r306770 but limits the logic to Itanium. MicrosoftCXXABI's
setThunkLinkage ensures that thunks aren't exported under that ABI, so
I'm handling this in ItaniumCXXABI's setThunkLinkage for symmetry.


https://reviews.llvm.org/D34972

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/dllexport-vtable-thunks.cpp


Index: test/CodeGenCXX/dllexport-vtable-thunks.cpp
===================================================================
--- /dev/null
+++ test/CodeGenCXX/dllexport-vtable-thunks.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -fdeclspec -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-itanium -fdeclspec -emit-llvm -o - %s | FileCheck %s
+
+struct __declspec(dllexport) A {
+  virtual void m();
+};
+struct __declspec(dllexport) B {
+  virtual void m();
+};
+struct __declspec(dllexport) C : A, B {
+  virtual void m();
+};
+void C::m() {}
+// CHECK: define dllexport void @_ZThn8_N1C1mEv
+
+struct Base {
+  virtual void m();
+};
+struct __declspec(dllexport) Derived : virtual Base {
+  virtual void m();
+};
+void Derived::m() {}
+// CHECK: define dllexport void @_ZTv0_n24_N7Derived1mEv
Index: lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -284,6 +284,11 @@
     // linkage together with vtables when needed.
     if (ForVTable && !Thunk->hasLocalLinkage())
       Thunk->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+
+    // Propagate dllexport storage.
+    const CXXMethodDecl *MD = cast<CXXMethodDecl>(GD.getDecl());
+    if (MD->hasAttr<DLLExportAttr>())
+      Thunk->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
   }

   llvm::Value *performThisAdjustment(CodeGenFunction &CGF, Address This,


_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits