Re: [PATCH] Enable vector instruction debugging for AIX

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>>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 (). 

I notice that the other registers are guarded via
PTHDB_FLAG_GPRS and similar flags.  Is there not any
equivalent flag for VMX / VSX registers?


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

Well, but in your new rs6000_nat_target::read_description
routine, you *do* in fact say that *every* machine always
has VMX and VSX registers.

If you want those registers to simply not be present at
all on machines that do not have them, read_description
should choose an appropriate regset based on __power_vmx
and __power_vsx.

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

I think this is different between AIX and Linux then.  If
you cannot write anything to the register, I agree it makes
sense to show it as "unavailable".


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

I see.  Still, the rs6000-aix-nat.c routines should only be called
for single-threaded processes, right?  For these processes, does the
kernel thread ID come from a different name space as the process ID,
or are those IDs just the same (they would be on Linux)?

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

What's supposed to happen is that for single-threaded processes,
fetch_registers in rs6000-aix-nat.c is called; while for multi-
threaded processes the routine in aix-thread.c is called.  The
latter will handle either user threads or kernel threads.

Looking at the big picture, I think the best way to remove the
duplication would be to move all handling of *kernel* threads
to rs6000-aix-nat.c, and leave only user threads in aix-threads.c.

That would mean using all three fields in ptid_t:
- pid should hold the pid
- lwp should hold the kernel thread ID (tid_t or pthdb_tid_t)
- tid should hold the user thread ID (pthdb_pthread_t)

That would match the Linux precedent, and would allow all
ptrace-related access to be done completely in rs6000-aix-nat.c
(using the lwp / tid_t for register access), and leave only
the context-based user thread access in aix-threads.c.

But I guess this could be done as a future cleanup.

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

Right now in this line you're trying to add:
+               || (op & 0xfc1fffff) == 0x7c012f98) /* stxvd2x vs0, r1, r5 */
the code does not match the comment.

As I do not know why you're making this change in the
first place, I cannot tell whether the code is correct
and the comment is wrong or vice versa. (Or maybe neither
is fully correct?)

Looking at the ISA document, we see this documentation
of the stxvd2x instruction:

  stxvd2x XS,RA,RB

  bit  0- 5: constant 31
  bit  6-10: S
  bit 11-15: RA
  bit 16-20: RB
  bit 21-30: constant 972
  bit 31:    SX

where RA and RB are 5-bit GPR numbers (0..31), while
XS is a 6-bit VSX register number (0..63), split into
the highest bit SX and the low 5 bits S.

If you want to match this instruction, you'll have to
verify that the two constant fields have the expected
value.  If you also want to restrict the match to certain
values or ranges for the RA, RB, and XS register numbers,
you have to match those as well.  This is where the current
masking / comparsion looks inconsistent to me.

Splitting your mask and comparison values up into the
bit fields are defined above, we have:

  mask:       0xfc1fffff
  comparison: 0x7c012f98

  bit  0- 5: mask all ones  - comparison 31
  bit  6-10: mask all zeros - comparison 0
  bit 11-15: mask all ones  - comparison 1
  bit 16-20: mask all ones  - comparison 5
  bit 21-30: mask all ones  - comparison 972
  bit 31:    mask all ones  - comparison 0

So, this enforces that the constant fields have the
correct values (good).  It also enforces that RA == 1
and RB == 5.  I guess this at least matches the comment,
but I do not understand the reason for matching just
this register combination.  (I guess RA == 1 is supposed
to match the stack pointer, but why RB == 5 ?)

However, where it gets weird is the XS field:  the
comment says it looks only for VS0.  But the test
actually allows many more vector registers here:
specifically, it allows any value for the low 5 bits,
and forces the high bit to zero. This means that the
test will match for VSX registers 0..32, but not for
VSX registers 33..63.  This seems strange.

Again, my primary question would be, why are you making
this change at all?  Based on that, what is the exact
set of possible instructions you want to match here?
Given that, it should be straightforward to construct
the correct mask / comparison values to implement that
choice.

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

I think as part of this patch, you should update the NEWS
file with the supported AIX versions going forward.

(Note that it would be good to actually verify that the
debugger builds and works correctly on all the versions
that we declare as supported.)

Some additional comments on the patch:

>+  /* vector registers.  */
>+  __vmx_context_t vmx;
>+  if (__power_vmx())
>+  {
>+    if (data->arch64)
>+      if (!ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0))
>+	memcpy (&context->vmx, &vmx, sizeof(__vmx_context_t));
>+    else
>+      if (!ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0))
>+	memcpy (&context->vmx, &vmx, sizeof(__vmx_context_t));
>+  }

The other registers are filled with zeros if ptrace fails,
why not here?

>+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;            
>+  for (regno = 0; regno < num_of_vrregs; regno++)
>+     regcache->raw_supply (tdep->ppc_vr0_regnum + regno, 
>+			   &(vmx.__vr[regno]));

Not sure about the contents of the vmx structure, but I'm
wondering if this is correct for the VRSAVE and VSCR
registers.  Are they part of the __vr array, or are they
in separate fields?

>+  for (regno = 0; regno < ppc_num_vshrs; regno++)
>+     regcache->raw_supply (tdep->ppc_vsr0_regnum + regno, 
>+                           &(vsx.__vsr_dw1[regno]));

This should only supply ppc_vsr0_upper_regnum etc.  (upper halves)
not the full vsr0.

 
>+      if (__power_vmx())
>+      {
>+        if (data->arch64)
>+          ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
>+        else
>+          ret = ptrace32 (PTT_READ_VEC, tid, (uintptr_t) &vmx, 0, 0);
>+        if (ret < 0)
>+          memset(&vmx, 0, sizeof(__vmx_context_t));
>+        for (i = 0; i < num_of_vrregs; i++)
>+          regcache->raw_supply (tdep->ppc_vr0_regnum + i, &(vmx.__vr[i]));
>+      }

This is strange - if ptrace fails, the fields are zeroed, but if
__power_vmx is false, they're just left uninitialized.  What is
even the point of the __power_vmx check in the first place here?^

>+  int ret;
>+  __vmx_context_t *vmx;
>+  __vsx_context_t  *vsx;
>+  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
> 
>   if (debug_aix_thread)
>     gdb_printf (gdb_stdlog, 
>@@ -1594,6 +1751,40 @@ store_regs_user_thread (const struct regcache *regcache, pthdb_pthread_t pdtid)
>     error (_("aix-thread: store_registers: pthdb_pthread_context returned %s"),
> 	   pd_status2str (status));
> 
>+  /* Fill altivec-registers.  */
>+
>+  if (__power_vmx())
>+  {
>+    memset(&vmx, 0, sizeof(__vmx_context_t));

This is wrong, and a serious memory corruption.  If you do this, vmx 
needs to be declared as __vmx_context_t, not __vmx_context_t *.

>+      if (__power_vmx())
>+      {
>+        if (data->arch64)
>+          ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
>+        else
>+          ret = ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
>+        if (ret < 0)
>+        {
>+          if (errno == ENXIO)
>+          {
>+            warning (_("Unable to fetch AltiVec registers."));
>+            memset(&vmx, 0, sizeof(__vmx_context_t)); 
>+          }
>+          fill_altivec(regcache, &vmx);
>+          if (data->arch64)
>+            ret = ptrace64aix (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
>+          else
>+            ret = ptrace32 (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);

Huh?  So you update with new values only if reading the old values
*failed*?  That logic looks clearly wrong here.

>+/* Header files for alti-vec reg.  */
>+#include <sys/context.h>
>+#include <sys/pthdebug.h>

Do you really need pthdebug.h here?  Even if we need to handle threads
here, it will only be *kernel* threads, so this shouldn't be needed.

>+  tid64_t  thrd_i = 0;
>+
>+  if (getthrds64(pid, &thrdentry, sizeof(struct thrdentry64),
>+                                            &thrd_i, 1) == 1)
>+    thrd_i = thrdentry.ti_tid;

So if this call fails, thrd_i just remains 0?  How does this
work, or what's the point of this?

>+const struct target_desc *
>+rs6000_nat_target::read_description ()
>+{
>+   if (ARCH64())
>+      return tdesc_powerpc_vsx64;
>+   else
>+      return tdesc_powerpc_vsx32;
>+}

See above - I think this needs to check whether we actually have
VMX and/or VSX registers.

>-      else if ((op & 0xfc1fffff) == 0x7c1f01ce)   /* stvx Vs, R31, R0 */
>+      else if ((op & 0xfc1fffff) == 0x7c1f01ce   /* stvx Vs, R31, R0 */
>+		|| (op & 0xfc1fffff) == 0x7c012f98) /* stxvd2x vs0, r1, r5 */

See discussion above.

Also, please ensure you're following the GNU coding style
throughout, and consistently follow the whitespace rules
(tab instead of 8 spaces; no whitespace at end of line).


Bye,
Ulrich