llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Cyndy Ishida (cyndyishida) <details> <summary>Changes</summary> Replace the opaque `ErrorStr` parameter in `ModuleManager::addModule` with a structured object that carries specific information for reporting. This allows the diagnostics to emit targeted notes instead of a single error string appended to the main diagnostic. Information that is relevant but is tied to implementation details of the compilation is reported as notes (e.g. signature mismatches) --- Patch is 21.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/190203.diff 8 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticSerializationKinds.td (+6-2) - (modified) clang/include/clang/Serialization/ModuleFile.h (+13) - (modified) clang/include/clang/Serialization/ModuleManager.h (+57-25) - (modified) clang/lib/Serialization/ASTReader.cpp (+24-27) - (modified) clang/lib/Serialization/ModuleManager.cpp (+58-40) - (added) clang/test/ClangScanDeps/modules-pch-signature-mismatch.c (+62) - (modified) clang/test/Modules/explicit-build.cpp (+4-2) - (modified) clang/test/Modules/invalid-module-dep.c (+2) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td index 475c19957c4be..5c74caf010ad9 100644 --- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td @@ -78,10 +78,14 @@ def err_ast_file_with_compiler_errors : Error< def err_module_file_conflict : Error< "module '%0' is defined in both '%1' and '%2'">, DefaultFatal; def err_ast_file_not_found : Error< - "%select{PCH|module|precompiled}0 file '%1' not found%select{|: %3}2">, DefaultFatal; + "%select{PCH|module|precompiled}0 file '%1' not found">, DefaultFatal; +def note_ast_file_buffer_failed : Note< + "unable to load buffer for precompiled file: %0">; +def note_ast_file_signature_failed : Note< + "unable to verify precompiled file signature: %0">; def err_ast_file_out_of_date : Error< "%select{PCH|module|precompiled}0 file '%1' is out of date and " - "needs to be rebuilt%select{|: %3}2">, DefaultFatal; + "needs to be rebuilt">, DefaultFatal; def err_ast_file_invalid : Error< "file '%1' is not a valid %select{PCH|module|precompiled}0 file: %2">, DefaultFatal; def note_module_file_imported_by : Note< diff --git a/clang/include/clang/Serialization/ModuleFile.h b/clang/include/clang/Serialization/ModuleFile.h index 303bd65a8aad0..3dfbf4de96355 100644 --- a/clang/include/clang/Serialization/ModuleFile.h +++ b/clang/include/clang/Serialization/ModuleFile.h @@ -32,6 +32,7 @@ #include <cassert> #include <cstdint> #include <memory> +#include <optional> #include <string> #include <vector> @@ -120,6 +121,18 @@ class InputFile { bool isNotFound() const { return Val.getInt() == NotFound; } }; +/// Describes a single change detected in a module file or input file. +struct Change { + enum ModificationKind { + Size, + ModTime, + Content, + None, + } Kind; + std::optional<int64_t> Old = std::nullopt; + std::optional<int64_t> New = std::nullopt; +}; + /// Specifies the high-level result of validating input files. enum class InputFilesValidation { /// Initial value, before the validation has been performed. diff --git a/clang/include/clang/Serialization/ModuleManager.h b/clang/include/clang/Serialization/ModuleManager.h index 1ef9aeee7e1fd..87aeafe61ee79 100644 --- a/clang/include/clang/Serialization/ModuleManager.h +++ b/clang/include/clang/Serialization/ModuleManager.h @@ -42,6 +42,53 @@ class PCHContainerReader; namespace serialization { +/// The result of attempting to add a new module. +class AddModuleResult { +public: + enum Kind { + /// The module file had already been loaded. + AlreadyLoaded, + /// The module file was just loaded in response to this call. + NewlyLoaded, + /// The module file is missing. + Missing, + /// The module file is out-of-date. + OutOfDate + }; + Kind K; + + ModuleFile *getModule() const { return Module; } + + StringRef getBufferError() const { + assert(K == Missing && !Module); + return BufferError; + } + + const SmallVector<Change, 2> &getChanges() const { + assert(K == OutOfDate && !Module); + return Changes; + } + + InputFilesValidation getValidationStatus() const { + assert(K == OutOfDate && !Module); + return ValidationStatus; + } + + StringRef getSignatureError() const { + assert(K == OutOfDate && !Module); + return SignatureError; + } + +private: + friend class ModuleManager; + + ModuleFile *Module = nullptr; + SmallVector<Change, 2> Changes; + InputFilesValidation ValidationStatus = InputFilesValidation::NotStarted; + std::string BufferError; + std::string SignatureError; +}; + /// Manages the set of modules loaded by an AST reader. class ModuleManager { /// The chain of AST files, in the order in which we started to load @@ -96,6 +143,13 @@ class ModuleManager { /// just an non-owning pointer. GlobalModuleIndex *GlobalIndex = nullptr; + bool isModuleFileOutOfDate(off_t Size, time_t ModTime, off_t ExpectedSize, + time_t ExpectedModTime, AddModuleResult &Result); + + bool checkSignature(ASTFileSignature Signature, + ASTFileSignature ExpectedSignature, + AddModuleResult &Result); + /// State used by the "visit" operation to avoid malloc traffic in /// calls to visit(). struct VisitState { @@ -187,21 +241,6 @@ class ModuleManager { /// Number of modules loaded unsigned size() const { return Chain.size(); } - /// The result of attempting to add a new module. - enum AddModuleResult { - /// The module file had already been loaded. - AlreadyLoaded, - - /// The module file was just loaded in response to this call. - NewlyLoaded, - - /// The module file is missing. - Missing, - - /// The module file is out-of-date. - OutOfDate - }; - using ASTFileSignatureReader = ASTFileSignature (*)(StringRef); /// Attempts to create a new module and add it to the list of known @@ -230,21 +269,14 @@ class ModuleManager { /// \param ReadSignature Reads the signature from an AST file without actually /// loading it. /// - /// \param Module A pointer to the module file if the module was successfully - /// loaded. - /// - /// \param ErrorStr Will be set to a non-empty string if any errors occurred - /// while trying to load the module. - /// - /// \return A pointer to the module that corresponds to this file name, - /// and a value indicating whether the module was loaded. + /// \return The result of attempting to add the module, including a pointer + /// to the module file if successfully loaded. AddModuleResult addModule(ModuleFileName FileName, ModuleKind Type, SourceLocation ImportLoc, ModuleFile *ImportedBy, unsigned Generation, off_t ExpectedSize, time_t ExpectedModTime, ASTFileSignature ExpectedSignature, - ASTFileSignatureReader ReadSignature, - ModuleFile *&Module, std::string &ErrorStr); + ASTFileSignatureReader ReadSignature); /// Remove the modules starting from First (to the end). void removeModules(ModuleIterator First); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 4ecdb563a1de2..a41149658206f 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -2910,16 +2910,6 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) { } } - struct Change { - enum ModificationKind { - Size, - ModTime, - Content, - None, - } Kind; - std::optional<int64_t> Old = std::nullopt; - std::optional<int64_t> New = std::nullopt; - }; auto HasInputContentChanged = [&](Change OriginalChange) { assert(ValidateASTInputFilesContent && "We should only check the content of the inputs with " @@ -5222,26 +5212,24 @@ ASTReader::ASTReadResult ASTReader::ReadASTCore( ModuleFile *ImportedBy, SmallVectorImpl<ImportedModule> &Loaded, off_t ExpectedSize, time_t ExpectedModTime, ASTFileSignature ExpectedSignature, unsigned ClientLoadCapabilities) { - ModuleFile *M; - std::string ErrorStr; - ModuleManager::AddModuleResult AddResult - = ModuleMgr.addModule(FileName, Type, ImportLoc, ImportedBy, - getGeneration(), ExpectedSize, ExpectedModTime, - ExpectedSignature, readASTFileSignature, - M, ErrorStr); - - switch (AddResult) { - case ModuleManager::AlreadyLoaded: + auto Result = ModuleMgr.addModule( + FileName, Type, ImportLoc, ImportedBy, getGeneration(), ExpectedSize, + ExpectedModTime, ExpectedSignature, readASTFileSignature); + ModuleFile *M = Result.getModule(); + + switch (Result.K) { + case AddModuleResult::AlreadyLoaded: { Diag(diag::remark_module_import) << M->ModuleName << M->FileName << (ImportedBy ? true : false) << (ImportedBy ? StringRef(ImportedBy->ModuleName) : StringRef()); return Success; + } - case ModuleManager::NewlyLoaded: + case AddModuleResult::NewlyLoaded: // Load module file below. break; - case ModuleManager::Missing: + case AddModuleResult::Missing: // The module file was missing; if the client can handle that, return // it. if (ClientLoadCapabilities & ARR_Missing) @@ -5249,11 +5237,12 @@ ASTReader::ASTReadResult ASTReader::ReadASTCore( // Otherwise, return an error. Diag(diag::err_ast_file_not_found) - << moduleKindForDiagnostic(Type) << FileName << !ErrorStr.empty() - << ErrorStr; + << moduleKindForDiagnostic(Type) << FileName; + if (!Result.getBufferError().empty()) + Diag(diag::note_ast_file_buffer_failed) << Result.getBufferError(); return Failure; - case ModuleManager::OutOfDate: + case AddModuleResult::OutOfDate: // We couldn't load the module file because it is out-of-date. If the // client can handle out-of-date, return it. if (ClientLoadCapabilities & ARR_OutOfDate) @@ -5261,8 +5250,16 @@ ASTReader::ASTReadResult ASTReader::ReadASTCore( // Otherwise, return an error. Diag(diag::err_ast_file_out_of_date) - << moduleKindForDiagnostic(Type) << FileName << !ErrorStr.empty() - << ErrorStr; + << moduleKindForDiagnostic(Type) << FileName; + for (const auto &C : Result.getChanges()) { + Diag(diag::note_fe_ast_file_modified) + << C.Kind << (C.Old && C.New) << llvm::itostr(C.Old.value_or(0)) + << llvm::itostr(C.New.value_or(0)); + } + Diag(diag::note_ast_file_input_files_validation_status) + << Result.getValidationStatus(); + if (!Result.getSignatureError().empty()) + Diag(diag::note_ast_file_signature_failed) << Result.getSignatureError(); return Failure; } diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp index ed7b6cf67674e..a284131857229 100644 --- a/clang/lib/Serialization/ModuleManager.cpp +++ b/clang/lib/Serialization/ModuleManager.cpp @@ -66,28 +66,30 @@ ModuleManager::lookupBuffer(StringRef Name) { return std::move(InMemoryBuffers[*Entry]); } -static bool checkModuleFile(off_t Size, time_t ModTime, off_t ExpectedSize, - time_t ExpectedModTime, std::string &ErrorStr) { +bool ModuleManager::isModuleFileOutOfDate(off_t Size, time_t ModTime, + off_t ExpectedSize, + time_t ExpectedModTime, + AddModuleResult &Result) { + bool OutOfDate = false; if (ExpectedSize && ExpectedSize != Size) { - ErrorStr = "module file has a different size than expected"; - return true; + Result.Changes.push_back({Change::Size, ExpectedSize, Size}); + OutOfDate = true; } if (ExpectedModTime && ExpectedModTime != ModTime) { - ErrorStr = "module file has a different modification time than expected"; - return true; + Result.Changes.push_back({Change::ModTime, ExpectedModTime, ModTime}); + OutOfDate = true; } - return false; + return OutOfDate; } -static bool checkSignature(ASTFileSignature Signature, - ASTFileSignature ExpectedSignature, - std::string &ErrorStr) { +bool ModuleManager::checkSignature(ASTFileSignature Signature, + ASTFileSignature ExpectedSignature, + AddModuleResult &Result) { if (!ExpectedSignature || Signature == ExpectedSignature) return false; - - ErrorStr = + Result.SignatureError = Signature ? "signature mismatch" : "could not read module signature"; return true; } @@ -105,13 +107,12 @@ static void updateModuleImports(ModuleFile &MF, ModuleFile *ImportedBy, } } -ModuleManager::AddModuleResult ModuleManager::addModule( +AddModuleResult ModuleManager::addModule( ModuleFileName FileName, ModuleKind Type, SourceLocation ImportLoc, ModuleFile *ImportedBy, unsigned Generation, off_t ExpectedSize, time_t ExpectedModTime, ASTFileSignature ExpectedSignature, - ASTFileSignatureReader ReadSignature, ModuleFile *&Module, - std::string &ErrorStr) { - Module = nullptr; + ASTFileSignatureReader ReadSignature) { + AddModuleResult Result; uint64_t InputFilesValidationTimestamp = 0; if (Type == MK_ImplicitModule) @@ -131,24 +132,33 @@ ModuleManager::AddModuleResult ModuleManager::addModule( std::optional<ModuleFileKey> FileKey = FileName.makeKey(FileMgr); if (!FileKey) { - ErrorStr = "module file not found"; - return Missing; + Result.K = AddModuleResult::Missing; + return Result; } - // Check whether we already loaded this module, before + // Check whether we already loaded this module before. + // Note: `isModuleFileOutOfDate` and `checkSignature` are mutally exclusive in + // practice. If a signature is stored, it means size/mtime values have been + // zeroed out. If size/mtime are non-NULL, the signature is empty. if (ModuleFile *ModuleEntry = lookup(*FileKey)) { - // Check file properties. - if (checkModuleFile(ModuleEntry->Size, ModuleEntry->ModTime, ExpectedSize, - ExpectedModTime, ErrorStr)) - return OutOfDate; + if (isModuleFileOutOfDate(ModuleEntry->Size, ModuleEntry->ModTime, + ExpectedSize, ExpectedModTime, Result)) { + Result.ValidationStatus = ModuleEntry->InputFilesValidationStatus; + Result.K = AddModuleResult::OutOfDate; + return Result; + } // Check the stored signature. - if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) - return OutOfDate; + if (checkSignature(ModuleEntry->Signature, ExpectedSignature, Result)) { + Result.ValidationStatus = ModuleEntry->InputFilesValidationStatus; + Result.K = AddModuleResult::OutOfDate; + return Result; + } - Module = ModuleEntry; + Result.Module = ModuleEntry; updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc); - return AlreadyLoaded; + Result.K = AddModuleResult::AlreadyLoaded; + return Result; } // Load the contents of the module @@ -176,8 +186,8 @@ ModuleManager::AddModuleResult ModuleManager::addModule( FileMgr.getOptionalFileRef(FileName, /*OpenFile=*/true, /*CacheFailure=*/false); if (!Entry) { - ErrorStr = "module file not found"; - return Missing; + Result.K = AddModuleResult::Missing; + return Result; } ModTime = Entry->getModificationTime(); Size = Entry->getSize(); @@ -186,7 +196,8 @@ ModuleManager::AddModuleResult ModuleManager::addModule( FileName)) { // Report that the module is out of date, since we tried (and failed) to // import it earlier. - return OutOfDate; + Result.K = AddModuleResult::OutOfDate; + return Result; } else { OptionalFileEntryRef Entry = expectedToOptional(FileName == StringRef("-") @@ -194,8 +205,8 @@ ModuleManager::AddModuleResult ModuleManager::addModule( : FileMgr.getFileRef(FileName, /*OpenFile=*/true, /*CacheFailure=*/false)); if (!Entry) { - ErrorStr = "module file not found"; - return Missing; + Result.K = AddModuleResult::Missing; + return Result; } // Get a buffer of the file and close the file descriptor when done. @@ -209,8 +220,9 @@ ModuleManager::AddModuleResult ModuleManager::addModule( /*RequiresNullTerminator=*/false); if (!Buf) { - ErrorStr = Buf.getError().message(); - return Missing; + Result.BufferError = Buf.getError().message(); + Result.K = AddModuleResult::Missing; + return Result; } Size = Entry->getSize(); @@ -232,21 +244,26 @@ ModuleManager::AddModuleResult ModuleManager::addModule( NewModule->Data = PCHContainerRdr.ExtractPCH(*NewModule->Buffer); // Check file properties. - if (checkModuleFile(Size, ModTime, ExpectedSize, ExpectedModTime, ErrorStr)) - return OutOfDate; + if (isModuleFileOutOfDate(Size, ModTime, ExpectedSize, ExpectedModTime, + Result)) { + Result.K = AddModuleResult::OutOfDate; + return Result; + } // Read the signature eagerly now so that we can check it. Avoid calling // ReadSignature unless there's something to check though. if (ExpectedSignature && checkSignature(ReadSignature(NewModule->Data), - ExpectedSignature, ErrorStr)) - return OutOfDate; + ExpectedSignature, Result)) { + Result.K = AddModuleResult::OutOfDate; + return Result; + } if (NewFileBuffer) getModuleCache().getInMemoryModuleCache().addPCM(FileName, std::move(NewFileBuffer)); // We're keeping this module. Store it in the map. - Module = Modules[*FileKey] = NewModule.get(); + Result.Module = Modules[*FileKey] = NewModule.get(); updateModuleImports(*NewModule, ImportedBy, ImportLoc); @@ -256,7 +273,8 @@ ModuleManager::AddModuleResult ModuleManager::addModule( Roots.push_back(NewModule.get()); Chain.push_back(std::move(NewModule)); - return NewlyLoaded; + Result.K = AddModuleResult::NewlyLoaded; + return Result; } void ModuleManager::removeModules(ModuleIterator First) { diff --git a/clang/test/ClangScanDeps/modules-pch-signature-mismatch.c b/clang/test/ClangScanDeps/modules-pch-signature-mismatch.c new file mode 100644 index 0000000000000..ab8ca36666cf9 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-pch-signature-mismatch.c @@ -0,0 +1,62 @@ +// UNSUPPORTED: target={{.*}}-aix{{.*}} + +/// Test that a signature mismatch on a PCH's module dependency is diagnosed +/// during dependency scanning. +/// +/// 1. Scan PCH dependencies with modules hash content encoded. +/// 2. Explicitly build module A and the PCH. +/// 3. Modify A's header, resulting in the same byte length and rebuild A for a new signature. +/// 4. Scan a TU that uses the out of date PCH to detect signature mismatch. + +// RUN: rm -rf %t && mkdir %t +// RUN: split-file %s %t + +// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.in > %t/cdb_pch.json +// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \ +// RUN: -format experimental-full -module-files-dir %t/build > %t/result_pch.json + +// RUN: %deps-to-rsp %t/result_pch.json --module-name=A > %t/A.rsp +// RUN: %deps-to-rsp %t/result_pch.json --tu-index=0 > %t/pch.rsp +// RUN: %clang @%t/A.rsp +// RUN: %clang @%t/pch.rsp + +// RUN: cp %t/a_alt.h %t/a.h +// RUN: %clang @%t/A.rsp + +// RUN: sed "s|DIR|%/t|g" %t/cdb_tu.json.in > %t/cdb_tu.json +// RUN: not clang-scan-deps -compilation-database %t/cdb_tu.json \ +// RUN: -format experimental-full -module-files-dir %t/build 2>&1 \ +// RUN: | FileCheck %s + +// CHECK: error:{{.*}}is out of date and needs to be rebuilt +// CHECK: note: unable to verify precompiled file signature: signature mismatch + +//--- module.modulemap +module A { + header "a.h" +} + +//--- a.h +typedef int A_t; + +//--- a_alt.h +typedef int A_u; + +//--- pch.h +#include "a.h" + +//--- tu.c + +//--- cdb_pch.json.in +[{ + "directory": "DIR", + "command": "clang -x c-header DIR/pch.h -fmodules -fmodules-cache-path=DIR/cache -Xclang -fmodules-hash-content -I DIR -o DIR/pch.h.pch", + "file": "DIR/pch.h" +}] + +//--- cdb_tu.json.in +[{ + "directory": "DIR", + "command": "clang -fsyntax-only DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -Xclang -fmodules-hash-content -I DIR -include DIR/pch.h -o DIR/tu.o", + "file": "DIR/tu.c" +}] diff --git a/clang/test/Modules/explicit-build.cpp b/clang/test/Modules/explicit-build.cpp index c2fe2024a3629..020a1dceede8d 100644 --- a/clang/test/Modules/explicit-build.cpp +++ b/clang/test/Modules/explicit-build.cpp @@ -170,7 +170,7 @@ // RUN: -fmodule-file=%t/nonexistent.pcm \ // RUN: %s 2>&1 | FileCheck --check-prefix=CHECK-NO-FILE %s // -// CHECK-NO-FILE: fatal error: module file '{{.*}}nonexistent.p... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/190203 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
