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@H@@QEAAHXZ" = comdat any
> > >
> > > $"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = comdat any
> > >
> > > @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = linkonce_odr dso_local
> > > global i32 0, comdat, align 4
> > >
> > > ; Function Attrs: noinline nounwind optnone
> > > define weak_odr dso_local i32 @"?Inclass@?$M@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@H@@QEAAHXZ@4HA", align 4
> > > %inc = add nsw i32 %0, 1
> > > store i32 %inc, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA", align
> > > 4
> > > %1 = load i32, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAHXZ"(%class.M* %mi)
> > > %call1 = call i16 @"?Inclass@?$M@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@F@@QEAAFXZ"(%class.M.0*) #1
> > > ```
> > >
> > >
> > > With the check, both functions are dllimported/exported and static local
> > > vars will be treated correctly.
> > > ```
> > > $"??4?$M@H@@QEAAAEAV0@AEBV0@@Z" = comdat any
> > >
> > > $"?Inclass@?$M@H@@QEAAHXZ" = comdat any
> > >
> > > $"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = comdat any
> > >
> > > @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAAEAV0@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@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@H@@QEAAHXZ@4HA", align 4
> > > %inc = add nsw i32 %0, 1
> > > store i32 %inc, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA", align
> > > 4
> > > %1 = load i32, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAHXZ"(%class.M* %mi)
> > > %call1 = call i16 @"?Inclass@?$M@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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
