Hi Ulrich and community,

Please find attached the patch. {See: 0001-Enable-vector-instruction-debugging-for-AIX.patch}.

Please see my comments in blue.

Kindly suggest for changes if needed and review this patch.

Have a nice day ahead.

Thanks and regards,
Aditya.

>So you're now using ptrace here as well.  From what I
>understand, this wrong - ptrace can only be used with
>kernel threads.  For user threads, registers need to
>be retrieved from the context managed by the thread
>library and retrieved via pthdb_pthread_context.

>Shouldn't the vector registers then also come from there?

>Do you have a way to test with using user threads?

Yes. So I did not get this back then. As my learning increased via the threads bug we had just solved I understood what you are trying to tell me here. Kindly see pdc_read_regs () and pdc_write_regs ().
We now do it via pthdb_pthread_context ().

Kindly see code 1 which uses user threads and it sample output 1. It works alright. Even our alti-vec-regs.exp testcase in test suite passes.

>>(gdb) info reg vr0
>>vr0            {uint128 = <unavailable>, v4_float = {<unavailable>, <unavailable>, <unavailable>, >><unavailable>}, v4_int32 = {<unavailable>, <unavailable>, <unavailable>, <unavailable>}, v8_int16 = >>{<unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>, >><unavailable>, <unavailable>}, v16_int8 = {<unavailable> <repeats 16 times>}}

>I still don't quite understand the intended behavior.  As far
>as I can see, there are three possible states:

>- The machine doesn't have any vector registers
>  (i.e. __power_vmx() returns 0)
If it doesn’t have we do not do any thing. We do not even say machine does not have any vector registers. Hence no else part to this if condition.

>- The machine does have vector registers, but the inferior
>  process does not use them
 > (i.e. __power_vmx() returns 1 but ptrace returns an error)

In this we intend to show it unavailable. Because till the user uses vector registers it has to be unavailable.

- The inferior process uses vector registers
  (i.e. no error from ptrace)
Here we show the value.

>What is the intended behavior in those cases?  In particular,
>should GDB behave differently between the first and the
>second case?  Should GDB not show any vector registers at all,
>should it show vector registers as <unavailable>, or should
>it show vector registers containing zero?

In Linux I have seen they show a 0, in the second case. So do we also do the same?
When I tried doing so, I can tell you ptrace () did not allow me to write 0 as well in store_vsx_register_aix(), since till user or debugee code actually uses it, it is unavailable. So we cannot write 0 in the ptrace call via PT_WRITE_VEC.

But yeah, we can use this regcache->raw_collect (), set to 0 in GDB data structures and return. We won’t be able to write 0 though through ptrace call in the kernel. This way we can show users it is zero like in the linux world. So shall we do it?


>>>+      ret = rs6000_ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0);
>>>+    else
>>>+      ret = rs6000_ptrace32 (PTT_READ_VSX, thrd_i, (int *)&vsx, 0, 0);
>
>>>Just noticed this here.  Everywhere else in this file, a *pid* is
>>>passed to rs6000_ptrace64/32.  Only in this new routine, you're
>>>apparently passing a thread ID.   Why should this be different just
>>>for these registers?  Shouldn't per-thread registers be handled in
>>>aix-thread.c anyway?
>>Well, this is because in the documentation I read, I did not find something
>>like a PT_READ_VSX that I can use. All I have is PTT_READ_VSX and in AIX
>>PTT _* options are usually thread IDs.. I am restricted.. If it existed,
>>I would have loved to use it. Like for writing general purpose registers
>>we have PTT_WRITE_GPRS and PT_WRITE_GPRS and can use it as per convenience
>>of thread or process..

>Huh.  This is really strange, and doesn't quite map on the current GDB
>implementation.  Everything related to thread IDs is currently done in
>the aix-threads.c file.  It seems not really clean to mix this up.

>How is this intended to be used?

>If we need thread IDs anyway (and apparently we can always get a thread
>ID even for a non-threaded process?), should we just use the aix-thread.c
>methods for all processes?

The online documentation I read { https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine } said The PTT_READ_VEC request reads the vector register state of the specified thread. There is no PT_READ_VEC or any call that takes <pid> to get vector registers in any online or internal documents I searched. So we need a thread ID to fetch vector registers.

Coming to just using aix-thread.c for all process, I did try it today. What is happening is fetch_registers () in aix-thread.c is not getting called after the thread target is set for any debugee that does not use pthreads or is multi threaded once the pthdebug session is established. {Like the program with name “code 2” pasted below this email}. Only while the process is getting born it does call fetch_registers () in the rs6000-aix-nat.c . If we can make the thread target to call fetch_registers () once the thread target is set it will work. But is that correct?

>As an aside, I don't see any documentation of PTT_READ_VSX either.
>Do you know since when this is supported?
From the documentation  I read it is 2009.

+  if (altivec_register_p (gdbarch, regno))
+    fetch_altivec_registers_aix (regcache);
+  if (vsx_register_p (gdbarch, regno))
+    fetch_vsx_registers_aix (regcache);

>I meant that you should skip everything else in those cases:

 > if (altivec_register_p (gdbarch, regno))
 >   {
 >     fetch_altivec_registers_aix (regcache);
>      return;

>just like in store_register.

This is also done.

>>Since for the stxvd2x instruction, the last bit is part of the
>>register number, these two lines should be merged into:
> > >            || (op & 0xfc1ffffe) == 0x7c012f98 /* stxvd2x Vs, r1, r5 */
>>
>>>As above, the comment needs to be updated to match.

>>You should still do this (the 0x...e mask, and the comment).

I did not get this.. Kindly give me more information.

>Well, at the very least this needs to be announced in the NEWS file.

>This still mentions AIX 4 being supported.

So what is the process and when do we do it. Do we do it after the commit of this patch??

There's still this one in there:
>-   bfd_mach_rs6k, &tdesc_rs6000},
>+   bfd_mach_rs6k, &tdesc_powerpc_vsx32},

>I don't think this should be needed either.
This I have removed.

----------------------------------------------------------
Code 1:-
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <pthread.h>
#include <assert.h>

pthread_barrier_t barrier;

#define NUM_THREADS 3

void *
thread_function (void *arg)
{
  /* This ensures that the breakpoint is only hit after both threads
     are created, so the test can always switch to the non-event
     thread when the breakpoint triggers.  */
  pthread_barrier_wait (&barrier);

  vector int Arr1[5] = {
  { 1, 2, 3, 4 },
  { 11, 22, 33, 44 },
  { 111, 222, 333, 444 },
  { 1111, 2222, 3333, 4444 },
  { 11111, 22222, 33333, 44444 }
};
  vector int Arr2[5] = {
  { 1, 2, 3, 4 },
  { 11, 22, 33, 44 },
  { 111, 222, 333, 444 },
  { 1111, 2222, 3333, 4444 },
  { 11111, 22222, 33333, 44444 }
};
vector int Arr3[5] = {
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 }
};
        printf("Adding Arrays\n");
        Arr3[1] = Arr1[0] + Arr2[0];
        printf("Arr3[1] = { %d, %d, %d, %d }\n", Arr3[1][0], Arr3[1][1], Arr3[1][2], Arr3[1][3]);



  while (1); /* break here */
}

int
main (void)
{
  int i;

  alarm (300);

  pthread_barrier_init (&barrier, NULL, NUM_THREADS);

  for (i = 0; i < NUM_THREADS; i++)
    {
      pthread_t thread;
      int res;

      res = pthread_create (&thread, NULL,
                            thread_function, NULL);
      assert (res == 0);
    }

  while (1)
    sleep (1);

  return 0;
}
-----------------------------------------------------
Output with patch:-
Reading symbols from /home/aditya/gdb_tests/vector_thread_test...
(gdb) r
Starting program: /home/aditya/gdb_tests/vector_thread_test
Adding Arrays
Arr3[1] = { 2, 4, 6, 8 }
Adding Arrays
Arr3[1] = { 2, 4, 6, 8 }
Adding Arrays
Arr3[1] = { 2, 4, 6, 8 }
[New Thread 258]
[New Thread 515]
[New Thread 772]

Thread 2 received signal SIGINT, Interrupt.
[Switching to Thread 258]
thread_function (arg=0x0) at /home/aditya/gdb_tests/vector_thread_test.c:63
63        while (1); /* break here */
(gdb) info reg $vr0
vr0            {uint128 = 0x2000000040000000600000008, v4_float = {0x2, 0x4, 0x6, 0x8}, v4_int32 = {0x2, 0x4, 0x6, 0x8}, v8_int16 = {0x0, 0x2, 0x0, 0x4, 0x0, 0x6, 0x0, 0x8}, v16_int8 = {0x0, 0x0, 0x0, 0x2, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x6, 0x0, 0x0, 0x0, 0x8}}
(gdb) thread 2
[Switching to thread 2 (Thread 258)]
#0  thread_function (arg=
0x0) at /home/aditya/gdb_tests/vector_thread_test.c:63
63        while (1); /* break here */
(gdb) info reg $vr0
vr0            {uint128 = 0x2000000040000000600000008, v4_float = {0x2, 0x4, 0x6, 0x8}, v4_int32 = {0x2, 0x4, 0x6, 0x8}, v8_int16 = {0x0, 0x2, 0x0, 0x4, 0x0, 0x6, 0x0, 0x8}, v16_int8 = {0x0, 0x0, 0x0, 0x2, 0x0, 0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x6, 0x0, 0x0, 0x0, 0x8}}
(gdb)
----------------------------------------
Code 2:-
#include <stdio.h>
int main () {
vector int Arr1[5] = {
  { 1, 2, 3, 4 },
  { 11, 22, 33, 44 },
  { 111, 222, 333, 444 },
  { 1111, 2222, 3333, 4444 },
  { 11111, 22222, 33333, 44444 }
};
vector int Arr2[5] = {
  { 1, 2, 3, 4 },
  { 11, 22, 33, 44 },
  { 111, 222, 333, 444 },
  { 1111, 2222, 3333, 4444 },
  { 11111, 22222, 33333, 44444 }
};
vector int Arr3[5] = {
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 },
  { 0, 0, 0, 0 }
};
        printf("Adding Arrays\n");
        Arr3[1] = Arr1[2] + Arr2[3];
        printf("Arr3[1] = { %d, %d, %d, %d }\n", Arr3[1][0], Arr3[1][1], Arr3[1][2], Arr3[1][3]);
        return 0;
}
------------------------------
Sample output 2:-
Reading symbols from /home/aditya/gdb_tests/vector_lab_test...
(gdb) b main
Breakpoint 1 at 0x1000052c: file /home/aditya/gdb_tests/vector_lab_test.c, line 3.
(gdb) r
Starting program: /home/aditya/gdb_tests/vector_lab_test

Breakpoint 1, main () at /home/aditya/gdb_tests/vector_lab_test.c:3
3       vector int Arr1[5] = {
(gdb) n
10      vector int Arr2[5] = {
(gdb) n

17      vector int Arr3[5] = {
(gdb) n
24              printf("Adding Arrays\n");
(gdb) n
Adding Arrays
25              Arr3[1] = Arr1[2] + Arr2[3];
(gdb)
26              printf("Arr3[1] = { %d, %d, %d, %d }\n", Arr3[1][0], Arr3[1][1], Arr3[1][2], Arr3[1][3]);
(gdb) info reg $vr0
vr0            {uint128 = 0x4c60000098c00000e5200001318, v4_float = {0x4c6, 0x98c, 0xe52, 0x1318}, v4_int32 = {0x4c6, 0x98c, 0xe52, 0x1318}, v8_int16 = {0x0, 0x4c6, 0x0, 0x98c, 0x0, 0xe52, 0x0, 0x1318}, v16_int8 = {0x0, 0x0, 0x4, 0xc6, 0x0, 0x0, 0x9, 0x8c, 0x0, 0x0, 0xe, 0x52, 0x0, 0x0, 0x13, 0x18}}
(gdb)

From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Wednesday, 16 November 2022 at 12:33 AM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>, simark@simark.ca <simark@simark.ca>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable vector instruction debugging for AIX
Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>>I just noticed that there seem to be two methods, handling
>>user threads and kernel threads, respectively.  You patch
>>only adds support for VMX/VSX registers to kernel threads
>>-- should this also be done for user threads?  (Same applies
>>to store_regs_... of course.)
>
>Yes, this I have added in the new patch..

So you're now using ptrace here as well.  From what I
understand, this wrong - ptrace can only be used with
kernel threads.  For user threads, registers need to
be retrieved from the context managed by the thread
library and retrieved via pthdb_pthread_context.

Shouldn't the vector registers then also come from there?

Do you have a way to test with using user threads?


>>So if access to VMX registers fails because ptrace fails,
>>you now zero out the registers.  But if access fails because
>>__power_vmx() fails, the registers are left undefined.
>
>>Is this intentional?  What the meaning of this difference?
>
>So, if VMX register fails I will get an output like this below.
>Otherwise, I set to 0 like it is in all other places. So I am
>not doing any special handling..
>
>(gdb) info reg vr0
>vr0            {uint128 = <unavailable>, v4_float = {<unavailable>, <unavailable>, <unavailable>, <unavailable>}, v4_int32 = {<unavailable>, <unavailable>, <unavailable>, <unavailable>}, v8_int16 = {<unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>, <unavailable>}, v16_int8 = {<unavailable> <repeats 16 times>}}

I still don't quite understand the intended behavior.  As far
as I can see, there are three possible states:

- The machine doesn't have any vector registers
  (i.e. __power_vmx() returns 0)
- The machine does have vector registers, but the inferior
  process does not use them
  (i.e. __power_vmx() returns 1 but ptrace returns an error)
- The inferior process uses vector registers
  (i.e. no error from ptrace)

What is the intended behavior in those cases?  In particular,
should GDB behave differently between the first and the
second case?  Should GDB not show any vector registers at all,
should it show vector registers as <unavailable>, or should
it show vector registers containing zero?


>>>+      ret = rs6000_ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0);
>>>+    else
>>>+      ret = rs6000_ptrace32 (PTT_READ_VSX, thrd_i, (int *)&vsx, 0, 0);
>
>>Just noticed this here.  Everywhere else in this file, a *pid* is
>>passed to rs6000_ptrace64/32.  Only in this new routine, you're
>>apparently passing a thread ID.   Why should this be different just
>>for these registers?  Shouldn't per-thread registers be handled in
>>aix-thread.c anyway?
>
>Well, this is because in the documentation I read, I did not find something
>like a PT_READ_VSX that I can use. All I have is PTT_READ_VSX and in AIX
>PTT _* options are usually thread IDs.. I am restricted.. If it existed,
>I would have loved to use it. Like for writing general purpose registers
>we have PTT_WRITE_GPRS and PT_WRITE_GPRS and can use it as per convenience
>of thread or process..

Huh.  This is really strange, and doesn't quite map on the current GDB
implementation.  Everything related to thread IDs is currently done in
the aix-threads.c file.  It seems not really clean to mix this up.

How is this intended to be used?

If we need thread IDs anyway (and apparently we can always get a thread
ID even for a non-threaded process?), should we just use the aix-thread.c
methods for all processes?

As an aside, I don't see any documentation of PTT_READ_VSX either.
Do you know since when this is supported?


>>I think you should call this only if the requested regno
>>actually *is* a VMX or VSX register.  Also, if this is the
>>case, you don't need to do anything else in this routine.
>>This should be separate checks at the beginning of the
>>routine, just like you now do in store_register.
>
>I have taken care of it in the new patch..

+  if (altivec_register_p (gdbarch, regno))
+    fetch_altivec_registers_aix (regcache);
+  if (vsx_register_p (gdbarch, regno))
+    fetch_vsx_registers_aix (regcache);

I meant that you should skip everything else in those cases:

  if (altivec_register_p (gdbarch, regno))
    {
      fetch_altivec_registers_aix (regcache);
      return;
    }
  if (vsx_register_p (gdbarch, regno))
    {
      fetch_vsx_registers_aix (regcache);
      return;
    }

just like in store_register.



>>Since for the stxvd2x instruction, the last bit is part of the
>>register number, these two lines should be merged into:
> >             || (op & 0xfc1ffffe) == 0x7c012f98 /* stxvd2x Vs, r1, r5 */
>
>>As above, the comment needs to be updated to match.

You should still do this (the 0x...e mask, and the comment).

>>This means that on older AIX versions, GDB simply won't work anymore,
>>right?  If so, I guess I'll leave if up to you to decide whether this
>>is acceptable.  But the minimum supported version should be documented
>>somewhere (there's still at least mention of AIX 4.x being supported).
>>Also, if we do switch the minimum supported version, I think there's
>>quite a bit of existing code intended to handle old versions that could
>>then just be removed ...
>
>How would you like me to document this. Let me know.. And we can work
>out if possible, via another patch to remove handling the old version codes.

Well, at the very least this needs to be announced in the NEWS file.

This still mentions AIX 4 being supported.


>>Should we change the default for "Motorola PowerPC 620"
>>at this stage?  Can we even test on that machine?
>>>ANS = No. No we cannot test on that machine.
>
>I have removed the same in this patch...

There's still this one in there:

   {"power", "POWER user-level", bfd_arch_rs6000,
-   bfd_mach_rs6k, &tdesc_rs6000},
+   bfd_mach_rs6k, &tdesc_powerpc_vsx32},

I don't think this should be needed either.

Bye,
Ulrich