fix cross build

fix cross build

[Nathan Sidwell]

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

In building a ppc cross compiler using a freshly built native compiler, I 
encountered an ICE in iterative_hash_expr compiling c-lex.c.  I extracted the 
attached testcase, showing the problem is with statement expressions. 
Investigation showed I_H_E seeing BLOCK and BIND_EXPR nodes, which is was 
unprepared for.  These two nodes are never considered equal by operand_equal_p, 
so we don't need to look into them further to refine the hash.

I'm not sure why a native i686-pc-linux-gnu bootstrap doesn't encounter this 
problem.

The attached patch resolves the ICE.  built and tested on i686-pc-linux-gnu, ok?

nathan

[-- Attachment #2: tree-ice.patch --]
[-- Type: text/x-patch, Size: 1281 bytes --]

2012-05-20  Nathan Sidwell  <nathan@acm.org>

	* tree.c (iterative_hash_expr): Add BLOCK and BIND_EXPR cases.

	* gcc.dg/stmt-expr-4.c: New.

Index: tree.c
===================================================================
--- tree.c	(revision 187628)
+++ tree.c	(working copy)
@@ -6998,6 +6998,11 @@ iterative_hash_expr (const_tree t, hashv
 	  }
 	return val;
       }
+    case BLOCK:
+    case BIND_EXPR:
+      /* These are never equal operands. The contain nodes we're not
+	 prepared for, so stop now.  */
+      return val;
     case MEM_REF:
       {
 	/* The type of the second operand is relevant, except for
Index: testsuite/gcc.dg/stmt-expr-4.c
===================================================================
--- testsuite/gcc.dg/stmt-expr-4.c	(revision 0)
+++ testsuite/gcc.dg/stmt-expr-4.c	(revision 0)
@@ -0,0 +1,22 @@
+
+/* { dg-options "-O2 -std=gnu99" } */
+/* Internal compiler error in iterative_hash_expr */
+
+struct tree_string
+{
+  char str[1];
+};
+
+union tree_node
+{
+  struct tree_string string;
+};
+
+char *Foo (union tree_node * num_string)
+{
+  char *str = ((union {const char * _q; char * _nq;})
+	       ((const char *)(({ __typeof (num_string) const __t
+				     = num_string;  __t; })
+			       ->string.str)))._nq;
+  return str;
+}

Re: fix cross build

[Richard Guenther]

On Sun, May 20, 2012 at 7:24 PM, Nathan Sidwell <nathan@acm.org> wrote:
> In building a ppc cross compiler using a freshly built native compiler, I
> encountered an ICE in iterative_hash_expr compiling c-lex.c.  I extracted
> the attached testcase, showing the problem is with statement expressions.
> Investigation showed I_H_E seeing BLOCK and BIND_EXPR nodes, which is was
> unprepared for.  These two nodes are never considered equal by
> operand_equal_p, so we don't need to look into them further to refine the
> hash.
>
> I'm not sure why a native i686-pc-linux-gnu bootstrap doesn't encounter this
> problem.
>
> The attached patch resolves the ICE.  built and tested on i686-pc-linux-gnu,
> ok?

Hmm - I think this papers over the issue that the CONSTRUCTOR is not
properly gimplified - it still contains a TARGET_EXPR which is not valid GIMPLE.
Why is that TARGET_EXPR not gimplified?

Richard.

> nathan

Re: fix cross build

[Nathan Sidwell]

On 05/21/12 11:03, Richard Guenther wrote:

> Hmm - I think this papers over the issue that the CONSTRUCTOR is not
> properly gimplified - it still contains a TARGET_EXPR which is not valid GIMPLE.
> Why is that TARGET_EXPR not gimplified?

As far as I can make out, it just doesn't look inside the constructor.

0  gimplify_expr (expr_p=0xb7e90394, pre_p=0xbfffe514, post_p=0xbfffd08c,
     gimple_test_f=0x84b0a80 <is_gimple_min_lval(tree_node*)>, fallback=3) at 
../../src/gcc/gimplify.c:7356
#1  0x084f5882 in gimplify_compound_lval (expr_p=0xb7e9cb94, pre_p=0xbfffe514, 
post_p=0xbfffd08c, fallback=1)
     at ../../src/gcc/gimplify.c:2259
#2  0x08519878 in gimplify_expr (expr_p=0xb7e9cb94, pre_p=0xbfffe514, 
post_p=0xbfffd08c,
     gimple_test_f=0x84eaf9f <is_gimple_reg_rhs_or_call(tree)>, fallback=1) at 
../../src/gcc/gimplify.c:7081
#3  0x085087f7 in gimplify_modify_expr (expr_p=0xbfffd6d0, pre_p=0xbfffe514, 
post_p=0xbfffd08c, want_value=false)
     at ../../src/gcc/gimplify.c:4843

The switch case at that stack frame is for CONSTRUCTORs.  It has the comment:
/* Don't reduce this in place; let gimplify_init_constructor work its
	     magic.  Buf if we're just elaborating this for side effects, just
	     gimplify any element that has side-effects.  */

But we never enter G_I_C before the ICE.

On the second time we get here, fallback has the value 1 at that point 
(fb_rvalue), so neither if condition is satisified, and we end up at
	    ret = GS_ALL_DONE;
we then proceed down to enter:
  else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p))
     {
       /* An rvalue will do.  Assign the gimplified expression into a
	 new temporary TMP and replace the original expression with
	 TMP.  First, make sure that the expression has a type so that
	 it can be assigned into a temporary.  */
       gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p)));
       *expr_p = get_formal_tmp_var (*expr_p, pre_p);
     }

and ICE inside GFTV when it attempts to generate the hash.

AFAICT, changing the test case to:
   char *str = ((union {const char * _q; char * _nq;})
	       ((const char *)((num_string)
			       ->string.str)))._nq;
(i.e. removing the stmt-expr) results in the same logical flow as above, but 
there's no TARGET_EXPR inside the constructor.

I'm not sure how it's supposed to work, so I'm a little lost.

Re: fix cross build

[Richard Guenther]

On Tue, May 22, 2012 at 3:24 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 05/21/12 11:03, Richard Guenther wrote:
>
>> Hmm - I think this papers over the issue that the CONSTRUCTOR is not
>> properly gimplified - it still contains a TARGET_EXPR which is not valid
>> GIMPLE.
>> Why is that TARGET_EXPR not gimplified?
>
>
> As far as I can make out, it just doesn't look inside the constructor.
>
> 0  gimplify_expr (expr_p=0xb7e90394, pre_p=0xbfffe514, post_p=0xbfffd08c,
>    gimple_test_f=0x84b0a80 <is_gimple_min_lval(tree_node*)>, fallback=3) at
> ../../src/gcc/gimplify.c:7356
> #1  0x084f5882 in gimplify_compound_lval (expr_p=0xb7e9cb94,
> pre_p=0xbfffe514, post_p=0xbfffd08c, fallback=1)
>    at ../../src/gcc/gimplify.c:2259
> #2  0x08519878 in gimplify_expr (expr_p=0xb7e9cb94, pre_p=0xbfffe514,
> post_p=0xbfffd08c,
>    gimple_test_f=0x84eaf9f <is_gimple_reg_rhs_or_call(tree)>, fallback=1) at
> ../../src/gcc/gimplify.c:7081
> #3  0x085087f7 in gimplify_modify_expr (expr_p=0xbfffd6d0, pre_p=0xbfffe514,
> post_p=0xbfffd08c, want_value=false)
>    at ../../src/gcc/gimplify.c:4843
>
> The switch case at that stack frame is for CONSTRUCTORs.  It has the
> comment:
> /* Don't reduce this in place; let gimplify_init_constructor work its
>             magic.  Buf if we're just elaborating this for side effects,
> just
>             gimplify any element that has side-effects.  */
>
> But we never enter G_I_C before the ICE.
>
> On the second time we get here, fallback has the value 1 at that point
> (fb_rvalue), so neither if condition is satisified, and we end up at
>            ret = GS_ALL_DONE;
> we then proceed down to enter:
>  else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p))
>    {
>      /* An rvalue will do.  Assign the gimplified expression into a
>         new temporary TMP and replace the original expression with
>         TMP.  First, make sure that the expression has a type so that
>         it can be assigned into a temporary.  */
>      gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p)));
>      *expr_p = get_formal_tmp_var (*expr_p, pre_p);
>    }
>
> and ICE inside GFTV when it attempts to generate the hash.
>
> AFAICT, changing the test case to:
>  char *str = ((union {const char * _q; char * _nq;})
>               ((const char *)((num_string)
>                               ->string.str)))._nq;
> (i.e. removing the stmt-expr) results in the same logical flow as above, but
> there's no TARGET_EXPR inside the constructor.
>
> I'm not sure how it's supposed to work, so I'm a little lost.

Hm, ok.  I see what happens.  The issue is that the CONSTRUCTOR has
elements with TREE_SIDE_EFFECTS set but is itself not TREE_SIDE_EFFECTS.
Which would have avoided all this mess in lookup_tmp_var.  I suppose
for duplicating the initializer you could even generate wrong-code with your fix
in(?).  Now, we never set TREE_SIDE_EFFECTS from build_constructor
(but only TREE_CONSTANT), so the fix could either be to fix that or to
exclude CONSTRUCTORs in lookup_tmp_var like

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 187772)
+++ gcc/gimplify.c      (working copy)
@@ -514,7 +514,8 @@ lookup_tmp_var (tree val, bool is_formal
      block, which means it will go into memory, causing much extra
      work in reload and final and poorer code generation, outweighing
      the extra memory allocation here.  */
-  if (!optimize || !is_formal || TREE_SIDE_EFFECTS (val))
+  if (!optimize || !is_formal || TREE_SIDE_EFFECTS (val)
+      || TREE_CODE (val) == CONSTRUCTOR)
     ret = create_tmp_from_val (val);
   else
     {

after all lookup_tmp_var performs a simple-minded CSE here.

But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
properly ...

Richard.

Re: fix cross build

[Nathan Sidwell]

On 05/22/12 15:12, Richard Guenther wrote:

thanks!

> But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
> properly ...

yeah, that would seem to be the error.  Looking ...

Re: fix cross build

[Nathan Sidwell]

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

On 05/22/12 15:12, Richard Guenther wrote:

> But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
> properly ...

the attached patch fixes the ICE and causes no regressions on i686-pc-linux-gnu.

ok?

nathan

[-- Attachment #2: ctor-side.patch --]
[-- Type: text/x-patch, Size: 1747 bytes --]

2012-05-23  Nathan Sidwell  <nathan@acm.org>

	* tree.c (build_constructor): Propagate TREE_SIDE_EFFECTS.

	* gcc.dg/stmt-expr-4.c: New.

Index: tree.c
===================================================================
--- tree.c	(revision 187628)
+++ tree.c	(working copy)
@@ -1416,17 +1416,24 @@ build_constructor (tree type, VEC(constr
   unsigned int i;
   constructor_elt *elt;
   bool constant_p = true;
+  bool side_effects_p = false;
 
   TREE_TYPE (c) = type;
   CONSTRUCTOR_ELTS (c) = vals;
 
   FOR_EACH_VEC_ELT (constructor_elt, vals, i, elt)
-    if (!TREE_CONSTANT (elt->value))
-      {
+    {
+      /* Mostly ctors will have elts that don't have side-effects, so
+	 the usual case is to scan all the elements.  Hence a single
+	 loop for both const and side effects, rather than one loop
+	 each (with early outs).  */
+      if (!TREE_CONSTANT (elt->value))
 	constant_p = false;
-	break;
-      }
+      if (TREE_SIDE_EFFECTS (elt->value))
+	side_effects_p = true;
+    }
 
+  TREE_SIDE_EFFECTS (c) = side_effects_p;
   TREE_CONSTANT (c) = constant_p;
 
   return c;
Index: testsuite/gcc.dg/stmt-expr-4.c
===================================================================
--- testsuite/gcc.dg/stmt-expr-4.c	(revision 0)
+++ testsuite/gcc.dg/stmt-expr-4.c	(revision 0)
@@ -0,0 +1,22 @@
+
+/* { dg-options "-O2 -std=gnu99" } */
+/* Internal compiler error in iterative_hash_expr */
+
+struct tree_string
+{
+  char str[1];
+};
+
+union tree_node
+{
+  struct tree_string string;
+};
+
+char *Foo (union tree_node * num_string)
+{
+  char *str = ((union {const char * _q; char * _nq;})
+	       ((const char *)(({ __typeof (num_string) const __t
+				     = num_string;  __t; })
+			       ->string.str)))._nq;
+  return str;
+}

Re: fix cross build

[Richard Guenther]

On Wed, May 23, 2012 at 7:58 PM, Nathan Sidwell <nathan@acm.org> wrote:
> On 05/22/12 15:12, Richard Guenther wrote:
>
>> But I wonder why CONSTRUCTORs do not inherit TREE_SIDE_EFFECTS
>> properly ...
>
>
> the attached patch fixes the ICE and causes no regressions on
> i686-pc-linux-gnu.
>
> ok?

Looks ok to me.  Though I wonder how we got away with that for so long time ...

What do others prefer?  Keep CONSTRUCTORs "broken" and paper over in
gimplify.c instead?

If you don't hear from somebody else in 24h the patch is ok as-is
(can you do some grepping whether there are callers of build_constructor
that set TREE_SIDE_EFFECTS on it afterwards?)

Thanks,
Richard.

> nathan

Re: fix cross build

[Nathan Sidwell]

On 05/24/12 09:03, Richard Guenther wrote:

> If you don't hear from somebody else in 24h the patch is ok as-is
> (can you do some grepping whether there are callers of build_constructor
> that set TREE_SIDE_EFFECTS on it afterwards?)

I committed the patch.  grepping C & C++ sources didn't show callers of 
build_constructor poking T_S_E themselves.  some of them did clear TREE_CONSTANT 
though.  Rather than chase the rationale for that I decided to let them lie.

nathan