llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Chuanqi Xu (ChuanqiXu9) <details> <summary>Changes</summary> The IsModuleFileUpToDate function was not properly validating input files for C++20 modules. By default, ASTReader skips input file validation for StandardCXXModule files unless ForceCheckCXX20ModulesInputFiles and ValidateASTInputFilesContent are both set. This change: - Passes ValidateASTInputFilesContent=true to ASTReader constructor - Uses ARR_OutOfDate flag for cleaner error handling - Simplifies the validation logic (ReadAST already validates internally) - Adds a test to verify header changes in module units are detected Assised with AI. --- Full diff: https://github.com/llvm/llvm-project/pull/187653.diff 2 Files Affected: - (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+16-17) - (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+70) ``````````diff diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 950392b91ac7c..387fe7ce0895a 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -269,28 +269,27 @@ bool IsModuleFileUpToDate(PathRef ModuleFilePath, PCHContainerOperations PCHOperations; CodeGenOptions CodeGenOpts; ASTReader Reader(PP, *ModCache, /*ASTContext=*/nullptr, - PCHOperations.getRawReader(), CodeGenOpts, {}); + PCHOperations.getRawReader(), CodeGenOpts, {}, + /*isysroot=*/"", + /*DisableValidationKind=*/DisableValidationForModuleKind::None, + /*AllowASTWithCompilerErrors=*/false, + /*AllowConfigurationMismatch=*/false, + /*ValidateSystemInputs=*/false, + /*ForceValidateUserInputs=*/true, + /*ValidateASTInputFilesContent=*/true); // We don't need any listener here. By default it will use a validator // listener. Reader.setListener(nullptr); - if (Reader.ReadAST(ModuleFileName::makeExplicit(ModuleFilePath), - serialization::MK_MainFile, SourceLocation(), - ASTReader::ARR_None) != ASTReader::Success) - return false; - - bool UpToDate = true; - Reader.getModuleManager().visit([&](serialization::ModuleFile &MF) -> bool { - Reader.visitInputFiles( - MF, /*IncludeSystem=*/false, /*Complain=*/false, - [&](const serialization::InputFile &IF, bool isSystem) { - if (!IF.getFile() || IF.isOutOfDate()) - UpToDate = false; - }); - return !UpToDate; - }); - return UpToDate; + // Use ARR_OutOfDate so that ReadAST returns OutOfDate instead of Failure + // when input files are modified. This allows us to detect staleness + // without treating it as a hard error. + // ReadAST will validate all input files internally and return OutOfDate + // if any file is modified. + return Reader.ReadAST(ModuleFileName::makeExplicit(ModuleFilePath), + serialization::MK_MainFile, SourceLocation(), + ASTReader::ARR_OutOfDate) == ASTReader::Success; } bool IsModuleFilesUpToDate( diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index 3132959a5967c..7cda291d3d3b7 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -644,6 +644,76 @@ import M; EXPECT_EQ(CDB.getGlobalScanningCount(), 1u); } +// Test that canReuse detects changes to headers included in module units. +// This verifies that the ASTReader correctly tracks header file dependencies +// in BMI files and that IsModuleFileUpToDate correctly validates them. +TEST_F(PrerequisiteModulesTests, CanReuseWithHeadersInModuleUnit) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + // Create a header file that will be included in a module unit + CDB.addFile("header1.h", R"cpp( +inline int getValue() { return 42; } + )cpp"); + + // Module M includes header1.h in the global module fragment + CDB.addFile("M.cppm", R"cpp( +module; +#include "header1.h" +export module M; +export int m_value = getValue(); + )cpp"); + + // Module N imports M (similar structure to ReusabilityTest) + CDB.addFile("N.cppm", R"cpp( +export module N; +import :Part; +import M; + )cpp"); + + // Add a module partition (similar to ReusabilityTest) + CDB.addFile("N-part.cppm", R"cpp( +export module N:Part; + )cpp"); + + ModulesBuilder Builder(CDB); + + // Build prerequisite modules for N (which depends on M) + auto NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS); + EXPECT_TRUE(NInfo); + + ParseInputs NInput = getInputs("N.cppm", CDB); + std::unique_ptr<CompilerInvocation> Invocation = + buildCompilerInvocation(NInput, DiagConsumer); + + // Initially, canReuse should return true + EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir))); + + // Test 1: Modify header1.h (included by M) + // canReuse should detect this change since M's BMI records header1.h as input + CDB.addFile("header1.h", R"cpp( +inline int getValue() { return 43; } + )cpp"); + EXPECT_FALSE(NInfo->canReuse(*Invocation, FS.view(TestDir))); + + // Rebuild and verify canReuse returns true again + NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS); + EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir))); + + // Test 2: Modify the module source file itself + CDB.addFile("M.cppm", R"cpp( +module; +#include "header1.h" +export module M; +export int m_value = getValue(); +export int m_new_value = 10; + )cpp"); + EXPECT_FALSE(NInfo->canReuse(*Invocation, FS.view(TestDir))); + + // Rebuild after module source change + NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS); + EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir))); +} + TEST_F(PrerequisiteModulesTests, PrebuiltModuleFileTest) { MockDirectoryCompilationDatabase CDB(TestDir, FS); `````````` </details> https://github.com/llvm/llvm-project/pull/187653 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
