<div dir="ltr"><div><div><div><div>Thanks for all the reviews!<br>I have addressed the curly brace nit, and I have committed revision 218159.<br><br></div>I will file the PR soon.<br><br></div>Cheers,<br></div>    Dario Domizioli<br></div>    SN Systems - Sony Computer Entertainment Group<br><br><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On 19 September 2014 19:24, 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">and yes, a bug for the alias to base class destructor would be nice.<br>
<span class="im HOEnZb"><br>
On 19 September 2014 12:35, Dario Domizioli <<a href="mailto:dario.domizioli@gmail.com">dario.domizioli@gmail.com</a>> wrote:<br>
</span><div class="HOEnZb"><div class="h5">> Thanks for the review!<br>
><br>
> Here's another patch with the comments addressed. It has become tiny! :-)<br>
><br>
> Having changed TryEmitBaseDestructorAsAlias back to the original<br>
> implementation, should I file some bug to keep track of the situation for<br>
> the future?<br>
><br>
> Cheers,<br>
>     Dario Domizioli<br>
>     SN Systems - Sony Computer Entertainment Group<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On 19 September 2014 15:52, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On 17 September 2014 10:07, Dario Domizioli <<a href="mailto:dario.domizioli@gmail.com">dario.domizioli@gmail.com</a>><br>
>> wrote:<br>
>> > Hi!<br>
>> > Thanks for the quick reply. I'm attaching a new patch which addresses<br>
>> > the<br>
>> > comments.<br>
>> ><br>
>> > I have created a new function CodeGenModule::setAliasAttributes() which<br>
>> > can<br>
>> > be invoked by the alias creation functions instead of<br>
>> > 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<br>
>> > ABI<br>
>> > seems to be generating only one constructor though (at least in the<br>
>> > simple<br>
>> > test), so the problem with aliasing is hidden and I can't really cover<br>
>> > that<br>
>> > line of code.<br>
>> ><br>
>><br>
>> +  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<br>
>> (%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>
><br>
><br>
</div></div></blockquote></div><br></div>