| Message ID | e989d2a5-5ef6-a605-b1b0-472b0ce87adc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid ICE when pretty-printing a VLA with an error bound (PR 85956) | expand |
On Wed, May 30, 2018 at 02:39:15PM -0600, Martin Sebor wrote: > gcc/c-family/ChangeLog: > > PR middle-end/85956 > * c-pretty-print.c (c_pretty_printer::direct_abstract_declarator): > Handle error-mark-node in array bounds gracefully. This isn't sufficient, as it still ICEs with C++: during GIMPLE pass: vrp In function ‘_Z3fooiPv._omp_fn.0’: tree check: expected class ‘type’, have ‘exceptional’ (error_mark) in build_int_cst, at tree.c:1342 #pragma omp parallel shared(a) default(none) ^~~ 0x15ef6b3 tree_class_check_failed(tree_node const*, tree_code_class, char const*, int, char const*) ../../gcc/tree.c:9385 0x80fb7c tree_class_check(tree_node*, tree_code_class, char const*, int, char const*) ../../gcc/tree.h:3258 0x15d017c build_int_cst(tree_node*, poly_int<1u, long>) ../../gcc/tree.c:1342 0xe2b685 round_up_loc(unsigned int, tree_node*, unsigned int) ../../gcc/fold-const.c:14330 0x1233717 finalize_type_size ../../gcc/stor-layout.c:1908 0x1238390 layout_type(tree_node*) ../../gcc/stor-layout.c:2578 0x15e9d8c build_array_type_1 ../../gcc/tree.c:7869 0x15ea022 build_array_type(tree_node*, tree_node*, bool) ../../gcc/tree.c:7906 0xad28b7 build_cplus_array_type(tree_node*, tree_node*) ../../gcc/cp/tree.c:985 0xad46c5 strip_typedefs(tree_node*, bool*) ../../gcc/cp/tree.c:1459 0x9312a8 type_to_string ../../gcc/cp/error.c:3176 0x93425c cp_printer ../../gcc/cp/error.c:4085 0x1f79f1b pp_format(pretty_printer*, text_info*) ../../gcc/pretty-print.c:1375 I came up with the following hack instead (or in addition to), replace those error_mark_node bounds with NULL (i.e. pretend flexible array members) if during OpenMP/OpenACC outlining we've decided not to pass around the bounds artificial decl because nothing really use it. Is this a reasonable hack, or shall we go with Martin's patch + similar change in C++ pretty printer to handle error_mark_node specially and perhaps also handle NULL specially too as the patch does, or both those FE changes and this, something else? Bootstrapped/regtested on x86_64-linux and i686-linux. 2018-05-31 Jakub Jelinek <jakub@redhat.com> PR middle-end/85956 * tree-inline.h (struct copy_body_data): Add adjust_array_error_bounds field. * tree-inline.c (remap_type_1): Formatting fix. If TYPE_MAX_VALUE of ARRAY_TYPE's TYPE_DOMAIN is newly error_mark_node, replace it with NULL if id->adjust_array_error_bounds. * omp-low.c (new_omp_context): Set cb.adjust_array_error_bounds. cp/ * error.c (dump_type_suffix): Don't crash on NULL max. testsuite/ * g++.dg/gomp/pr85956.c: New test. --- gcc/tree-inline.h.jj 2018-01-03 10:19:54.931533922 +0100 +++ gcc/tree-inline.h 2018-05-30 18:59:47.433022338 +0200 @@ -151,6 +151,10 @@ struct copy_body_data /* Do not create new declarations when within type remapping. */ bool prevent_decl_creation_for_types; + + /* Replace error_mark_node as upper bound of array types with + NULL. */ + bool adjust_array_error_bounds; }; /* Weights of constructions for estimate_num_insns. */ --- gcc/tree-inline.c.jj 2018-05-29 13:57:38.360164318 +0200 +++ gcc/tree-inline.c 2018-05-30 19:06:49.897605141 +0200 @@ -519,11 +519,21 @@ remap_type_1 (tree type, copy_body_data if (TYPE_MAIN_VARIANT (new_tree) != new_tree) { - gcc_checking_assert (TYPE_DOMAIN (type) == TYPE_DOMAIN (TYPE_MAIN_VARIANT (type))); + gcc_checking_assert (TYPE_DOMAIN (type) + == TYPE_DOMAIN (TYPE_MAIN_VARIANT (type))); TYPE_DOMAIN (new_tree) = TYPE_DOMAIN (TYPE_MAIN_VARIANT (new_tree)); } else - TYPE_DOMAIN (new_tree) = remap_type (TYPE_DOMAIN (new_tree), id); + { + TYPE_DOMAIN (new_tree) = remap_type (TYPE_DOMAIN (new_tree), id); + /* For array bounds where we have decided not to copy over the bounds + variable which isn't used in OpenMP/OpenACC region, change them to + NULL. */ + if (TYPE_MAX_VALUE (TYPE_DOMAIN (new_tree)) == error_mark_node + && id->adjust_array_error_bounds + && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node) + TYPE_MAX_VALUE (TYPE_DOMAIN (new_tree)) = NULL_TREE; + } break; case RECORD_TYPE: --- gcc/omp-low.c.jj 2018-02-10 00:22:01.337951717 +0100 +++ gcc/omp-low.c 2018-05-30 19:03:13.876307134 +0200 @@ -855,6 +855,7 @@ new_omp_context (gimple *stmt, omp_conte } ctx->cb.decl_map = new hash_map<tree, tree>; + ctx->cb.adjust_array_error_bounds = true; return ctx; } --- gcc/cp/error.c.jj 2018-05-25 14:34:41.958381711 +0200 +++ gcc/cp/error.c 2018-05-30 19:19:26.890594362 +0200 @@ -922,8 +922,10 @@ dump_type_suffix (cxx_pretty_printer *pp if (tree dtype = TYPE_DOMAIN (t)) { tree max = TYPE_MAX_VALUE (dtype); + if (max == NULL_TREE) + ; /* Zero-length arrays have an upper bound of SIZE_MAX. */ - if (integer_all_onesp (max)) + else if (integer_all_onesp (max)) pp_character (pp, '0'); else if (tree_fits_shwi_p (max)) pp_wide_integer (pp, tree_to_shwi (max) + 1); --- gcc/testsuite/g++.dg/gomp/pr85956.c.jj 2018-05-30 19:20:06.236645197 +0200 +++ gcc/testsuite/g++.dg/gomp/pr85956.c 2018-05-30 19:16:13.313344213 +0200 @@ -0,0 +1,12 @@ +/* PR middle-end/85956 */ +/* { dg-do compile } */ +/* { dg-additional-options "-O2 -Wall" } */ + +void +foo (int n, void *p) +{ + int (*a)[n] = (int (*)[n]) p; + #pragma omp parallel shared(a) default(none) + #pragma omp master + a[-1][-1] = 42; /* { dg-warning "array subscript -1 is below array bounds" } */ +} Jakub
On Thu, May 31, 2018 at 2:58 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, May 30, 2018 at 02:39:15PM -0600, Martin Sebor wrote: >> gcc/c-family/ChangeLog: >> >> PR middle-end/85956 >> * c-pretty-print.c (c_pretty_printer::direct_abstract_declarator): >> Handle error-mark-node in array bounds gracefully. > > This isn't sufficient, as it still ICEs with C++: > during GIMPLE pass: vrp > In function ‘_Z3fooiPv._omp_fn.0’: > tree check: expected class ‘type’, have ‘exceptional’ (error_mark) in build_int_cst, at tree.c:1342 > #pragma omp parallel shared(a) default(none) > ^~~ > 0x15ef6b3 tree_class_check_failed(tree_node const*, tree_code_class, char const*, int, char const*) > ../../gcc/tree.c:9385 > 0x80fb7c tree_class_check(tree_node*, tree_code_class, char const*, int, char const*) > ../../gcc/tree.h:3258 > 0x15d017c build_int_cst(tree_node*, poly_int<1u, long>) > ../../gcc/tree.c:1342 > 0xe2b685 round_up_loc(unsigned int, tree_node*, unsigned int) > ../../gcc/fold-const.c:14330 > 0x1233717 finalize_type_size > ../../gcc/stor-layout.c:1908 > 0x1238390 layout_type(tree_node*) > ../../gcc/stor-layout.c:2578 > 0x15e9d8c build_array_type_1 > ../../gcc/tree.c:7869 > 0x15ea022 build_array_type(tree_node*, tree_node*, bool) > ../../gcc/tree.c:7906 > 0xad28b7 build_cplus_array_type(tree_node*, tree_node*) > ../../gcc/cp/tree.c:985 > 0xad46c5 strip_typedefs(tree_node*, bool*) > ../../gcc/cp/tree.c:1459 > 0x9312a8 type_to_string > ../../gcc/cp/error.c:3176 > 0x93425c cp_printer > ../../gcc/cp/error.c:4085 > 0x1f79f1b pp_format(pretty_printer*, text_info*) > ../../gcc/pretty-print.c:1375 > > I came up with the following hack instead (or in addition to), > replace those error_mark_node bounds with NULL (i.e. pretend flexible array > members) if during OpenMP/OpenACC outlining we've decided not to pass around > the bounds artificial decl because nothing really use it. > > Is this a reasonable hack, or shall we go with Martin's patch + similar > change in C++ pretty printer to handle error_mark_node specially and perhaps > also handle NULL specially too as the patch does, or both those FE changes > and this, something else? We generally try to avoid embedded error_mark_node within other trees. If the array bound is erroneous, can we replace the whole array type with error_mark_node? Jason
On Thu, May 31, 2018 at 09:14:33AM -0400, Jason Merrill wrote: > > I came up with the following hack instead (or in addition to), > > replace those error_mark_node bounds with NULL (i.e. pretend flexible array > > members) if during OpenMP/OpenACC outlining we've decided not to pass around > > the bounds artificial decl because nothing really use it. > > > > Is this a reasonable hack, or shall we go with Martin's patch + similar > > change in C++ pretty printer to handle error_mark_node specially and perhaps > > also handle NULL specially too as the patch does, or both those FE changes > > and this, something else? > > We generally try to avoid embedded error_mark_node within other trees. > If the array bound is erroneous, can we replace the whole array type > with error_mark_node? The array bound isn't errorneous, just becomes unknown (well, known only in an outer function), we still need to know it is an array type and that it has 0 as the low bound. Instead of replacing it with NULL we in theory could just create another VAR_DECL and never initialize it, it wouldn't be far from what happens with some other VLAs - during optimizations it is possible to array bound var is optimized away. Just it would be much more work to do it that way. Jakub
On 05/31/2018 07:30 AM, Jakub Jelinek wrote: > On Thu, May 31, 2018 at 09:14:33AM -0400, Jason Merrill wrote: >>> I came up with the following hack instead (or in addition to), >>> replace those error_mark_node bounds with NULL (i.e. pretend flexible array >>> members) if during OpenMP/OpenACC outlining we've decided not to pass around >>> the bounds artificial decl because nothing really use it. >>> >>> Is this a reasonable hack, or shall we go with Martin's patch + similar >>> change in C++ pretty printer to handle error_mark_node specially and perhaps >>> also handle NULL specially too as the patch does, or both those FE changes >>> and this, something else? >> >> We generally try to avoid embedded error_mark_node within other trees. >> If the array bound is erroneous, can we replace the whole array type >> with error_mark_node? > > The array bound isn't errorneous, just becomes unknown (well, known only in > an outer function), we still need to know it is an array type and that it > has 0 as the low bound. > Instead of replacing it with NULL we in theory could just create another > VAR_DECL and never initialize it, it wouldn't be far from what happens with > some other VLAs - during optimizations it is possible to array bound var is > optimized away. Just it would be much more work to do it that way. In my mind the issue boils down to two questions: 1) should the pretty printer handle error-mark-node gracefully or is it appropriate for it to abort? 2) is it appropriate to be embedding/using error_mark_node in valid constructs as a proxy for "unused" or "unknown" or such? I would expect the answer to (1) to be yes. Despite that, I agree with Jason that the answer to (2) should be no. That said, I don't think the fix for this bug needs to depend on solving (2). We can avoid the ICE by changing the pretty printers and adjust the openmp implementation later. Martin
On Thu, May 31, 2018 at 11:00 AM, Martin Sebor <msebor@gmail.com> wrote: > On 05/31/2018 07:30 AM, Jakub Jelinek wrote: >> >> On Thu, May 31, 2018 at 09:14:33AM -0400, Jason Merrill wrote: >>>> >>>> I came up with the following hack instead (or in addition to), >>>> replace those error_mark_node bounds with NULL (i.e. pretend flexible >>>> array >>>> members) if during OpenMP/OpenACC outlining we've decided not to pass >>>> around >>>> the bounds artificial decl because nothing really use it. >>>> >>>> Is this a reasonable hack, or shall we go with Martin's patch + similar >>>> change in C++ pretty printer to handle error_mark_node specially and >>>> perhaps >>>> also handle NULL specially too as the patch does, or both those FE >>>> changes >>>> and this, something else? >>> >>> >>> We generally try to avoid embedded error_mark_node within other trees. >>> If the array bound is erroneous, can we replace the whole array type >>> with error_mark_node? >> >> >> The array bound isn't errorneous, just becomes unknown (well, known only >> in >> an outer function), we still need to know it is an array type and that it >> has 0 as the low bound. >> Instead of replacing it with NULL we in theory could just create another >> VAR_DECL and never initialize it, it wouldn't be far from what happens >> with >> some other VLAs - during optimizations it is possible to array bound var >> is >> optimized away. Just it would be much more work to do it that way. > > > In my mind the issue boils down to two questions: > > 1) should the pretty printer handle error-mark-node gracefully > or is it appropriate for it to abort? > 2) is it appropriate to be embedding/using error_mark_node in > valid constructs as a proxy for "unused" or "unknown" or > such? > > I would expect the answer to (1) to be yes. Despite that, > I agree with Jason that the answer to (2) should be no. > > That said, I don't think the fix for this bug needs to depend > on solving (2). We can avoid the ICE by changing the pretty > printers and adjust the openmp implementation later. The problem with embedded error_mark_node is that lots of places are going to blow up like this, and we don't want to change everything to expect it. Adjusting the pretty-printer might fix this particular testcase, but other things are likely to get tripped up by the same problem. Where is the error_mark_node coming from in the first place? Jason
On Thu, May 31, 2018 at 11:19:08AM -0400, Jason Merrill wrote: > > In my mind the issue boils down to two questions: > > > > 1) should the pretty printer handle error-mark-node gracefully > > or is it appropriate for it to abort? > > 2) is it appropriate to be embedding/using error_mark_node in > > valid constructs as a proxy for "unused" or "unknown" or > > such? > > > > I would expect the answer to (1) to be yes. Despite that, > > I agree with Jason that the answer to (2) should be no. > > > > That said, I don't think the fix for this bug needs to depend > > on solving (2). We can avoid the ICE by changing the pretty > > printers and adjust the openmp implementation later. > > The problem with embedded error_mark_node is that lots of places are > going to blow up like this, and we don't want to change everything to > expect it. Adjusting the pretty-printer might fix this particular > testcase, but other things are likely to get tripped up by the same > problem. > > Where is the error_mark_node coming from in the first place? remap_type invoked during omp-low.c (scan_omp). omp_copy_decl returns error_mark_node for decls that tree-inline.c wants to remap, but they aren't actually remapped for some reason. For normal VLAs gimplify.c makes sure the needed artifical decls are firstprivatized, but in this case (VLA not in some decl's type, but just referenced indirectly through pointers) nothing scans those unless those temporaries are actually used in the code. Jakub
On Thu, May 31, 2018 at 11:31 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, May 31, 2018 at 11:19:08AM -0400, Jason Merrill wrote: >> > In my mind the issue boils down to two questions: >> > >> > 1) should the pretty printer handle error-mark-node gracefully >> > or is it appropriate for it to abort? >> > 2) is it appropriate to be embedding/using error_mark_node in >> > valid constructs as a proxy for "unused" or "unknown" or >> > such? >> > >> > I would expect the answer to (1) to be yes. Despite that, >> > I agree with Jason that the answer to (2) should be no. >> > >> > That said, I don't think the fix for this bug needs to depend >> > on solving (2). We can avoid the ICE by changing the pretty >> > printers and adjust the openmp implementation later. >> >> The problem with embedded error_mark_node is that lots of places are >> going to blow up like this, and we don't want to change everything to >> expect it. Adjusting the pretty-printer might fix this particular >> testcase, but other things are likely to get tripped up by the same >> problem. >> >> Where is the error_mark_node coming from in the first place? > > remap_type invoked during omp-low.c (scan_omp). > omp_copy_decl returns error_mark_node for decls that tree-inline.c wants > to remap, but they aren't actually remapped for some reason. > For normal VLAs gimplify.c makes sure the needed artifical decls are > firstprivatized, but in this case (VLA not in some decl's type, but just > referenced indirectly through pointers) nothing scans those unless > those temporaries are actually used in the code. Returning error_mark_node from omp_copy_decl and then continuing seems like the problem, then. Would it really be that hard to return an uninitialized variable instead? Jason
On Thu, May 31, 2018 at 01:34:19PM -0400, Jason Merrill wrote: > >> Where is the error_mark_node coming from in the first place? > > > > remap_type invoked during omp-low.c (scan_omp). > > omp_copy_decl returns error_mark_node for decls that tree-inline.c wants > > to remap, but they aren't actually remapped for some reason. > > For normal VLAs gimplify.c makes sure the needed artifical decls are > > firstprivatized, but in this case (VLA not in some decl's type, but just > > referenced indirectly through pointers) nothing scans those unless > > those temporaries are actually used in the code. > > Returning error_mark_node from omp_copy_decl and then continuing seems > like the problem, then. Would it really be that hard to return an > uninitialized variable instead? The routine doesn't know if it is used in a context of a VLA bound or something else, in the former case it is acceptable to swap it for some other var, but in the latter case it would be just a bug, so using error_mark_node in that case instead is better to catch those. I can try to do that in tree-inline.c, but really not sure how hard would it be. Or handle it in the gimplifier, scan for such vars and add private clauses for those, that will mean nothing will be passed around. Jakub
On 05/31/2018 11:43 AM, Jakub Jelinek wrote: > On Thu, May 31, 2018 at 01:34:19PM -0400, Jason Merrill wrote: >>>> Where is the error_mark_node coming from in the first place? >>> >>> remap_type invoked during omp-low.c (scan_omp). >>> omp_copy_decl returns error_mark_node for decls that tree-inline.c wants >>> to remap, but they aren't actually remapped for some reason. >>> For normal VLAs gimplify.c makes sure the needed artifical decls are >>> firstprivatized, but in this case (VLA not in some decl's type, but just >>> referenced indirectly through pointers) nothing scans those unless >>> those temporaries are actually used in the code. >> >> Returning error_mark_node from omp_copy_decl and then continuing seems >> like the problem, then. Would it really be that hard to return an >> uninitialized variable instead? > > The routine doesn't know if it is used in a context of a VLA bound or > something else, in the former case it is acceptable to swap it for some > other var, but in the latter case it would be just a bug, so using > error_mark_node in that case instead is better to catch those. > I can try to do that in tree-inline.c, but really not sure how hard would it > be. > Or handle it in the gimplifier, scan for such vars and add private clauses > for those, that will mean nothing will be passed around. Are you still working on this approach or should I resubmit my simple patch with the corresponding change for the C++ FE? Martin
PR middle-end/85956 - ICE in wide_int_to_tree_1:1549 gcc/c-family/ChangeLog: PR middle-end/85956 * c-pretty-print.c (c_pretty_printer::direct_abstract_declarator): Handle error-mark-node in array bounds gracefully. gcc/testsuite/ChangeLog: PR middle-end/85956 * gcc.dg/gomp/pr85956.c: New test. Index: gcc/c-family/c-pretty-print.c =================================================================== --- gcc/c-family/c-pretty-print.c (revision 260969) +++ gcc/c-family/c-pretty-print.c (working copy) @@ -570,20 +570,26 @@ c_pretty_printer::direct_abstract_declarator (tree break; case ARRAY_TYPE: - pp_c_left_bracket (this); - if (TYPE_DOMAIN (t) && TYPE_MAX_VALUE (TYPE_DOMAIN (t))) - { - tree maxval = TYPE_MAX_VALUE (TYPE_DOMAIN (t)); - tree type = TREE_TYPE (maxval); + { + pp_c_left_bracket (this); - if (tree_fits_shwi_p (maxval)) - pp_wide_integer (this, tree_to_shwi (maxval) + 1); - else - expression (fold_build2 (PLUS_EXPR, type, maxval, - build_int_cst (type, 1))); - } - pp_c_right_bracket (this); - direct_abstract_declarator (TREE_TYPE (t)); + if (tree dom = TYPE_DOMAIN (t)) + { + tree maxval = TYPE_MAX_VALUE (dom); + if (maxval && maxval != error_mark_node) + { + tree type = TREE_TYPE (maxval); + + if (tree_fits_shwi_p (maxval)) + pp_wide_integer (this, tree_to_shwi (maxval) + 1); + else + expression (fold_build2 (PLUS_EXPR, type, maxval, + build_int_cst (type, 1))); + } + } + pp_c_right_bracket (this); + direct_abstract_declarator (TREE_TYPE (t)); + } break; case IDENTIFIER_NODE: Index: gcc/testsuite/gcc.dg/gomp/pr85956.c =================================================================== --- gcc/testsuite/gcc.dg/gomp/pr85956.c (nonexistent) +++ gcc/testsuite/gcc.dg/gomp/pr85956.c (working copy) @@ -0,0 +1,13 @@ +/* PR middle-end/85956 - ICE in wide_int_to_tree_1, at tree.c:1549 + { dg-do compile } + { dg-options "-O2 -Wall -fopenmp" } */ + +void foo (int n, void *p) +{ + int (*a)[n] = (int (*)[n]) p; + +#pragma omp parallel shared(a) default(none) +#pragma omp master + + a[-1][-1] = 42; /* { dg-warning "\\\[-Warray-bounds]" } */ +}