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

[Rafael Espíndola]

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