[Module] Prefer precompiled module even when header is private

[ilya-biryukov]

Currently, Clang picks a module where the header is non-private even when the header is textual and the other is modular. This change makes it prefer a module where the header is modular instead. Access checking is done separately and checks if any module has access to a header, so resolving to a different module should result in better performance with no semantics change.

In cases where the same header is used by multiple modules, this may cause unnecessary reparses of the textual include and prohibits optimizations removing modular inputs when -fmodules-embed-all-files are used.

I am changing the default behavior as having multiple modules that have the same header is rare and so this is unlikely to break anyone in the first place, per discussion #138227 that led to this change.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Ilya Biryukov (ilya-biryukov)

Changes

Currently, Clang picks a module where the header is non-private even when the header is textual and the other is modular. This change makes it prefer a module where the header is modular instead. Access checking is done separately and checks if any module has access to a header, so resolving to a different module should result in better performance with no semantics change.

In cases where the same header is used by multiple modules, this may cause unnecessary reparses of the textual include and prohibits optimizations removing modular inputs when -fmodules-embed-all-files are used.

I am changing the default behavior as having multiple modules that have the same header is rare and so this is unlikely to break anyone in the first place, per discussion #138227 that led to this change.

---

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

2 Files Affected:

• (modified) clang/lib/Lex/ModuleMap.cpp (+5-5)
• (added) clang/test/Modules/multiple-modules-per-header-embed-all-files.cpp (+60)

diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 637a08fe4dcdb..fc0a0c1cfc873 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -572,16 +572,16 @@ static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New,
   if (New.getModule()->isAvailable() && !Old.getModule()->isAvailable())
     return true;
 
-  // Prefer a public header over a private header.
-  if ((New.getRole() & ModuleMap::PrivateHeader) !=
-      (Old.getRole() & ModuleMap::PrivateHeader))
-    return !(New.getRole() & ModuleMap::PrivateHeader);
-
   // Prefer a non-textual header over a textual header.
   if ((New.getRole() & ModuleMap::TextualHeader) !=
       (Old.getRole() & ModuleMap::TextualHeader))
     return !(New.getRole() & ModuleMap::TextualHeader);
 
+  // Prefer a public header over a private header.
+  if ((New.getRole() & ModuleMap::PrivateHeader) !=
+      (Old.getRole() & ModuleMap::PrivateHeader))
+    return !(New.getRole() & ModuleMap::PrivateHeader);
+
   // Prefer a non-excluded header over an excluded header.
   if ((New.getRole() == ModuleMap::ExcludedHeader) !=
       (Old.getRole() == ModuleMap::ExcludedHeader))
diff --git a/clang/test/Modules/multiple-modules-per-header-embed-all-files.cpp b/clang/test/Modules/multiple-modules-per-header-embed-all-files.cpp
new file mode 100644
index 0000000000000..19f1db1689667
--- /dev/null
+++ b/clang/test/Modules/multiple-modules-per-header-embed-all-files.cpp
@@ -0,0 +1,60 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+//
+// First, build a module with a header.
+//
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules1.map -fmodule-name=a -emit-module -xc++ -fmodules-embed-all-files -o %t/a.pcm %t/modules1.map
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules1.map -fmodule-map-file=%t/modules2.map -fmodule-name=b -emit-module \
+// RUN:   -fmodule-file=%t/a.pcm -xc++ -fmodules-embed-all-files -o %t/b.pcm %t/modules2.map
+// 
+// Remove the header and check we can still build the code that finds it in a PCM.
+//
+// RUN: rm %t/foo.h
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules2.map -fmodule-file=%t/b.pcm -fsyntax-only %t/use.cpp
+
+//--- modules1.map
+module a {
+  module foo {
+    header "foo.h"
+    export *
+  }
+  export *
+}
+
+//--- modules2.map
+module all_textual {
+  module foo {
+    textual header "foo.h"
+    export *
+  }
+  module wrap_foo {
+    textual header "wrap_foo.h"
+    export *
+  }
+  export *
+}
+
+module b {
+  module wrap_foo {
+    private header "wrap_foo.h"
+    export *
+  }
+  export *
+}
+
+
+//--- foo.h
+#ifndef FOO_H
+#define FOO_H
+void foo();
+#endif
+
+//--- wrap_foo.h
+#include "foo.h"
+
+//--- use.cpp
+#include "wrap_foo.h"
+
+void test() {
+  foo();
+}

[ilya-biryukov]

This change will fix the breakages we've had internally with #138227, unblocking it.