Re: PATCH for Re: target/9797: ARM structure initialization bug

[Daniel Jacobowitz <drow at mvista dot com>]

Could someone review this patch?  I believe that this is the same
problem as PR 9853, which is high-priority for GCC 3.3.

On Mon, Feb 24, 2003 at 05:20:25PM -0500, Daniel Jacobowitz wrote:
> On Fri, Feb 21, 2003 at 07:17:07PM -0500, drow at mvista dot com wrote:
> > >Synopsis:       ARM miscompiles C99-style struct initializers
> 
> > >Description:
> > 	Compile the testcase below with no options; it will abort.
> > 	Then, for the really weird part, add only -fstrict-aliasing.  The
> > 	generated code works.
> > 	Similarly, -O2 works, -O2 -fno-strict-aliasing aborts.
> 
> I've been investigating this further.  The problem appears to occur
> here, in expand_expr (expr.c:7040):
> 
>           if (target == 0 || ! safe_from_p (target, exp, 1)
>               || GET_CODE (target) == PARALLEL)
>             target
>               = assign_temp (build_qualified_type (type,
>                                                    (TYPE_QUALS (type)
>                                                     | (TREE_READONLY (exp)
>                                                        *TYPE_QUAL_CONST))),
>                              0, TREE_ADDRESSABLE (exp), 1);
> 
>           store_constructor (exp, target, 0, int_expr_size (exp));
>           return target;
> 
> With strict aliasing off, we get ! safe_from_p (target, exp, 1) (from
> that I constructed a testcase which worked even with strict aliasing
> on).  In any case, we assign the temp a stack slot at this point.  It
> happens to have ->level == 2.
> 
> We start level 2 here:
> #0  push_temp_slots () at /big/fsf/balm-head/gcc/function.c:1256
> #1  0x08140871 in expand_assignment (to=0x40365948, from=0x40022840,
> 	want_value=0, suggest_reg=0)
>     at /big/fsf/balm-head/gcc/expr.c:4247
> 
> We hit the line above here:
> #0  expand_expr (exp=0x40022840, target=0x40346a14, tmode=BLKmode,
> 	modifier=EXPAND_NORMAL)
>     at /big/fsf/balm-head/gcc/expr.c:7048
> #1  0x081414a1 in store_expr (exp=0x40022840, target=0x40346a14,
> 	want_value=0)
>     at /big/fsf/balm-head/gcc/expr.c:4446
> #2  0x08140882 in expand_assignment (to=0x40365948, from=0x40022840,
> 	want_value=0, suggest_reg=0)
>     at /big/fsf/balm-head/gcc/expr.c:4248
> 
> We allocate a temp slot with level 2.  Then, we go on to recurse into
> the individual field initializations.  They've got CONSTRUCTORs in them
> too.  So we end up here:
> #1  0x082b3f15 in expand_decl_init (decl=0x403659b4)
> 	at /big/fsf/balm-head/gcc/stmt.c:4061
> 
> via:
> #10 0x081414a1 in store_expr (exp=0x40022840, target=0x40346a14,
> 	want_value=0)
>     at /big/fsf/balm-head/gcc/expr.c:4446
> #11 0x08140882 in expand_assignment (to=0x40365948, from=0x40022840,
> 	want_value=0, suggest_reg=0)
>     at /big/fsf/balm-head/gcc/expr.c:4248
> 
> The end of expand_decl_init has:
>   /* Free any temporaries we made while initializing the decl.  */
>   preserve_temp_slots (NULL_RTX);
>   free_temp_slots ();
> 
> But nothing's pushed a new temp slot level!  So we free the stack slot
> for the containing object.
> 
> There seems to be a missing push/pop of the temp level.  At a guess
> it's in expand_decl_init.  This patch fixes my problem but I'd really
> appreciate eyes from someone more familiar with this code; I don't know
> if there's really somewhere else that's failing to push/pop.  There are
> seven other places in the compiler (one in c-typeck.c, six in stmt.c)
> that call free_temp_slots without immediately popping a temp slot
> level; I have a suspicion that they are all wrong also.
> 
> Is this patch correct or am I barking up the wrong tree?  Bootstrapped
> and regtested on i686-linux, since ARM won't build newlib at the moment
> (dies in verify_local_live_at_start).
> 
> -- 
> Daniel Jacobowitz
> MontaVista Software                         Debian GNU/Linux Developer
> 
> 2003-02-24  Daniel Jacobowitz  <drow at mvista dot com>
> 
> 	Fix PR target/9797
> 	* stmt.c (expand_decl_init): Call push_temp_slots () and
> 	pop_temp_slots ().
> 
> 2003-02-24  Daniel Jacobowitz  <drow at mvista dot com>
> 
> 	* gcc.c-torture/execute/20030224-2.c: New test.
> 
> Index: stmt.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/stmt.c,v
> retrieving revision 1.290
> diff -u -p -r1.290 stmt.c
> --- stmt.c	15 Feb 2003 21:22:55 -0000	1.290
> +++ stmt.c	24 Feb 2003 19:32:24 -0000
> @@ -4036,6 +4036,8 @@ expand_decl_init (decl)
>  
>    /* Compute and store the initial value now.  */
>  
> +  push_temp_slots ();
> +
>    if (DECL_INITIAL (decl) == error_mark_node)
>      {
>        enum tree_code code = TREE_CODE (TREE_TYPE (decl));
> @@ -4059,6 +4061,7 @@ expand_decl_init (decl)
>    /* Free any temporaries we made while initializing the decl.  */
>    preserve_temp_slots (NULL_RTX);
>    free_temp_slots ();
> +  pop_temp_slots ();
>  }
>  
>  /* CLEANUP is an expression to be executed at exit from this binding contour;
> --- /dev/null	1969-12-31 19:00:00.000000000 -0500
> +++ testsuite/gcc.c-torture/execute/20030224-2.c	2003-02-24 15:05:02.000000000 -0500
> @@ -0,0 +1,28 @@
> +/* Make sure that we don't free any temp stack slots associated with
> +   initializing marker before we're finished with them.  */
> +
> +extern void abort();
> +
> +typedef struct { short v16; } __attribute__((packed)) jint16_t;
> +
> +struct node {
> +  jint16_t magic;
> +  jint16_t nodetype;
> +  int totlen;
> +} __attribute__((packed));
> +
> +struct node node, *node_p = &node;
> +
> +int main()
> +{
> +  struct node marker = {
> +    .magic = (jint16_t) {0x1985},
> +    .nodetype = (jint16_t) {0x2003},
> +    .totlen = node_p->totlen
> +  };
> +  if (marker.magic.v16 != 0x1985)
> +    abort();
> +  if (marker.nodetype.v16 != 0x2003)
> +    abort();
> +  return 0;
> +}
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer