[clang] [RFC][C++20][Modules] Relax ODR check in unnamed modules (PR #111160)

[Dmitry Polukhin via cfe-commits]

https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/111160

>From dc4a79c2ee5630eb551ea3a40b4bd67da20c7034 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Fri, 4 Oct 2024 06:41:34 -0700
Subject: [PATCH 1/2] [RFC][C++20][Modules] Relax ODR check in unnamed modules
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Summary:
Option `-fskip-odr-check-in-gmf` is set by default and I think it is what most of C++ developers want. But in header units clang ODR checking is too strict that makes them hard to use, see example in the diff. This diff relaxes ODR checks for unnamed modules to match GMF ODR checking.

Alternative solution is to add special handling for CXXFoldExpr to ignore differences in “callee”. In particular it will treat the following fragments identical. I think it might be reasonable default behavior instead of `-fskip-odr-check-in-gmf`.
```
CXXFoldExpr 0x55556588b1b8 '<dependent type>'
|-<<<NULL>>>
|-UnresolvedLookupExpr 0x55556588b140 '<dependent type>' lvalue (no ADL) = '__cmp_cat_id' 0x55556588ac18
| `-TemplateArgument type '_Ts'
|   `-TemplateTypeParmType 0x55556588adf0 '_Ts' dependent contains_unexpanded_pack depth 0 index 0 pack
|     `-TemplateTypeParm 0x55556588ad70 '_Ts'
`-<<<NULL>>>

CXXFoldExpr 0x55556588b670 '<dependent type>'
|-UnresolvedLookupExpr 0x55556588b628 '<overloaded function type>' lvalue (ADL) = 'operator|' 0x55556588ae60
|-UnresolvedLookupExpr 0x55556588b5b0 '<dependent type>' lvalue (no ADL) = '__cmp_cat_id' 0x55556588b0d8
| `-TemplateArgument type '_Ts'
|   `-TemplateTypeParmType 0x55556588b260 '_Ts' dependent contains_unexpanded_pack depth 0 index 0 pack
|     `-TemplateTypeParm 0x55556588b1e0 '_Ts'
`-<<<NULL>>>
```

Test Plan: check-clang
---
 clang/include/clang/Serialization/ASTReader.h |  2 +-
 .../Headers/header-unit-common-cmp-cat.cpp    | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Headers/header-unit-common-cmp-cat.cpp

diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index aa88560b259a3b..109c905d2bebb6 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2527,7 +2527,7 @@ class BitsUnpacker {
 
 inline bool shouldSkipCheckingODR(const Decl *D) {
   return D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
-         D->isFromGlobalModule();
+         (D->isFromGlobalModule() || !D->isInNamedModule());
 }
 
 } // namespace clang
diff --git a/clang/test/Headers/header-unit-common-cmp-cat.cpp b/clang/test/Headers/header-unit-common-cmp-cat.cpp
new file mode 100644
index 00000000000000..b9d00ba042fda5
--- /dev/null
+++ b/clang/test/Headers/header-unit-common-cmp-cat.cpp
@@ -0,0 +1,32 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header bz0.h
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header bz1.h
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header -fmodule-file=bz0.pcm -fmodule-file=bz1.pcm bz.cpp
+
+//--- compare
+template<typename _Tp>
+inline constexpr unsigned __cmp_cat_id = 1;
+
+template<typename... _Ts>
+constexpr auto __common_cmp_cat() {
+  (__cmp_cat_id<_Ts> | ...);
+}
+
+//--- bz0.h
+template <class T>
+int operator|(T, T);
+
+#include "compare"
+// expected-no-diagnostics
+
+//--- bz1.h
+#include "compare"
+// expected-no-diagnostics
+
+//--- bz.cpp
+#include "compare"
+
+import "bz0.h"; // expected-warning {{the implementation of header units is in an experimental phase}}
+import "bz1.h"; // expected-warning {{the implementation of header units is in an experimental phase}}

>From 6cb15c6a9a2ec5fccfef354922869bcf8f67f6fc Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Tue, 8 Oct 2024 05:13:43 -0700
Subject: [PATCH 2/2] Make test closer to real issue in gcc libstc++

---
 .../Headers/header-unit-common-cmp-cat.cpp    | 24 ++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/clang/test/Headers/header-unit-common-cmp-cat.cpp b/clang/test/Headers/header-unit-common-cmp-cat.cpp
index b9d00ba042fda5..faeb3ec32127f5 100644
--- a/clang/test/Headers/header-unit-common-cmp-cat.cpp
+++ b/clang/test/Headers/header-unit-common-cmp-cat.cpp
@@ -1,11 +1,14 @@
 // RUN: rm -fR %t
 // RUN: split-file %s %t
 // RUN: cd %t
-// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header bz0.h
-// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header bz1.h
-// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -emit-header-unit -xc++-user-header -fmodule-file=bz0.pcm -fmodule-file=bz1.pcm bz.cpp
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -I. -emit-header-unit -xc++-user-header bz1.h
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -I. -emit-header-unit -xc++-user-header bz2.h
+// RUN: %clang_cc1 -verify -std=c++20 -fskip-odr-check-in-gmf -I. -emit-header-unit -xc++-user-header -fmodule-file=bz1.pcm -fmodule-file=bz2.pcm bz.cpp
 
 //--- compare
+namespace std {
+namespace __detail {
+
 template<typename _Tp>
 inline constexpr unsigned __cmp_cat_id = 1;
 
@@ -14,19 +17,24 @@ constexpr auto __common_cmp_cat() {
   (__cmp_cat_id<_Ts> | ...);
 }
 
+} // namespace __detail
+} // namespace std
+
 //--- bz0.h
 template <class T>
 int operator|(T, T);
 
-#include "compare"
+//--- bz1.h
+#include "bz0.h"
+#include <compare>
 // expected-no-diagnostics
 
-//--- bz1.h
-#include "compare"
+//--- bz2.h
+#include <compare>
 // expected-no-diagnostics
 
 //--- bz.cpp
-#include "compare"
+#include <compare>
 
-import "bz0.h"; // expected-warning {{the implementation of header units is in an experimental phase}}
 import "bz1.h"; // expected-warning {{the implementation of header units is in an experimental phase}}
+import "bz2.h"; // expected-warning {{the implementation of header units is in an experimental phase}}