[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl
Takuto Ikuta via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 31 02:48:59 PDT 2018
takuto.ikuta added a comment.
Thank you for quick fix!
================
Comment at: clang/lib/AST/ASTContext.cpp:9552
+ // overwrite linkage of explicit template instantiation
+ // definition/declaration.
+ return GVA_DiscardableODR;
----------------
takuto.ikuta wrote:
> hans wrote:
> > Can you give an example for why this is needed?
> Sorry, this change does not need. Removed.
Sorry, this change is necessary still.
Without this, definition of inline function in explicit template instantiation declaration is not be emitted, due to GVA_AvailableExternally linkage.
But we stop exporting definition of inline function in explicit template instantiation definition too.
So without this, definition of dllimported inline function of explicit template instantiation declaration won't be available.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+ if (ClassExported) {
----------------
hans wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > hans wrote:
> > > > takuto.ikuta wrote:
> > > > > takuto.ikuta wrote:
> > > > > > takuto.ikuta wrote:
> > > > > > > hans wrote:
> > > > > > > > takuto.ikuta wrote:
> > > > > > > > > hans wrote:
> > > > > > > > > > I still don't understand why we need these checks for template instantiations. Why does it matter whether the functions get inlined or not?
> > > > > > > > > When function of dllimported class is not inlined, such funtion needs to be dllexported from implementation library.
> > > > > > > > >
> > > > > > > > > c.h
> > > > > > > > > ```
> > > > > > > > > template<typename T>
> > > > > > > > > class EXPORT C {
> > > > > > > > > public:
> > > > > > > > > void f() {}
> > > > > > > > > };
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > cuser.cc
> > > > > > > > > ```
> > > > > > > > > #include "c.h"
> > > > > > > > >
> > > > > > > > > void cuser() {
> > > > > > > > > C<int> c;
> > > > > > > > > c.f(); // This function may not be inlined when EXPORT is __declspec(dllimport), so link may fail.
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > When cuser.cc and c.h are built to different library, cuser.cc needs to be able to see dllexported C::f() when C::f() is not inlined.
> > > > > > > > > This is my understanding.
> > > > > > > > Your example doesn't use explicit instantiation definition or declaration, so doesn't apply here I think.
> > > > > > > >
> > > > > > > > As long as the dllexport and dllimport attributes matches it's fine. It doesn't matter whether the function gets inlined or not, the only thing that matters is that if it's marked dllimport on the "consumer side", it must be dllexport when building the dll.
> > > > > > > Hmm, I changed the linkage for functions having DLLExport/ImportStaticLocal Attr.
> > > > > > >
> > > > > > I changed linkage in ASTContext so that inline function is emitted when it is necessary when we use fno-dllexport-inlines.
> > > > > I revived explicit template instantiation check.
> > > > > Found that this is necessary.
> > > > >
> > > > > For explicit template instantiation, inheriting dll attribute from function for local static var is run before inheriting dll attribute from class for member functions. So I cannot detect local static var of inline function after class level dll attribute processing for explicit template instantiation.
> > > > >
> > > > Oh I see, it's a static local problem..
> > > > Can you provide a concrete example that does not work without your check?
> > > > Maybe this is the right thing to do, but I would like to understand exactly what the problem is.
> > > For the following code
> > > ```
> > > template<typename T>
> > > class M{
> > > public:
> > >
> > > T Inclass() {
> > > static T static_x;
> > > ++static_x;
> > > return static_x;
> > > }
> > > };
> > >
> > > template class __declspec(dllexport) M<int>;
> > >
> > > extern template class __declspec(dllimport) M<short>;
> > >
> > > int f (){
> > > M<int> mi;
> > > M<short> ms;
> > > return mi.Inclass() + ms.Inclass();
> > > }
> > >
> > > ```
> > >
> > > llvm code without instantiation check become like below. Both inline functions of M<int> and M<short> is not dllimported/exported.
> > > ```
> > > $"?Inclass@?$M at H@@QEAAHXZ" = comdat any
> > >
> > > $"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA" = comdat any
> > >
> > > @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA" = linkonce_odr dso_local global i32 0, comdat, align 4
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define weak_odr dso_local i32 @"?Inclass@?$M at H@@QEAAHXZ"(%class.M* %this) #0 comdat align 2 {
> > > entry:
> > > %this.addr = alloca %class.M*, align 8
> > > store %class.M* %this, %class.M** %this.addr, align 8
> > > %this1 = load %class.M*, %class.M** %this.addr, align 8
> > > %0 = load i32, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > %inc = add nsw i32 %0, 1
> > > store i32 %inc, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > %1 = load i32, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > ret i32 %1
> > > }
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define dso_local i32 @"?f@@YAHXZ"() #0 {
> > > entry:
> > > %mi = alloca %class.M, align 1
> > > %ms = alloca %class.M.0, align 1
> > > %call = call i32 @"?Inclass@?$M at H@@QEAAHXZ"(%class.M* %mi)
> > > %call1 = call i16 @"?Inclass@?$M at F@@QEAAFXZ"(%class.M.0* %ms)
> > > %conv = sext i16 %call1 to i32
> > > %add = add nsw i32 %call, %conv
> > > ret i32 %add
> > > }
> > >
> > > declare dso_local i16 @"?Inclass@?$M at F@@QEAAFXZ"(%class.M.0*) #1
> > > ```
> > >
> > >
> > > With the check, both functions are dllimported/exported and static local vars will be treated correctly.
> > > ```
> > > $"??4?$M at H@@QEAAAEAV0 at AEBV0@@Z" = comdat any
> > >
> > > $"?Inclass@?$M at H@@QEAAHXZ" = comdat any
> > >
> > > $"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA" = comdat any
> > >
> > > @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA" = linkonce_odr dso_local global i32 0, comdat, align 4
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define weak_odr dso_local dllexport dereferenceable(1) %class.M* @"??4?$M at H@@QEAAAEAV0 at AEBV0@@Z"(%class.M* %this, %class.M* dereferenceable(1)) #0 comdat align 2 {
> > > entry:
> > > %.addr = alloca %class.M*, align 8
> > > %this.addr = alloca %class.M*, align 8
> > > store %class.M* %0, %class.M** %.addr, align 8
> > > store %class.M* %this, %class.M** %this.addr, align 8
> > > %this1 = load %class.M*, %class.M** %this.addr, align 8
> > > ret %class.M* %this1
> > > }
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define weak_odr dso_local dllexport i32 @"?Inclass@?$M at H@@QEAAHXZ"(%class.M* %this) #0 comdat align 2 {
> > > entry:
> > > %this.addr = alloca %class.M*, align 8
> > > store %class.M* %this, %class.M** %this.addr, align 8
> > > %this1 = load %class.M*, %class.M** %this.addr, align 8
> > > %0 = load i32, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > %inc = add nsw i32 %0, 1
> > > store i32 %inc, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > %1 = load i32, i32* @"?static_x@?2??Inclass@?$M at H@@QEAAHXZ at 4HA", align 4
> > > ret i32 %1
> > > }
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define dso_local i32 @"?f@@YAHXZ"() #0 {
> > > entry:
> > > %mi = alloca %class.M, align 1
> > > %ms = alloca %class.M.0, align 1
> > > %call = call i32 @"?Inclass@?$M at H@@QEAAHXZ"(%class.M* %mi)
> > > %call1 = call i16 @"?Inclass@?$M at F@@QEAAFXZ"(%class.M.0* %ms)
> > > %conv = sext i16 %call1 to i32
> > > %add = add nsw i32 %call, %conv
> > > ret i32 %add
> > > }
> > >
> > > declare dllimport i16 @"?Inclass@?$M at F@@QEAAFXZ"(%class.M.0*) #1
> > > ```
> > Thanks! This seems like a problem with the current code though, and hopefully we can fix it instead of working around it in your patch.
> >
> > A shorter version of your example:
> >
> > ```
> > template <typename T> struct S {
> > int foo() { static int x; return x++; }
> > };
> >
> > template struct __declspec(dllexport) S<int>;
> >
> > int f() {
> > S<int> a;
> > return a.foo();
> > }
> > ```
> >
> > Clang will dllexport `S<int>::foo()`, but not the static local. That's a bug.
> >
> > I'll see if I can fix (tracking in https://bugs.llvm.org/show_bug.cgi?id=39496).
> The fix for that was committed in r345699. I think we can now remove the checks for instantiation here.
Thanks! I removed this check again.
https://reviews.llvm.org/D51340
More information about the cfe-commits
mailing list