c++: problematic assert in reference_binding [PR113141]
Checks
Commit Message
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
Comments
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
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
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
>
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.
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.
@@ -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;
}
}
new file mode 100644
@@ -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);
+}
new file mode 100644
@@ -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}});
+}