[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:9552
+    // overwrite linkage of explicit template instantiation
+    // definition/declaration.
+    return GVA_DiscardableODR;
----------------
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > takuto.ikuta wrote:
> > > > hans wrote:
> > > > > Can you give an example for why this is needed?
> > > > Sorry, this change does not need. Removed.
> > > Sorry, this change is necessary still.
> > > 
> > > Without this, definition of inline function in explicit template instantiation declaration is not be emitted, due to GVA_AvailableExternally linkage.
> > > But we stop exporting definition of inline function in explicit template instantiation definition too.
> > > 
> > > So without this, definition of dllimported inline function of explicit template instantiation declaration won't be available.
> > > 
> > Can you provide a code example of why this is needed?
> If we don't change function linkage, following code will be compiled like below with -fno-dllexport-inlines.
> 
> ```
> template<typename>
> class M{
> public:
>   void foo() {}
> };
> 
> template class __declspec(dllexport) M<int>;
> 
> extern template class __declspec(dllimport) M<short>;
> 
> void f (){
>   M<int> mi;
>   mi.foo();
> 
>   M<short> ms;
>   ms.foo();
> }
> ```
> 
> ```
> $"?foo@?$M at H@@QEAAXXZ" = comdat any
> 
> ; Function Attrs: noinline nounwind optnone
> define weak_odr dso_local void @"?foo@?$M at H@@QEAAXXZ"(%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
>   ret void
> }
> 
> ; Function Attrs: noinline nounwind optnone
> define dso_local void @"?f@@YAXXZ"() #0 {
> entry:
>   %mi = alloca %class.M, align 1
>   %ms = alloca %class.M.0, align 1
>   call void @"?foo@?$M at H@@QEAAXXZ"(%class.M* %mi)
>   call void @"?foo@?$M at F@@QEAAXXZ"(%class.M.0* %ms)
>   ret void
> }
> 
> declare dso_local void @"?foo@?$M at F@@QEAAXXZ"(%class.M.0*) #1
> ```
> 
> M<short>::foo() is declared, but inline function is not dllexported (See M<int>::foo() is not dllexported). So this code cannot be linked because definition of M<short>::foo does not exist. If the function is properly inlined, we don't need to have this code. But I'm not sure why the function is not inlined.
Interesting. I wonder how -fvisibility-inlines-hidden handles this...


```
template <typename> struct S {
  void foo() {}
};

template struct S<int>;

void use() {
  S<int> s;
  s.foo();
}

$ g++ -fvisibility-inlines-hidden -c a.cc && objdump -t a.o | grep _ZN1SIiE3fooEv
0000000000000000 l    d  .text._ZN1SIiE3fooEv	0000000000000000 .text._ZN1SIiE3fooEv
0000000000000000  w    F .text._ZN1SIiE3fooEv	000000000000000b _ZN1SIiE3fooEv  <--- Not hidden.
```

(If I comment out the explicit instantiation definition above, foo() is hidden as expected.)

Okay, it seems that for explicit instantiation definitions, -fvisibility-inlines-hidden does not apply.

And thinking more about it, that makes sense.

I don't think we should change the linkage like this though, I think we should just not apply the /Zc:dllexportInlines- for explicit instantiation decls and definitions in checkClassLevelDLLAttribute(). I realize you had code to check for this before, but now we have a good and well understood reason.


https://reviews.llvm.org/D51340