[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl
Takuto Ikuta via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 30 01:58:51 PDT 2018
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 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
```
https://reviews.llvm.org/D51340
More information about the cfe-commits
mailing list