Re: [rfa:ppc64] Fix 64-bit PPC ELF function calls

[Kevin Buettner <kevinb at redhat dot com>]

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.  Don't forget to do it again
after you commit your ppc64_sysv_abi_push_dummy_call() changes.

> > Also, a minor nit: in the comment...
> > 
> >   /* Find a value for the TOC register.  Every symbol should have both
> >      ".FN" and "FN" in the minimal symbol table.  "FN" points at the
> >      F's descriptor, while ".FN" points at the entry point (which
> >      matches FUNC_ADDR).  Need to reverse from FUNC_ADDR back to the
> >      FN's descriptor address.  */
> > 
> > ...at the beginning of the third line down, shouldn't that be:
> > 
> >      FN's descriptor, [...]
> > 
> > If not, what does `F' refer to?
> 
> It's a tipo, thanks.  The term "FN" is used in the ABI.

Here's another typo:

+      /* Compute the actual stack space requirements.  48 is lifted
+         direct from the ABI, it is ment to hold holds things like the

s/ment/meant/

And another:

+		/* WARNING: cagney/2003-09-21: Strictly speaking, this
+                   isn't necessary, unfortunatly, GCC appears to get
+                   struct parameter passing wrong putting odd sized
+                   structs in memory instead of a register.  Work
+                   around this by always writing the value to memory.
+                   Fortunatly, doing this simplifies the cost.  */

s/Fortunatly/Fortunately/

> ok?

Not yet.

Please add

  gdb_assert (tdep->wordsize == 8)

somewhere early in ppc64_sysv_abi_push_dummy_call().

I don't mind seeing tdep->wordsize used in the calls to align_up(),
align_down(), etc.  because this makes it easier to reuse this code
(in a copy/paste sense) in the future, but it is a 64-bit ABI and
asserting that the wordsize is 8 bytes at some point makes it easier
to verify that this function is correct without having to do a
non-local analysis to figure out the possible values of
tdep->wordsize.

....

+  /* The address of the top of the parameter save region (typically
+     points at the local variable region).  On entry, the SP is
+     pointing at this.  */
+  const CORE_ADDR param_top = sp;

I find this name confusing.  If I understand the rest of the code
correctly, this actually ends up being the bottom of the parameter
save area - since the stack grows from high to low addresses.  (Of
course, if you draw a picture, it might be topmost in the picture...)

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?

....

+  /* 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?

....

+      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?

       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.

....

+		  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.

Kevin