Page MenuHomePhabricator

[clang-scan-deps] Add basic support for Clang modules.
ClosedPublic

Authored by Bigcheese on Oct 10 2019, 2:42 PM.

Details

Summary

This fixes two issues that prevent simple uses of Clang modules from working.

  • We would previously minimize _every_ file opened by clang, even module maps and module pcm files. Now we only minimize files with known extensions. It would be better if we knew which files clang intended to open as a source file, but this works for now.
  • We previously cached every lookup, even failed lookups. This is a problem because clang stats the module cache directory before building a module and creating that directory. If we cache that failure then the subsequent pcm load doesn't see the module cache and fails.

Overall this still leaves us building minmized modules on disk during scanning.
This will need to be improved eventually for performance, but this is correct,
and works for now.

Diff Detail

Event Timeline

Bigcheese created this revision.Oct 10 2019, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 2:42 PM
arphaman added inline comments.Oct 10 2019, 2:49 PM
lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
135 ↗(On Diff #224484)

Let me do some experiments to find files in our codebase that have some weird extensions we should minimize as well, to check what else could be there.

test/ClangScanDeps/Inputs/modules_cdb.json
4 ↗(On Diff #224484)

You probably want to put in -fimplicit-modules explicitly as the driver might not infer it outside of Darwin.

test/ClangScanDeps/modules.cpp
39 ↗(On Diff #224484)

Missing windows path slash thing.

Bigcheese updated this revision to Diff 224507.Oct 10 2019, 5:25 PM
Bigcheese marked 2 inline comments as done.

Addressed review comments.

dexonsmith added inline comments.Oct 10 2019, 7:32 PM
lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
135 ↗(On Diff #224484)

I think we should get the standard preprocessed source extensions for completeness, even though it would be rare for someone to send those in. IIRC, .i, .ii, .mi, and .mmi (not sure about that last one).

Bigcheese updated this revision to Diff 225311.Wed, Oct 16, 2:56 PM

Added .i, .ii, .mi, and .mmi as files to minimize.

Bigcheese marked an inline comment as done.Wed, Oct 16, 2:56 PM
arphaman accepted this revision.Thu, Oct 24, 10:49 AM

@Bigcheese I don't have time right now to do the build experiments, so I'll leave it as follow-up for me to resolve later. LGTM.

This revision is now accepted and ready to land.Thu, Oct 24, 10:49 AM
This revision was automatically updated to reflect the committed changes.