[PATCH] Fix ctor/dtor aliases losing 'dllexport'

[Dario Domizioli]

Hi!
Thanks for the quick reply. I'm attaching a new patch which addresses the
comments.

I have created a new function CodeGenModule::setAliasAttributes() which can
be invoked by the alias creation functions instead of SetCommonAttributes.
The new function includes a call to SetCommonAttributes just like its
already existing sister setNonAliasAttributes() does.

With this patch, the test does cover all the code for the dllexport
transferral, and the only thing it does not cover is whether
TryEmitDefinitionAsAlias calls the new setAliasAttributes(). The MSVC ABI
seems to be generating only one constructor though (at least in the simple
test), so the problem with aliasing is hidden and I can't really cover that
line of code.

Cheers,
    Dario Domizioli
    SN Systems - Sony Computer Entertainment Group





On 16 September 2014 19:09, Reid Kleckner <rnk at google.com> wrote:

> At a high-level, can you factor this out into something like
> CodeGenModule::setAliasAttributes?
>
> +// RUN: %clang -target x86_64-windows-gnu %s -S -emit-llvm -o - |
> FileCheck %s
>
> Don't use the clang driver (%clang) in codegen tests. Use %clang_cc1. This
> means you will need to pass -mconstructor-aliases.
>
> On Tue, Sep 16, 2014 at 10:42 AM, Dario Domizioli <
> dario.domizioli at gmail.com> wrote:
>
>> Hello Clang,
>>
>> I have noticed that when the compiler emits aliases for simple
>> constructors and destructors it does not transfer the 'dllexport' storage
>> class attribute to the alias.
>> Because in the source code there is only "the constructor" and the user
>> cannot differentiate between the C1 and C2 portions of it, if one portion
>> is dllexport'ed then the other must be too. At the moment however C1 is
>> aliased to C2, and the alias does not have 'dllexport'. This can cause link
>> errors. The same is true for the destructor (and its D1 and D2 portions).
>>
>> The attached patch fixed the issue, although I am not 100% sure that my
>> change is in the right place.
>> Rafael's refactor in r217847 means that the same code has to exist in two
>> places: TryEmitDefinitionAsAlias() in CGCXX.cpp and
>> emitConstructorDestructorAlias() in ItaniumCXXABI.cpp.
>> Maybe there's a better place where to put the change...
>>
>> Also, my test assumes that C1 is aliased to C2 (and D1 to D2), and I have
>> checked it's true with the provided triple which exercises the Itanium ABI
>> code, but I cannot seem to get the "x86_64-windows-msvc" triple to create
>> aliases for C1 and D1, so I don't have coverage for the code in
>> TryEmitDefinitionAsAlias.
>> Can anybody provide advice on that? Or should I just fix the issue in the
>> Itanium ABI and worry about the msvc ABI later? It would seem a shame not
>> to fix both cases...
>>
>> Cheers,
>>     Dario Domizioli
>>     SN Systems - Sony Computer Entertainment Group
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140917/75053cfb/attachment.html>
-------------- next part --------------
Index: lib/CodeGen/CGCXX.cpp
===================================================================
--- lib/CodeGen/CGCXX.cpp	(revision 217952)
+++ lib/CodeGen/CGCXX.cpp	(working copy)
@@ -191,7 +191,7 @@
   }
 
   // Finally, set up the alias with its proper name and attributes.
-  SetCommonAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);
+  setAliasAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);
 
   return false;
 }
Index: lib/CodeGen/CodeGenModule.cpp
===================================================================
--- lib/CodeGen/CodeGenModule.cpp	(revision 217952)
+++ lib/CodeGen/CodeGenModule.cpp	(working copy)
@@ -786,6 +786,22 @@
     addUsedGlobal(GV);
 }
 
+void CodeGenModule::setAliasAttributes(const Decl *D,
+                                       llvm::GlobalValue *GV) {
+  SetCommonAttributes(D, GV);
+
+  // Process the dllexport attribute based on whether the original definition
+  // (not necessarily the aliasee) was exported.
+  const auto *VD = cast<ValueDecl>(D);
+  if (VD->hasAttr<DLLExportAttr>()) {
+    const auto *FD = dyn_cast<FunctionDecl>(VD);
+    // The dllexport attribute is always emitted for variables but is
+    // ignored for not defined functions.
+    if (!FD || FD->hasBody())
+      GV->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
+  }
+}
+
 void CodeGenModule::setNonAliasAttributes(const Decl *D,
                                           llvm::GlobalObject *GO) {
   SetCommonAttributes(D, GO);
Index: lib/CodeGen/CodeGenModule.h
===================================================================
--- lib/CodeGen/CodeGenModule.h	(revision 217952)
+++ lib/CodeGen/CodeGenModule.h	(working copy)
@@ -1066,6 +1066,10 @@
   /// NOTE: This should only be called for definitions.
   void SetCommonAttributes(const Decl *D, llvm::GlobalValue *GV);
 
+  /// Set attributes which must be preserved by an alias. This includes common
+  /// attributes (i.e. it includes a call to SetCommonAttributes).
+  void setAliasAttributes(const Decl *D, llvm::GlobalValue *GV);
+
   void addReplacement(StringRef Name, llvm::Constant *C);
 private:
 
Index: lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- lib/CodeGen/ItaniumCXXABI.cpp	(revision 217952)
+++ lib/CodeGen/ItaniumCXXABI.cpp	(working copy)
@@ -3068,7 +3068,7 @@
   }
 
   // Finally, set up the alias with its proper name and attributes.
-  CGM.SetCommonAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);
+  CGM.setAliasAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);
 }
 
 void ItaniumCXXABI::emitCXXStructor(const CXXMethodDecl *MD,
Index: test/CodeGenCXX/dllexport-alias.cpp
===================================================================
--- test/CodeGenCXX/dllexport-alias.cpp	(revision 0)
+++ test/CodeGenCXX/dllexport-alias.cpp	(working copy)
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -mconstructor-aliases %s -S -emit-llvm -o - | FileCheck %s
+
+// This test assumes that the C1 constructor will be aliased to the C2
+// constructor, and the D1 destructor to the D2. It then checks that the aliases
+// are dllexport'ed.
+
+class __declspec(dllexport) A {
+public:
+    A();
+    ~A();
+};
+
+A::A() {}
+
+A::~A() {}
+
+// CHECK: @_ZN1AC1Ev = dllexport alias void (%class.A*)* @_ZN1AC2Ev
+// CHECK: @_ZN1AD1Ev = dllexport alias void (%class.A*)* @_ZN1AD2Ev