[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl
Hans Wennborg via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 30 02:30:36 PDT 2018
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 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).
https://reviews.llvm.org/D51340
More information about the cfe-commits
mailing list