Page MenuHomePhabricator

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

Authored by llunak on Nov 3 2019, 1:29 PM.

Details

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.

Diff Detail

Event Timeline

llunak created this revision.Nov 3 2019, 1:29 PM
rsmith accepted this revision.Dec 5 2019, 2:52 PM

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.

This revision is now accepted and ready to land.Dec 5 2019, 2:52 PM

@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 added a comment.Dec 6 2019, 1:58 AM

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.

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

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.

This revision was automatically updated to reflect the committed changes.
dblaikie added a subscriber: rnk.Mar 17 2020, 1:42 PM

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?)

@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).

@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)

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

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

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.

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 updated this revision to Diff 261722.May 3 2020, 1:24 PM

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

llunak reopened this revision.May 3 2020, 1:25 PM

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

This revision is now accepted and ready to land.May 3 2020, 1:25 PM

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 requested changes to this revision.May 12 2020, 2:06 PM

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)

This revision now requires changes to proceed.May 12 2020, 2:06 PM
llunak updated this revision to Diff 263838.May 13 2020, 1:12 PM
llunak edited the summary of this revision. (Show Details)

Updated commit message.

llunak updated this revision to Diff 263839.May 13 2020, 1:16 PM
llunak edited the summary of this revision. (Show Details)

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.

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 added a subscriber: aganea.May 14 2020, 12:35 AM

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 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)

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.
dexonsmith resigned from this revision.Jun 26 2020, 3:11 PM
dblaikie accepted this revision.Jul 8 2020, 12:10 PM

Sounds good

This revision is now accepted and ready to land.Jul 8 2020, 12:10 PM
This revision was automatically updated to reflect the committed changes.