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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
