| Message ID | 20240125191804.2574786-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: problematic assert in reference_binding [PR113141] | expand |
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.
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.
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
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 > >
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
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 > >
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}}); +}