| Message ID | 20200213195600.495394-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix ICE with ill-formed array list-initialization [PR93712] | expand |
On 2/13/20 8:56 PM, Marek Polacek wrote: > My P0388R4 patch changed build_array_conv to create an identity > conversion at the start of the conversion chain. Hmm, an identity conversion of {} suggests that it has a type, which it doesn't in the language. I'm not strongly against it, but what was the reason for this change? > That was a sound change but now we crash in convert_like_real > > 7457 case ck_identity: > 7458 if (BRACE_ENCLOSED_INITIALIZER_P (expr)) > 7459 { > 7460 int nelts = CONSTRUCTOR_NELTS (expr); > 7461 if (nelts == 0) > 7462 expr = build_value_init (totype, complain); > 7463 else if (nelts == 1) > 7464 expr = CONSTRUCTOR_ELT (expr, 0)->value; > 7465 else > 7466 gcc_unreachable (); // HERE > 7467 } Right, this is assuming that any other {} will either be ill-formed or handled by ck_aggr or ck_list. How are we getting here without going through one of those? > in a test like this > > int f (int const (&)[2]) > { > return f({1, " "}); > } > > I considered fixing this when performing overload resolution (clang says > "no matching function for call to 'f'"), but then it occured to me that > we crash in different contexts too, so I'm just turning the assert into > an early return. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2020-02-13 Marek Polacek <polacek@redhat.com> > > PR c++/93712 - ICE with ill-formed array list-initialization. > * call.c (convert_like_real): Turn an assert into a return. > > * g++.dg/cpp0x/initlist-array11.C: New test. > --- > gcc/cp/call.c | 2 +- > gcc/testsuite/g++.dg/cpp0x/initlist-array11.C | 10 ++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > index 51621b7dd87..eba0ed8041d 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -7463,7 +7463,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, > else if (nelts == 1) > expr = CONSTRUCTOR_ELT (expr, 0)->value; > else > - gcc_unreachable (); > + return error_mark_node; > } > expr = mark_use (expr, /*rvalue_p=*/!convs->rvaluedness_matches_p, > /*read_p=*/true, UNKNOWN_LOCATION, > diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > new file mode 100644 > index 00000000000..7e76b588471 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > @@ -0,0 +1,10 @@ > +// PR c++/93712 - ICE with ill-formed array list-initialization. > +// { dg-do compile { target c++11 } } > + > +int f (const int (&)[2]); > + > +int g () > +{ > + const int (&r)[2] = {1, "foo"}; // { dg-error "invalid conversion" } > + return f({1, "foo"}); // { dg-error "invalid conversion" } > +} > > base-commit: 1d69147af203d4dcd2270429f90c93f1a37ddfff >
On Fri, Feb 14, 2020 at 09:12:58AM +0100, Jason Merrill wrote: > On 2/13/20 8:56 PM, Marek Polacek wrote: > > My P0388R4 patch changed build_array_conv to create an identity > > conversion at the start of the conversion chain. > > Hmm, an identity conversion of {} suggests that it has a type, which it > doesn't in the language. I'm not strongly against it, but what was the > reason for this change? There are two reasons: 1) without it we couldn't get to the original expression at the start of the conversion chain (saved in .u.expr), this is needed in compare_ics: 10660 tree n1 = nelts_initialized_by_list_init (t1); 10661 tree n2 = nelts_initialized_by_list_init (t2); and nelts_initialized_by_list_init uses conv_get_original_expr for arrays that have no dimensions. 2) struct conversion says /* An implicit conversion sequence, in the sense of [over.best.ics]. The first conversion to be performed is at the end of the chain. That conversion is always a cr_identity conversion. */ and we were breaking that promise. > > That was a sound change but now we crash in convert_like_real > > > > 7457 case ck_identity: > > 7458 if (BRACE_ENCLOSED_INITIALIZER_P (expr)) > > 7459 { > > 7460 int nelts = CONSTRUCTOR_NELTS (expr); > > 7461 if (nelts == 0) > > 7462 expr = build_value_init (totype, complain); > > 7463 else if (nelts == 1) > > 7464 expr = CONSTRUCTOR_ELT (expr, 0)->value; > > 7465 else > > 7466 gcc_unreachable (); // HERE > > 7467 } > > Right, this is assuming that any other {} will either be ill-formed or > handled by ck_aggr or ck_list. How are we getting here without going > through one of those? So here we have a ck_aggr which is bad_p, so in convert_like_real we go to the 7285 if (convs->bad_p block at the beginning, which will look at the next conversion: 7319 for (; t ; t = next_conversion (t)) that conversion is a ck_identity which is not bad_p, so we call 7343 else if (t->kind == ck_user || !t->bad_p) 7344 { 7345 expr = convert_like_real (t, expr, fn, argnum, 7346 /*issue_conversion_warnings=*/false, 7347 /*c_cast_p=*/false, 7348 complain); and crash there. Marek
On 2/19/20 7:30 PM, Marek Polacek wrote: > On Fri, Feb 14, 2020 at 09:12:58AM +0100, Jason Merrill wrote: >> On 2/13/20 8:56 PM, Marek Polacek wrote: >>> My P0388R4 patch changed build_array_conv to create an identity >>> conversion at the start of the conversion chain. >> >> Hmm, an identity conversion of {} suggests that it has a type, which it >> doesn't in the language. I'm not strongly against it, but what was the >> reason for this change? > > There are two reasons: > 1) without it we couldn't get to the original expression at the start > of the conversion chain (saved in .u.expr), this is needed in compare_ics: > 10660 tree n1 = nelts_initialized_by_list_init (t1); > 10661 tree n2 = nelts_initialized_by_list_init (t2); > and nelts_initialized_by_list_init uses conv_get_original_expr for > arrays that have no dimensions. Ah, ck_aggr and ck_list probably should have used u.expr like ck_identity and ck_ambig.... > 2) struct conversion says > /* An implicit conversion sequence, in the sense of [over.best.ics]. > The first conversion to be performed is at the end of the chain. > That conversion is always a cr_identity conversion. */ > and we were breaking that promise. ...or, if we're going to enforce this, ck_ambig will need to change as well. And build_aggr_conv and build_complex_conv will need adjusting (one way or another). Jason
On Thu, Feb 20, 2020 at 12:28:42AM +0000, Jason Merrill wrote: > On 2/19/20 7:30 PM, Marek Polacek wrote: > > On Fri, Feb 14, 2020 at 09:12:58AM +0100, Jason Merrill wrote: > > > On 2/13/20 8:56 PM, Marek Polacek wrote: > > > > My P0388R4 patch changed build_array_conv to create an identity > > > > conversion at the start of the conversion chain. > > > > > > Hmm, an identity conversion of {} suggests that it has a type, which it > > > doesn't in the language. I'm not strongly against it, but what was the > > > reason for this change? > > > > There are two reasons: > > 1) without it we couldn't get to the original expression at the start > > of the conversion chain (saved in .u.expr), this is needed in compare_ics: > > 10660 tree n1 = nelts_initialized_by_list_init (t1); > > 10661 tree n2 = nelts_initialized_by_list_init (t2); > > and nelts_initialized_by_list_init uses conv_get_original_expr for > > arrays that have no dimensions. > > Ah, ck_aggr and ck_list probably should have used u.expr like ck_identity > and ck_ambig.... > > > 2) struct conversion says > > /* An implicit conversion sequence, in the sense of [over.best.ics]. > > The first conversion to be performed is at the end of the chain. > > That conversion is always a cr_identity conversion. */ > > and we were breaking that promise. > > ...or, if we're going to enforce this, ck_ambig will need to change as well. > > And build_aggr_conv and build_complex_conv will need adjusting (one way or > another). Changed my mind :-). Turned out using u.expr for ck_aggr is a much smaller change. Moreover, that struct conversion talks about cr_identity, a rank, not ck_identity. ck_list uses u.list, so I'm not changing that. -- >8 -- My P0388R4 patch changed build_array_conv to create an identity conversion at the start of the conversion chain and now we crash in convert_like_real: 7457 case ck_identity: 7458 if (BRACE_ENCLOSED_INITIALIZER_P (expr)) 7459 { 7460 int nelts = CONSTRUCTOR_NELTS (expr); 7461 if (nelts == 0) 7462 expr = build_value_init (totype, complain); 7463 else if (nelts == 1) 7464 expr = CONSTRUCTOR_ELT (expr, 0)->value; 7465 else 7466 gcc_unreachable (); // HERE 7467 } in a test like this int f (int const (&)[2]) { return f({1, "M"}); } Instead of creating a ck_identity at the start of the conversion chain, so that conv_get_original_expr can be used with a ck_aggr, let's set u.expr for a ck_aggr, and adjust next_conversion not to try to see what's next in the chain if it gets a ck_aggr. Bootstrapped/regtested on x86_64-linux, built cmcstl2 and Boost, ok for trunk? 2020-02-21 Marek Polacek <polacek@redhat.com> PR c++/93712 - ICE with ill-formed array list-initialization. * call.c (next_conversion): Return NULL for ck_aggr. (build_aggr_conv): Set u.expr instead of u.next. (build_array_conv): Likewise. (build_complex_conv): Likewise. (conv_get_original_expr): Handle ck_aggr. * g++.dg/cpp0x/initlist-array11.C: New test. --- gcc/cp/call.c | 17 +++++++++-------- gcc/testsuite/g++.dg/cpp0x/initlist-array11.C | 10 ++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-array11.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index f47f96bf1c2..84230b9ecb8 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -117,13 +117,13 @@ struct conversion { /* The next conversion in the chain. Since the conversions are arranged from outermost to innermost, the NEXT conversion will actually be performed before this conversion. This variant is - used only when KIND is neither ck_identity, ck_ambig nor + used only when KIND is neither ck_identity, ck_aggr, ck_ambig nor ck_list. Please use the next_conversion function instead of using this field directly. */ conversion *next; /* The expression at the beginning of the conversion chain. This - variant is used only if KIND is ck_identity or ck_ambig. You can - use conv_get_original_expr to get this expression. */ + variant is used only if KIND is ck_identity, ck_aggr, or ck_ambig. + You can use conv_get_original_expr to get this expression. */ tree expr; /* The array of conversions for an initializer_list, so this variant is used only when KIN D is ck_list. */ @@ -861,7 +861,8 @@ next_conversion (conversion *conv) if (conv == NULL || conv->kind == ck_identity || conv->kind == ck_ambig - || conv->kind == ck_list) + || conv->kind == ck_list + || conv->kind == ck_aggr) return NULL; return conv->u.next; } @@ -1030,7 +1031,7 @@ build_aggr_conv (tree type, tree ctor, int flags, tsubst_flags_t complain) c->rank = cr_exact; c->user_conv_p = true; c->check_narrowing = true; - c->u.next = NULL; + c->u.expr = ctor; return c; } @@ -1083,7 +1084,7 @@ build_array_conv (tree type, tree ctor, int flags, tsubst_flags_t complain) c->rank = rank; c->user_conv_p = user; c->bad_p = bad; - c->u.next = build_identity_conv (TREE_TYPE (ctor), ctor); + c->u.expr = ctor; return c; } @@ -1129,7 +1130,7 @@ build_complex_conv (tree type, tree ctor, int flags, c->rank = rank; c->user_conv_p = user; c->bad_p = bad; - c->u.next = NULL; + c->u.expr = ctor; return c; } @@ -10498,7 +10499,7 @@ static tree conv_get_original_expr (conversion *c) { for (; c; c = next_conversion (c)) - if (c->kind == ck_identity || c->kind == ck_ambig) + if (c->kind == ck_identity || c->kind == ck_ambig || c->kind == ck_aggr) return c->u.expr; return NULL_TREE; } diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C new file mode 100644 index 00000000000..d3e58dea129 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C @@ -0,0 +1,10 @@ +// PR c++/93712 - ICE with ill-formed array list-initialization. +// { dg-do compile { target c++11 } } + +int f (const int (&)[2]); + +int g () +{ + const int (&r)[2] = {1, "foo"}; // { dg-error "conversion" } + return f({1, "foo"}); // { dg-error "conversion" } +} base-commit: 8d1780b56d0cb1d50115d4e925e81cd8b9cb2923
On 2/21/20 6:48 PM, Marek Polacek wrote: > On Thu, Feb 20, 2020 at 12:28:42AM +0000, Jason Merrill wrote: >> On 2/19/20 7:30 PM, Marek Polacek wrote: >>> On Fri, Feb 14, 2020 at 09:12:58AM +0100, Jason Merrill wrote: >>>> On 2/13/20 8:56 PM, Marek Polacek wrote: >>>>> My P0388R4 patch changed build_array_conv to create an identity >>>>> conversion at the start of the conversion chain. >>>> >>>> Hmm, an identity conversion of {} suggests that it has a type, which it >>>> doesn't in the language. I'm not strongly against it, but what was the >>>> reason for this change? >>> >>> There are two reasons: >>> 1) without it we couldn't get to the original expression at the start >>> of the conversion chain (saved in .u.expr), this is needed in compare_ics: >>> 10660 tree n1 = nelts_initialized_by_list_init (t1); >>> 10661 tree n2 = nelts_initialized_by_list_init (t2); >>> and nelts_initialized_by_list_init uses conv_get_original_expr for >>> arrays that have no dimensions. >> >> Ah, ck_aggr and ck_list probably should have used u.expr like ck_identity >> and ck_ambig.... >> >>> 2) struct conversion says >>> /* An implicit conversion sequence, in the sense of [over.best.ics]. >>> The first conversion to be performed is at the end of the chain. >>> That conversion is always a cr_identity conversion. */ >>> and we were breaking that promise. >> >> ...or, if we're going to enforce this, ck_ambig will need to change as well. >> >> And build_aggr_conv and build_complex_conv will need adjusting (one way or >> another). > > Changed my mind :-). Turned out using u.expr for ck_aggr is a much smaller > change. Moreover, that struct conversion talks about cr_identity, a rank, > not ck_identity. > > ck_list uses u.list, so I'm not changing that. > > -- >8 -- > My P0388R4 patch changed build_array_conv to create an identity > conversion at the start of the conversion chain and now we crash > in convert_like_real: > > 7457 case ck_identity: > 7458 if (BRACE_ENCLOSED_INITIALIZER_P (expr)) > 7459 { > 7460 int nelts = CONSTRUCTOR_NELTS (expr); > 7461 if (nelts == 0) > 7462 expr = build_value_init (totype, complain); > 7463 else if (nelts == 1) > 7464 expr = CONSTRUCTOR_ELT (expr, 0)->value; > 7465 else > 7466 gcc_unreachable (); // HERE > 7467 } > > in a test like this > > int f (int const (&)[2]) > { return f({1, "M"}); } > > Instead of creating a ck_identity at the start of the conversion chain, > so that conv_get_original_expr can be used with a ck_aggr, let's set > u.expr for a ck_aggr, and adjust next_conversion not to try to see > what's next in the chain if it gets a ck_aggr. > > Bootstrapped/regtested on x86_64-linux, built cmcstl2 and Boost, ok for > trunk? OK. > 2020-02-21 Marek Polacek <polacek@redhat.com> > > PR c++/93712 - ICE with ill-formed array list-initialization. > * call.c (next_conversion): Return NULL for ck_aggr. > (build_aggr_conv): Set u.expr instead of u.next. > (build_array_conv): Likewise. > (build_complex_conv): Likewise. > (conv_get_original_expr): Handle ck_aggr. > > * g++.dg/cpp0x/initlist-array11.C: New test. > --- > gcc/cp/call.c | 17 +++++++++-------- > gcc/testsuite/g++.dg/cpp0x/initlist-array11.C | 10 ++++++++++ > 2 files changed, 19 insertions(+), 8 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > index f47f96bf1c2..84230b9ecb8 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -117,13 +117,13 @@ struct conversion { > /* The next conversion in the chain. Since the conversions are > arranged from outermost to innermost, the NEXT conversion will > actually be performed before this conversion. This variant is > - used only when KIND is neither ck_identity, ck_ambig nor > + used only when KIND is neither ck_identity, ck_aggr, ck_ambig nor > ck_list. Please use the next_conversion function instead > of using this field directly. */ > conversion *next; > /* The expression at the beginning of the conversion chain. This > - variant is used only if KIND is ck_identity or ck_ambig. You can > - use conv_get_original_expr to get this expression. */ > + variant is used only if KIND is ck_identity, ck_aggr, or ck_ambig. > + You can use conv_get_original_expr to get this expression. */ > tree expr; > /* The array of conversions for an initializer_list, so this > variant is used only when KIN D is ck_list. */ > @@ -861,7 +861,8 @@ next_conversion (conversion *conv) > if (conv == NULL > || conv->kind == ck_identity > || conv->kind == ck_ambig > - || conv->kind == ck_list) > + || conv->kind == ck_list > + || conv->kind == ck_aggr) > return NULL; > return conv->u.next; > } > @@ -1030,7 +1031,7 @@ build_aggr_conv (tree type, tree ctor, int flags, tsubst_flags_t complain) > c->rank = cr_exact; > c->user_conv_p = true; > c->check_narrowing = true; > - c->u.next = NULL; > + c->u.expr = ctor; > return c; > } > > @@ -1083,7 +1084,7 @@ build_array_conv (tree type, tree ctor, int flags, tsubst_flags_t complain) > c->rank = rank; > c->user_conv_p = user; > c->bad_p = bad; > - c->u.next = build_identity_conv (TREE_TYPE (ctor), ctor); > + c->u.expr = ctor; > return c; > } > > @@ -1129,7 +1130,7 @@ build_complex_conv (tree type, tree ctor, int flags, > c->rank = rank; > c->user_conv_p = user; > c->bad_p = bad; > - c->u.next = NULL; > + c->u.expr = ctor; > return c; > } > > @@ -10498,7 +10499,7 @@ static tree > conv_get_original_expr (conversion *c) > { > for (; c; c = next_conversion (c)) > - if (c->kind == ck_identity || c->kind == ck_ambig) > + if (c->kind == ck_identity || c->kind == ck_ambig || c->kind == ck_aggr) > return c->u.expr; > return NULL_TREE; > } > diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > new file mode 100644 > index 00000000000..d3e58dea129 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C > @@ -0,0 +1,10 @@ > +// PR c++/93712 - ICE with ill-formed array list-initialization. > +// { dg-do compile { target c++11 } } > + > +int f (const int (&)[2]); > + > +int g () > +{ > + const int (&r)[2] = {1, "foo"}; // { dg-error "conversion" } > + return f({1, "foo"}); // { dg-error "conversion" } > +} > > base-commit: 8d1780b56d0cb1d50115d4e925e81cd8b9cb2923 >
diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 51621b7dd87..eba0ed8041d 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -7463,7 +7463,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, else if (nelts == 1) expr = CONSTRUCTOR_ELT (expr, 0)->value; else - gcc_unreachable (); + return error_mark_node; } expr = mark_use (expr, /*rvalue_p=*/!convs->rvaluedness_matches_p, /*read_p=*/true, UNKNOWN_LOCATION, diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C new file mode 100644 index 00000000000..7e76b588471 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-array11.C @@ -0,0 +1,10 @@ +// PR c++/93712 - ICE with ill-formed array list-initialization. +// { dg-do compile { target c++11 } } + +int f (const int (&)[2]); + +int g () +{ + const int (&r)[2] = {1, "foo"}; // { dg-error "invalid conversion" } + return f({1, "foo"}); // { dg-error "invalid conversion" } +}
My P0388R4 patch changed build_array_conv to create an identity conversion at the start of the conversion chain. That was a sound change but now we crash in convert_like_real 7457 case ck_identity: 7458 if (BRACE_ENCLOSED_INITIALIZER_P (expr)) 7459 { 7460 int nelts = CONSTRUCTOR_NELTS (expr); 7461 if (nelts == 0) 7462 expr = build_value_init (totype, complain); 7463 else if (nelts == 1) 7464 expr = CONSTRUCTOR_ELT (expr, 0)->value; 7465 else 7466 gcc_unreachable (); // HERE 7467 } in a test like this int f (int const (&)[2]) { return f({1, " "}); } I considered fixing this when performing overload resolution (clang says "no matching function for call to 'f'"), but then it occured to me that we crash in different contexts too, so I'm just turning the assert into an early return. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2020-02-13 Marek Polacek <polacek@redhat.com> PR c++/93712 - ICE with ill-formed array list-initialization. * call.c (convert_like_real): Turn an assert into a return. * g++.dg/cpp0x/initlist-array11.C: New test. --- gcc/cp/call.c | 2 +- gcc/testsuite/g++.dg/cpp0x/initlist-array11.C | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-array11.C base-commit: 1d69147af203d4dcd2270429f90c93f1a37ddfff