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

[Jason Merrill <jason@redhat.com>]

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