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

[Dario Domizioli]

Thanks for all the reviews!
I have addressed the curly brace nit, and I have committed revision 218159.

I will file the PR soon.

Cheers,
    Dario Domizioli
    SN Systems - Sony Computer Entertainment Group




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

> and yes, a bug for the alias to base class destructor would be nice.
>
> On 19 September 2014 12:35, 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/7a5f7cd1/attachment.html>