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

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5308 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

>This seems unnecessarily complex, as wordsize is always a power of 2.
>It could simply be:
>+                 space += ((len - argbytes + wordsize - 1) & -wordsize);

>>If not let me know what else I can learn or need to change.

This is done.

>I think this is still not quite correct.  Depending on the type,
>some parameters that are not integers (e.g. structs) should not
>be extended, as that doesn't make sense for this type.  When
>loading into registers, you see the existing code:

>          memset (word, 0, reg_size);
>          if (type->code () == TYPE_CODE_INT
>             || type->code () == TYPE_CODE_ENUM
>             || type->code () == TYPE_CODE_BOOL
>             || type->code () == TYPE_CODE_CHAR)
>            /* Sign or zero extend the "int" into a "word".  */

>It would probably be best to duplicate that logic for the
>stack argument case.

This is done.

>Also, the "argbytes" case seems wrong - this is the case where
>an argument fits "half" in registers and "half" on the stack.
>In those cases, it does not make any sense to "extend" just
>the second half on its own ...

Yes I realised this. Thanks. I removed the zero extension. Just kept those changes that will keep it synced with the rest of the code and needed as per the word size.

Hope this looks good now. Kindly let me know. If not kindly push these changes.

Outputs are pasted below..

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 7:43 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:

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

This seems unnecessarily complex, as wordsize is always a power of 2.
It could simply be:
+                 space += ((len - argbytes + wordsize - 1) & -wordsize);

>If not let me know what else I can learn or need to change.

I think this is still not quite correct.  Depending on the type,
some parameters that are not integers (e.g. structs) should not
be extended, as that doesn't make sense for this type.  When
loading into registers, you see the existing code:

          memset (word, 0, reg_size);
          if (type->code () == TYPE_CODE_INT
             || type->code () == TYPE_CODE_ENUM
             || type->code () == TYPE_CODE_BOOL
             || type->code () == TYPE_CODE_CHAR)
            /* Sign or zero extend the "int" into a "word".  */
            store_unsigned_integer (word, reg_size, byte_order,
                                    unpack_long (type, arg->contents ().data ()));
          else
            memcpy (word, arg->contents ().data (), len);

It would probably be best to duplicate that logic for the
stack argument case.

Also, the "argbytes" case seems wrong - this is the case where
an argument fits "half" in registers and "half" on the stack.
In those cases, it does not make any sense to "extend" just
the second half on its own ...

Bye,
Ulrich

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

From f7d50b75cdb783e4942b333adfb836c68cecf2bb Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 25 Aug 2023 10:24:58 -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 | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 829f55981ca..a3e9e94841c 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -649,7 +649,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       if (argbytes)
 	{
-	  space += ((len - argbytes + 3) & -4);
+	  space += ((len - argbytes + wordsize -1) & -wordsize);
 	  jj = argno + 1;
 	}
       else
@@ -658,7 +658,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 () + wordsize -1) & -wordsize);
 	}
 
       /* Add location required for the rest of the parameters.  */
@@ -679,11 +679,11 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       if (argbytes)
 	{
-	  write_memory (sp + 24 + (ii * 4),
+	  write_memory (sp + 24 + (ii * wordsize),
 			arg->contents ().data () + argbytes,
 			len - argbytes);
 	  ++argno;
-	  ii += ((len - argbytes + 3) & -4) / 4;
+	  ii += ((len - argbytes + wordsize - 1) & -wordsize) / wordsize;
 	}
 
       /* Push the rest of the arguments into stack.  */
@@ -707,8 +707,21 @@ 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 && 
+	     (type->code () == TYPE_CODE_INT
+	      || type->code () == TYPE_CODE_ENUM
+	      || type->code () == TYPE_CODE_BOOL
+	      || type->code () == TYPE_CODE_CHAR ))
+	    {
+	      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 + 6 * wordsize + (ii * wordsize), word, PPC_MAX_REGISTER_SIZE);
+	    }
+	  else
+	    write_memory (sp + 6 * wordsize + (ii * wordsize), arg->contents ().data (), len);
+	  ii += ((len + wordsize -1) & -wordsize) / wordsize;
 	}
     }
 
-- 
2.38.3