<div dir="ltr">lgtm, as a follow up we should make EmitAliasDefinition call this to avoid duplicate code</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 19, 2014 at 9:35 AM, Dario Domizioli <span dir="ltr"><<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks for the review!<div><br></div><div>Here's another patch with the comments addressed. It has become tiny! :-)</div><div><br></div><div>Having changed TryEmitBaseDestructorAsAlias back to the original implementation, should I file some bug to keep track of the situation for the future?</div><span class=""><div><br></div><div>Cheers,</div><div>    Dario Domizioli</div><div>    SN Systems - Sony Computer Entertainment Group</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></span><div><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On 19 September 2014 15:52, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 17 September 2014 10:07, Dario Domizioli <<a href="mailto:dario.domizioli@gmail.com" target="_blank">dario.domizioli@gmail.com</a>> wrote:<br>
> Hi!<br>
> Thanks for the quick reply. I'm attaching a new patch which addresses the<br>
> comments.<br>
><br>
> I have created a new function CodeGenModule::setAliasAttributes() which can<br>
> be invoked by the alias creation functions instead of SetCommonAttributes.<br>
> The new function includes a call to SetCommonAttributes just like its<br>
> already existing sister setNonAliasAttributes() does.<br>
><br>
> With this patch, the test does cover all the code for the dllexport<br>
> transferral, and the only thing it does not cover is whether<br>
> TryEmitDefinitionAsAlias calls the new setAliasAttributes(). The MSVC ABI<br>
> seems to be generating only one constructor though (at least in the simple<br>
> test), so the problem with aliasing is hidden and I can't really cover that<br>
> line of code.<br>
><br>
<br>
</span>+  const auto *VD = cast<ValueDecl>(D);<br>
<br>
If an unconditional cast is to be made, it is probably better to have<br>
CodeGenModule::setAliasAttributes take a ValueDecl. By why you need<br>
the cast? You can call hasAttr on D directly.<br>
<br>
+    // The dllexport attribute is always emitted for variables but is<br>
+    // ignored for not defined functions.<br>
<br>
What does it mean to dll export a variable declaration? In any case,<br>
this function is only called on definitions. It should probably do<br>
something like<br>
<br>
  /// NOTE: This should only be called for definitions.<br>
  void SetCommonAttributes(const Decl *D, llvm::GlobalValue *GV);<br>
<br>
and avoid the test.<br>
<br>
Another interesting case is<br>
<br>
struct A {<br>
  ~A();<br>
};<br>
A::~A() {}<br>
struct B : public A {<br>
  ~B();<br>
};<br>
B::~B() {}<br>
<br>
Compile with "-O1 -disable-llvm-optzns" and you get<br>
<br>
@_ZN1BD2Ev = alias bitcast (void (%struct.A*)* @_ZN1AD2Ev to void (%struct.B*)*)<br>
<br>
but add a  __declspec(dllexport) and we emit a function. It looks like<br>
the logic for doing so is working around this very bug in a specific<br>
case! Simply deleting it on top of your patch makes clang produce<br>
<br>
@_ZN1BD2Ev = dllexport alias bitcast (void (%struct.A*)* @_ZN1AD2Ev to<br>
void (%struct.B*)*<br>
<br>
Which looks correct.<br>
<br>
This last part (alias to a base class destructor) can probably be a<br>
subsequent patch, but in that case please leave the dead call to<br>
setAliasAttributes out of the first patch (the one in<br>
TryEmitBaseDestructorAsAlias).<br>
<br>
Thanks,<br>
Rafael<br>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>