[Sema] Compare canonical conversion function #154158
Conversation
With lazy template loading, it is possible to find non-canonical FunctionDecls, depending on when redecl chains are completed. This is a problem for templated conversion operators that would allow to call either the copy assignment or the move assignment operator. This ambiguity is resolved by isBetterReferenceBindingKind (called from CompareStandardConversionSequences) ranking rvalue refs over lvalue refs. Unfortunately, this fix is hard to test in isolation without the changes in llvm#133057 that make lazy template loading more likely to complete redecl chains at "inconvenient" times. The added reproducer passes before and after this commit, but would have failed with the proposed changes of the linked PR. Kudos to Maksim Ivanov for providing an initial version of the reproducer that I further simplified.
hahnjo
force-pushed
the
overload-func-canonical
branch
from
05dce54 to
25ca9b4
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules Author: Jonas Hahnfeld (hahnjo) ChangesWith lazy template loading, it is possible to find non-canonical Unfortunately, this fix is hard to test in isolation without the changes in #133057 that Kudos to @emaxx-google for providing an initial version of the reproducer that I further simplified. Full diff: https://github.com/llvm/llvm-project/pull/154158.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 7c4405b414c47..690125d880293 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -4404,14 +4404,19 @@ CompareImplicitConversionSequences(Sema &S, SourceLocation Loc,
Result = CompareStandardConversionSequences(S, Loc,
ICS1.Standard, ICS2.Standard);
else if (ICS1.isUserDefined()) {
+ auto ConvFunc1 = ICS1.UserDefined.ConversionFunction;
+ if (ConvFunc1)
+ ConvFunc1 = ConvFunc1->getCanonicalDecl();
+ auto ConvFunc2 = ICS2.UserDefined.ConversionFunction;
+ if (ConvFunc2)
+ ConvFunc2 = ConvFunc2->getCanonicalDecl();
// User-defined conversion sequence U1 is a better conversion
// sequence than another user-defined conversion sequence U2 if
// they contain the same user-defined conversion function or
// constructor and if the second standard conversion sequence of
// U1 is better than the second standard conversion sequence of
// U2 (C++ 13.3.3.2p3).
- if (ICS1.UserDefined.ConversionFunction ==
- ICS2.UserDefined.ConversionFunction)
+ if (ConvFunc1 == ConvFunc2)
Result = CompareStandardConversionSequences(S, Loc,
ICS1.UserDefined.After,
ICS2.UserDefined.After);
diff --git a/clang/test/Modules/pr133057.cpp b/clang/test/Modules/pr133057.cpp
new file mode 100644
index 0000000000000..b273fc318058e
--- /dev/null
+++ b/clang/test/Modules/pr133057.cpp
@@ -0,0 +1,143 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=hf -fno-cxx-modules -fmodules -fno-implicit-modules %t/CMO.cppmap -o %t/WI9.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=g -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm %t/E6H.cppmap -o %t/4BK.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=r -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm %t/HMT.cppmap -o %t/LUM.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=q -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/LUM.pcm -fmodule-file=%t/4BK.pcm %t/JOV.cppmap -o %t/9VX.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -verify -fsyntax-only -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/9VX.pcm %t/XFD.cc
+
+//--- 2OT.h
+#include "LQ1.h"
+
+namespace ciy {
+namespace xqk {
+template <typename>
+class vum {
+ public:
+ using sc = std::C::wmd;
+ friend bool operator==(vum, vum);
+};
+template <typename>
+class me {
+ public:
+ using vbh = vum<me>;
+ using sc = std::C::vy<vbh>::sc;
+ template <typename db>
+ operator db() { return {}; }
+};
+} // namespace xqk
+template <typename vus>
+xqk::me<vus> uvo(std::C::wmd, vus);
+} // namespace ciy
+
+class ua {
+ std::C::wmd kij() {
+ ciy::uvo(kij(), '-');
+ return {};
+ }
+};
+
+//--- 9KF.h
+#include "LQ1.h"
+#include "2OT.h"
+namespace {
+void al(std::C::wmd lou) { std::C::jv<std::C::wmd> yt = ciy::uvo(lou, '/'); }
+} // namespace
+
+//--- CMO.cppmap
+module "hf" {
+header "LQ1.h"
+}
+
+
+//--- E6H.cppmap
+module "g" {
+export *
+header "2OT.h"
+}
+
+
+//--- HMT.cppmap
+module "r" {
+header "2OT.h"
+}
+
+
+//--- JOV.cppmap
+module "q" {
+header "9KF.h"
+}
+
+
+//--- LQ1.h
+namespace std {
+namespace C {
+template <class zd>
+struct vy : zd {};
+template <class ub>
+struct vy<ub*> {
+ typedef ub jz;
+};
+struct wmd {};
+template <class uo, class zt>
+void sk(uo k, zt gf) {
+ (void)(k != gf);
+}
+template <class uo>
+class fm {
+ public:
+ fm(uo);
+};
+template <class kj, class kju>
+bool operator==(kj, kju);
+template <class epn>
+void afm(epn) {
+ using yp = vy<epn>;
+ if (__is_trivially_copyable(yp)) {
+ sk(fm(epn()), nullptr);
+ }
+}
+template <class ub>
+class jv {
+ public:
+ constexpr void gq();
+ ub *nef;
+};
+template <class ub>
+constexpr void jv<ub>::gq() {
+ afm(nef);
+}
+} // namespace C
+} // namespace std
+namespace ciy {
+} // namespace ciy
+
+//--- XFD.cc
+// expected-no-diagnostics
+#include "LQ1.h"
+#include "2OT.h"
+class wiy {
+ public:
+ std::C::wmd eyb();
+};
+template <typename wpa>
+void i(wpa fg) {
+ std::C::jv<std::C::wmd> zs;
+ zs = ciy::uvo(fg.eyb(), '\n');
+}
+namespace ciy {
+namespace xqk {
+struct sbv;
+std::C::jv<sbv> ns() {
+ std::C::jv<sbv> ubs;
+ ubs.gq();
+ return ubs;
+}
+} // namespace xqk
+} // namespace ciy
+void s() {
+ wiy fg;
+ i(fg);
+}
|
|
@llvm/pr-subscribers-clang Author: Jonas Hahnfeld (hahnjo) ChangesWith lazy template loading, it is possible to find non-canonical Unfortunately, this fix is hard to test in isolation without the changes in #133057 that Kudos to @emaxx-google for providing an initial version of the reproducer that I further simplified. Full diff: https://github.com/llvm/llvm-project/pull/154158.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 7c4405b414c47..690125d880293 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -4404,14 +4404,19 @@ CompareImplicitConversionSequences(Sema &S, SourceLocation Loc,
Result = CompareStandardConversionSequences(S, Loc,
ICS1.Standard, ICS2.Standard);
else if (ICS1.isUserDefined()) {
+ auto ConvFunc1 = ICS1.UserDefined.ConversionFunction;
+ if (ConvFunc1)
+ ConvFunc1 = ConvFunc1->getCanonicalDecl();
+ auto ConvFunc2 = ICS2.UserDefined.ConversionFunction;
+ if (ConvFunc2)
+ ConvFunc2 = ConvFunc2->getCanonicalDecl();
// User-defined conversion sequence U1 is a better conversion
// sequence than another user-defined conversion sequence U2 if
// they contain the same user-defined conversion function or
// constructor and if the second standard conversion sequence of
// U1 is better than the second standard conversion sequence of
// U2 (C++ 13.3.3.2p3).
- if (ICS1.UserDefined.ConversionFunction ==
- ICS2.UserDefined.ConversionFunction)
+ if (ConvFunc1 == ConvFunc2)
Result = CompareStandardConversionSequences(S, Loc,
ICS1.UserDefined.After,
ICS2.UserDefined.After);
diff --git a/clang/test/Modules/pr133057.cpp b/clang/test/Modules/pr133057.cpp
new file mode 100644
index 0000000000000..b273fc318058e
--- /dev/null
+++ b/clang/test/Modules/pr133057.cpp
@@ -0,0 +1,143 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=hf -fno-cxx-modules -fmodules -fno-implicit-modules %t/CMO.cppmap -o %t/WI9.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=g -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm %t/E6H.cppmap -o %t/4BK.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=r -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm %t/HMT.cppmap -o %t/LUM.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=q -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/LUM.pcm -fmodule-file=%t/4BK.pcm %t/JOV.cppmap -o %t/9VX.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -verify -fsyntax-only -fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/9VX.pcm %t/XFD.cc
+
+//--- 2OT.h
+#include "LQ1.h"
+
+namespace ciy {
+namespace xqk {
+template <typename>
+class vum {
+ public:
+ using sc = std::C::wmd;
+ friend bool operator==(vum, vum);
+};
+template <typename>
+class me {
+ public:
+ using vbh = vum<me>;
+ using sc = std::C::vy<vbh>::sc;
+ template <typename db>
+ operator db() { return {}; }
+};
+} // namespace xqk
+template <typename vus>
+xqk::me<vus> uvo(std::C::wmd, vus);
+} // namespace ciy
+
+class ua {
+ std::C::wmd kij() {
+ ciy::uvo(kij(), '-');
+ return {};
+ }
+};
+
+//--- 9KF.h
+#include "LQ1.h"
+#include "2OT.h"
+namespace {
+void al(std::C::wmd lou) { std::C::jv<std::C::wmd> yt = ciy::uvo(lou, '/'); }
+} // namespace
+
+//--- CMO.cppmap
+module "hf" {
+header "LQ1.h"
+}
+
+
+//--- E6H.cppmap
+module "g" {
+export *
+header "2OT.h"
+}
+
+
+//--- HMT.cppmap
+module "r" {
+header "2OT.h"
+}
+
+
+//--- JOV.cppmap
+module "q" {
+header "9KF.h"
+}
+
+
+//--- LQ1.h
+namespace std {
+namespace C {
+template <class zd>
+struct vy : zd {};
+template <class ub>
+struct vy<ub*> {
+ typedef ub jz;
+};
+struct wmd {};
+template <class uo, class zt>
+void sk(uo k, zt gf) {
+ (void)(k != gf);
+}
+template <class uo>
+class fm {
+ public:
+ fm(uo);
+};
+template <class kj, class kju>
+bool operator==(kj, kju);
+template <class epn>
+void afm(epn) {
+ using yp = vy<epn>;
+ if (__is_trivially_copyable(yp)) {
+ sk(fm(epn()), nullptr);
+ }
+}
+template <class ub>
+class jv {
+ public:
+ constexpr void gq();
+ ub *nef;
+};
+template <class ub>
+constexpr void jv<ub>::gq() {
+ afm(nef);
+}
+} // namespace C
+} // namespace std
+namespace ciy {
+} // namespace ciy
+
+//--- XFD.cc
+// expected-no-diagnostics
+#include "LQ1.h"
+#include "2OT.h"
+class wiy {
+ public:
+ std::C::wmd eyb();
+};
+template <typename wpa>
+void i(wpa fg) {
+ std::C::jv<std::C::wmd> zs;
+ zs = ciy::uvo(fg.eyb(), '\n');
+}
+namespace ciy {
+namespace xqk {
+struct sbv;
+std::C::jv<sbv> ns() {
+ std::C::jv<sbv> ubs;
+ ubs.gq();
+ return ubs;
+}
+} // namespace xqk
+} // namespace ciy
+void s() {
+ wiy fg;
+ i(fg);
+}
|
You can see the test failing with the proposed changes of #133057 here: https://github.com/llvm/llvm-project/actions/runs/17069513671/job/48394595749?pr=133057#step:3:7985 |
|
I don't feel bad for the patch itself.
But this may be concerning. Do you feel this may cause more problem? |
Of course this can result in more problems like the one fixed here. That is the box we opened with lazy loading of template instantiations. |
let's wait for the testing response from @emaxx-google and @ilya-biryukov |
|
@ilya-biryukov @emaxx-google would it be possible to test this PR in isolation while we are waiting for more details on #133057? |
I agree. Can we improve the response times -- I feel that longer times require quite difficult context switching on our end. |
We would love to respond quickly, but our internal compiler testing/release is a lot of work, and we might be catching up slowly depending on how disruptive the recent commits are for our code base. Right now, we are quite unhappy, but we'll prioritize this as much as we can. Sorry for the delay. |
I tested this out and didn't notice any breakages. I think this is fine to land if you already have review of the code itself. In the future, you can also tag |
With lazy template loading, it is possible to find non-canonical
FunctionDecls, depending on when redecl chains are completed. This
is a problem for templated conversion operators that would allow to
call either the copy assignment or the move assignment operator.
This ambiguity is resolved by isBetterReferenceBindingKind (called
from CompareStandardConversionSequences) ranking rvalue refs over
lvalue refs.
Unfortunately, this fix is hard to test in isolation without the
changes in llvm#133057 that
make lazy template loading more likely to complete redecl chains
at "inconvenient" times. The added reproducer passes before and
after this commit, but would have failed with the proposed changes
of the linked PR.
Kudos to Maksim Ivanov for providing an initial version of the
reproducer that I further simplified.
9 participants
With lazy template loading, it is possible to find non-canonical
FunctionDecls, depending on when redecl chains are completed. This is a problem for templated conversion operators that would allow to call either the copy assignment or the move assignment operator. This ambiguity is resolved byisBetterReferenceBindingKind(called fromCompareStandardConversionSequences) ranking rvalue refs over lvalue refs.Unfortunately, this fix is hard to test in isolation without the changes in #133057 that make lazy template loading more likely to complete redecl chains at "inconvenient" times. The added reproducer passes before and after this commit, but would have failed with the proposed changes of the linked PR.
Kudos to @emaxx-google for providing an initial version of the reproducer that I further simplified.