hans added a comment. >> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o >> lib.so lib.cc >> $ g++ main.cc lib.so >> /tmp/cc557J3i.o: In function `S::bar()': >> main.cc:(.text._ZN1S3barEv[_ZN1S3barEv]+0xd): undefined reference to >> `foo()' >> >> >> So this is a known issue with `-fvisibility-inlines-hidden`. This doesn't >> come up often in practice, but when it does the developer needs to deal with >> it. > > Yeah, that is the reason of few chromium code changes. > https://chromium-review.googlesource.com/c/chromium/src/+/1212379
Ah, thanks! I hadn't seen what the fixes would look like. >> However, the static local problem is much scarier, because that leads to >> run-time bugs: > > Currently this CL doesn't take care of inline function that is not member of > a class. > > `lib.h`: > > #ifndef LIB_H > #define LIB_H > > struct __attribute__((visibility("default"))) S { > int bar() { > static int x = 0; return x++; > } > int baz(); > }; > > #endif > > > `lib.cc`: > > #include "lib.h" > > int S::baz() { > return bar(); > } > > > Then, static local in inline function is treated correctly. I think it's possible to get the same problem with member functions, but that doesn't matter, it's an existing problem so we don't need to solve it, just be aware. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719 + TSK != TSK_ExplicitInstantiationDeclaration && + TSK != TSK_ExplicitInstantiationDefinition) { + if (ClassExported) { ---------------- 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). https://reviews.llvm.org/D51340 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits