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

[Dario Domizioli]

Thanks for the review!

Here's another patch with the comments addressed. It has become tiny! :-)

Having changed TryEmitBaseDestructorAsAlias back to the original
implementation, should I file some bug to keep track of the situation for
the future?

Cheers,
    Dario Domizioli
    SN Systems - Sony Computer Entertainment Group






On 19 September 2014 15:52, Rafael Espíndola <rafael.espindola at gmail.com>
wrote:

> On 17 September 2014 10:07, Dario Domizioli <dario.domizioli at gmail.com>
> wrote:
> > 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.
> >
>
> +  const auto *VD = cast<ValueDecl>(D);
>
> If an unconditional cast is to be made, it is probably better to have
> CodeGenModule::setAliasAttributes take a ValueDecl. By why you need
> the cast? You can call hasAttr on D directly.
>
> +    // The dllexport attribute is always emitted for variables but is
> +    // ignored for not defined functions.
>
> What does it mean to dll export a variable declaration? In any case,
> this function is only called on definitions. It should probably do
> something like
>
>   /// NOTE: This should only be called for definitions.
>   void SetCommonAttributes(const Decl *D, llvm::GlobalValue *GV);
>
> and avoid the test.
>
> Another interesting case is
>
> struct A {
>   ~A();
> };
> A::~A() {}
> struct B : public A {
>   ~B();
> };
> B::~B() {}
>
> Compile with "-O1 -disable-llvm-optzns" and you get
>
> @_ZN1BD2Ev = alias bitcast (void (%struct.A*)* @_ZN1AD2Ev to void
> (%struct.B*)*)
>
> but add a  __declspec(dllexport) and we emit a function. It looks like
> the logic for doing so is working around this very bug in a specific
> case! Simply deleting it on top of your patch makes clang produce
>
> @_ZN1BD2Ev = dllexport alias bitcast (void (%struct.A*)* @_ZN1AD2Ev to
> void (%struct.B*)*
>
> Which looks correct.
>
> This last part (alias to a base class destructor) can probably be a
> subsequent patch, but in that case please leave the dead call to
> setAliasAttributes out of the first patch (the one in
> TryEmitBaseDestructorAsAlias).
>
> Thanks,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140919/26e2d787/attachment.html>
-------------- next part --------------
Index: lib/CodeGen/CodeGenModule.cpp
===================================================================
--- lib/CodeGen/CodeGenModule.cpp	(revision 218126)
+++ lib/CodeGen/CodeGenModule.cpp	(working copy)
@@ -786,6 +786,17 @@
     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.
+  if (D->hasAttr<DLLExportAttr>()) {
+    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 218126)
+++ lib/CodeGen/CodeGenModule.h	(working copy)
@@ -1069,6 +1069,12 @@
   /// 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).
+  ///
+  /// NOTE: This should only be called for definitions.
+  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 218126)
+++ 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