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

[Reid Kleckner]

lgtm, as a follow up we should make EmitAliasDefinition call this to avoid
duplicate code

On Fri, Sep 19, 2014 at 9:35 AM, Dario Domizioli <dario.domizioli at gmail.com>
wrote:

> 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/5ef24d07/attachment.html>