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 <[email protected]> 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 <[email protected]> > wrote: > >> On 17 September 2014 10:07, Dario Domizioli <[email protected]> >> 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 >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
