hans added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+ if (ClassExported) {
----------------
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.
https://reviews.llvm.org/D51340
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
