Re: [PATCH] Fix for call feature having nine parameters or more in AIX

[Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>]

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

Hi Ulrich and GDB community members,

Thank you for the feedback. Please find attached the patch. See:- 0001-Fix-for-call-feature-having-9th-function-parameter-a.patch

> I agree you've identified a problem, but I think your patch isn't quite complete.

> For example, immediately after the code you changed follows:
>          ii += ((len + 3) & -4) / 4;
> The intent is to always uses full stack slots even for arguments of odd sizes.

> But I understand in the 64-bit ABI the stack slot size is 8 bytes, so this
> should round up to the next multiple of 8.

> Similarly, you need to make sure that the first loop that computes the *size*
> of the stack that will be used for arguments performs the same calculations
> as the code that actually fills in the arguments - or else you can overwrite
> unrelated areas:
>          space += ((len - argbytes + 3) & -4);
>          space += ((val->type ()->length ()) + 3) & -4;
> All of this should round up to wordsize instead of 4, I guess.
> This will now round up to word size after the changes in this patch

I get why you are saying this. So the slack slots need to be of 8 bytes. In this case they are adding 3 cz assume a character guy comes then it becomes 1+3 = 4, which then & -4 will give you 4. Even if a bigger guy than character datatype comes it will be 4 at least. So it will be rounded to 4 all the time.

Similarly when for our 8 byte friend we will need the number 7 instead of 3.

So 4 byte = 3, 8 byte = 7, this is a pattern of 2 power log base 2 (wordsize) -1. Example if the wordsize is 4 then we get 2*pow ( log 2 (4)) – 1 = 2 pow (2) – 1 = 4 – 1 =3.. Same substitute we can do for 8 word size and we will get seven. So in my patch you will see this change wherever possible.

-                 space += ((len - argbytes + 3) & -4);
+                space += ((len - argbytes + (int) pow (2, (int)log2 (wordsize)) -1) & -wordsize);

So now space becomes 4, 8, 12 in 32 bit mode and 8, 16, 24 in 64 bit mode for 9th, 10th and 11th parameter of a function. Same goes with ii as well. It will come 8, 9, 10 irrespective of any word size.

Hope this works for all of us..

Please see the 32 bit and 64 bit output pasted. They work fine.

Thank you once again for guiding me. Let me know if it works. If this okay please push this patch. If not let me know what else I can learn or need to change.

Also, I am curious to know why didn’t I get a SEG fault or garbage the last time and why was it working without these space changes. I was trying to understand but somehow am not fully clear about this. Let me know if you did..

Have a nice day ahead.

Thanks and regards,
Aditya.

32 bit output with patch:-

Reading symbols from /home/aditya/gdb_tests/nine_parameter_func...
(gdb) b main
Breakpoint 1 at 0x1000078c: file /home/aditya/gdb_tests/nine_parameter_func.c, line 27.
(gdb) r
Starting program: /home/aditya/gdb_tests/nine_parameter_func

Breakpoint 1, main () at /home/aditya/gdb_tests/nine_parameter_func.c:27
27        const float register f3 = 19.0;
(gdb) list
22        printf ("j = %d \n", j);
23        return (int)(d);
24      }
25      int main ()
26      {
27        const float register f3 = 19.0;
28        const int register i1 = 700;
29        printf("%f \n", f3 + i1);
30        b ();
31        a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 19);
(gdb) call  a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 19)
812.000000
9th para = 9 , 10th para = 983
j = 9
$1 = 1041
(gdb) call  a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 25)
812.000000
9th para = 9 , 10th para = 983
j = 9
$2 = 1047
(gdb)

64 bit output with patch:-

Breakpoint 1, main () at /home/aditya/gdb_tests/nine_parameter_func.c:27
27        const float register f3 = 19.0;
(gdb) lsit
Undefined command: "lsit".  Try "help".
(gdb) list
22        printf ("j = %d \n", j);
23        return (int)(d);
24      }
25      int main ()
26      {
27        const float register f3 = 19.0;
28        const int register i1 = 700;
29        printf("%f \n", f3 + i1);
30        b ();
31        a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 19);
(gdb) call a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 19)
812.000000
9th para = 9 , 10th para = 983
j = 9
$1 = 1041
(gdb) call a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 25)
812.000000
9th para = 9 , 10th para = 983
j = 9
$2 = 1047
(gdb) call a (1, 2, 3, 4, 5, 6, 7, 8, 9, 983, 30)
812.000000
9th para = 9 , 10th para = 983
j = 9
$3 = 1052
(gdb)

From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Friday, 25 August 2023 at 4:49 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Fix for call feature having nine parameters or more in AIX
Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>So, debugging further I realized that the parameters of function in AIX
>are stored in registers 3 to 10. More about this fact can be read in this
>document {https://www.ibm.com/docs/en/aix/7.2?topic=overview-register-usage-conventions}.
>If the function has more than 8 parameters then the 9th one onwards,  we store
>the function parameters in the stack. This can be seen in the rs6000-aix-tdep.c
>file in the dummy_call function from line 700 and beyond. Over here we have
>this line below.
>
>write_memory (sp + 24 + (ii * 4), arg->contents ().data (), len);
>
>This the root cause of this issue.

I agree you've identified a problem, but I think your patch isn't quite complete.

For example, immediately after the code you changed follows:
          ii += ((len + 3) & -4) / 4;
The intent is to always uses full stack slots even for arguments of odd sizes.

But I understand in the 64-bit ABI the stack slot size is 8 bytes, so this
should round up to the next multiple of 8.

Similarly, you need to make sure that the first loop that computes the *size*
of the stack that will be used for arguments performs the same calculations
as the code that actually fills in the arguments - or else you can overwrite
unrelated areas:

      if (argbytes)
        {
          space += ((len - argbytes + 3) & -4);
          jj = argno + 1;
        }
      else
        jj = argno;


      for (; jj < nargs; ++jj)
        {
          struct value *val = args[jj];
          space += ((val->type ()->length ()) + 3) & -4;
        }

All of this should round up to wordsize instead of 4, I guess.

Bye,
Ulrich

[-- Attachment #2: 0001-Fix-for-call-feature-having-9th-function-parameter-a.patch --]
[-- Type: application/octet-stream, Size: 3427 bytes --]

From 77187ed24062c95bab11c044240cd0c78b8736d1 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 25 Aug 2023 08:22:00 -0500
Subject: [PATCH] Fix for call feature having 9th function parameter and beyond
   corrupt values.

In AIX the first eight function parameters are stored from R3 to R10.
If there are more than eight parameters in a function then we store the 9th parameter onwards in the stack.
While doing so, in 64 bit mode the words were not zero extended and was coming like 32 bit mode.
This patch is a fix to the same.
---
 gdb/rs6000-aix-tdep.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 829f55981ca..ebcb9850dc7 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -39,6 +39,7 @@
 #include "gdbsupport/xml-utils.h"
 #include "trad-frame.h"
 #include "frame-unwind.h"
+#include <math.h>
 
 /* If the kernel has to deliver a signal, it pushes a sigcontext
    structure on the stack and then calls the signal handler, passing
@@ -649,7 +650,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       if (argbytes)
 	{
-	  space += ((len - argbytes + 3) & -4);
+	  space += ((len - argbytes + (int) pow (2, (int)log2 (wordsize)) -1) & -wordsize);
 	  jj = argno + 1;
 	}
       else
@@ -658,7 +659,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
       for (; jj < nargs; ++jj)
 	{
 	  struct value *val = args[jj];
-	  space += ((val->type ()->length ()) + 3) & -4;
+	  space += (((val->type ()->length ()) + (int) pow (2, (int)log2 (wordsize)) -1) & -wordsize);
 	}
 
       /* Add location required for the rest of the parameters.  */
@@ -679,11 +680,20 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       if (argbytes)
 	{
-	  write_memory (sp + 24 + (ii * 4),
-			arg->contents ().data () + argbytes,
-			len - argbytes);
+	  if (wordsize == 8)
+	    {
+	      gdb_byte word[PPC_MAX_REGISTER_SIZE];
+	      memset (word, 0, PPC_MAX_REGISTER_SIZE);
+	      store_unsigned_integer (word, tdep->wordsize, byte_order,
+				      unpack_long (type, arg->contents ().data ()));
+	      write_memory (sp + 48 + (ii * wordsize), word, PPC_MAX_REGISTER_SIZE);
+	    }
+	  else
+	    write_memory (sp + 24 + (ii * wordsize),
+			  arg->contents ().data () + argbytes,
+			  len - argbytes);
 	  ++argno;
-	  ii += ((len - argbytes + 3) & -4) / 4;
+	  ii += ((len - argbytes + (int) pow (2, (int)log2 (wordsize)) - 1) & -wordsize) / wordsize;
 	}
 
       /* Push the rest of the arguments into stack.  */
@@ -707,8 +717,17 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	      ++f_argno;
 	    }
 
-	  write_memory (sp + 24 + (ii * 4), arg->contents ().data (), len);
-	  ii += ((len + 3) & -4) / 4;
+	  if (wordsize == 8)
+	    {
+	      gdb_byte word[PPC_MAX_REGISTER_SIZE];
+	      memset (word, 0, PPC_MAX_REGISTER_SIZE);
+	      store_unsigned_integer (word, tdep->wordsize, byte_order,
+				      unpack_long (type, arg->contents ().data ()));
+	      write_memory (sp + 48 + (ii * wordsize), word, PPC_MAX_REGISTER_SIZE);
+	    }
+	  else
+	    write_memory (sp + 24 + (ii * wordsize), arg->contents ().data (), len);
+	  ii += ((len + (int) pow (2, (int)log2 (wordsize)) -1) & -wordsize) / wordsize;
 	}
     }
 
-- 
2.38.3