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

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta added a comment.

Ping?

This patch reduced obj size largely, and I expect this makes distributed build (like goma) faster by reducing data transferred on network.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5244
+                   false))
+    CmdArgs.push_back("-fvisibility-inlines-hidden");
+
----------------
takuto.ikuta wrote:
> rnk wrote:
> > takuto.ikuta wrote:
> > > hans wrote:
> > > > takuto.ikuta wrote:
> > > > > hans wrote:
> > > > > > Huh, does this actually affect whether functions get dllexported or not?
> > > > > Sorry, what you want to ask?
> > > > > 
> > > > > This will used to not add dllexport attr in L5690 of SemaDeclCXX.cpp.
> > > > > 
> > > > Oops, I didn't see that. I'm glad to see this is looking so simple :-)
> > > > 
> > > > Actually, I don't think we should the same flag name for this, since "hidden" is an ELF concept, not a COFF one, just that we should match the behaviour of the flag.
> > > > 
> > > > Hmm, or do people use -fvisibility-inlines-hidden on MinGW or something? Where does the hidden-dllimport.cpp file come from?
> > > > 
> > > > Also, is it the case that -fvisibility-inlines-hidden just ignores the problem of static local variables? If that's the case we can probably do it too, we just have to be sure, and document it eventually.
> > > > 
> > > I confirmed that -fvisibility-inlines-hidden treats local static var correctly in linux.
> > > So I'm trying to export inline functions if it has local static variables.
> > This sounds like it would be really hard in general, since you can hide static locals almost anywhere:
> > ```
> > struct Foo {
> >   static int foo() {
> >     return ([]() { static int x; return ++x; })();
> >   }
> > };
> > ```
> > Can we reuse the RecursiveASTVisitor @hans added to check if we can emit dllimport inline functions as available_externally definitions? I think it will find exactly the circumstances we are looking for, i.e. export all the things that cannot be emitted inline in other DLLs.
> Actually, StmtVisitor added dll export attribute to local static variable in lambda function. And static variables seems exported.
> 
> But I found other issue in current implementation, some cxx method decls passed to emitting function before local static variable checker adds dll export attributes. In such case local static variables are not exported and link failure happens.
> 
> So let me try to use DLLImportFunctionVisitor in CodeGenModule::shouldEmitFunction for exported function instead of processing/checking dll attribute around SemaDeclCXX.
I think avoiding dll export is better to be done before we reached around CodeGenModule. Also removing dll attribute later made it difficult to pass tests.


https://reviews.llvm.org/D51340