[PATCH] D48426: [clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801)

[Hans Wennborg via Phabricator via cfe-commits]

hans marked 4 inline comments as done.
hans added a comment.

In https://reviews.llvm.org/D48426#1140057, @dblaikie wrote:

> In https://reviews.llvm.org/D48426#1139823, @rnk wrote:
>
> > `LangOpts.ModulesCodegen` is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie
>
>
> Does it have to be only the dllexported inline functions - or could this be used to home all inline functions? (I guess not - presumably it's not acceptable for the compiler to implicitly promote something to dllexport (& if it doesn't do that promotion, then the function wouldn't be visible from the use))


Right, I don't think we could do that. But regular inline functions isn't as large a problem as the dllexport ones, which we have to emit even if they're not referenced, causing a huge amount of redundant codegen.

> Is it valid to provide a definition for optimization purposes (LLVM's available_externally linkage) for these inline functions? Or is it required that, after a change to the header (modifying the implementation of one of these dllexported inline functions), the DLL they're exported could be rebuilt without rebuilding the clients & the change in behavior would be correctly applied?

Yes, we will provide such definitions (not just available_externally, but weak_odr) if the inline function is referenced, as usual. This is only homing unreferenced but "force emitted", i.e. DeclMustBeEmitted, functions.



================
Comment at: include/clang/AST/ASTContext.h:2886-2887
+
+  // XXX: I don't like adding this to ASTContext, but I ran out of ideas for how ASTContext::DeclMustBeEmitted() would know about it otherwise.
+  bool BuildingPCHWithObjectFile = false;
 };
----------------
rnk wrote:
> Does `LangOpts.CompilingPCH` do the right thing, or is that also true when we make a PCH with no object file? In any case, I'd probably make this a langopt. Even if it's functionally different from ModulesCodegen and ModulesDebugInfo, it's the same basic idea.
Yes, CompilingPCH is always set when building PCHs, also when there's no object file so we can't use that.

Moving BuildingPCHWithObjectFile to LangOpts sounds good to me.

Actually that's interesting, because it means the flag will get written into the PCH will all the other LangOpts, and we could in theory look for that in ASTReader. But it seems the code isn't really set up to store the LangOpts coming out of an AST file, we only verify that they're compatible.


================
Comment at: test/CodeGen/pch-dllexport.cpp:22
+// PCH: define weak_odr dso_local dllexport x86_thiscallcc void @"?bar at S@@QAEXXZ"
+// PCHWITHOBJ-NOT: define weak_odr dso_local dllexport x86_thiscallcc void @"?bar at S@@QAEXXZ"
+};
----------------
dblaikie wrote:
> This is a pretty specific "NOT" - maybe:
> 
>   define {{.*}}@"?bar at ... 
> 
> would be better to ensure no definition is provided? (similarly above)
> 
> Also, should this file have some non-exported inline functions to check those do the right thing?
Updated the -NOT checks and added test.


https://reviews.llvm.org/D48426