| Message ID | 20191129233309.GO10088@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] Fix ICE in build_new_op_1 (PR c++/92705) | expand |
On 11/29/19 6:33 PM, Jakub Jelinek wrote: > Hi! > > The changed code in build_new_op_1 ICEs on the following testcase, > because conv is user_conv_p with kind == ck_ambig, for which next_conversion > returns NULL. It seems in other spots where for user_conv_p we are walking > the conversion chain we also don't assume there must be ck_user, so this > patch just uses the first conv if ck_user is not found (so that the previous > diagnostics about ambiguous conversion is emitted). It seems like various places could use a function to strip down to the first ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion encountered: source_type, the user-defined conversion comparision in compare_ics, and now here. Mind doing that refactoring? Maybe call it strip_standard_conversion. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2019-11-29 Jakub Jelinek <jakub@redhat.com> > > PR c++/92705 > * call.c (build_new_op_1): For user_conv_p, if there is no > ck_user conversion, use the first one. > > * g++.dg/conversion/ambig4.C: New test. > > --- gcc/cp/call.c.jj 2019-11-29 12:50:09.664608879 +0100 > +++ gcc/cp/call.c 2019-11-29 14:09:54.311718859 +0100 > @@ -6370,8 +6370,12 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[0]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + for (conversion *t = conv; t; t = next_conversion (t)) > + if (t->kind == ck_user) > + { > + conv = t; > + break; > + } > arg1 = convert_like (conv, arg1, complain); > } > > @@ -6380,8 +6384,12 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[1]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + for (conversion *t = conv; t; t = next_conversion (t)) > + if (t->kind == ck_user) > + { > + conv = t; > + break; > + } > arg2 = convert_like (conv, arg2, complain); > } > } > @@ -6391,8 +6399,12 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[2]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + for (conversion *t = conv; t; t = next_conversion (t)) > + if (t->kind == ck_user) > + { > + conv = t; > + break; > + } > arg3 = convert_like (conv, arg3, complain); > } > } > --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj 2019-11-29 14:11:35.239183848 +0100 > +++ gcc/testsuite/g++.dg/conversion/ambig4.C 2019-11-29 14:11:07.006613238 +0100 > @@ -0,0 +1,14 @@ > +// PR c++/92705 > +// { dg-do compile } > + > +struct A {}; > +struct B {}; > +struct C { operator B * (); }; // { dg-message "candidate" } > +struct D { operator B * (); }; // { dg-message "candidate" } > +struct E : C, D { operator A * (); }; > + > +void > +foo (E e, int B::* pmf) > +{ > + int i = e->*pmf; // { dg-error "is ambiguous" } > +} > > Jakub >
On Mon, Dec 02, 2019 at 01:48:46PM -0500, Jason Merrill wrote: > On 11/29/19 6:33 PM, Jakub Jelinek wrote: > > Hi! > > > > The changed code in build_new_op_1 ICEs on the following testcase, > > because conv is user_conv_p with kind == ck_ambig, for which next_conversion > > returns NULL. It seems in other spots where for user_conv_p we are walking > > the conversion chain we also don't assume there must be ck_user, so this > > patch just uses the first conv if ck_user is not found (so that the previous > > diagnostics about ambiguous conversion is emitted). > > It seems like various places could use a function to strip down to the first > ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion encountered: > source_type, the user-defined conversion comparision in compare_ics, and now > here. Mind doing that refactoring? Maybe call it > strip_standard_conversion. In neither of the spots it is exactly equivalent to what the code was doing before (or what the patched build_new_op_1 did), but this is an area I know next to nothing about, so I've just tried to type into patch what you wrote above. Do you mean something like below (didn't see regressions in make check-c++-all, but haven't tried full bootstrap/regtest yet)? 2019-12-02 Jason Merrill <jason@redhat.com> Jakub Jelinek <jakub@redhat.com> PR c++/92705 * call.c (strip_standard_conversion): New function. (build_new_op_1): Use it for user_conv_p. (compare_ics): Likewise. (source_type): Likewise. * g++.dg/conversion/ambig4.C: New test. --- gcc/cp/call.c.jj 2019-11-30 00:15:46.298953538 +0100 +++ gcc/cp/call.c 2019-12-02 23:02:48.494379961 +0100 @@ -865,6 +865,22 @@ next_conversion (conversion *conv) return conv->u.next; } +/* Strip to the first ck_user, ck_ambig, ck_list, ck_aggr or ck_identity + encountered. */ + +static conversion * +strip_standard_conversion (conversion *conv) +{ + while (conv + && conv->kind != ck_user + && conv->kind != ck_ambig + && conv->kind != ck_list + && conv->kind != ck_aggr + && conv->kind != ck_identity) + conv = next_conversion (conv); + return conv; +} + /* Subroutine of build_aggr_conv: check whether CTOR, a braced-init-list, is a valid aggregate initializer for array type ATYPE. */ @@ -6370,8 +6386,7 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[0]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + conv = strip_standard_conversion (conv); arg1 = convert_like (conv, arg1, complain); } @@ -6380,8 +6395,7 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[1]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + conv = strip_standard_conversion (conv); arg2 = convert_like (conv, arg2, complain); } } @@ -6391,8 +6405,7 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[2]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + conv = strip_standard_conversion (conv); arg3 = convert_like (conv, arg3, complain); } } @@ -10538,17 +10551,8 @@ compare_ics (conversion *ics1, conversio if (ics1->user_conv_p || ics1->kind == ck_list || ics1->kind == ck_aggr || ics2->kind == ck_aggr) { - conversion *t1; - conversion *t2; - - for (t1 = ics1; t1 && t1->kind != ck_user; t1 = next_conversion (t1)) - if (t1->kind == ck_ambig || t1->kind == ck_aggr - || t1->kind == ck_list) - break; - for (t2 = ics2; t2 && t2->kind != ck_user; t2 = next_conversion (t2)) - if (t2->kind == ck_ambig || t2->kind == ck_aggr - || t2->kind == ck_list) - break; + conversion *t1 = strip_standard_conversion (ics1); + conversion *t2 = strip_standard_conversion (ics2); if (!t1 || !t2 || t1->kind != t2->kind) return 0; @@ -10956,14 +10960,7 @@ compare_ics (conversion *ics1, conversio static tree source_type (conversion *t) { - for (;; t = next_conversion (t)) - { - if (t->kind == ck_user - || t->kind == ck_ambig - || t->kind == ck_identity) - return t->type; - } - gcc_unreachable (); + return strip_standard_conversion (t)->type; } /* Note a warning about preferring WINNER to LOSER. We do this by storing --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj 2019-12-02 22:52:24.407017475 +0100 +++ gcc/testsuite/g++.dg/conversion/ambig4.C 2019-12-02 22:52:24.407017475 +0100 @@ -0,0 +1,14 @@ +// PR c++/92705 +// { dg-do compile } + +struct A {}; +struct B {}; +struct C { operator B * (); }; // { dg-message "candidate" } +struct D { operator B * (); }; // { dg-message "candidate" } +struct E : C, D { operator A * (); }; + +void +foo (E e, int B::* pmf) +{ + int i = e->*pmf; // { dg-error "is ambiguous" } +} Jakub
On 12/2/19 5:17 PM, Jakub Jelinek wrote: > On Mon, Dec 02, 2019 at 01:48:46PM -0500, Jason Merrill wrote: >> On 11/29/19 6:33 PM, Jakub Jelinek wrote: >>> Hi! >>> >>> The changed code in build_new_op_1 ICEs on the following testcase, >>> because conv is user_conv_p with kind == ck_ambig, for which next_conversion >>> returns NULL. It seems in other spots where for user_conv_p we are walking >>> the conversion chain we also don't assume there must be ck_user, so this >>> patch just uses the first conv if ck_user is not found (so that the previous >>> diagnostics about ambiguous conversion is emitted). >> >> It seems like various places could use a function to strip down to the first >> ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion encountered: >> source_type, the user-defined conversion comparision in compare_ics, and now >> here. Mind doing that refactoring? Maybe call it >> strip_standard_conversion. > > In neither of the spots it is exactly equivalent to what the code was doing > before (or what the patched build_new_op_1 did), but this is an area I know > next to nothing about, so I've just tried to type into patch what you wrote > above. Do you mean something like below (didn't see regressions in make > check-c++-all, but haven't tried full bootstrap/regtest yet)? Yes, thanks. OK if testing passes. Jason > 2019-12-02 Jason Merrill <jason@redhat.com> > Jakub Jelinek <jakub@redhat.com> > > PR c++/92705 > * call.c (strip_standard_conversion): New function. > (build_new_op_1): Use it for user_conv_p. > (compare_ics): Likewise. > (source_type): Likewise. > > * g++.dg/conversion/ambig4.C: New test. > > --- gcc/cp/call.c.jj 2019-11-30 00:15:46.298953538 +0100 > +++ gcc/cp/call.c 2019-12-02 23:02:48.494379961 +0100 > @@ -865,6 +865,22 @@ next_conversion (conversion *conv) > return conv->u.next; > } > > +/* Strip to the first ck_user, ck_ambig, ck_list, ck_aggr or ck_identity > + encountered. */ > + > +static conversion * > +strip_standard_conversion (conversion *conv) > +{ > + while (conv > + && conv->kind != ck_user > + && conv->kind != ck_ambig > + && conv->kind != ck_list > + && conv->kind != ck_aggr > + && conv->kind != ck_identity) > + conv = next_conversion (conv); > + return conv; > +} > + > /* Subroutine of build_aggr_conv: check whether CTOR, a braced-init-list, > is a valid aggregate initializer for array type ATYPE. */ > > @@ -6370,8 +6386,7 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[0]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + conv = strip_standard_conversion (conv); > arg1 = convert_like (conv, arg1, complain); > } > > @@ -6380,8 +6395,7 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[1]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + conv = strip_standard_conversion (conv); > arg2 = convert_like (conv, arg2, complain); > } > } > @@ -6391,8 +6405,7 @@ build_new_op_1 (const op_location_t &loc > conv = cand->convs[2]; > if (conv->user_conv_p) > { > - while (conv->kind != ck_user) > - conv = next_conversion (conv); > + conv = strip_standard_conversion (conv); > arg3 = convert_like (conv, arg3, complain); > } > } > @@ -10538,17 +10551,8 @@ compare_ics (conversion *ics1, conversio > if (ics1->user_conv_p || ics1->kind == ck_list > || ics1->kind == ck_aggr || ics2->kind == ck_aggr) > { > - conversion *t1; > - conversion *t2; > - > - for (t1 = ics1; t1 && t1->kind != ck_user; t1 = next_conversion (t1)) > - if (t1->kind == ck_ambig || t1->kind == ck_aggr > - || t1->kind == ck_list) > - break; > - for (t2 = ics2; t2 && t2->kind != ck_user; t2 = next_conversion (t2)) > - if (t2->kind == ck_ambig || t2->kind == ck_aggr > - || t2->kind == ck_list) > - break; > + conversion *t1 = strip_standard_conversion (ics1); > + conversion *t2 = strip_standard_conversion (ics2); > > if (!t1 || !t2 || t1->kind != t2->kind) > return 0; > @@ -10956,14 +10960,7 @@ compare_ics (conversion *ics1, conversio > static tree > source_type (conversion *t) > { > - for (;; t = next_conversion (t)) > - { > - if (t->kind == ck_user > - || t->kind == ck_ambig > - || t->kind == ck_identity) > - return t->type; > - } > - gcc_unreachable (); > + return strip_standard_conversion (t)->type; > } > > /* Note a warning about preferring WINNER to LOSER. We do this by storing > --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj 2019-12-02 22:52:24.407017475 +0100 > +++ gcc/testsuite/g++.dg/conversion/ambig4.C 2019-12-02 22:52:24.407017475 +0100 > @@ -0,0 +1,14 @@ > +// PR c++/92705 > +// { dg-do compile } > + > +struct A {}; > +struct B {}; > +struct C { operator B * (); }; // { dg-message "candidate" } > +struct D { operator B * (); }; // { dg-message "candidate" } > +struct E : C, D { operator A * (); }; > + > +void > +foo (E e, int B::* pmf) > +{ > + int i = e->*pmf; // { dg-error "is ambiguous" } > +} > > > Jakub >
--- gcc/cp/call.c.jj 2019-11-29 12:50:09.664608879 +0100 +++ gcc/cp/call.c 2019-11-29 14:09:54.311718859 +0100 @@ -6370,8 +6370,12 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[0]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + for (conversion *t = conv; t; t = next_conversion (t)) + if (t->kind == ck_user) + { + conv = t; + break; + } arg1 = convert_like (conv, arg1, complain); } @@ -6380,8 +6384,12 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[1]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + for (conversion *t = conv; t; t = next_conversion (t)) + if (t->kind == ck_user) + { + conv = t; + break; + } arg2 = convert_like (conv, arg2, complain); } } @@ -6391,8 +6399,12 @@ build_new_op_1 (const op_location_t &loc conv = cand->convs[2]; if (conv->user_conv_p) { - while (conv->kind != ck_user) - conv = next_conversion (conv); + for (conversion *t = conv; t; t = next_conversion (t)) + if (t->kind == ck_user) + { + conv = t; + break; + } arg3 = convert_like (conv, arg3, complain); } } --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj 2019-11-29 14:11:35.239183848 +0100 +++ gcc/testsuite/g++.dg/conversion/ambig4.C 2019-11-29 14:11:07.006613238 +0100 @@ -0,0 +1,14 @@ +// PR c++/92705 +// { dg-do compile } + +struct A {}; +struct B {}; +struct C { operator B * (); }; // { dg-message "candidate" } +struct D { operator B * (); }; // { dg-message "candidate" } +struct E : C, D { operator A * (); }; + +void +foo (E e, int B::* pmf) +{ + int i = e->*pmf; // { dg-error "is ambiguous" } +}