[PATCH] c++: problematic assert in reference_binding [PR113141]

[PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

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.

-- >8 --

In the bad reference binding shortcutting commit r13-1755-g68f37670eff0b872
I made us check the badness criteria in reference_binding earlier in
the function so that we can fail fast and avoid unnecessary template
instantiation during the first pass of overload resolution.  This was
for the most part obviously safe, except now the badness criteria are
also checked before the recursive case[1] in reference_binding, whereas
before the criteria didn't apply in that case.  So in order to justify
this hoisting I added a sanity check that the badness criteria are still
sound in the recursive case.

Unfortunately the below testcases triggers the sanity check.  I'm not
sure if this means the bad conversion shortcutting is unsound since I
wasn't able to construct a testcase that affects overload resolution.
And if it is unsound, I'm not sure how we can make it sound in light of
this recursive logic for non-direct user-defined conversions.  But we
can at least restore the pre-r13-1755 behavior for the below two
testcases by simply getting rid of this sanity check.

[1]: Added in
https://gcc.gnu.org/pipermail/gcc-patches/2014-April/386365.html

	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/ref10.C: New test.
	* g++.dg/conversion/ref11.C: New test.
---
 gcc/cp/call.cc                          |  1 -
 gcc/testsuite/g++.dg/conversion/ref10.C | 13 +++++++++++++
 gcc/testsuite/g++.dg/conversion/ref11.C | 16 ++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref10.C
 create mode 100644 gcc/testsuite/g++.dg/conversion/ref11.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 9de0d77c423..2dce52bc7b8 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -2034,7 +2034,6 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags,
 	    if (!new_second)
 	      return bad_direct_conv ? bad_direct_conv : nullptr;
 	    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/ref10.C b/gcc/testsuite/g++.dg/conversion/ref10.C
new file mode 100644
index 00000000000..633b7e48e47
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref10.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/conversion/ref11.C b/gcc/testsuite/g++.dg/conversion/ref11.C
new file mode 100644
index 00000000000..f893f12dafa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref11.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.43.0.386.ge02ecfcc53

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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?

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.

Jason

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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.

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.
> 
> Jason

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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.
>>
>> Jason
> 

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

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;
 	    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
the correct behavior anyway IIUC.  The testsuite is otherwise happy
with this change.

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 4440 bytes --]

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?

> the correct behavior anyway IIUC.  The testsuite is otherwise happy
> with this change.

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 4687 bytes --]

On Mon, 29 Jan 2024, 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?

Ping.

> 
> > the correct behavior anyway IIUC.  The testsuite is otherwise happy
> > with this change.

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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.

Jason

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 5376 bytes --]

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

> 
> Jason
> 
> 

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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.

Jason

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 6275 bytes --]

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.

> 
> Jason
> 
> 

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Jason Merrill]

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?

Jason

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 7525 bytes --]

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

> 
> Jason
> 
> 

Re: [PATCH] c++: problematic assert in reference_binding [PR113141]

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 9990 bytes --]

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