[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't (PR #89005)

[Ian Anderson via cfe-commits]

https://github.com/ian-twilightcoder updated https://github.com/llvm/llvm-project/pull/89005

>From 011ff5a95a4a5de1dc6ae2d271ae42f28075b2b0 Mon Sep 17 00:00:00 2001
From: Ian Anderson <iana at apple.com>
Date: Tue, 16 Apr 2024 17:08:28 -0700
Subject: [PATCH] [clang][modules] HeaderSearch::MarkFileModuleHeader sets
 textual headers' HeaderFileInfo non-external when it shouldn't

HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every `textual header` to non-external.
---
 clang/include/clang/Lex/HeaderSearch.h   |  4 +-
 clang/lib/Lex/HeaderSearch.cpp           | 14 +++--
 clang/unittests/Lex/HeaderSearchTest.cpp | 67 ++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 4 deletions(-)

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 574723b33866a..18c0bab4a81e4 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1313,11 +1313,19 @@ OptionalFileEntryRef HeaderSearch::LookupSubframeworkHeader(
 // File Info Management.
 //===----------------------------------------------------------------------===//
 
+static bool moduleMembershipNeedsMerge(const HeaderFileInfo *HFI,
+                                       ModuleMap::ModuleHeaderRole Role) {
+  if (ModuleMap::isModular(Role))
+    return !HFI->isModuleHeader || HFI->isTextualModuleHeader;
+  else if (!HFI->isModuleHeader && (Role & ModuleMap::TextualHeader))
+    return !HFI->isTextualModuleHeader;
+  else
+    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 +1440,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 c578fa72c859e..f1735c2d46946 100644
--- a/clang/unittests/Lex/HeaderSearchTest.cpp
+++ b/clang/unittests/Lex/HeaderSearchTest.cpp
@@ -308,5 +308,72 @@ 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 *Search.LookupFile(
+        HeaderPath, SourceLocation(), /*isAngled=*/false, /*FromDir=*/nullptr,
+        /*CurDir=*/nullptr, /*Includers=*/{}, /*SearchPath=*/nullptr,
+        /*RelativePath=*/nullptr, /*RequestingModule=*/nullptr,
+        /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr,
+        /*IsFrameworkFound=*/nullptr);
+  };
+
+  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, false);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::TextualHeader, 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, false);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader, 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(ModularFI->isModuleHeader);
+  EXPECT_FALSE(ModularFI->isTextualModuleHeader);
+
+  // Compiling the module should make the HFI local.
+  Search.MarkFileModuleHeader(ModularFE, ModuleMap::NormalHeader, true);
+  Search.MarkFileModuleHeader(TextualFE, ModuleMap::NormalHeader, true);
+  EXPECT_FALSE(Search.getExistingFileInfo(ModularFE)->External);
+  EXPECT_FALSE(Search.getExistingFileInfo(TextualFE)->External);
+}
+
 } // namespace
 } // namespace clang