[PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Patrick Palka]

The Cilk Plus code may sometimes turn a COND_EXPR into an
error_mark_node for no good reason.  This can be seen by compiling the
test case c-c++-common/cilk-plus/CK/pr60469.c with both gcc and g++ and
observing the differences of the -fdump-tree-original dumps:

With gcc, we get the following code in the GENERIC dump:

finally
  {
    _Cilk_sync;
    D.1897.worker->current_stack_frame = D.1897.call_parent;
    __cilkrts_pop_frame (&D.1897);
    if (D.1897.flags != 16777216)
      {
        __cilkrts_leave_frame (&D.1897);
      }
  }

Whereas with g++ we get

finally
  {
    _Cilk_sync;
    D.2387.worker->current_stack_frame = D.2387.call_parent;
    __cilkrts_pop_frame (&D.2387);
    <<< error >>>
  }

Notice that the COND_EXPR is replaced with an << error >> in the g++
dump.

This patch fixes the COND_EXPR handling within Cilk Plus.  I think the
cause is a simple typo/logic bug.

gcc/cp/ChangeLog:

	* cp-array-notation.c (cp_expand_cond_array_notations): Return
	error_mark_node only if find_rank failed, not if it was
	successful.
---
 gcc/cp/cp-array-notation.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
index 8862af1..3a6d773 100644
--- a/gcc/cp/cp-array-notation.c
+++ b/gcc/cp/cp-array-notation.c
@@ -807,8 +807,8 @@ cp_expand_cond_array_notations (tree orig_stmt)
       if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank)
 	  || !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true,
 			 &yes_rank)
-	  || find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
-			&no_rank))
+	  || !find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
+			 &no_rank))
 	return error_mark_node;
       /* If the condition has a zero rank, then handle array notations in body
 	 separately.  */
-- 
2.7.0.rc3.56.g64157c6.dirty

[PATCH 2/3] Avoid creating an initializer for a flexible array member

[Patrick Palka]

If we do create such an initializer, we end up with an error_mark_node
during gimplification, because in cp-gimplify.c we pass this
VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
it spits on an error_mark_node.  This happens in e.g. the test case
g++.dg/ext/array1.C.

This patch makes it so that we avoid generating an initializer for a
flexible array member, thus avoiding to end up with an error_mark_node
during gimplification.  The same kind of thing is done ~90 lines down
from the code I changed.

gcc/cp/ChangeLog:

	* init.c (perform_member_init): Avoid creating an initializer
	for a flexible array member.
---
 gcc/cp/init.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 09c1183..0011178 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -645,9 +645,14 @@ perform_member_init (tree member, tree init)
       /* mem() means value-initialization.  */
       if (TREE_CODE (type) == ARRAY_TYPE)
 	{
-	  init = build_vec_init_expr (type, init, tf_warning_or_error);
-	  init = build2 (INIT_EXPR, type, decl, init);
-	  finish_expr_stmt (init);
+	  /* Initialize the array only if it's not a flexible
+	     array member (i.e., if it has an upper bound).  */
+	  if (TYPE_DOMAIN (type) && TYPE_MAX_VALUE (TYPE_DOMAIN (type)))
+	    {
+	      init = build_vec_init_expr (type, init, tf_warning_or_error);
+	      init = build2 (INIT_EXPR, type, decl, init);
+	      finish_expr_stmt (init);
+	    }
 	}
       else
 	{
-- 
2.7.0.rc3.56.g64157c6.dirty

[PATCH 3/3] [RFC] Treat a gimplification failure as an internal error

[Patrick Palka]

This patch makes it so that a gimplification failure is considered to be
an internal error under normal circumstances, so that we otherwise avoid
silently generating wrong code if e.g. a buggy frontend fed us a
malformed tree.

The rationale for this change is that it's better to abort compilation
than to silently generate wrong code.  During gimplification we should
only see e.g. an error_mark_node if the frontend has already issued an
error.  Otherwise it is likely a bug in frontend.

This patch, for example, turns the PR c++/68948 wrong-code bug into an
ICE on invalid bug.  During testing it also caught two latent "bugs"
(patches 1 and 2 in this series).

This series was tested on x86_64-pc-linux-gnu, with --enable-languages=all,ada,go,
no new regressions.

Does this seem like a reasonable invariant to add to the gimplifier?

gcc/cp/ChangeLog:

	* cp-gimplify.c (gimplify_expr_stmt): Don't convert an
	error_mark_node to an empty statement.

gcc/ChangeLog:

	* gimplify.c (gimplify_return_expr): Remove a redundant test
	for error_mark_node.
	(gimplify_decl_expr): Return GS_ERROR if an initializer is an
	error_mark_node.
	(gimplify_expr): Treat a gimplification failure as an internal
	error.  Remove now-redundant GIMPLE_CHECKING checking code.
---
 gcc/cp/cp-gimplify.c |  5 +----
 gcc/gimplify.c       | 27 +++++++++++++--------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 373c9e1..2b0aaaf 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -424,16 +424,13 @@ gimplify_expr_stmt (tree *stmt_p)
 {
   tree stmt = EXPR_STMT_EXPR (*stmt_p);
 
-  if (stmt == error_mark_node)
-    stmt = NULL;
-
   /* Gimplification of a statement expression will nullify the
      statement if all its side effects are moved to *PRE_P and *POST_P.
 
      In this case we will not want to emit the gimplified statement.
      However, we may still want to emit a warning, so we do that before
      gimplification.  */
-  if (stmt && warn_unused_value)
+  if (stmt && stmt != error_mark_node && warn_unused_value)
     {
       if (!TREE_SIDE_EFFECTS (stmt))
 	{
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index bc90401..9a1d723 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1288,8 +1288,7 @@ gimplify_return_expr (tree stmt, gimple_seq *pre_p)
     }
 
   if (!ret_expr
-      || TREE_CODE (ret_expr) == RESULT_DECL
-      || ret_expr == error_mark_node)
+      || TREE_CODE (ret_expr) == RESULT_DECL)
     {
       greturn *ret = gimple_build_return (ret_expr);
       gimple_set_no_warning (ret, TREE_NO_WARNING (stmt));
@@ -1449,6 +1448,9 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
     {
       tree init = DECL_INITIAL (decl);
 
+      if (init == error_mark_node)
+	return GS_ERROR;
+
       if (TREE_CODE (DECL_SIZE_UNIT (decl)) != INTEGER_CST
 	  || (!TREE_STATIC (decl)
 	      && flag_stack_check == GENERIC_STACK_CHECK
@@ -1464,7 +1466,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
 	  && DECL_ARTIFICIAL (decl) && DECL_NAME (decl) == NULL_TREE)
 	gimple_add_tmp_var (decl);
 
-      if (init && init != error_mark_node)
+      if (init)
 	{
 	  if (!TREE_STATIC (decl))
 	    {
@@ -11036,17 +11038,6 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
     }
   else
     {
-#ifdef ENABLE_GIMPLE_CHECKING
-      if (!(fallback & fb_mayfail))
-	{
-	  fprintf (stderr, "gimplification failed:\n");
-	  print_generic_expr (stderr, *expr_p, 0);
-	  debug_tree (*expr_p);
-	  internal_error ("gimplification failed");
-	}
-#endif
-      gcc_assert (fallback & fb_mayfail);
-
       /* If this is an asm statement, and the user asked for the
 	 impossible, don't die.  Fail and let gimplify_asm_expr
 	 issue an error.  */
@@ -11064,6 +11055,14 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
     }
 
  out:
+  /* If gimplification failed, then either such failure was explicitly permitted
+     (via the FB_MAYFAIL flag) or the frontend fed us a malformed tree, e.g. one
+     containing an ERROR_MARK node -- for which the FE should have already issued an
+     error diagnostic.  Otherwise it is likely that an FE bug was triggered, in
+     which case it is better to abort than to risk silently generating wrong
+     code.  */
+  if (ret == GS_ERROR)
+    gcc_assert ((fallback & fb_mayfail) || seen_error ());
   input_location = saved_location;
   return ret;
 }
-- 
2.7.0.rc3.56.g64157c6.dirty

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Jeff Law]

On 12/31/2015 08:40 AM, Patrick Palka wrote:
> The Cilk Plus code may sometimes turn a COND_EXPR into an
> error_mark_node for no good reason.  This can be seen by compiling the
> test case c-c++-common/cilk-plus/CK/pr60469.c with both gcc and g++ and
> observing the differences of the -fdump-tree-original dumps:
>
> With gcc, we get the following code in the GENERIC dump:
>
> finally
>    {
>      _Cilk_sync;
>      D.1897.worker->current_stack_frame = D.1897.call_parent;
>      __cilkrts_pop_frame (&D.1897);
>      if (D.1897.flags != 16777216)
>        {
>          __cilkrts_leave_frame (&D.1897);
>        }
>    }
>
> Whereas with g++ we get
>
> finally
>    {
>      _Cilk_sync;
>      D.2387.worker->current_stack_frame = D.2387.call_parent;
>      __cilkrts_pop_frame (&D.2387);
>      <<< error >>>
>    }
>
> Notice that the COND_EXPR is replaced with an << error >> in the g++
> dump.
>
> This patch fixes the COND_EXPR handling within Cilk Plus.  I think the
> cause is a simple typo/logic bug.
>
> gcc/cp/ChangeLog:
>
> 	* cp-array-notation.c (cp_expand_cond_array_notations): Return
> 	error_mark_node only if find_rank failed, not if it was
> 	successful.
Can you use -fdump-tree-original in the testcase and verify there's no 
<<< error >>> expressions in the resulting dump file?

With that change, this is OK.

jeff

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Jakub Jelinek]

On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
> >gcc/cp/ChangeLog:
> >
> >	* cp-array-notation.c (cp_expand_cond_array_notations): Return
> >	error_mark_node only if find_rank failed, not if it was
> >	successful.
> Can you use -fdump-tree-original in the testcase and verify there's no <<<
> error >>> expressions in the resulting dump file?
> 
> With that change, this is OK.

I think the patch is incomplete.  Because, find_rank does not always emit
an error if it returns false, so we again have cases where we can get
error_mark_node in the code without error being emitted.
      else if (*rank != current_rank)
        {
          /* In this case, find rank is being recursed through a set of
             expression of the form A <OPERATION> B, where A and B both have
             array notations in them and the rank of A is not equal to rank of B.
             A simple example of such case is the following: X[:] + Y[:][:] */
          *rank = current_rank;
          return false;
        }
and other spots.  E.g.
                  if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
                    error_at (EXPR_LOCATION (prev_arg),
                              "rank mismatch between %qE and %qE", prev_arg,
                              TREE_OPERAND (expr, ii));
looks very suspicious.

	Jakub

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Patrick Palka]

On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>> >gcc/cp/ChangeLog:
>> >
>> >     * cp-array-notation.c (cp_expand_cond_array_notations): Return
>> >     error_mark_node only if find_rank failed, not if it was
>> >     successful.
>> Can you use -fdump-tree-original in the testcase and verify there's no <<<
>> error >>> expressions in the resulting dump file?
>>
>> With that change, this is OK.
>
> I think the patch is incomplete.  Because, find_rank does not always emit
> an error if it returns false, so we again have cases where we can get
> error_mark_node in the code without error being emitted.
>       else if (*rank != current_rank)
>         {
>           /* In this case, find rank is being recursed through a set of
>              expression of the form A <OPERATION> B, where A and B both have
>              array notations in them and the rank of A is not equal to rank of B.
>              A simple example of such case is the following: X[:] + Y[:][:] */
>           *rank = current_rank;
>           return false;
>         }
> and other spots.  E.g.
>                   if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>                     error_at (EXPR_LOCATION (prev_arg),
>                               "rank mismatch between %qE and %qE", prev_arg,
>                               TREE_OPERAND (expr, ii));
> looks very suspicious.

Hmm, good point. Here's a contrived test case that causes find_rank to
return false without emitting an error message thus we again end up
with an error_mark_node in the gimplifier:

/* { dg-do compile } */
/* { dg-options "-fcilkplus" } */

void foo() {}

#define ALEN 1024

int main(int argc, char* argv[])
{
  typedef void (*f) (void *);
  f b[ALEN], c[ALEN][ALEN];
  (b[:]) ((void *)c[:][:]);
  _Cilk_spawn foo();
  return 0;
}

But this patch was intended to only fix the testsuite fallout that
patch 3 would have otherwise caused, and not to e.g. fix all the bugs
with find_rank.

(BTW patch 3 also makes this test case trigger an ICE, instead of
being silently miscompiled.)

Re: [PATCH 2/3] Avoid creating an initializer for a flexible array member

[Martin Sebor]

On 12/31/2015 08:40 AM, Patrick Palka wrote:
> If we do create such an initializer, we end up with an error_mark_node
> during gimplification, because in cp-gimplify.c we pass this
> VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
> it spits on an error_mark_node.  This happens in e.g. the test case
> g++.dg/ext/array1.C.
>
> This patch makes it so that we avoid generating an initializer for a
> flexible array member, thus avoiding to end up with an error_mark_node
> during gimplification.  The same kind of thing is done ~90 lines down
> from the code I changed.

I don't think this change is correct (or complete).  We could
decide that the kind of code it's meant to allow should be
accepted in general but if we did, this change alone would not
be sufficient.

In the way of background, the existing special treatment for
flexible array members was added to prevent rejecting cases
like this one:

   struct A { A (int); };
   struct B {
       B ();
       int n;
       A a[];
   };

Since B's ctor above doesn't initialize the flexible array
member it's well-formed.  Ditto if B's ctor is defined but
avoids initializing B::a.

The proposed change has the effect of also accepting the
following modified version of the example above that is
currently rejected:

   struct A { A (int); };
   struct B {
       B (): n (), a () { }
       int n;
       A a[];
   };

In this modified example, B's ctor attempts to default-initialize
the flexible array member and its elements using their respective
default ctors even though no such ctor for the latter exists.

Since C++ flexible array members are a GCC extension whose C++
specific aspects are essentially undocumented it's open to
discussion whether or not code like this should be accepted.
Clang rejects it, but one could make the argument that since
the flexible array member has no elements, default initializing
it should be allowed even if the same initialization would be
ill-formed if the array wasn't empty.

If the code should be accepted as written above then it should
also be accepted if B's ctor were omitted or defaulted. I.e.,
the following should be treated the same as the above:

   struct A { A (int); };
   struct B {
       // ditto with B() = default;
       int n;
       A a[];
   } b;

The patch doesn't handle either of these cases and the code
is rejected both with and without it.

Martin

PS While looking at this I experimented with zero-length
arrays to see if they might serve as a precedent.
Unfortunately, I discovered that those aren't currently
handled entirely consistently either.  I opened bug 69127
with the details.

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Jeff Law]

On 01/02/2016 04:26 PM, Patrick Palka wrote:
> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>>>> gcc/cp/ChangeLog:
>>>>
>>>>      * cp-array-notation.c (cp_expand_cond_array_notations): Return
>>>>      error_mark_node only if find_rank failed, not if it was
>>>>      successful.
>>> Can you use -fdump-tree-original in the testcase and verify there's no <<<
>>> error >>> expressions in the resulting dump file?
>>>
>>> With that change, this is OK.
>>
>> I think the patch is incomplete.  Because, find_rank does not always emit
>> an error if it returns false, so we again have cases where we can get
>> error_mark_node in the code without error being emitted.
>>        else if (*rank != current_rank)
>>          {
>>            /* In this case, find rank is being recursed through a set of
>>               expression of the form A <OPERATION> B, where A and B both have
>>               array notations in them and the rank of A is not equal to rank of B.
>>               A simple example of such case is the following: X[:] + Y[:][:] */
>>            *rank = current_rank;
>>            return false;
>>          }
>> and other spots.  E.g.
>>                    if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>>                      error_at (EXPR_LOCATION (prev_arg),
>>                                "rank mismatch between %qE and %qE", prev_arg,
>>                                TREE_OPERAND (expr, ii));
>> looks very suspicious.
>
> Hmm, good point. Here's a contrived test case that causes find_rank to
> return false without emitting an error message thus we again end up
> with an error_mark_node in the gimplifier:
>
> /* { dg-do compile } */
> /* { dg-options "-fcilkplus" } */
>
> void foo() {}
>
> #define ALEN 1024
>
> int main(int argc, char* argv[])
> {
>    typedef void (*f) (void *);
>    f b[ALEN], c[ALEN][ALEN];
>    (b[:]) ((void *)c[:][:]);
>    _Cilk_spawn foo();
>    return 0;
> }
>
> But this patch was intended to only fix the testsuite fallout that
> patch 3 would have otherwise caused, and not to e.g. fix all the bugs
> with find_rank.
>
> (BTW patch 3 also makes this test case trigger an ICE, instead of
> being silently miscompiled.)
Can you please include this test (xfailed) when you commit patch #1.  I 
think you want the test to scan for error_mark_node in the gimplified dump.

Jeff

Re: [PATCH 2/3] Avoid creating an initializer for a flexible array member

[Patrick Palka]

On Sun, Jan 3, 2016 at 3:14 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 12/31/2015 08:40 AM, Patrick Palka wrote:
>>
>> If we do create such an initializer, we end up with an error_mark_node
>> during gimplification, because in cp-gimplify.c we pass this
>> VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
>> it spits on an error_mark_node.  This happens in e.g. the test case
>> g++.dg/ext/array1.C.
>>
>> This patch makes it so that we avoid generating an initializer for a
>> flexible array member, thus avoiding to end up with an error_mark_node
>> during gimplification.  The same kind of thing is done ~90 lines down
>> from the code I changed.
>
>
> I don't think this change is correct (or complete).  We could
> decide that the kind of code it's meant to allow should be
> accepted in general but if we did, this change alone would not
> be sufficient.
>
> In the way of background, the existing special treatment for
> flexible array members was added to prevent rejecting cases
> like this one:
>
>   struct A { A (int); };
>   struct B {
>       B ();
>       int n;
>       A a[];
>   };
>
> Since B's ctor above doesn't initialize the flexible array
> member it's well-formed.  Ditto if B's ctor is defined but
> avoids initializing B::a.
>
> The proposed change has the effect of also accepting the
> following modified version of the example above that is
> currently rejected:
>
>   struct A { A (int); };
>   struct B {
>       B (): n (), a () { }
>       int n;
>       A a[];
>   };
>
> In this modified example, B's ctor attempts to default-initialize
> the flexible array member and its elements using their respective
> default ctors even though no such ctor for the latter exists.
>
> Since C++ flexible array members are a GCC extension whose C++
> specific aspects are essentially undocumented it's open to
> discussion whether or not code like this should be accepted.
> Clang rejects it, but one could make the argument that since
> the flexible array member has no elements, default initializing
> it should be allowed even if the same initialization would be
> ill-formed if the array wasn't empty.
>
> If the code should be accepted as written above then it should
> also be accepted if B's ctor were omitted or defaulted. I.e.,
> the following should be treated the same as the above:
>
>   struct A { A (int); };
>   struct B {
>       // ditto with B() = default;
>       int n;
>       A a[];
>   } b;
>
> The patch doesn't handle either of these cases and the code
> is rejected both with and without it.

Good catch.  I had not intended to change the validity of certain uses
of flexible array members with this patch.

So at least for now it seems that we should not avoid building a
VEC_INIT_EXPR of a flexible array member, since calling
build_vec_init_expr is necessary to diagnose some invalid uses of
flexible array members. So, I see three possible solutions to avoid
propagating an error_mark_node to the gimplifier while preserving the
exact behavior of flexible array members:

Continue to unconditionally call_build_vec_init_expr() in
perform_member_init(), and

1. Don't add the resulting VEC_INIT_EXPR to the statement tree if the
array in question has no upper bound (i.e. is a flexible array
member); or
2. Modify build_vec_init() to return an empty statement instead of
returning error_mark_node, if the array to be initialized has no upper
bound (and its initializer is NULL_TREE); or
3. Modify cp_gimplify_expr() to avoid calling build_vec_init() if the
VEC_INIT_EXPR in question is for an array with no upper bound (and its
initializer is NULL_TREE).

Which approach is best? The third one is the most conservative and the
one least likely to cause inadvertent changes in behavior elsewhere, I
think.

(In the end however, this "bug" is mostly harmless, and a patch fixing
it is really only useful in combination with patch #3/3 to avoid a
spurious ICE-on-valid.)

Re: [PATCH 3/3] [RFC] Treat a gimplification failure as an internal error

[Patrick Palka]

On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> This patch makes it so that a gimplification failure is considered to be
> an internal error under normal circumstances, so that we otherwise avoid
> silently generating wrong code if e.g. a buggy frontend fed us a
> malformed tree.
>
> The rationale for this change is that it's better to abort compilation
> than to silently generate wrong code.  During gimplification we should
> only see e.g. an error_mark_node if the frontend has already issued an
> error.  Otherwise it is likely a bug in frontend.
>
> This patch, for example, turns the PR c++/68948 wrong-code bug into an
> ICE on invalid bug.  During testing it also caught two latent "bugs"
> (patches 1 and 2 in this series).
>
> This series was tested on x86_64-pc-linux-gnu, with --enable-languages=all,ada,go,
> no new regressions.
>
> Does this seem like a reasonable invariant to add to the gimplifier?
>
> gcc/cp/ChangeLog:
>
>         * cp-gimplify.c (gimplify_expr_stmt): Don't convert an
>         error_mark_node to an empty statement.
>
> gcc/ChangeLog:
>
>         * gimplify.c (gimplify_return_expr): Remove a redundant test
>         for error_mark_node.
>         (gimplify_decl_expr): Return GS_ERROR if an initializer is an
>         error_mark_node.
>         (gimplify_expr): Treat a gimplification failure as an internal
>         error.  Remove now-redundant GIMPLE_CHECKING checking code.
> ---
>  gcc/cp/cp-gimplify.c |  5 +----
>  gcc/gimplify.c       | 27 +++++++++++++--------------
>  2 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
> index 373c9e1..2b0aaaf 100644
> --- a/gcc/cp/cp-gimplify.c
> +++ b/gcc/cp/cp-gimplify.c
> @@ -424,16 +424,13 @@ gimplify_expr_stmt (tree *stmt_p)
>  {
>    tree stmt = EXPR_STMT_EXPR (*stmt_p);
>
> -  if (stmt == error_mark_node)
> -    stmt = NULL;
> -
>    /* Gimplification of a statement expression will nullify the
>       statement if all its side effects are moved to *PRE_P and *POST_P.
>
>       In this case we will not want to emit the gimplified statement.
>       However, we may still want to emit a warning, so we do that before
>       gimplification.  */
> -  if (stmt && warn_unused_value)
> +  if (stmt && stmt != error_mark_node && warn_unused_value)
>      {
>        if (!TREE_SIDE_EFFECTS (stmt))
>         {
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index bc90401..9a1d723 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1288,8 +1288,7 @@ gimplify_return_expr (tree stmt, gimple_seq *pre_p)
>      }
>
>    if (!ret_expr
> -      || TREE_CODE (ret_expr) == RESULT_DECL
> -      || ret_expr == error_mark_node)
> +      || TREE_CODE (ret_expr) == RESULT_DECL)
>      {
>        greturn *ret = gimple_build_return (ret_expr);
>        gimple_set_no_warning (ret, TREE_NO_WARNING (stmt));
> @@ -1449,6 +1448,9 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>      {
>        tree init = DECL_INITIAL (decl);
>
> +      if (init == error_mark_node)
> +       return GS_ERROR;
> +
>        if (TREE_CODE (DECL_SIZE_UNIT (decl)) != INTEGER_CST
>           || (!TREE_STATIC (decl)
>               && flag_stack_check == GENERIC_STACK_CHECK
> @@ -1464,7 +1466,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>           && DECL_ARTIFICIAL (decl) && DECL_NAME (decl) == NULL_TREE)
>         gimple_add_tmp_var (decl);
>
> -      if (init && init != error_mark_node)
> +      if (init)
>         {
>           if (!TREE_STATIC (decl))
>             {
> @@ -11036,17 +11038,6 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>      }
>    else
>      {
> -#ifdef ENABLE_GIMPLE_CHECKING
> -      if (!(fallback & fb_mayfail))
> -       {
> -         fprintf (stderr, "gimplification failed:\n");
> -         print_generic_expr (stderr, *expr_p, 0);
> -         debug_tree (*expr_p);
> -         internal_error ("gimplification failed");
> -       }
> -#endif
> -      gcc_assert (fallback & fb_mayfail);
> -
>        /* If this is an asm statement, and the user asked for the
>          impossible, don't die.  Fail and let gimplify_asm_expr
>          issue an error.  */
> @@ -11064,6 +11055,14 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>      }
>
>   out:
> +  /* If gimplification failed, then either such failure was explicitly permitted
> +     (via the FB_MAYFAIL flag) or the frontend fed us a malformed tree, e.g. one
> +     containing an ERROR_MARK node -- for which the FE should have already issued an
> +     error diagnostic.  Otherwise it is likely that an FE bug was triggered, in
> +     which case it is better to abort than to risk silently generating wrong
> +     code.  */
> +  if (ret == GS_ERROR)
> +    gcc_assert ((fallback & fb_mayfail) || seen_error ());
>    input_location = saved_location;
>    return ret;
>  }
> --
> 2.7.0.rc3.56.g64157c6.dirty
>

Ping.

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Patrick Palka]

On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote:
> On 01/02/2016 04:26 PM, Patrick Palka wrote:
>>
>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>>      * cp-array-notation.c (cp_expand_cond_array_notations): Return
>>>>>      error_mark_node only if find_rank failed, not if it was
>>>>>      successful.
>>>>
>>>> Can you use -fdump-tree-original in the testcase and verify there's no
>>>> <<<
>>>> error >>> expressions in the resulting dump file?
>>>>
>>>> With that change, this is OK.
>>>
>>>
>>> I think the patch is incomplete.  Because, find_rank does not always emit
>>> an error if it returns false, so we again have cases where we can get
>>> error_mark_node in the code without error being emitted.
>>>        else if (*rank != current_rank)
>>>          {
>>>            /* In this case, find rank is being recursed through a set of
>>>               expression of the form A <OPERATION> B, where A and B both
>>> have
>>>               array notations in them and the rank of A is not equal to
>>> rank of B.
>>>               A simple example of such case is the following: X[:] +
>>> Y[:][:] */
>>>            *rank = current_rank;
>>>            return false;
>>>          }
>>> and other spots.  E.g.
>>>                    if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>>>                      error_at (EXPR_LOCATION (prev_arg),
>>>                                "rank mismatch between %qE and %qE",
>>> prev_arg,
>>>                                TREE_OPERAND (expr, ii));
>>> looks very suspicious.
>>
>>
>> Hmm, good point. Here's a contrived test case that causes find_rank to
>> return false without emitting an error message thus we again end up
>> with an error_mark_node in the gimplifier:
>>
>> /* { dg-do compile } */
>> /* { dg-options "-fcilkplus" } */
>>
>> void foo() {}
>>
>> #define ALEN 1024
>>
>> int main(int argc, char* argv[])
>> {
>>    typedef void (*f) (void *);
>>    f b[ALEN], c[ALEN][ALEN];
>>    (b[:]) ((void *)c[:][:]);
>>    _Cilk_spawn foo();
>>    return 0;
>> }
>>
>> But this patch was intended to only fix the testsuite fallout that
>> patch 3 would have otherwise caused, and not to e.g. fix all the bugs
>> with find_rank.
>>
>> (BTW patch 3 also makes this test case trigger an ICE, instead of
>> being silently miscompiled.)
>
> Can you please include this test (xfailed) when you commit patch #1.  I
> think you want the test to scan for error_mark_node in the gimplified dump.

There are four subdirectories under
gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which
directory should this new xfailed test go?

>
> Jeff
>

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Jeff Law]

On 01/10/2016 08:55 PM, Patrick Palka wrote:
> On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote:
>> On 01/02/2016 04:26 PM, Patrick Palka wrote:
>>>
>>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>
>>>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>>>>>>
>>>>>> gcc/cp/ChangeLog:
>>>>>>
>>>>>>       * cp-array-notation.c (cp_expand_cond_array_notations): Return
>>>>>>       error_mark_node only if find_rank failed, not if it was
>>>>>>       successful.
>>>>>
>>>>> Can you use -fdump-tree-original in the testcase and verify there's no
>>>>> <<<
>>>>> error >>> expressions in the resulting dump file?
>>>>>
>>>>> With that change, this is OK.
>>>>
>>>>
>>>> I think the patch is incomplete.  Because, find_rank does not always emit
>>>> an error if it returns false, so we again have cases where we can get
>>>> error_mark_node in the code without error being emitted.
>>>>         else if (*rank != current_rank)
>>>>           {
>>>>             /* In this case, find rank is being recursed through a set of
>>>>                expression of the form A <OPERATION> B, where A and B both
>>>> have
>>>>                array notations in them and the rank of A is not equal to
>>>> rank of B.
>>>>                A simple example of such case is the following: X[:] +
>>>> Y[:][:] */
>>>>             *rank = current_rank;
>>>>             return false;
>>>>           }
>>>> and other spots.  E.g.
>>>>                     if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>>>>                       error_at (EXPR_LOCATION (prev_arg),
>>>>                                 "rank mismatch between %qE and %qE",
>>>> prev_arg,
>>>>                                 TREE_OPERAND (expr, ii));
>>>> looks very suspicious.
>>>
>>>
>>> Hmm, good point. Here's a contrived test case that causes find_rank to
>>> return false without emitting an error message thus we again end up
>>> with an error_mark_node in the gimplifier:
>>>
>>> /* { dg-do compile } */
>>> /* { dg-options "-fcilkplus" } */
>>>
>>> void foo() {}
>>>
>>> #define ALEN 1024
>>>
>>> int main(int argc, char* argv[])
>>> {
>>>     typedef void (*f) (void *);
>>>     f b[ALEN], c[ALEN][ALEN];
>>>     (b[:]) ((void *)c[:][:]);
>>>     _Cilk_spawn foo();
>>>     return 0;
>>> }
>>>
>>> But this patch was intended to only fix the testsuite fallout that
>>> patch 3 would have otherwise caused, and not to e.g. fix all the bugs
>>> with find_rank.
>>>
>>> (BTW patch 3 also makes this test case trigger an ICE, instead of
>>> being silently miscompiled.)
>>
>> Can you please include this test (xfailed) when you commit patch #1.  I
>> think you want the test to scan for error_mark_node in the gimplified dump.
>
> There are four subdirectories under
> gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which
> directory should this new xfailed test go?
These are for array notation, so AN seems best.

Jeff

Re: [PATCH 3/3] [RFC] Treat a gimplification failure as an internal error

[Jeff Law]

On 01/10/2016 08:20 PM, Patrick Palka wrote:
> On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> This patch makes it so that a gimplification failure is considered to be
>> an internal error under normal circumstances, so that we otherwise avoid
>> silently generating wrong code if e.g. a buggy frontend fed us a
>> malformed tree.
>>
>> The rationale for this change is that it's better to abort compilation
>> than to silently generate wrong code.  During gimplification we should
>> only see e.g. an error_mark_node if the frontend has already issued an
>> error.  Otherwise it is likely a bug in frontend.
>>
>> This patch, for example, turns the PR c++/68948 wrong-code bug into an
>> ICE on invalid bug.  During testing it also caught two latent "bugs"
>> (patches 1 and 2 in this series).
>>
>> This series was tested on x86_64-pc-linux-gnu, with --enable-languages=all,ada,go,
>> no new regressions.
>>
>> Does this seem like a reasonable invariant to add to the gimplifier?
>>
>> gcc/cp/ChangeLog:
>>
>>          * cp-gimplify.c (gimplify_expr_stmt): Don't convert an
>>          error_mark_node to an empty statement.
So this passes any such error_mark_nodes through to the gimplifier, 
which will give us a nice error.  Right?

>>
>> gcc/ChangeLog:
>>
>>          * gimplify.c (gimplify_return_expr): Remove a redundant test
>>          for error_mark_node.
>>          (gimplify_decl_expr): Return GS_ERROR if an initializer is an
>>          error_mark_node.
>>          (gimplify_expr): Treat a gimplification failure as an internal
>>          error.  Remove now-redundant GIMPLE_CHECKING checking code.
I'd generally be in favor of a change like this; I don't offhand recall 
any rationale behind allowing gimplification to continue after hitting 
an error.

My worry is that we're potentially opening ourselves up to a slew of 
ICEs as the gimplifier as a whole I don't think has been audited to 
ensure that it handles error_mark_node is a sane fashion.

So I'd tend to want to wait for the next stage1.

jeff

Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion

[Patrick Palka]

On Thu, 14 Jan 2016, Jeff Law wrote:

> On 01/10/2016 08:55 PM, Patrick Palka wrote:
>> On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote:
>>> On 01/02/2016 04:26 PM, Patrick Palka wrote:
>>>> 
>>>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>>>>>>> 
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>>       * cp-array-notation.c (cp_expand_cond_array_notations): Return
>>>>>>>       error_mark_node only if find_rank failed, not if it was
>>>>>>>       successful.
>>>>>> 
>>>>>> Can you use -fdump-tree-original in the testcase and verify there's no
>>>>>> <<<
>>>>>> error >>> expressions in the resulting dump file?
>>>>>> 
>>>>>> With that change, this is OK.
>>>>> 
>>>>> 
>>>>> I think the patch is incomplete.  Because, find_rank does not always 
>>>>> emit
>>>>> an error if it returns false, so we again have cases where we can get
>>>>> error_mark_node in the code without error being emitted.
>>>>>         else if (*rank != current_rank)
>>>>>           {
>>>>>             /* In this case, find rank is being recursed through a set 
>>>>> of
>>>>>                expression of the form A <OPERATION> B, where A and B 
>>>>> both
>>>>> have
>>>>>                array notations in them and the rank of A is not equal to
>>>>> rank of B.
>>>>>                A simple example of such case is the following: X[:] +
>>>>> Y[:][:] */
>>>>>             *rank = current_rank;
>>>>>             return false;
>>>>>           }
>>>>> and other spots.  E.g.
>>>>>                     if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>>>>>                       error_at (EXPR_LOCATION (prev_arg),
>>>>>                                 "rank mismatch between %qE and %qE",
>>>>> prev_arg,
>>>>>                                 TREE_OPERAND (expr, ii));
>>>>> looks very suspicious.
>>>> 
>>>> 
>>>> Hmm, good point. Here's a contrived test case that causes find_rank to
>>>> return false without emitting an error message thus we again end up
>>>> with an error_mark_node in the gimplifier:
>>>> 
>>>> /* { dg-do compile } */
>>>> /* { dg-options "-fcilkplus" } */
>>>> 
>>>> void foo() {}
>>>> 
>>>> #define ALEN 1024
>>>> 
>>>> int main(int argc, char* argv[])
>>>> {
>>>>     typedef void (*f) (void *);
>>>>     f b[ALEN], c[ALEN][ALEN];
>>>>     (b[:]) ((void *)c[:][:]);
>>>>     _Cilk_spawn foo();
>>>>     return 0;
>>>> }
>>>> 
>>>> But this patch was intended to only fix the testsuite fallout that
>>>> patch 3 would have otherwise caused, and not to e.g. fix all the bugs
>>>> with find_rank.
>>>> 
>>>> (BTW patch 3 also makes this test case trigger an ICE, instead of
>>>> being silently miscompiled.)
>>> 
>>> Can you please include this test (xfailed) when you commit patch #1.  I
>>> think you want the test to scan for error_mark_node in the gimplified 
>>> dump.
>> 
>> There are four subdirectories under
>> gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which
>> directory should this new xfailed test go?
> These are for array notation, so AN seems best.
>
> Jeff
>

Thanks, this is what I'm going to commit some time tomorrow (assuming no
objections).

I have written the new xfail'd test case to expect that a
"rank mismatch" error ought to have been issued, which I hope is the
correct expectation.

gcc/cp/ChangeLog:

 	* cp-array-notation.c (cp_expand_cond_array_notations): Return
 	error_mark_node only if find_rank failed, not if it was
 	successful.

gcc/testsuite/ChangeLog:

 	* c-c++-common/cilk-plus/AN/an-if.c: Check that the original
 	dump does not contain an error_mark_node.
 	* c-c++-common/cilk-plus/CK/pr60469.c: Likewise.
 	* c-c++-common/cilk-plus/AN/fn_ptr-2.c: New xfail'd test.
---
  gcc/cp/cp-array-notation.c                         |  4 ++--
  gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c    |  5 ++++-
  gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c | 14 ++++++++++++++
  gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c  |  5 ++++-
  4 files changed, 24 insertions(+), 4 deletions(-)
  create mode 100644 gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c

diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
index f7a4598..4687ced 100644
--- a/gcc/cp/cp-array-notation.c
+++ b/gcc/cp/cp-array-notation.c
@@ -807,8 +807,8 @@ cp_expand_cond_array_notations (tree orig_stmt)
        if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank)
  	  || !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true,
  			 &yes_rank)
-	  || find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
-			&no_rank))
+	  || !find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
+			 &no_rank))
  	return error_mark_node;
        /* If the condition has a zero rank, then handle array notations in body
  	 separately.  */
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
index 4bf85b5..4ac46ab 100644
--- a/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
@@ -1,5 +1,5 @@
  /* { dg-do run } */
-/* { dg-options "-fcilkplus" } */
+/* { dg-options "-fcilkplus -fdump-tree-original" } */

  #if HAVE_IO
  #include <stdio.h>
@@ -46,3 +46,6 @@ int main() {
      }
      return 0;
  }
+
+/* The C++ FE once emitted a bogus error_mark_node for this test case.  */
+/* { dg-final { scan-tree-dump-not "<<< error >>>" "original" } } */
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c
new file mode 100644
index 0000000..4e1990f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+typedef void (*f) (void *);
+f b[1024];
+void *c[1024][1024];
+
+int
+main (void)
+{
+  (b[:]) (c[:][:]); /* { dg-error "rank mismatch" "" { xfail *-*-* } } */
+  return 0;
+}
+
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
index ca0cf7f..670df17 100644
--- a/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
+++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
@@ -1,6 +1,6 @@
  /* PR middle-end/60469 */
  /* { dg-do compile } */
-/* { dg-options "-fcilkplus" } */
+/* { dg-options "-fcilkplus -fdump-tree-original" } */

  void foo() {}

@@ -13,3 +13,6 @@ int main(int argc, char* argv[])
    _Cilk_spawn foo();
    return 0;
  }
+
+/* The C++ FE once emitted a bogus error_mark_node for this test case.  */
+/* { dg-final { scan-tree-dump-not "<<< error >>>" "original" } } */
-- 
2.7.0.83.gdfccd77.dirty

Re: [PATCH 3/3] [RFC] Treat a gimplification failure as an internal error

[Patrick Palka]

On Thu, Jan 14, 2016 at 4:31 PM, Jeff Law <law@redhat.com> wrote:
> On 01/10/2016 08:20 PM, Patrick Palka wrote:
>>
>> On Thu, Dec 31, 2015 at 10:40 AM, Patrick Palka <patrick@parcs.ath.cx>
>> wrote:
>>>
>>> This patch makes it so that a gimplification failure is considered to be
>>> an internal error under normal circumstances, so that we otherwise avoid
>>> silently generating wrong code if e.g. a buggy frontend fed us a
>>> malformed tree.
>>>
>>> The rationale for this change is that it's better to abort compilation
>>> than to silently generate wrong code.  During gimplification we should
>>> only see e.g. an error_mark_node if the frontend has already issued an
>>> error.  Otherwise it is likely a bug in frontend.
>>>
>>> This patch, for example, turns the PR c++/68948 wrong-code bug into an
>>> ICE on invalid bug.  During testing it also caught two latent "bugs"
>>> (patches 1 and 2 in this series).
>>>
>>> This series was tested on x86_64-pc-linux-gnu, with
>>> --enable-languages=all,ada,go,
>>> no new regressions.
>>>
>>> Does this seem like a reasonable invariant to add to the gimplifier?
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>          * cp-gimplify.c (gimplify_expr_stmt): Don't convert an
>>>          error_mark_node to an empty statement.
>
> So this passes any such error_mark_nodes through to the gimplifier, which
> will give us a nice error.  Right?

(Sorry for the late reply..)

Yes, this change to gimplify_expr_stmt() and the change made to
gimplfy_decl_expr() are to make sure that we propagate any relevant
internal error_mark_nodes to gimplify_expr(), which will trigger the
assertion therein.  This one is particular is pretty important since
the C++ FE seems to make a lot of EXPR_STMTs.

>
>>>
>>> gcc/ChangeLog:
>>>
>>>          * gimplify.c (gimplify_return_expr): Remove a redundant test
>>>          for error_mark_node.

This one is just a simplification -- earlier in the function we have
already tested for error_mark_node.

>>>          (gimplify_decl_expr): Return GS_ERROR if an initializer is an
>>>          error_mark_node.
>>>          (gimplify_expr): Treat a gimplification failure as an internal
>>>          error.  Remove now-redundant GIMPLE_CHECKING checking code.
>
> I'd generally be in favor of a change like this; I don't offhand recall any
> rationale behind allowing gimplification to continue after hitting an error.
>
> My worry is that we're potentially opening ourselves up to a slew of ICEs as
> the gimplifier as a whole I don't think has been audited to ensure that it
> handles error_mark_node is a sane fashion.
>
> So I'd tend to want to wait for the next stage1.

No problem, I will make sure to ping this patch then.