This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PING^2][PATCH] PR middle-end/29683 detect stack collision of split args


This PR (29683) causes bad code to be generated on targets that allow an
argument to be split between the stack and registers on targets which
accumulate outgoing args.  I just verified that it still fails on latest
mainline.  This is the third time I've submitted for review -- is there
a middle-end maintainer willing to have a look?

Thanks -

Josh

:ADDPATCH middle-end:

Josh Conner wrote:
> Ping...
> 
> Josh Conner wrote:
>> In PR29683, arguments that are split between the stack and registers can
>> corrupt other arguments.  In the example from the PR, we have a tree
>> stmt like:
>>
>>   VerifyValues (filler, 0, a$mbr1, GetConst (filler, a));
>>
>> Where the third argument to VerifyValues (a) is on the stack, and the
>> second argument to GetConst (a$mbr1) is split between the stack and
>> registers.  expand_call processes a$mbr1 before the call to GetConst,
>> and saves it onto the stack.  Then, when evaluating the call to
>> GetConst, there is logic in store_one_arg that should detect that we
>> need to overwrite the same stack location and save it aside:
>>
>>   if (ACCUMULATE_OUTGOING_ARGS && !(flags & ECF_SIBCALL))
>>     {
>>       /* If this is being stored into a pre-allocated, fixed-size, \
>> 	 stack area,
>>          save any previous data at that location.  */
>>       if (argblock && ! variable_size && arg->stack)
>>
>>       ...
>>
>> Unfortunately, this logic doesn't trigger for split arguments, since
>> arg->stack is NULL.
>>
>> The attached patch addresses this by calculating arg->stack and
>> arg->stack_slot for split (partial) arguments, as well.  By doing this,
>> we enable store_one_arg to detect overlaps, and to save off the
>> conflicting locations as needed.  One slight modification is needed to
>> store_one_arg, so that it only saves off the size of the portion on the
>> stack, not the size of the whole argument.  Interestingly enough, the
>> other places where the save_area is manipulated were already using the
>> correct value (locate_size.constant, not expr_size (tree_value)).
>>
>> I bootstrapped and verified no regressions on powerpc-apple-darwin8.8.0
>> (--enable-languages=c,c++,objc,obj-c++ --disable-multilib).  I also
>> verified on i686-pc-linux-gnu, although this platform is not susceptible
>> to this failure.  Finally, I also verified on sh-none-elf, hoping I
>> could reproduce this on a second platform, only to realize it didn't
>> accumulate outgoing args.  None the less, it passed all regression tests
>> as well.
>>
>> Note that we should probably consider disabling TER-ing a function call
>> into a function call, as setting up a stack argument just to save it
>> aside is pretty inefficient.  I have a second patch for that, but I
>> thought it was important to fix the expand_call failure first.
>>
>> OK for mainline?  Should this be considered for 4.2/4.1, as well?
>>
>> - Josh
>>
>> 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.
>>
>> ------------------------------------------------------------------------
>>
>> Index: gcc/calls.c
>> ===================================================================
>> --- gcc/calls.c	(revision 118072)
>> +++ gcc/calls.c	(working copy)
>> @@ -1357,7 +1357,8 @@ compute_argument_addresses (struct arg_d
>>  	  unsigned int align, boundary;
>>  
>>  	  /* 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;
>>  
>>  	  if (GET_CODE (offset) == CONST_INT)
>> @@ -1366,9 +1367,27 @@ compute_argument_addresses (struct arg_d
>>  	    addr = gen_rtx_PLUS (Pmode, arg_reg, offset);
>>  
>>  	  addr = plus_constant (addr, arg_offset);
>> -	  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.
>> +	       */
>> +	      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 = 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));
>> +	    }
>> +	  else
>> +	    {
>> +	      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);
>> +	    }
>>  	  set_mem_align (args[i].stack_slot, args[i].locate.boundary);
>>  
>>  	  /* Function incoming arguments may overlap with sibling call
>> @@ -4057,7 +4094,7 @@ store_one_arg (struct arg_data *arg, rtx
>>  		  arg->save_area = assign_temp (nt, 0, 1, 1);
>>  		  preserve_temp_slots (arg->save_area);
>>  		  emit_block_move (validize_mem (arg->save_area), stack_area,
>> -				   expr_size (arg->tree_value),
>> +				   GEN_INT (arg->locate.size.constant),
>>  				   BLOCK_OP_CALL_PARM);
>>  		}
>>  	      else
>>
>> ------------------------------------------------------------------------
>>
>> /* { dg-do run } */
>> /* { dg-options "-Os -fno-inline-functions" } */
>>
>> void abort (void);
>>
>> typedef struct {
>>   int x[7];
>> } agg7;
>>
>> typedef struct {
>>   int mbr1;
>>   int mbr2;
>> } agg2;
>>
>> int expected = 31415;
>> agg7 filler;
>>
>> int GetConst (agg7 filler, agg2 split)
>> {
>>   return expected;
>> }
>>
>> void VerifyValues (agg7 filler, int last_reg, int first_stack, int second_stack)
>> {
>>   if (first_stack != 123 || second_stack != expected)
>>     abort ();
>> }
>>
>> void RunTest (agg2 a)
>> {
>>   int result;
>> 	
>>   result = GetConst (filler, a);
>>   VerifyValues (filler, 0, a.mbr1, result);
>> }
>>
>> int main(void)
>> {
>>   agg2 result = {123, 456};
>>   RunTest (result);
>>   return 0;
>> }
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]