ChuanqiXu created this revision.
ChuanqiXu added reviewers: rsmith, cor3ntin, aaron.ballman, tbaeder.
ChuanqiXu added projects: clang-modules, clang-language-wg.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Close https://github.com/llvm/llvm-project/issues/63544.
Background: We landed std modules in libcxx recently but we haven't landed the
corresponding in-tree tests. According to @Mordante, there are only 1% libcxx
tests failing with std modules. And the major blocking issue is the lambda
expression in the require clauses.
The root cause of the issue is that previously we never consider any lambda
expression as the same. Per [temp.over.link]p5:
> Two lambda-expressions are never considered equivalent.
I thought this is an oversight at first but @rsmith explains that in the
wording, the program is as if there is only a single definition, and a single
lambda-expression. So we don't need worry about this in the spec. The
explanation makes sense. But it didn't reflect to the implementation directly.
Here is a cycle in the implementation. If we want to merge two definitions, we
need to make sure its implementation are the same. But according to the
explanation above, we need to judge if two lambda-expression are the same by
looking at its parent definitions. So here is the problem.
To solve the problem, I think we have to profile the lambda expressions
actually to get the accurate information. But we can't do this universally. So
in this patch I tried to modify the interface of `Stmt::Profile` and only
profile the lambda expression during the process of merging the constraint
expressions.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D153957
Files:
clang/include/clang/AST/Stmt.h
clang/lib/AST/ASTContext.cpp
clang/lib/AST/StmtProfile.cpp
clang/test/Modules/merge-requires-with-lambdas.cppm
clang/test/Modules/pr63544.cppm
Index: clang/test/Modules/pr63544.cppm
===================================================================
--- /dev/null
+++ clang/test/Modules/pr63544.cppm
@@ -0,0 +1,88 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++23 %t/a.cppm -emit-module-interface -o %t/m-a.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -emit-module-interface -o %t/m-b.pcm
+// RUN: %clang_cc1 -std=c++23 %t/m.cppm -emit-module-interface -o %t/m.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++23 %t/pr63544.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- foo.h
+
+namespace std {
+struct strong_ordering {
+ int n;
+ constexpr operator int() const { return n; }
+ static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+namespace std {
+template <typename _Tp>
+class optional {
+private:
+ using value_type = _Tp;
+ value_type __val_;
+ bool __engaged_;
+public:
+ constexpr bool has_value() const noexcept
+ {
+ return this->__engaged_;
+ }
+
+ constexpr const value_type& operator*() const& noexcept
+ {
+ return __val_;
+ }
+
+ optional(_Tp v) : __val_(v) {
+ __engaged_ = true;
+ }
+};
+
+template <class _Tp>
+concept __is_derived_from_optional = requires(const _Tp& __t) { []<class __Up>(const optional<__Up>&) {}(__t); };
+
+template <class _Tp, class _Up>
+ requires(!__is_derived_from_optional<_Up>)
+constexpr strong_ordering
+operator<=>(const optional<_Tp>& __x, const _Up& __v) {
+ return __x.has_value() ? *__x <=> __v : strong_ordering::less;
+}
+} // namespace std
+
+//--- a.cppm
+module;
+#include "foo.h"
+export module m:a;
+export namespace std {
+ using std::optional;
+ using std::operator<=>;
+}
+
+//--- b.cppm
+module;
+#include "foo.h"
+export module m:b;
+export namespace std {
+ using std::optional;
+ using std::operator<=>;
+}
+
+//--- m.cppm
+export module m;
+export import :a;
+export import :b;
+
+//--- pr63544.cpp
+// expected-no-diagnostics
+import m;
+int pr63544() {
+ std::optional<int> a(43);
+ int t{3};
+ return a<=>t;
+}
Index: clang/test/Modules/merge-requires-with-lambdas.cppm
===================================================================
--- /dev/null
+++ clang/test/Modules/merge-requires-with-lambdas.cppm
@@ -0,0 +1,100 @@
+// Tests that we can merge the concept declarations with lambda well.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 %t/A0.cppm -emit-module-interface -o %t/A0.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A1.cppm -emit-module-interface -o %t/A1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA1.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A2.cppm -emit-module-interface -o %t/A2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA2.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A3.cppm -emit-module-interface -o %t/A3.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA3.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- A.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up&) {}(__t); };
+
+//--- A1.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(__Up) {}(__t); };
+
+//--- A2.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up& __u) {
+ (int)__u;
+}(__t); };
+
+//--- A3.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { [t = '?']<class __Up>(const __Up&) {
+ (int)t;
+}(__t); };
+
+//--- A.cppm
+module;
+#include "A.h"
+export module A;
+export using ::A;
+
+//--- A0.cppm
+module;
+#include "A.h"
+export module A0;
+export using ::A;
+
+//--- TestA.cpp
+// expected-no-diagnostics
+import A;
+import A0;
+
+template <class C>
+void f(C) requires(A<C>) {}
+
+//--- A1.cppm
+module;
+#include "A1.h"
+export module A1;
+export using ::A;
+
+//--- TestA1.cpp
+import A;
+import A1;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A2.cppm
+module;
+#include "A2.h"
+export module A2;
+export using ::A;
+
+//--- TestA2.cpp
+import A;
+import A2;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A3.cppm
+module;
+#include "A3.h"
+export module A3;
+export using ::A;
+
+//--- TestA3.cpp
+import A;
+import A3;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
Index: clang/lib/AST/StmtProfile.cpp
===================================================================
--- clang/lib/AST/StmtProfile.cpp
+++ clang/lib/AST/StmtProfile.cpp
@@ -29,10 +29,12 @@
protected:
llvm::FoldingSetNodeID &ID;
bool Canonical;
+ bool ProfileLambdaExpr;
public:
- StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical)
- : ID(ID), Canonical(Canonical) {}
+ StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical,
+ bool ProfileLambdaExpr)
+ : ID(ID), Canonical(Canonical), ProfileLambdaExpr(ProfileLambdaExpr) {}
virtual ~StmtProfiler() {}
@@ -83,8 +85,10 @@
public:
StmtProfilerWithPointers(llvm::FoldingSetNodeID &ID,
- const ASTContext &Context, bool Canonical)
- : StmtProfiler(ID, Canonical), Context(Context) {}
+ const ASTContext &Context, bool Canonical,
+ bool ProfileLambdaExpr)
+ : StmtProfiler(ID, Canonical, ProfileLambdaExpr), Context(Context) {}
+
private:
void HandleStmtClass(Stmt::StmtClass SC) override {
ID.AddInteger(SC);
@@ -181,7 +185,7 @@
ODRHash &Hash;
public:
StmtProfilerWithoutPointers(llvm::FoldingSetNodeID &ID, ODRHash &Hash)
- : StmtProfiler(ID, false), Hash(Hash) {}
+ : StmtProfiler(ID, false, false), Hash(Hash) {}
private:
void HandleStmtClass(Stmt::StmtClass SC) override {
@@ -2027,14 +2031,27 @@
void
StmtProfiler::VisitLambdaExpr(const LambdaExpr *S) {
- // Do not recursively visit the children of this expression. Profiling the
- // body would result in unnecessary work, and is not safe to do during
- // deserialization.
- VisitStmtNoChildren(S);
+ if (!ProfileLambdaExpr) {
+ // Do not recursively visit the children of this expression. Profiling the
+ // body would result in unnecessary work, and is not safe to do during
+ // deserialization.
+ VisitStmtNoChildren(S);
+
+ // C++20 [temp.over.link]p5:
+ // Two lambda-expressions are never considered equivalent.
+ VisitDecl(S->getLambdaClass());
+
+ return;
+ }
- // C++20 [temp.over.link]p5:
- // Two lambda-expressions are never considered equivalent.
- VisitDecl(S->getLambdaClass());
+ CXXRecordDecl *Lambda = S->getLambdaClass();
+ ID.AddInteger(Lambda->getODRHash());
+
+ for (const auto &Capture : Lambda->captures()) {
+ ID.AddInteger(Capture.getCaptureKind());
+ if (Capture.capturesVariable())
+ VisitDecl(Capture.getCapturedVar());
+ }
}
void
@@ -2388,8 +2405,8 @@
}
void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
- bool Canonical) const {
- StmtProfilerWithPointers Profiler(ID, Context, Canonical);
+ bool Canonical, bool ProfileLambdaExpr) const {
+ StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr);
Profiler.Visit(this);
}
Index: clang/lib/AST/ASTContext.cpp
===================================================================
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6327,8 +6327,8 @@
return true;
llvm::FoldingSetNodeID XCEID, YCEID;
- XCE->Profile(XCEID, *this, /*Canonical=*/true);
- YCE->Profile(YCEID, *this, /*Canonical=*/true);
+ XCE->Profile(XCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
+ YCE->Profile(YCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
return XCEID == YCEID;
}
@@ -6693,13 +6693,8 @@
// ConceptDecl wouldn't be the same if their constraint expression differs.
if (const auto *ConceptX = dyn_cast<ConceptDecl>(X)) {
const auto *ConceptY = cast<ConceptDecl>(Y);
- const Expr *XCE = ConceptX->getConstraintExpr();
- const Expr *YCE = ConceptY->getConstraintExpr();
- assert(XCE && YCE && "ConceptDecl without constraint expression?");
- llvm::FoldingSetNodeID XID, YID;
- XCE->Profile(XID, *this, /*Canonical=*/true);
- YCE->Profile(YID, *this, /*Canonical=*/true);
- if (XID != YID)
+ if (!isSameConstraintExpr(ConceptX->getConstraintExpr(),
+ ConceptY->getConstraintExpr()))
return false;
}
Index: clang/include/clang/AST/Stmt.h
===================================================================
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -1297,8 +1297,13 @@
/// parameters are identified by index/level rather than their
/// declaration pointers) or the exact representation of the statement as
/// written in the source.
+ /// \param ProfileLambdaExpr whether or not to profile lambda expressions.
+ /// When false, the lambda expressions are never considered to be equal to
+ /// other lambda expressions. When true, the lambda expressions with the same
+ /// implementation will be considered to be the same. ProfileLambdaExpr should
+ /// only be true when we try to merge two declarations within modules.
void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
- bool Canonical) const;
+ bool Canonical, bool ProfileLambdaExpr = false) const;
/// Calculate a unique representation for a statement that is
/// stable across compiler invocations.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
