[clang] [RFC][C++20][Modules] Relax ODR check in unnamed modules (PR #111160)
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 8 05:26:58 PDT 2024
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/3] [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/3] 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}}
>From a1a9ef3fc7cd2bc5c16f1ac470d2b7e37f8039d2 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Tue, 8 Oct 2024 05:26:07 -0700
Subject: [PATCH 3/3] Test for header unit specifically.
---
clang/include/clang/AST/DeclBase.h | 3 +++
clang/lib/AST/DeclBase.cpp | 4 ++++
2 files changed, 7 insertions(+)
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index ee662ed73d7e0e..a3447d19909752 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -687,6 +687,9 @@ class alignas(8) Decl {
/// Whether this declaration comes from a named module.
bool isInNamedModule() const;
+ /// Whether this declaration comes from a header unit.
+ bool isFromHeaderUnit() const;
+
/// Return true if this declaration has an attribute which acts as
/// definition of the entity, such as 'alias' or 'ifunc'.
bool hasDefiningAttr() const;
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
index f42857f20efc44..48b91dca1f6d91 100644
--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -1168,6 +1168,10 @@ bool Decl::isInNamedModule() const {
return getOwningModule() && getOwningModule()->isNamedModule();
}
+bool Decl::isFromHeaderUnit() const {
+ return getOwningModule() && getOwningModule()->isHeaderUnit();
+}
+
static Decl::Kind getKind(const Decl *D) { return D->getKind(); }
static Decl::Kind getKind(const DeclContext *DC) { return DC->getDeclKind(); }
More information about the cfe-commits
mailing list