Message ID | 8a93abd6-69ae-4ff5-5332-eb8791d492f1@oracle.com |
---|---|
State | New |
Headers | show |
Series | [C++] PR 84632 ("[8 Regression] internal compiler error: tree check: expected record_type or union_type or qual_union_type, have array_type in reduced_constant_expression_p...") | expand |
On Thu, Mar 22, 2018 at 4:05 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > yesterday I figured out that this issue is in fact orthogonal to possible > improvements to maybe_deduce_size_from_array_init, indeed it happens also > when we are not deducing the size. The testcase is very similar to that > recently submitted for c++/78345, which led Jason to add the bit of > additional checking around the middle of build_aggr_init. Since we did > already have a similar check a bit earlier - not requiring the computation > of from_array - I added one for VAR_DECLs too in the same place and > tweaked/improved a bit the error message in the process (a few greps > revealed that we don't seem to have any other generic error message using > the form "bad ..."), consistently with the diagnostic that we already > provide for, eg, a simple > > int a[2] = a; > > FYI, I must also add that entire testsuite doesn't have a test triggering > the existing TREE_CODE (init) == TREE_LIST check, and failed so far to > construct one... (many such issues are normally catched by digest_init). > Tested x86_64-linux. That whole block is there to support this sort of initialization, as an ancient extension. Since it isn't generally allowed, and is causing trouble, let's remove it and reject anything that isn't an initializer list. Jason
Hi, On 22/03/2018 19:08, Jason Merrill wrote: > On Thu, Mar 22, 2018 at 4:05 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> Hi, >> >> yesterday I figured out that this issue is in fact orthogonal to possible >> improvements to maybe_deduce_size_from_array_init, indeed it happens also >> when we are not deducing the size. The testcase is very similar to that >> recently submitted for c++/78345, which led Jason to add the bit of >> additional checking around the middle of build_aggr_init. Since we did >> already have a similar check a bit earlier - not requiring the computation >> of from_array - I added one for VAR_DECLs too in the same place and >> tweaked/improved a bit the error message in the process (a few greps >> revealed that we don't seem to have any other generic error message using >> the form "bad ..."), consistently with the diagnostic that we already >> provide for, eg, a simple >> >> int a[2] = a; >> >> FYI, I must also add that entire testsuite doesn't have a test triggering >> the existing TREE_CODE (init) == TREE_LIST check, and failed so far to >> construct one... (many such issues are normally catched by digest_init). >> Tested x86_64-linux. > That whole block is there to support this sort of initialization, as > an ancient extension. Since it isn't generally allowed, and is > causing trouble, let's remove it and reject anything that isn't an > initializer list. Nice. While I start implementing the above, any hint about thing like g++.dg/cpp0x/initlist68.C, which we would reject because involves a plain constructor of type ARRAY_TYPE, not a proper BRACE_ENCLOSED_INITIALIZER_P? Also, less important I guess, we would also reject g++.dg/torture/pr70499.C to which you added -fpermissive in the occasion of c++/78345. Or maybe you meant something a bit less drastic ;) Paolo.
Hi again, On 22/03/2018 19:58, Paolo Carlini wrote: > Nice. While I start implementing the above, any hint about thing like > g++.dg/cpp0x/initlist68.C, which we would reject because involves a > plain constructor of type ARRAY_TYPE, not a proper > BRACE_ENCLOSED_INITIALIZER_P? Also, less important I guess, we would > also reject g++.dg/torture/pr70499.C to which you added -fpermissive > in the occasion of c++/78345. Or maybe you meant something a bit less > drastic ;) I suspect that allowing for either BRACE_ENCLOSED_INITIALIZER_P or CONSTRUCTOR with a check of the type (like you changed build_vec_init at the time: https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/cp/init.c?r1=246244&r2=246243&pathrev=246244 ) would work fine. Paolo.
On Thu, Mar 22, 2018 at 3:35 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi again, > > On 22/03/2018 19:58, Paolo Carlini wrote: > > Nice. While I start implementing the above, any hint about thing like > g++.dg/cpp0x/initlist68.C, which we would reject because involves a plain > constructor of type ARRAY_TYPE, not a proper BRACE_ENCLOSED_INITIALIZER_P? > Also, less important I guess, we would also reject g++.dg/torture/pr70499.C > to which you added -fpermissive in the occasion of c++/78345. Or maybe you > meant something a bit less drastic ;) > > I suspect that allowing for either BRACE_ENCLOSED_INITIALIZER_P or > CONSTRUCTOR with a check of the type (like you changed build_vec_init at the > time: > https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/cp/init.c?r1=246244&r2=246243&pathrev=246244 > ) would work fine. Yes, that sounds good. And if pr70499.C fails, let's fix the testcase to add the missing braces. Jason
Hi, On 22/03/2018 22:19, Jason Merrill wrote: > On Thu, Mar 22, 2018 at 3:35 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> Hi again, >> >> On 22/03/2018 19:58, Paolo Carlini wrote: >> >> Nice. While I start implementing the above, any hint about thing like >> g++.dg/cpp0x/initlist68.C, which we would reject because involves a plain >> constructor of type ARRAY_TYPE, not a proper BRACE_ENCLOSED_INITIALIZER_P? >> Also, less important I guess, we would also reject g++.dg/torture/pr70499.C >> to which you added -fpermissive in the occasion of c++/78345. Or maybe you >> meant something a bit less drastic ;) >> >> I suspect that allowing for either BRACE_ENCLOSED_INITIALIZER_P or >> CONSTRUCTOR with a check of the type (like you changed build_vec_init at the >> time: >> https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/cp/init.c?r1=246244&r2=246243&pathrev=246244 >> ) would work fine. > Yes, that sounds good. And if pr70499.C fails, let's fix the testcase > to add the missing braces. Ok about pr70499.C. Unfortunately, however, the above idea still is not enough: for, say, lambda-array.C we have to accept as init an INDIRECT_REF with ARRAY_TYPE, thus qualified CONSTRUCTOR isn't enough, it really looks like we have to accept various (?) TREE_CODEs with ARRAY_TYPE as type, not just CONSTRUCTORs, to handle normal well formed code. Thus my best try so far would be the below, in testing. What do you think? Cheers, Paolo. /////////////////////
... with patch ;) If you are curious where the heck that INDIRECT_REF is coming from, is coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. Grrr. Paolo. /////////////////////// Index: cp/init.c =================================================================== --- cp/init.c (revision 258758) +++ cp/init.c (working copy) @@ -1688,14 +1688,6 @@ build_aggr_init (tree exp, tree init, int flags, t } else { - /* An array may not be initialized use the parenthesized - initialization form -- unless the initializer is "()". */ - if (init && TREE_CODE (init) == TREE_LIST) - { - if (complain & tf_error) - error ("bad array initializer"); - return error_mark_node; - } /* Must arrange to initialize each element of EXP from elements of INIT. */ if (cv_qualified_p (type)) @@ -1705,14 +1697,15 @@ build_aggr_init (tree exp, tree init, int flags, t from_array = (itype && same_type_p (TREE_TYPE (init), TREE_TYPE (exp))); - if (init && !from_array - && !BRACE_ENCLOSED_INITIALIZER_P (init)) + if (init && ((!from_array + && !BRACE_ENCLOSED_INITIALIZER_P (init)) + /* See c++/84632. */ + || VAR_P (init))) { if (complain & tf_error) - permerror (init_loc, "array must be initialized " - "with a brace-enclosed initializer"); - else - return error_mark_node; + error_at (init_loc, "array must be initialized " + "with a brace-enclosed initializer"); + return error_mark_node; } } Index: testsuite/g++.dg/init/array49.C =================================================================== --- testsuite/g++.dg/init/array49.C (nonexistent) +++ testsuite/g++.dg/init/array49.C (working copy) @@ -0,0 +1,6 @@ +// PR c++/84632 +// { dg-additional-options "-w" } + +class { + &a; // { dg-error "forbids declaration" } +} b[2] = b; // { dg-error "initialized" } Index: testsuite/g++.dg/torture/pr70499.C =================================================================== --- testsuite/g++.dg/torture/pr70499.C (revision 258758) +++ testsuite/g++.dg/torture/pr70499.C (working copy) @@ -1,5 +1,5 @@ // { dg-do compile } -// { dg-additional-options "-w -fpermissive -Wno-psabi" } +// { dg-additional-options "-w -Wno-psabi" } // { dg-additional-options "-mavx" { target x86_64-*-* i?86-*-* } } typedef double __m256d __attribute__ ((__vector_size__ (32), __may_alias__)); @@ -30,7 +30,7 @@ struct Foo { template<typename Tx> __attribute__((__always_inline__)) inline void inlineFunc(Tx hx[]) { Tx x = hx[0], y = hx[1]; - Tx lam[1] = (x*y); + Tx lam[1] = {(x*y)}; } void FooBarFunc () {
On Thu, Mar 22, 2018 at 5:39 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > ... with patch ;) > > If you are curious where the heck that INDIRECT_REF is coming from, is > coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. Grrr. Hmm, maybe build_vec_init should call itself directly rather than via build_aggr_init in the case of multidimensional arrays. Jason
Hi, On 22/03/2018 23:26, Jason Merrill wrote: > On Thu, Mar 22, 2018 at 5:39 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> ... with patch ;) >> >> If you are curious where the heck that INDIRECT_REF is coming from, is >> coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. Grrr. > Hmm, maybe build_vec_init should call itself directly rather than via > build_aggr_init in the case of multidimensional arrays. Yes, arranging things like that seems doable. However, yesterday, while fiddling with the idea and instrumenting the code with some gcc_asserts, I noticed that we have yet another tree code to handle, TARGET_EXPR, as in lines #41, #47, #56 of ext/complit12.C, and in that case build_aggr_init is simply called by check_initializer via build_aggr_init_full_exprs, the "normal" path. Well, unless we want to adjust/reject complit12.C too, which clang rejects, in fact with errors on lines #19 and #29 too. The below passes testing. Thanks, Paolo. ///////////////// Index: cp/init.c =================================================================== --- cp/init.c (revision 258758) +++ cp/init.c (working copy) @@ -1688,14 +1688,6 @@ build_aggr_init (tree exp, tree init, int flags, t } else { - /* An array may not be initialized use the parenthesized - initialization form -- unless the initializer is "()". */ - if (init && TREE_CODE (init) == TREE_LIST) - { - if (complain & tf_error) - error ("bad array initializer"); - return error_mark_node; - } /* Must arrange to initialize each element of EXP from elements of INIT. */ if (cv_qualified_p (type)) @@ -1705,14 +1697,16 @@ build_aggr_init (tree exp, tree init, int flags, t from_array = (itype && same_type_p (TREE_TYPE (init), TREE_TYPE (exp))); - if (init && !from_array - && !BRACE_ENCLOSED_INITIALIZER_P (init)) + if (init && !BRACE_ENCLOSED_INITIALIZER_P (init) + && (!from_array + || (TREE_CODE (init) != CONSTRUCTOR + && TREE_CODE (init) != INDIRECT_REF + && TREE_CODE (init) != TARGET_EXPR))) { if (complain & tf_error) - permerror (init_loc, "array must be initialized " - "with a brace-enclosed initializer"); - else - return error_mark_node; + error_at (init_loc, "array must be initialized " + "with a brace-enclosed initializer"); + return error_mark_node; } } Index: testsuite/g++.dg/init/array49.C =================================================================== --- testsuite/g++.dg/init/array49.C (nonexistent) +++ testsuite/g++.dg/init/array49.C (working copy) @@ -0,0 +1,6 @@ +// PR c++/84632 +// { dg-additional-options "-w" } + +class { + &a; // { dg-error "forbids declaration" } +} b[2] = b; // { dg-error "initialized" } Index: testsuite/g++.dg/torture/pr70499.C =================================================================== --- testsuite/g++.dg/torture/pr70499.C (revision 258758) +++ testsuite/g++.dg/torture/pr70499.C (working copy) @@ -1,5 +1,5 @@ // { dg-do compile } -// { dg-additional-options "-w -fpermissive -Wno-psabi" } +// { dg-additional-options "-w -Wno-psabi" } // { dg-additional-options "-mavx" { target x86_64-*-* i?86-*-* } } typedef double __m256d __attribute__ ((__vector_size__ (32), __may_alias__)); @@ -30,7 +30,7 @@ struct Foo { template<typename Tx> __attribute__((__always_inline__)) inline void inlineFunc(Tx hx[]) { Tx x = hx[0], y = hx[1]; - Tx lam[1] = (x*y); + Tx lam[1] = {(x*y)}; } void FooBarFunc () {
On Fri, Mar 23, 2018 at 6:13 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > On 22/03/2018 23:26, Jason Merrill wrote: >> >> On Thu, Mar 22, 2018 at 5:39 PM, Paolo Carlini <paolo.carlini@oracle.com> >> wrote: >>> >>> ... with patch ;) >>> >>> If you are curious where the heck that INDIRECT_REF is coming from, is >>> coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. Grrr. >> >> Hmm, maybe build_vec_init should call itself directly rather than via >> build_aggr_init in the case of multidimensional arrays. > > Yes, arranging things like that seems doable. However, yesterday, while > fiddling with the idea and instrumenting the code with some gcc_asserts, I > noticed that we have yet another tree code to handle, TARGET_EXPR, as in > lines #41, #47, #56 of ext/complit12.C, and in that case build_aggr_init is > simply called by check_initializer via build_aggr_init_full_exprs, the > "normal" path. Well, unless we want to adjust/reject complit12.C too, which > clang rejects, in fact with errors on lines #19 and #29 too. The below > passes testing. I think I'd like to allow TARGET_EXPR here, with a comment about compound literals, but avoid INDIRECT_REF with that build_vec_init change. Jason
Hi, On 23/03/2018 13:39, Jason Merrill wrote: > On Fri, Mar 23, 2018 at 6:13 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> On 22/03/2018 23:26, Jason Merrill wrote: >>> On Thu, Mar 22, 2018 at 5:39 PM, Paolo Carlini <paolo.carlini@oracle.com> >>> wrote: >>>> ... with patch ;) >>>> >>>> If you are curious where the heck that INDIRECT_REF is coming from, is >>>> coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. Grrr. >>> Hmm, maybe build_vec_init should call itself directly rather than via >>> build_aggr_init in the case of multidimensional arrays. >> Yes, arranging things like that seems doable. However, yesterday, while >> fiddling with the idea and instrumenting the code with some gcc_asserts, I >> noticed that we have yet another tree code to handle, TARGET_EXPR, as in >> lines #41, #47, #56 of ext/complit12.C, and in that case build_aggr_init is >> simply called by check_initializer via build_aggr_init_full_exprs, the >> "normal" path. Well, unless we want to adjust/reject complit12.C too, which >> clang rejects, in fact with errors on lines #19 and #29 too. The below >> passes testing. > I think I'd like to allow TARGET_EXPR here, with a comment about > compound literals, but avoid INDIRECT_REF with that build_vec_init > change. I see. Having run the full testsuite a number of times with additional gcc_asserts, I'm very confident that something as simple as the below is fine, at least as far as the testsuite + variants of lambda-array.C is concerned. In it I'm also proposing a gcc_assert verifying that the very idea of not using any diagnostic conditional makes sense for the internally generated INDIRECT_REFs: in the existing build_aggr_init if the types were different from_array would be zero and, for INDIRECT_REF as init, the condition true. Thanks, Paolo. /////////////////////// Index: cp/init.c =================================================================== --- cp/init.c (revision 258846) +++ cp/init.c (working copy) @@ -1688,14 +1688,6 @@ build_aggr_init (tree exp, tree init, int flags, t } else { - /* An array may not be initialized use the parenthesized - initialization form -- unless the initializer is "()". */ - if (init && TREE_CODE (init) == TREE_LIST) - { - if (complain & tf_error) - error ("bad array initializer"); - return error_mark_node; - } /* Must arrange to initialize each element of EXP from elements of INIT. */ if (cv_qualified_p (type)) @@ -1705,14 +1697,17 @@ build_aggr_init (tree exp, tree init, int flags, t from_array = (itype && same_type_p (TREE_TYPE (init), TREE_TYPE (exp))); - if (init && !from_array - && !BRACE_ENCLOSED_INITIALIZER_P (init)) + if (init && !BRACE_ENCLOSED_INITIALIZER_P (init) + && (!from_array + || (TREE_CODE (init) != CONSTRUCTOR + /* Can happen, eg, handling the compound-literals + extension (ext/complit12.C). */ + && TREE_CODE (init) != TARGET_EXPR))) { if (complain & tf_error) - permerror (init_loc, "array must be initialized " - "with a brace-enclosed initializer"); - else - return error_mark_node; + error_at (init_loc, "array must be initialized " + "with a brace-enclosed initializer"); + return error_mark_node; } } @@ -4371,7 +4366,19 @@ build_vec_init (tree base, tree maxindex, tree ini elt_init = cp_build_modify_expr (input_location, to, NOP_EXPR, from, complain); else if (type_build_ctor_call (type)) - elt_init = build_aggr_init (to, from, 0, complain); + { + if (TREE_CODE (type) == ARRAY_TYPE + && from && TREE_CODE (from) == INDIRECT_REF) + { + gcc_assert (same_type_ignoring_top_level_qualifiers_p + (type, TREE_TYPE (from))); + elt_init = build_vec_init (to, NULL_TREE, from, + /*explicit_value_init_p=*/false, + /*from_array=*/1, complain); + } + else + elt_init = build_aggr_init (to, from, 0, complain); + } else if (from) elt_init = cp_build_modify_expr (input_location, to, NOP_EXPR, from, complain); Index: testsuite/g++.dg/init/array49.C =================================================================== --- testsuite/g++.dg/init/array49.C (nonexistent) +++ testsuite/g++.dg/init/array49.C (working copy) @@ -0,0 +1,6 @@ +// PR c++/84632 +// { dg-additional-options "-w" } + +class { + &a; // { dg-error "forbids declaration" } +} b[2] = b; // { dg-error "initialized" } Index: testsuite/g++.dg/torture/pr70499.C =================================================================== --- testsuite/g++.dg/torture/pr70499.C (revision 258846) +++ testsuite/g++.dg/torture/pr70499.C (working copy) @@ -1,5 +1,5 @@ // { dg-do compile } -// { dg-additional-options "-w -fpermissive -Wno-psabi" } +// { dg-additional-options "-w -Wno-psabi" } // { dg-additional-options "-mavx" { target x86_64-*-* i?86-*-* } } typedef double __m256d __attribute__ ((__vector_size__ (32), __may_alias__)); @@ -30,7 +30,7 @@ struct Foo { template<typename Tx> __attribute__((__always_inline__)) inline void inlineFunc(Tx hx[]) { Tx x = hx[0], y = hx[1]; - Tx lam[1] = (x*y); + Tx lam[1] = {(x*y)}; } void FooBarFunc () {
On Mon, Mar 26, 2018 at 5:19 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > On 23/03/2018 13:39, Jason Merrill wrote: >> On Fri, Mar 23, 2018 at 6:13 AM, Paolo Carlini <paolo.carlini@oracle.com> >> wrote: >>> >>> On 22/03/2018 23:26, Jason Merrill wrote: >>>> >>>> On Thu, Mar 22, 2018 at 5:39 PM, Paolo Carlini >>>> <paolo.carlini@oracle.com> >>>> wrote: >>>>> >>>>> ... with patch ;) >>>>> >>>>> If you are curious where the heck that INDIRECT_REF is coming from, is >>>>> coming from the gimplifier, cp_gimpliify_expr, via build_vec_init. >>>>> Grrr. >>>> >>>> Hmm, maybe build_vec_init should call itself directly rather than via >>>> build_aggr_init in the case of multidimensional arrays. >>> >>> Yes, arranging things like that seems doable. However, yesterday, while >>> fiddling with the idea and instrumenting the code with some gcc_asserts, >>> I >>> noticed that we have yet another tree code to handle, TARGET_EXPR, as in >>> lines #41, #47, #56 of ext/complit12.C, and in that case build_aggr_init >>> is >>> simply called by check_initializer via build_aggr_init_full_exprs, the >>> "normal" path. Well, unless we want to adjust/reject complit12.C too, >>> which >>> clang rejects, in fact with errors on lines #19 and #29 too. The below >>> passes testing. >> >> I think I'd like to allow TARGET_EXPR here, with a comment about >> compound literals, but avoid INDIRECT_REF with that build_vec_init >> change. > > I see. Having run the full testsuite a number of times with additional > gcc_asserts, I'm very confident that something as simple as the below is > fine, at least as far as the testsuite + variants of lambda-array.C is > concerned. In it I'm also proposing a gcc_assert verifying that the very > idea of not using any diagnostic conditional makes sense for the internally > generated INDIRECT_REFs: in the existing build_aggr_init if the types were > different from_array would be zero and, for INDIRECT_REF as init, the > condition true. Your build_aggr_init change is OK, but I had in mind something more general in build_vec_init: diff --git a/gcc/cp/init.c b/gcc/cp/init.c index ff52c42c1ad..d689390d117 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -4367,7 +4367,10 @@ build_vec_init (tree base, tree maxindex, tree init, else from = NULL_TREE; - if (from_array == 2) + if (TREE_CODE (type) == ARRAY_TYPE) + elt_init = build_vec_init (to, NULL_TREE, from, /*val_init*/false, + from_array, complain); + else if (from_array == 2) elt_init = cp_build_modify_expr (input_location, to, NOP_EXPR, from, complain); else if (type_build_ctor_call (type))
Hi, On 26/03/2018 19:12, Jason Merrill wrote: > Your build_aggr_init change is OK, but I had in mind something more > general in build_vec_init: Oh nice. Shall I test it together with my build_aggr_type bits and the testcases and commit it if everything is Ok? By the way - FYI - what I had tested was used *only* by lambda-array.C and lambda-errloc.C. Paolo.
On Mon, Mar 26, 2018 at 1:57 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > On 26/03/2018 19:12, Jason Merrill wrote: >> >> Your build_aggr_init change is OK, but I had in mind something more >> general in build_vec_init: > > Oh nice. Shall I test it together with my build_aggr_type bits and the > testcases and commit it if everything is Ok? Please. Jason
Index: cp/init.c =================================================================== --- cp/init.c (revision 258745) +++ cp/init.c (working copy) @@ -1688,12 +1688,14 @@ build_aggr_init (tree exp, tree init, int flags, t } else { - /* An array may not be initialized use the parenthesized - initialization form -- unless the initializer is "()". */ - if (init && TREE_CODE (init) == TREE_LIST) + /* An array may not be initialized usinge the parenthesized + initialization form -- unless the initializer is "()". + Also reject VAR_DECLs (c++/84632). */ + if (init && (TREE_CODE (init) == TREE_LIST || VAR_P (init))) { if (complain & tf_error) - error ("bad array initializer"); + error_at (init_loc, "array must be initialized " + "with a brace-enclosed initializer"); return error_mark_node; } /* Must arrange to initialize each element of EXP Index: testsuite/g++.dg/init/array49.C =================================================================== --- testsuite/g++.dg/init/array49.C (nonexistent) +++ testsuite/g++.dg/init/array49.C (working copy) @@ -0,0 +1,6 @@ +// PR c++/84632 +// { dg-additional-options "-w" } + +class { + &a; // { dg-error "forbids declaration" } +} b[2] = b; // { dg-error "initialized" }