Re: [PATCH] PR middle-end/29683 detect stack collision of split args

[Ian Lance Taylor <iant at google dot com>]

Josh Conner <jconner@apple.com> writes:

> 2006-11-01  Josh Conner  <jconner@apple.com>
> 
> 	PR middle-end/29683
> 	* calls.c (compute_argument_addresses): Set stack and stack_slot
> 	for partial args, too.
> 	(store_one_arg): Use locate.size.constant for the size when
> 	generating a save_area.
> 
> 2006-11-01  Josh Conner  <jconner@apple.com>
> 
> 	PR middle-end/29683
> 	* gcc.dg/pr29683.c: New.

Sorry for the slow review.

>  	  /* Skip this parm if it will not be passed on the stack.  */
> -	  if (! args[i].pass_on_stack && args[i].reg != 0)
> +	  if (! args[i].pass_on_stack && args[i].reg != 0
> +	      && args[i].partial == 0)
>  	    continue;

I think this will be easier to read if you put each clause on a
separate line:

  if (! args[i].pass_on_stack
      && args[i].reg != 0
      && args[i].partial == 0)


> -	  args[i].stack = gen_rtx_MEM (args[i].mode, addr);
> -	  set_mem_attributes (args[i].stack,
> -			      TREE_TYPE (args[i].tree_value), 1);
> +
> +	  if (args[i].partial != 0)
> +	    {
> +	      /* Only part of the parameter is being passed on the stack.
> +		 Generate a simple memory reference of the correct size.
> +	       */

Put the "*/" at the end of the previous line, with two spaces after
the period.

> +	      unsigned int units_on_stack;
> +	      enum machine_mode mode;
> +
> +	      units_on_stack = args[i].locate.size.constant;
> +	      mode = mode_for_size (units_on_stack * BITS_PER_UNIT,
> +				    MODE_INT, 0);

The call to mode_for_size should pass the last parameter as 1 (and the
second one you added should also change, but see below).

> +	      args[i].stack = gen_rtx_MEM (mode, addr);
> +	      set_mem_size (args[i].stack, GEN_INT (units_on_stack));
> +	    }
> +	  else
> +	    {
> +	      args[i].stack = gen_rtx_MEM (args[i].mode, addr);
> +	      set_mem_attributes (args[i].stack,
> +				  TREE_TYPE (args[i].tree_value), 1);
> +	    }
>  	  align = BITS_PER_UNIT;
>  	  boundary = args[i].locate.boundary;
>  	  if (args[i].locate.where_pad != downward)
> @@ -1386,9 +1405,27 @@ compute_argument_addresses (struct arg_d
>  	    addr = gen_rtx_PLUS (Pmode, arg_reg, slot_offset);
>  
>  	  addr = plus_constant (addr, arg_offset);
> -	  args[i].stack_slot = gen_rtx_MEM (args[i].mode, addr);
> -	  set_mem_attributes (args[i].stack_slot,
> -			      TREE_TYPE (args[i].tree_value), 1);
> +
> +	  if (args[i].partial != 0)
> +	    {
> +	      /* Only part of the parameter is being passed on the stack.
> +		 Generate a simple memory reference of the correct size.
> +	       */
> +	      unsigned int units_on_stack;
> +	      enum machine_mode mode;
> +
> +	      units_on_stack = args[i].locate.size.constant;
> +	      mode = mode_for_size (units_on_stack * BITS_PER_UNIT,
> +				    MODE_INT, 0);
> +	      args[i].stack_slot = gen_rtx_MEM (mode, addr);
> +	      set_mem_size (args[i].stack, GEN_INT (units_on_stack));
> +	    }

Here you have a duplicate computation of units_on_stack and mode
within 30 lines.  Please eliminate that duplication.  Either move
units_on_stack and mode one block level higher, or break this out into
a function which returns a MEM to assign to .stack or .stack_slot, as
appropriate.


Patch is OK with those changes.

Thanks.

Ian