I guess it was originally done this way because we have a number of DependencyScanning{Tool,Worker} instances

LGTM. The vfsroot-include.c is a -cc1 invocation, so it could probably be switched to use -verify instead of FileCheck'ing the output, unless there's something I'm missing. But that doesn't need to be for this patch.

Since perf impact is minimal, LGTM.

In D143613#4115857, @jansvoboda11 wrote:

LGTM, but would be nice to have a test for this.

Add a test

I think we should verify the performance impact to running tests is minimal before landing this, like they did with the _GLIBCXX_ASSERTIONS change. Also CCing @ldionne in case he has opinions on us using libc++ assertions. Otherwise LGTM.

• Improved doc comment for RoundTrip (mostly followed the suggested text; also converted from to /).
• Renamed variables
• Moved default value for RoundTripArgs

+1 for determinism. Okay I spent some time trying to understand how this code is used and the non-virtual paths make some sense now. I am a bit skeptical about this on-disk-hash-table by filepath but that's separate from this patch.

This should allow the path serialization of input files to use the paths used when looking up a file entry, instead of the last reference.

Dupe of https://reviews.llvm.org/D143428 ?

In D142780#4087250, @rmaz wrote:

In that case we are seeing non-deterministic header paths serialized in pcm files. IIUC the header files are serialized based in their unique ID, so it wouldn't be possible to handle this case, is this right?

Iterating over SeenFileEntries and skipping VirtualFileEntries looks good to me.

Removing ImplicitModulePCMPath LGTM; not sure about the other change.

I'm not that familiar with development practices for libunwind, but is possible to write a test for this?

Please add a comment to each place you're doing this since it's non-obvious why the filename would be different to people not familiar with the mapping code. Otherwise LGTM

Sorry for missing these. I think I addressed all of them now.

Re-added a few of my minor comments that I think got lost on a specific version of the diff. Otherwise LGTM.

libclang changes LGTM, thanks! I'll let the other reviewers who have domain expertise speak for the rest of the patch.

Thanks, the libclang change LGTM other than my comment about the function names.

All my comments are addressed, thanks!

Thanks for the explanation, make sense. LGTM with the documentation tweak!

If the original HeaderSearchOptions didn't have any VFS overlay files, adopt the PCM ones.

Please add a comment about why this equality check is necessary.

LGTM once the test failure is fixed

• Removed outdated FIXME
• Ran clang-format
• Attempt to fix Windows test failure: the StringSet of paths was failing to match, so switch to DenseSet<FileEntry *>. Note: the path that ends up in the command-line is from a FileEntryRef, but the equality of FileEntry * is how we check whether to include it at all. For now we just re-lookup the paths which should always hit the path cache in FIleManager. A future improvement could be to track FileEntryRef for the module map paths directly instead of strings.

Thanks for the timely review!

In D135220#3899674, @dexonsmith wrote:

In D135220#3898849, @hans wrote:

Relatedly, we ran into a problem with clang-cl /showIncludes not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726

Interestingly, that discussion points out that -MD works correctly: