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

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+    while (FD && !getDLLAttr(FD) &&
+           !FD->hasAttr<DLLExportStaticLocalAttr>() &&
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11995
+
+        // Function having static local variables should be exported.
+        auto *ExportAttr = cast<InheritableAttr>(NewAttr->clone(getASTContext()));
----------------
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > hans wrote:
> > > > Isn't it enough that the static local is exported, does the function itself need to be exported too?
> > > Adding dllexport only to variable does not export variable when the function is not used.
> > > But dllimport is not necessary for function, removed.
> > Hmm, I wonder if we could fix that instead? That is, export the variable in that case even if the function is not used?
> I see. I'll investigate which code path emits static variables
static local variable seems to be exported in
https://github.com/llvm-project/llvm-project-20170507/blob/f54514c50350743d3d1a3c5aa80cbe4bb449eb71/clang/lib/CodeGen/CGDecl.cpp#L378

But emitting static local var only by bypassing function emission seems difficult.
Let me leave as-is here.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+          TSK != TSK_ExplicitInstantiationDeclaration &&
+          TSK != TSK_ExplicitInstantiationDefinition) {
+        if (ClassExported) {
----------------
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.



https://reviews.llvm.org/D51340