This is an archive of the discontinued LLVM Phabricator instance.

Modules: Add LangOptions::CacheGeneratedPCH
ClosedPublic

Authored by dexonsmith on Mar 9 2019, 9:22 AM.

Details

Summary

Add an option to cache the generated PCH in the ModuleCache when
emitting it. This protects clients that build PCHs and read them in the
same process, allowing them to avoid race conditions between parallel
jobs the same way that Clang's implicit module build system does.

Diff Detail

Event Timeline

dexonsmith created this revision.Mar 9 2019, 9:22 AM

This commit by itself doesn't change any behavior, right?

clang/lib/Frontend/FrontendActions.cpp
115

What's the + for?

182

Why is this the condition, as opposed to just "do this for all modules, don't do it for PCHs"? And doesn't BuildingImplicitModule imply isCompilingModule()? (Note that BuildingImplicitModule probably isn't the same as the original condition ImplicitModules.)

dexonsmith marked 2 inline comments as done.Mar 11 2019, 1:56 PM

This commit by itself doesn't change any behavior, right?

Correct, it just exposes an option.

clang/lib/Frontend/FrontendActions.cpp
115

Heh, I was trying to figure out those +s as well until I added this without one and it didn't compile. The problem is that bitfields can't be forwarded through the llvm::make_unique variadic. The + creates a local temporary with the same value.

182

(Note that BuildingImplicitModule probably isn't the same as the original condition ImplicitModules.)

Nice catch; I ported the logic from ASTWriter incorrectly. I'll fix this.

Why is this the condition, as opposed to just "do this for all modules, don't do it for PCHs"? And doesn't BuildingImplicitModule imply isCompilingModule()?

I was cargo-culting the logic for how PCHGenerator::HandleTranslationUnit sets the Module parameter to ASTWriter::WriteAST. I think that if I fix the above to match the original condition I'll need to check isCompilingModule()... but maybe BuildingImplicitModule is already &&-ing these together? I'll check.

Updated the constructor call to PCHGenerator in GenerateModuleAction::CreateASTConsumer to use BuildingImplicitModule on its own. Checking where it's set (only in compileModuleImpl), it's exactly the condition we want here.

dexonsmith marked 2 inline comments as done.Mar 11 2019, 8:34 PM
dexonsmith added inline comments.
clang/lib/Frontend/FrontendActions.cpp
182

Updated the patch to just use BuildingImplicitModule. The previous condition in PCHGenerator::HandleTranslationUnit seems to have been equivalent.

  • Previously in PCHGenerator::HandleTranslationUnit, WritingModule would be non-null only if we're currently building a module, as opposed to writing out a PCH. The logic to write to the in-memory cache also checked if LangOptions::ImplicitModules was set.
  • Now in GenerateModuleAction::CreateASTConsumer we check BuildingImplicitModule which is set in compileModuleImpl before executing the action.

I'm choosing this because it better matches the code around.

jordan_rose accepted this revision.Mar 12 2019, 10:38 AM
This revision is now accepted and ready to land.Mar 12 2019, 10:38 AM
dexonsmith closed this revision.Mar 12 2019, 11:37 AM

Committed in r355950. Thanks for the review.