[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1523
+  TU.ExtraArgs.push_back("-fmodule-map-file=/module.map");
+  TU.OverlayRealFileSystem = true;
+
----------------
sammccall wrote:
> I'm a bit torn on this - this is obviously a bit messy (writing modules to the real filesystem and exposing the whole FS to our test).
> It's not a huge thing and we could slap a FIXME on it, though you clearly have to plumb it through a few layers.
> 
> I think the alternative is an explicit module build:
>  - subclass `clang::GenerateHeaderModuleAction`to override the output so it ends up in a buffer
>  - add a method to TestTU like buildHeaderModule() that returns a `std::string` containing the module content
>  - This test would have two TestTU instances, and the second looks something like:
> ```
> TU2.AdditionalFiles["foo.pcm"] = TU1.buildHeaderModule();
> TU2.ExtraArgs = {"-fprebuild-module-path=.", "-std=c++20", "-fmodules"};
> TU2.HeaderCode = "import foo;\n#undef X";
> ```
> 
> I guess whether this is actually better probably depends on whether we're likely to go in this direction (explicitly building and managing a module cache ourselves) for modules. If we're likely to let clang implicitly build modules and maybe just put the physical cache storage behind an abstraction, maybe it's best to overlay the FS for now and rip this out later. If we're going to do explicit module builds and schedule them ourselves, then starting to experiment in that direction is useful, and there won't be an obvious time to rip out the OverlayRealFileSystem feature.
> 
> I think all of which is to say feel free to land it in this form, or experiment further.
> I think the OverlayRealFileSystem should maybe be `ExposeModuleCacheDir` or something with a FIXME to come up with a better solution, though. It's not a feature of TesttU we should be using for other purposes (without similar careful consideration)
I think we should support implicit modules in general. It's a useful feature. This means we should be able to test it. While it might not matter for this particular fix, we need some implicit modules tests to prevent regressions.

We should also have explicit modules tests, of course. All of that is coming soon :-)

Ultimately, I think a proper solution is to either:
a. Support writing to disk in VFS, then implement that in InMemoryFS.
b. Create abstraction around writing implicitly compiled module to disk, intercept that in test and update the memory FS.

Both seem acceptable to me, although I haven't looked at details yet. I think for now, we should just accept the sad fact that some tests will write to disk. It should be easy to remove it later.

I added a FIXME and renamed to OverlayRealFilesystemForModules to make it clear it should not be used for anything else. Does that sound good to you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80525/new/

https://reviews.llvm.org/D80525