https://github.com/cosminconst updated https://github.com/llvm/llvm-project/pull/225108
>From 8993f5fb2d8ea44a05cb05a55369ca583e067d5d Mon Sep 17 00:00:00 2001 From: Cosmin Constantin <[email protected]> Date: Thu, 17 Sep 2026 17:11:48 +0100 Subject: [PATCH 1/2] [clang][modules] Don't associate a header with an unresolved submodule `HeaderFileInfoTrait::ReadData` resolves a submodule ID out of an AST file and hands the result straight to `ModuleMap::addHeader`: Module *Mod = Reader.getSubmodule(GlobalSMID); ... if (FE || (FE = getFile(key))) { Module::Header H = {std::string(key.Filename), "", *FE}; ModMap.addHeader(Mod, H, HeaderRole, /*Imported=*/true); } `addHeader` dereferences that argument immediately, through `Mod->isForBuilding()` and `Mod->addHeader()`, but `getSubmodule` can return null. An AST file whose submodule table does not agree with what is loaded therefore turns a diagnosed error into a null dereference. Skip the association when the submodule does not resolve. The header is left unattached, and `HFI.mergeModuleMembership` below still runs, so the role bookkeeping is unaffected. Observed as a SIGSEGV in lldb, in these circumstances: - the binary was built with `-fmodule-file-home-is-cwd`, which makes `ASTWriter` store paths in a module file relative to the working directory and omit the `MODULE_DIRECTORY` record; - lldb replays that flag out of the binary's debug info, and falls back to building clang modules implicitly because the explicit module files recorded there are no longer present; - lldb's working directory is an ancestor of its module cache, so that prefix is stripped from the paths written into the implicitly built module files. One module file then gets referenced both absolutely and relatively. The two spellings defeat `ModuleManager`'s de-duplication, the module file is loaded a second time, and reading header info in that state reaches this code with a submodule ID that does not resolve: clang::ModuleMap::addHeader clang::serialization::reader::HeaderFileInfoTrait::ReadData clang::ASTReader::GetHeaderFileInfo clang::HeaderSearch::getExistingFileInfo clang::HeaderSearch::findUsableModuleForHeader clang::HeaderSearch::getFileAndSuggestModule ... clang::CompilerInstance::compileModule This only prevents the crash; it does not address the path handling that leaves the submodule table unresolvable. Unfortunately I couldn't create a test to instrument the exact crash scenario. `clang/test/Modules` (952) and `clang/test/PCH` (298) pass with the change, with no new failures. --- clang/lib/Serialization/ASTReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 76b3ecb8f96db..223b290442f52 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -2441,7 +2441,7 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d, ModuleMap &ModMap = Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap(); - if (FE || (FE = getFile(key))) { + if (Mod && (FE || (FE = getFile(key)))) { // FIXME: NameAsWritten Module::Header H = {std::string(key.Filename), "", *FE}; ModMap.addHeader(Mod, H, HeaderRole, /*Imported=*/true); >From 505c0aea0680b92df14a64907b5c536121eed1f3 Mon Sep 17 00:00:00 2001 From: Cosmin Constantin <[email protected]> Date: Mon, 21 Sep 2026 17:08:17 +0100 Subject: [PATCH 2/2] [clang][modules] Add a test for the unresolved submodule guard Builds a module file, loads it, then remaps its local submodule IDs past the end of `SubmodulesLoaded` so that `getSubmodule` takes its out-of-range path -- reporting the error and returning null -- and reads the header info back. Without the preceding change this segfaults in `ModuleMap::addHeader`. --- clang/unittests/Serialization/CMakeLists.txt | 1 + .../Serialization/UnresolvedSubmoduleTest.cpp | 165 ++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100644 clang/unittests/Serialization/UnresolvedSubmoduleTest.cpp diff --git a/clang/unittests/Serialization/CMakeLists.txt b/clang/unittests/Serialization/CMakeLists.txt index f0837c10b3815..e5710815664bb 100644 --- a/clang/unittests/Serialization/CMakeLists.txt +++ b/clang/unittests/Serialization/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_unittest(SerializationTests PreambleInNamedModulesTest.cpp LoadSpecLazilyTest.cpp SourceLocationEncodingTest.cpp + UnresolvedSubmoduleTest.cpp VarDeclConstantInitTest.cpp CLANG_LIBS clangAST diff --git a/clang/unittests/Serialization/UnresolvedSubmoduleTest.cpp b/clang/unittests/Serialization/UnresolvedSubmoduleTest.cpp new file mode 100644 index 0000000000000..d4f12aba531f6 --- /dev/null +++ b/clang/unittests/Serialization/UnresolvedSubmoduleTest.cpp @@ -0,0 +1,165 @@ +//===- unittests/Serialization/UnresolvedSubmoduleTest.cpp ----------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/Basic/FileManager.h" +#include "clang/Basic/Module.h" +#include "clang/Driver/CreateInvocationFromArgs.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Frontend/CompilerInvocation.h" +#include "clang/Frontend/FrontendActions.h" +#include "clang/Lex/HeaderSearch.h" +#include "clang/Serialization/ASTBitCodes.h" +#include "clang/Serialization/ASTReader.h" +#include "clang/Serialization/ContinuousRangeMap.h" +#include "clang/Serialization/ModuleFile.h" +#include "clang/Serialization/ModuleManager.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" + +#include "gtest/gtest.h" + +using namespace llvm; +using namespace clang; + +namespace { + +class UnresolvedSubmoduleTest : public ::testing::Test { + void SetUp() override { + ASSERT_FALSE( + sys::fs::createUniqueDirectory("unresolved-submodule", TestDir)); + } + + void TearDown() override { sys::fs::remove_directories(TestDir); } + +public: + SmallString<256> TestDir; + + void addFile(StringRef Path, StringRef Contents) { + SmallString<256> AbsPath(TestDir); + sys::path::append(AbsPath, Path); + ASSERT_FALSE(sys::fs::create_directories(sys::path::parent_path(AbsPath))); + std::error_code EC; + raw_fd_ostream OS(AbsPath, EC); + ASSERT_FALSE(EC); + OS << Contents; + } +}; + +// A header in a module file is associated with the submodule it belongs to when +// its header info is read. If the submodule ID does not resolve, getSubmodule() +// reports the error and returns null, and the association has to be skipped: +// ModuleMap::addHeader() dereferences the Module it is given. +TEST_F(UnresolvedSubmoduleTest, HeaderInfoWithUnresolvableSubmoduleID) { + addFile("mod/module.modulemap", R"cc( +module A { + module a1 { header "a1.h" export * } + module a2 { header "a2.h" export * } +} +)cc"); + addFile("mod/a1.h", "static inline int a1(void) { return 1; }\n"); + addFile("mod/a2.h", "static inline int a2(void) { return 2; }\n"); + + SmallString<256> ModuleMap(TestDir); + sys::path::append(ModuleMap, "mod", "module.modulemap"); + SmallString<256> HeaderPath(TestDir); + sys::path::append(HeaderPath, "mod", "a1.h"); + SmallString<256> PCMPath(TestDir); + sys::path::append(PCMPath, "A.pcm"); + + { + CreateInvocationOptions CIOpts; + CIOpts.VFS = vfs::createPhysicalFileSystem(); + DiagnosticOptions DiagOpts; + IntrusiveRefCntPtr<DiagnosticsEngine> Diags = + CompilerInstance::createDiagnostics(*CIOpts.VFS, DiagOpts); + CIOpts.Diags = Diags; + + const char *Args[] = {"clang", + "-fmodules", + "-fno-implicit-modules", + "-x", + "c", + "-Xclang", + "-emit-module", + "-Xclang", + "-fmodule-name=A", + ModuleMap.c_str(), + "-o", + PCMPath.c_str()}; + std::shared_ptr<CompilerInvocation> Invocation = + createInvocation(Args, CIOpts); + ASSERT_TRUE(Invocation); + Invocation->getFrontendOpts().DisableFree = false; + + CompilerInstance Instance(std::move(Invocation)); + Instance.setDiagnostics(Diags); + Instance.createVirtualFileSystem(CIOpts.VFS); + Instance.createFileManager(); + Instance.getFrontendOpts().OutputFile = PCMPath.str().str(); + + GenerateModuleFromModuleMapAction Action; + ASSERT_TRUE(Instance.ExecuteAction(Action)); + ASSERT_FALSE(Diags->hasErrorOccurred()); + } + + CreateInvocationOptions CIOpts; + CIOpts.VFS = vfs::createPhysicalFileSystem(); + DiagnosticOptions DiagOpts; + IntrusiveRefCntPtr<DiagnosticsEngine> Diags = + CompilerInstance::createDiagnostics(*CIOpts.VFS, DiagOpts); + CIOpts.Diags = Diags; + + const char *Args[] = {"clang", "-fmodules", "-fno-implicit-modules", + "-x", "c", "-"}; + std::shared_ptr<CompilerInvocation> Invocation = + createInvocation(Args, CIOpts); + ASSERT_TRUE(Invocation); + Invocation->getFrontendOpts().DisableFree = false; + + CompilerInstance Clang(std::move(Invocation)); + Clang.setDiagnostics(Diags); + Clang.createVirtualFileSystem(CIOpts.VFS); + Clang.createFileManager(); + Clang.createSourceManager(); + ASSERT_TRUE(Clang.createTarget()); + Clang.createPreprocessor(TU_Complete); + Clang.createASTContext(); + Clang.createASTReader(); + + IntrusiveRefCntPtr<ASTReader> Reader = Clang.getASTReader(); + ASSERT_TRUE(Reader); + + ASSERT_EQ(Reader->ReadAST(ModuleFileName::makeExplicit(PCMPath.str()), + serialization::MK_ExplicitModule, SourceLocation(), + ASTReader::ARR_None), + ASTReader::Success); + + serialization::ModuleFile &MF = Reader->getModuleManager().getPrimaryModule(); + ASSERT_GT(MF.LocalNumSubmodules, 0u); + + // Point the module file's local submodule IDs far past the end of + // SubmodulesLoaded. getSubmodule() diagnoses that and returns null, which is + // the state a module file with an inconsistent submodule table produces. + MF.SubmoduleRemap = ContinuousRangeMap<uint32_t, int, 2>(); + { + ContinuousRangeMap<uint32_t, int, 2>::Builder B(MF.SubmoduleRemap); + B.insert(std::make_pair(MF.LocalBaseSubmoduleID, 1 << 20)); + } + + auto FE = Clang.getFileManager().getOptionalFileRef(HeaderPath); + ASSERT_TRUE(FE); + + // Reading the header info walks every loaded module file, reaches this module + // file's entry for a1.h, and fails to resolve its submodule. This must not + // dereference the null Module. + Reader->GetHeaderFileInfo(*FE); +} + +} // anonymous namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
