[clangd] [Modules] Fix to correctly handle module dependencies #142828
Conversation
- 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.
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: None (fleeting-xx) ChangesThis 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
@ChuanqiXu9 for review Full diff: https://github.com/llvm/llvm-project/pull/142828.diff 1 Files Affected:
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));
}
|
|
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. |
|
Added the test but disabled it for |
| # RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp | ||
| # RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json | ||
| # RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp |
There was a problem hiding this comment.
Using sed for paths like this isn't portable, and I'm seeing failures on some internal windows bots because of it. The UNSUPPORTED here doesn't seem sufficient.
Notably, since on windows paths will include backslashes, a number of paths will be interpreted as having escape sequences, so we end up with either an error from the sed command saying we can't use that escape sequence there, or some kind of garbage in the replacement text.
There was a problem hiding this comment.
@bogner in case @fleeting-xx doesn't respond in time, would you like to fix these failures in windows by updating the UNSUPPORTED clause properly? I hope we can get this in 20.x. Thanks in ahead.
There was a problem hiding this comment.
We're seeing failures on Windows too: https://crbug.com/423658620
I've disabled the test in http://github.com/llvm/llvm-project/commit/5471d933af193eb2be4992428271cdcc7013599b
There was a problem hiding this comment.
@fleeting-xx I suspect the right medium to long term solution here would be to convert these tests to unit tests, as @ChuanqiXu9 suggested. This would avoid having to mess around with the config file using sed since all of that set up could be done programatically.
Thanks @zmodem for the short term fix!
There was a problem hiding this comment.
Sorry I missed this. I'll see about getting these moved to unit tests.
|
@fleeting-xx BTW, I think this worth to be backported to 20.x. But we need to fix the above problem first. |
The test fails (sometimes); see discussion on #142828
The test fails (sometimes); see discussion on llvm/llvm-project#142828
|
Failed to cherry-pick: 93b0bf6 https://github.com/llvm/llvm-project/actions/runs/15574087990 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
…142828) This is a re-application of llvm#142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies. - 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
The test fails (sometimes); see discussion on llvm#142828
|
FWIW, I sent #143647 |
…142828) This is a re-application of llvm#142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies. - 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.
The test fails (sometimes); see discussion on llvm#142828
…142828) This is a re-application of llvm#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
The test fails (sometimes); see discussion on llvm#142828
…142828) This is a re-application of llvm#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
…142828) This is a re-application of llvm#142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies. - 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.
The test fails (sometimes); see discussion on llvm#142828
The test fails (sometimes); see discussion on llvm/llvm-project#142828
The test fails (sometimes); see discussion on llvm#142828
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
@ChuanqiXu9 for review