[PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

The syntactically valid but undefined test case submitted in bug
85956 shows that the pretty-printer ICEs when passed a pointer
to variable-length array whose upper bound is an error-mark-node.

The ICE is triggered by -Warray-bounds discovering that a negative
subscript into the VLA is out-of-bounds and trying to mention
the type of the VLA in the diagnostic.  The error bound appears
to be the result of the omp pass.

The attached change avoids the ICE by ignoring error-mark-node
as an array bound.  It results in -Warray-bounds printing:

   array subscript -1 is below array bounds of ‘int[]’

for the VLA type.  That's not ideal because the printed type is
indistinguishable from an array with an unknown bound, but in
the absence of a valid bound I can't really think of a solution
that's much better (but see below).

Without -fopenmp, when it detects an invalid index into a VLA
-Warray-bounds might print something like:

   array subscript -1 is below array bounds of ‘int[<Uef30> + 1]’

The <Uef30> + 1 represents the variable-length upper bound.  That
doesn't seem particularly informative or helpful but since it's
unrelated to the ICE (and might come up in other diagnostics
besides -Warray-bounds) I haven't changed it in the attached
patch.

If we want to print VLAs differently in diagnostics I think it
should be done in a separate change.  One possibility is to
leave the bound out altogether as this patch does.  Another is
to use the C [*] VLA syntax.  That should turn the above into:

   array subscript -1 is below array bounds of ‘int[*]’

This notation could be used just for valid VLAs or also for
the openmp VLAs with an error upper bound if they can't be
made valid.

Martin

[-- Attachment #2: gcc-85956.diff --]
[-- Type: text/x-patch, Size: 2241 bytes --]

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]" } */
+}

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jakub Jelinek]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jason Merrill]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jakub Jelinek]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Martin Sebor]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jason Merrill]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jakub Jelinek]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jason Merrill]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Jakub Jelinek]

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

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956)

[Martin Sebor]

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

[PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956, take 2)

[Jakub Jelinek]

Hi!

On Thu, May 31, 2018 at 01:34:19PM -0400, Jason Merrill wrote:
> 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 following patch does that, but not from omp_copy_decl, but only in the
caller for the array bounds (the rest would be still errors).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-01-07  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/85956
	PR lto/88733
	* 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
	a dummy "omp dummy var" variable if id->adjust_array_error_bounds.
	* omp-low.c (new_omp_context): Set cb.adjust_array_error_bounds.
fortran/
	* trans-openmp.c: Include attribs.h.
	(gfc_walk_alloc_comps, gfc_omp_clause_linear_ctor): Handle
	VAR_DECL max bound with "omp dummy var" attribute like NULL or
	error_mark_node - recompute number of elts independently.
testsuite/
	* c-c++-common/gomp/pr85956.c: New test.
	* g++.dg/gomp/pr88733.C: New test.

--- gcc/tree-inline.h.jj	2019-01-07 12:37:38.644966905 +0100
+++ gcc/tree-inline.h	2019-01-07 18:03:27.478852009 +0100
@@ -122,6 +122,10 @@ struct copy_body_data
   /* True if the location information will need to be reset.  */
   bool reset_location;
 
+  /* Replace error_mark_node as upper bound of array types with
+     an uninitialized VAR_DECL temporary.  */
+  bool adjust_array_error_bounds;
+
   /* A function to be called when duplicating BLOCK nodes.  */
   void (*transform_lang_insert_block) (tree);
 
--- gcc/tree-inline.c.jj	2019-01-07 12:37:38.635967053 +0100
+++ gcc/tree-inline.c	2019-01-07 19:09:29.887727827 +0100
@@ -523,11 +523,27 @@ 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
+	     an uninitialized VAR_DECL temporary.  */
+	  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)
+	    {
+	      tree v = create_tmp_var (TREE_TYPE (TYPE_DOMAIN (new_tree)));
+	      DECL_ATTRIBUTES (v)
+		= tree_cons (get_identifier ("omp dummy var"), NULL_TREE,
+			     DECL_ATTRIBUTES (v));
+	      TYPE_MAX_VALUE (TYPE_DOMAIN (new_tree)) = v;
+	    }
+        }
       break;
 
     case RECORD_TYPE:
--- gcc/omp-low.c.jj	2019-01-07 12:37:38.501969255 +0100
+++ gcc/omp-low.c	2019-01-07 18:03:27.509851500 +0100
@@ -872,6 +872,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/fortran/trans-openmp.c.jj	2019-01-01 12:37:52.699391804 +0100
+++ gcc/fortran/trans-openmp.c	2019-01-07 19:17:00.295377803 +0100
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.
 #include "diagnostic-core.h"
 #undef GCC_DIAG_STYLE
 #define GCC_DIAG_STYLE __gcc_gfc__
+#include "attribs.h"
 
 int ompws_flags;
 
@@ -297,10 +298,19 @@ gfc_walk_alloc_comps (tree decl, tree de
 	}
       else
 	{
+	  bool compute_nelts = false;
 	  if (!TYPE_DOMAIN (type)
 	      || TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE
 	      || TYPE_MIN_VALUE (TYPE_DOMAIN (type)) == error_mark_node
 	      || TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == error_mark_node)
+	    compute_nelts = true;
+	  else if (VAR_P (TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
+	    {
+	      tree a = DECL_ATTRIBUTES (TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
+	      if (lookup_attribute ("omp dummy var", a))
+		compute_nelts = true;
+	    }
+	  if (compute_nelts)
 	    {
 	      tem = fold_build2 (EXACT_DIV_EXPR, sizetype,
 				 TYPE_SIZE_UNIT (type),
@@ -912,11 +922,20 @@ gfc_omp_clause_linear_ctor (tree clause,
       && (!GFC_DECL_GET_SCALAR_ALLOCATABLE (OMP_CLAUSE_DECL (clause))
 	  || !POINTER_TYPE_P (type)))
     {
+      bool compute_nelts = false;
       gcc_assert (TREE_CODE (type) == ARRAY_TYPE);
       if (!TYPE_DOMAIN (type)
 	  || TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE
 	  || TYPE_MIN_VALUE (TYPE_DOMAIN (type)) == error_mark_node
 	  || TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == error_mark_node)
+	compute_nelts = true;
+      else if (VAR_P (TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
+	{
+	  tree a = DECL_ATTRIBUTES (TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
+	  if (lookup_attribute ("omp dummy var", a))
+	    compute_nelts = true;
+	}
+      if (compute_nelts)
 	{
 	  nelems = fold_build2 (EXACT_DIV_EXPR, sizetype,
 				TYPE_SIZE_UNIT (type),
--- gcc/testsuite/c-c++-common/gomp/pr85956.c.jj	2019-01-07 18:03:27.509851500 +0100
+++ gcc/testsuite/c-c++-common/gomp/pr85956.c	2019-01-07 18:03:27.509851500 +0100
@@ -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" } */
+}
--- gcc/testsuite/g++.dg/gomp/pr88733.C.jj	2019-01-07 19:42:10.226723145 +0100
+++ gcc/testsuite/g++.dg/gomp/pr88733.C	2019-01-07 19:41:51.200024075 +0100
@@ -0,0 +1,29 @@
+// PR lto/88733
+// { dg-do compile }
+// { dg-additional-options "-flto -ffat-lto-objects" { target lto } }
+
+struct A { int f; } a;
+
+__attribute__((noipa)) void
+bar (A **x, int)
+{
+  x[0] = &a;
+}
+
+int
+foo (int n)
+{
+  int g;
+  A *j[n];
+  bar (j, n);
+#pragma omp parallel
+#pragma omp single
+  g = j[0]->f;
+  return g;
+}
+
+int
+main ()
+{
+  foo (0);
+}


	Jakub

Re: [PATCH] avoid ICE when pretty-printing a VLA with an error bound (PR 85956, take 2)

[Jeff Law]

On 1/7/19 3:42 PM, Jakub Jelinek wrote:
> Hi!
> 
> On Thu, May 31, 2018 at 01:34:19PM -0400, Jason Merrill wrote:
>> 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 following patch does that, but not from omp_copy_decl, but only in the
> caller for the array bounds (the rest would be still errors).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-01-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/85956
> 	PR lto/88733
> 	* 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
> 	a dummy "omp dummy var" variable if id->adjust_array_error_bounds.
> 	* omp-low.c (new_omp_context): Set cb.adjust_array_error_bounds.
> fortran/
> 	* trans-openmp.c: Include attribs.h.
> 	(gfc_walk_alloc_comps, gfc_omp_clause_linear_ctor): Handle
> 	VAR_DECL max bound with "omp dummy var" attribute like NULL or
> 	error_mark_node - recompute number of elts independently.
> testsuite/
> 	* c-c++-common/gomp/pr85956.c: New test.
> 	* g++.dg/gomp/pr88733.C: New test.
OK.
jeff