[RFC] Linux-specific ppc32 ABI

[RFC] Linux-specific ppc32 ABI

[Luis Machado]

Hi,

GDB currently handles stack frame creation when calling inferior
functions for SYSV/ppc32 through the ppc_sysv_abi_push_dummy_call(...)
method. GDB on Linux/ppc32 relies on this function as well since it
doesn't have an explicit implementation in ppc-linux-tdep.c.

The problem is that the implementation for
ppc_sysv_abi_push_dummy_call(...) has a problem that needs to be fixed,
related to the size of float parameters that are meant to be stored in
the stack during parameter passing in a function.

The question is, would it be safe to assume that SYSV would comply with
Linux ABI's especifications for ppc32, so it would only be necessary to
change the SYSV implementation, or would it be a better idea to just
write new Linux-specific code in ppc-linux-tdep.c?

The only difference between these two functions would be the fix for
float parameter passing through the stack and the place they're located:
"ppc-linux-tdep.c" vs "ppc-sysv-tdep.c".

I presume it's better to just write new Linux-specific code in case we
need to treat those ABI's differently in the future.

What do you think?

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Daniel Jacobowitz]

On Thu, Jan 10, 2008 at 05:00:24PM -0200, Luis Machado wrote:
> I presume it's better to just write new Linux-specific code in case we
> need to treat those ABI's differently in the future.

What other target(s) use the existing function?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

On Thu, 2008-01-10 at 14:13 -0500, Daniel Jacobowitz wrote:
> On Thu, Jan 10, 2008 at 05:00:24PM -0200, Luis Machado wrote:
> > I presume it's better to just write new Linux-specific code in case we
> > need to treat those ABI's differently in the future.
> 
> What other target(s) use the existing function?

Since I don't see calls to set_gdbarch_push_dummy_call in any
OS-specific tdep file, I'd say all OSes supported by GDB on PowerPC.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Joel Brobecker]

> The question is, would it be safe to assume that SYSV would comply with
> Linux ABI's especifications for ppc32, so it would only be necessary to
> change the SYSV implementation, or would it be a better idea to just
> write new Linux-specific code in ppc-linux-tdep.c?

Is there a different ABI for Linux on powerpc?

> I presume it's better to just write new Linux-specific code in case we
> need to treat those ABI's differently in the future.

Assuming a difference in ABI does exist:

How about an extra flag inside the gdbarch_tdep structure that
you'll be using inside the push dummy call?  It would be a shame
duplicate this function when most of it is common.

-- 
Joel

Re: [RFC] Linux-specific ppc32 ABI

[Luis Machado]

> Assuming a difference in ABI does exist:

Yes, there are a few differences between what the SYSTEM V ABI says and
what Linux actually does. But most of the Linux specs follow the SYSTEM
V ABI.

> How about an extra flag inside the gdbarch_tdep structure that
> you'll be using inside the push dummy call?  It would be a shame
> duplicate this function when most of it is common.

Yes. This would work, but putting linux specific code inside the
ppc-sysv-tdep.c file doesn't look quite right. But if there isn't a
better way of doing this, should be OK.

Regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Daniel Jacobowitz]

On Fri, Jan 11, 2008 at 01:55:20PM -0200, Luis Machado wrote:
> > How about an extra flag inside the gdbarch_tdep structure that
> > you'll be using inside the push dummy call?  It would be a shame
> > duplicate this function when most of it is common.
> 
> Yes. This would work, but putting linux specific code inside the
> ppc-sysv-tdep.c file doesn't look quite right. But if there isn't a
> better way of doing this, should be OK.

For practical reasons, this is how it is usually done.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Linux-specific ppc32 ABI

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

Hi,

On Fri, 2008-01-11 at 10:57 -0500, Daniel Jacobowitz wrote:
> On Fri, Jan 11, 2008 at 01:55:20PM -0200, Luis Machado wrote:
> > > How about an extra flag inside the gdbarch_tdep structure that
> > > you'll be using inside the push dummy call?  It would be a shame
> > > duplicate this function when most of it is common.
> > 
> > Yes. This would work, but putting linux specific code inside the
> > ppc-sysv-tdep.c file doesn't look quite right. But if there isn't a
> > better way of doing this, should be OK.
> 
> For practical reasons, this is how it is usually done.

Actually, we won't need to go through the flag. I've checked and the
PPC/Linux ABI conforms with the PPC/SysV ABI regarding Calling Sequences
and the Stack layout.

Considering we have a function with a great number of float parameters,
exceeding the number of available FP registers, we need to store those
parameters in the stack space, and then there's a problem with the
ABI/GDB.

The SysV ABI says we should promote exceeding float parameters (the ones
that should go to the stack) to double, and store then with 8 bytes
alignment in the stack.

Trying to call a function like that, from within GDB, made it clear that
some parameters were getting trashed, and this is due to the fact that
GCC (XLC as well) doesn't promote floats to doubles and does not align
them to 8 bytes in the stack. Actually, it just aligns the prototyped
float parameters to 4 bytes in the stack.

This is something that needs to be improved in the ABI text. I've
attached a note next to the portion of code responsible for handling
float parameters.

The patch follows, with a minor change in the ABI handling for PPC32 and
a testcase Thiago wrote to verify the problem.

Comments?

Regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

[-- Attachment #2: ppc32-float-abi.diff --]
[-- Type: text/x-patch, Size: 8420 bytes --]

2008-01-11  Luis Machado  <luisgpm@br.ibm.com>
	    Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* testsuite/gdb.base/callfuncs.c (t_float_many_args): New function.
	(t_double_many_args): New function.
	* testsuite/gdb.base/callfuncs.exp: Add tests for exceeding float
	and double parameters passed through the stack.
	* ppc-sysv-tdep.c: Convert spaces to tabs.
	(ppc_sysv_abi_push_dummy_call): Align floats to	4 bytes in the stack.

Index: gdb/testsuite/gdb.base/callfuncs.c
===================================================================
--- gdb.orig/testsuite/gdb.base/callfuncs.c	2008-01-11 09:59:22.000000000 -0800
+++ gdb/testsuite/gdb.base/callfuncs.c	2008-01-11 09:59:33.000000000 -0800
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright 1993, 1994, 1995, 1998, 1999, 2000, 2001, 2004, 2007, 2008
+   Copyright 1993, 1994, 1995, 1998, 1999, 2000, 2001, 2004, 2007
    Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -46,9 +46,35 @@
 
 float float_val1 = 3.14159;
 float float_val2 = -2.3765;
+float float_val3 = 0.25;
+float float_val4 = 1.25;
+float float_val5 = 2.25;
+float float_val6 = 3.25;
+float float_val7 = 4.25;
+float float_val8 = 5.25;
+float float_val9 = 6.25;
+float float_val10 = 7.25;
+float float_val11 = 8.25;
+float float_val12 = 9.25;
+float float_val13 = 10.25;
+float float_val14 = 11.25;
+float float_val15 = 12.25;
 
 double double_val1 = 45.654;
 double double_val2 = -67.66;
+double double_val3 = 0.25;
+double double_val4 = 1.25;
+double double_val5 = 2.25;
+double double_val6 = 3.25;
+double double_val7 = 4.25;
+double double_val8 = 5.25;
+double double_val9 = 6.25;
+double double_val10 = 7.25;
+double double_val11 = 8.25;
+double double_val12 = 9.25;
+double double_val13 = 10.25;
+double double_val14 = 11.25;
+double double_val15 = 12.25;
 
 #define DELTA (0.001)
 
@@ -298,6 +324,39 @@
 	  && (float_arg2 - float_val2) > -DELTA);
 }
 
+/* This function has many arguments to force some of them to be passed via
+   the stack instead of registers, to test that GDB can construct correctly
+   the parameter save area. Note that Linux/ppc32 has 8 float registers to use
+   for float parameter passing and Linux/ppc64 has 13, so the number of
+   arguments has to be at least 14 to contemplate these platforms.  */
+
+float
+#ifdef NO_PROTOTYPES
+t_float_many_args (f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13,
+		   f14, f15)
+     float f1, float f2, float f3, float f4, float f5, float f6, float f7,
+     float f8, float f9, float f10, float f11, float f12, float f13, float f14,
+     float f15;
+#else
+t_float_many_args (float f1, float f2, float f3, float f4, float f5, float f6,
+		   float f7, float f8, float f9, float f10, float f11,
+		   float f12, float f13, float f14, float f15)
+#endif
+{
+  float sum_args;
+  float sum_values;
+
+  sum_args = f1 + f2 + f3 + f4 + f5 + f6 + f7 + f8 + f9 + f10 + f11 + f12
+	     + f13 + f14 + f15;
+  sum_values = float_val1 + float_val2 + float_val3 + float_val4 + float_val5
+	       + float_val6 + float_val7 + float_val8 + float_val9
+	       + float_val10 + float_val11 + float_val12 + float_val13
+	       + float_val14 + float_val15;
+
+  return ((sum_args - sum_values) < DELTA
+	  && (sum_args - sum_values) > -DELTA);
+}
+
 #ifdef PROTOTYPES
 int t_double_values (double double_arg1, double double_arg2)
 #else
@@ -311,6 +370,39 @@
 	  && (double_arg2 - double_val2) > -DELTA);
 }
 
+/* This function has many arguments to force some of them to be passed via
+   the stack instead of registers, to test that GDB can construct correctly
+   the parameter save area. Note that Linux/ppc32 has 8 float registers to use
+   for float parameter passing and Linux/ppc64 has 13, so the number of
+   arguments has to be at least 14 to contemplate these platforms.  */
+
+double
+#ifdef NO_PROTOTYPES
+t_double_many_args (f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13,
+		   f14, f15)
+     double f1, double f2, double f3, double f4, double f5, double f6,
+     double f7, double f8, double f9, double f10, double f11, double f12,
+     double f13, double f14, double f15;
+#else
+t_double_many_args (double f1, double f2, double f3, double f4, double f5,
+		    double f6, double f7, double f8, double f9, double f10,
+		    double f11, double f12, double f13, double f14, double f15)
+#endif
+{
+  double sum_args;
+  double sum_values;
+
+  sum_args = f1 + f2 + f3 + f4 + f5 + f6 + f7 + f8 + f9 + f10 + f11 + f12
+	     + f13 + f14 + f15;
+  sum_values = double_val1 + double_val2 + double_val3 + double_val4
+	       + double_val5 + double_val6 + double_val7 + double_val8
+	       + double_val9 + double_val10 + double_val11 + double_val12
+	       + double_val13 + double_val14 + double_val15;
+
+  return ((sum_args - sum_values) < DELTA
+	  && (sum_args - sum_values) > -DELTA);
+}
+
 #ifdef PROTOTYPES
 int t_string_values (char *string_arg1, char *string_arg2)
 #else
Index: gdb/testsuite/gdb.base/callfuncs.exp
===================================================================
--- gdb.orig/testsuite/gdb.base/callfuncs.exp	2008-01-11 09:59:22.000000000 -0800
+++ gdb/testsuite/gdb.base/callfuncs.exp	2008-01-11 09:59:33.000000000 -0800
@@ -1,5 +1,5 @@
 # Copyright 1992, 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003,
-# 2004, 2007, 2008 Free Software Foundation, Inc.
+# 2004, 2007 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -155,6 +155,8 @@
         
 	gdb_test "p t_float_values2(3.14159,float_val2)" " = 1"
 
+	gdb_test "p t_float_many_args (float_val1, float_val2, float_val3, float_val4, float_val5, float_val6, float_val7, float_val8, float_val9, float_val10, float_val11, float_val12, float_val13, float_val14, float_val15)" " = 1" "Call function with many float arguments."
+
 	gdb_test "p t_small_values(1,2,3,4,5,6,7,8,9,10)" " = 55"
 
 	gdb_test "p t_double_values(0.0,0.0)" " = 0"
@@ -163,6 +165,8 @@
 	gdb_test "p t_double_values(45.654,double_val2)" " = 1"
 	gdb_test "p t_double_values(double_val1,-67.66)" " = 1"
 
+	gdb_test "p t_double_many_args (double_val1, double_val2, double_val3, double_val4, double_val5, double_val6, double_val7, double_val8, double_val9, double_val10, double_val11, double_val12, double_val13, double_val14, double_val15)" " = 1" "Call function with many double arguments."
+
 	gdb_test "p t_double_int(99.0, 1)" " = 0"
 	gdb_test "p t_double_int(99.0, 99)" " = 1"
 	gdb_test "p t_int_double(99, 1.0)" " = 0"
Index: gdb/ppc-sysv-tdep.c
===================================================================
--- gdb.orig/ppc-sysv-tdep.c	2008-01-11 09:59:22.000000000 -0800
+++ gdb/ppc-sysv-tdep.c	2008-01-11 13:22:52.000000000 -0800
@@ -129,17 +129,35 @@
 		}
 	      else
 		{
-		  /* SysV ABI converts floats to doubles before
-		     writing them to an 8 byte aligned stack location.  */
-		  argoffset = align_up (argoffset, 8);
+		  /* The ppc32 SysV ABI tells us to convert floats to doubles
+		     before writing them to an 8 byte aligned stack location.
+
+		     NOTE: 2008/01/11: This is not exactly right.  GCC does not
+		     convert floats to doubles neither store floats into
+		     8 bytes aligned stack locations when we have a prototyped
+		     float parameter.  In this case GCC simply stores floats
+		     into 4 bytes aligned locations.  The promotion of floats
+		     to doubles happens, for example, if we have functions with
+		     float varargs (in which case the floats are converted to
+		     doubles and we will see them as so).  The ABI text needs
+		     to be updated.  */
+
+		  /* Align to 4 bytes or 8 bytes depending on the type of
+		     the argument (float or double).  */
+		  argoffset = align_up (argoffset, len);
+
 		  if (write_pass)
 		    {
-		      char memval[8];
+		      char memval[len];
 		      convert_typed_floating (val, type, memval,
-					      builtin_type_ieee_double);
+					      (len == 4)?
+					      builtin_type_ieee_single
+					      : builtin_type_ieee_double);
 		      write_memory (sp + argoffset, val, len);
 		    }
-		  argoffset += 8;
+		  /* Set offset according to the length of the argument's
+		     type (float or double).  */
+		  argoffset += len;
 		}
 	    }
 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

On Fri, 2008-01-11 at 19:25 -0200, Luis Machado wrote:
> > > > How about an extra flag inside the gdbarch_tdep structure that
> > > > you'll be using inside the push dummy call?  It would be a shame
> > > > duplicate this function when most of it is common.

<snip>

> Actually, we won't need to go through the flag. I've checked and the
> PPC/Linux ABI conforms with the PPC/SysV ABI regarding Calling Sequences
> and the Stack layout.

<snip>

> The SysV ABI says we should promote exceeding float parameters (the ones
> that should go to the stack) to double, and store then with 8 bytes
> alignment in the stack.

<snip>

> GCC (XLC as well) doesn't promote floats to doubles and does not align
> them to 8 bytes in the stack. Actually, it just aligns the prototyped
> float parameters to 4 bytes in the stack.
> 
> This is something that needs to be improved in the ABI text.
> 
> The patch follows, with a minor change in the ABI handling for PPC32 and
> a testcase Thiago wrote to verify the problem.
> 
> Comments?

Ping?

It would be nice if someone could test this patch in other systems which
use the SysV ppc32 ABI, to see if they actually implement what is in the
ABI specification or if they also do what Linux actually does. If the
former, this patch could go in. If the latter, we could use the
suggested flag in gdbarch_tdep to restrict this fix only to Linux.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

On Fri, 2008-01-25 at 14:05 -0200, Thiago Jung Bauermann wrote:
> It would be nice if someone could test this patch in other systems
> which use the SysV ppc32 ABI, to see if they actually implement what
> is in the ABI specification or if they also do what Linux actually
> does. If the former, this patch could go in. If the latter, we could
> use the suggested flag in gdbarch_tdep to restrict this fix only to
> Linux.

Ouch, the "former" and "latter" parts are swapped here. But you get what
I mean. :-)
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Mark Kettenis]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Date: Fri, 25 Jan 2008 14:05:55 -0200

> > The SysV ABI says we should promote exceeding float parameters (the ones
> > that should go to the stack) to double, and store then with 8 bytes
> > alignment in the stack.

This makes quite a bit of sense, since it solves problems with
unprototyped functions.

> > GCC (XLC as well) doesn't promote floats to doubles and does not align
> > them to 8 bytes in the stack. Actually, it just aligns the prototyped
> > float parameters to 4 bytes in the stack.

So both compilers are buggy.

> > This is something that needs to be improved in the ABI text.

Well, I suppose you can change it, but I wouldn't call it an
improvement.  Who controls the 32-bit PowerPC SystemV ABI these days?

I guess it makes sense for GDB to follow the broken GCC ABI for now,
since it doesn't seem there is a compiler that implements the SysV ABI
correctly.

> Ping?
> 
> It would be nice if someone could test this patch in other systems which
> use the SysV ppc32 ABI, to see if they actually implement what is in the
> ABI specification or if they also do what Linux actually does. If the
> former, this patch could go in. If the latter, we could use the
> suggested flag in gdbarch_tdep to restrict this fix only to Linux.

It seems OpenBSD/powerpc is similar to Linux here.  Without the patch I get

FAIL: gdb.base/callfuncs.exp: Call function with many float arguments.

which turns into a PASS if a apply the patch.  The patch itself isn't
quite right though.

The biggest problem is that variable-sized arrays are not in ISO C90,
so you can't use them; use alloca(3) instead, or simply allocate a
fixed amount that's big enough.

The second problem is that the conversion result of
convert_typed_floating() never actually gets used (the old code had
the same problem).  This must mean however, that doing the conversion
is completely redundant, and that the code can be simplified to:

		  /* Align to 4 bytes or 8 bytes depending on the type of
		     the argument (float or double).  */
		  argoffset = align_up (argoffset, len);
		  if (write_pass)
		      write_memory (sp + argoffset, val, len);
		  argoffset += len;

I'm also not really happy with the way the comment is phrased.

How about the attached diff?


Index: ppc-sysv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-sysv-tdep.c,v
retrieving revision 1.45
diff -u -p -r1.45 ppc-sysv-tdep.c
--- ppc-sysv-tdep.c	1 Jan 2008 22:53:12 -0000	1.45
+++ ppc-sysv-tdep.c	26 Jan 2008 15:24:36 -0000
@@ -129,17 +129,19 @@ ppc_sysv_abi_push_dummy_call (struct gdb
 		}
 	      else
 		{
-		  /* SysV ABI converts floats to doubles before
-		     writing them to an 8 byte aligned stack location.  */
-		  argoffset = align_up (argoffset, 8);
+		  /* The SysV ABI tells us to convert floats to
+		     doubles before writing them to an 8 byte aligned
+		     stack location.  Unfortunately GCC does not do
+		     that, and stores floats without into 4 bytes
+		     aligned locations without converting them to
+		     doubles. */
+
+		  /* Align to 4 bytes or 8 bytes depending on the type of
+		     the argument (float or double).  */
+		  argoffset = align_up (argoffset, len);
 		  if (write_pass)
-		    {
-		      char memval[8];
-		      convert_typed_floating (val, type, memval,
-					      builtin_type_ieee_double);
 		      write_memory (sp + argoffset, val, len);
-		    }
-		  argoffset += 8;
+		  argoffset += len;
 		}
 	    }
 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT

Re: [RFC] Linux-specific ppc32 ABI

[Joseph S. Myers]

On Sat, 26 Jan 2008, Mark Kettenis wrote:

> > > GCC (XLC as well) doesn't promote floats to doubles and does not align
> > > them to 8 bytes in the stack. Actually, it just aligns the prototyped
> > > float parameters to 4 bytes in the stack.
> 
> So both compilers are buggy.

They would be buggy if they were targetting PowerPC Solaris from 1995 and 
if the 1995 ABI from Sun is an accurate description of the ABI on that 
system in the first place.  They are not targetting that OS, they are 
targetting OSes where the 1995 document is only indicative guidance to a 
predecessor of the ABIs in use on those OSes.

> > > This is something that needs to be improved in the ABI text.
> 
> Well, I suppose you can change it, but I wouldn't call it an
> improvement.  Who controls the 32-bit PowerPC SystemV ABI these days?

The Power.org ABI working group, which is working on producing a 
copyright-clean document (so not using any of the Sun text), copyrights to 
be assigned to the Linux Foundation, describing the ABIs (both hard and 
soft float, and E500 and Altivec extensions) as they are in use today for 
both GNU/Linux and bare-metal (EABI) targets (GNU/Linux and EABI differ in 
the default for -maix-struct-return, and in the default size of long 
double, but are otherwise the same at the function-call level).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC] Linux-specific ppc32 ABI

[Mark Kettenis]

> Date: Sat, 26 Jan 2008 18:18:17 +0000 (UTC)
> From: "Joseph S. Myers" <joseph@codesourcery.com>
> 
> On Sat, 26 Jan 2008, Mark Kettenis wrote:
> 
> > > > GCC (XLC as well) doesn't promote floats to doubles and does not align
> > > > them to 8 bytes in the stack. Actually, it just aligns the prototyped
> > > > float parameters to 4 bytes in the stack.
> > 
> > So both compilers are buggy.
> 
> They would be buggy if they were targetting PowerPC Solaris from 1995 and 
> if the 1995 ABI from Sun is an accurate description of the ABI on that 
> system in the first place.  They are not targetting that OS, they are 
> targetting OSes where the 1995 document is only indicative guidance to a 
> predecessor of the ABIs in use on those OSes.

Well, I'd be surprised if this particular deviation from the 1995
document (which is pretty much a corner case) was a deliberate
deviation.  I'm not suggesting the bug should be fixed though, so I
guess I should have called it a feature ;)

> > > > This is something that needs to be improved in the ABI text.
> > 
> > Well, I suppose you can change it, but I wouldn't call it an
> > improvement.  Who controls the 32-bit PowerPC SystemV ABI these days?
> 
> The Power.org ABI working group, which is working on producing a 
> copyright-clean document (so not using any of the Sun text), copyrights to 
> be assigned to the Linux Foundation, describing the ABIs (both hard and 
> soft float, and E500 and Altivec extensions) as they are in use today for 
> both GNU/Linux and bare-metal (EABI) targets (GNU/Linux and EABI differ in 
> the default for -maix-struct-return, and in the default size of long 
> double, but are otherwise the same at the function-call level).

Thanks for the clarification.

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

On Sat, 2008-01-26 at 16:35 +0100, Mark Kettenis wrote:
> > It would be nice if someone could test this patch in other systems which
> > use the SysV ppc32 ABI, to see if they actually implement what is in the
> > ABI specification or if they also do what Linux actually does. If the
> > former, this patch could go in. If the latter, we could use the
> > suggested flag in gdbarch_tdep to restrict this fix only to Linux.
> 
> It seems OpenBSD/powerpc is similar to Linux here.  Without the patch I get
> 
> FAIL: gdb.base/callfuncs.exp: Call function with many float arguments.
> 
> which turns into a PASS if a apply the patch.  The patch itself isn't
> quite right though.

Thanks for testing.

> How about the attached diff?

Fine by me, just one small issue:

> +		     that, and stores floats without into 4 bytes
> +		     aligned locations without converting them to

The first "without" above shouldn't be there.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

On Sat, 2008-01-26 at 16:35 +0100, Mark Kettenis wrote:
> How about the attached diff?

Just a reminder that the testcase additions in Luis' patch need to go in
as well..
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: [RFC] Linux-specific ppc32 ABI

[Joel Brobecker]

Hi Mark,

> How about the attached diff?

Sounds like everyone is happy with the patch you suggested. Thiago
just mentioned one nit in your comments (repeated below). Can the patch
be checked in?  We'd like to have it for the release.

Thank you.

> Index: ppc-sysv-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ppc-sysv-tdep.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 ppc-sysv-tdep.c
> --- ppc-sysv-tdep.c	1 Jan 2008 22:53:12 -0000	1.45
> +++ ppc-sysv-tdep.c	26 Jan 2008 15:24:36 -0000
> @@ -129,17 +129,19 @@ ppc_sysv_abi_push_dummy_call (struct gdb
>  		}
>  	      else
>  		{
> -		  /* SysV ABI converts floats to doubles before
> -		     writing them to an 8 byte aligned stack location.  */
> -		  argoffset = align_up (argoffset, 8);
> +		  /* The SysV ABI tells us to convert floats to
> +		     doubles before writing them to an 8 byte aligned
> +		     stack location.  Unfortunately GCC does not do
> +		     that, and stores floats without into 4 bytes
                                             ^^^^^^^ should be removed
> +		     aligned locations without converting them to
> +		     doubles. */
> +
> +		  /* Align to 4 bytes or 8 bytes depending on the type of
> +		     the argument (float or double).  */
> +		  argoffset = align_up (argoffset, len);
>  		  if (write_pass)
> -		    {
> -		      char memval[8];
> -		      convert_typed_floating (val, type, memval,
> -					      builtin_type_ieee_double);
>  		      write_memory (sp + argoffset, val, len);
> -		    }
> -		  argoffset += 8;
> +		  argoffset += len;
>  		}
>  	    }
>  	  else if (TYPE_CODE (type) == TYPE_CODE_FLT

-- 
Joel

Re: [RFC] Linux-specific ppc32 ABI

[Mark Kettenis]

> Date: Fri, 1 Feb 2008 14:38:39 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> Hi Mark,
> 
> > How about the attached diff?
> 
> Sounds like everyone is happy with the patch you suggested. Thiago
> just mentioned one nit in your comments (repeated below). Can the patch
> be checked in?  We'd like to have it for the release.
> 

I tweaked the comment a bit more.  See below what I comitted.

Thiago/Luis, can you commit the testsuite change?


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
	Luis Machado  <luisgpm@br.ibm.com>
	Thiago Jung Bauermann  <bauerman@br.ibm.com>	

	* ppc-sysv-tdep.c (ppc_sysv_abi_push_dummy_call): Pass floats that
	don't fit into registerson the stack the way GCC does.

Index: ppc-sysv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-sysv-tdep.c,v
retrieving revision 1.47
diff -u -p -r1.47 ppc-sysv-tdep.c
--- ppc-sysv-tdep.c 1 Feb 2008 15:04:18 -0000 1.47
+++ ppc-sysv-tdep.c 2 Feb 2008 00:07:14 -0000
@@ -129,17 +129,21 @@ ppc_sysv_abi_push_dummy_call (struct gdb
 		}
 	      else
 		{
-		  /* SysV ABI converts floats to doubles before
-		     writing them to an 8 byte aligned stack location.  */
-		  argoffset = align_up (argoffset, 8);
+		  /* The SysV ABI tells us to convert floats to
+		     doubles before writing them to an 8 byte aligned
+		     stack location.  Unfortunately GCC does not do
+		     that, and stores floats into 4 byte aligned
+		     locations without converting them to doubles.
+		     Since there is no know compiler that actually
+		     follows the ABI here, we implement the GCC
+		     convention.  */
+
+		  /* Align to 4 bytes or 8 bytes depending on the type of
+		     the argument (float or double).  */
+		  argoffset = align_up (argoffset, len);
 		  if (write_pass)
-		    {
-		      char memval[8];
-		      convert_typed_floating (val, type, memval,
-					      builtin_type_ieee_double);
 		      write_memory (sp + argoffset, val, len);
-		    }
-		  argoffset += 8;
+		  argoffset += len;
 		}
 	    }
 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT

Re: [RFC] Linux-specific ppc32 ABI

[Thiago Jung Bauermann]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

On Sat, 2008-02-02 at 01:08 +0100, Mark Kettenis wrote:
> Thiago/Luis, can you commit the testsuite change?

Sure. I committed the attached patch.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

[-- Attachment #2: ppc32-float-abi-testcases.diff --]
[-- Type: text/x-patch, Size: 6316 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.1567
diff -u -r1.1567 ChangeLog
--- ChangeLog	5 Feb 2008 22:20:51 -0000	1.1567
+++ ChangeLog	6 Feb 2008 03:52:41 -0000
@@ -1,3 +1,10 @@
+2008-02-06  Thiago Jung Bauermann  <bauerman@br.ibm.com>
+
+	* gdb.base/callfuncs.c (t_float_many_args): New function.
+	(t_double_many_args): New function.
+	* gdb.base/callfuncs.exp: Add tests for exceeding float
+	and double parameters passed through the stack.
+
 2008-02-05  Joel Brobecker  <brobecker@adacore.com>
 
 	* gdb.ada/complete/pck.ads, gdb.ada/complete/pck.adb,
Index: gdb.base/callfuncs.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/callfuncs.c,v
retrieving revision 1.10
diff -u -r1.10 callfuncs.c
--- gdb.base/callfuncs.c	1 Jan 2008 22:53:18 -0000	1.10
+++ gdb.base/callfuncs.c	6 Feb 2008 03:52:41 -0000
@@ -46,9 +46,35 @@
 
 float float_val1 = 3.14159;
 float float_val2 = -2.3765;
+float float_val3 = 0.25;
+float float_val4 = 1.25;
+float float_val5 = 2.25;
+float float_val6 = 3.25;
+float float_val7 = 4.25;
+float float_val8 = 5.25;
+float float_val9 = 6.25;
+float float_val10 = 7.25;
+float float_val11 = 8.25;
+float float_val12 = 9.25;
+float float_val13 = 10.25;
+float float_val14 = 11.25;
+float float_val15 = 12.25;
 
 double double_val1 = 45.654;
 double double_val2 = -67.66;
+double double_val3 = 0.25;
+double double_val4 = 1.25;
+double double_val5 = 2.25;
+double double_val6 = 3.25;
+double double_val7 = 4.25;
+double double_val8 = 5.25;
+double double_val9 = 6.25;
+double double_val10 = 7.25;
+double double_val11 = 8.25;
+double double_val12 = 9.25;
+double double_val13 = 10.25;
+double double_val14 = 11.25;
+double double_val15 = 12.25;
 
 #define DELTA (0.001)
 
@@ -298,6 +324,39 @@
 	  && (float_arg2 - float_val2) > -DELTA);
 }
 
+/* This function has many arguments to force some of them to be passed via
+   the stack instead of registers, to test that GDB can construct correctly
+   the parameter save area. Note that Linux/ppc32 has 8 float registers to use
+   for float parameter passing and Linux/ppc64 has 13, so the number of
+   arguments has to be at least 14 to contemplate these platforms.  */
+
+float
+#ifdef NO_PROTOTYPES
+t_float_many_args (f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13,
+		   f14, f15)
+     float f1, float f2, float f3, float f4, float f5, float f6, float f7,
+     float f8, float f9, float f10, float f11, float f12, float f13, float f14,
+     float f15;
+#else
+t_float_many_args (float f1, float f2, float f3, float f4, float f5, float f6,
+		   float f7, float f8, float f9, float f10, float f11,
+		   float f12, float f13, float f14, float f15)
+#endif
+{
+  float sum_args;
+  float sum_values;
+
+  sum_args = f1 + f2 + f3 + f4 + f5 + f6 + f7 + f8 + f9 + f10 + f11 + f12
+	     + f13 + f14 + f15;
+  sum_values = float_val1 + float_val2 + float_val3 + float_val4 + float_val5
+	       + float_val6 + float_val7 + float_val8 + float_val9
+	       + float_val10 + float_val11 + float_val12 + float_val13
+	       + float_val14 + float_val15;
+
+  return ((sum_args - sum_values) < DELTA
+	  && (sum_args - sum_values) > -DELTA);
+}
+
 #ifdef PROTOTYPES
 int t_double_values (double double_arg1, double double_arg2)
 #else
@@ -311,6 +370,39 @@
 	  && (double_arg2 - double_val2) > -DELTA);
 }
 
+/* This function has many arguments to force some of them to be passed via
+   the stack instead of registers, to test that GDB can construct correctly
+   the parameter save area. Note that Linux/ppc32 has 8 float registers to use
+   for float parameter passing and Linux/ppc64 has 13, so the number of
+   arguments has to be at least 14 to contemplate these platforms.  */
+
+double
+#ifdef NO_PROTOTYPES
+t_double_many_args (f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13,
+		   f14, f15)
+     double f1, double f2, double f3, double f4, double f5, double f6,
+     double f7, double f8, double f9, double f10, double f11, double f12,
+     double f13, double f14, double f15;
+#else
+t_double_many_args (double f1, double f2, double f3, double f4, double f5,
+		    double f6, double f7, double f8, double f9, double f10,
+		    double f11, double f12, double f13, double f14, double f15)
+#endif
+{
+  double sum_args;
+  double sum_values;
+
+  sum_args = f1 + f2 + f3 + f4 + f5 + f6 + f7 + f8 + f9 + f10 + f11 + f12
+	     + f13 + f14 + f15;
+  sum_values = double_val1 + double_val2 + double_val3 + double_val4
+	       + double_val5 + double_val6 + double_val7 + double_val8
+	       + double_val9 + double_val10 + double_val11 + double_val12
+	       + double_val13 + double_val14 + double_val15;
+
+  return ((sum_args - sum_values) < DELTA
+	  && (sum_args - sum_values) > -DELTA);
+}
+
 #ifdef PROTOTYPES
 int t_string_values (char *string_arg1, char *string_arg2)
 #else
Index: gdb.base/callfuncs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/callfuncs.exp,v
retrieving revision 1.23
diff -u -r1.23 callfuncs.exp
--- gdb.base/callfuncs.exp	1 Jan 2008 22:53:18 -0000	1.23
+++ gdb.base/callfuncs.exp	6 Feb 2008 03:52:41 -0000
@@ -155,6 +155,8 @@
         
 	gdb_test "p t_float_values2(3.14159,float_val2)" " = 1"
 
+	gdb_test "p t_float_many_args (float_val1, float_val2, float_val3, float_val4, float_val5, float_val6, float_val7, float_val8, float_val9, float_val10, float_val11, float_val12, float_val13, float_val14, float_val15)" " = 1" "Call function with many float arguments."
+
 	gdb_test "p t_small_values(1,2,3,4,5,6,7,8,9,10)" " = 55"
 
 	gdb_test "p t_double_values(0.0,0.0)" " = 0"
@@ -163,6 +165,8 @@
 	gdb_test "p t_double_values(45.654,double_val2)" " = 1"
 	gdb_test "p t_double_values(double_val1,-67.66)" " = 1"
 
+	gdb_test "p t_double_many_args (double_val1, double_val2, double_val3, double_val4, double_val5, double_val6, double_val7, double_val8, double_val9, double_val10, double_val11, double_val12, double_val13, double_val14, double_val15)" " = 1" "Call function with many double arguments."
+
 	gdb_test "p t_double_int(99.0, 1)" " = 0"
 	gdb_test "p t_double_int(99.0, 99)" " = 1"
 	gdb_test "p t_int_double(99, 1.0)" " = 0"