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

[Dmitry Polukhin via cfe-commits]

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

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, making them hard to use, as seen in the 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 as identical. I think it might be reasonable default behavior instead of `-fskip-odr-check-in-gmf` for header units and GMF. It might be reasonable compromise for checking some ODRs but allow most common issues.
```
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

>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] [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}}