[clangd] [Modules] Fix to correctly handle module dependencies

[fleeting-xx]

This is a re-application of #142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies.

Changes

• Fix dangling string references in the return value of getAllRequiredModules()
• Change a couple of calls in getOrBuildModuleFile() to use the loop variable instead of the ModuleName parameter.

@ChuanqiXu9 for review

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: None (fleeting-xx)

Changes

This is a re-application of llvm/llvm-project#142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies.

Changes

• Fix dangling string references in the return value of getAllRequiredModules()
• Change a couple of calls in getOrBuildModuleFile() to use the loop variable instead of the ModuleName parameter.

@ChuanqiXu9 for review

---

Full diff: https://github.com/llvm/llvm-project/pull/142828.diff

1 Files Affected:

• (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+12-10)

diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index bf77f43bd28bb..d88aa01aad05d 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -430,10 +430,10 @@ class CachingProjectModules : public ProjectModules {
 /// Collect the directly and indirectly required module names for \param
 /// ModuleName in topological order. The \param ModuleName is guaranteed to
 /// be the last element in \param ModuleNames.
-llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource,
-                                                   CachingProjectModules &MDB,
-                                                   StringRef ModuleName) {
-  llvm::SmallVector<llvm::StringRef> ModuleNames;
+llvm::SmallVector<std::string> getAllRequiredModules(PathRef RequiredSource,
+                                                     CachingProjectModules &MDB,
+                                                     StringRef ModuleName) {
+  llvm::SmallVector<std::string> ModuleNames;
   llvm::StringSet<> ModuleNamesSet;
 
   auto VisitDeps = [&](StringRef ModuleName, auto Visitor) -> void {
@@ -444,7 +444,7 @@ llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource,
       if (ModuleNamesSet.insert(RequiredModuleName).second)
         Visitor(RequiredModuleName, Visitor);
 
-    ModuleNames.push_back(ModuleName);
+    ModuleNames.push_back(ModuleName.str());
   };
   VisitDeps(ModuleName, VisitDeps);
 
@@ -494,13 +494,13 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
   // Get Required modules in topological order.
   auto ReqModuleNames = getAllRequiredModules(RequiredSource, MDB, ModuleName);
   for (llvm::StringRef ReqModuleName : ReqModuleNames) {
-    if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
+    if (BuiltModuleFiles.isModuleUnitBuilt(ReqModuleName))
       continue;
 
     if (auto Cached = Cache.getModule(ReqModuleName)) {
       if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles,
                                TFS.view(std::nullopt))) {
-        log("Reusing module {0} from {1}", ModuleName,
+        log("Reusing module {0} from {1}", ReqModuleName,
             Cached->getModuleFilePath());
         BuiltModuleFiles.addModuleFile(std::move(Cached));
         continue;
@@ -508,14 +508,16 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
       Cache.remove(ReqModuleName);
     }
 
+    std::string ReqFileName =
+        MDB.getSourceForModuleName(ReqModuleName, RequiredSource);
     llvm::Expected<ModuleFile> MF = buildModuleFile(
-        ModuleName, ModuleUnitFileName, getCDB(), TFS, BuiltModuleFiles);
+        ReqModuleName, ReqFileName, getCDB(), TFS, BuiltModuleFiles);
     if (llvm::Error Err = MF.takeError())
       return Err;
 
-    log("Built module {0} to {1}", ModuleName, MF->getModuleFilePath());
+    log("Built module {0} to {1}", ReqModuleName, MF->getModuleFilePath());
     auto BuiltModuleFile = std::make_shared<const ModuleFile>(std::move(*MF));
-    Cache.add(ModuleName, BuiltModuleFile);
+    Cache.add(ReqModuleName, BuiltModuleFile);
     BuiltModuleFiles.addModuleFile(std::move(BuiltModuleFile));
   }
 

[ChuanqiXu9]

I think you should add the test for clang-tools-extra/clangd/test/module_dependencies.test in the original patch and disable it for Windows.

[fleeting-xx]

Added the test but disabled it for host=aarch64-pc-windows-msvc instead of system-windows. That will disable it specifically for the platform/build server it wasn't working on.

[ChuanqiXu9]

LGTM then. Thanks.

BTW, clangd prefer unittests than lit test. So you'd better to use unittests next time.