llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) <details> <summary>Changes</summary> …ule files When compile_commands.json contains relative paths in -fmodule-file= arguments (as generated by CMake), clangd failed to find the BMI files because it was looking for them relative to the wrong working directory. This patch fixes the issue by converting relative paths to absolute paths based on the compilation directory (CompileCommand.Directory) before checking if the module file exists and is up to date. Added a unit test that verifies the fix works correctly. AI Assisted --- Full diff: https://github.com/llvm/llvm-project/pull/187654.diff 2 Files Affected: - (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+12-3) - (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+84) ``````````diff diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 950392b91ac7c..ce57bf3c0a60e 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -584,12 +584,21 @@ void ModulesBuilder::ModulesBuilderImpl::getPrebuiltModuleFile( if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName)) continue; - if (IsModuleFileUpToDate(ModuleFilePath, BuiltModuleFiles, + // Convert relative path to absolute path based on the compilation directory + llvm::SmallString<256> AbsoluteModuleFilePath; + if (llvm::sys::path::is_relative(ModuleFilePath)) { + AbsoluteModuleFilePath = Inputs.CompileCommand.Directory; + llvm::sys::path::append(AbsoluteModuleFilePath, ModuleFilePath); + } else { + AbsoluteModuleFilePath = ModuleFilePath; + } + + if (IsModuleFileUpToDate(AbsoluteModuleFilePath, BuiltModuleFiles, TFS.view(std::nullopt))) { log("Reusing prebuilt module file {0} of module {1} for {2}", - ModuleFilePath, ModuleName, ModuleUnitFileName); + AbsoluteModuleFilePath, ModuleName, ModuleUnitFileName); BuiltModuleFiles.addModuleFile( - PrebuiltModuleFile::make(ModuleName, ModuleFilePath)); + PrebuiltModuleFile::make(ModuleName, AbsoluteModuleFilePath)); } } } diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index 3132959a5967c..1791906191960 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -17,6 +17,7 @@ #include "ModulesBuilder.h" #include "ScanningProjectModules.h" #include "TestTU.h" +#include "support/Path.h" #include "support/ThreadsafeFS.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/FileSystem.h" @@ -673,6 +674,89 @@ import M; EXPECT_EQ(HS.PrebuiltModuleFiles, HS2.PrebuiltModuleFiles); } +// Test that prebuilt module files with relative paths are correctly resolved. +// This tests the fix for the issue where clangd couldn't find BMI files when +// the compilation database contained relative paths in -fmodule-file= arguments. +TEST_F(PrerequisiteModulesTests, PrebuiltModuleFileWithRelativePath) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("M.cppm", R"cpp( +export module M; +export int m_value = 42; + )cpp"); + + CDB.addFile("U.cpp", R"cpp( +import M; +int use() { return m_value; } + )cpp"); + + // Step 1: Build the module file using ModulesBuilder + ModulesBuilder Builder(CDB); + auto ModuleInfo = + Builder.buildPrerequisiteModulesFor(getFullPath("U.cpp"), FS); + ASSERT_TRUE(ModuleInfo); + + HeaderSearchOptions HS(TestDir); + ModuleInfo->adjustHeaderSearchOptions(HS); + ASSERT_EQ(HS.PrebuiltModuleFiles.count("M"), 1u); + + // Get the absolute path of the built module file + std::string OriginalBMPath = HS.PrebuiltModuleFiles["M"]; + ASSERT_TRUE(llvm::sys::path::is_absolute(OriginalBMPath)); + ASSERT_TRUE(llvm::sys::fs::exists(OriginalBMPath)); + + // Step 2: Create a subdirectory in TestDir and copy the BMI there + SmallString<256> BMSubDir(TestDir); + llvm::sys::path::append(BMSubDir, "prebuilt_modules"); + ASSERT_FALSE(llvm::sys::fs::create_directories(BMSubDir)); + + SmallString<256> NewBMPath(BMSubDir); + llvm::sys::path::append(NewBMPath, "M.pcm"); + + // Copy the BMI file to the new location + ASSERT_FALSE(llvm::sys::fs::copy_file(OriginalBMPath, NewBMPath)); + ASSERT_TRUE(llvm::sys::fs::exists(NewBMPath)); + + // Step 3: Create a relative path from the new absolute path + std::string RelativeBMPath = + llvm::StringRef(NewBMPath).drop_front(TestDir.size() + 1).str(); + ASSERT_FALSE(RelativeBMPath.empty()); + ASSERT_TRUE(llvm::sys::path::is_relative(RelativeBMPath)); + + // Step 4: Create a new CDB with relative path in -fmodule-file= + MockDirectoryCompilationDatabase CDBWithRelativePath(TestDir, FS); + + CDBWithRelativePath.addFile("M.cppm", R"cpp( +export module M; +export int m_value = 42; + )cpp"); + + CDBWithRelativePath.addFile("U.cpp", R"cpp( +import M; +int use() { return m_value; } + )cpp"); + + // Use relative path in -fmodule-file= argument + CDBWithRelativePath.ExtraClangFlags.push_back( + "-fmodule-file=M=" + RelativeBMPath); + + // Step 5: Verify that clangd can find and reuse the prebuilt module file + ModulesBuilder BuilderWithRelativePath(CDBWithRelativePath); + auto ModuleInfo2 = BuilderWithRelativePath.buildPrerequisiteModulesFor( + getFullPath("U.cpp"), FS); + ASSERT_TRUE(ModuleInfo2); + + HeaderSearchOptions HS2(TestDir); + ModuleInfo2->adjustHeaderSearchOptions(HS2); + + // The module file should be found and the paths should match + ASSERT_EQ(HS2.PrebuiltModuleFiles.count("M"), 1u); + EXPECT_EQ(HS2.PrebuiltModuleFiles["M"], std::string(NewBMPath)) + << "Expected absolute path: " << NewBMPath + << "\nGot: " << HS2.PrebuiltModuleFiles["M"] + << "\nRelative path used: " << RelativeBMPath; +} + } // namespace } // namespace clang::clangd `````````` </details> https://github.com/llvm/llvm-project/pull/187654 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
