[RFC][C++20][Modules] Relax ODR check in unnamed modules #111160

dmpolukhin · 2024-10-04T14:00:11Z

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

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

llvmbot · 2024-10-04T14:00:47Z

@llvm/pr-subscribers-clang

Author: Dmitry Polukhin (dmpolukhin)

Changes

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 '&lt;dependent type&gt;'
|-&lt;&lt;&lt;NULL&gt;&gt;&gt;
|-UnresolvedLookupExpr 0x55556588b140 '&lt;dependent type&gt;' 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'
`-&lt;&lt;&lt;NULL&gt;&gt;&gt;

CXXFoldExpr 0x55556588b670 '&lt;dependent type&gt;'
|-UnresolvedLookupExpr 0x55556588b628 '&lt;overloaded function type&gt;' lvalue (ADL) = 'operator|' 0x55556588ae60
|-UnresolvedLookupExpr 0x55556588b5b0 '&lt;dependent type&gt;' 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'
`-&lt;&lt;&lt;NULL&gt;&gt;&gt;

Test Plan: check-clang

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

2 Files Affected:

(modified) clang/include/clang/Serialization/ASTReader.h (+1-1)
(added) clang/test/Headers/header-unit-common-cmp-cat.cpp (+32)

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

llvmbot · 2024-10-04T14:00:48Z

@llvm/pr-subscribers-clang-modules

Author: Dmitry Polukhin (dmpolukhin)

Changes

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 '&lt;dependent type&gt;'
|-&lt;&lt;&lt;NULL&gt;&gt;&gt;
|-UnresolvedLookupExpr 0x55556588b140 '&lt;dependent type&gt;' 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'
`-&lt;&lt;&lt;NULL&gt;&gt;&gt;

CXXFoldExpr 0x55556588b670 '&lt;dependent type&gt;'
|-UnresolvedLookupExpr 0x55556588b628 '&lt;overloaded function type&gt;' lvalue (ADL) = 'operator|' 0x55556588ae60
|-UnresolvedLookupExpr 0x55556588b5b0 '&lt;dependent type&gt;' 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'
`-&lt;&lt;&lt;NULL&gt;&gt;&gt;

Test Plan: check-clang

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

2 Files Affected:

(modified) clang/include/clang/Serialization/ASTReader.h (+1-1)
(added) clang/test/Headers/header-unit-common-cmp-cat.cpp (+32)

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

clang/include/clang/Serialization/ASTReader.h

ChuanqiXu9 · 2024-10-08T02:17:03Z

clang/test/Headers/header-unit-common-cmp-cat.cpp

+template <class T>
+int operator|(T, T);
+
+#include "compare"


But this case, I feel this is a real ODR violation. The intention of skipping ODR checks in GMF is we have too many false positive ODR violation diagnostics. But I prefer to not make it for real ODR violations.

I updated test case to be closer to the real issue with gcc libstdc++. The test case is identical to polluted-operator.cppm but with header units. Depending on include order it has ODR or not, it is non-intentional violation that is hard to avoid at sale.

dmpolukhin requested a review from ChuanqiXu9 October 4, 2024 14:00

llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Oct 4, 2024

ChuanqiXu9 reviewed Oct 8, 2024

View reviewed changes

dmpolukhin added 2 commits October 8, 2024 05:13

Make test closer to real issue in gcc libstc++

6cb15c6

Test for header unit specifically.

a1a9ef3

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 8, 2024

cor3ntin requested a review from Bigcheese October 8, 2024 18:28

Use isFromHeaderUnit for real this time

7329e60

ChuanqiXu9 approved these changes Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][C++20][Modules] Relax ODR check in unnamed modules #111160

[RFC][C++20][Modules] Relax ODR check in unnamed modules #111160

dmpolukhin commented Oct 4, 2024

llvmbot commented Oct 4, 2024

llvmbot commented Oct 4, 2024

ChuanqiXu9 Oct 8, 2024

dmpolukhin Oct 8, 2024

[RFC][C++20][Modules] Relax ODR check in unnamed modules #111160

Are you sure you want to change the base?

[RFC][C++20][Modules] Relax ODR check in unnamed modules #111160

Conversation

dmpolukhin commented Oct 4, 2024

llvmbot commented Oct 4, 2024

llvmbot commented Oct 4, 2024

ChuanqiXu9 Oct 8, 2024

Choose a reason for hiding this comment

dmpolukhin Oct 8, 2024

Choose a reason for hiding this comment