Page MenuHomePhabricator

[clang][modules] Add -cc1 option to backup PCM files
Changes PlannedPublic

Authored by jansvoboda11 on May 3 2021, 7:14 AM.

Details

Summary

During a highly parallel build with -fimplicit-modules, multiple translation units may generate a PCM file for the same module at the same filesystem path in a short sequence.

When the first Clang instance tries to verify signature of the module on import, it discovers the new version of the PCM file (produced by latter Clang instance) that may have a different signature, leading to a mismatch and failed build.

To be able to debug such mismatch, it's invaluable to be able to compare the latter PCM (on disk) with the first one (overwritten).

This patch adds new -cc1 option -fbackup-module that tells Clang to store each PCM to the normal location and also to a path with an unique suffix (the PID of the Clang instance).

This is mostly additional change, but PCHContainerGenerator now doesn't unconditionally destroy the given PCHBuffer anymore and instead just decreases its reference count.

Diff Detail

Unit TestsFailed

TimeTest
580 msx64 windows > Clang.Modules::backup-module.c
Script: -- : 'RUN: at line 1'; rm -rf C:\ws\w32-1\llvm-project\premerge-checks\build\tools\clang\test\Modules\Output\backup-module.c.tmp && mkdir C:\ws\w32-1\llvm-project\premerge-checks\build\tools\clang\test\Modules\Output\backup-module.c.tmp

Event Timeline

jansvoboda11 created this revision.May 3 2021, 7:14 AM
jansvoboda11 requested review of this revision.May 3 2021, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 7:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

A few high-level comments:

  • Something like this *does* seem really useful for debugging problems with implicit module builds using a fuzzy context hash. But I feel like the use case is carved out at the wrong place; I feel like we either want this to be:
    • more restrictive: only supported for implicit+fuzzy modules, so it's not misused; or
    • more general: supported for all output files (CompilerInstance::createOutputFile), to provide a debugging option in other volatile filesystem situations.
  • Changing the file extension doesn't seem ideal; it'd be better to embed the PID (or whatever it is) before the extension.
    • I'm not sure I love adding the pid to the filename at all TBH; although I'm not sure what'd be better.
  • From a file-handling level, there's definite crossover with temporary files (write to temporary location, then move atomically into place). It might be cleaner to:
    • Write to the temp location, copy the temp to the .pid location, and move the temp to the final location.
    • Write to the unique/uncontended location (as the new temporary location), then copy the file (e.g., using llvm::copy_file) to the final location. I don't know if llvm::copy_file is atomic though...
jansvoboda11 planned changes to this revision.Fri, May 21, 8:44 AM