Add /Zc:DllexportInlines option to clang-cl

[takuto.ikuta]

Summary

This CL adds /Zc:DllexportInlines flag to clang-cl.
When Zc:DllexportInlines- is specified, inline class member function is not exported if the function does not have local static variables.

By not exporting inline function, code for those functions are not generated and that reduces both compile time and obj size. Also this flag does not import inline functions from dllimported class if the function does not have local static variables.

On my 24C48T windows10 machine, build performance of chrome target in chromium repository is like below.
These stats are come with 'target_cpu="x86" enable_nacl = false is_component_build=true dcheck_always_on=true` build config and applied

• https://chromium-review.googlesource.com/c/chromium/src/+/1212379
• https://chromium-review.googlesource.com/c/v8/v8/+/1186017

Below stats were taken with this patch applied on https://github.com/llvm-project/llvm-project-20170507/commit/a05115cd4c57ff76b0f529e38118765b58ed7f2e

config	build time	speedup	build dir size
with patch, PCH on, debug	1h10m0s	x1.13	35.6GB
without patch, PCH on, debug	1h19m17s		49.0GB
with patch, PCH off, debug	1h15m45s	x1.16	33.7GB
without patch, PCH off, debug	1h28m10s		52.3GB
with patch, PCH on, release	1h13m13s	x1.22	26.2GB
without patch, PCH on, release	1h29m57s		37.5GB
with patch, PCH off, release	1h23m38s	x1.32	23.7GB
without patch, PCH off, release	1h50m50s		38.7GB

This patch reduced obj size and the number of exported symbols largely, that improved link time too.
e.g. link time stats of blink_core.dll become like below

	cold disk cache	warm disk cache
with patch, PCH on, debug	71s	30s
without patch, PCH on, debug	111s	48s

This patch's implementation is based on Nico Weber's patch. I modified to support static local variable, added tests and took stats.

Bug: https://bugs.llvm.org/show_bug.cgi?id=33628

[takuto.ikuta]

In D51340#1226989, @takuto.ikuta wrote:

In D51340#1222013, @hans wrote:

Did both your builds use PCH? It'd be interesting to see the difference without PCH too; the effect should be even larger.

Added stats of without PCH build.

The summary should probably reference https://bugs.llvm.org/show_bug.cgi?id=33628 and it needs to mention how it affects dllimport too.

Added to description, thanks!

Okay, after reading through the patch, it seems we're still marking class members dllexport, and then you selectively remove the attribute later. That does feel a little bit backward... Does -fvisibility-inlines-hidden also have the static local problem, or how does that flag handle it?

Ah, maybe I can get performance improvement just support fvisibility-inlines-hidden in clang-cl. Let me try.

I just support fvisibility-inlines-hidden in clang-cl and that looks to work intended.

[hans]

clang/lib/Driver/ToolChains/Clang.cpp

5244

Huh, does this actually affect whether functions get dllexported or not?

[takuto.ikuta]

clang/lib/Driver/ToolChains/Clang.cpp

5244

Sorry, what you want to ask?

This will used to not add dllexport attr in L5690 of SemaDeclCXX.cpp.

[hans]

clang/lib/Driver/ToolChains/Clang.cpp

5244

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.

[takuto.ikuta]

I'm trying to handle local static var correctly.

[takuto.ikuta]

clang/lib/Driver/ToolChains/Clang.cpp

5244

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.

[rnk]

clang/lib/Driver/ToolChains/Clang.cpp

5244

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.

[takuto.ikuta]

clang/lib/Driver/ToolChains/Clang.cpp

5244

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.

[takuto.ikuta]

Current implementation cannot build chrome when pch is enabled.
undefined symbol error happens during linking blink_modules.dll

[takuto.ikuta]

PTAL again.

I confirmed that current patch can link chrome and functions with local static variable are exported.

But current ToT clang was not improved well by this patch.
I guess there is some change recently making effect of this patch smaller. Or chromium has many inline functions with static local variable?

[takuto.ikuta]

Remove unnecessary willHaveBody check condition

[takuto.ikuta]

Ping?

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

clang/lib/Driver/ToolChains/Clang.cpp

5244

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.

[hans]

In D51340#1243331, @takuto.ikuta wrote:

Ping?

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

I'll try to look at it this week.

Have you confirmed on some Chromium file, e.g. stroke_opacity_custom.cc (go/stroke-opactity-custom) that the number of functions that we codegen decreases as expected? I'd expect this to save a lot of compile time.

[takuto.ikuta]

In D51340#1243453, @hans wrote:

In D51340#1243331, @takuto.ikuta wrote:

Ping?

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

I'll try to look at it this week.

Have you confirmed on some Chromium file, e.g. stroke_opacity_custom.cc (go/stroke-opactity-custom) that the number of functions that we codegen decreases as expected? I'd expect this to save a lot of compile time.

Yes, stroke_opacity_custom.obj export 18 symbols, reduced from 781 in PCH build, compile time is reduced from 8.2 seconds to 6.7 seconds.

[hans]

The static local stuff still makes me nervous. How much does Chromium break if we just ignore the problem? And how does -fvisibility-inlines-hidden handle the issue?

Also, Sema already has a check for static locals in C inline functions:

$ echo "inline void f() { static int x; }" | bin/clang -c -x c -
<stdin>:1:19: warning: non-constant static local variable in inline function may be different in different files [-Wstatic-local-in-inline]
inline void f() { static int x; }
                  ^

could we reuse that check somehow for this case with static locals in dllexport inline methods?

clang/lib/Sema/SemaDecl.cpp

12624 ↗

(On Diff #166450)

Hmm, now we're adding an AST walk over all inline methods which probably slows us down a bit. Not sure I have any better ideas though.

In any case, ActOnFinishInlineFunctionDef needs a comment explaining why it's doing this.

clang/lib/Sema/SemaDeclCXX.cpp

5743

What worries me is that now we have logic in two different places about inheriting the dll attribute.

The new place in ActOnFinishInlineFunctionDef doesn't have the same checks (checking for MS ABI and the template specialization kind) which means the logic for when to inherit is already a little out of sync...

[takuto.ikuta]

Extract inline function export check to function

[takuto.ikuta]

Update comment for Sema::ActOnFinishInlineFunctionDef

[takuto.ikuta]

Thank you for review!

In D51340#1246508, @hans wrote:

The static local stuff still makes me nervous. How much does Chromium break if we just ignore the problem? And how does -fvisibility-inlines-hidden handle the issue?

I'm not sure how much chromium will break, if we ignore static local variables. But ignoring static local var may cause some unhappy behavior and experience to developer.
I'd like to have check for local static variables as -fvisibility-inlines-hidden does.

-fvisibility-inlines-hidden checks static local around here.
https://github.com/llvm-project/llvm-project-20170507/blob/77b7ca42b0ea496373c5e5a9710b1fe11e1fba68/clang/lib/AST/Decl.cpp#L1265

Also, Sema already has a check for static locals in C inline functions:

$ echo "inline void f() { static int x; }" | bin/clang -c -x c -
<stdin>:1:19: warning: non-constant static local variable in inline function may be different in different files [-Wstatic-local-in-inline]
inline void f() { static int x; }
                  ^

could we reuse that check somehow for this case with static locals in dllexport inline methods?

I think we can do the same thing when we are given -fno-dllexport-inlines around here.
https://github.com/llvm-project/llvm-project-20170507/blob/77b7ca42b0ea496373c5e5a9710b1fe11e1fba68/clang/lib/Sema/SemaDecl.cpp#L6661

But I bit prefer to do export functions with static local variables. Because that is consistent with -fvisibility-inlines-hidden and we won't need more changes to chromium than the CLs in description.

clang/lib/Sema/SemaDeclCXX.cpp

5743

Agree. Do you allow me to extract check to function and re-use that?

[takuto.ikuta]

clang/lib/Sema/SemaDecl.cpp

12624 ↗

(On Diff #166450)

Added comment. I think typical code does not have static variables in inline function and this check is worth to be done for the performance improvement.

[takuto.ikuta]

Ping?

[hans]

In D51340#1254898, @takuto.ikuta wrote:

Ping?

Sorry, I know I'm being slow to look at this :-( I have not forgotten, it's just a lot of other things happening at the same time.

[takuto.ikuta]

In D51340#1256299, @hans wrote:

In D51340#1254898, @takuto.ikuta wrote:

Ping?

Sorry, I know I'm being slow to look at this :-( I have not forgotten, it's just a lot of other things happening at the same time.

I see. Sorry for rushing you. I wait until you have time.

[hans]

clang/lib/Sema/SemaDeclCXX.cpp

5640

Okay, breaking out this logic is a little better, but I still don't like that we now have split the "inherit dllexport attribute" in two places: one for non-inline functions and one for inline (even if they both call this function). It feels like it will be hard to maintain.

Here is another idea:

When we inherit the dllexport attribute to class members, if getLangOpts().DllExportInlines is false, we don't put dllexport on inline functions, but instead we put a new attribute "dllexportstaticlocals".

That attribute only has the effect that it makes static locals exported. We would check for it when computing the linkage of the static local, similar to how it works in hidden functions.

This has two benefits: it doesn't complicate the "inherit dllexport" code very much, and it avoids the need for a separate AST walk of the function.

What do you think?

[takuto.ikuta]

address comment

[takuto.ikuta]

Thank you for review!

clang/lib/Sema/SemaDeclCXX.cpp

5640

I implemented your idea the way I understood.
Use new attribute to check static local var later.

Due to difference of treating between linkage and dll attribute, I inherit dll attribute in Sema/SemaDecl.cpp instead of AST/Decl.cpp

[hans]

Thanks! I think this is getting close now.

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

Why does this need to be a loop? I don't think FunctionDecl's can be nested?

11995 ↗

(On Diff #168979)

Isn't it enough that the static local is exported, does the function itself need to be exported too?

clang/lib/Sema/SemaDeclCXX.cpp

5770

I don't think checking for Microsoft ABI is necessary, just checking the DllExportInlines flag should be enough.

5772

But I don't understand why the template stuff is checked here...

The way I imagined this, we'd only need to change the code when creating NewAttr below..
Something like

NewAttr = ClassAtr->clone()...
if (!getLandOpts().DllExportInlines() && Member is an inline method)
  NewAttr = our new dllimport/export static locals attribute

What do you think?

[takuto.ikuta]

address comment

[takuto.ikuta]

Thank you for review! I updated the code.

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

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; })();
  }
};

11995 ↗

(On Diff #168979)

Adding dllexport only to variable does not export variable when the function is not used.
But dllimport is not necessary for function, removed.

clang/lib/Sema/SemaDeclCXX.cpp

5772

But I don't understand why the template stuff is checked here...

Templated inline function is not always inlined, it seems depending on optimization level.

I updated the code as you wrote in later part of comment.

[hans]

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

I don't think the lambda (or its static local) should be exported in this case.

11995 ↗

(On Diff #168979)

Hmm, I wonder if we could fix that instead? That is, export the variable in that case even if the function is not used?

clang/lib/Sema/SemaDeclCXX.cpp

5788

I still don't understand why we need these checks for template instantiations. Why does it matter whether the functions get inlined or not?

[takuto.ikuta]

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

If we don't export static_x, dllimport library cannot find static_x when linking?

11995 ↗

(On Diff #168979)

I see. I'll investigate which code path emits static variables

clang/lib/Sema/SemaDeclCXX.cpp

5788

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.

[hans]

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

I believe even if C is marked dllimport, the lambda will not be dllimport. The inline definition will be used.

clang/lib/Sema/SemaDeclCXX.cpp

5788

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.

[takuto.ikuta]

Address comment

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

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.

11995 ↗

(On Diff #168979)

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.

clang/lib/Sema/SemaDeclCXX.cpp

5788

Hmm, I changed the linkage for functions having DLLExport/ImportStaticLocal Attr.

[takuto.ikuta]

Export function inside explicit template instantiation definition

[takuto.ikuta]

Remove unnecessary attr creation

[takuto.ikuta]

Fix linkage for inline function of explicit template instantiation declaration

[takuto.ikuta]

remove comment out code

[takuto.ikuta]

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

clang/lib/Sema/SemaDeclCXX.cpp

5788

I changed linkage in ASTContext so that inline function is emitted when it is necessary when we use fno-dllexport-inlines.

[takuto.ikuta]

ping? Can I go forward in this way?

[hans]

In 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.

clang/include/clang/Basic/Attr.td

2688 ↗

(On Diff #169792)

The comment should explain what the attribute does, also the dllimport one below.

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

Can you give an example for why this is needed?

clang/lib/Sema/SemaDecl.cpp

11976 ↗

(On Diff #168979)

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@S@@QAEHXZ
   /EXPORT:??4S@@QAEAAU0@ABU0@@Z
   /EXPORT:??4S@@QAEAAU0@$$QAU0@@Z
   /EXPORT:?y@?1??f@S@@QAEHXZ@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.

[takuto.ikuta]

Added explanation comment for added attributes and rebased

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

Sorry, this change does not need. Removed.

clang/lib/Sema/SemaDeclCXX.cpp

5788

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.

[hans]

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.

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.

And what about the lambdas?

lib.h:

#ifndef LIB_H
#define LIB_H

struct __attribute__((visibility("default"))) S {
  int bar() { return ([]() { static int x; return x++; })(); }
  int baz();
};

#endif

lib.cc:

#include "lib.h"

int S::baz() { return bar(); }

$ 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

Interesting, the lambda function and its static local are not hidden.

Either that's because they're applying the "static locals of hidden functions in visible decl contexts are visible" thing. Or alternatively, they never applied the hidden visibility to the lambda in the first place.

I'm not entirely sure what this means for the dllexport case though. Maybe the loop in the current patch is fine.

[hans]

clang/lib/Sema/SemaDeclCXX.cpp

5788

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.

[takuto.ikuta]

In 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-.

clang/lib/Sema/SemaDeclCXX.cpp

5788

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@H@@QEAAHXZ" = comdat any

$"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = comdat any

@"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = linkonce_odr dso_local global i32 0, comdat, align 4

; Function Attrs: noinline nounwind optnone
define weak_odr dso_local i32 @"?Inclass@?$M@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@H@@QEAAHXZ@4HA", align 4
  %inc = add nsw i32 %0, 1
  store i32 %inc, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA", align 4
  %1 = load i32, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAHXZ"(%class.M* %mi)
  %call1 = call i16 @"?Inclass@?$M@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@F@@QEAAFXZ"(%class.M.0*) #1

With the check, both functions are dllimported/exported and static local vars will be treated correctly.

$"??4?$M@H@@QEAAAEAV0@AEBV0@@Z" = comdat any

$"?Inclass@?$M@H@@QEAAHXZ" = comdat any

$"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = comdat any

@"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAAEAV0@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@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@H@@QEAAHXZ@4HA", align 4
  %inc = add nsw i32 %0, 1
  store i32 %inc, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA", align 4
  %1 = load i32, i32* @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@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@H@@QEAAHXZ"(%class.M* %mi)
  %call1 = call i16 @"?Inclass@?$M@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@F@@QEAAFXZ"(%class.M.0*) #1

[hans]

$ 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

Ah, thanks! I hadn't seen what the fixes would look like.

However, the static local problem is much scarier, because that leads to run-time bugs:

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.

I think it's possible to get the same problem with member functions, but that doesn't matter, it's an existing problem so we don't need to solve it, just be aware.

clang/lib/Sema/SemaDeclCXX.cpp

5788

Thanks! This seems like a problem with the current code though, and hopefully we can fix it instead of working around it in your patch.

A shorter version of your example:

template <typename T> struct S {
  int foo() { static int x; return x++; }
};

template struct __declspec(dllexport) S<int>;

int f() {
  S<int> a;
  return a.foo();
}

Clang will dllexport S<int>::foo(), but not the static local. That's a bug.

I'll see if I can fix (tracking in https://bugs.llvm.org/show_bug.cgi?id=39496).

[hans]

clang/lib/Sema/SemaDeclCXX.cpp

5788

The fix for that was committed in r345699. I think we can now remove the checks for instantiation here.

[takuto.ikuta]

Rebased to take r345699

[takuto.ikuta]

Thank you for quick fix!

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

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.

clang/lib/Sema/SemaDeclCXX.cpp

5788

Thanks! I removed this check again.

[hans]

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

Can you provide a code example of why this is needed?

[takuto.ikuta]

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

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@H@@QEAAXXZ" = comdat any

; Function Attrs: noinline nounwind optnone
define weak_odr dso_local void @"?foo@?$M@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@H@@QEAAXXZ"(%class.M* %mi)
  call void @"?foo@?$M@F@@QEAAXXZ"(%class.M.0* %ms)
  ret void
}

declare dso_local void @"?foo@?$M@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.

[hans]

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

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.

[takuto.ikuta]

export/import explicit template instantiation function

[takuto.ikuta]

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

Thank you for confirmation. I revived the check.

[rnk]

clang/lib/AST/ASTContext.cpp

9552 ↗

(On Diff #169792)

I agree, this option should not affect explicit instantiations. Inline functions from explicit instantiations should still be exported+weak_odr, and imported+available_externally for extern declarations.

[rnk]

clang/include/clang/Basic/LangOptions.h

218

We should define this in the LangOptions.def file.

[takuto.ikuta]

Added option to LangOptions.def

[takuto.ikuta]

Thank you for review!

clang/include/clang/Basic/LangOptions.h

218

Moved.

[hans]

Thank you Takuto! I think the functionality looks great now: it's not too complicated :-)

I only have comments about the test.

clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

3 ↗

(On Diff #172090)

Instead of "DEFAULT", I think "CHECK" is the default prefix to use when using the same prefix for multiple invocations.

Also, instead of INLINE and NOINLINE (which sounds like it's about inlining), I'd suggest perhaps "EXPORTINLINES" and "NOEXPORTINLINES" or something like that.

6 ↗

(On Diff #172090)

Instead of using -O0 here and above, consider using -O1 -disable-llvm-passes so we get available_externally functions.

9 ↗

(On Diff #172090)

This change doesn't have any effect on non-member functions, so I don't think we need these tests.

42 ↗

(On Diff #172090)

Can you move the checks down to where the variable/function is defined? That makes it easier to read the test I think.

49 ↗

(On Diff #172090)

I think this test case should be first in the file, because it's really showing the core of this feature.

I would suggest just calling "ExportedClass" to make it simpler (if it was a template, then we'd put template in the name). Also, I suggest making it a struct instead so you don't have to write "public:".

51 ↗

(On Diff #172090)

This check is only using part of the mangled name..
But I don't think the patch does anything special for this kind of function, so I'd just skip this test.

58 ↗

(On Diff #172090)

This doesn't seem to be used for anything?

73 ↗

(On Diff #172090)

Why foceinline? I think it should be just "inline". This goes for all use of forceinline in this file.

76 ↗

(On Diff #172090)

Hmm, I would have thought we should export this function.

102 ↗

(On Diff #172090)

I'm not sure that OutclassDefFunc() adds much value to the test. Also templateValue below. I think they can be removed.

122 ↗

(On Diff #172090)

We should also have a test with implicit instantiation, and then the inline function should not be exported when using /Zc:dllexportInlines-.

132 ↗

(On Diff #172090)

Again, I think we can drop the out-of-line function since this change should not affect those.

144 ↗

(On Diff #172090)

This case is not interesting, since it has nothing to do with dllexport/import, I think it can be removed.

148 ↗

(On Diff #172090)

Ditto.

165 ↗

(On Diff #172090)

Again, make the classes "struct" and remove the "public:" lines :-)

166 ↗

(On Diff #172090)

I don't see any test lines that reference this inner class. I guess it's not interesting?

172 ↗

(On Diff #172090)

This check looks wrong. I think there should be a dllimport declaration for this function? (Or an available_externally definition if we use -O1?)

181 ↗

(On Diff #172090)

Let's put this first in the class, since it's the "basic" case, and the static local is more of a special case.

[hans]

clang/include/clang/Driver/CLCompatOptions.td

304

Just one more small thing I remembered: please add a test in tests/Driver/cl-options.c to make sure we translate this to the cc1 flag correctly.

[takuto.ikuta]

Simplify test and fix some behavior.

[takuto.ikuta]

Hans, thank you for review! I addressed all your comment and fixed small behavior.

clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

76 ↗

(On Diff #172090)

I see.
Added MD->isThisDeclarationADefinition() check in Sema::checkClassLevelDLLAttribute
This will make change like https://chromium-review.googlesource.com/c/v8/v8/+/1186017 unnecessary.

122 ↗

(On Diff #172090)

Added test and MD->isImplicitlyInstantiable() check for the test.

172 ↗

(On Diff #172090)

I think it is OK not dllimporting this function as far as static variable is dllimported and definition of this function is emitted.

[takuto.ikuta]

Added cl driver flag test

[takuto.ikuta]

I missed the comment about driver flag test.

[hans]

Thanks! I don't think the new isThisDeclarationADefinition() and isImplicitlyInstantiable() checks are right. Besides that, I only have a few more comments about the test.

clang/lib/Sema/SemaDeclCXX.cpp

5785

The isThisDeclarationADefinition() and isImplicitlyInstantiable() checks don't look right. Just checking isInlined() should be enough.

clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

9 ↗

(On Diff #172317)

Why this separate STATIC invocation? It looks just the same as the invocation above.

Oh I see, it's because the test is mixing -DAG and -NOT lines, which breaks things.

I have a suggestion for avoiding the -NOT lines below, and that means this separate invocation to check for statics (and the other one below) can be removed.

25 ↗

(On Diff #172317)

Instead of a -NOT check, it would be better to "use" the function somewhere so that it's emitted, and check that it's not dllexport. That will make it easier to see the difference between NOEXPORTINLINE and EXPORTINLINE.

Also it avoids -NOT checks, which don't interact well with -DAG checks, and would let you remove the separate STATIC invocation.

44 ↗

(On Diff #172317)

Having an inline function and not defining it like this is not so great, especially in a dllexport class. I don't think there's any need to change the behaviour here or test for it.

47 ↗

(On Diff #172317)

But with -fno-dllexport-inlines, I don't think it should be exported.

It really doesn't matter if the body is inside or outside the class -- it's still an inline member function, and so the flag should apply.

53 ↗

(On Diff #172317)

I'd suggest calling it "OutoflineDefFunc()" instead.

65 ↗

(On Diff #172317)

I don't think there's any reason to dllexport the "user" function.

81 ↗

(On Diff #172317)

I don't think this instantiation is different from the ​​template class TemplateExportedClass<B22>; one below, so I think we can skip it.

83 ↗

(On Diff #172317)

There should also be a check to make sure it's exported also in the NOEXPORTINLINE case.

88 ↗

(On Diff #172317)

Thanks, this is the implicit instantiation case. It would be good to have an inline function with a static local to make sure it does get exported even with the flag.

[takuto.ikuta]

Address comment

[takuto.ikuta]

added a few more check

[takuto.ikuta]

Thank you for quick review!

clang/lib/Sema/SemaDeclCXX.cpp

5785

I see.

[hans]

Assuming the test still passes when you put the EXPORTINLINE filecheck prefix back, looks good to me!

clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

9 ↗

(On Diff #172332)

Did the "EXPORTINLINE" check-prefix get lost here?

[takuto.ikuta]

Fix check-prefix

[takuto.ikuta]

Thank you! I'll submit this if other people do not have any comments.

clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

9 ↗

(On Diff #172332)

Good catch!

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_internal_in_extern_inline : ExtWarn<
   "static %select{function|variable}0 %1 is used in an inline function with "
   "external linkage">, InGroup<StaticInInline>;
 def ext_internal_in_extern_inline_quiet : Extension<
   "static %select{function|variable}0 %1 is used in an inline function with "
   "external linkage">, InGroup<StaticInInline>;
 def warn_static_local_in_extern_inline : Warning<
   "non-constant static local variable in inline function may be different "
   "in different files">, InGroup<StaticLocalInInline>;
+def warn_static_local_in_inline_member_of_exported_class : Warning<
+  "static %0 used in an inline function of "
+  "exported class isn't exported due to -fno-dllexport-inlines">,
+  InGroup<StaticInInline>;
 def note_convert_inline_to_static : Note<
   "use 'static' to give inline function %0 internal linkage">;
 
 def ext_redefinition_of_typedef : ExtWarn<
   "redefinition of typedef %0 is a C11 feature">,
   InGroup<DiagGroup<"typedef-redefinition"> >;
 def err_redefinition_variably_modified_typedef : Error<
   "redefinition of %select{typedef|type alias}0 for variably-modified type %1">;

clang/include/clang/Basic/LangOptions.h

public:
   /// Name of the IR file that contains the result of the OpenMP target
   /// host code generation.
   std::string OMPHostIRFile;
 
   /// Indicates whether the front-end is explicitly told that the
   /// input is a header file (i.e. -x c-header).
   bool IsHeaderFile = false;
 
+  /// If set, dllexported classes dllexport their inline methods.
+  bool DllexportInlines = true;
+
   LangOptions();
 
   // Define accessors/mutators for language options of enumeration type.
 #define LANGOPT(Name, Bits, Default, Description)
 #define ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
   Type get##Name() const { return static_cast<Type>(Name); } \
   void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
 #include "clang/Basic/LangOptions.def"
#include "clang/Basic/LangOptions.def"
 
   /// Return the OpenCL C or C++ version as a VersionTuple.
   VersionTuple getOpenCLVersionTuple() const;
 };
 
 /// Floating point control options
 class FPOptions {
 public:
-  FPOptions() : fp_contract(LangOptions::FPC_Off), 
+  FPOptions() : fp_contract(LangOptions::FPC_Off),
                 fenv_access(LangOptions::FEA_Off) {}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
       : fp_contract(static_cast<LangOptions::FPContractModeKind>(I & 3)),
         fenv_access(static_cast<LangOptions::FEnvAccessModeKind>((I >> 2) & 1))
         {}
 
public:
   }
 
   void setDisallowFEnvAccess() { fenv_access = LangOptions::FEA_Off; }
 
   /// Used to serialize this.
   unsigned getInt() const { return fp_contract | (fenv_access << 2); }
 
 private:
-  /// Adjust BinaryOperator::FPFeatures to match the total bit-field size 
+  /// Adjust BinaryOperator::FPFeatures to match the total bit-field size
   /// of these two.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;
 };
 
 /// Describes the kind of translation unit being processed.
 enum TranslationUnitKind {
   /// The translation unit is a complete translation unit.

clang/include/clang/Basic/LangOptions.def

 LANGOPT(Char8             , 1, 0, "char8_t keyword")
 LANGOPT(DeclSpecKeyword   , 1, 0, "__declspec keyword")
 BENIGN_LANGOPT(DollarIdents   , 1, 1, "'$' in identifiers")
 BENIGN_LANGOPT(AsmPreprocessor, 1, 0, "preprocessor in asm mode")
 LANGOPT(GNUMode           , 1, 1, "GNU extensions")
 LANGOPT(GNUKeywords       , 1, 1, "GNU keywords")
 BENIGN_LANGOPT(ImplicitInt, 1, !C99 && !CPlusPlus, "C89 implicit 'int'")
 LANGOPT(Digraphs          , 1, 0, "digraphs")
+LANGOPT(DllexportInlines  , 1, 1, "If dllexport a class should dllexport inline methods in the Microsoft ABI")
 BENIGN_LANGOPT(HexFloats  , 1, C99, "C99 hexadecimal float constants")
 LANGOPT(CXXOperatorNames  , 1, 0, "C++ operator name keywords")
 LANGOPT(AppleKext         , 1, 0, "Apple kext support")
 BENIGN_LANGOPT(PascalStrings, 1, 0, "Pascal string support")
 LANGOPT(WritableStrings   , 1, 0, "writable string support")
 LANGOPT(ConstStrings      , 1, 0, "const-qualified string support")
 LANGOPT(LaxVectorConversions , 1, 1, "lax vector conversions")
 LANGOPT(AltiVec           , 1, 0, "AltiVec-style vector initializers")

clang/include/clang/Driver/CC1Options.td

 def compress_debug_sections_EQ : Joined<["-"], "compress-debug-sections=">,
     HelpText<"DWARF debug sections compression type">;
 def mno_exec_stack : Flag<["-"], "mnoexecstack">,
   HelpText<"Mark the file as not needing an executable stack">;
 def massembler_fatal_warnings : Flag<["-"], "massembler-fatal-warnings">,
   HelpText<"Make assembler warnings fatal">;
 def mrelax_relocations : Flag<["--"], "mrelax-relocations">,
     HelpText<"Use relaxable elf relocations">;
 def msave_temp_labels : Flag<["-"], "msave-temp-labels">,
   HelpText<"Save temporary labels in the symbol table. "
            "Note this may change .s semantics and shouldn't generally be used "
            "on compiler-generated code.">;
 def mrelocation_model : Separate<["-"], "mrelocation-model">,
   HelpText<"The relocation model to use">, Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">;
 def fno_math_builtin : Flag<["-"], "fno-math-builtin">,
   HelpText<"Disable implicit builtin knowledge of math functions">;
 }

 def fbracket_depth : Separate<["-"], "fbracket-depth">,
   HelpText<"Maximum nesting level for parentheses, brackets, and braces">;
 def fconst_strings : Flag<["-"], "fconst-strings">,
   HelpText<"Use a const qualified type for string literals in C and ObjC">;
 def fno_const_strings : Flag<["-"], "fno-const-strings">,
   HelpText<"Don't use a const qualified type for string literals in C and ObjC">;
 def fno_bitfield_type_align : Flag<["-"], "fno-bitfield-type-align">,
   HelpText<"Ignore bit-field types when aligning structures">;
+def fno_dllexport_inlines : Flag<["-"], "fno-dllexport-inlines">,
+  HelpText<"Don't export inline functions">;
 def ffake_address_space_map : Flag<["-"], "ffake-address-space-map">,
   HelpText<"Use a fake address space map; OpenCL testing purposes only">;
 def faddress_space_map_mangling_EQ : Joined<["-"], "faddress-space-map-mangling=">, MetaVarName<"<yes|no|target>">,
   HelpText<"Set the mode for address space map based mangling; OpenCL testing purposes only">;
 def funknown_anytype : Flag<["-"], "funknown-anytype">,
   HelpText<"Enable parser support for the __unknown_anytype type; for testing purposes only">;
 def fdebugger_support : Flag<["-"], "fdebugger-support">,
   HelpText<"Enable special debugger support behavior">;

clang/include/clang/Driver/CLCompatOptions.td

def _SLASH_Yc : CLJoined<"Yc">,
   HelpText<"Generate a pch file for all code up to and including <filename>">,
   MetaVarName<"<filename>">;
 def _SLASH_Yu : CLJoined<"Yu">,
   HelpText<"Load a pch file and use it instead of all code up to "
            "and including <filename>">,
   MetaVarName<"<filename>">;
 def _SLASH_Y_ : CLFlag<"Y-">,
   HelpText<"Disable precompiled headers, overrides /Yc and /Yu">;
+def _SLASH_Zc_dllexportInlines : CLFlag<"Zc:dllexportInlines">;
+def _SLASH_Zc_dllexportInlines_ : CLFlag<"Zc:dllexportInlines-">;
 def _SLASH_Fp : CLJoined<"Fp">,
   HelpText<"Set pch filename (with /Yc and /Yu)">, MetaVarName<"<filename>">;
 
 def _SLASH_Gd : CLFlag<"Gd">,
   HelpText<"Set __cdecl as a default calling convention">;
 def _SLASH_Gr : CLFlag<"Gr">,
   HelpText<"Set __fastcall as a default calling convention">;
 def _SLASH_Gz : CLFlag<"Gz">,

clang/lib/Driver/ToolChains/Clang.cpp

else
     VolatileOptionID = options::OPT__SLASH_volatile_iso;
 
   if (Arg *A = Args.getLastArg(options::OPT__SLASH_volatile_Group))
     VolatileOptionID = A->getOption().getID();
 
   if (VolatileOptionID == options::OPT__SLASH_volatile_ms)
     CmdArgs.push_back("-fms-volatile");
 
+  if (Args.hasFlag(options::OPT__SLASH_Zc_dllexportInlines_,
+                   options::OPT__SLASH_Zc_dllexportInlines,
+                   false))
+    CmdArgs.push_back("-fno-dllexport-inlines");
+
   Arg *MostGeneralArg = Args.getLastArg(options::OPT__SLASH_vmg);
   Arg *BestCaseArg = Args.getLastArg(options::OPT__SLASH_vmb);
   if (MostGeneralArg && BestCaseArg)
     D.Diag(clang::diag::err_drv_argument_not_allowed_with)
         << MostGeneralArg->getAsString(Args) << BestCaseArg->getAsString(Args);
 
   if (MostGeneralArg) {
     Arg *SingleArg = Args.getLastArg(options::OPT__SLASH_vms);

clang/lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
       const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
       if (!IsInputCompatibleWithStandard(IK, Std)) {
         Diags.Report(diag::err_drv_argument_not_allowed_with)
           << A->getAsString(Args) << GetInputKindName(IK);
       }
     }
   }
 
+  if (Args.hasArg(OPT_fno_dllexport_inlines))
+    Opts.DllexportInlines = false;
+
   if (const Arg *A = Args.getLastArg(OPT_fcf_protection_EQ)) {
     StringRef Name = A->getValue();
     if (Name == "full" || Name == "branch") {
       Opts.CFProtectionBranch = 1;
     }
   }
   // -cl-std only applies for OpenCL language standards.
   // Override the -std option in this case.

clang/lib/Sema/SemaDeclCXX.cpp

// Nested classes and class templates.
       CheckAbstractClassUsage(Info, cast<CXXRecordDecl>(D));
     } else if (isa<ClassTemplateDecl>(D)) {
       CheckAbstractClassUsage(Info,
                              cast<ClassTemplateDecl>(D)->getTemplatedDecl());
     }
   }
 }
 
+namespace {
+
+// StaticLocalVarChecker is used to warn if static local variables are used in
+// inline functions of exported class with fno-dllexport-inlines
+class StaticLocalVarChecker final : public ConstStmtVisitor<StaticLocalVarChecker> {
+ private:
+  Sema &SemaRef;
+
+ public:
+  StaticLocalVarChecker(Sema &S) : SemaRef(S) {}
+  void VisitDeclStmt(const DeclStmt *E) {
+    for (const Decl *D : E->decls()) {
+      if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
+        if (VD->isStaticLocal() && !VD->checkInitIsICE()) {
+          SemaRef.Diag(VD->getLocation(),
+                       diag::warn_static_local_in_inline_member_of_exported_class)
+              << VD;
+        }
+      }
+    }
+  }
+
+  void VisitStmt(const Stmt *S) {
+    for (const Stmt *Child : S->children()) {
+      if (Child) {
+        Visit(Child);
+      }
+    }
+  }
+};
+} // namespace
+
 static void ReferenceDllExportedMembers(Sema &S, CXXRecordDecl *Class) {
   Attr *ClassAttr = getDLLAttr(Class);
   if (!ClassAttr)
     return;
 
   assert(ClassAttr->getKind() == attr::DLLExport);
 
   TemplateSpecializationKind TSK = Class->getTemplateSpecializationKind();
if (VD && Member->getAttr<DLLExportAttr>() &&
         VD->getStorageClass() == SC_Static &&
         TSK == TSK_ImplicitInstantiation)
       S.MarkVariableReferenced(VD->getLocation(), VD);
 
     auto *MD = dyn_cast<CXXMethodDecl>(Member);
     if (!MD)
       continue;
 
+    // XXX: need to skip inlines here
+    // or not needed because they no longer get dllexport attribs. hmm, best place here
+    // or checkClassLevelDLLAttribute?
+    if (MD->isInlined() && !getDLLAttr(MD)) {
+      const Stmt *Body = nullptr;
+      StaticLocalVarChecker Checker(S);
+
+      if (const FunctionDecl *Definition = MD->getDefinition()) {
+        Body = Definition->getBody();
+
+        if (S.Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+            !S.getLangOpts().DllexportInlines &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDeclaration &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDefinition &&
+            Body) {
+          Checker.Visit(Body);
+        }
+      }
+    }
+
     if (Member->getAttr<DLLExportAttr>()) {
       if (MD->isUserProvided()) {
         // Instantiate non-default class member functions ...
 
         // .. except for certain kinds of template specializations.
         if (TSK == TSK_ImplicitInstantiation && !ClassAttr->isInherited())
           continue;
 
         S.MarkFunctionReferenced(Class->getLocation(), MD);
 
         // The function will be passed to the consumer when its definition is
         // encountered.
       } else if (!MD->isTrivial() || MD->isExplicitlyDefaulted() ||
                  MD->isCopyAssignmentOperator() ||
                  MD->isMoveAssignmentOperator()) {
         // Synthesize and instantiate non-trivial implicit methods, explicitly
         // defaulted methods, and the copy and move assignment operators. The
         // latter are exported even if they are trivial, because the address of
         // an operator can be taken and should compare equal across libraries.
         DiagnosticErrorTrap Trap(S.Diags);
         S.MarkFunctionReferenced(Class->getLocation(), MD);
         if (Trap.hasErrorOccurred()) {
           S.Diag(ClassAttr->getLocation(), diag::note_due_to_dllexported_class)
               << Class << !S.getLangOpts().CPlusPlus11;
           break;
         }
 
         // There is no later point when we will see the definition of this
if (LastExportedDefaultCtor) {
       S.Diag(CD->getLocation(), diag::note_entity_declared_at)
           << CD->getDeclName();
       return;
     }
     LastExportedDefaultCtor = CD;
   }
 }
 
 void Sema::checkClassLevelCodeSegAttribute(CXXRecordDecl *Class) {
   // Mark any compiler-generated routines with the implicit code_seg attribute.
   for (auto *Method : Class->methods()) {
     if (Method->isUserProvided())
       continue;
     if (Attr *A = getImplicitCodeSegOrSectionAttrForFunction(Method, /*IsDefinition=*/true))
       Method->addAttr(A);
   }
 }
 
 /// Check class-level dllimport/dllexport attribute.
 void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
   Attr *ClassAttr = getDLLAttr(Class);
 
   // MSVC inherits DLL attributes to partial class template specializations.
   if (Context.getTargetInfo().getCXXABI().isMicrosoft() && !ClassAttr) {
     if (auto *Spec = dyn_cast<ClassTemplatePartialSpecializationDecl>(Class)) {
       if (Attr *TemplateAttr =
               getDLLAttr(Spec->getSpecializedTemplate()->getTemplatedDecl())) {
         auto *A = cast<InheritableAttr>(TemplateAttr->clone(getASTContext()));
         A->setInherited(true);
         ClassAttr = A;
       }
     }
   }
 
   if (!ClassAttr)
     return;
 
   if (!Class->isExternallyVisible()) {
     Diag(Class->getLocation(), diag::err_attribute_dll_not_extern)
         << Class << ClassAttr;
     return;
   }
if (MD) {
         continue;
 
       if (MD->isInlined()) {
         // MinGW does not import or export inline methods.
         if (!Context.getTargetInfo().getCXXABI().isMicrosoft() &&
             !Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())
           continue;
 
+        if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+            !getLangOpts().DllexportInlines &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDeclaration &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDefinition &&
+
+            // To check static local var later.
+            MD->isDefined()) {
+          // Warn later if MD has static local variable.
+          continue;
+        }
+
         // MSVC versions before 2015 don't export the move assignment operators
         // and move constructor, so don't attempt to import/export them if
         // we have a definition.
         auto *Ctor = dyn_cast<CXXConstructorDecl>(MD);
         if ((MD->isMoveAssignmentOperator() ||
              (Ctor && Ctor->isMoveConstructor())) &&
             !getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015))
           continue;
 
         // MSVC2015 doesn't export trivial defaulted x-tor but copy assign
         // operator is exported anyway.
         if (getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
             (Ctor || isa<CXXDestructorDecl>(MD)) && MD->isTrivial())
           continue;
       }
     }
 
     // Don't apply dllimport attributes to static data members of class template
     // instantiations when the attribute is propagated from a derived class.
     if (VD && PropagatedImport)
       continue;
 
     if (!cast<NamedDecl>(Member)->isExternallyVisible())
       continue;
 
     if (!getDLLAttr(Member)) {
       auto *NewAttr =
           cast<InheritableAttr>(ClassAttr->clone(getASTContext()));
       NewAttr->setInherited(true);
       Member->addAttr(NewAttr);
 
       if (MD) {
         // Propagate DLLAttr to friend re-declarations of MD that have already
         // been constructed.
         for (FunctionDecl *FD = MD->getMostRecentDecl(); FD;
              FD = FD->getPreviousDecl()) {
           if (FD->getFriendObjectKind() == Decl::FOK_None)
             continue;
           assert(!getDLLAttr(FD) &&
                  "friend re-decl should not already have a DLLAttr");
           NewAttr = cast<InheritableAttr>(ClassAttr->clone(getASTContext()));
           NewAttr->setInherited(true);
           FD->addAttr(NewAttr);
         }
       }

clang/test/CodeGenCXX/dllexport-no-inline.cpp

+// RUN: %clang_cc1 %s -fms-extensions -triple x86_64-windows-msvc       \
+// RUN:     -fno-dllexport-inlines -emit-llvm -O0 -o - -verify -DNOEXPORT_INLINE | \
+// RUN:     FileCheck --check-prefix=DEFAULT --check-prefix=NOINLINE %s
+
+// RUN: %clang_cc1 %s -fms-extensions -triple x86_64-windows-msvc       \
+// RUN:     -emit-llvm -O0 -o - |                                       \
+// RUN:     FileCheck --check-prefix=DEFAULT --check-prefix=INLINE %s
+
+
+// Function
+
+// DEFAULT-DAG: define dso_local dllexport void @"?NormalFunction@@YAXXZ"()
+void __declspec(dllexport) NormalFunction() {}
+
+
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?AlwaysInlineFunction@@
+__forceinline void __declspec(dllexport) AlwaysInlineFunction() {}
+
+
+// Class member function
+
+class __declspec(dllexport) NoTemplateExportedClass {
+  // DEFAULT-NOT: NoTemplateExportedClass@NoTemplateExportedClass@@
+  NoTemplateExportedClass() = default;
+
+  // NOINLINE-NOT: InclassDefFunc@NoTemplateExportedClass
+  // INLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@NoTemplateExportedClass@@AEAAXXZ"
+  void InclassDefFunc() {}
+
+  int f();
+
+  // FIX: make this DEFAULT-DAG.
+  // INLINE-DAG: define weak_odr dso_local dllexport i32 @"?InclassDefFuncWithStaticVariable@NoTemplateExportedClass@@AEAAHXZ"
+  int InclassDefFuncWithStaticVariable() {
+#ifdef NOEXPORT_INLINE
+    // expected-warning@+2 {{static 'static_variable' used in an inline function of exported class isn't exported due to -fno-dllexport-inlines}}
+#endif
+    static int static_variable = f();
+
+    static const int static_const_variable = 1; // expected-no-warning
+    constexpr int static_constexpr_variable = 2; // expected-no-warning
+    static const int static_const_array[] = {1, 2, 3}; // expected-no-warning
+
+    return ++static_variable + static_const_variable + static_constexpr_variable +
+        static_const_array[0];
+  }
+
+  // DEFAULT-NOT: InlineOutclassDefFuncWihtoutDefinition
+  __forceinline void InlineOutclassDefFuncWihtoutDefinition();
+
+  // FIX: Do not export this when -ms-no-dllexport-inline.
+  // DEFAULT-DAG: define weak_odr dso_local dllexport void @"?InlineOutclassDefFunc@NoTemplateExportedClass@@AEAAXXZ"
+  __forceinline void InlineOutclassDefFunc();
+
+  // FIX: Do not export this when -ms-no-dllexport-inline is given and warn for local static var.
+  // DEFAULT-DAG: define weak_odr dso_local dllexport i32 @"?InlineOutclassDefFuncWithStaticVariable@NoTemplateExportedClass@@AEAAHXZ"
+  __forceinline int InlineOutclassDefFuncWithStaticVariable();
+
+  // DEFAULT-DAG: define dso_local dllexport void @"?OutclassDefFunc@NoTemplateExportedClass@@AEAAXXZ"
+  void OutclassDefFunc();
+};
+
+void NoTemplateExportedClass::OutclassDefFunc() {}
+
+__forceinline void NoTemplateExportedClass::InlineOutclassDefFunc() {}
+
+__forceinline int NoTemplateExportedClass::InlineOutclassDefFuncWithStaticVariable() {
+    static int static_variable = 0;
+    return ++static_variable;
+}
+
+template<typename T>
+class __declspec(dllexport) TemplateExportedClass {
+  void InclassDefFunc() {}
+  void OutclassDefFunc();
+
+  T templateValue;
+};
+
+// DEFAULT-NOT: define dso_local dllexport void @"?OutclassDefFunc@NoTemplateExportedClass@@
+template<typename T> void TemplateExportedClass<T>::OutclassDefFunc() {}
+
+class A11{};
+class B22{};
+
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateExportedClass@VA11@@
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?OutclassDefFunc@?$TemplateExportedClass@VA11@@
+template class __declspec(dllexport) TemplateExportedClass<A11>;
+
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateExportedClass@VB22@@
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?OutclassDefFunc@?$TemplateExportedClass@VB22@@
+template class TemplateExportedClass<B22>;
+
+
+template<typename T>
+class TemplateNoExportedClass {
+  void InclassDefFunc() {}
+  void OutclassDefFunc();
+};
+
+template<typename T> void TemplateNoExportedClass<T>::OutclassDefFunc() {}
+
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateNoExportedClass@VA11@@
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?OutclassDefFunc@?$TemplateNoExportedClass@VA11@@
+template class __declspec(dllexport) TemplateNoExportedClass<A11>;
+
+// DEFAULT-DAG: define weak_odr dso_local void @"?InclassDefFunc@?$TemplateNoExportedClass@VB22@@
+// DEFAULT-DAG: define weak_odr dso_local void @"?OutclassDefFunc@?$TemplateNoExportedClass@VB22@@
+template class TemplateNoExportedClass<B22>;
+
+
+class __declspec(dllimport) ImportedClass {
+public:
+  class ImportedInnerClass {
+   public:
+    void OutClassDefFunc();
+  };
+
+  // INLINE-DAG: declare dllimport void @"?InClassDefFunc@ImportedClass@
+  // NOINLINE-NOT: dllimport .* InClassDefFunc@ImportedClass@
+  void InClassDefFunc() {
+    i.OutClassDefFunc();
+  }
+  ImportedInnerClass i;
+};
+
+void InClassDefFuncUser() {
+  // This is necessary for declare statement of ImportedClass::InClassDefFunc().
+  ImportedClass c;
+  c.InClassDefFunc();
+}