Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[llunak]

Summary

Allow to build PCH's (with -building-pch-with-obj and the extra .o file) with -fmodules-codegen -fmodules-debuginfo to allow emitting shared code into the extra .o file, similarly to how it works with modules. A bit of a misnomer, but the underlying functionality is the same. This saves up to 20% of build time here. The patch is fairly simple, it basically just duplicates -fmodules checks to also alternatively check -building-pch-with-obj.

This already got committed as cbc9d22e49b434b6ceb2eb94b67079d02e0a7b74, but then got reverted in 7ea9a6e0220da36ff2fd1fbc29c2755be23e5166 because of PR44953, as discussed in D74846. This is a corrected version which does not include two places for the PCH case that aren't included in the modules -fmodules-codegen path either.

[llunak]

Ping.

[rsmith]

It's a bit weird for this to be controlled by a -fmodules flag, but it's only a -cc1 flag, so I'm OK with that; we can rename it if/when we expose it from the driver.

[dblaikie]

@rsmith - thanks for looking at this. I'd looked at it a bit and I was/am still wondering whether there's a way to coalesce these codepaths between PCH and the existing modular code generation support? But if you're good with it, that's certainly good enough for me.

[llunak]

In D69778#1771799, @rsmith wrote:

It's a bit weird for this to be controlled by a -fmodules flag, but it's only a -cc1 flag, so I'm OK with that; we can rename it if/when we expose it from the driver.

It's a bit weird, but I didn't want to add -fpch-codegen and then duplicate the checks everywhere. And the -building-pch-with-obj part requires a non-trivial build setup anyway. But I can create another patch that maps it all to some common flag, if wanted.

In D69778#1772125, @dblaikie wrote:

I was/am still wondering whether there's a way to coalesce these codepaths between PCH and the existing modular code generation support?

In what way could this be united more? The way I understand it, the code paths are pretty much the same, they just need to account for being invoked in different contexts. This patch is tiny compared to what it was originally before I made it reuse all the codegen code.

[dblaikie]

In D69778#1772125, @dblaikie wrote:

I was/am still wondering whether there's a way to coalesce these codepaths between PCH and the existing modular code generation support?

In what way could this be united more? The way I understand it, the code paths are pretty much the same, they just need to account for being invoked in different contexts. This patch is tiny compared to what it was originally before I made it reuse all the codegen code.

I guess one aspect is that -building-pch-with-obj seems like it duplicates the fmodules-codegen concept (both basically are a flag passed during pcm/pch build time that says "I promise to build an object file from this pcm/pch, so rely on that assumption when building other things that depend on the pcm/pch) - if I'd noticed the -building-pch-with-obj going in I would've made the point then.

But yeah, that's out of scope for this patch, but might be something that'd be good to look into to try to merge these features/codepaths more.

[llunak]

In D69778#1776472, @dblaikie wrote:

I guess one aspect is that -building-pch-with-obj seems like it duplicates the fmodules-codegen concept (both basically are a flag passed during pcm/pch build time that says "I promise to build an object file from this pcm/pch, so rely on that assumption when building other things that depend on the pcm/pch) - if I'd noticed the -building-pch-with-obj going in I would've made the point then.

Is that described somewhere? There's no mention of modules codegen anywhere under clang/docs. I was under the impression that modules created the codegen part directly in the .pcm in ELF (or whatever) format.

But yeah, that's out of scope for this patch, but might be something that'd be good to look into to try to merge these features/codepaths more.

Note that AFAICT the -building-pch-with-obj flag is pretty much internal, used only by the MSVC-like PCH creation, so presumably this could be still unified more without any real harm. And I think I could do the work if it'd be agreed that this would be a good thing.

As for this patch itself, could you please also review https://reviews.llvm.org/D69585 and https://reviews.llvm.org/D69779 ? I have not really tried this patch without the first one, so I'm a bit hesitant to merge it alone, and I quickly ran into the problem fixed by the second patch, so I'd prefer to merge them all three together if possible.

[dblaikie]

In D69778#1779042, @llunak wrote:

In D69778#1776472, @dblaikie wrote:

I guess one aspect is that -building-pch-with-obj seems like it duplicates the fmodules-codegen concept (both basically are a flag passed during pcm/pch build time that says "I promise to build an object file from this pcm/pch, so rely on that assumption when building other things that depend on the pcm/pch) - if I'd noticed the -building-pch-with-obj going in I would've made the point then.

Is that described somewhere? There's no mention of modules codegen anywhere under clang/docs. I was under the impression that modules created the codegen part directly in the .pcm in ELF (or whatever) format.

Nah, not as yet - I think I provided some examples in this/other reviews you & I have been having, and in the clang tests. Ah, yeah, this one: https://reviews.llvm.org/D69779#1801771 (

But yeah, that's out of scope for this patch, but might be something that'd be good to look into to try to merge these features/codepaths more.

Note that AFAICT the -building-pch-with-obj flag is pretty much internal, used only by the MSVC-like PCH creation, so presumably this could be still unified more without any real harm. And I think I could do the work if it'd be agreed that this would be a good thing.

@rnk - know anything about the history of -building-pch-with-obj and whether it could be replaced/merged with -fmodules-codegen? (-fmodules-codegen + -fmodules-debuginfo, perhaps?)

[llunak]

In D69778#1927526, @dblaikie wrote:

@rnk - know anything about the history of -building-pch-with-obj and whether it could be replaced/merged with -fmodules-codegen? (-fmodules-codegen + -fmodules-debuginfo, perhaps?)

It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT -fmodules-codegen is a superset of -building-pch-with-obj, the important difference is that -building-pch-with-obj decides which code will be shared while building TUs, while -fmodules-codegen decides while building the PCH. This in practice means that -building-pch-with-obj shares only code that would be actually used, which makes the result easier to use - using PCH with -fmodules-codegen pretty much requires something like -Wl,--gc-sections, otherwise there are many undefined references to internal symbols from other libraries caused by emitted code that isn't actually used. Since -building-pch-with-obj is internally used by clang-cl /Yc , -fmodules-codegen cannot be a drop-in replacement for it, unless the Microsoft linker automatically discards unused symbols (I don't know, no idea about Windows stuff).

[dblaikie]

In D69778#1927761, @llunak wrote:

In D69778#1927526, @dblaikie wrote:

@rnk - know anything about the history of -building-pch-with-obj and whether it could be replaced/merged with -fmodules-codegen? (-fmodules-codegen + -fmodules-debuginfo, perhaps?)

It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT -fmodules-codegen is a superset of -building-pch-with-obj, the important difference is that -building-pch-with-obj decides which code will be shared while building TUs, while -fmodules-codegen decides while building the PCH.

I'm not sure I follow - how would that be possible? building the object from the PCH is done independently of any usage, so it can't depend on the way the PCH is used by any TU that includes it, I think?

Hmm, yeah, seems it's not context-sensitive (actually Hans says on the original review it is context sensitive - but in the reverse: it ensures that /unreferenced/ dllexported functions are exported), but it is dllexport sensitive. So that's probably enough to not make this generalizable - seems I did ask these questions back when the feature was reviewed: https://reviews.llvm.org/D48426 & I'd forgotten. Sorry about that. Seems there's not much/nothing to do here - the two features are similar, but sufficiently different. ( @hans in case he's got any thoughts here)

[llunak]

In D69778#1928111, @dblaikie wrote:

In D69778#1927761, @llunak wrote:

It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT -fmodules-codegen is a superset of -building-pch-with-obj, the important difference is that -building-pch-with-obj decides which code will be shared while building TUs, while -fmodules-codegen decides while building the PCH.

I'm not sure I follow - how would that be possible? building the object from the PCH is done independently of any usage, so it can't depend on the way the PCH is used by any TU that includes it, I think?

If a TU uses a PCH, it'll result in some code emitted in every TU that uses the same PCH (e.g. template instantiations from the PCH). And -building-pch-with-obj detects these and discards them in all TUs except the PCH's object file. That means that the code shared in the PCH's object file is only code that would be duplicated in all TUs using the PCH. This is fundamentally different from -fmodules-codegen, which decides what will be emitted and shared already when creating the PCH, which has the consequence that it selects also a lot of code that will be eventually unused, which causes problems like the -Wl,--gc-sections part or not being necessarily worth it (IIRC even your -fmodules-codegen commit says so). So while -fmodules-codegen should be generally superior, -building-pch-with-obj is trivial to use. That's my understanding of both the approaches (and I spent quite a lot of time looking at both when working on D69778 and D69585).

[dblaikie]

In D69778#1928417, @llunak wrote:

In D69778#1928111, @dblaikie wrote:

In D69778#1927761, @llunak wrote:

It comes from 08c5a7b8fda1b97df9d027d1ac096f93edeb6c2f . AFAICT -fmodules-codegen is a superset of -building-pch-with-obj, the important difference is that -building-pch-with-obj decides which code will be shared while building TUs, while -fmodules-codegen decides while building the PCH.

I'm not sure I follow - how would that be possible? building the object from the PCH is done independently of any usage, so it can't depend on the way the PCH is used by any TU that includes it, I think?

If a TU uses a PCH, it'll result in some code emitted in every TU that uses the same PCH (e.g. template instantiations from the PCH). And -building-pch-with-obj detects these and discards them in all TUs except the PCH's object file. That means that the code shared in the PCH's object file is only code that would be duplicated in all TUs using the PCH.

I'm not sure how that could be possible - there's no data transferred between the compilation of the TU's object files and the PCH's object file, right?

Looks to me like the original patch claims the PCH object file contains all the dllexported inline functions from the header. Which, yes, is different from -fmodules-codegen, but doesn't sound like it's based on usage & I'm not sure how it could be based on usage.

This is fundamentally different from -fmodules-codegen, which decides what will be emitted and shared already when creating the PCH, which has the consequence that it selects also a lot of code that will be eventually unused, which causes problems like the -Wl,--gc-sections part or not being necessarily worth it (IIRC even your -fmodules-codegen commit says so). So while -fmodules-codegen should be generally superior, -building-pch-with-obj is trivial to use. That's my understanding of both the approaches (and I spent quite a lot of time looking at both when working on D69778 and D69585).

[llunak]

In D69778#1929737, @dblaikie wrote:

I'm not sure how that could be possible - there's no data transferred between the compilation of the TU's object files and the PCH's object file, right?

Yes, there is, in a way - the PCH itself. I didn't say everything from the PCH was shared, in fact it's exactly the point that it's not everything from the PCH, unlike -fmodules-codegen.

Looks to me like the original patch claims the PCH object file contains all the dllexported inline functions from the header. Which, yes, is different from -fmodules-codegen, but doesn't sound like it's based on usage & I'm not sure how it could be based on usage.

It's based on what's to be emitted. See ASTContext::DeclMustBeEmitted(), I think the comment there says it all.

[dblaikie]

In D69778#1929817, @llunak wrote:

In D69778#1929737, @dblaikie wrote:

I'm not sure how that could be possible - there's no data transferred between the compilation of the TU's object files and the PCH's object file, right?

Yes, there is, in a way - the PCH itself. I didn't say everything from the PCH was shared, in fact it's exactly the point that it's not everything from the PCH, unlike -fmodules-codegen.

Looks to me like the original patch claims the PCH object file contains all the dllexported inline functions from the header. Which, yes, is different from -fmodules-codegen, but doesn't sound like it's based on usage & I'm not sure how it could be based on usage.

It's based on what's to be emitted. See ASTContext::DeclMustBeEmitted(), I think the comment there says it all.

Ah, I understand what you're getting at now - thanks for walking me through it!

[llunak]

Reopening because of https://reviews.llvm.org/D74846, updated version includes the partial revert from there.

[llunak]

Reopening because of https://reviews.llvm.org/D74846, updated version includes the partial revert from there.

[llunak]

Ping? I've reopened this one as suggested in D74846, but apparently it's kept the accepted state, so I'm not sure if this needs another approval or what to do here.

[dblaikie]

So the original commit ( cbc9d22e49b4 ) was reverted at some point, and now you're proposing recommitting it with a slight change?

Could you summarize (in the summary (which should hopefully be included in the commit message eventually)) the commit (include the hash), the revert (including the hash), reasons for revert and what's changed in this patch compared to the original commit that addresses the reasons for reverting? (& ideally what extra testing was done on the newer version of the patch to ensure the original issue or another like it would now be caught before committing)

[llunak]

Updated commit message.

[llunak]

In D69778#2032363, @dblaikie wrote:

So the original commit ( cbc9d22e49b4 ) was reverted at some point, and now you're proposing recommitting it with a slight change?

Yes.

Could you summarize (in the summary (which should hopefully be included in the commit message eventually)) the commit (include the hash), the revert (including the hash), reasons for revert and what's changed in this patch compared to the original commit that addresses the reasons for reverting?

Done and see D74846 for details and the exact change.

(& ideally what extra testing was done on the newer version of the patch to ensure the original issue or another like it would now be caught before committing)

There's no testing besides checking that PR44953 no longer crashes. As said in D74846, I don't see how to do a test for this, and if there's a problem with this patch then the same problem should also exist with modules.

[dblaikie]

In D69778#2035006, @llunak wrote:

In D69778#2032363, @dblaikie wrote:

So the original commit ( cbc9d22e49b4 ) was reverted at some point, and now you're proposing recommitting it with a slight change?

Yes.

Could you summarize (in the summary (which should hopefully be included in the commit message eventually)) the commit (include the hash), the revert (including the hash), reasons for revert and what's changed in this patch compared to the original commit that addresses the reasons for reverting?

Done and see D74846 for details and the exact change.

(& ideally what extra testing was done on the newer version of the patch to ensure the original issue or another like it would now be caught before committing)

There's no testing besides checking that PR44953 no longer crashes. As said in D74846, I don't see how to do a test for this, and if there's a problem with this patch then the same problem should also exist with modules.

Do you have a sense of the larger testing that PR44953 was reduced from? Have you tried compiling a non-trivial codebase (I assume you might've tested it with Open Office, for instance) - wonder why PR44953 didn't show up there? (perhaps just the construct needed to tickle the issue isn't common/isn't common in Open Office code, etc) and/or maybe see if the person who filed PR44953 might be able to test this version of the patch on the original codebase that bug was reduced from?

[llunak]

In D69778#2035318, @dblaikie wrote:

Do you have a sense of the larger testing that PR44953 was reduced from? Have you tried compiling a non-trivial codebase (I assume you might've tested it with Open Office, for instance) - wonder why PR44953 didn't show up there? (perhaps just the construct needed to tickle the issue isn't common/isn't common in Open Office code, etc)

I've been building LibreOffice and some of its 3rd-party dependencies such as Skia for months with this patch, without problems. PR44953 seems to be a corner case, we do not use D3DX12 headers in LO, and (I presume) __declspec(selectany) is a niche feature. And while I do not understand why PR44953 was happening, I do understand why my original patch was triggering it, and it will not happen with this patch (or, if it will happen, then it'll happen with modules as well). And I've tested with the testcase in PR44953.

(It's LibreOffice BTW, OpenOffice is anything but dead.)

and/or maybe see if the person who filed PR44953 might be able to test this version of the patch on the original codebase that bug was reduced from?

@aganea Have you tried how this version of the patch affects PR44953? If not, could you please try?

[aganea]

In D69778#2035805, @llunak wrote:

@aganea Have you tried how this version of the patch affects PR44953? If not, could you please try?

I've applied the patch, I don't see the issue raised in PR44953 anymore.

However as discussed offline with @llunak I'm just raising a concern: when building with this patch + D69585 (-fpch-instantiate-templates) + -fmodules-debuginfo -fmodules-codegen as instructed, the PCH compilation in one of games crashes with the callstack below. On another game, the compilation succeeds, but I can see link errors for a few symbols referenced from the .PCH.OBJ.

This patch may be fine on its own, because -fmodules-debuginfo -fmodules-codegen are not enabled by default with /Yc, but more work is needed to make this feature the default. This could be done in a subsequent patch.

Unknown code
UNREACHABLE executed at D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726!
Stack dump:
0.    Program arguments: -cc1 -triple x86_64-pc-windows-msvc19.20.0 -emit-obj (edited) -building-pch-with-obj -fpch-instantiate-templates -fmodules-codegen -fmodules-debuginfo -include-pch D:\(edited)\MyProject.pch -pch-through-header=(edited)\precomp.h -include (edited)\precomp.h (edited) -fms-compatibility-version=19.20 -std=c++17 -fdelayed-template-parsing -finline-functions -fobjc-runtime=gcc -fdiagnostics-show-option -fdiagnostics-absolute-paths -vectorize-loops -vectorize-slp -O3 -faddrsig -o D:\(edited)\MyProject.pch.obj -x c++ D:\(edited)\precomp.cpp
1.    <eof> parser at end of file
2.    Code generation
3.    Running pass 'Function Pass Manager' on module '(edited)\precomp.cpp'.
4.    Running pass 'X86 DAG->DAG Instruction Selection' on function '@"?GetDeterminant@Matrix44f@MyNamespace@@QEBAMXZ"'
#0 0x00007ff6bada1c06 HandleAbort D:\llvm-project\llvm\lib\Support\Windows\Signals.inc:408:0
#1 0x00007ff6beb4b0e1 raise D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
#2 0x00007ff6beb40e5c abort D:\llvm-project\build64_stage0\minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
#3 0x00007ff6bad58207 llvm::llvm_unreachable_internal(char const *, char const *, unsigned int) D:\llvm-project\llvm\lib\Support\ErrorHandling.cpp:210:0
#4 0x00007ff6bbb14d2e llvm::TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5726:0
#5 0x00007ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
#6 0x00007ff6bbb140b3 llvm::TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5671:0
#7 0x00007ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
#8 0x00007ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
#9 0x00007ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
#10 0x00007ff6bbb1496c llvm::TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5643:0
#11 0x00007ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
#12 0x00007ff6bbb14538 llvm::TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\TargetLowering.cpp:5708:0
#13 0x00007ff6bba1dd26 llvm::X86TargetLowering::getNegatedExpression(class llvm::SDValue, class llvm::SelectionDAG &, bool, bool, unsigned int) const D:\llvm-project\llvm\lib\Target\X86\X86ISelLowering.cpp:42707:0
#14 0x00007ff6bca4c6fb `anonymous namespace'::DAGCombiner::visit D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1573:0
#15 0x00007ff6bca30e4b `anonymous namespace'::DAGCombiner::combine D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:1634:0
#16 0x00007ff6bca3052f llvm::SelectionDAG::Combine(enum llvm::CombineLevel, class llvm::AAResults *, enum llvm::CodeGenOpt::Level) D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\DAGCombiner.cpp:21222:0
#17 0x00007ff6bbb3729b llvm::SelectionDAGISel::CodeGenAndEmitDAG(void) D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\SelectionDAGISel.cpp:816:0
#18 0x00007ff6bbb36d1b llvm::SelectionDAGISel::SelectBasicBlock(class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Instruction, 1, 0, void>, 0, 1>, class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Instruction, 1, 0, void>, 0, 1>, bool &) D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\SelectionDAGISel.cpp:736:0
#19 0x00007ff6bbb36657 llvm::SelectionDAGISel::SelectAllBasicBlocks(class llvm::Function const &) D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\SelectionDAGISel.cpp:1611:0
#20 0x00007ff6bbb32558 llvm::SelectionDAGISel::runOnMachineFunction(class llvm::MachineFunction &) D:\llvm-project\llvm\lib\CodeGen\SelectionDAG\SelectionDAGISel.cpp:504:0
#21 0x00007ff6bb8e2b0a `anonymous namespace'::X86DAGToDAGISel::runOnMachineFunction D:\llvm-project\llvm\lib\Target\X86\X86ISelDAGToDAG.cpp:195:0
#22 0x00007ff6baf8d2de llvm::MachineFunctionPass::runOnFunction(class llvm::Function &) D:\llvm-project\llvm\lib\CodeGen\MachineFunctionPass.cpp:73:0
#23 0x00007ff6ba9ff6d2 llvm::FPPassManager::runOnFunction(class llvm::Function &) D:\llvm-project\llvm\lib\IR\LegacyPassManager.cpp:1481:0
#24 0x00007ff6ba9ffa13 llvm::FPPassManager::runOnModule(class llvm::Module &) D:\llvm-project\llvm\lib\IR\LegacyPassManager.cpp:1517:0
#25 0x00007ff6baa0009c llvm::legacy::PassManagerImpl::run(class llvm::Module &) D:\llvm-project\llvm\lib\IR\LegacyPassManager.cpp:1694:0
#26 0x00007ff6bc02dc79 clang::EmitBackendOutput(class clang::DiagnosticsEngine &, class clang::HeaderSearchOptions const &, class clang::CodeGenOptions const &, class clang::TargetOptions const &, class clang::LangOptions const &, class llvm::DataLayout const &, class llvm::Module *, enum clang::BackendAction, class std::unique_ptr<class llvm::raw_pwrite_stream, struct std::default_delete<class llvm::raw_pwrite_stream>>) D:\llvm-project\clang\lib\CodeGen\BackendUtil.cpp:1550:0
#27 0x00007ff6bc397f9d clang::BackendConsumer::HandleTranslationUnit(class clang::ASTContext &) D:\llvm-project\clang\lib\CodeGen\CodeGenAction.cpp:339:0
#28 0x00007ff6bd9e2193 clang::ParseAST(class clang::Sema &, bool, bool) D:\llvm-project\clang\lib\Parse\ParseAST.cpp:178:0
#29 0x00007ff6bc2f88f4 clang::FrontendAction::Execute(void) D:\llvm-project\clang\lib\Frontend\FrontendAction.cpp:940:0
#30 0x00007ff6bae9e4bb clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) D:\llvm-project\clang\lib\Frontend\CompilerInstance.cpp:965:0
#31 0x00007ff6baefed5c clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) D:\llvm-project\clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:290:0
#32 0x00007ff6ba62798e cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) D:\llvm-project\clang\tools\driver\cc1_main.cpp:240:0
#33 0x00007ff6ba6249e5 ExecuteCC1Tool D:\llvm-project\clang\tools\driver\driver.cpp:329:0
#34 0x00007ff6ba624688 main D:\llvm-project\clang\tools\driver\driver.cpp:403:0
#35 0x00007ff6beb2cdd0 __scrt_common_main_seh d:\A01\_work\6\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#36 0x00007ff93be17bd4 (C:\WINDOWS\System32\KERNEL32.DLL+0x17bd4)
#37 0x00007ff93bfeced1 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x6ced1)

[llunak]

So what more needs to be done here to get this change accepted? It's already missed 10.0 and 11.1 is getting branched off in about 3 weeks.

• The change was already accepted once and the problem leading to its revert has already been fixed.
• It just enables for PCH something that's been enabled for modules for a long time (so whatever may be wrong with it will probably be wrong with modules as well).
• I've been using the feature for 8 months.
• There's no publicly reported problem with it (I know aganea reported them above, but they are private, so nobody can do anything about them).
• The feature is opt-in and disabled by default.

[llunak]

Ping.

[llunak]

Ping ...

[dblaikie]

Sounds good

clang/lib/Serialization/ASTReader.cpp

case EAGERLY_DESERIALIZED_DECLS:
       // about "interesting" decls (for instance, if we're building a module).
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         EagerlyDeserializedDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case MODULAR_CODEGEN_DECLS:
       // FIXME: Skip reading this record if our ASTConsumer doesn't care about
       // them (ie: if we're not codegenerating this module).
-      if (F.Kind == MK_MainFile)
+      if (F.Kind == MK_MainFile ||
+          getContext().getLangOpts().BuildingPCHWithObjectFile)
         for (unsigned I = 0, N = Record.size(); I != N; ++I)
           EagerlyDeserializedDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case SPECIAL_TYPES:
       if (SpecialTypes.empty()) {
         for (unsigned I = 0, N = Record.size(); I != N; ++I)
           SpecialTypes.push_back(getGlobalTypeID(F, Record[I]));

clang/lib/Serialization/ASTReaderDecl.cpp

return llvm::make_range(MergedRedeclIterator<DeclT>(D),
                           MergedRedeclIterator<DeclT>());
 }
 
 uint64_t ASTDeclReader::GetCurrentCursorOffset() {
   return Loc.F->DeclsCursor.GetCurrentBitNo() + Loc.F->GlobalBitOffset;
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  if (Record.readInt())
+  if (Record.readInt()) {
     Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
+        Reader.DeclIsFromPCHWithObjectFile(FD))
+      Reader.DefinitionSource[FD] = true;
+  }
   if (auto *CD = dyn_cast<CXXConstructorDecl>(FD)) {
     CD->setNumCtorInitializers(Record.readInt());
     if (CD->getNumCtorInitializers())
       CD->CtorInitializers = ReadGlobalOffset();
   }
   // Store the offset of the body so we can lazily load it later.
   Reader.PendingBodies[FD] = GetCurrentCursorOffset();
   HasPendingBody = true;
ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
   }
 
   if (VD->hasAttr<BlocksAttr>() && VD->getType()->getAsCXXRecordDecl()) {
     Expr *CopyExpr = Record.readExpr();
     if (CopyExpr)
       Reader.getContext().setBlockVarCopyInit(VD, CopyExpr, Record.readInt());
   }
 
-  if (VD->getStorageDuration() == SD_Static && Record.readInt())
+  if (VD->getStorageDuration() == SD_Static && Record.readInt()) {
     Reader.DefinitionSource[VD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
+        Reader.DeclIsFromPCHWithObjectFile(VD))
+      Reader.DefinitionSource[VD] = true;
+  }
 
   enum VarKind {
     VarNotTemplate = 0, VarTemplate, StaticDataMemberSpecialization
   };
   switch ((VarKind)Record.readInt()) {
   case VarNotTemplate:
     // Only true variables (not parameters or implicit parameters) can be
     // merged; the other kinds are not really redeclarable at all.
void ASTDeclReader::ReadCXXDefinitionData(
   #define FIELD(Name, Width, Merge) \
   Data.Name = Record.readInt();
   #include "clang/AST/CXXRecordDeclDefinitionBits.def"
 
   // Note: the caller has deserialized the IsLambda bit already.
   Data.ODRHash = Record.readInt();
   Data.HasODRHash = true;
 
-  if (Record.readInt())
+  if (Record.readInt()) {
     Reader.DefinitionSource[D] = Loc.F->Kind == ModuleKind::MK_MainFile;
+    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
+        Reader.DeclIsFromPCHWithObjectFile(D))
+      Reader.DefinitionSource[D] = true;
+  }
 
   Data.NumBases = Record.readInt();
   if (Data.NumBases)
     Data.Bases = ReadGlobalOffset();
   Data.NumVBases = Record.readInt();
   if (Data.NumVBases)
     Data.VBases = ReadGlobalOffset();
 

clang/lib/Serialization/ASTWriter.cpp

void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
   Record->push_back(Data.IsLambda);
 
   #define FIELD(Name, Width, Merge) \
   Record->push_back(Data.Name);
   #include "clang/AST/CXXRecordDeclDefinitionBits.def"
 
   // getODRHash will compute the ODRHash if it has not been previously computed.
   Record->push_back(D->getODRHash());
-  bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo &&
-                          Writer->WritingModule && !D->isDependentType();
+  bool ModulesDebugInfo =
+      Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
   Record->push_back(ModulesDebugInfo);
   if (ModulesDebugInfo)
     Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
 
   // IsLambda bit is already saved.
 
   Record->push_back(Data.NumBases);
   if (Data.NumBases > 0)

clang/lib/Serialization/ASTWriterDecl.cpp

 }
 
 void ASTRecordWriter::AddFunctionDefinition(const FunctionDecl *FD) {
   // Switch case IDs are per function body.
   Writer->ClearSwitchCaseIDs();
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
     Optional<GVALinkage> Linkage;
-    if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+    if (Writer->WritingModule &&
+        Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
       // When building a C++ Modules TS module interface unit, a strong
       // definition in the module interface is provided by the compilation of
       // that module interface unit, not by its users. (Inline functions are
       // still emitted in module users.)
       Linkage = Writer->Context->GetGVALinkageForFunction(FD);
       ModulesCodegen = *Linkage == GVA_StrongExternal;
     }
     if (Writer->Context->getLangOpts().ModulesCodegen) {

clang/test/Modules/Inputs/codegen-flags/foo.h

+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif

clang/test/PCH/codegen.cpp

+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl