Hi all, Thank you for the feedback. Please find attached the new patch. [See: 0001-Enable-Vector-instruction-debugging-support-for-AIX.patch ] Kindly let me know if any changes required. I have answered point by point for the feedback below. Please find below. Have a nice day ahead. Thanks and regards, Aditya. --------------------------------------------------------------------------------------------------------------- >- >- if (tdep->ppc_mq_regnum >= 0) >- regcache->raw_supply (tdep->ppc_mq_regnum, (char *) &sprs32.pt_mq); Why remove this support? That looks unrelated. Ans = Agreed. I made a mistake. I have corrected the same. Coding style, should use full sentence here (and elsewhere), e.g.: /* Vector registers. */ Ans = Taken care in all comments. Are these types/functions (__vmx_context_t, __power_vmx) available unconditionally, or does there need to be some configure check (to avoid breaking compilation on older AIX version for example). Ans = They are all available since AIX version 5.3. So it should not break. The rest of the code uses "ptrace64aix" to use either ptrace64 or ptracex depending on what's available. Why is this not needed here? Ans = Taken care in complete aix-thread.c file changes In the rest of this function, a failing ptrace call causes the registers to be initialized to zero. Why is it different here? Ans = You were right I have taken care of it. See memset () function. Please move the "fill" routines next to the other "fill" routines. Also, add a comment before the routine like with the others. Ans = I have done it. >+#include "gdbthread.h" What is this needed for here? Ans = I have removed it. >+#include >+#include Are these available unconditionally? ANS = Available from AIX 5.3 >+int have_ptrace_getvrregs = 1; >+int have_ptrace_getsetvsxregs = 1; These aren't handled consistenly in the rest of the code. You should decide how you plan to handle the non-availability of these ptrace features (silently ignore, treat as zero, treat as unavailable) and then handle them consistently. ANS = Taken care and eliminated. Thank you for the suggestion. + ret = ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0); Same question as in aix-thread.c - should this check ARCH64 to see if ptrace64 is even available? ANS = Taken care here as well See above. Final patches submitted upstream should never have any code commented out - please resolve these questions before. ANS = removed. Thanks > nr = regmap (gdbarch, regno, &isfloat); > >+ if (altivec_register_p (gdbarch, regno)) >+ { >+ store_altivec_register_aix (regcache, regno); >+ return; >+ } >+ if (vsx_register_p (gdbarch, regno)) >+ { >+ store_vsx_register_aix (regcache, regno); >+ return; >+ } If you do it this way, it should be *before* the regmap call (which doesn't handle any of these registers.) ANS = Implemented. Thanks >- if (regno != -1) >- fetch_register (regcache, regno); >- >- else This doesn't look right? ANS = Yes, I understood that sometimes when we know what register to write we need it. I have made some changed here in the patch. +const struct target_desc * >+rs6000_nat_target::read_description () >+{ >+ if (ARCH64()) >+ return tdesc_powerpc_vsx64; >+ else >+ return tdesc_powerpc_vsx32; >+} Well, is this true? Shouldn't this check whether the machine actually *has* VSX, and return a different tdesc otherwise? ANS = AIX 5.3 onwards we get those _vmx_context_t, __power_vmx variables to support these target descriptions. Whitespace only change - please remove. ANS = Done Are these helpers really needed? In other places the same check is just performed inline. This should be consistent everywhere. ANS = Done for (i = tdep->ppc_vsr0_upper_regnum; >+ i < tdep->ppc_vsr0_upper_regnum + 32; >+ i++, offset++) >+ ppc_supply_reg (regcache, i, (const gdb_byte *) vsxregs, offset * 8, 8); Formatting. ANS = I have taken care. But in case there is an update for large for loop statements let me know. >+ ppc_supply_reg (regcache, i, (const gdb_byte *) vsxregs, offset * 8, 8); Formatting. Also you should be consistent with code above and use offset += 8 instead of the multiplication. ANS = Done >+ ppc_collect_reg (regcache, i, (gdb_byte *) vsxregs, 0, 8); This looks wrong - it always uses offset zero? ANS = Corrected >+ if (have_altivec) >+ cb (".aix-vmx", 560, 560, &rs6000_aix32_vrregset, "AIX altivec", cb_data); >+ >+ if (have_vsx) >+ cb (".aix-vsx", 256, 256, &rs6000_aix32_vsxregset, "AIX vsx", cb_data); As those regsets are the same between 32- and 64-bit, they should probably be just called rs6000_aix_vrregset etc. ANS = No, their offsets are different. Hence, we have different variables for them. >- { >- if (pc == (li_found_pc + 4)) >- { >- vr_reg = GET_SRC_REG (op); >- /* If this is the first vector reg to be saved, or if >- it has a lower number than others previously seen, >- reupdate the frame info. */ >- if (fdata->saved_vr == -1 || fdata->saved_vr > vr_reg) >- { >- fdata->saved_vr = vr_reg; >- fdata->vr_offset = vr_saved_offset + offset; >- } >- vr_saved_offset = -1; >- vr_reg = -1; >- li_found_pc = 0; >- } So I don't know enough about the details of the prologue generated by the various compilers to understand exactly what's going on here, but this change looks wrong - it now completely ignores the "li_found_pc", so why even compute it? More specifically, the intention of the previous code was to only look for the first instances of stvx* in the prolog at a particular place (because later instances might serve a different purpose). This change just ignores all this. ANS = I agree, I got this wrong in the previous patch. I have corrected it now. Kindly let me know. >- bfd_mach_ppc_620, &tdesc_powerpc_64}, >+ bfd_mach_ppc_620, &tdesc_powerpc_vsx64}, 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. > /* if != -1, fdata.saved_vr is the smallest number of saved_vr. > All vr's from saved_vr to vr31 are saved. */ > if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1) >- { >- if (fdata.saved_vr >= 0) >- { >- int i; >- CORE_ADDR vr_addr = cache->base + fdata.vr_offset; >- for (i = fdata.saved_vr; i < 32; i++) >- { >- cache->saved_regs[tdep->ppc_vr0_regnum + i].set_addr (vr_addr); >- vr_addr += register_size (gdbarch, tdep->ppc_vr0_regnum); >- } >- } >- } >+ { >+ int i; >+ CORE_ADDR vr_addr = cache->base + fdata.vr_offset; >+ for (i = 0; i < 66; i++) >+ { >+ cache->saved_regs[tdep->ppc_vsr0_upper_regnum + i].set_addr (vr_addr); >+ vr_addr += register_size (gdbarch, tdep- >ppc_vsr0_upper_regnum); >+ } >+ } This looks wrong: - It completely removes loading stored values for altivec registers - It always loads VSX registers even if they haven't been saved - It uses up to 66 "VSX upper-half" registers even though only ANS = I got this wrong in the previous patch. I have corrected it now. Kindly let me know. ________________________________ From: Ulrich Weigand Sent: 14 October 2022 18:11 To: gdb-patches@sourceware.org ; Aditya Kamath1 ; simark@simark.ca Cc: Sangamesh Mallayya Subject: Re: [PATCH] Enable vector instruction debugging for AIX Aditya Kamath1 wrote: >Please find attached the patch. [See 0001-PATCH-Enable-Vector-support- for-AIX.patch] Some comments on the patch inline. >diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c >index 57e5756e144..15400f5b90d 100644 --- a/gdb/aix-thread.c >+++ b/gdb/aix-thread.c >@@ -1347,11 +1347,75 @@ fetch_regs_kernel_thread (struct regcache *regcache, int regno, > supply_sprs32 (regcache, sprs32.pt_iar, sprs32.pt_msr, sprs32.pt_cr, > sprs32.pt_lr, sprs32.pt_ctr, sprs32.pt_xer, > sprs32.pt_fpscr); >- >- if (tdep->ppc_mq_regnum >= 0) >- regcache->raw_supply (tdep->ppc_mq_regnum, (char *) &sprs32.pt_mq); Why remove this support? That looks unrelated. >+ /* vector registers */ Coding style, should use full sentence here (and elsewhere), e.g.: /* Vector registers. */ >+ if (tdep->ppc_vr0_regnum != -1 && regno >= tdep->ppc_vr0_regnum) >+ { >+ int ret = 0; >+ int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep- >ppc_vr0_regnum + 1; >+ __vmx_context_t vmx; >+ memset(&vmx, 0, sizeof(__vmx_context_t)); >+ if (__power_vmx()) Are these types/functions (__vmx_context_t, __power_vmx) available unconditionally, or does there need to be some configure check (to avoid breaking compilation on older AIX version for example). >+ { >+ ret = ptrace64 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0); The rest of the code uses "ptrace64aix" to use either ptrace64 or ptracex depending on what's available. Why is this not needed here? >+ if (ret < 0) >+ { >+ if (errno == ENXIO) >+ return; >+ perror_with_name (_("Unable to fetch AltiVec registers")); >+ } In the rest of this function, a failing ptrace call causes the registers to be initialized to zero. Why is it different here? >+static void >+fill_altivec (const struct regcache *regcache, __vmx_context_t *vmx) Please move the "fill" routines next to the other "fill" routines. Also, add a comment before the routine like with the others. >diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c >index cb141427696..e989a9a6739 100644 >--- a/gdb/rs6000-aix-nat.c >+++ b/gdb/rs6000-aix-nat.c >@@ -54,6 +54,19 @@ > #include > #include > >+#include "gdbthread.h" What is this needed for here? >+#include >+#include Are these available unconditionally? >+int have_ptrace_getvrregs = 1; >+int have_ptrace_getsetvsxregs = 1; These aren't handled consistenly in the rest of the code. You should decide how you plan to handle the non-availability of these ptrace features (silently ignore, treat as zero, treat as unavailable) and then handle them consistently. >+static void >+store_vsx_register_aix (struct regcache *regcache, int regno) >+{ >+ int ret; >+ struct gdbarch *gdbarch = regcache->arch (); >+ ppc_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >+ struct thrdentry64 thrdentry; >+ __vsx_context_t vsx; >+ pid_t pid = inferior_ptid.pid (); >+ tid64_t thrd_i = 0; >+ >+ if (getthrds64(pid, &thrdentry, sizeof(struct thrdentry64), >+ &thrd_i, 1) == 1) >+ thrd_i = thrdentry.ti_tid; >+ memset(&vsx, 0, sizeof(__vsx_context_t)); >+ if (__power_vsx()) >+ { >+ ret = ptrace64 (PTT_READ_VSX, thrd_i, (long long) &vsx, 0, 0); Same question as in aix-thread.c - should this check ARCH64 to see if ptrace64 is even available? It's strange to have those checks everywhere else in this file but not here. Similar question whether getthrds64 etc. are available unconditionally. >+ if (ret < 0) >+ { >+ if (errno == ENXIO) >+ { >+ /* have_ptrace_getsetvsxregs = 0; */ >+ warning (_("Unable to fetch VSX registers.")); >+ return; >+ } See above. Final patches submitted upstream should never have any code commented out - please resolve these questions before. >@@ -264,6 +355,17 @@ store_register (struct regcache *regcache, int regno) > > nr = regmap (gdbarch, regno, &isfloat); > >+ if (altivec_register_p (gdbarch, regno)) >+ { >+ store_altivec_register_aix (regcache, regno); >+ return; >+ } >+ if (vsx_register_p (gdbarch, regno)) >+ { >+ store_vsx_register_aix (regcache, regno); >+ return; >+ } If you do it this way, it should be *before* the regmap call (which doesn't handle any of these registers.) >@@ -312,10 +506,6 @@ void > rs6000_nat_target::fetch_registers (struct regcache *regcache, int regno) > { > struct gdbarch *gdbarch = regcache->arch (); >- if (regno != -1) >- fetch_register (regcache, regno); >- >- else This doesn't look right? >+ if (have_ptrace_getvrregs) >+ if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1) >+ { >+ fetch_altivec_registers_aix (regcache); >+ } >+ >+ if (have_ptrace_getsetvsxregs) >+ if (tdep->ppc_vsr0_upper_regnum != -1) >+ { >+ fetch_vsx_registers_aix (regcache); >+ } No need for { } when it's a single statement. >+const struct target_desc * >+rs6000_nat_target::read_description () >+{ >+ if (ARCH64()) >+ return tdesc_powerpc_vsx64; >+ else >+ return tdesc_powerpc_vsx32; >+} Well, is this true? Shouldn't this check whether the machine actually *has* VSX, and return a different tdesc otherwise? >@@ -145,7 +161,7 @@ aix_sighandle_frame_prev_register (frame_info_ptr this_frame, > > static int > aix_sighandle_frame_sniffer (const struct frame_unwind *self, >- frame_info_ptr this_frame, >+ frame_info_ptr this_frame, > void **this_prologue_cache) > { > CORE_ADDR pc = get_frame_pc (this_frame); Whitespace only change - please remove. >+/* Return non-zero if the architecture described by GDBARCH has >+ VSX registers (vsr0 --- vsr63). */ >+static int >+rs6000_aix_vsx_support_p (struct gdbarch *gdbarch) >+{ >+ ppc_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >+ return tdep->ppc_vsr0_regnum >= 0; >+} >+ >+/* Return non-zero if the architecture described by GDBARCH has >+ Altivec registers (vr0 --- vr31, vrsave and vscr). */ >+static int >+rs6000_aix_altivec_support_p (struct gdbarch *gdbarch) >+{ >+ ppc_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >+ return (tdep->ppc_vr0_regnum >= 0 >+ && tdep->ppc_vrsave_regnum >= 0); >+} Are these helpers really needed? In other places the same check is just performed inline. This should be consistent everywhere. >+ for (i = tdep->ppc_vr0_regnum, offset = offsets->vr0_offset; >+ i < tdep->ppc_vr0_regnum + ppc_num_vrs; >+ i++, offset += 16) Formatting. >+ for (i = tdep->ppc_vsr0_upper_regnum; >+ i < tdep->ppc_vsr0_upper_regnum + 32; >+ i++, offset++) >+ ppc_supply_reg (regcache, i, (const gdb_byte *) vsxregs, offset * 8, 8); Formatting. Also you should be consistent with code above and use offset += 8 instead of the multiplication. >+ for (i = tdep->ppc_vsr0_upper_regnum; >+ i < tdep->ppc_vsr0_upper_regnum + 32; >+ i++) >+ ppc_collect_reg (regcache, i, (gdb_byte *) vsxregs, 0, 8); This looks wrong - it always uses offset zero? >@@ -262,10 +462,19 @@ rs6000_aix_iterate_over_regset_sections (struct gdbarch *gdbarch, > const struct regcache *regcache) > { > ppc_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >+ int have_altivec = tdep->ppc_vr0_regnum != -1; >+ int have_vsx = tdep->ppc_vsr0_upper_regnum != -1; >+ > if (tdep->wordsize == 4) > cb (".reg", 592, 592, &rs6000_aix32_regset, NULL, cb_data); > else > cb (".reg", 576, 576, &rs6000_aix64_regset, NULL, cb_data); >+ >+ if (have_altivec) >+ cb (".aix-vmx", 560, 560, &rs6000_aix32_vrregset, "AIX altivec", cb_data); >+ >+ if (have_vsx) >+ cb (".aix-vsx", 256, 256, &rs6000_aix32_vsxregset, "AIX vsx", cb_data); As those regsets are the same between 32- and 64-bit, they should probably be just called rs6000_aix_vrregset etc. >@@ -351,7 +560,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > > arg = args[argno]; > type = check_typedef (value_type (arg)); >- len = type->length (); >+ len = type->length (); Another whitespace-only change, please remove. >index 8b6d666bbe7..6e8ffc5fb0d 100644 >--- a/gdb/rs6000-tdep.c >+++ b/gdb/rs6000-tdep.c >@@ -1972,7 +1972,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > /* 001110 00000 00000 iiii iiii iiii iiii */ > /* 001110 01110 00000 iiii iiii iiii iiii */ > else if ((op & 0xffff0000) == 0x38000000 /* li r0, SIMM */ >- || (op & 0xffff0000) == 0x39c00000) /* li r14, SIMM */ >+ || (op & 0xffff0000) == 0x39c00000 /* li r14, SIMM */ >+ || (op & 0xff0f0000) == 0x38000000) /* li, r4, SIMM */ > { > if ((op & 0xffff0000) == 0x38000000) > r0_contains_arg = 0; >@@ -1986,24 +1987,21 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > } > /* Store vector register S at (r31+r0) aligned to 16 bytes. */ > /* 011111 sssss 11111 00000 00111001110 */ >- else if ((op & 0xfc1fffff) == 0x7c1f01ce) /* stvx Vs, R31, R0 */ >- { >- if (pc == (li_found_pc + 4)) >- { >- vr_reg = GET_SRC_REG (op); >- /* If this is the first vector reg to be saved, or if >- it has a lower number than others previously seen, >- reupdate the frame info. */ >- if (fdata->saved_vr == -1 || fdata->saved_vr > vr_reg) >- { >- fdata->saved_vr = vr_reg; >- fdata->vr_offset = vr_saved_offset + offset; >- } >- vr_saved_offset = -1; >- vr_reg = -1; >- li_found_pc = 0; >- } >- } >+ else if ((op & 0xfc1fffff) == 0x7c1f01ce /* stvx Vs, R31, R0 */ >+ || (op & 0xfc1fffff) == 0x7c012f99 /* stxvd2x Vs,r1, r5 */ >+ || (op & 0xfc1fffff) == 0x7c012f98 /* stxvd2x vs0, r1, r5 */ >+ || (op & 0xfc1ff000) == 0x7c012000) >+ { >+ vr_reg = GET_SRC_REG (op); >+ /* If this is the first vector reg to be saved, or if >+ it has a lower number than others previously seen, >+ reupdate the frame info. */ >+ fdata->saved_vr = vr_reg; >+ fdata->vr_offset = vr_saved_offset; >+ vr_saved_offset = -1; >+ vr_reg = -1; >+ li_found_pc = 0; >+ } So I don't know enough about the details of the prologue generated by the various compilers to understand exactly what's going on here, but this change looks wrong - it now completely ignores the "li_found_pc", so why even compute it? More specifically, the intention of the previous code was to only look for the first instances of stvx* in the prolog at a particular place (because later instances might serve a different purpose). This change just ignores all this. Is prologue parsing even still necessary on AIX? On Linux these days we should always get this info via DWARF CIF. >@@ -2665,8 +2663,8 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, > && regnum >= tdep->ppc_fp0_regnum > && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs > && type->code () == TYPE_CODE_FLT >- && (type->length () >- != builtin_type (gdbarch)->builtin_double->length ())); >+ && (type->length () >+ != builtin_type (gdbarch)->builtin_double->length ())); Another white-space only change. >@@ -3466,7 +3464,7 @@ struct ppc_variant > unsigned long mach; > > /* Target description for this variant. */ >- const struct target_desc **tdesc; >+ struct target_desc **tdesc; This looks just wrong. >@@ -3504,7 +3502,7 @@ static struct ppc_variant variants[] = > {"powerpc64", "PowerPC 64-bit user-level", bfd_arch_powerpc, > bfd_mach_ppc64, &tdesc_powerpc_altivec64}, > {"620", "Motorola PowerPC 620", bfd_arch_powerpc, >- bfd_mach_ppc_620, &tdesc_powerpc_64}, >+ bfd_mach_ppc_620, &tdesc_powerpc_vsx64}, Should we change the default for "Motorola PowerPC 620" at this stage? Can we even test on that machine? >@@ -3680,18 +3678,15 @@ rs6000_frame_cache (frame_info_ptr this_frame, void **this_cache) > /* if != -1, fdata.saved_vr is the smallest number of saved_vr. > All vr's from saved_vr to vr31 are saved. */ > if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1) >- { >- if (fdata.saved_vr >= 0) >- { >- int i; >- CORE_ADDR vr_addr = cache->base + fdata.vr_offset; >- for (i = fdata.saved_vr; i < 32; i++) >- { >- cache->saved_regs[tdep->ppc_vr0_regnum + i].set_addr (vr_addr); >- vr_addr += register_size (gdbarch, tdep->ppc_vr0_regnum); >- } >- } >- } >+ { >+ int i; >+ CORE_ADDR vr_addr = cache->base + fdata.vr_offset; >+ for (i = 0; i < 66; i++) >+ { >+ cache->saved_regs[tdep->ppc_vsr0_upper_regnum + i].set_addr (vr_addr); >+ vr_addr += register_size (gdbarch, tdep- >ppc_vsr0_upper_regnum); >+ } >+ } This looks wrong: - It completely removes loading stored values for altivec registers - It always loads VSX registers even if they haven't been saved - It uses up to 66 "VSX upper-half" registers even though only 32 of those exist. - Are there even any call-saved VSX registers in the AIX ABI? In the Linux ABI there aren't. Bye, Ulrich