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

[adamcz]

Summary

When preamble contains #undef, indexing code finds the matching #define
and uses that during indexing. However, it would only look for local
definitions. If the macro was defined in a module, MacroInfo
would be nullptr and clangd would crash.

This change fixes the crash by looking for definition in module when
that happens. The indexing result is then exactly the same whether
modules are used or not.

The indexing of macros happens for preamble only, so then #undef must be
in the preamble, which is why we need two .h files in a test.

Note that clangd is currently not ready for module support, but this
brings us one step closer.

[sammccall]

clang-tools-extra/clangd/test/symbols-modules.test

1

Generally, we prefer to write tests as gunit tests where feasible, this gathers the inputs into one place, avoids too much messing around with the filesystem, and assertions on something a little simpler than the full JSON output. It also make it easier to factor out common bits across tests.

If I'm understanding this test right it should be fairly easy to port to a gunit test with TestTU (setting AdditionalFiles and ExtraArgs) which would assert on headerSymbols(). This would probably go in SymbolCollectorTests, similar to e.g. TEST_F(SymbolCollectorTest, NonModularHeader).

Obviously this relates to where other tests around indexing modules might live once we have those. If you'd prefer them to be lit tests, it'd be nice to know a bit about that.

clang/include/clang/Lex/Preprocessor.h

1126

This looks fairly grungy and I don't totally understand it :-(
I guess walking the getPrevious() chain on the macro definition until we hit one with info doesn't work?
If we can't reuse something existing for this, we should probably ask rsmith if this is sensible.

API quibbles:

• it seems this only differs for modules, but the name doesn't mention modules
• maybe this should just be a "bool LookIntoModules" in getMacroInfo? it has only 3 callers.

clang/lib/Index/IndexingAction.cpp

155

You don't need to call getMacroInfo() here, getMacroInfoForLatestDefinition() does this.

[adamcz]

Addressed review comments.

[sammccall]

So this is late in the game but... maybe we should just not report this case as a reference?

#undef foo is valid if foo was never defined, and doesn't refer to anything. If we similarly don't resolve the reference in this case as we only import macros that were used and undef doesn't count as a use... I think that's defensible as a weird edge case.

In practice, I doubt anyone cares - AIUI the cases we've seen this, it's defensive claiming of the macro namespace, and not a targeted undef of a particular known macro at all.

[adamcz]

Changed to just ignore undefs without matching def

[adamcz]

Resurrecting this, with the ignore #undef when #define is in a module approach. Still worth having a test, especially that it's the first test we have for modules.

Note that the test code changed since last version due to introduction of ThreasafeFS since my original change.

[sammccall]

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

1509 ↗

(On Diff #274482)

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().)

1523 ↗

(On Diff #274482)

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::GenerateHeaderModuleActionto 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)

[adamcz]

final(?) review comments

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

1523 ↗

(On Diff #274482)

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?

[sammccall]

clang-tools-extra/clangd/unittests/TestFS.h

51 ↗

(On Diff #284360)

also ForModules here?

clang-tools-extra/clangd/unittests/TestTU.h

70 ↗

(On Diff #284360)

nit: "of implicit modules, where the module file is written to disk and later read back"

71 ↗

(On Diff #284360)

nit: I'm not sure the (V)FS should be involved at all in the final test setup.
The way I see it, the module storage is basically a cache from (module name + validity-stuff) ==> bytes, and *reads* could be abstracted just like writes.
For tests this could just be an in-memory map of course, but I think even in production we'd benefit a bit from not having to express this map through the filesystem. (E.g the way it's versioned doesn't have much to do with the way versioned filesystems are)

Only actionable thing for now is to maybe tweak this comment slightly, but wanted to mention it.

[adamcz]

var rename, comments

clang-tools-extra/clangd/unittests/TestTU.h

71 ↗

(On Diff #284360)

Updated comment, is this better?

[JesperAntonsson]

Hi,

In our local Jenkins machinery, the UndefOfModuleMacro test fails on master when built with gcc. Pasting the crash dump below. Any ideas?

BR /Jesper

12:24:39 FAIL: Clangd Unit Tests :: ./ClangdTests/SymbolCollectorTest.UndefOfModuleMacro (31492 of 65755)
12:24:39 **** TEST 'Clangd Unit Tests :: ./ClangdTests/SymbolCollectorTest.UndefOfModuleMacro' FAILED ****
12:24:39 Note: Google Test filter = SymbolCollectorTest.UndefOfModuleMacro
12:24:39 [==========] Running 1 test from 1 test case.
12:24:39 [----------] Global test environment set-up.
12:24:39 [----------] 1 test from SymbolCollectorTest
12:24:39 [ RUN ] SymbolCollectorTest.UndefOfModuleMacro
12:24:39 VFS: failed to set CWD to /clangd-test: No such file or directory
12:24:39 VFS: failed to set CWD to /clangd-test: No such file or directory
12:24:39 Built preamble of size 214456 for file /clangd-test/TestTU.cpp version null
12:24:39 VFS: failed to set CWD to /clangd-test: No such file or directory
12:24:39 #0 0x00000000009e175a llvm::sys::PrintStackTrace(llvm::raw_ostream&) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x9e175a)
12:24:39 #1 0x00000000009df764 llvm::sys::RunSignalHandlers() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x9df764)
12:24:39 #2 0x00000000009df8a3 SignalHandler(int) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x9df8a3)
12:24:39 #3 0x00007f4051202630 __restore_rt (/lib64/libpthread.so.0+0xf630)
12:24:39 #4 0x0000000000eb291c llvm::SimpleBitstreamCursor::Read(unsigned int) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xeb291c)
12:24:39 #5 0x000000000233a2fc llvm::SimpleBitstreamCursor::ReadVBR(unsigned int) [clone .constprop.173] ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x233a2fc)
12:24:39 #6 0x000000000233b9f3 llvm::BitstreamCursor::readRecord(unsigned int, llvm::SmallVectorImpl<unsigned long>&, llvm::StringRef*) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x233b9f3)
12:24:39 #7 0x0000000001797360 clang::ASTReader::ReadSLocEntry(int)::{lambda(llvm::BitstreamCursor&, llvm::StringRef)#1}::operator()(llvm::BitstreamCursor&, llvm::StringRef) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x1797360)
12:24:39 #8 0x00000000017c2511 clang::ASTReader::ReadSLocEntry(int) [clone .part.2799] ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0x17c2511)
12:24:39 #9 0x0000000000dd752d clang::SourceManager::loadSLocEntry(unsigned int, bool*) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xdd752d)
12:24:39 #10 0x0000000000dab66c clang::DiagnosticsEngine::DiagStateMap::lookup(clang::SourceManager&, clang::SourceLocation) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xdab66c)
12:24:39 #11 0x0000000000dae275 clang::DiagnosticIDs::getDiagnosticSeverity(unsigned int, clang::SourceLocation, clang::DiagnosticsEngine const&) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xdae275)
12:24:39 #12 0x0000000000dae725 clang::DiagnosticIDs::getDiagnosticLevel(unsigned int, clang::SourceLocation, clang::DiagnosticsEngine const&) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xdae725)
12:24:39 #13 0x0000000000daf181 clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&) const ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xdaf181)
12:24:39 #14 0x0000000000da50ab clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50ab)
12:24:39 #15 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
12:24:39 #16 0x0000000000da50cb clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50cb)
12:24:39 #17 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
12:24:39 #18 0x0000000000da50cb clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50cb)
12:24:39 #19 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
12:24:39 #20 0x0000000000da50cb clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50cb)
12:24:39 #21 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
[...]
12:24:39 #252 0x0000000000da50cb clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50cb)
12:24:39 #253 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
12:24:39 #254 0x0000000000da50cb clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda50cb)
12:24:39 #255 0x0000000000da546c clang::DiagnosticsEngine::ReportDelayed() ([...]/tools/clang/tools/extra/clangd/unittests/./ClangdTests+0xda546c)
12:24:39
12:24:39 ****
12:24:39 Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
12:29:50
12:29:50 1 warning(s) in tests
12:29:50 ****
12:29:50 Failed Tests (1):
12:29:50 Clangd Unit Tests :: ./ClangdTests/SymbolCollectorTest.UndefOfModuleMacro
12:29:50
12:29:50
12:29:50 Testing Time: 636.08s
12:29:50 Unsupported : 502
12:29:50 Passed : 65067
12:29:50 Expectedly Failed: 185
12:29:50 Failed : 1
12:29:51 FAILED: CMakeFiles/check-all

[adamcz]

This is quite strange. I can't reproduce this with gcc 7.5.0, 9.3.0 or 10.0.1. Which version are you using?

There are bugs around clangd + modules + diagnostics. I'm fixing one in https://reviews.llvm.org/D85753 (not in it's final form yet), but it doesn't seem to be the same issue (can't be sure, but I think this is using the right SourceManager). This test is not supposed to generate any diagnostics, so that's the really surprising bit. Are you building from clean HEAD, or do you have any local modifications that could generate a extra diagnostics in this test?

The other option is that the failure is caused by module_cache_path. This test, unlike other clangd tests, needs real file system (to write the .pcm file and then read it back) and perhaps that doesn't work in your jenkins setup.

Are there any special options or things unique to your setup I could try out to reproduce this?

[bjope]

In D80525#2215641, @adamcz wrote:

This is quite strange. I can't reproduce this with gcc 7.5.0, 9.3.0 or 10.0.1. Which version are you using?

There are bugs around clangd + modules + diagnostics. I'm fixing one in https://reviews.llvm.org/D85753 (not in it's final form yet), but it doesn't seem to be the same issue (can't be sure, but I think this is using the right SourceManager). This test is not supposed to generate any diagnostics, so that's the really surprising bit. Are you building from clean HEAD, or do you have any local modifications that could generate a extra diagnostics in this test?

The other option is that the failure is caused by module_cache_path. This test, unlike other clangd tests, needs real file system (to write the .pcm file and then read it back) and perhaps that doesn't work in your jenkins setup.

Are there any special options or things unique to your setup I could try out to reproduce this?

We actually have had problems reproducing it outside jenkins ourselves as well. That is kind of why we were a bit clueless about what to do and what might be causing it.

Are those VFS: failed to set CWD to /clangd-test: No such file or directory outputs expected?

[bjope]

Might be that it is some other commit that cause the problem. Looking a bit closer at the jenkins history it seems like the tests passed once after this patch was added. But the next time it failed.

That second time the "[Diagnoostics] Reworked -Wstring-concatenation" commit had been pushed as well (commit b9af72bffe). And I see that there are some string concatenation going on in the test case.

[adamcz]

In D80525#2215755, @bjope wrote:

In D80525#2215641, @adamcz wrote:

This is quite strange. I can't reproduce this with gcc 7.5.0, 9.3.0 or 10.0.1. Which version are you using?

There are bugs around clangd + modules + diagnostics. I'm fixing one in https://reviews.llvm.org/D85753 (not in it's final form yet), but it doesn't seem to be the same issue (can't be sure, but I think this is using the right SourceManager). This test is not supposed to generate any diagnostics, so that's the really surprising bit. Are you building from clean HEAD, or do you have any local modifications that could generate a extra diagnostics in this test?

The other option is that the failure is caused by module_cache_path. This test, unlike other clangd tests, needs real file system (to write the .pcm file and then read it back) and perhaps that doesn't work in your jenkins setup.

Are there any special options or things unique to your setup I could try out to reproduce this?

We actually have had problems reproducing it outside jenkins ourselves as well. That is kind of why we were a bit clueless about what to do and what might be causing it.

Are those VFS: failed to set CWD to /clangd-test: No such file or directory outputs expected?

Yes, they are harmless. InMemoryFS uses fake paths like /clangd-test, but the real file system can't follow that. The real file system is supposed to be used for module cache only.

If you can't reproduce this locally, but can on Jenkins that makes me think it's definitely something with filesystem. I suppose running strace on jenkins to log all FS access is not easy?

Maybe try to figure out what the effective ModuleCachePath is and whether that makes sense (i.e. is accessible) in your Jenkins setup? I can't think of a way to make clangd test log it, but you can just:

llvm::errs() << ModuleCachePath << "\n"; llvm::errs().flush();

as last line of setModuleCachePath() in clang/lib/clang/Lex/HeaderSearch.h (around line 325) to see what path it's using. There's probably a much smarter way to do this, but I don't know it.

This is still a random guess that it's something to do with file system.

[adamcz]

Looks like it's an issue with not clearing the module cache. I'll revert this change to fix the build, then re-submit with proper fix.

[bjope]

In D80525#2215778, @adamcz wrote:

Looks like it's an issue with not clearing the module cache. I'll revert this change to fix the build, then re-submit with proper fix.

Ok, thanks. Let me know if you want me to investigate further on my side (such as adding more logging in those jenkins jobs).

clang-tools-extra/clangd/test/Inputs/symbols-modules/bar.h

+#include "foo.h"
+
+#undef X

clang-tools-extra/clangd/test/Inputs/symbols-modules/compile_commands.json

+[{
+  "directory": "DIRECTORY",
+  "command": "clang -fmodules -Xclang -fmodule-map-file-home-is-cwd -fmodule-map-file=module.map foo.cpp",
+  "file": "DIRECTORY/foo.cpp"
+}]

clang-tools-extra/clangd/test/Inputs/symbols-modules/foo.h

+#define X 1

clang-tools-extra/clangd/test/Inputs/symbols-modules/module.map

+module foo {
+  header "foo.h"
+  export *
+}

clang-tools-extra/clangd/test/symbols-modules.test

+# Verify that we do not crash and correctly find the definition of a macro that
+# was imported from a module and then #undef-ed in preamble.
+#
+# Copy over the test files and fix the paths.
+# RUN: rm -rf %t
+# RUN: cp -r %S/Inputs/symbols-modules %t
+# RUN: sed -i -e "s|DIRECTORY|%t|" %t/compile_commands.json
+#
+# Fix the paths in this file as well. We will pass this copy to clangd.
+# RUN: sed -e "s|DIRECTORY|%/t|g" %s > %t.test.1
+#
+# RUN: clangd -lit-test < %t.test.1 | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]}}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://DIRECTORY/foo.cpp","languageId":"cpp","version":1,"text":"#include \"bar.h\"\nvoid foo();\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"X"}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:    "result": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "containerName": "",
+# CHECK-NEXT:        "kind": 15,
+# CHECK-NEXT:        "location": {
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": {{.*}},
+# CHECK-NEXT:              "line": {{.*}}
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": {{.*}},
+# CHECK-NEXT:              "line": {{.*}}
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "uri": "file://{{.*}}/foo.h"
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "name": "X"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}

clang/include/clang/Lex/Preprocessor.h

public:
   MacroInfo *getMacroInfo(const IdentifierInfo *II) {
     if (!II->hasMacroDefinition())
       return nullptr;
     if (auto MD = getMacroDefinition(II))
       return MD.getMacroInfo();
     return nullptr;
   }
 
+  // Returns MacroInfo for the latest definition of II. It could possibly be
+  // imported from a module (thus not accesible via getMacroInfo()), whether
+  // it's active or not.
+  MacroInfo *getMacroInfoForLatestDefinition(const IdentifierInfo *II) {
+    if (!II->hasMacroDefinition())
+      return nullptr;
+    if (auto *MI = getMacroInfo(II))
+      return MI;
+    MacroState &S = CurSubmoduleState->Macros[II];
+    auto ActiveModuleMacros = S.getActiveModuleMacros(*this, II);
+    for (auto MM = ActiveModuleMacros.rbegin(); MM != ActiveModuleMacros.rend();
+         MM++) {
+      if (auto *MI = (*MM)->getMacroInfo())
+        return MI;
+    }
+    auto OverridenMacros = S.getOverriddenMacros();
+    for (auto MM = OverridenMacros.rbegin(); MM != OverridenMacros.rend();
+         MM++) {
+      if (auto *MI = (*MM)->getMacroInfo())
+        return MI;
+    }
+    return nullptr;
+  }
+
   /// Given an identifier, return the latest non-imported macro
   /// directive for that identifier.
   ///
   /// One can iterate over all previous macro directives from the most recent
   /// one.
   MacroDirective *getLocalMacroDirectiveHistory(const IdentifierInfo *II) const;
 
   /// Add a directive to the macro directive history for this identifier.

clang/lib/Index/IndexingAction.cpp

static bool topLevelDeclVisitor(void *context, const Decl *D) {
   IndexingContext &IndexCtx = *static_cast<IndexingContext*>(context);
   return IndexCtx.indexTopLevelDecl(D);
 }
 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IndexCtx) {
   Unit.visitLocalTopLevelDecls(&IndexCtx, topLevelDeclVisitor);
 }
 
-static void indexPreprocessorMacros(const Preprocessor &PP,
+static void indexPreprocessorMacros(Preprocessor &PP,
                                     IndexDataConsumer &DataConsumer) {
-  for (const auto &M : PP.macros())
-    if (MacroDirective *MD = M.second.getLatest())
+  for (const auto &M : PP.macros()) {
+    if (MacroDirective *MD = M.second.getLatest()) {
+      auto *MI = MD->getMacroInfo();
+      if (!MI) {
+        // It is possible for MI to be nullptr if we our translation unit had
+        // only #undef of a macro defined in a module. In that case, dig deeper
+        // and use the definition from the module to yield the same indexing
+        // result whether we are using modules or not.
+        MI = PP.getMacroInfoForLatestDefinition(M.first);
+      }
       DataConsumer.handleMacroOccurrence(
-          M.first, MD->getMacroInfo(),
-          static_cast<unsigned>(index::SymbolRole::Definition),
+          M.first, MI, static_cast<unsigned>(index::SymbolRole::Definition),
           MD->getLocation());
     }
+  }
+}
 
 void index::indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
                          IndexingOptions Opts) {
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Unit.getASTContext());
   DataConsumer.initialize(Unit.getASTContext());
   DataConsumer.setPreprocessor(Unit.getPreprocessorPtr());