takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1278799, @hans wrote:
> I've been thinking more about your example with static locals in lambda and
> how this works with regular dllexport.
>
> It got me thinking more about the problem of exposing inline functions from a
> library. For example:
>
> `lib.h`:
>
> #ifndef LIB_H
> #define LIB_H
>
> int foo();
>
> struct __declspec(dllimport) S {
> int bar() { return foo(); }
> };
>
> #endif
>
>
>
> `lib.cc`:
>
> #include "lib.h"
>
> int foo() { return 123; }
>
>
> `main.cc`:
>
> #include "lib.h"
>
> int main() {
> S s;
> return s.bar();
> }
>
>
> Here, Clang will not emit the body of `S::bar()`, because it references the
> non-dllimport function `foo()`, and trying to referencing `foo()` outside the
> library would result in a link error. This is what the
> `DLLImportFunctionVisitor` is used for. For the same reason, MSVC will also
> not inline dllimport functions.
>
> Now, with `/Zc:dllexportInlines-`, we no longer mark `S::bar()` dllimport,
> and so we do emit it, causing that link error. The same problem happens with
> `-fvisibility-inlines-hidden`: substitute the `declspec` above for
> `__attribute__((visibility("default")))` above and try it:
>
> $ 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
> However, the static local problem is much scarier, because that leads to
> run-time bugs:
>
> `lib.h`:
>
> #ifndef LIB_H
> #define LIB_H
>
> inline int foo() { static int x = 0; return x++; }
>
> struct __attribute__((visibility("default"))) S {
> int bar() { return foo(); }
> int baz();
> };
>
> #endif
>
>
> `lib.cc`:
>
> #include "lib.h"
>
> int S::baz() { return foo(); }
>
>
> `main.cc`:
>
> #include <stdio.h>
> #include "lib.h"
>
> int main() {
> S s;
> printf("s.bar(): %d\n", s.bar());
> printf("s.baz(): %d\n", s.baz());
> return 0;
> }
>
>
> If we build the program normally, we get the expected output:
>
> $ g++ lib.cc main.cc && ./a.out
> s.bar(): 0
> s.baz(): 1
>
>
> but building as a shared library shows the problem:
>
> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o
> lib.so lib.cc
> $ g++ main.cc lib.so
> $ LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
> s.bar(): 0
> s.baz(): 0
>
>
> Oops.
>
> This does show that it's a pre-existing problem with the model of not
> exporting inline functions though. I don't think we need to solve this
> specifically for Windows, I think we should match what
> `-fvisibility-inlines-hidden` is doing.
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.
$ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o lib.so
lib.cc
$ g++ main.cc lib.so
$ LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
s.bar(): 0
s.baz(): 1
This is the same behavior with `/Zc:dllexportInlines-`.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+ if (ClassExported) {
----------------
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
```
https://reviews.llvm.org/D51340
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
