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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1509
+TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
+  auto TU = TestTU::withCode(R"cpp(#include "/bar.h")cpp");
+  TU.AdditionalFiles["/bar.h"] = R"cpp(
----------------
are the leading slashes here needed, or can we use "bar.h" and have everything be relative to testRoot()?
(relative paths in AdditionalFiles are relative to testRoot().)


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1523
+  TU.ExtraArgs.push_back("-fmodule-map-file=/module.map");
+  TU.OverlayRealFileSystem = true;
+
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80525