Author: Ian Anderson Date: 2024-06-15T07:41:58-07:00 New Revision: 29b0d5755409639cf061667233125fb75d624b7c
URL: https://github.com/llvm/llvm-project/commit/29b0d5755409639cf061667233125fb75d624b7c DIFF: https://github.com/llvm/llvm-project/commit/29b0d5755409639cf061667233125fb75d624b7c.diff LOG: [clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't (#89005) HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every `textual header` to non-external. Added: Modified: clang/include/clang/Lex/HeaderSearch.h clang/lib/Lex/HeaderSearch.cpp clang/unittests/Lex/HeaderSearchTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index 5ac63dddd4d4e..d8ca1c528de36 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -90,7 +90,9 @@ struct HeaderFileInfo { LLVM_PREFERRED_TYPE(bool) unsigned isModuleHeader : 1; - /// Whether this header is a `textual header` in a module. + /// Whether this header is a `textual header` in a module. If a header is + /// textual in one module and normal in another module, this bit will not be + /// set, only `isModuleHeader`. LLVM_PREFERRED_TYPE(bool) unsigned isTextualModuleHeader : 1; diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index d6da6c2fe6c0e..9321a36b79a7f 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -1313,11 +1313,18 @@ OptionalFileEntryRef HeaderSearch::LookupSubframeworkHeader( // File Info Management. //===----------------------------------------------------------------------===// +static bool moduleMembershipNeedsMerge(const HeaderFileInfo *HFI, + ModuleMap::ModuleHeaderRole Role) { + if (ModuleMap::isModular(Role)) + return !HFI->isModuleHeader || HFI->isTextualModuleHeader; + if (!HFI->isModuleHeader && (Role & ModuleMap::TextualHeader)) + return !HFI->isTextualModuleHeader; + return false; +} + static void mergeHeaderFileInfoModuleBits(HeaderFileInfo &HFI, bool isModuleHeader, bool isTextualModuleHeader) { - assert((!isModuleHeader || !isTextualModuleHeader) && - "A header can't build with a module and be textual at the same time"); HFI.isModuleHeader |= isModuleHeader; if (HFI.isModuleHeader) HFI.isTextualModuleHeader = false; @@ -1432,7 +1439,7 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE, if ((Role & ModuleMap::ExcludedHeader)) return; auto *HFI = getExistingFileInfo(FE); - if (HFI && HFI->isModuleHeader) + if (HFI && !moduleMembershipNeedsMerge(HFI, Role)) return; } diff --git a/clang/unittests/Lex/HeaderSearchTest.cpp b/clang/unittests/Lex/HeaderSearchTest.cpp index a5f193ef37ce8..38ce3812c204f 100644 --- a/clang/unittests/Lex/HeaderSearchTest.cpp +++ b/clang/unittests/Lex/HeaderSearchTest.cpp @@ -323,5 +323,73 @@ TEST_F(HeaderSearchTest, HeaderMapFrameworkLookup) { EXPECT_EQ(Search.getIncludeNameForHeader(FE), "Foo/Foo.h"); } +TEST_F(HeaderSearchTest, HeaderFileInfoMerge) { + auto AddHeader = [&](std::string HeaderPath) -> FileEntryRef { + VFS->addFile(HeaderPath, 0, + llvm::MemoryBuffer::getMemBufferCopy("", HeaderPath), + /*User=*/std::nullopt, /*Group=*/std::nullopt, + llvm::sys::fs::file_type::regular_file); + return *FileMgr.getOptionalFileRef(HeaderPath); + }; + + class MockExternalHeaderFileInfoSource : public ExternalHeaderFileInfoSource { + HeaderFileInfo GetHeaderFileInfo(FileEntryRef FE) { + HeaderFileInfo HFI; + auto FileName = FE.getName(); + if (FileName == ModularPath) + HFI.mergeModuleMembership(ModuleMap::NormalHeader); + else if (FileName == TextualPath) + HFI.mergeModuleMembership(ModuleMap::TextualHeader); + HFI.External = true; + HFI.IsValid = true; + return HFI; + } + + public: + std::string ModularPath = "/modular.h"; + std::string TextualPath = "/textual.h"; + }; + + auto ExternalSource = new MockExternalHeaderFileInfoSource(); + Search.SetExternalSource(ExternalSource); + + // Everything should start out external. + auto ModularFE = AddHeader(ExternalSource->ModularPath); + auto TextualFE = AddHeader(ExternalSource->TextualPath); + EXPECT_TRUE(Search.getExistingFileInfo(ModularFE)->External); + EXPECT_TRUE(Search.getExistingFileInfo(TextualFE)->External); + + // Marking the same role should keep it external + Search.MarkFileModuleHeader(ModularFE, ModuleMap::NormalHeader, + /*isCompilingModuleHeader=*/false); + Search.MarkFileModuleHeader(TextualFE, ModuleMap::TextualHeader, + /*isCompilingModuleHeader=*/false); + EXPECT_TRUE(Search.getExistingFileInfo(ModularFE)->External); + EXPECT_TRUE(Search.getExistingFileInfo(TextualFE)->External); + + // textual -> modular should update the HFI, but modular -> textual should be + // a no-op. + Search.MarkFileModuleHeader(ModularFE, ModuleMap::TextualHeader, + /*isCompilingModuleHeader=*/false); + Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader, + /*isCompilingModuleHeader=*/false); + auto ModularFI = Search.getExistingFileInfo(ModularFE); + auto TextualFI = Search.getExistingFileInfo(TextualFE); + EXPECT_TRUE(ModularFI->External); + EXPECT_TRUE(ModularFI->isModuleHeader); + EXPECT_FALSE(ModularFI->isTextualModuleHeader); + EXPECT_FALSE(TextualFI->External); + EXPECT_TRUE(TextualFI->isModuleHeader); + EXPECT_FALSE(TextualFI->isTextualModuleHeader); + + // Compiling the module should make the HFI local. + Search.MarkFileModuleHeader(ModularFE, ModuleMap::NormalHeader, + /*isCompilingModuleHeader=*/true); + Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader, + /*isCompilingModuleHeader=*/true); + EXPECT_FALSE(Search.getExistingFileInfo(ModularFE)->External); + EXPECT_FALSE(Search.getExistingFileInfo(TextualFE)->External); +} + } // namespace } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
