> On Oct 6,  6:19pm, Andrew Cagney wrote:
> 
> 
>> > I notice some 80+ character lines in ppc64_sysv_abi_push_dummy_call().
>> > Could you adjust these so that they're 80 characters or less?
> 
>> 
>> I'll run the file through gdb_indent.sh, as a separate commit.
> 
> 
> I see that you've already done it.

Yes, obvious.

> Don't forget to do it again
> after you commit your ppc64_sysv_abi_push_dummy_call() changes.

The attached has already been re-indented.  Otherwize the diff ends up 
containing unnecessary white space changes.

> Here's another typo:
[...]
> s/ment/meant/
[...]
> s/Fortunatly/Fortunately/

Fixed, along with a few others.

> Not yet.
> 
> Please add
> 
>   gdb_assert (tdep->wordsize == 8)
> 
> somewhere early in ppc64_sysv_abi_push_dummy_call().

Added, along with your comments.

> Anyway, in this instance, I think it's advisable to avoid the names
> "top" and "bottom" altogether.  Perhaps ``param_end'' is a more
> appropriate name?

I replaced that variable with vparam_size and gparam_size.

> +  /* Address, on the stack, of the next general (integer, struct,
> +     float, ...) parameter.  */
> +  CORE_ADDR gparam = 0;
> +  /* Address, on the stack, of the next vector.  */
> +  CORE_ADDR vparam = 0;
> 
> These are addresses when write_pass == 1, but merely a running
> total of the number of words needed when write_pass == 0.  Calling
> them addresses is misleading.
> 
> +  /* Go through the argument list twice.
> +
> +     Pass 1: Figure out how much stack space is required for arguments
> +     and pushed values.
> +
> +     Pass 2: Replay the same computation but this time also write the
> +     values out to the target.  */
> 
> Perhaps you could explain the dual roles of gparam and vparam in the
> above comment?

Yes but slightly further down where they are computed.

> +      if (!write_pass)
> +	{
> +	  vparam = align_down (param_top - vparam, 16);
> +	  gparam = align_down (vparam - gparam, 16);
> +	  sp = align_down (gparam - 48, 16);
> +	}
> 
> The ABI says:
> 
>     The parameter save area, which is located at a fixed offset of 48
>     bytes from the stack pointer, is reserved in each stack frame for use
>     as an argument list.  A minimum of 8 doublewords is always reserved.
> 
> I don't see where you've accounted for this 8 doubleword minimum. 
> Perhaps revise this to something along the following lines?

Good point.

>        if (!write_pass)
>  	{
>  	  vparam = align_down (param_top - vparam, 16);
> 	  if (gparam < 8 * tdep->wordsize)
> 	    gparam = 8 * tdep->wordsize;
>  	  gparam = align_down (vparam - gparam, 16);
>  	  sp = align_down (gparam - 48, 16);
>  	}
> 
> I'm not sure this is right either since that alters the vector save
> area.  I don't happen to have a copy of the Altivec ABI though.

Done (but also rearanged).  The vector save area can be zero in size.

> +		  if (greg <= 10)
> +		    {
> +		      /* It goes into the register, left aligned (as
> +                         far as I can tell).  */
> 
> I can't find anything in the ABI to contradict this, but I found it
> surprising that floating point arguments would be left aligned.  If
> I'm not mistaken (for big endian), this'll mean that the the float
> is stored in the most significant half of the register.

It now reads:

		      /* The ABI states "Single precision floating
		         point values are mapped to the first word in
		         a single doubleword" and "... floating point
		         values mapped to the first eight doublewords
		         of the parameter save area are also passed in
		         general registers").

			 This code interprets that to mean: store it,
			 left aligned, in the general register.  */


Andrew