On Wed, 1 May 2024, Patrick Palka wrote: > On Wed, 1 May 2024, Jason Merrill wrote: > > > On 4/12/24 13:22, Patrick Palka wrote: > > > On Fri, 12 Apr 2024, Jason Merrill wrote: > > > > > > > On 3/26/24 09:44, Patrick Palka wrote: > > > > > On Thu, 7 Mar 2024, Jason Merrill wrote: > > > > > > > > > > > On 1/29/24 17:42, Patrick Palka wrote: > > > > > > > On Mon, 29 Jan 2024, Patrick Palka wrote: > > > > > > > > > > > > > > > On Fri, 26 Jan 2024, Jason Merrill wrote: > > > > > > > > > > > > > > > > > On 1/26/24 17:11, Jason Merrill wrote: > > > > > > > > > > On 1/26/24 16:52, Jason Merrill wrote: > > > > > > > > > > > On 1/25/24 14:18, Patrick Palka wrote: > > > > > > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does > > > > > > > > > > > > this > > > > > > > > > > > > look > > > > > > > > > > > > OK for trunk/13? This isn't a very satisfactory fix, > > > > > > > > > > > > but > > > > > > > > > > > > at > > > > > > > > > > > > least > > > > > > > > > > > > it safely fixes these testcases I guess. Note that > > > > > > > > > > > > there's > > > > > > > > > > > > implementation disagreement about the second testcase, > > > > > > > > > > > > GCC > > > > > > > > > > > > always > > > > > > > > > > > > accepted it but Clang/MSVC/icc reject it. > > > > > > > > > > > > > > > > > > > > > > Because of trying to initialize int& from {c}; removing > > > > > > > > > > > the > > > > > > > > > > > extra > > > > > > > > > > > braces > > > > > > > > > > > makes it work everywhore. > > > > > > > > > > > > > > > > > > > > > > https://eel.is/c++draft/dcl.init#list-3.10 says that we > > > > > > > > > > > always > > > > > > > > > > > generate a > > > > > > > > > > > prvalue in this case, so perhaps we shouldn't recalculate > > > > > > > > > > > if > > > > > > > > > > > the > > > > > > > > > > > initializer is an init-list? > > > > > > > > > > > > > > > > > > > > ...but it seems bad to silently bind a const int& to a > > > > > > > > > > prvalue > > > > > > > > > > instead > > > > > > > > > > of > > > > > > > > > > directly to the reference returned by the operator, as clang > > > > > > > > > > does > > > > > > > > > > if > > > > > > > > > > we add > > > > > > > > > > const to the second testcase, so I think there's a defect in > > > > > > > > > > the > > > > > > > > > > standard > > > > > > > > > > here. > > > > > > > > > > > > > > > > > > Perhaps bullet 3.9 should change to "...its referenced type is > > > > > > > > > reference-related to E <ins>or scalar</ins>, ..." > > > > > > > > > > > > > > > > > > > Maybe for now also disable the maybe_valid heuristics in the > > > > > > > > > > case > > > > > > > > > > of > > > > > > > > > > an > > > > > > > > > > init-list? > > > > > > > > > > > > > > > > > > > > > The first testcase is special because it's a C-style cast; > > > > > > > > > > > seems > > > > > > > > > > > like the > > > > > > > > > > > maybe_valid = false heuristics should be disabled if > > > > > > > > > > > c_cast_p. > > > > > > > > > > > > > > > > Thanks a lot for the pointers. IIUC c_cast_p and > > > > > > > > LOOKUP_SHORTCUT_BAD_CONVS > > > > > > > > should already be mutually exclusive, since the latter is set > > > > > > > > only > > > > > > > > when > > > > > > > > computing argument conversions, so it shouldn't be necessary to > > > > > > > > check > > > > > > > > c_cast_p. > > > > > > > > > > > > > > > > I suppose we could disable the heuristic for init-lists, but > > > > > > > > after > > > > > > > > some > > > > > > > > digging I noticed that the heuristics were originally in same > > > > > > > > spot > > > > > > > > they > > > > > > > > are now until r5-601-gd02f620dc0bb3b moved them to get checked > > > > > > > > after > > > > > > > > the recursive recalculation case in reference_binding, > > > > > > > > returning a > > > > > > > > bad > > > > > > > > conversion instead of NULL. (Then in > > > > > > > > r13-1755-g68f37670eff0b872 I > > > > > > > > moved > > > > > > > > them back; IIRC that's why I felt confident that moving the > > > > > > > > checks > > > > > > > > was > > > > > > > > safe.) > > > > > > > > Thus we didn't always accept the second testcase, we only > > > > > > > > started > > > > > > > > doing so > > > > > > > > in > > > > > > > > GCC 5: https://godbolt.org/z/6nsEW14fh (sorry for missing this > > > > > > > > and > > > > > > > > saying > > > > > > > > we > > > > > > > > always accepted it) > > > > > > > > > > > > > > > > And indeed the current order of checks seems consistent with > > > > > > > > that > > > > > > > > of > > > > > > > > [dcl.init.ref]/5. So I wonder if we don't instead want to > > > > > > > > "complete" > > > > > > > > the NULL-to-bad-conversion adjustment in r5-601-gd02f620dc0bb3b > > > > > > > > and > > > > > > > > do: > > > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > > > * call.cc (reference_binding): Set bad_p according to > > > > > > > > maybe_valid_p in the recursive case as well. Remove > > > > > > > > redundant gcc_assert. > > > > > > > > > > > > > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > > > > > > > > index 9de0d77c423..c4158b2af37 100644 > > > > > > > > --- a/gcc/cp/call.cc > > > > > > > > +++ b/gcc/cp/call.cc > > > > > > > > @@ -2033,8 +2033,8 @@ reference_binding (tree rto, tree rfrom, > > > > > > > > tree > > > > > > > > expr, > > > > > > > > bool c_cast_p, int flags, > > > > > > > > sflags, complain); > > > > > > > > if (!new_second) > > > > > > > > return bad_direct_conv ? bad_direct_conv : > > > > > > > > nullptr; > > > > > > > > + t->bad_p = !maybe_valid_p; > > > > > > > > > > > > > > Oops, that should be |= not =. > > > > > > > > > > > > > > > > Perhaps bullet 3.9 should change to "...its referenced type is > > > > > > > > > reference-related to E <ins>or scalar</ins>, ..." > > > > > > > > conv = merge_conversion_sequences (t, new_second); > > > > > > > > - gcc_assert (maybe_valid_p || conv->bad_p); > > > > > > > > return conv; > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > This'd mean we'd go back to rejecting the second testcase (only > > > > > > > > the > > > > > > > > call, not the direct-init, interestingly enough), but that seems > > > > > > > > to be > > > > > > > > > > > > > > In the second testcase, with the above fix initialize_reference > > > > > > > silently > > > > > > > returns error_mark_node for the direct-init without issuing a > > > > > > > diagnostic, because in the error path convert_like doesn't find > > > > > > > anything > > > > > > > wrong with the bad conversion. So more changes need to be made if > > > > > > > we > > > > > > > want to set bad_p in the recursive case of reference_binding it > > > > > > > seems; > > > > > > > dunno if that's the path we want to go down? > > > > > > > > > > > > > > On the other hand, disabling the badness checks in certain cases > > > > > > > seems > > > > > > > to be undesirable as well, since AFAICT their current position is > > > > > > > consistent with [dcl.init.ref]/5? > > > > > > > > > > > > > > So I wonder if we should just go with the safest thing at this > > > > > > > stage, > > > > > > > which would be the original patch that removes the problematic > > > > > > > assert? > > > > > > > > > > > > I still think the assert is correct, and the problem is that > > > > > > maybe_valid_p > > > > > > is > > > > > > wrong; these cases turn out to be valid, so maybe_valid_p should be > > > > > > true. > > > > > > > > > > I'm afraid then I don't know how we can statically identify these > > > > > cases > > > > > without actually performing the conversion, in light of the recursion > > > > > :/ > > > > > Do you mind taking this PR? I don't feel well-versed enough with the > > > > > reference binding rules to tackle this adequately.. > > > > > > > > That ended up being a surprisingly deep dive, but I've now checked in > > > > separate > > > > fixes for the two cases. > > > > > > Very interesting, thanks a lot. > > > > ...but I don't think my fixes are suitable for GCC 13, so would you apply > > your > > original patch to the 13 branch? > > Will do
Pushed as r13-8670: -- >8 -- Subject: [PATCH] c++: problematic assert in reference_binding [PR113141] r14-9946 / r14-9947 fixed this PR properly for GCC 14. For GCC 13, let's just remove the problematic assert. PR c++/113141 gcc/cp/ChangeLog: * call.cc (reference_binding): Remove badness criteria sanity check in the recursive case. gcc/testsuite/ChangeLog: * g++.dg/conversion/ref12.C: New test. * g++.dg/cpp0x/initlist-ref1.C: new test. --- gcc/cp/call.cc | 1 - gcc/testsuite/g++.dg/conversion/ref12.C | 13 +++++++++++++ gcc/testsuite/g++.dg/cpp0x/initlist-ref1.C | 16 ++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/conversion/ref12.C create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-ref1.C diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index b10bdc62d38..70c7f6178b8 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -2017,7 +2017,6 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags, if (!new_second) return NULL; conv = merge_conversion_sequences (t, new_second); - gcc_assert (maybe_valid_p || conv->bad_p); return conv; } } diff --git a/gcc/testsuite/g++.dg/conversion/ref12.C b/gcc/testsuite/g++.dg/conversion/ref12.C new file mode 100644 index 00000000000..633b7e48e47 --- /dev/null +++ b/gcc/testsuite/g++.dg/conversion/ref12.C @@ -0,0 +1,13 @@ +// PR c++/113141 + +struct Matrix { }; + +struct TPoint3 { operator const Matrix(); }; + +void f(Matrix&); + +int main() { + TPoint3 X; + Matrix& m = (Matrix &)X; + f((Matrix &)X); +} diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-ref1.C b/gcc/testsuite/g++.dg/cpp0x/initlist-ref1.C new file mode 100644 index 00000000000..f893f12dafa --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-ref1.C @@ -0,0 +1,16 @@ +// PR c++/113141 +// { dg-do compile { target c++11 } } + +struct ConvToRef { + operator int&(); +}; + +struct A { int& r; }; + +void f(A); + +int main() { + ConvToRef c; + A a{{c}}; + f({{c}}); +} -- 2.45.0