Re: r281395 - Try harder to not inline dllimport functions referencing non-dllimport functions
r281413 for the constructors. On Tue, Sep 13, 2016 at 2:58 PM, Hans Wennborgwrote: > Good point. Constructors are also a problem, I'll try to fix. > > It's not exactly the same issue, because they do show up in the AST, > but they're referenced with a CXXConstructExpr, and not the > DeclRefExpr which DLLImportFunctionVisitor is looking for. > > There doesn't seem to be any problem with operator= though. > > On Tue, Sep 13, 2016 at 2:36 PM, Nico Weber via cfe-commits > wrote: >> Could other implicit functions (operator=, ctors) have similar issues? >> >> On Tue, Sep 13, 2016 at 5:08 PM, Hans Wennborg via cfe-commits >> wrote: >>> >>> Author: hans >>> Date: Tue Sep 13 16:08:20 2016 >>> New Revision: 281395 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=281395=rev >>> Log: >>> Try harder to not inline dllimport functions referencing non-dllimport >>> functions >>> >>> In r246338, code was added to check for this, but it failed to take into >>> account implicit destructor invocations because those are not reflected >>> in the AST. This adds a separate check for them. >>> >>> Modified: >>> cfe/trunk/lib/CodeGen/CodeGenModule.cpp >>> cfe/trunk/test/CodeGenCXX/dllimport-members.cpp >>> cfe/trunk/test/CodeGenCXX/dllimport.cpp >>> >>> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=281395=281394=281395=diff >>> >>> == >>> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Sep 13 16:08:20 2016 >>> @@ -1740,8 +1740,17 @@ CodeGenModule::isTriviallyRecursive(cons >>>return Walker.Result; >>> } >>> >>> -bool >>> -CodeGenModule::shouldEmitFunction(GlobalDecl GD) { >>> +// Check if T is a class type with a destructor that's not dllimport. >>> +static bool HasNonDllImportDtor(QualType T) { >>> + if (const RecordType *RT = dyn_cast(T)) >>> +if (CXXRecordDecl *RD = dyn_cast(RT->getDecl())) >>> + if (RD->getDestructor() && >>> !RD->getDestructor()->hasAttr()) >>> +return true; >>> + >>> + return false; >>> +} >>> + >>> +bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) { >>>if (getFunctionLinkage(GD) != >>> llvm::Function::AvailableExternallyLinkage) >>> return true; >>>const auto *F = cast(GD.getDecl()); >>> @@ -1754,6 +1763,18 @@ CodeGenModule::shouldEmitFunction(Global >>> Visitor.TraverseFunctionDecl(const_cast (F)); >>> if (!Visitor.SafeToInline) >>>return false; >>> + >>> +if (const CXXDestructorDecl *Dtor = dyn_cast(F)) { >>> + // Implicit destructor invocations aren't captured in the AST, so >>> the >>> + // check above can't see them. Check for them manually here. >>> + for (const Decl *Member : Dtor->getParent()->decls()) >>> +if (isa(Member)) >>> + if (HasNonDllImportDtor(cast(Member)->getType())) >>> +return false; >>> + for (const CXXBaseSpecifier : Dtor->getParent()->bases()) >>> +if (HasNonDllImportDtor(B.getType())) >>> + return false; >>> +} >>>} >>> >>>// PR9614. Avoid cases where the source code is lying to us. An >>> available >>> >>> Modified: cfe/trunk/test/CodeGenCXX/dllimport-members.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport-members.cpp?rev=281395=281394=281395=diff >>> >>> == >>> --- cfe/trunk/test/CodeGenCXX/dllimport-members.cpp (original) >>> +++ cfe/trunk/test/CodeGenCXX/dllimport-members.cpp Tue Sep 13 16:08:20 >>> 2016 >>> @@ -44,7 +44,7 @@ void useSpecials() { >>> } >>> >>> // Used to force non-trivial special members. >>> -struct ForceNonTrivial { >>> +struct __declspec(dllimport) ForceNonTrivial { >>>ForceNonTrivial(); >>>~ForceNonTrivial(); >>>ForceNonTrivial(const ForceNonTrivial&); >>> >>> Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport.cpp?rev=281395=281394=281395=diff >>> >>> == >>> --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original) >>> +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Tue Sep 13 16:08:20 2016 >>> @@ -347,6 +347,13 @@ __declspec(dllimport) inline int *Refere >>> // MO1-DAG: define available_externally dllimport i32* >>> @"\01?ReferencingImportedDelete@@YAPAHXZ" >>> USE(ReferencingImportedNew) >>> USE(ReferencingImportedDelete) >>> +struct ClassWithDtor { ~ClassWithDtor() {} }; >>> +struct __declspec(dllimport) ClassWithNonDllImportField { ClassWithDtor >>> t; }; >>> +struct __declspec(dllimport) ClassWithNonDllImportBase : public >>> ClassWithDtor { }; >>> +USECLASS(ClassWithNonDllImportField); >>>
Re: r281395 - Try harder to not inline dllimport functions referencing non-dllimport functions
Good point. Constructors are also a problem, I'll try to fix. It's not exactly the same issue, because they do show up in the AST, but they're referenced with a CXXConstructExpr, and not the DeclRefExpr which DLLImportFunctionVisitor is looking for. There doesn't seem to be any problem with operator= though. On Tue, Sep 13, 2016 at 2:36 PM, Nico Weber via cfe-commitswrote: > Could other implicit functions (operator=, ctors) have similar issues? > > On Tue, Sep 13, 2016 at 5:08 PM, Hans Wennborg via cfe-commits > wrote: >> >> Author: hans >> Date: Tue Sep 13 16:08:20 2016 >> New Revision: 281395 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=281395=rev >> Log: >> Try harder to not inline dllimport functions referencing non-dllimport >> functions >> >> In r246338, code was added to check for this, but it failed to take into >> account implicit destructor invocations because those are not reflected >> in the AST. This adds a separate check for them. >> >> Modified: >> cfe/trunk/lib/CodeGen/CodeGenModule.cpp >> cfe/trunk/test/CodeGenCXX/dllimport-members.cpp >> cfe/trunk/test/CodeGenCXX/dllimport.cpp >> >> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=281395=281394=281395=diff >> >> == >> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Sep 13 16:08:20 2016 >> @@ -1740,8 +1740,17 @@ CodeGenModule::isTriviallyRecursive(cons >>return Walker.Result; >> } >> >> -bool >> -CodeGenModule::shouldEmitFunction(GlobalDecl GD) { >> +// Check if T is a class type with a destructor that's not dllimport. >> +static bool HasNonDllImportDtor(QualType T) { >> + if (const RecordType *RT = dyn_cast(T)) >> +if (CXXRecordDecl *RD = dyn_cast(RT->getDecl())) >> + if (RD->getDestructor() && >> !RD->getDestructor()->hasAttr()) >> +return true; >> + >> + return false; >> +} >> + >> +bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) { >>if (getFunctionLinkage(GD) != >> llvm::Function::AvailableExternallyLinkage) >> return true; >>const auto *F = cast(GD.getDecl()); >> @@ -1754,6 +1763,18 @@ CodeGenModule::shouldEmitFunction(Global >> Visitor.TraverseFunctionDecl(const_cast (F)); >> if (!Visitor.SafeToInline) >>return false; >> + >> +if (const CXXDestructorDecl *Dtor = dyn_cast(F)) { >> + // Implicit destructor invocations aren't captured in the AST, so >> the >> + // check above can't see them. Check for them manually here. >> + for (const Decl *Member : Dtor->getParent()->decls()) >> +if (isa(Member)) >> + if (HasNonDllImportDtor(cast(Member)->getType())) >> +return false; >> + for (const CXXBaseSpecifier : Dtor->getParent()->bases()) >> +if (HasNonDllImportDtor(B.getType())) >> + return false; >> +} >>} >> >>// PR9614. Avoid cases where the source code is lying to us. An >> available >> >> Modified: cfe/trunk/test/CodeGenCXX/dllimport-members.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport-members.cpp?rev=281395=281394=281395=diff >> >> == >> --- cfe/trunk/test/CodeGenCXX/dllimport-members.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/dllimport-members.cpp Tue Sep 13 16:08:20 >> 2016 >> @@ -44,7 +44,7 @@ void useSpecials() { >> } >> >> // Used to force non-trivial special members. >> -struct ForceNonTrivial { >> +struct __declspec(dllimport) ForceNonTrivial { >>ForceNonTrivial(); >>~ForceNonTrivial(); >>ForceNonTrivial(const ForceNonTrivial&); >> >> Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport.cpp?rev=281395=281394=281395=diff >> >> == >> --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Tue Sep 13 16:08:20 2016 >> @@ -347,6 +347,13 @@ __declspec(dllimport) inline int *Refere >> // MO1-DAG: define available_externally dllimport i32* >> @"\01?ReferencingImportedDelete@@YAPAHXZ" >> USE(ReferencingImportedNew) >> USE(ReferencingImportedDelete) >> +struct ClassWithDtor { ~ClassWithDtor() {} }; >> +struct __declspec(dllimport) ClassWithNonDllImportField { ClassWithDtor >> t; }; >> +struct __declspec(dllimport) ClassWithNonDllImportBase : public >> ClassWithDtor { }; >> +USECLASS(ClassWithNonDllImportField); >> +USECLASS(ClassWithNonDllImportBase); >> +// MO1-DAG: declare dllimport x86_thiscallcc void >> @"\01??1ClassWithNonDllImportBase@@QAE@XZ"(%struct.ClassWithNonDllImportBase*) >> +// MO1-DAG: declare dllimport x86_thiscallcc void >>
Re: r281395 - Try harder to not inline dllimport functions referencing non-dllimport functions
Could other implicit functions (operator=, ctors) have similar issues? On Tue, Sep 13, 2016 at 5:08 PM, Hans Wennborg via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: hans > Date: Tue Sep 13 16:08:20 2016 > New Revision: 281395 > > URL: http://llvm.org/viewvc/llvm-project?rev=281395=rev > Log: > Try harder to not inline dllimport functions referencing non-dllimport > functions > > In r246338, code was added to check for this, but it failed to take into > account implicit destructor invocations because those are not reflected > in the AST. This adds a separate check for them. > > Modified: > cfe/trunk/lib/CodeGen/CodeGenModule.cpp > cfe/trunk/test/CodeGenCXX/dllimport-members.cpp > cfe/trunk/test/CodeGenCXX/dllimport.cpp > > Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CodeGenModule.cpp?rev=281395=281394=281395=diff > > == > --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original) > +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Sep 13 16:08:20 2016 > @@ -1740,8 +1740,17 @@ CodeGenModule::isTriviallyRecursive(cons >return Walker.Result; > } > > -bool > -CodeGenModule::shouldEmitFunction(GlobalDecl GD) { > +// Check if T is a class type with a destructor that's not dllimport. > +static bool HasNonDllImportDtor(QualType T) { > + if (const RecordType *RT = dyn_cast(T)) > +if (CXXRecordDecl *RD = dyn_cast(RT->getDecl())) > + if (RD->getDestructor() && !RD->getDestructor()->hasAttr< > DLLImportAttr>()) > +return true; > + > + return false; > +} > + > +bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) { >if (getFunctionLinkage(GD) != llvm::Function:: > AvailableExternallyLinkage) > return true; >const auto *F = cast(GD.getDecl()); > @@ -1754,6 +1763,18 @@ CodeGenModule::shouldEmitFunction(Global > Visitor.TraverseFunctionDecl(const_cast(F)); > if (!Visitor.SafeToInline) >return false; > + > +if (const CXXDestructorDecl *Dtor = dyn_cast(F)) { > + // Implicit destructor invocations aren't captured in the AST, so > the > + // check above can't see them. Check for them manually here. > + for (const Decl *Member : Dtor->getParent()->decls()) > +if (isa(Member)) > + if (HasNonDllImportDtor(cast(Member)->getType())) > +return false; > + for (const CXXBaseSpecifier : Dtor->getParent()->bases()) > +if (HasNonDllImportDtor(B.getType())) > + return false; > +} >} > >// PR9614. Avoid cases where the source code is lying to us. An > available > > Modified: cfe/trunk/test/CodeGenCXX/dllimport-members.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > CodeGenCXX/dllimport-members.cpp?rev=281395=281394=281395=diff > > == > --- cfe/trunk/test/CodeGenCXX/dllimport-members.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/dllimport-members.cpp Tue Sep 13 16:08:20 > 2016 > @@ -44,7 +44,7 @@ void useSpecials() { > } > > // Used to force non-trivial special members. > -struct ForceNonTrivial { > +struct __declspec(dllimport) ForceNonTrivial { >ForceNonTrivial(); >~ForceNonTrivial(); >ForceNonTrivial(const ForceNonTrivial&); > > Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > CodeGenCXX/dllimport.cpp?rev=281395=281394=281395=diff > > == > --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Tue Sep 13 16:08:20 2016 > @@ -347,6 +347,13 @@ __declspec(dllimport) inline int *Refere > // MO1-DAG: define available_externally dllimport i32* @"\01? > ReferencingImportedDelete@@YAPAHXZ" > USE(ReferencingImportedNew) > USE(ReferencingImportedDelete) > +struct ClassWithDtor { ~ClassWithDtor() {} }; > +struct __declspec(dllimport) ClassWithNonDllImportField { ClassWithDtor > t; }; > +struct __declspec(dllimport) ClassWithNonDllImportBase : public > ClassWithDtor { }; > +USECLASS(ClassWithNonDllImportField); > +USECLASS(ClassWithNonDllImportBase); > +// MO1-DAG: declare dllimport x86_thiscallcc void @"\01?? > 1ClassWithNonDllImportBase@@QAE@XZ"(%struct.ClassWithNonDllImportBase*) > +// MO1-DAG: declare dllimport x86_thiscallcc void @"\01?? > 1ClassWithNonDllImportField@@QAE@XZ"(%struct.ClassWithNonDllImportField*) > > // A dllimport function with a TLS variable must not be > available_externally. > __declspec(dllimport) inline void FunctionWithTLSVar() { static __thread > int x = 42; } > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list
r281395 - Try harder to not inline dllimport functions referencing non-dllimport functions
Author: hans Date: Tue Sep 13 16:08:20 2016 New Revision: 281395 URL: http://llvm.org/viewvc/llvm-project?rev=281395=rev Log: Try harder to not inline dllimport functions referencing non-dllimport functions In r246338, code was added to check for this, but it failed to take into account implicit destructor invocations because those are not reflected in the AST. This adds a separate check for them. Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp cfe/trunk/test/CodeGenCXX/dllimport-members.cpp cfe/trunk/test/CodeGenCXX/dllimport.cpp Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=281395=281394=281395=diff == --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original) +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Sep 13 16:08:20 2016 @@ -1740,8 +1740,17 @@ CodeGenModule::isTriviallyRecursive(cons return Walker.Result; } -bool -CodeGenModule::shouldEmitFunction(GlobalDecl GD) { +// Check if T is a class type with a destructor that's not dllimport. +static bool HasNonDllImportDtor(QualType T) { + if (const RecordType *RT = dyn_cast(T)) +if (CXXRecordDecl *RD = dyn_cast(RT->getDecl())) + if (RD->getDestructor() && !RD->getDestructor()->hasAttr()) +return true; + + return false; +} + +bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) { if (getFunctionLinkage(GD) != llvm::Function::AvailableExternallyLinkage) return true; const auto *F = cast(GD.getDecl()); @@ -1754,6 +1763,18 @@ CodeGenModule::shouldEmitFunction(Global Visitor.TraverseFunctionDecl(const_cast(F)); if (!Visitor.SafeToInline) return false; + +if (const CXXDestructorDecl *Dtor = dyn_cast(F)) { + // Implicit destructor invocations aren't captured in the AST, so the + // check above can't see them. Check for them manually here. + for (const Decl *Member : Dtor->getParent()->decls()) +if (isa(Member)) + if (HasNonDllImportDtor(cast(Member)->getType())) +return false; + for (const CXXBaseSpecifier : Dtor->getParent()->bases()) +if (HasNonDllImportDtor(B.getType())) + return false; +} } // PR9614. Avoid cases where the source code is lying to us. An available Modified: cfe/trunk/test/CodeGenCXX/dllimport-members.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport-members.cpp?rev=281395=281394=281395=diff == --- cfe/trunk/test/CodeGenCXX/dllimport-members.cpp (original) +++ cfe/trunk/test/CodeGenCXX/dllimport-members.cpp Tue Sep 13 16:08:20 2016 @@ -44,7 +44,7 @@ void useSpecials() { } // Used to force non-trivial special members. -struct ForceNonTrivial { +struct __declspec(dllimport) ForceNonTrivial { ForceNonTrivial(); ~ForceNonTrivial(); ForceNonTrivial(const ForceNonTrivial&); Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport.cpp?rev=281395=281394=281395=diff == --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original) +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Tue Sep 13 16:08:20 2016 @@ -347,6 +347,13 @@ __declspec(dllimport) inline int *Refere // MO1-DAG: define available_externally dllimport i32* @"\01?ReferencingImportedDelete@@YAPAHXZ" USE(ReferencingImportedNew) USE(ReferencingImportedDelete) +struct ClassWithDtor { ~ClassWithDtor() {} }; +struct __declspec(dllimport) ClassWithNonDllImportField { ClassWithDtor t; }; +struct __declspec(dllimport) ClassWithNonDllImportBase : public ClassWithDtor { }; +USECLASS(ClassWithNonDllImportField); +USECLASS(ClassWithNonDllImportBase); +// MO1-DAG: declare dllimport x86_thiscallcc void @"\01??1ClassWithNonDllImportBase@@QAE@XZ"(%struct.ClassWithNonDllImportBase*) +// MO1-DAG: declare dllimport x86_thiscallcc void @"\01??1ClassWithNonDllImportField@@QAE@XZ"(%struct.ClassWithNonDllImportField*) // A dllimport function with a TLS variable must not be available_externally. __declspec(dllimport) inline void FunctionWithTLSVar() { static __thread int x = 42; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits