[clang][modules] Only modulemaps of included textual headers are affecting

[sam-mccall]

Prior to this change, modulemaps describing textual headers are considered
to affect the current module whenever HeaderFileInfos for those headers exist.

This wastes creates false dependencies that (among other things) waste
SourceLocation space.

After this change, textual headers don't cause their modulemap to be considered
affecting, unless those textual headers are included (in which case their
modulemap is required e.g. for layering check).

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Sam McCall (sam-mccall)

Changes

Prior to this change, modulemaps describing textual headers are considered
to affect the current module whenever HeaderFileInfos for those headers exist.

This wastes creates false dependencies that (among other things) waste
SourceLocation space.

After this change, textual headers don't cause their modulemap to be considered
affecting, unless those textual headers are included (in which case their
modulemap is required e.g. for layering check).

---

Full diff: https://github.com/llvm/llvm-project/pull/89729.diff

5 Files Affected:

• (modified) clang/include/clang/Lex/HeaderSearch.h (+4)
• (modified) clang/lib/Lex/HeaderSearch.cpp (+4)
• (modified) clang/lib/Lex/PPDirectives.cpp (+1)
• (modified) clang/lib/Serialization/ASTWriter.cpp (+6-4)
• (added) clang/test/Modules/prune-non-affecting-module-map-files-textual.c (+47)

diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h
index c5f90ef4cb3682..c24ef15005f36b 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -544,6 +544,10 @@ class HeaderSearch {
                               bool isImport, bool ModulesEnabled, Module *M,
                               bool &IsFirstIncludeOfFile);
 
+  /// Record that we're parsing this file as part of the current module, if any.
+  /// The header's owning modulemap is considered to affect the current module.
+  void EnteredTextualFile(FileEntryRef File);
+
   /// Return whether the specified file is a normal header,
   /// a system header, or a C++ friendly system header.
   SrcMgr::CharacteristicKind getFileDirFlavor(FileEntryRef File) {
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0632882b296146..84c7e09638d574 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1441,6 +1441,10 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
   HFI.isCompilingModuleHeader |= isCompilingModuleHeader;
 }
 
+void HeaderSearch::EnteredTextualFile(FileEntryRef File) {
+  getFileInfo(File).isCompilingModuleHeader = true;
+}
+
 bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor &PP,
                                           FileEntryRef File, bool isImport,
                                           bool ModulesEnabled, Module *M,
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 0b22139ebe81de..196c7795ca05e8 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -2609,6 +2609,7 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
   }
 
   assert(!IsImportDecl && "failed to diagnose missing module for import decl");
+  HeaderInfo.EnteredTextualFile(*File);
   return {ImportAction::None};
 }
 
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 8a4b36207c4734..4825c245a4b846 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
       continue;
 
     const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-    if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+    if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
+                 !HFI->isCompilingModuleHeader))
       continue;
 
     for (const auto &KH : HS.findResolvedModulesForHeader(*File)) {
@@ -2055,11 +2056,12 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
 
     // Get the file info. Skip emitting this file if we have no information on
     // it as a header file (in which case HFI will be null) or if it hasn't
-    // changed since it was loaded. Also skip it if it's for a modular header
-    // from a different module; in that case, we rely on the module(s)
+    // changed since it was loaded. Also skip it if it's for a non-excluded
+    // header from a different module; in that case, we rely on the module(s)
     // containing the header to provide this information.
     const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-    if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+    if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
+                 !HFI->isCompilingModuleHeader))
       continue;
 
     // Massage the file path into an appropriate form.
diff --git a/clang/test/Modules/prune-non-affecting-module-map-files-textual.c b/clang/test/Modules/prune-non-affecting-module-map-files-textual.c
new file mode 100644
index 00000000000000..12a42c52a940b9
--- /dev/null
+++ b/clang/test/Modules/prune-non-affecting-module-map-files-textual.c
@@ -0,0 +1,47 @@
+// This test checks that a module map with a textual header can be marked as
+// non-affecting.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: split-file %s %t
+
+//--- X.modulemap
+module X { textual header "X.h" }
+//--- X.h
+typedef int X_int;
+
+//--- Y.modulemap
+module Y { textual header "Y.h" }
+//--- Y.h
+typedef int Y_int;
+
+//--- A.modulemap
+module A { header "A.h" export * }
+//--- A.h
+#include "X.h"
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/A.modulemap -fmodule-name=A -o %t/A0.pcm \
+// RUN:   -fmodule-map-file=%t/X.modulemap
+// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/A0.pcm | FileCheck %s --check-prefix=A0 --implicit-check-not=Y.modulemap
+// A0: Input file: {{.*}}/X.modulemap
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/A.modulemap -fmodule-name=A -o %t/A1.pcm \
+// RUN:   -fmodule-map-file=%t/X.modulemap -fmodule-map-file=%t/Y.modulemap
+// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/A0.pcm | FileCheck %s --check-prefix=A1 \
+// RUN:   --implicit-check-not=Y.modulemap
+// A1: Input file: {{.*}}/X.modulemap
+
+// RUN: diff %t/A0.pcm %t/A1.pcm
+
+//--- B.modulemap
+module B { header "B.h" export * }
+//--- B.h
+#include "A.h"
+typedef X_int B_int;
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/B.modulemap -fmodule-name=B -o %t/B.pcm \
+// RUN:   -fmodule-file=A=%t/A0.pcm \
+// RUN:   -fmodule-map-file=%t/A.modulemap -fmodule-map-file=%t/X.modulemap -fmodule-map-file=%t/Y.modulemap
+// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/B.pcm | FileCheck %s --check-prefix=B \
+// RUN:   --implicit-check-not=X.modulemap --implicit-check-not=Y.modulemap
+// B: Input file: {{.*}}/B.modulemap
+

[jansvoboda11]

The documentation for isCompilingModuleHeader says:

  /// Whether this header is part of the module that we are building, even if it
  /// doesn't build with the module. i.e. this will include `excluded` and
  /// `textual` headers as well as normal headers.

To me this means this should set only for headers that semantically belong to the current module, not headers that happen to be compiled with it due to being textual.

[sam-mccall]

Oops, I meant to alter that description in this patch, it seems that change got lost.

At this point, isCompilingModuleHeader is used only for this "is affecting" check, and for a heuristic related to translating #include into #import. The latter only cares about modular headers, so it seems reasonable to change the semantics for textual headers to something more useful. (But I do need to be careful we're only setting this for textual modular headers).