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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In https://reviews.llvm.org/D51340#1266312, @takuto.ikuta wrote:

> Hans, I addressed all your comments.
>  How do you think about current implementation?


Just a few questions, but I think it's pretty good.



================
Comment at: clang/include/clang/Basic/Attr.td:2688
+  // This attribute is used internally only when -fno-dllexport-inlines is
+  // passed.
+  let Spellings = [];
----------------
The comment should explain what the attribute does, also the dllimport one below.


================
Comment at: clang/lib/AST/ASTContext.cpp:9552
+    // overwrite linkage of explicit template instantiation
+    // definition/declaration.
+    return GVA_DiscardableODR;
----------------
Can you give an example for why this is needed?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+    while (FD && !getDLLAttr(FD) &&
+           !FD->hasAttr<DLLExportStaticLocalAttr>() &&
----------------
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > hans wrote:
> > > > takuto.ikuta wrote:
> > > > > hans wrote:
> > > > > > Why does this need to be a loop? I don't think FunctionDecl's can be nested?
> > > > > This is for static local var in lambda function.
> > > > > static_x's ParentFunction does not become f().
> > > > > ```
> > > > > class __declspec(dllexport) C {
> > > > >   int f() {
> > > > >     return ([]() { static int static_x; return ++static_x; })();
> > > > >   }
> > > > > };
> > > > > ```
> > > > I don't think the lambda (or its static local) should be exported in this case.
> > > If we don't export static_x, dllimport library cannot find static_x when linking?
> > > 
> > > 
> > I believe even if C is marked dllimport, the lambda will not be dllimport. The inline definition will be used.
> Do you say importing/implementation library should have different instance of static_x here?
> Current clang does not generate such obj.
> 
> I think static_x should be dllimported. But without this loop, static_x cannot find FD of C::f() having DLLImport/ExportStaticLocalAttr and static_x won't be treated as imported/exported variable.
> 
> And if static_x is not exported, importing library and implementation library will not have the same instance of static_x when C::f() is inlined.
Look at what MSVC does:

```
>type a.cc
struct __declspec(dllexport) S {
  int f() {
    static int y;
    return ([]() { static int static_x; return ++static_x; })() + y;
  }
};

void use(S *s) { s->f(); }

>cl -c a.cc && dumpbin /directives a.obj
Microsoft (R) C/C++ Optimizing Compiler Version 19.12.25834 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

a.cc
Microsoft (R) COFF/PE Dumper Version 14.12.25834.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file a.obj

File Type: COFF OBJECT

   Linker Directives
   -----------------
   /DEFAULTLIB:LIBCMT
   /DEFAULTLIB:OLDNAMES
   /EXPORT:?f at S@@QAEHXZ
   /EXPORT:??4S@@QAEAAU0 at ABU0@@Z
   /EXPORT:??4S@@QAEAAU0@$$QAU0@@Z
   /EXPORT:?y@?1??f at S@@QAEHXZ at 4HA,DATA

  Summary

           8 .bss
          50 .chks64
          64 .debug$S
          A6 .drectve
          6A .text$mn

```

As you can see S::f and S::f::y is exported, but the lambda and its static local are not.

Oh, but I guess you're asking what if we don't dllexport S::f anymore because we're not dllexporting inline methods anymore... hmm, yes, then I do suppose it needs to be exported so maybe this is right after all.


https://reviews.llvm.org/D51340