* PowerPC, Fix-test-gdb.base-store.exp @ 2023-10-12 14:51 Carl Love 2023-10-12 14:58 ` [Patch 1/2] " Carl Love 2023-10-12 15:00 ` [PATCH 2/2] " Carl Love 0 siblings, 2 replies; 27+ messages in thread From: Carl Love @ 2023-10-12 14:51 UTC (permalink / raw) To: gdb-patches, UlrichWeigand; +Cc: cel GDB maintainers: The gdb.base/store.exp test has five failures on PowerPC. Part of the issue is the PowerPC DWARF mapping is not correct on Linux. The first patch in this series fixes the PowerPC DWARF mapping. Once the mapping was fixed another issue was found with how the signal handlers were handled on PowerPC. The issue is on PowerPC the sequence of events for handling signals is a little different on PowerPC from most other architectures. The first patch fixes the PowerPC DWARF register mapping and the signal handling issue on PowerPC. These changes fix three of the five failures on the store.exp test. The second patch fixes the remaining issue for the store.exp test. The remaining issues are due to handling the 128-bit floating point values. The 64-bit floating point values are stored in the floating point registers while the 128-bit floating point values are stored in a VSR. The DWARF uses the floating point register numbers for both the 64-bit and 128-bit floating point values. The second patch adds a routine to check if a value is a 128-bit floating point value, if so it then adjusts the register mapping to point to the VSR registers instead of the 64-bit floating point registers. The series of two patches have been tested on PowerPC LE and BE. The patches have also been tested on Intel X86-64. No regression failures was found. Please let me know if these patches are acceptable for mainline. Thanks. Carl ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 14:51 PowerPC, Fix-test-gdb.base-store.exp Carl Love @ 2023-10-12 14:58 ` Carl Love 2023-10-13 20:34 ` Keith Seitz 2023-10-16 14:31 ` Andrew Burgess 2023-10-12 15:00 ` [PATCH 2/2] " Carl Love 1 sibling, 2 replies; 27+ messages in thread From: Carl Love @ 2023-10-12 14:58 UTC (permalink / raw) To: gdb-patches, UlrichWeigand; +Cc: cel GDB maintainers: This is the first patch in the series which fixes the DWWARF register mapping and signal handling issues on PowerPC. Carl ----------------------------------------------- rs6000, Fix Linux DWARF register mapping The PowerPC DWARF register mapping is the same for the .eh_frame and .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and .debug_frame on other operating systems. The current GDB support for mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. The files have some legacy mappings for spe_acc, spefscr, EV which was removed from GCC in 2017. This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle the DWARF register mappings on Linux. Function rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum and gdbarch_stab_reg_to_regnum since the mappings are the same. The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to call set_gdbarch_dwarf2_reg_to_regnum map the new function rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, dwarf2_frame_set_adjust_regnum is called to map rs6000_linux_adjust_frame_regnum into the architecture. The second issue fixed by this patch is the check for subroutine process_event_stop_test. Need to make sure the frame is not the SIGTRAMP_FRAME. The sequence of events on most platforms is: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the handler. ... However on PowerPC the sequence of events is: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the trampoline. 3) The trampoline performs a normal function call to the handler. ... We want "nexti" to step into, not over, signal handlers invoked by the kernel. This is the case most platforms as the kernel puts a signal trampoline frame onto the stack to handle proper return after the handler. However, on some platforms such as PowerPC, the kernel actually uses a trampoline to handle *invocation* of the handler. The issue is fixed in function process_event_stop_test by adding a check that the frame is not a SIGTRAMP_FRAME to the if statement to stop in a subroutine call. This prevents GDB from erroneously detecting the trampoline invocation as a subroutine call. This patch fixes two regression test failures in gdb.base/store.exp. It also fixes two regression failures in gdb.python/py-thread-exited.exp. Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no new regressions. --- gdb/infrun.c | 13 ++++++++++ gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/gdb/infrun.c b/gdb/infrun.c index 4730d290442..922d8a6913d 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) initial outermost frame, before sp was valid, would have code_addr == &_start. See the comment in frame_id::operator== for more. */ + + /* We want "nexti" to step into, not over, signal handlers invoked + by the kernel, therefore this subroutine check should not trigger + for a signal handler invocation. On most platforms, this is already + not the case, as the kernel puts a signal trampoline frame onto the + stack to handle proper return after the handler, and therefore at this + point, the current frame is a grandchild of the step frame, not a + child. However, on some platforms, the kernel actually uses a + trampoline to handle *invocation* of the handler. In that case, + when executing the first instruction of the trampoline, this check + would erroneously detect the trampoline invocation as a subroutine + call. Fix this by checking for SIGTRAMP_FRAME. */ if ((get_stack_frame_id (frame) != ecs->event_thread->control.step_stack_frame_id) + && get_frame_type (frame) != SIGTRAMP_FRAME && ((frame_unwind_caller_id (get_current_frame ()) == ecs->event_thread->control.step_stack_frame_id) && ((ecs->event_thread->control.step_stack_frame_id diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 784dafa59db..7fb90799dff 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -83,6 +83,7 @@ #include "features/rs6000/powerpc-isa207-vsx64l.c" #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" #include "features/rs6000/powerpc-e500l.c" +#include "dwarf2/frame.h" /* Shared library operations for PowerPC-Linux. */ static struct target_so_ops powerpc_so_ops; @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, return per_inferior->disp_step_buf->prepare (thread, displaced_pc); } +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ +static int +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); + + if (0 <= num && num <= 31) + return tdep->ppc_gp0_regnum + num; + else if (32 <= num && num <= 63) + /* FIXME: jimb/2004-05-05: What should we do when the debug info + specifies registers the architecture doesn't have? Our + callers don't check the value we return. */ + return tdep->ppc_fp0_regnum + (num - 32); + else if (77 <= num && num < 77 + 32) + return tdep->ppc_vr0_regnum + (num - 77); + else + switch (num) + { + case 65: + return tdep->ppc_lr_regnum; + case 66: + return tdep->ppc_ctr_regnum; + case 76: + return tdep->ppc_xer_regnum; + case 109: + return tdep->ppc_vrsave_regnum; + case 110: + return tdep->ppc_vrsave_regnum - 1; /* vscr */ + } + + /* Unknown DWARF register number. */ + return -1; +} + +/* Translate a .eh_frame register to DWARF register, or adjust a + .debug_frame register. */ + + +static int +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, + int eh_frame_p) +{ + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ + return num; +} + static void ppc_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); set_gdbarch_stap_parse_special_token (gdbarch, ppc_stap_parse_special_token); + /* Linux DWARF register mapping is different from the othe OS's. */ + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + /* Note on Linux the mapping for the DWARF registers and the stab registers + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the + stab register mappings as well. */ + set_gdbarch_stab_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); if (tdep->wordsize == 4) { -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 14:58 ` [Patch 1/2] " Carl Love @ 2023-10-13 20:34 ` Keith Seitz 2023-10-13 21:00 ` Carl Love 2023-10-16 14:31 ` Andrew Burgess 1 sibling, 1 reply; 27+ messages in thread From: Keith Seitz @ 2023-10-13 20:34 UTC (permalink / raw) To: cel; +Cc: Ulrich.Weigand, gdb-patches Carl Love wrote: > This patch fixes two regression test failures in gdb.base/store.exp. It > also fixes two regression failures in gdb.python/py-thread-exited.exp. I did not notice any failures on HEAD in this test? But I also don't see any new regressions in any test with your patch. Just FAIL -> PASS. > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no > new regressions. I've read through this patch (and tested it), and I only have a few very trivial fixes to request. I don't think there's any reason to repost to fix a couple of typos, so just await a proper maintainers approval and commit with that approval. I don't know this code sufficiently well to give a proper approval, but I did not notice anything egregiously wrong. Tested-by: Keith Seitz <keiths@redhat.com> Thanks for the patch! Keith --- gdb/infrun.c | 13 ++++++++++ gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 784dafa59db..7fb90799dff 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -83,6 +83,7 @@ #include "features/rs6000/powerpc-isa207-vsx64l.c" #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" #include "features/rs6000/powerpc-e500l.c" +#include "dwarf2/frame.h" /* Shared library operations for PowerPC-Linux. */ static struct target_so_ops powerpc_so_ops; @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, return per_inferior->disp_step_buf->prepare (thread, displaced_pc); } +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ +static int +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); + + if (0 <= num && num <= 31) + return tdep->ppc_gp0_regnum + num; + else if (32 <= num && num <= 63) + /* FIXME: jimb/2004-05-05: What should we do when the debug info + specifies registers the architecture doesn't have? Our + callers don't check the value we return. */ + return tdep->ppc_fp0_regnum + (num - 32); + else if (77 <= num && num < 77 + 32) + return tdep->ppc_vr0_regnum + (num - 77); + else + switch (num) + { + case 65: + return tdep->ppc_lr_regnum; + case 66: + return tdep->ppc_ctr_regnum; + case 76: + return tdep->ppc_xer_regnum; + case 109: + return tdep->ppc_vrsave_regnum; + case 110: + return tdep->ppc_vrsave_regnum - 1; /* vscr */ + } + + /* Unknown DWARF register number. */ + return -1; +} + The spacing here is inconsistent. In the function above, there is no newline between the comment and the definition. Here there are two newlines: +/* Translate a .eh_frame register to DWARF register, or adjust a + .debug_frame register. */ + + +static int +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, + int eh_frame_p) +{ + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ + return num; +} + static void ppc_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); set_gdbarch_stap_parse_special_token (gdbarch, ppc_stap_parse_special_token); + /* Linux DWARF register mapping is different from the othe OS's. */ Note the typo, "othe[r]". The correct plural form is "OSes". + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + /* Note on Linux the mapping for the DWARF registers and the stab registers + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the + stab register mappings as well. */ + set_gdbarch_stab_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); if (tdep->wordsize == 4) { -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-13 20:34 ` Keith Seitz @ 2023-10-13 21:00 ` Carl Love 2023-10-16 11:12 ` Ulrich Weigand 0 siblings, 1 reply; 27+ messages in thread From: Carl Love @ 2023-10-13 21:00 UTC (permalink / raw) To: Keith Seitz, cel; +Cc: Ulrich.Weigand, gdb-patches, cel Keith: Thanks for the review. I have fixed the typos as noted below. Carl ---------- On Fri, 2023-10-13 at 13:34 -0700, Keith Seitz wrote: > Carl Love wrote: > > > This patch fixes two regression test failures in > > gdb.base/store.exp. It > > also fixes two regression failures in gdb.python/py-thread- > > exited.exp. > > I did not notice any failures on HEAD in this test? But I also don't > see > any new regressions in any test with your patch. Just FAIL -> PASS. > > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > > with no > > new regressions. > > I've read through this patch (and tested it), and I only have a few > very > trivial fixes to request. I don't think there's any reason to repost > to > fix a couple of typos, so just await a proper maintainers approval > and > commit with that approval. > > I don't know this code sufficiently well to give a proper approval, > but > I did not notice anything egregiously wrong. > > Tested-by: Keith Seitz <keiths@redhat.com> > > Thanks for the patch! > Keith > > --- > gdb/infrun.c | 13 ++++++++++ > gdb/ppc-linux-tdep.c | 56 > ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 784dafa59db..7fb90799dff 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -83,6 +83,7 @@ > #include "features/rs6000/powerpc-isa207-vsx64l.c" > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > +#include "dwarf2/frame.h" > > /* Shared library operations for PowerPC-Linux. */ > static struct target_so_ops powerpc_so_ops; > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch > *arch, thread_info *thread, > return per_inferior->disp_step_buf->prepare (thread, > displaced_pc); > } > > +/* Convert a Dwarf 2 register number to a GDB register number for > Linux. */ > +static int > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > + > + if (0 <= num && num <= 31) > + return tdep->ppc_gp0_regnum + num; > + else if (32 <= num && num <= 63) > + /* FIXME: jimb/2004-05-05: What should we do when the debug info > + specifies registers the architecture doesn't have? Our > + callers don't check the value we return. */ > + return tdep->ppc_fp0_regnum + (num - 32); > + else if (77 <= num && num < 77 + 32) > + return tdep->ppc_vr0_regnum + (num - 77); > + else > + switch (num) > + { > + case 65: > + return tdep->ppc_lr_regnum; > + case 66: > + return tdep->ppc_ctr_regnum; > + case 76: > + return tdep->ppc_xer_regnum; > + case 109: > + return tdep->ppc_vrsave_regnum; > + case 110: > + return tdep->ppc_vrsave_regnum - 1; /* vscr */ > + } > + > + /* Unknown DWARF register number. */ > + return -1; > +} > + > > The spacing here is inconsistent. In the function above, there is no > newline between the comment and the definition. Here there are two > newlines: Added a blank line between comment and rs6000_linux_dwarf2_reg_to_regnum function definition. > > +/* Translate a .eh_frame register to DWARF register, or adjust a > + .debug_frame register. */ > + > + Removed the extra line. > +static int > +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, > + int eh_frame_p) > +{ > + /* Linux uses the same numbering for .debug_frame numbering as > .eh_frame. */ > + return num; > +} > + > static void > ppc_linux_init_abi (struct gdbarch_info info, > struct gdbarch *gdbarch) > @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, > set_gdbarch_stap_is_single_operand (gdbarch, > ppc_stap_is_single_operand); > set_gdbarch_stap_parse_special_token (gdbarch, > ppc_stap_parse_special_token); > + /* Linux DWARF register mapping is different from the othe > OS's. */ > > Note the typo, "othe[r]". The correct plural form is "OSes". Ah, thanks for catching the missing r and the spelling error. Fixed. > > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + /* Note on Linux the mapping for the DWARF registers and the stab > registers > + use the same numbers. Install > rs6000_linux_dwarf2_reg_to_regnum for the > + stab register mappings as well. */ > + set_gdbarch_stab_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + dwarf2_frame_set_adjust_regnum (gdbarch, > rs6000_linux_adjust_frame_regnum); > > if (tdep->wordsize == 4) > { ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-13 21:00 ` Carl Love @ 2023-10-16 11:12 ` Ulrich Weigand 0 siblings, 0 replies; 27+ messages in thread From: Ulrich Weigand @ 2023-10-16 11:12 UTC (permalink / raw) To: cel, Keith Seitz; +Cc: gdb-patches Carl Love <cel@us.ibm.com> wrote: >Thanks for the review. I have fixed the typos as noted below. This patch (including fixed for the issues pointed out by Keith) is OK. Thanks, Ulrich ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 14:58 ` [Patch 1/2] " Carl Love 2023-10-13 20:34 ` Keith Seitz @ 2023-10-16 14:31 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love 2023-10-20 18:08 ` [PATCH 1/2, ver2] " Carl Love 1 sibling, 2 replies; 27+ messages in thread From: Andrew Burgess @ 2023-10-16 14:31 UTC (permalink / raw) To: Carl Love, gdb-patches, UlrichWeigand; +Cc: cel Carl Love <cel@us.ibm.com> writes: > GDB maintainers: > > This is the first patch in the series which fixes the DWWARF register > mapping and signal handling issues on PowerPC. > > Carl > > ----------------------------------------------- > > rs6000, Fix Linux DWARF register mapping > > The PowerPC DWARF register mapping is the same for the .eh_frame and > .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and > .debug_frame on other operating systems. The current GDB support for > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. > The files have some legacy mappings for spe_acc, spefscr, EV which was > removed from GCC in 2017. > > This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle > the DWARF register mappings on Linux. Function > rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to > call set_gdbarch_dwarf2_reg_to_regnum map the new function > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > dwarf2_frame_set_adjust_regnum is called to map > rs6000_linux_adjust_frame_regnum into the architecture. > > The second issue fixed by this patch is the check for subroutine > process_event_stop_test. Need to make sure the frame is not the > SIGTRAMP_FRAME. The sequence of events on most platforms is: Usually for GDB we avoid bundling unrelated changes into a single commit. Each commit should address one self contained issue (as far as possible). I really struggling to see any connection between the two fixes you have here. > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the handler. > ... > > However on PowerPC the sequence of events is: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the trampoline. > 3) The trampoline performs a normal function call to the handler. > ... > > We want "nexti" to step into, not over, signal handlers invoked > by the kernel. This is the case most platforms as the kernel puts a > signal trampoline frame onto the stack to handle proper return after the > handler. However, on some platforms such as PowerPC, the kernel actually > uses a trampoline to handle *invocation* of the handler. > > The issue is fixed in function process_event_stop_test by adding a check > that the frame is not a SIGTRAMP_FRAME to the if statement to stop in > a subroutine call. This prevents GDB from erroneously detecting the > trampoline invocation as a subroutine call. > > This patch fixes two regression test failures in gdb.base/store.exp. It > also fixes two regression failures in gdb.python/py-thread-exited.exp. On the one random PPC box I tried this patch on, I'm not seeing any failures in gdb.python/py-thread-exited.exp either before, or after this commit. Which tests in gdb.python/py-thread-exited.exp are you seeing as broken? And which of the two fixes in this commit fix the problems you're seeing? > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no > new regressions. > --- > gdb/infrun.c | 13 ++++++++++ > gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 4730d290442..922d8a6913d 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) > initial outermost frame, before sp was valid, would > have code_addr == &_start. See the comment in frame_id::operator== > for more. */ > + > + /* We want "nexti" to step into, not over, signal handlers invoked > + by the kernel, therefore this subroutine check should not trigger > + for a signal handler invocation. On most platforms, this is already > + not the case, as the kernel puts a signal trampoline frame onto the > + stack to handle proper return after the handler, and therefore at this > + point, the current frame is a grandchild of the step frame, not a > + child. However, on some platforms, the kernel actually uses a > + trampoline to handle *invocation* of the handler. In that case, > + when executing the first instruction of the trampoline, this check > + would erroneously detect the trampoline invocation as a subroutine > + call. Fix this by checking for SIGTRAMP_FRAME. */ > if ((get_stack_frame_id (frame) > != ecs->event_thread->control.step_stack_frame_id) > + && get_frame_type (frame) != SIGTRAMP_FRAME > && ((frame_unwind_caller_id (get_current_frame ()) > == ecs->event_thread->control.step_stack_frame_id) > && ((ecs->event_thread->control.step_stack_frame_id > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 784dafa59db..7fb90799dff 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -83,6 +83,7 @@ > #include "features/rs6000/powerpc-isa207-vsx64l.c" > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > +#include "dwarf2/frame.h" > > /* Shared library operations for PowerPC-Linux. */ > static struct target_so_ops powerpc_so_ops; > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, > return per_inferior->disp_step_buf->prepare (thread, displaced_pc); > } > > +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ > +static int > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > + > + if (0 <= num && num <= 31) > + return tdep->ppc_gp0_regnum + num; > + else if (32 <= num && num <= 63) > + /* FIXME: jimb/2004-05-05: What should we do when the debug info > + specifies registers the architecture doesn't have? Our > + callers don't check the value we return. */ I see this comment was just copied from else where, but isn't the answer just: return -1 ? The comment about the 'return -1' at the trail of this function seems to suggest that would be the correct thing to do. I guess I'm asking: do we need to add another copy of this (I think out of date) fixme? Thanks, Andrew > + return tdep->ppc_fp0_regnum + (num - 32); > + else if (77 <= num && num < 77 + 32) > + return tdep->ppc_vr0_regnum + (num - 77); > + else > + switch (num) > + { > + case 65: > + return tdep->ppc_lr_regnum; > + case 66: > + return tdep->ppc_ctr_regnum; > + case 76: > + return tdep->ppc_xer_regnum; > + case 109: > + return tdep->ppc_vrsave_regnum; > + case 110: > + return tdep->ppc_vrsave_regnum - 1; /* vscr */ > + } > + > + /* Unknown DWARF register number. */ > + return -1; > +} > + > +/* Translate a .eh_frame register to DWARF register, or adjust a > + .debug_frame register. */ > + > + > +static int > +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, > + int eh_frame_p) > +{ > + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ > + return num; > +} > + > static void > ppc_linux_init_abi (struct gdbarch_info info, > struct gdbarch *gdbarch) > @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, > set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); > set_gdbarch_stap_parse_special_token (gdbarch, > ppc_stap_parse_special_token); > + /* Linux DWARF register mapping is different from the othe OS's. */ > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + /* Note on Linux the mapping for the DWARF registers and the stab registers > + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the > + stab register mappings as well. */ > + set_gdbarch_stab_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); > > if (tdep->wordsize == 4) > { > -- > 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 14:31 ` Andrew Burgess @ 2023-10-16 15:51 ` Carl Love 2023-10-19 15:54 ` Carl Love 2023-10-24 8:50 ` Andrew Burgess 2023-10-20 18:08 ` [PATCH 1/2, ver2] " Carl Love 1 sibling, 2 replies; 27+ messages in thread From: Carl Love @ 2023-10-16 15:51 UTC (permalink / raw) To: Andrew Burgess, gdb-patches, UlrichWeigand; +Cc: cel Andrew: Thanks for the review and comments. See responses below. On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote: > Carl Love <cel@us.ibm.com> writes: > > > GDB maintainers: > > > > This is the first patch in the series which fixes the DWWARF > > register > > mapping and signal handling issues on PowerPC. > > > > Carl > > > > ----------------------------------------------- > > > > rs6000, Fix Linux DWARF register mapping > > > > The PowerPC DWARF register mapping is the same for the .eh_frame > > and > > .debug_frame on Linux. PowerPC uses different mapping for > > .eh_frame and > > .debug_frame on other operating systems. The current GDB support > > for > > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum > > and > > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct > > for Linux. > > The files have some legacy mappings for spe_acc, spefscr, EV which > > was > > removed from GCC in 2017. > > > > This patch adds a two new functions > > rs6000_linux_dwarf2_reg_to_regnum, > > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c > > to handle > > the DWARF register mappings on Linux. Function > > rs6000_linux_dwarf2_reg_to_regnum is installed for both > > gdb_dwarf_to_regnum > > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated > > to > > call set_gdbarch_dwarf2_reg_to_regnum map the new function > > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > > dwarf2_frame_set_adjust_regnum is called to map > > rs6000_linux_adjust_frame_regnum into the architecture. > > > > The second issue fixed by this patch is the check for subroutine > > process_event_stop_test. Need to make sure the frame is not the > > SIGTRAMP_FRAME. The sequence of events on most platforms is: > > Usually for GDB we avoid bundling unrelated changes into a single > commit. Each commit should address one self contained issue (as far > as > possible). > > I really struggling to see any connection between the two fixes you > have > here. As mentioned in the commit log, the first patch which fixes the DWARF register mapping fixes two of the 5 failures seen in store.exp. Specifically the patch fixes the issue of GDB not stopping in the handler which results in the step.exp test failing. The second patch then fixes the rest of the issues with the step.exp test. > > > 1) Some code is running when a signal arrives. > > 2) The kernel handles the signal and dispatches to the handler. > > ... > > > > However on PowerPC the sequence of events is: > > > > 1) Some code is running when a signal arrives. > > 2) The kernel handles the signal and dispatches to the > > trampoline. > > 3) The trampoline performs a normal function call to the handler. > > ... > > > > We want "nexti" to step into, not over, signal handlers invoked > > by the kernel. This is the case most platforms as the kernel puts > > a > > signal trampoline frame onto the stack to handle proper return > > after the > > handler. However, on some platforms such as PowerPC, the kernel > > actually > > uses a trampoline to handle *invocation* of the handler. > > > > The issue is fixed in function process_event_stop_test by adding a > > check > > that the frame is not a SIGTRAMP_FRAME to the if statement to stop > > in > > a subroutine call. This prevents GDB from erroneously detecting > > the > > trampoline invocation as a subroutine call. > > > > This patch fixes two regression test failures in > > gdb.base/store.exp. It > > also fixes two regression failures in gdb.python/py-thread- > > exited.exp. > > On the one random PPC box I tried this patch on, I'm not seeing any > failures in gdb.python/py-thread-exited.exp either before, or after > this > commit. > > Which tests in gdb.python/py-thread-exited.exp are you seeing as > broken? > And which of the two fixes in this commit fix the problems you're > seeing? The comment about the py-thread-exited.exp should have been removed. I found that sometimes that test fails and sometime passes. I have yet to dig into it and figure out why the test is inconsistent. The inconsistent behavior exists with and without these patches. I will remove the comment about the gdb.python/py-thread-exited.exp fixes as that is erroneous. > > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > > with no > > new regressions. > > --- > > gdb/infrun.c | 13 ++++++++++ > > gdb/ppc-linux-tdep.c | 56 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 69 insertions(+) > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > index 4730d290442..922d8a6913d 100644 > > --- a/gdb/infrun.c > > +++ b/gdb/infrun.c > > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct > > execution_control_state *ecs) > > initial outermost frame, before sp was valid, would > > have code_addr == &_start. See the comment in > > frame_id::operator== > > for more. */ > > + > > + /* We want "nexti" to step into, not over, signal handlers > > invoked > > + by the kernel, therefore this subroutine check should not > > trigger > > + for a signal handler invocation. On most platforms, this is > > already > > + not the case, as the kernel puts a signal trampoline frame > > onto the > > + stack to handle proper return after the handler, and > > therefore at this > > + point, the current frame is a grandchild of the step frame, > > not a > > + child. However, on some platforms, the kernel actually uses > > a > > + trampoline to handle *invocation* of the handler. In that > > case, > > + when executing the first instruction of the trampoline, this > > check > > + would erroneously detect the trampoline invocation as a > > subroutine > > + call. Fix this by checking for SIGTRAMP_FRAME. */ > > if ((get_stack_frame_id (frame) > > != ecs->event_thread->control.step_stack_frame_id) > > + && get_frame_type (frame) != SIGTRAMP_FRAME > > && ((frame_unwind_caller_id (get_current_frame ()) > > == ecs->event_thread->control.step_stack_frame_id) > > && ((ecs->event_thread->control.step_stack_frame_id > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > > index 784dafa59db..7fb90799dff 100644 > > --- a/gdb/ppc-linux-tdep.c > > +++ b/gdb/ppc-linux-tdep.c > > @@ -83,6 +83,7 @@ > > #include "features/rs6000/powerpc-isa207-vsx64l.c" > > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > > #include "features/rs6000/powerpc-e500l.c" > > +#include "dwarf2/frame.h" > > > > /* Shared library operations for PowerPC-Linux. */ > > static struct target_so_ops powerpc_so_ops; > > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch > > *arch, thread_info *thread, > > return per_inferior->disp_step_buf->prepare (thread, > > displaced_pc); > > } > > > > +/* Convert a Dwarf 2 register number to a GDB register number for > > Linux. */ > > +static int > > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int > > num) > > +{ > > + ppc_gdbarch_tdep *tdep = > > gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > > + > > + if (0 <= num && num <= 31) > > + return tdep->ppc_gp0_regnum + num; > > + else if (32 <= num && num <= 63) > > + /* FIXME: jimb/2004-05-05: What should we do when the debug > > info > > + specifies registers the architecture doesn't have? Our > > + callers don't check the value we return. */ > > I see this comment was just copied from else where, but isn't the > answer > just: return -1 ? > > The comment about the 'return -1' at the trail of this function seems > to > suggest that would be the correct thing to do. > > I guess I'm asking: do we need to add another copy of this (I think > out > of date) fixme? Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the specific changes for the Linux DWARF mappings. The comment looked out of date to me, but I left it for consistency with the other two functions that have the same comment. It would probably be best to investigate the comment further and update it in all places in a separate patch. Would it be acceptable to leave the comment as is for now, for consistency sake, and I will work on a separate cleanup patch to address removing the comment in the original two places and this additional place? I will want to look into it a bit more but I think we can just remove the comment. But I do want to verify that the return value is never used first. Carl ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 15:51 ` Carl Love @ 2023-10-19 15:54 ` Carl Love 2023-10-24 8:50 ` Andrew Burgess 1 sibling, 0 replies; 27+ messages in thread From: Carl Love @ 2023-10-19 15:54 UTC (permalink / raw) To: Carl Love, Andrew Burgess, gdb-patches, UlrichWeigand Andrew: Just wanted to make sure you were OK with response, particularly about creating a new patch to address the issues with the comment in the code before I commit the two patches. Sorry for the slow response. My email is being migrated from cel@us.ibm.com to cel@linux.ibm.com. There have been some issues with the migration. I don't seem to be getting email at the old address but the new address does seem to work. So any replies need to include the new email address. Thanks. Carl On Mon, 2023-10-16 at 08:51 -0700, Carl Love wrote: > Andrew: > > Thanks for the review and comments. See responses below. > > On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote: > > Carl Love <cel@us.ibm.com> writes: > > > > > GDB maintainers: > > > > > > This is the first patch in the series which fixes the DWWARF > > > register > > > mapping and signal handling issues on PowerPC. > > > > > > Carl > > > > > > ----------------------------------------------- > > > > > > rs6000, Fix Linux DWARF register mapping > > > > > > The PowerPC DWARF register mapping is the same for the .eh_frame > > > and > > > .debug_frame on Linux. PowerPC uses different mapping for > > > .eh_frame and > > > .debug_frame on other operating systems. The current GDB support > > > for > > > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum > > > and > > > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct > > > for Linux. > > > The files have some legacy mappings for spe_acc, spefscr, EV > > > which > > > was > > > removed from GCC in 2017. > > > > > > This patch adds a two new functions > > > rs6000_linux_dwarf2_reg_to_regnum, > > > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c > > > to handle > > > the DWARF register mappings on Linux. Function > > > rs6000_linux_dwarf2_reg_to_regnum is installed for both > > > gdb_dwarf_to_regnum > > > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > > > > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is > > > updated > > > to > > > call set_gdbarch_dwarf2_reg_to_regnum map the new function > > > rs6000_linux_dwarf2_reg_to_regnum for the > > > architecture. Similarly, > > > dwarf2_frame_set_adjust_regnum is called to map > > > rs6000_linux_adjust_frame_regnum into the architecture. > > > > > > The second issue fixed by this patch is the check for subroutine > > > process_event_stop_test. Need to make sure the frame is not the > > > SIGTRAMP_FRAME. The sequence of events on most platforms is: > > > > Usually for GDB we avoid bundling unrelated changes into a single > > commit. Each commit should address one self contained issue (as > > far > > as > > possible). > > > > I really struggling to see any connection between the two fixes you > > have > > here. > > As mentioned in the commit log, the first patch which fixes the DWARF > register mapping fixes two of the 5 failures seen in store.exp. > Specifically the patch fixes the issue of GDB not stopping in the > handler which results in the step.exp test failing. The second patch > then fixes the rest of the issues with the step.exp test. > > > 1) Some code is running when a signal arrives. > > > 2) The kernel handles the signal and dispatches to the handler. > > > ... > > > > > > However on PowerPC the sequence of events is: > > > > > > 1) Some code is running when a signal arrives. > > > 2) The kernel handles the signal and dispatches to the > > > trampoline. > > > 3) The trampoline performs a normal function call to the > > > handler. > > > ... > > > > > > We want "nexti" to step into, not over, signal handlers invoked > > > by the kernel. This is the case most platforms as the kernel > > > puts > > > a > > > signal trampoline frame onto the stack to handle proper return > > > after the > > > handler. However, on some platforms such as PowerPC, the kernel > > > actually > > > uses a trampoline to handle *invocation* of the handler. > > > > > > The issue is fixed in function process_event_stop_test by adding > > > a > > > check > > > that the frame is not a SIGTRAMP_FRAME to the if statement to > > > stop > > > in > > > a subroutine call. This prevents GDB from erroneously detecting > > > the > > > trampoline invocation as a subroutine call. > > > > > > This patch fixes two regression test failures in > > > gdb.base/store.exp. It > > > also fixes two regression failures in gdb.python/py-thread- > > > exited.exp. > > > > On the one random PPC box I tried this patch on, I'm not seeing any > > failures in gdb.python/py-thread-exited.exp either before, or after > > this > > commit. > > > > Which tests in gdb.python/py-thread-exited.exp are you seeing as > > broken? > > And which of the two fixes in this commit fix the problems you're > > seeing? > > The comment about the py-thread-exited.exp should have been > removed. I > found that sometimes that test fails and sometime passes. I have yet > to dig into it and figure out why the test is inconsistent. The > inconsistent behavior exists with and without these patches. > > I will remove the comment about the gdb.python/py-thread-exited.exp > fixes as that is erroneous. > > > > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > > > with no > > > new regressions. > > > --- > > > gdb/infrun.c | 13 ++++++++++ > > > gdb/ppc-linux-tdep.c | 56 > > > ++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 69 insertions(+) > > > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > > index 4730d290442..922d8a6913d 100644 > > > --- a/gdb/infrun.c > > > +++ b/gdb/infrun.c > > > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct > > > execution_control_state *ecs) > > > initial outermost frame, before sp was valid, would > > > have code_addr == &_start. See the comment in > > > frame_id::operator== > > > for more. */ > > > + > > > + /* We want "nexti" to step into, not over, signal handlers > > > invoked > > > + by the kernel, therefore this subroutine check should not > > > trigger > > > + for a signal handler invocation. On most platforms, this > > > is > > > already > > > + not the case, as the kernel puts a signal trampoline frame > > > onto the > > > + stack to handle proper return after the handler, and > > > therefore at this > > > + point, the current frame is a grandchild of the step frame, > > > not a > > > + child. However, on some platforms, the kernel actually > > > uses > > > a > > > + trampoline to handle *invocation* of the handler. In that > > > case, > > > + when executing the first instruction of the trampoline, > > > this > > > check > > > + would erroneously detect the trampoline invocation as a > > > subroutine > > > + call. Fix this by checking for SIGTRAMP_FRAME. */ > > > if ((get_stack_frame_id (frame) > > > != ecs->event_thread->control.step_stack_frame_id) > > > + && get_frame_type (frame) != SIGTRAMP_FRAME > > > && ((frame_unwind_caller_id (get_current_frame ()) > > > == ecs->event_thread->control.step_stack_frame_id) > > > && ((ecs->event_thread->control.step_stack_frame_id > > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > > > index 784dafa59db..7fb90799dff 100644 > > > --- a/gdb/ppc-linux-tdep.c > > > +++ b/gdb/ppc-linux-tdep.c > > > @@ -83,6 +83,7 @@ > > > #include "features/rs6000/powerpc-isa207-vsx64l.c" > > > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > > > #include "features/rs6000/powerpc-e500l.c" > > > +#include "dwarf2/frame.h" > > > > > > /* Shared library operations for PowerPC-Linux. */ > > > static struct target_so_ops powerpc_so_ops; > > > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch > > > *arch, thread_info *thread, > > > return per_inferior->disp_step_buf->prepare (thread, > > > displaced_pc); > > > } > > > > > > +/* Convert a Dwarf 2 register number to a GDB register number > > > for > > > Linux. */ > > > +static int > > > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int > > > num) > > > +{ > > > + ppc_gdbarch_tdep *tdep = > > > gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > > > + > > > + if (0 <= num && num <= 31) > > > + return tdep->ppc_gp0_regnum + num; > > > + else if (32 <= num && num <= 63) > > > + /* FIXME: jimb/2004-05-05: What should we do when the debug > > > info > > > + specifies registers the architecture doesn't have? Our > > > + callers don't check the value we return. */ > > > > I see this comment was just copied from else where, but isn't the > > answer > > just: return -1 ? > > > > The comment about the 'return -1' at the trail of this function > > seems > > to > > suggest that would be the correct thing to do. > > > > I guess I'm asking: do we need to add another copy of this (I think > > out > > of date) fixme? > > Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the > specific changes for the Linux DWARF mappings. The comment looked > out > of date to me, but I left it for consistency with the other two > functions that have the same comment. It would probably be best to > investigate the comment further and update it in all places in a > separate patch. > > Would it be acceptable to leave the comment as is for now, for > consistency sake, and I will work on a separate cleanup patch to > address removing the comment in the original two places and this > additional place? I will want to look into it a bit more but I think > we can just remove the comment. But I do want to verify that the > return value is never used first. > > Carl > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 15:51 ` Carl Love 2023-10-19 15:54 ` Carl Love @ 2023-10-24 8:50 ` Andrew Burgess 2023-10-24 16:05 ` Carl Love 1 sibling, 1 reply; 27+ messages in thread From: Andrew Burgess @ 2023-10-24 8:50 UTC (permalink / raw) To: Carl Love, gdb-patches, UlrichWeigand; +Cc: cel Carl Love <cel@us.ibm.com> writes: > Andrew: > > Thanks for the review and comments. See responses below. Carl, Sorry for my slow reply. > > On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote: >> Carl Love <cel@us.ibm.com> writes: >> >> > GDB maintainers: >> > >> > This is the first patch in the series which fixes the DWWARF >> > register >> > mapping and signal handling issues on PowerPC. >> > >> > Carl >> > >> > ----------------------------------------------- >> > >> > rs6000, Fix Linux DWARF register mapping >> > >> > The PowerPC DWARF register mapping is the same for the .eh_frame >> > and >> > .debug_frame on Linux. PowerPC uses different mapping for >> > .eh_frame and >> > .debug_frame on other operating systems. The current GDB support >> > for >> > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum >> > and >> > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct >> > for Linux. >> > The files have some legacy mappings for spe_acc, spefscr, EV which >> > was >> > removed from GCC in 2017. >> > >> > This patch adds a two new functions >> > rs6000_linux_dwarf2_reg_to_regnum, >> > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c >> > to handle >> > the DWARF register mappings on Linux. Function >> > rs6000_linux_dwarf2_reg_to_regnum is installed for both >> > gdb_dwarf_to_regnum >> > and gdbarch_stab_reg_to_regnum since the mappings are the same. >> > >> > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated >> > to >> > call set_gdbarch_dwarf2_reg_to_regnum map the new function >> > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, >> > dwarf2_frame_set_adjust_regnum is called to map >> > rs6000_linux_adjust_frame_regnum into the architecture. >> > >> > The second issue fixed by this patch is the check for subroutine >> > process_event_stop_test. Need to make sure the frame is not the >> > SIGTRAMP_FRAME. The sequence of events on most platforms is: >> >> Usually for GDB we avoid bundling unrelated changes into a single >> commit. Each commit should address one self contained issue (as far >> as >> possible). >> >> I really struggling to see any connection between the two fixes you >> have >> here. > > As mentioned in the commit log, the first patch which fixes the DWARF > register mapping fixes two of the 5 failures seen in store.exp. > Specifically the patch fixes the issue of GDB not stopping in the > handler which results in the step.exp test failing. The second patch > then fixes the rest of the issues with the step.exp test. I'm now even more confused. The commit message for the first patch suggests that it contains two separate fixes, but the commit message, even your updated commit message for 'ver2' only mentions store.exp. To be more specific, in patch 1/2, the changes in ppc-linux-tdep.c seem unrelated to the changes in infrun.c. And I'm confused about how the changes in infrun.c can fix anything in the store.exp test script -- the comments relating to that part of the patch specifically talk about 'nexti' and signal handlers, but I don't see either of these being used in store.exp (we do use 'next' though, so maybe that's what you mean?) But I'm not clear if a signal is invoked in that test at all. In the text above you do mention step.exp, so maybe that's what the infrun.c changes are fixing? I'll grab a PPC machine and have a play, but your 'ver 2' commit message has no mention of step.exp, so if that test is important you might need to update the text. Usually in GDB when we talk about each patch containing a single fix we're talking about changes to GDB functionality, not fixing a test script. So it's not: 'Patch 1/1 fixes all of store.exp' but rather, 'Patch 1/2 fixes PPC DWARF register mapping' and 'Patch 2/2 fixes nexti blah blah signal frames'. >> >> > 1) Some code is running when a signal arrives. >> > 2) The kernel handles the signal and dispatches to the handler. >> > ... >> > >> > However on PowerPC the sequence of events is: >> > >> > 1) Some code is running when a signal arrives. >> > 2) The kernel handles the signal and dispatches to the >> > trampoline. >> > 3) The trampoline performs a normal function call to the handler. >> > ... >> > >> > We want "nexti" to step into, not over, signal handlers invoked >> > by the kernel. This is the case most platforms as the kernel puts >> > a >> > signal trampoline frame onto the stack to handle proper return >> > after the >> > handler. However, on some platforms such as PowerPC, the kernel >> > actually >> > uses a trampoline to handle *invocation* of the handler. >> > >> > The issue is fixed in function process_event_stop_test by adding a >> > check >> > that the frame is not a SIGTRAMP_FRAME to the if statement to stop >> > in >> > a subroutine call. This prevents GDB from erroneously detecting >> > the >> > trampoline invocation as a subroutine call. >> > >> > This patch fixes two regression test failures in >> > gdb.base/store.exp. It >> > also fixes two regression failures in gdb.python/py-thread- >> > exited.exp. >> >> On the one random PPC box I tried this patch on, I'm not seeing any >> failures in gdb.python/py-thread-exited.exp either before, or after >> this >> commit. >> >> Which tests in gdb.python/py-thread-exited.exp are you seeing as >> broken? >> And which of the two fixes in this commit fix the problems you're >> seeing? > > The comment about the py-thread-exited.exp should have been removed. I > found that sometimes that test fails and sometime passes. I have yet > to dig into it and figure out why the test is inconsistent. The > inconsistent behavior exists with and without these patches. > > I will remove the comment about the gdb.python/py-thread-exited.exp > fixes as that is erroneous. Thanks. > > >> >> > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 >> > with no >> > new regressions. >> > --- >> > gdb/infrun.c | 13 ++++++++++ >> > gdb/ppc-linux-tdep.c | 56 >> > ++++++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 69 insertions(+) >> > >> > diff --git a/gdb/infrun.c b/gdb/infrun.c >> > index 4730d290442..922d8a6913d 100644 >> > --- a/gdb/infrun.c >> > +++ b/gdb/infrun.c >> > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct >> > execution_control_state *ecs) >> > initial outermost frame, before sp was valid, would >> > have code_addr == &_start. See the comment in >> > frame_id::operator== >> > for more. */ >> > + >> > + /* We want "nexti" to step into, not over, signal handlers >> > invoked >> > + by the kernel, therefore this subroutine check should not >> > trigger >> > + for a signal handler invocation. On most platforms, this is >> > already >> > + not the case, as the kernel puts a signal trampoline frame >> > onto the >> > + stack to handle proper return after the handler, and >> > therefore at this >> > + point, the current frame is a grandchild of the step frame, >> > not a >> > + child. However, on some platforms, the kernel actually uses >> > a >> > + trampoline to handle *invocation* of the handler. In that >> > case, >> > + when executing the first instruction of the trampoline, this >> > check >> > + would erroneously detect the trampoline invocation as a >> > subroutine >> > + call. Fix this by checking for SIGTRAMP_FRAME. */ >> > if ((get_stack_frame_id (frame) >> > != ecs->event_thread->control.step_stack_frame_id) >> > + && get_frame_type (frame) != SIGTRAMP_FRAME >> > && ((frame_unwind_caller_id (get_current_frame ()) >> > == ecs->event_thread->control.step_stack_frame_id) >> > && ((ecs->event_thread->control.step_stack_frame_id >> > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c >> > index 784dafa59db..7fb90799dff 100644 >> > --- a/gdb/ppc-linux-tdep.c >> > +++ b/gdb/ppc-linux-tdep.c >> > @@ -83,6 +83,7 @@ >> > #include "features/rs6000/powerpc-isa207-vsx64l.c" >> > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" >> > #include "features/rs6000/powerpc-e500l.c" >> > +#include "dwarf2/frame.h" >> > >> > /* Shared library operations for PowerPC-Linux. */ >> > static struct target_so_ops powerpc_so_ops; >> > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch >> > *arch, thread_info *thread, >> > return per_inferior->disp_step_buf->prepare (thread, >> > displaced_pc); >> > } >> > >> > +/* Convert a Dwarf 2 register number to a GDB register number for >> > Linux. */ >> > +static int >> > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int >> > num) >> > +{ >> > + ppc_gdbarch_tdep *tdep = >> > gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); >> > + >> > + if (0 <= num && num <= 31) >> > + return tdep->ppc_gp0_regnum + num; >> > + else if (32 <= num && num <= 63) >> > + /* FIXME: jimb/2004-05-05: What should we do when the debug >> > info >> > + specifies registers the architecture doesn't have? Our >> > + callers don't check the value we return. */ >> >> I see this comment was just copied from else where, but isn't the >> answer >> just: return -1 ? >> >> The comment about the 'return -1' at the trail of this function seems >> to >> suggest that would be the correct thing to do. >> >> I guess I'm asking: do we need to add another copy of this (I think >> out >> of date) fixme? > > Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the > specific changes for the Linux DWARF mappings. The comment looked out > of date to me, but I left it for consistency with the other two > functions that have the same comment. It would probably be best to > investigate the comment further and update it in all places in a > separate patch. > > Would it be acceptable to leave the comment as is for now, for > consistency sake, and I will work on a separate cleanup patch to > address removing the comment in the original two places and this > additional place? I will want to look into it a bit more but I think > we can just remove the comment. But I do want to verify that the > return value is never used first. I took a quick look. There are three callers: 1 in dwarf2/loc.c, and 2 in jit.c. One of the callers in jit.c does fail to check the return value, but this looks like a bug to me. The other callers do check for a -1 return value. The comment in gdbarch-gen.h is also explicit that the right thing to do is return -1. Coupled with the fact that the comment, in its current location just makes no sense I think the best idea would be to drop it from your new function. Cleaning up the older code in the future would be nice, but is not required. But I don't think we need to introduce more incorrect comments just for consistency. Thanks, Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-24 8:50 ` Andrew Burgess @ 2023-10-24 16:05 ` Carl Love 0 siblings, 0 replies; 27+ messages in thread From: Carl Love @ 2023-10-24 16:05 UTC (permalink / raw) To: Andrew Burgess, Carl Love, gdb-patches, UlrichWeigand Andrew: On Tue, 2023-10-24 at 09:50 +0100, Andrew Burgess wrote: > Carl Love <cel@us.ibm.com> writes: > > > Andrew: > > > > Thanks for the review and comments. See responses below. > > Carl, > > Sorry for my slow reply. > > > On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote: > > > Carl Love <cel@us.ibm.com> writes: > > > > > > > GDB maintainers: > > > > > > > > This is the first patch in the series which fixes the DWWARF > > > > register > > > > mapping and signal handling issues on PowerPC. > > > > > > > > Carl > > > > > > > > ----------------------------------------------- > > > > > > > > rs6000, Fix Linux DWARF register mapping > > > > > > > > The PowerPC DWARF register mapping is the same for the > > > > .eh_frame > > > > and > > > > .debug_frame on Linux. PowerPC uses different mapping for > > > > .eh_frame and > > > > .debug_frame on other operating systems. The current GDB > > > > support > > > > for > > > > mapping the DWARF registers in > > > > rs6000_linux_dwarf2_reg_to_regnum > > > > and > > > > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not > > > > correct > > > > for Linux. > > > > The files have some legacy mappings for spe_acc, spefscr, EV > > > > which > > > > was > > > > removed from GCC in 2017. > > > > > > > > This patch adds a two new functions > > > > rs6000_linux_dwarf2_reg_to_regnum, > > > > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux- > > > > tdep.c > > > > to handle > > > > the DWARF register mappings on Linux. Function > > > > rs6000_linux_dwarf2_reg_to_regnum is installed for both > > > > gdb_dwarf_to_regnum > > > > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > > > > > > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is > > > > updated > > > > to > > > > call set_gdbarch_dwarf2_reg_to_regnum map the new function > > > > rs6000_linux_dwarf2_reg_to_regnum for the > > > > architecture. Similarly, > > > > dwarf2_frame_set_adjust_regnum is called to map > > > > rs6000_linux_adjust_frame_regnum into the architecture. > > > > > > > > The second issue fixed by this patch is the check for > > > > subroutine > > > > process_event_stop_test. Need to make sure the frame is not > > > > the > > > > SIGTRAMP_FRAME. The sequence of events on most platforms is: > > > > > > Usually for GDB we avoid bundling unrelated changes into a single > > > commit. Each commit should address one self contained issue (as > > > far > > > as > > > possible). > > > > > > I really struggling to see any connection between the two fixes > > > you > > > have > > > here. > > > > As mentioned in the commit log, the first patch which fixes the > > DWARF > > register mapping fixes two of the 5 failures seen in store.exp. > > Specifically the patch fixes the issue of GDB not stopping in the > > handler which results in the step.exp test failing. The second > > patch > > then fixes the rest of the issues with the step.exp test. > > I'm now even more confused. > > The commit message for the first patch suggests that it contains two > separate fixes, but the commit message, even your updated commit > message > for 'ver2' only mentions store.exp. > > To be more specific, in patch 1/2, the changes in ppc-linux-tdep.c > seem > unrelated to the changes in infrun.c. And I'm confused about how the > changes in infrun.c can fix anything in the store.exp test script -- > the > comments relating to that part of the patch specifically talk about > 'nexti' and signal handlers, but I don't see either of these being > used > in store.exp (we do use 'next' though, so maybe that's what you > mean?) > But I'm not clear if a signal is invoked in that test at all. > > In the text above you do mention step.exp, so maybe that's what the > infrun.c changes are fixing? I'll grab a PPC machine and have a > play, > but your 'ver 2' commit message has no mention of step.exp, so if > that > test is important you might need to update the text. > The 128-bit floating point issues in store.exp will need a Power10 system which supports the 128-bit floating point types. A Power 9 system is not sufficient. OK, I think I see what is missing in the explanation. So initially, I fixed the DWARF register mapping for the store.exp test. This fixed a couple of the issues with the store.exp test. Then in the second patch we fixed the DWARF register mapping for the 128-bit floating point values. Specifically, DWARF used the same register numbers for 64-bit and 128-bit floating point values. However, the 128-bit floating point values are stored in the VSR register file while the 64-bit values are stored in the floating point registers. The function ieee_128_float_regnum_adjust takes care of that fix. So at this point, the store.exp test works. But now I have a regression failure on gdb.base/sigstep.exp. This test is specifically dealing with signal handlers. Fixing the register mapping, in patch 1, exposed an underlying bug in the signal handler code. The fixes for the nexti, stepi in the signal handling fixes the regression error. The signal handling fix is needed as part of the DWARF register mapping fix so we don't have any regression failures and so these fixes got rolled into the first patch but it was not really explained in the original patch description. So yes, there are really two things getting fixed in the first patch. But you don't see the signal handler issue until after the register mapping is fixed for Linux. The signal handler issues should be exposed on Power 9 if we split out the DWARF register mapping from the signal handler fixes. I didn't play with these individually on Power 9 as part of the patch work. All of the patch work was done on Power 10. So maybe it will be best if I redo the two patch sequence into three patches. Patch 1, fix the mapping; Patch 2 fix the signal handling issue exposed once the register mapping is fixed; Patch 3 fix the handling of the 128-bit float DWARF register mapping. This should then make it clearer what is going on in each patch. Thoughts? Carl > Usually in GDB when we talk about each patch containing a single fix > we're talking about changes to GDB functionality, not fixing a test > script. So it's not: 'Patch 1/1 fixes all of store.exp' but rather, > 'Patch 1/2 fixes PPC DWARF register mapping' and 'Patch 2/2 fixes > nexti > blah blah signal frames'. > > > > > 1) Some code is running when a signal arrives. > > > > 2) The kernel handles the signal and dispatches to the > > > > handler. > > > > ... > > > > > > > > However on PowerPC the sequence of events is: > > > > > > > > 1) Some code is running when a signal arrives. > > > > 2) The kernel handles the signal and dispatches to the > > > > trampoline. > > > > 3) The trampoline performs a normal function call to the > > > > handler. > > > > ... > > > > > > > > We want "nexti" to step into, not over, signal handlers invoked > > > > by the kernel. This is the case most platforms as the kernel > > > > puts > > > > a > > > > signal trampoline frame onto the stack to handle proper return > > > > after the > > > > handler. However, on some platforms such as PowerPC, the > > > > kernel > > > > actually > > > > uses a trampoline to handle *invocation* of the handler. > > > > > > > > The issue is fixed in function process_event_stop_test by > > > > adding a > > > > check > > > > that the frame is not a SIGTRAMP_FRAME to the if statement to > > > > stop > > > > in > > > > a subroutine call. This prevents GDB from erroneously > > > > detecting > > > > the > > > > trampoline invocation as a subroutine call. > > > > > > > > This patch fixes two regression test failures in > > > > gdb.base/store.exp. It > > > > also fixes two regression failures in gdb.python/py-thread- > > > > exited.exp. > > > > > > On the one random PPC box I tried this patch on, I'm not seeing > > > any > > > failures in gdb.python/py-thread-exited.exp either before, or > > > after > > > this > > > commit. > > > > > > Which tests in gdb.python/py-thread-exited.exp are you seeing as > > > broken? > > > And which of the two fixes in this commit fix the problems you're > > > seeing? > > > > The comment about the py-thread-exited.exp should have been > > removed. I > > found that sometimes that test fails and sometime passes. I have > > yet > > to dig into it and figure out why the test is inconsistent. The > > inconsistent behavior exists with and without these patches. > > > > I will remove the comment about the gdb.python/py-thread-exited.exp > > fixes as that is erroneous. > > Thanks. > > > > > > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > > > > with no > > > > new regressions. > > > > --- > > > > gdb/infrun.c | 13 ++++++++++ > > > > gdb/ppc-linux-tdep.c | 56 > > > > ++++++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 69 insertions(+) > > > > > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > > > index 4730d290442..922d8a6913d 100644 > > > > --- a/gdb/infrun.c > > > > +++ b/gdb/infrun.c > > > > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct > > > > execution_control_state *ecs) > > > > initial outermost frame, before sp was valid, would > > > > have code_addr == &_start. See the comment in > > > > frame_id::operator== > > > > for more. */ > > > > + > > > > + /* We want "nexti" to step into, not over, signal handlers > > > > invoked > > > > + by the kernel, therefore this subroutine check should not > > > > trigger > > > > + for a signal handler invocation. On most platforms, this > > > > is > > > > already > > > > + not the case, as the kernel puts a signal trampoline > > > > frame > > > > onto the > > > > + stack to handle proper return after the handler, and > > > > therefore at this > > > > + point, the current frame is a grandchild of the step > > > > frame, > > > > not a > > > > + child. However, on some platforms, the kernel actually > > > > uses > > > > a > > > > + trampoline to handle *invocation* of the handler. In > > > > that > > > > case, > > > > + when executing the first instruction of the trampoline, > > > > this > > > > check > > > > + would erroneously detect the trampoline invocation as a > > > > subroutine > > > > + call. Fix this by checking for SIGTRAMP_FRAME. */ > > > > if ((get_stack_frame_id (frame) > > > > != ecs->event_thread->control.step_stack_frame_id) > > > > + && get_frame_type (frame) != SIGTRAMP_FRAME > > > > && ((frame_unwind_caller_id (get_current_frame ()) > > > > == ecs->event_thread->control.step_stack_frame_id) > > > > && ((ecs->event_thread->control.step_stack_frame_id > > > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > > > > index 784dafa59db..7fb90799dff 100644 > > > > --- a/gdb/ppc-linux-tdep.c > > > > +++ b/gdb/ppc-linux-tdep.c > > > > @@ -83,6 +83,7 @@ > > > > #include "features/rs6000/powerpc-isa207-vsx64l.c" > > > > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > > > > #include "features/rs6000/powerpc-e500l.c" > > > > +#include "dwarf2/frame.h" > > > > > > > > /* Shared library operations for PowerPC-Linux. */ > > > > static struct target_so_ops powerpc_so_ops; > > > > @@ -2088,6 +2089,52 @@ > > > > ppc_linux_displaced_step_prepare (gdbarch > > > > *arch, thread_info *thread, > > > > return per_inferior->disp_step_buf->prepare (thread, > > > > displaced_pc); > > > > } > > > > > > > > +/* Convert a Dwarf 2 register number to a GDB register number > > > > for > > > > Linux. */ > > > > +static int > > > > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, > > > > int > > > > num) > > > > +{ > > > > + ppc_gdbarch_tdep *tdep = > > > > gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > > > > + > > > > + if (0 <= num && num <= 31) > > > > + return tdep->ppc_gp0_regnum + num; > > > > + else if (32 <= num && num <= 63) > > > > + /* FIXME: jimb/2004-05-05: What should we do when the > > > > debug > > > > info > > > > + specifies registers the architecture doesn't have? Our > > > > + callers don't check the value we return. */ > > > > > > I see this comment was just copied from else where, but isn't the > > > answer > > > just: return -1 ? > > > > > > The comment about the 'return -1' at the trail of this function > > > seems > > > to > > > suggest that would be the correct thing to do. > > > > > > I guess I'm asking: do we need to add another copy of this (I > > > think > > > out > > > of date) fixme? > > > > Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the > > specific changes for the Linux DWARF mappings. The comment looked > > out > > of date to me, but I left it for consistency with the other two > > functions that have the same comment. It would probably be best to > > investigate the comment further and update it in all places in a > > separate patch. > > > > Would it be acceptable to leave the comment as is for now, for > > consistency sake, and I will work on a separate cleanup patch to > > address removing the comment in the original two places and this > > additional place? I will want to look into it a bit more but I > > think > > we can just remove the comment. But I do want to verify that the > > return value is never used first. > > I took a quick look. There are three callers: 1 in dwarf2/loc.c, and > 2 > in jit.c. One of the callers in jit.c does fail to check the return > value, but this looks like a bug to me. The other callers do check > for > a -1 return value. The comment in gdbarch-gen.h is also explicit > that > the right thing to do is return -1. > > Coupled with the fact that the comment, in its current location just > makes no sense I think the best idea would be to drop it from your > new > function. Cleaning up the older code in the future would be nice, > but > is not required. But I don't think we need to introduce more > incorrect > comments just for consistency. > > Thanks, > Andrew > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 14:31 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love @ 2023-10-20 18:08 ` Carl Love 2023-10-24 9:30 ` Andrew Burgess 1 sibling, 1 reply; 27+ messages in thread From: Carl Love @ 2023-10-20 18:08 UTC (permalink / raw) To: Andrew Burgess, Carl Love, gdb-patches, UlrichWeigand, keiths GDB maintainers: Ver2, added a missing blank line, removed an extra blank line, fixed spelling mistakes. Removed comment in ChangeLog about fixing py- thread-exited.exp test errors. Patch just fixes issues with the store.exp. Did not change the comment: > + /* FIXME: jimb/2004-05-05: What should we do when the debug info > + specifies registers the architecture doesn't have? Our > + callers don't check the value we return. */ Will create a new patch to address the comment in all three places in the source code. Note, fixing the register mapping fixes two of the five test failures for the store.exp test. Patch was retested on Power10. This is the first patch in the series which fixes the DWWARF register mapping and signal handling issues on PowerPC. Carl ------------------------------------------------- Fix Linux DWARF register mapping The PowerPC DWARF register mapping is the same for the .eh_frame and .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and .debug_frame on other operating systems. The current GDB support for mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. The files have some legacy mappings for spe_acc, spefscr, EV which was removed from GCC in 2017. This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle the DWARF register mappings on Linux. Function rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum and gdbarch_stab_reg_to_regnum since the mappings are the same. The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to call set_gdbarch_dwarf2_reg_to_regnum map the new function rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, dwarf2_frame_set_adjust_regnum is called to map rs6000_linux_adjust_frame_regnum into the architecture. The second issue fixed by this patch is the check for subroutine process_event_stop_test. Need to make sure the frame is not the SIGTRAMP_FRAME. The sequence of events on most platforms is: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the handler. ... However on PowerPC the sequence of events is: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the trampoline. 3) The trampoline performs a normal function call to the handler. ... We want "nexti" to step into, not over, signal handlers invoked by the kernel. This is the case most platforms as the kernel puts a signal trampoline frame onto the stack to handle proper return after the handler. However, on some platforms such as PowerPC, the kernel actually uses a trampoline to handle *invocation* of the handler. The issue is fixed in function process_event_stop_test by adding a check that the frame is not a SIGTRAMP_FRAME to the if statement to stop in a subroutine call. This prevents GDB from erroneously detecting the trampoline invocation as a subroutine call. This patch fixes two regression test failures in gdb.base/store.exp. Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no new regressions. --- gdb/infrun.c | 13 ++++++++++ gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/gdb/infrun.c b/gdb/infrun.c index 4730d290442..922d8a6913d 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) initial outermost frame, before sp was valid, would have code_addr == &_start. See the comment in frame_id::operator== for more. */ + + /* We want "nexti" to step into, not over, signal handlers invoked + by the kernel, therefore this subroutine check should not trigger + for a signal handler invocation. On most platforms, this is already + not the case, as the kernel puts a signal trampoline frame onto the + stack to handle proper return after the handler, and therefore at this + point, the current frame is a grandchild of the step frame, not a + child. However, on some platforms, the kernel actually uses a + trampoline to handle *invocation* of the handler. In that case, + when executing the first instruction of the trampoline, this check + would erroneously detect the trampoline invocation as a subroutine + call. Fix this by checking for SIGTRAMP_FRAME. */ if ((get_stack_frame_id (frame) != ecs->event_thread->control.step_stack_frame_id) + && get_frame_type (frame) != SIGTRAMP_FRAME && ((frame_unwind_caller_id (get_current_frame ()) == ecs->event_thread->control.step_stack_frame_id) && ((ecs->event_thread->control.step_stack_frame_id diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 784dafa59db..dc03430e2af 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -83,6 +83,7 @@ #include "features/rs6000/powerpc-isa207-vsx64l.c" #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" #include "features/rs6000/powerpc-e500l.c" +#include "dwarf2/frame.h" /* Shared library operations for PowerPC-Linux. */ static struct target_so_ops powerpc_so_ops; @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, return per_inferior->disp_step_buf->prepare (thread, displaced_pc); } +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ + +static int +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); + + if (0 <= num && num <= 31) + return tdep->ppc_gp0_regnum + num; + else if (32 <= num && num <= 63) + /* FIXME: jimb/2004-05-05: What should we do when the debug info + specifies registers the architecture doesn't have? Our + callers don't check the value we return. */ + return tdep->ppc_fp0_regnum + (num - 32); + else if (77 <= num && num < 77 + 32) + return tdep->ppc_vr0_regnum + (num - 77); + else + switch (num) + { + case 65: + return tdep->ppc_lr_regnum; + case 66: + return tdep->ppc_ctr_regnum; + case 76: + return tdep->ppc_xer_regnum; + case 109: + return tdep->ppc_vrsave_regnum; + case 110: + return tdep->ppc_vrsave_regnum - 1; /* vscr */ + } + + /* Unknown DWARF register number. */ + return -1; +} + +/* Translate a .eh_frame register to DWARF register, or adjust a + .debug_frame register. */ + +static int +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, + int eh_frame_p) +{ + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ + return num; +} + static void ppc_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); set_gdbarch_stap_parse_special_token (gdbarch, ppc_stap_parse_special_token); + /* Linux DWARF register mapping is different from the other OSes. */ + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + /* Note on Linux the mapping for the DWARF registers and the stab registers + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the + stab register mappings as well. */ + set_gdbarch_stab_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); if (tdep->wordsize == 4) { -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-20 18:08 ` [PATCH 1/2, ver2] " Carl Love @ 2023-10-24 9:30 ` Andrew Burgess 2023-10-25 13:24 ` Ulrich Weigand 0 siblings, 1 reply; 27+ messages in thread From: Andrew Burgess @ 2023-10-24 9:30 UTC (permalink / raw) To: Carl Love, Carl Love, gdb-patches, UlrichWeigand, keiths Carl Love <cel@linux.ibm.com> writes: > GDB maintainers: > > Ver2, added a missing blank line, removed an extra blank line, fixed > spelling mistakes. Removed comment in ChangeLog about fixing py- > thread-exited.exp test errors. Patch just fixes issues with the > store.exp. Did not change the comment: > >> + /* FIXME: jimb/2004-05-05: What should we do when the debug info >> + specifies registers the architecture doesn't have? Our >> + callers don't check the value we return. */ > > Will create a new patch to address the comment in all three places in > the source code. > > Note, fixing the register mapping fixes two of the five test failures > for the store.exp test. > > Patch was retested on Power10. > > This is the first patch in the series which fixes the DWWARF register > mapping and signal handling issues on PowerPC. > > Carl > > > ------------------------------------------------- > Fix Linux DWARF register mapping > > The PowerPC DWARF register mapping is the same for the .eh_frame and > .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and > .debug_frame on other operating systems. The current GDB support for > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. > The files have some legacy mappings for spe_acc, spefscr, EV which was > removed from GCC in 2017. > > This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle > the DWARF register mappings on Linux. Function > rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to > call set_gdbarch_dwarf2_reg_to_regnum map the new function > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > dwarf2_frame_set_adjust_regnum is called to map > rs6000_linux_adjust_frame_regnum into the architecture. > > The second issue fixed by this patch is the check for subroutine > process_event_stop_test. Need to make sure the frame is not the > SIGTRAMP_FRAME. The sequence of events on most platforms is: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the handler. > ... > > However on PowerPC the sequence of events is: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the trampoline. > 3) The trampoline performs a normal function call to the handler. > ... > > We want "nexti" to step into, not over, signal handlers invoked > by the kernel. This is the case most platforms as the kernel puts a > signal trampoline frame onto the stack to handle proper return after the > handler. However, on some platforms such as PowerPC, the kernel actually > uses a trampoline to handle *invocation* of the handler. > > The issue is fixed in function process_event_stop_test by adding a check > that the frame is not a SIGTRAMP_FRAME to the if statement to stop in > a subroutine call. This prevents GDB from erroneously detecting the > trampoline invocation as a subroutine call. > > This patch fixes two regression test failures in gdb.base/store.exp. > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no > new regressions. Hi Carl, Sorry for being such a pain w.r.t. this patch. Honestly, it's mostly because you touched infrun.c that I'm so interested here. I left some notes here that need addressing: https://inbox.sourceware.org/gdb-patches/6f9c8fa20962129048384d74f6f15d1b37d255ed.camel@us.ibm.com/T/#m83b4f0d7da45f15c2df44344fbf3e326cf7435e3 But I wonder if you could be more specific about when you would expect to see the failures in gdb.base/store.exp. I grabbed a random Power9 Linux machine and tried running store.exp with an unpatched GDB, and see no failures. I guess there's a specific architecture/compiler combo that I need in order to see failures -- but if those details are in the above text, I'm just not seeing them. Could you give some more details about the setup needed to see failures on store.exp. And I'd like to see clearer details about which tests the infrun.c changes will fix. Thanks, Andrew > --- > gdb/infrun.c | 13 ++++++++++ > gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 4730d290442..922d8a6913d 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) > initial outermost frame, before sp was valid, would > have code_addr == &_start. See the comment in frame_id::operator== > for more. */ > + > + /* We want "nexti" to step into, not over, signal handlers invoked > + by the kernel, therefore this subroutine check should not trigger > + for a signal handler invocation. On most platforms, this is already > + not the case, as the kernel puts a signal trampoline frame onto the > + stack to handle proper return after the handler, and therefore at this > + point, the current frame is a grandchild of the step frame, not a > + child. However, on some platforms, the kernel actually uses a > + trampoline to handle *invocation* of the handler. In that case, > + when executing the first instruction of the trampoline, this check > + would erroneously detect the trampoline invocation as a subroutine > + call. Fix this by checking for SIGTRAMP_FRAME. */ > if ((get_stack_frame_id (frame) > != ecs->event_thread->control.step_stack_frame_id) > + && get_frame_type (frame) != SIGTRAMP_FRAME > && ((frame_unwind_caller_id (get_current_frame ()) > == ecs->event_thread->control.step_stack_frame_id) > && ((ecs->event_thread->control.step_stack_frame_id > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 784dafa59db..dc03430e2af 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -83,6 +83,7 @@ > #include "features/rs6000/powerpc-isa207-vsx64l.c" > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > +#include "dwarf2/frame.h" > > /* Shared library operations for PowerPC-Linux. */ > static struct target_so_ops powerpc_so_ops; > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, > return per_inferior->disp_step_buf->prepare (thread, displaced_pc); > } > > +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ > + > +static int > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > + > + if (0 <= num && num <= 31) > + return tdep->ppc_gp0_regnum + num; > + else if (32 <= num && num <= 63) > + /* FIXME: jimb/2004-05-05: What should we do when the debug info > + specifies registers the architecture doesn't have? Our > + callers don't check the value we return. */ > + return tdep->ppc_fp0_regnum + (num - 32); > + else if (77 <= num && num < 77 + 32) > + return tdep->ppc_vr0_regnum + (num - 77); > + else > + switch (num) > + { > + case 65: > + return tdep->ppc_lr_regnum; > + case 66: > + return tdep->ppc_ctr_regnum; > + case 76: > + return tdep->ppc_xer_regnum; > + case 109: > + return tdep->ppc_vrsave_regnum; > + case 110: > + return tdep->ppc_vrsave_regnum - 1; /* vscr */ > + } > + > + /* Unknown DWARF register number. */ > + return -1; > +} > + > +/* Translate a .eh_frame register to DWARF register, or adjust a > + .debug_frame register. */ > + > +static int > +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, > + int eh_frame_p) > +{ > + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ > + return num; > +} > + > static void > ppc_linux_init_abi (struct gdbarch_info info, > struct gdbarch *gdbarch) > @@ -2135,6 +2182,15 @@ ppc_linux_init_abi (struct gdbarch_info info, > set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); > set_gdbarch_stap_parse_special_token (gdbarch, > ppc_stap_parse_special_token); > + /* Linux DWARF register mapping is different from the other OSes. */ > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + /* Note on Linux the mapping for the DWARF registers and the stab registers > + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the > + stab register mappings as well. */ > + set_gdbarch_stab_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); > > if (tdep->wordsize == 4) > { > -- > 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-24 9:30 ` Andrew Burgess @ 2023-10-25 13:24 ` Ulrich Weigand 2023-10-30 9:45 ` Andrew Burgess 0 siblings, 1 reply; 27+ messages in thread From: Ulrich Weigand @ 2023-10-25 13:24 UTC (permalink / raw) To: gdb-patches, Andrew Burgess, cel, Keith Seitz Andrew Burgess <aburgess@redhat.com> wrote: >Sorry for being such a pain w.r.t. this patch. Honestly, it's mostly >because you touched infrun.c that I'm so interested here. [...] >And I'd like to see clearer details about which tests the infrun.c >changes will fix. Hi Andrew, as I suggested that particular infrun.c change to Carl, let me add some background to what Carl replied. This one-line change in infrun.c fixes a (potential) common-code bug in existing code. However, this bug is likely dormant on all current platforms: First of all, the bug only shows up on platforms where the kernel uses a trampoline to dispatch *calls to* the signal handler (not just *returns from* the signal handler). Many platforms use a trampoline for signal return, and that is working fine, but the only platform I'm aware of that uses a trampoline for signal handler calls is (recent kernels for) PowerPC. I believe the rationale for using a trampoline here is to improve performance by avoiding unbalancing of the branch predictor's call/return stack. However, on PowerPC the bug is dormant as well as it is hidden by *another* bug that prevents correct unwinding out of the signal trampoline. This is because the custom CFI for the trampoline uses a register number (VSCR) that is not ever used by compiler-generated CFI, and that particular register is mapped to an invalid number by the current PowerPC DWARF mapper. Now, as Carl fixed the PowerPC DWARF mapper to fix some test case regressions that showed up on POWER10, he added correct mappings for all registers including VSCR. This then caused unwinding out of the signal trampoline to start working correctly, which in turn exposed the pre-existing infrun.c bug in handling signal call trampolines. Now, that particular bug is that if the kernel uses a signal call trampoline, infrun.c will see that the current PC is in a frame that is an immediate child of the step frame (as the trampoline frame unwinds to the step frame) - and then incorrectly thinks that a *subroutine call* must have happened. But we don't want to treat a signal handler invocation like a subroutine call, in particular we want "nexti" to step into it. That's fixed by the one-line change. Bye, Ulrich ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-25 13:24 ` Ulrich Weigand @ 2023-10-30 9:45 ` Andrew Burgess 2023-10-30 16:44 ` Ulrich Weigand 0 siblings, 1 reply; 27+ messages in thread From: Andrew Burgess @ 2023-10-30 9:45 UTC (permalink / raw) To: Ulrich Weigand, gdb-patches, cel, Keith Seitz Ulrich Weigand <Ulrich.Weigand@de.ibm.com> writes: > Andrew Burgess <aburgess@redhat.com> wrote: > >>Sorry for being such a pain w.r.t. this patch. Honestly, it's mostly >>because you touched infrun.c that I'm so interested here. > [...] >>And I'd like to see clearer details about which tests the infrun.c >>changes will fix. > > Hi Andrew, as I suggested that particular infrun.c change > to Carl, let me add some background to what Carl replied. > > This one-line change in infrun.c fixes a (potential) common-code > bug in existing code. However, this bug is likely dormant on > all current platforms: > > First of all, the bug only shows up on platforms where the > kernel uses a trampoline to dispatch *calls to* the signal > handler (not just *returns from* the signal handler). Many > platforms use a trampoline for signal return, and that is > working fine, but the only platform I'm aware of that uses > a trampoline for signal handler calls is (recent kernels for) > PowerPC. I believe the rationale for using a trampoline here > is to improve performance by avoiding unbalancing of the > branch predictor's call/return stack. > > However, on PowerPC the bug is dormant as well as it is hidden > by *another* bug that prevents correct unwinding out of the > signal trampoline. This is because the custom CFI for the > trampoline uses a register number (VSCR) that is not ever used > by compiler-generated CFI, and that particular register is > mapped to an invalid number by the current PowerPC DWARF > mapper. Ulrich, Thanks for the great explanation. I think adding this to the commit message would be a great improvement. But also, you said: > Now, as Carl fixed the PowerPC DWARF mapper to fix some test > case regressions that showed up on POWER10, he added correct > mappings for all registers including VSCR. This then caused > unwinding out of the signal trampoline to start working > correctly, which in turn exposed the pre-existing infrun.c bug > in handling signal call trampolines. As part of the commit message at this point I'd like to see the name of at least one (if the number is small, I'd list them all though) test(s) that required this change in order to keep working correctly. Often, when I end up looking at old commits in the history, I want to know, what is an example of something that this change actually fixes, and it's super frustrating when I have to try and figure this information out for myself -- given that it must have been known at commit time. > > Now, that particular bug is that if the kernel uses a signal > call trampoline, infrun.c will see that the current PC is > in a frame that is an immediate child of the step frame (as > the trampoline frame unwinds to the step frame) - and then > incorrectly thinks that a *subroutine call* must have happened. > But we don't want to treat a signal handler invocation like > a subroutine call, in particular we want "nexti" to step > into it. That's fixed by the one-line change. Thanks, Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-30 9:45 ` Andrew Burgess @ 2023-10-30 16:44 ` Ulrich Weigand 2023-10-30 17:16 ` Carl Love 2023-10-30 17:25 ` [PATCH 1/2, ver3] " Carl Love 0 siblings, 2 replies; 27+ messages in thread From: Ulrich Weigand @ 2023-10-30 16:44 UTC (permalink / raw) To: gdb-patches, Andrew Burgess, cel, Keith Seitz Andrew Burgess <aburgess@redhat.com> wrote: >I think adding this to the commit message would be a great improvement. >But also, you said: > > >Now, as Carl fixed the PowerPC DWARF mapper to fix some test > >case regressions that showed up on POWER10, he added correct > >mappings for all registers including VSCR. This then caused > >unwinding out of the signal trampoline to start working > >correctly, which in turn exposed the pre-existing infrun.c bug > >in handling signal call trampolines. > >As part of the commit message at this point I'd like to see the name of >at least one (if the number is small, I'd list them all though) test(s) >that required this change in order to keep working correctly. I believe this showed up in sigstep.exp, but Carl would know for sure. Carl, please update the commit message as requested by Andrew. Thanks, Ulrich ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2, ver2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-30 16:44 ` Ulrich Weigand @ 2023-10-30 17:16 ` Carl Love 2023-10-30 17:25 ` [PATCH 1/2, ver3] " Carl Love 1 sibling, 0 replies; 27+ messages in thread From: Carl Love @ 2023-10-30 17:16 UTC (permalink / raw) To: Ulrich Weigand, gdb-patches, Andrew Burgess, Keith Seitz; +Cc: cel Ulrich, Andrew: On Mon, 2023-10-30 at 16:44 +0000, Ulrich Weigand wrote: > Andrew Burgess <aburgess@redhat.com> wrote: > > > I think adding this to the commit message would be a great > > improvement. > > But also, you said: > > > > > Now, as Carl fixed the PowerPC DWARF mapper to fix some test > > > case regressions that showed up on POWER10, he added correct > > > mappings for all registers including VSCR. This then caused > > > unwinding out of the signal trampoline to start working > > > correctly, which in turn exposed the pre-existing infrun.c bug > > > in handling signal call trampolines. > > > > As part of the commit message at this point I'd like to see the > > name of > > at least one (if the number is small, I'd list them all though) > > test(s) > > that required this change in order to keep working correctly. > > I believe this showed up in sigstep.exp, but Carl would know for > sure. > Carl, please update the commit message as requested by Andrew. I have updated the commit message adding some of the additional detail that Ulrch gave on the underlying issue. The commit message now starts with an overview of the initial issue to be fixed in store.exp followed by a discussion of the underlying bug that was then exposed by test sigstep.exp. The commit message then goes on with the detailed discussion of the fix for both issues. Finally, it summarizes the failures in both store.exp and sigstep.exp that are fixed. Hopefully this updated commit message clearly explains the two separate issues that the patch addresses. Thanks for the feedback from Andrew and the help from Ulrich to improve the commit message. I will post the update patch for your review. Carl ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2, ver3] PowerPC, Fix-test-gdb.base-store.exp 2023-10-30 16:44 ` Ulrich Weigand 2023-10-30 17:16 ` Carl Love @ 2023-10-30 17:25 ` Carl Love 2023-11-06 18:24 ` Carl Love 2023-11-08 10:54 ` Andrew Burgess 1 sibling, 2 replies; 27+ messages in thread From: Carl Love @ 2023-10-30 17:25 UTC (permalink / raw) To: Ulrich Weigand, gdb-patches, Andrew Burgess, Keith Seitz; +Cc: cel Andrew, Ulrich: Per the discussions, the commit message has been updated to make it clear that the patch fixes the DWARF register mapping which fixes two of the test failures in step.exp. The fix exposed a dormant issue with the signal handling on PowerPC. The underlying issue which is then exposed causes the signal handling test sigstep.exp to fail. The test sigstep.exp tests stepping into a signal handler. The patch also fixes the underlying signal handler issue. This new verion of the patch only updates the commit log to make it clear the patch is fixing multiple issues. Please let me know if the commit description is clear and acceptable. Thanks. Carl ------------------------------------------------------------ rs6000, Fix Linux DWARF register mapping Overview of issues fixed by the patch. The primary issue this patch fixes is the DWARF register mapping for Linux. The changes in ppc-linux-tdep.c fix the DWARF register mapping issues. The register mapping issue is responsible for two of the five regression bugs seen in gdb.base/store.exp. Once the register mapping is fixed, an underlying issue with the unwinding of the signal trampoline in common-code in ifrun.c was found. This underlying bug is best described by Ulrich in the following description. The unwinder bug shows up on platforms where the kernel uses a trampoline to dispatch "calls to" the signal handler (not just *returns from* the signal handler). Many platforms use a trampoline for signal return, and that is working fine, but the only platform I'm (Ulrich) aware of that uses a trampoline for signal handler calls is (recent kernels for) PowerPC. I believe the rationale for using a trampoline here is to improve performance by avoiding unbalancing of the branch predictor's call/return stack. However, on PowerPC the bug is dormant as well as it is hidden by *another* bug that prevents correct unwinding out of the signal trampoline. This is because the custom CFI for the trampoline uses a register number (VSCR) that is not ever used by compiler-generated CFI, and that particular register is mapped to an invalid number by the current PowerPC DWARF mapper. The underlying unwinder bug is exposed by the "new" regression failures in gdb.base/sigstep.exp. These failures were previously masked by the fact that GDB was not seeing a valid frame when it tried to unwind the frames. The sigstep.exp test is specifically testing stepping into a signal handler. With the correct DWARF register mapping in place, specifically the VSCR mapping, the signal trampoline code now unwinds to a valid frame exposing the pre-existing bug in how the signal handler on PowerPC works. The one line change infrun.c fixes the exiting bug in the common-code for platforms that use a trampoline to dispatch calls to the signal handler by not stopping in the SIGTRAMP_FRAME. Detailed description of the DWARF register mapping fix. The PowerPC DWARF register mapping is the same for the .eh_frame and .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and .debug_frame on other operating systems. The current GDB support for mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. The files have some legacy mappings for spe_acc, spefscr, EV which was removed from GCC in 2017. This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle the DWARF register mappings on Linux. Function rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum and gdbarch_stab_reg_to_regnum since the mappings are the same. The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to call set_gdbarch_dwarf2_reg_to_regnum map the new function rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, dwarf2_frame_set_adjust_regnum is called to map rs6000_linux_adjust_frame_regnum into the architecture. Additional detail on the signal handling fix. The specific sequence of events for handling a signal on most architectures is as follows: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the handler. ... However on PowerPC the sequence of events is: 1) Some code is running when a signal arrives. 2) The kernel handles the signal and dispatches to the trampoline. 3) The trampoline performs a normal function call to the handler. ... We want the "nexti" to step into, not over, signal handlers invoked by the kernel. This is the case for most platforms as the kernel puts a signal trampoline frame onto the stack to handle proper return after the handler. However, on some platforms such as PowerPC, the kernel actually uses a trampoline to handle *invocation* of the handler. We do not want GDB to stop in the SIGTRAMP_FRAME. The issue is fixed in function process_event_stop_test by adding a check that the frame is not a SIGTRAMP_FRAME to the if statement to stop in a subroutine call. This prevents GDB from erroneously detecting the trampoline invocation as a subroutine call. This patch fixes two regression test failures in gdb.base/store.exp. The patch then fixes an exposed, dormant, signal handling issue that is exposed in the signal handling test gdb.base/sigstep.exp. The patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no new regressions. Note, only two of the five failures in store.exp are fixed. The remaining three failures are fixed in a following patch. --- gdb/infrun.c | 13 +++++++++++ gdb/ppc-linux-tdep.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/gdb/infrun.c b/gdb/infrun.c index 4730d290442..922d8a6913d 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) initial outermost frame, before sp was valid, would have code_addr == &_start. See the comment in frame_id::operator== for more. */ + + /* We want "nexti" to step into, not over, signal handlers invoked + by the kernel, therefore this subroutine check should not trigger + for a signal handler invocation. On most platforms, this is already + not the case, as the kernel puts a signal trampoline frame onto the + stack to handle proper return after the handler, and therefore at this + point, the current frame is a grandchild of the step frame, not a + child. However, on some platforms, the kernel actually uses a + trampoline to handle *invocation* of the handler. In that case, + when executing the first instruction of the trampoline, this check + would erroneously detect the trampoline invocation as a subroutine + call. Fix this by checking for SIGTRAMP_FRAME. */ if ((get_stack_frame_id (frame) != ecs->event_thread->control.step_stack_frame_id) + && get_frame_type (frame) != SIGTRAMP_FRAME && ((frame_unwind_caller_id (get_current_frame ()) == ecs->event_thread->control.step_stack_frame_id) && ((ecs->event_thread->control.step_stack_frame_id diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 784dafa59db..8d975336fe5 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -83,6 +83,7 @@ #include "features/rs6000/powerpc-isa207-vsx64l.c" #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" #include "features/rs6000/powerpc-e500l.c" +#include "dwarf2/frame.h" /* Shared library operations for PowerPC-Linux. */ static struct target_so_ops powerpc_so_ops; @@ -2088,6 +2089,49 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, return per_inferior->disp_step_buf->prepare (thread, displaced_pc); } +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ + +static int +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); + + if (0 <= num && num <= 31) + return tdep->ppc_gp0_regnum + num; + else if (32 <= num && num <= 63) + return tdep->ppc_fp0_regnum + (num - 32); + else if (77 <= num && num < 77 + 32) + return tdep->ppc_vr0_regnum + (num - 77); + else + switch (num) + { + case 65: + return tdep->ppc_lr_regnum; + case 66: + return tdep->ppc_ctr_regnum; + case 76: + return tdep->ppc_xer_regnum; + case 109: + return tdep->ppc_vrsave_regnum; + case 110: + return tdep->ppc_vrsave_regnum - 1; /* vscr */ + } + + /* Unknown DWARF register number. */ + return -1; +} + +/* Translate a .eh_frame register to DWARF register, or adjust a + .debug_frame register. */ + +static int +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, + int eh_frame_p) +{ + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ + return num; +} + static void ppc_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) @@ -2135,6 +2179,15 @@ ppc_linux_init_abi (struct gdbarch_info info, set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); set_gdbarch_stap_parse_special_token (gdbarch, ppc_stap_parse_special_token); + /* Linux DWARF register mapping is different from the other OSes. */ + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + /* Note on Linux the mapping for the DWARF registers and the stab registers + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the + stab register mappings as well. */ + set_gdbarch_stab_reg_to_regnum (gdbarch, + rs6000_linux_dwarf2_reg_to_regnum); + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); if (tdep->wordsize == 4) { -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2, ver3] PowerPC, Fix-test-gdb.base-store.exp 2023-10-30 17:25 ` [PATCH 1/2, ver3] " Carl Love @ 2023-11-06 18:24 ` Carl Love 2023-11-08 10:54 ` Andrew Burgess 1 sibling, 0 replies; 27+ messages in thread From: Carl Love @ 2023-11-06 18:24 UTC (permalink / raw) To: Ulrich Weigand, gdb-patches, Andrew Burgess, Keith Seitz; +Cc: cel Ping Andrew: Is the updated commit log OK? Carl ----------------------------------------- On Mon, 2023-10-30 at 10:25 -0700, Carl Love wrote: > Andrew, Ulrich: > > Per the discussions, the commit message has been updated to make it > clear that the patch fixes the DWARF register mapping which fixes two > of the test failures in step.exp. The fix exposed a dormant issue > with > the signal handling on PowerPC. The underlying issue which is then > exposed causes the signal handling test sigstep.exp to fail. The > test > sigstep.exp tests stepping into a signal handler. The patch also > fixes > the underlying signal handler issue. > > This new verion of the patch only updates the commit log to make it > clear the patch is fixing multiple issues. > > Please let me know if the commit description is clear and acceptable. > > Thanks. > > Carl > ------------------------------------------------------------ > rs6000, Fix Linux DWARF register mapping > > Overview of issues fixed by the patch. > > The primary issue this patch fixes is the DWARF register mapping for > Linux. The changes in ppc-linux-tdep.c fix the DWARF register > mapping > issues. The register mapping issue is responsible for two of the > five regression bugs seen in gdb.base/store.exp. > > Once the register mapping is fixed, an underlying issue with the > unwinding > of the signal trampoline in common-code in ifrun.c was found. This > underlying bug is best described by Ulrich in the following > description. > > The unwinder bug shows up on platforms where the kernel uses a > trampoline > to dispatch "calls to" the signal handler (not just *returns from* > the > signal handler). Many platforms use a trampoline for signal > return, and > that is working fine, but the only platform I'm (Ulrich) aware of > that > uses a trampoline for signal handler calls is (recent kernels for) > PowerPC. I believe the rationale for using a trampoline here > is to improve performance by avoiding unbalancing of the > branch predictor's call/return stack. > > However, on PowerPC the bug is dormant as well as it is hidden > by *another* bug that prevents correct unwinding out of the > signal trampoline. This is because the custom CFI for the > trampoline uses a register number (VSCR) that is not ever used > by compiler-generated CFI, and that particular register is > mapped to an invalid number by the current PowerPC DWARF mapper. > > The underlying unwinder bug is exposed by the "new" regression > failures > in gdb.base/sigstep.exp. These failures were previously masked by > the fact that GDB was not seeing a valid frame when it tried to > unwind > the frames. The sigstep.exp test is specifically testing stepping > into > a signal handler. With the correct DWARF register mapping in place, > specifically the VSCR mapping, the signal trampoline code now unwinds > to a > valid frame exposing the pre-existing bug in how the signal handler > on > PowerPC works. The one line change infrun.c fixes the exiting bug in > the common-code for platforms that use a trampoline to dispatch calls > to the signal handler by not stopping in the SIGTRAMP_FRAME. > > Detailed description of the DWARF register mapping fix. > > The PowerPC DWARF register mapping is the same for the .eh_frame and > .debug_frame on Linux. PowerPC uses different mapping for .eh_frame > and > .debug_frame on other operating systems. The current GDB support for > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for > Linux. > The files have some legacy mappings for spe_acc, spefscr, EV which > was > removed from GCC in 2017. > > This patch adds a two new functions > rs6000_linux_dwarf2_reg_to_regnum, > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to > handle > the DWARF register mappings on Linux. Function > rs6000_linux_dwarf2_reg_to_regnum is installed for both > gdb_dwarf_to_regnum > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to > call set_gdbarch_dwarf2_reg_to_regnum map the new function > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > dwarf2_frame_set_adjust_regnum is called to map > rs6000_linux_adjust_frame_regnum into the architecture. > > Additional detail on the signal handling fix. > > The specific sequence of events for handling a signal on most > architectures is as follows: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the handler. > ... > > However on PowerPC the sequence of events is: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the trampoline. > 3) The trampoline performs a normal function call to the handler. > ... > > We want the "nexti" to step into, not over, signal handlers invoked > by > the kernel. This is the case for most platforms as the kernel puts a > signal trampoline frame onto the stack to handle proper return after > the > handler. However, on some platforms such as PowerPC, the kernel > actually > uses a trampoline to handle *invocation* of the handler. We do not > want GDB to stop in the SIGTRAMP_FRAME. The issue is fixed in > function > process_event_stop_test by adding a check that the frame is not a > SIGTRAMP_FRAME to the if statement to stop in a subroutine > call. This > prevents GDB from erroneously detecting the trampoline invocation as > a > subroutine call. > > This patch fixes two regression test failures in gdb.base/store.exp. > > The patch then fixes an exposed, dormant, signal handling issue that > is exposed in the signal handling test gdb.base/sigstep.exp. > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > with > no new regressions. Note, only two of the five failures in store.exp > are fixed. The remaining three failures are fixed in a following > patch. > --- > gdb/infrun.c | 13 +++++++++++ > gdb/ppc-linux-tdep.c | 53 > ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 4730d290442..922d8a6913d 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct > execution_control_state *ecs) > initial outermost frame, before sp was valid, would > have code_addr == &_start. See the comment in > frame_id::operator== > for more. */ > + > + /* We want "nexti" to step into, not over, signal handlers invoked > + by the kernel, therefore this subroutine check should not > trigger > + for a signal handler invocation. On most platforms, this is > already > + not the case, as the kernel puts a signal trampoline frame onto > the > + stack to handle proper return after the handler, and therefore > at this > + point, the current frame is a grandchild of the step frame, not > a > + child. However, on some platforms, the kernel actually uses a > + trampoline to handle *invocation* of the handler. In that > case, > + when executing the first instruction of the trampoline, this > check > + would erroneously detect the trampoline invocation as a > subroutine > + call. Fix this by checking for SIGTRAMP_FRAME. */ > if ((get_stack_frame_id (frame) > != ecs->event_thread->control.step_stack_frame_id) > + && get_frame_type (frame) != SIGTRAMP_FRAME > && ((frame_unwind_caller_id (get_current_frame ()) > == ecs->event_thread->control.step_stack_frame_id) > && ((ecs->event_thread->control.step_stack_frame_id > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 784dafa59db..8d975336fe5 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -83,6 +83,7 @@ > #include "features/rs6000/powerpc-isa207-vsx64l.c" > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > +#include "dwarf2/frame.h" > > /* Shared library operations for PowerPC-Linux. */ > static struct target_so_ops powerpc_so_ops; > @@ -2088,6 +2089,49 @@ ppc_linux_displaced_step_prepare (gdbarch > *arch, thread_info *thread, > return per_inferior->disp_step_buf->prepare (thread, > displaced_pc); > } > > +/* Convert a Dwarf 2 register number to a GDB register number for > Linux. */ > + > +static int > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > + > + if (0 <= num && num <= 31) > + return tdep->ppc_gp0_regnum + num; > + else if (32 <= num && num <= 63) > + return tdep->ppc_fp0_regnum + (num - 32); > + else if (77 <= num && num < 77 + 32) > + return tdep->ppc_vr0_regnum + (num - 77); > + else > + switch (num) > + { > + case 65: > + return tdep->ppc_lr_regnum; > + case 66: > + return tdep->ppc_ctr_regnum; > + case 76: > + return tdep->ppc_xer_regnum; > + case 109: > + return tdep->ppc_vrsave_regnum; > + case 110: > + return tdep->ppc_vrsave_regnum - 1; /* vscr */ > + } > + > + /* Unknown DWARF register number. */ > + return -1; > +} > + > +/* Translate a .eh_frame register to DWARF register, or adjust a > + .debug_frame register. */ > + > +static int > +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, > + int eh_frame_p) > +{ > + /* Linux uses the same numbering for .debug_frame numbering as > .eh_frame. */ > + return num; > +} > + > static void > ppc_linux_init_abi (struct gdbarch_info info, > struct gdbarch *gdbarch) > @@ -2135,6 +2179,15 @@ ppc_linux_init_abi (struct gdbarch_info info, > set_gdbarch_stap_is_single_operand (gdbarch, > ppc_stap_is_single_operand); > set_gdbarch_stap_parse_special_token (gdbarch, > ppc_stap_parse_special_token); > + /* Linux DWARF register mapping is different from the other > OSes. */ > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + /* Note on Linux the mapping for the DWARF registers and the stab > registers > + use the same numbers. Install > rs6000_linux_dwarf2_reg_to_regnum for the > + stab register mappings as well. */ > + set_gdbarch_stab_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + dwarf2_frame_set_adjust_regnum (gdbarch, > rs6000_linux_adjust_frame_regnum); > > if (tdep->wordsize == 4) > { ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2, ver3] PowerPC, Fix-test-gdb.base-store.exp 2023-10-30 17:25 ` [PATCH 1/2, ver3] " Carl Love 2023-11-06 18:24 ` Carl Love @ 2023-11-08 10:54 ` Andrew Burgess 1 sibling, 0 replies; 27+ messages in thread From: Andrew Burgess @ 2023-11-08 10:54 UTC (permalink / raw) To: Carl Love, Ulrich Weigand, gdb-patches, Keith Seitz; +Cc: cel Carl Love <cel@linux.ibm.com> writes: > Andrew, Ulrich: > > Per the discussions, the commit message has been updated to make it > clear that the patch fixes the DWARF register mapping which fixes two > of the test failures in step.exp. The fix exposed a dormant issue with > the signal handling on PowerPC. The underlying issue which is then > exposed causes the signal handling test sigstep.exp to fail. The test > sigstep.exp tests stepping into a signal handler. The patch also fixes > the underlying signal handler issue. > > This new verion of the patch only updates the commit log to make it > clear the patch is fixing multiple issues. > > Please let me know if the commit description is clear and acceptable. > > Thanks. > > Carl > ------------------------------------------------------------ > rs6000, Fix Linux DWARF register mapping > > Overview of issues fixed by the patch. > > The primary issue this patch fixes is the DWARF register mapping for > Linux. The changes in ppc-linux-tdep.c fix the DWARF register mapping > issues. The register mapping issue is responsible for two of the > five regression bugs seen in gdb.base/store.exp. > > Once the register mapping is fixed, an underlying issue with the unwinding > of the signal trampoline in common-code in ifrun.c was found. This > underlying bug is best described by Ulrich in the following description. > > The unwinder bug shows up on platforms where the kernel uses a trampoline > to dispatch "calls to" the signal handler (not just *returns from* the > signal handler). Many platforms use a trampoline for signal return, and > that is working fine, but the only platform I'm (Ulrich) aware of that > uses a trampoline for signal handler calls is (recent kernels for) > PowerPC. I believe the rationale for using a trampoline here > is to improve performance by avoiding unbalancing of the > branch predictor's call/return stack. > > However, on PowerPC the bug is dormant as well as it is hidden > by *another* bug that prevents correct unwinding out of the > signal trampoline. This is because the custom CFI for the > trampoline uses a register number (VSCR) that is not ever used > by compiler-generated CFI, and that particular register is > mapped to an invalid number by the current PowerPC DWARF mapper. > > The underlying unwinder bug is exposed by the "new" regression failures > in gdb.base/sigstep.exp. These failures were previously masked by > the fact that GDB was not seeing a valid frame when it tried to unwind > the frames. The sigstep.exp test is specifically testing stepping into > a signal handler. With the correct DWARF register mapping in place, > specifically the VSCR mapping, the signal trampoline code now unwinds to a > valid frame exposing the pre-existing bug in how the signal handler on > PowerPC works. The one line change infrun.c fixes the exiting bug in > the common-code for platforms that use a trampoline to dispatch calls > to the signal handler by not stopping in the SIGTRAMP_FRAME. > > Detailed description of the DWARF register mapping fix. > > The PowerPC DWARF register mapping is the same for the .eh_frame and > .debug_frame on Linux. PowerPC uses different mapping for .eh_frame and > .debug_frame on other operating systems. The current GDB support for > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux. > The files have some legacy mappings for spe_acc, spefscr, EV which was > removed from GCC in 2017. > > This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum, > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle > the DWARF register mappings on Linux. Function > rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to > call set_gdbarch_dwarf2_reg_to_regnum map the new function > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > dwarf2_frame_set_adjust_regnum is called to map > rs6000_linux_adjust_frame_regnum into the architecture. > > Additional detail on the signal handling fix. > > The specific sequence of events for handling a signal on most > architectures is as follows: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the handler. > ... > > However on PowerPC the sequence of events is: > > 1) Some code is running when a signal arrives. > 2) The kernel handles the signal and dispatches to the trampoline. > 3) The trampoline performs a normal function call to the handler. > ... > > We want the "nexti" to step into, not over, signal handlers invoked by > the kernel. This is the case for most platforms as the kernel puts a > signal trampoline frame onto the stack to handle proper return after the > handler. However, on some platforms such as PowerPC, the kernel actually > uses a trampoline to handle *invocation* of the handler. We do not > want GDB to stop in the SIGTRAMP_FRAME. The issue is fixed in function > process_event_stop_test by adding a check that the frame is not a > SIGTRAMP_FRAME to the if statement to stop in a subroutine call. This > prevents GDB from erroneously detecting the trampoline invocation as a > subroutine call. > > This patch fixes two regression test failures in gdb.base/store.exp. > > The patch then fixes an exposed, dormant, signal handling issue that > is exposed in the signal handling test gdb.base/sigstep.exp. > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with > no new regressions. Note, only two of the five failures in store.exp > are fixed. The remaining three failures are fixed in a following > patch. Carl, Sorry for the slow delay, I had to find some time to play with this a little. But thanks to the expanded explanation, it's now clear what's going on. This all looks great. Approved-By: Andrew Burgess <aburgess@redhat.com> Thanks, Andrew > --- > gdb/infrun.c | 13 +++++++++++ > gdb/ppc-linux-tdep.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 4730d290442..922d8a6913d 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs) > initial outermost frame, before sp was valid, would > have code_addr == &_start. See the comment in frame_id::operator== > for more. */ > + > + /* We want "nexti" to step into, not over, signal handlers invoked > + by the kernel, therefore this subroutine check should not trigger > + for a signal handler invocation. On most platforms, this is already > + not the case, as the kernel puts a signal trampoline frame onto the > + stack to handle proper return after the handler, and therefore at this > + point, the current frame is a grandchild of the step frame, not a > + child. However, on some platforms, the kernel actually uses a > + trampoline to handle *invocation* of the handler. In that case, > + when executing the first instruction of the trampoline, this check > + would erroneously detect the trampoline invocation as a subroutine > + call. Fix this by checking for SIGTRAMP_FRAME. */ > if ((get_stack_frame_id (frame) > != ecs->event_thread->control.step_stack_frame_id) > + && get_frame_type (frame) != SIGTRAMP_FRAME > && ((frame_unwind_caller_id (get_current_frame ()) > == ecs->event_thread->control.step_stack_frame_id) > && ((ecs->event_thread->control.step_stack_frame_id > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 784dafa59db..8d975336fe5 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -83,6 +83,7 @@ > #include "features/rs6000/powerpc-isa207-vsx64l.c" > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > +#include "dwarf2/frame.h" > > /* Shared library operations for PowerPC-Linux. */ > static struct target_so_ops powerpc_so_ops; > @@ -2088,6 +2089,49 @@ ppc_linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, > return per_inferior->disp_step_buf->prepare (thread, displaced_pc); > } > > +/* Convert a Dwarf 2 register number to a GDB register number for Linux. */ > + > +static int > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch); > + > + if (0 <= num && num <= 31) > + return tdep->ppc_gp0_regnum + num; > + else if (32 <= num && num <= 63) > + return tdep->ppc_fp0_regnum + (num - 32); > + else if (77 <= num && num < 77 + 32) > + return tdep->ppc_vr0_regnum + (num - 77); > + else > + switch (num) > + { > + case 65: > + return tdep->ppc_lr_regnum; > + case 66: > + return tdep->ppc_ctr_regnum; > + case 76: > + return tdep->ppc_xer_regnum; > + case 109: > + return tdep->ppc_vrsave_regnum; > + case 110: > + return tdep->ppc_vrsave_regnum - 1; /* vscr */ > + } > + > + /* Unknown DWARF register number. */ > + return -1; > +} > + > +/* Translate a .eh_frame register to DWARF register, or adjust a > + .debug_frame register. */ > + > +static int > +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num, > + int eh_frame_p) > +{ > + /* Linux uses the same numbering for .debug_frame numbering as .eh_frame. */ > + return num; > +} > + > static void > ppc_linux_init_abi (struct gdbarch_info info, > struct gdbarch *gdbarch) > @@ -2135,6 +2179,15 @@ ppc_linux_init_abi (struct gdbarch_info info, > set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand); > set_gdbarch_stap_parse_special_token (gdbarch, > ppc_stap_parse_special_token); > + /* Linux DWARF register mapping is different from the other OSes. */ > + set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + /* Note on Linux the mapping for the DWARF registers and the stab registers > + use the same numbers. Install rs6000_linux_dwarf2_reg_to_regnum for the > + stab register mappings as well. */ > + set_gdbarch_stab_reg_to_regnum (gdbarch, > + rs6000_linux_dwarf2_reg_to_regnum); > + dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum); > > if (tdep->wordsize == 4) > { > -- > 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 14:51 PowerPC, Fix-test-gdb.base-store.exp Carl Love 2023-10-12 14:58 ` [Patch 1/2] " Carl Love @ 2023-10-12 15:00 ` Carl Love 2023-10-13 20:35 ` Keith Seitz 2023-10-16 14:36 ` Andrew Burgess 1 sibling, 2 replies; 27+ messages in thread From: Carl Love @ 2023-10-12 15:00 UTC (permalink / raw) To: gdb-patches, UlrichWeigand; +Cc: cel GDB maintainers: This is the second patch which fixes the 128-bit floating point register mappings. Carl -------------------------------------------------------- Fix test gdb.base/store.exp The test currently fails for IEEE 128-bit floating point types. PowerPC supports the IBM double 128-bit floating point format and IEEE 128-bit format. The IBM double 128-bit floating point format uses two 64-bit floating point registers to store the 128-bit value. The IEEE 128-bit floating point format stores the value in a single 128-bit vector-scalar register (vsr). The various floating point values, 32-bit float, 64-bit double, IBM double 128-bit float and IEEE 128-bit floating point numbers are all mapped to the DWARF fpr numbers. The issue is the IEEE 128-bit floating point values are actually stored in a vsr not the fprs. This patch changes the register mapping for the vsrs from the fpr to the vsr registers so the value is properly accessed by GDB. The functions rs6000_linux_register_to_value, rs6000_linux_value_to_register, rs6000_linux_value_from_register check if the value is an IEEE 128-bit floating point value and adjust the register number as needed. The test in function rs6000_convert_register_p is fixed to so it is only true for floating point values. This patch fixes three regression tests in gdb.base/store.exp. The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE with no regressions. Change to inline function --- gdb/ppc-linux-tdep.c | 4 +++ gdb/rs6000-tdep.c | 66 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 7fb90799dff..ba646a7230f 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "elf-bfd.h" #include "producer.h" +#include "target-float.h" #include "features/rs6000/powerpc-32l.c" #include "features/rs6000/powerpc-altivec32l.c" @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) /* FIXME: jimb/2004-05-05: What should we do when the debug info specifies registers the architecture doesn't have? Our callers don't check the value we return. */ + /* Map dwarf register numbers for floating point, double, IBM double and + IEEE 128-bit floating point to the fpr range. Will have to fix the + mapping for the IEEE 128-bit register numbers later. */ return tdep->ppc_fp0_regnum + (num - 32); else if (77 <= num && num < 77 + 32) return tdep->ppc_vr0_regnum + (num - 77); diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 23397d037ae..ada9cea3353 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs && type->code () == TYPE_CODE_FLT && (type->length () - != builtin_type (gdbarch)->builtin_double->length ())); + == builtin_type (gdbarch)->builtin_float->length ())); +} + +static int +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type, + int regnum) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); + + /* If we have the an IEEE 128-bit floating point value, need to map the + register number to the corresponding VSR. */ + if (tdep->ppc_vsr0_regnum != -1 + && regnum >= tdep->ppc_fp0_regnum + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) + && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad) + && (type->length() == 16)) + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; + + return regnum; } static int @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + if (!get_frame_register_bytes (frame, regnum, 0, gdb::make_array_view (from, register_size (gdbarch, @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + target_float_convert (from, type, to, builtin_type (gdbarch)->builtin_double); put_frame_register (frame, regnum, to); } +static struct value * +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type, + int regnum, struct frame_id frame_id) +{ + int len = type->length (); + struct value *value = value::allocate (type); + frame_info_ptr frame; + + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + + value->set_lval (lval_register); + frame = frame_find_by_id (frame_id); + + if (frame == NULL) + frame_id = null_frame_id; + else + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); + + VALUE_NEXT_FRAME_ID (value) = frame_id; + VALUE_REGNUM (value) = regnum; + + /* Any structure stored in more than one register will always be + an integral number of registers. Otherwise, you need to do + some fiddling with the last register copied here for little + endian machines. */ + if (type_byte_order (type) == BFD_ENDIAN_BIG + && len < register_size (gdbarch, regnum)) + /* Big-endian, and we want less than full size. */ + value->set_offset (register_size (gdbarch, regnum) - len); + else + value->set_offset (0); + + return value; +} + /* The type of a function that moves the value of REG between CACHE or BUF --- in either direction. */ typedef enum register_status (*move_ev_register_func) (struct regcache *, @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p); set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); + set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register); set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum); set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum); -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 15:00 ` [PATCH 2/2] " Carl Love @ 2023-10-13 20:35 ` Keith Seitz 2023-10-13 21:00 ` Carl Love 2023-10-16 14:36 ` Andrew Burgess 1 sibling, 1 reply; 27+ messages in thread From: Keith Seitz @ 2023-10-13 20:35 UTC (permalink / raw) To: cel; +Cc: Ulrich.Weigand, gdb-patches Carl Love wrote: As in your previous patch, I only have some really trivial nits to point out. So treat similarly, and thank you for the patch. > The test currently fails for IEEE 128-bit floating point types. PowerPC > supports the IBM double 128-bit floating point format and IEEE 128-bit Looks like a tab got inserted above? > format. The IBM double 128-bit floating point format uses two 64-bit > floating point registers to store the 128-bit value. The IEEE 128-bit > floating point format stores the value in a single 128-bit vector-scalar > register (vsr). > The various floating point values, 32-bit float, 64-bit double, IBM double > 128-bit float and IEEE 128-bit floating point numbers are all mapped to the > DWARF fpr numbers. The issue is the IEEE 128-bit floating point values are > actually stored in a vsr not the fprs. This patch changes the register > mapping for the vsrs from the fpr to the vsr registers so the value is > properly accessed by GDB. The functions rs6000_linux_register_to_value, > rs6000_linux_value_to_register, rs6000_linux_value_from_register check if > the value is an IEEE 128-bit floating point value and adjust the register > number as needed. The test in function rs6000_convert_register_p is fixed > to so it is only true for floating point values. "fixed [to] so" > This patch fixes three regression tests in gdb.base/store.exp. > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE > with no regressions. Indeed! Tested-by: Keith Seitz <keiths@redhat.com> > Change to inline function Stray line or was there something more to communicate? Keith --- gdb/ppc-linux-tdep.c | 4 +++ gdb/rs6000-tdep.c | 66 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 7fb90799dff..ba646a7230f 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "elf-bfd.h" #include "producer.h" +#include "target-float.h" #include "features/rs6000/powerpc-32l.c" #include "features/rs6000/powerpc-altivec32l.c" @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) /* FIXME: jimb/2004-05-05: What should we do when the debug info specifies registers the architecture doesn't have? Our callers don't check the value we return. */ + /* Map dwarf register numbers for floating point, double, IBM double and + IEEE 128-bit floating point to the fpr range. Will have to fix the + mapping for the IEEE 128-bit register numbers later. */ I suggest rewording this last incomplete sentence to something like: "The mapping for the IEEE 128-bit register numbers will have to be fixed later." return tdep->ppc_fp0_regnum + (num - 32); else if (77 <= num && num < 77 + 32) return tdep->ppc_vr0_regnum + (num - 77); diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 23397d037ae..ada9cea3353 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs && type->code () == TYPE_CODE_FLT && (type->length () - != builtin_type (gdbarch)->builtin_double->length ())); + == builtin_type (gdbarch)->builtin_float->length ())); +} + +static int +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type, + int regnum) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); + + /* If we have the an IEEE 128-bit floating point value, need to map the + register number to the corresponding VSR. */ "If we have the an" --> "If we have an". "need to" --> "" + if (tdep->ppc_vsr0_regnum != -1 + && regnum >= tdep->ppc_fp0_regnum + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) + && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad) + && (type->length() == 16)) + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; + + return regnum; } static int @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + Add "--" before "need". Similarly below in a few spots. if (!get_frame_register_bytes (frame, regnum, 0, gdb::make_array_view (from, register_size (gdbarch, @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + target_float_convert (from, type, to, builtin_type (gdbarch)->builtin_double); put_frame_register (frame, regnum, to); } +static struct value * +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type, + int regnum, struct frame_id frame_id) +{ + int len = type->length (); + struct value *value = value::allocate (type); + frame_info_ptr frame; + + /* We have an IEEE 128-bit float need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + + value->set_lval (lval_register); + frame = frame_find_by_id (frame_id); + + if (frame == NULL) + frame_id = null_frame_id; + else + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); + + VALUE_NEXT_FRAME_ID (value) = frame_id; + VALUE_REGNUM (value) = regnum; + + /* Any structure stored in more than one register will always be + an integral number of registers. Otherwise, you need to do + some fiddling with the last register copied here for little + endian machines. */ + if (type_byte_order (type) == BFD_ENDIAN_BIG + && len < register_size (gdbarch, regnum)) + /* Big-endian, and we want less than full size. */ + value->set_offset (register_size (gdbarch, regnum) - len); + else + value->set_offset (0); + + return value; +} + /* The type of a function that moves the value of REG between CACHE or BUF --- in either direction. */ typedef enum register_status (*move_ev_register_func) (struct regcache *, @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p); set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); + set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register); set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum); set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum); -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-13 20:35 ` Keith Seitz @ 2023-10-13 21:00 ` Carl Love 2023-10-16 11:13 ` Ulrich Weigand 0 siblings, 1 reply; 27+ messages in thread From: Carl Love @ 2023-10-13 21:00 UTC (permalink / raw) To: Keith Seitz, cel; +Cc: Ulrich.Weigand, gdb-patches Keith: Thanks for catching the typos. I have fixed them locally for now as noted below. Carl -------------- On Fri, 2023-10-13 at 13:35 -0700, Keith Seitz wrote: > Carl Love wrote: > > As in your previous patch, I only have some really trivial nits to > point out. > So treat similarly, and thank you for the patch. > > > The test currently fails for IEEE 128-bit floating point > > types. PowerPC > > supports the IBM double 128-bit floating point format and IEEE > > 128-bit > > Looks like a tab got inserted above? Yea, Git gui doesn't show tabs in the window. Fixed > > > format. The IBM double 128-bit floating point format uses two 64- > > bit > > floating point registers to store the 128-bit value. The IEEE 128- > > bit > > floating point format stores the value in a single 128-bit vector- > > scalar > > register (vsr). > > The various floating point values, 32-bit float, 64-bit double, IBM > > double > > 128-bit float and IEEE 128-bit floating point numbers are all > > mapped to the > > DWARF fpr numbers. The issue is the IEEE 128-bit floating point > > values are > > actually stored in a vsr not the fprs. This patch changes the > > register > > mapping for the vsrs from the fpr to the vsr registers so the value > > is > > properly accessed by GDB. The functions > > rs6000_linux_register_to_value, > > rs6000_linux_value_to_register, rs6000_linux_value_from_register > > check if > > the value is an IEEE 128-bit floating point value and adjust the > > register > > number as needed. The test in function rs6000_convert_register_p > > is fixed > > to so it is only true for floating point values. > > "fixed [to] so" > Looks like the "to" didn't get cleanup in the last re-write. Changed "to so it" to "so it". > > This patch fixes three regression tests in gdb.base/store.exp. > > > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power > > 10 LE > > with no regressions. > > Indeed! > > Tested-by: Keith Seitz <keiths@redhat.com> > > > Change to inline function > > Stray line or was there something more to communicate? Stray line, got missed in the last edit. Removed. Thanks. > > Keith > --- > gdb/ppc-linux-tdep.c | 4 +++ > gdb/rs6000-tdep.c | 66 > +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 7fb90799dff..ba646a7230f 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -63,6 +63,7 @@ > #include <ctype.h> > #include "elf-bfd.h" > #include "producer.h" > +#include "target-float.h" > > #include "features/rs6000/powerpc-32l.c" > #include "features/rs6000/powerpc-altivec32l.c" > @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct > gdbarch *gdbarch, int num) > /* FIXME: jimb/2004-05-05: What should we do when the debug info > specifies registers the architecture doesn't have? Our > callers don't check the value we return. */ > + /* Map dwarf register numbers for floating point, double, IBM > double and > + IEEE 128-bit floating point to the fpr range. Will have to > fix the > + mapping for the IEEE 128-bit register numbers later. */ > > I suggest rewording this last incomplete sentence to something like: > "The mapping > for the IEEE 128-bit register numbers will have to be fixed later." > > return tdep->ppc_fp0_regnum + (num - 32); > else if (77 <= num && num < 77 + 32) > return tdep->ppc_vr0_regnum + (num - 77); > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 23397d037ae..ada9cea3353 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch > *gdbarch, int regnum, > && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs > && type->code () == TYPE_CODE_FLT > && (type->length () > - != builtin_type (gdbarch)->builtin_double->length ())); > + == builtin_type (gdbarch)->builtin_float->length ())); > +} > + > +static int > +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type > *type, > + int regnum) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); > + > + /* If we have the an IEEE 128-bit floating point value, need to > map the > + register number to the corresponding VSR. */ > > "If we have the an" --> "If we have an". "need to" --> "" > > + if (tdep->ppc_vsr0_regnum != -1 > + && regnum >= tdep->ppc_fp0_regnum > + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) > + && (gdbarch_long_double_format (gdbarch) == > floatformats_ieee_quad) > + && (type->length() == 16)) > + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; > + > + return regnum; > } > > static int > @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr > frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float need to change regnum mapping > from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > > Add "--" before "need". Similarly below in a few spots. Fixed. > > if (!get_frame_register_bytes (frame, regnum, 0, > gdb::make_array_view (from, > register_size > (gdbarch, > @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr > frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float need to change regnum mapping > from Added "--" before "need". > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > target_float_convert (from, type, > to, builtin_type (gdbarch)->builtin_double); > put_frame_register (frame, regnum, to); > } > > +static struct value * > +rs6000_value_from_register (struct gdbarch *gdbarch, struct type > *type, > + int regnum, struct frame_id frame_id) > +{ > + int len = type->length (); > + struct value *value = value::allocate (type); > + frame_info_ptr frame; > + > + /* We have an IEEE 128-bit float need to > change regnum mapping from Added "--" before "need". > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > + value->set_lval (lval_register); > + frame = frame_find_by_id (frame_id); > + > + if (frame == NULL) > + frame_id = null_frame_id; > + else > + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); > + > + VALUE_NEXT_FRAME_ID (value) = frame_id; > + VALUE_REGNUM (value) = regnum; > + > + /* Any structure stored in more than one register will always be > + an integral number of registers. Otherwise, you need to do > + some fiddling with the last register copied here for little > + endian machines. */ > + if (type_byte_order (type) == BFD_ENDIAN_BIG > + && len < register_size (gdbarch, regnum)) > + /* Big-endian, and we want less than full size. */ > + value->set_offset (register_size (gdbarch, regnum) - len); > + else > + value->set_offset (0); > + > + return value; > +} > + > /* The type of a function that moves the value of REG between CACHE > or BUF --- in either direction. */ > typedef enum register_status (*move_ev_register_func) (struct > regcache *, > @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, > struct gdbarch_list *arches) > set_gdbarch_convert_register_p (gdbarch, > rs6000_convert_register_p); > set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); > set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); > + set_gdbarch_value_from_register (gdbarch, > rs6000_value_from_register); > > set_gdbarch_stab_reg_to_regnum (gdbarch, > rs6000_stab_reg_to_regnum); > set_gdbarch_dwarf2_reg_to_regnum (gdbarch, > rs6000_dwarf2_reg_to_regnum); ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-13 21:00 ` Carl Love @ 2023-10-16 11:13 ` Ulrich Weigand 0 siblings, 0 replies; 27+ messages in thread From: Ulrich Weigand @ 2023-10-16 11:13 UTC (permalink / raw) To: cel, Keith Seitz; +Cc: gdb-patches Carl Love <cel@us.ibm.com> wrote: >Thanks for catching the typos. I have fixed them locally for now as >noted below. This patch (including fixed for the issues pointed out by Keith) is OK. Thanks, Ulrich ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-12 15:00 ` [PATCH 2/2] " Carl Love 2023-10-13 20:35 ` Keith Seitz @ 2023-10-16 14:36 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love 2023-10-20 18:08 ` Carl Love 1 sibling, 2 replies; 27+ messages in thread From: Andrew Burgess @ 2023-10-16 14:36 UTC (permalink / raw) To: Carl Love, gdb-patches, UlrichWeigand; +Cc: cel Carl Love <cel@us.ibm.com> writes: > GDB maintainers: > > This is the second patch which fixes the 128-bit floating point > register mappings. > > Carl > -------------------------------------------------------- > Fix test gdb.base/store.exp > > The test currently fails for IEEE 128-bit floating point types. PowerPC > supports the IBM double 128-bit floating point format and IEEE 128-bit > format. The IBM double 128-bit floating point format uses two 64-bit > floating point registers to store the 128-bit value. The IEEE 128-bit > floating point format stores the value in a single 128-bit vector-scalar > register (vsr). > > The various floating point values, 32-bit float, 64-bit double, IBM double > 128-bit float and IEEE 128-bit floating point numbers are all mapped to the > DWARF fpr numbers. The issue is the IEEE 128-bit floating point values are > actually stored in a vsr not the fprs. This patch changes the register > mapping for the vsrs from the fpr to the vsr registers so the value is > properly accessed by GDB. The functions rs6000_linux_register_to_value, > rs6000_linux_value_to_register, rs6000_linux_value_from_register check if > the value is an IEEE 128-bit floating point value and adjust the register > number as needed. The test in function rs6000_convert_register_p is fixed > to so it is only true for floating point values. > > This patch fixes three regression tests in gdb.base/store.exp. > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE > with no regressions. > > Change to inline function > --- > gdb/ppc-linux-tdep.c | 4 +++ > gdb/rs6000-tdep.c | 66 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index 7fb90799dff..ba646a7230f 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -63,6 +63,7 @@ > #include <ctype.h> > #include "elf-bfd.h" > #include "producer.h" > +#include "target-float.h" > > #include "features/rs6000/powerpc-32l.c" > #include "features/rs6000/powerpc-altivec32l.c" > @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > /* FIXME: jimb/2004-05-05: What should we do when the debug info > specifies registers the architecture doesn't have? Our > callers don't check the value we return. */ > + /* Map dwarf register numbers for floating point, double, IBM double and > + IEEE 128-bit floating point to the fpr range. Will have to fix the > + mapping for the IEEE 128-bit register numbers later. */ > return tdep->ppc_fp0_regnum + (num - 32); > else if (77 <= num && num < 77 + 32) > return tdep->ppc_vr0_regnum + (num - 77); > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 23397d037ae..ada9cea3353 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, > && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs > && type->code () == TYPE_CODE_FLT > && (type->length () > - != builtin_type (gdbarch)->builtin_double->length ())); > + == builtin_type (gdbarch)->builtin_float->length ())); > +} > + > +static int > +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type, > + int regnum) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); > + > + /* If we have the an IEEE 128-bit floating point value, need to map the > + register number to the corresponding VSR. */ > + if (tdep->ppc_vsr0_regnum != -1 > + && regnum >= tdep->ppc_fp0_regnum > + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) > + && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad) > + && (type->length() == 16)) > + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; > + > + return regnum; > } > > static int > @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > if (!get_frame_register_bytes (frame, regnum, 0, > gdb::make_array_view (from, > register_size (gdbarch, > @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > target_float_convert (from, type, > to, builtin_type (gdbarch)->builtin_double); > put_frame_register (frame, regnum, to); > } > > +static struct value * > +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type, > + int regnum, struct frame_id frame_id) > +{ > + int len = type->length (); > + struct value *value = value::allocate (type); > + frame_info_ptr frame; > + > + /* We have an IEEE 128-bit float need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > + value->set_lval (lval_register); > + frame = frame_find_by_id (frame_id); You can move the declaration of frame to here: frame_info_ptr frame = frame_find_by_id (frame_id); which is the preferred GDB style these days. Reviewed-By: Andrew Burgess <aburgess@redhat.com> Thanks, Andrew > + > + if (frame == NULL) > + frame_id = null_frame_id; > + else > + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); > + > + VALUE_NEXT_FRAME_ID (value) = frame_id; > + VALUE_REGNUM (value) = regnum; > + > + /* Any structure stored in more than one register will always be > + an integral number of registers. Otherwise, you need to do > + some fiddling with the last register copied here for little > + endian machines. */ > + if (type_byte_order (type) == BFD_ENDIAN_BIG > + && len < register_size (gdbarch, regnum)) > + /* Big-endian, and we want less than full size. */ > + value->set_offset (register_size (gdbarch, regnum) - len); > + else > + value->set_offset (0); > + > + return value; > +} > + > /* The type of a function that moves the value of REG between CACHE > or BUF --- in either direction. */ > typedef enum register_status (*move_ev_register_func) (struct regcache *, > @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p); > set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); > set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); > + set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register); > > set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum); > set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum); > -- > 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 14:36 ` Andrew Burgess @ 2023-10-16 15:51 ` Carl Love 2023-10-20 18:08 ` Carl Love 1 sibling, 0 replies; 27+ messages in thread From: Carl Love @ 2023-10-16 15:51 UTC (permalink / raw) To: Andrew Burgess, gdb-patches, UlrichWeigand; +Cc: cel Andrew: On Mon, 2023-10-16 at 15:36 +0100, Andrew Burgess wrote: > Carl Love <cel@us.ibm.com> writes: > > > GDB maintainers: > > > > This is the second patch which fixes the 128-bit floating point > > register mappings. > > <snip> > > +static struct value * > > +rs6000_value_from_register (struct gdbarch *gdbarch, struct type > > *type, > > + int regnum, struct frame_id frame_id) > > +{ > > + int len = type->length (); > > + struct value *value = value::allocate (type); > > + frame_info_ptr frame; > > + > > + /* We have an IEEE 128-bit float need to change regnum mapping > > from > > + fpr to vsr. */ > > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > > + > > + value->set_lval (lval_register); > > + frame = frame_find_by_id (frame_id); > > You can move the declaration of frame to here: > > frame_info_ptr frame = frame_find_by_id (frame_id); OK, fixed. > > which is the preferred GDB style these days. > > Reviewed-By: Andrew Burgess <aburgess@redhat.com> > > Thanks, > Andrew > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-16 14:36 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love @ 2023-10-20 18:08 ` Carl Love 2023-10-24 8:53 ` Andrew Burgess 1 sibling, 1 reply; 27+ messages in thread From: Carl Love @ 2023-10-20 18:08 UTC (permalink / raw) To: Andrew Burgess, Carl Love, gdb-patches, UlrichWeigand, keiths GDB maintainers: Ver 2: In the new function, rs6000_value_from_register, moved the declaration of frame to where it gets initialized. Patch was retested on Power10. This is the second patch which fixes the 128-bit floating point register mappings. This fixes the remaining three issues with the store.exp test. Carl ----------------------------------------- rs6000, Fix test gdb.base/store.exp The test currently fails for IEEE 128-bit floating point types. PowerPC supports the IBM double 128-bit floating point format and IEEE 128-bit format. The IBM double 128-bit floating point format uses two 64-bit floating point registers to store the 128-bit value. The IEEE 128-bit floating point format stores the value in a single 128-bit vector-scalar register (vsr). The various floating point values, 32-bit float, 64-bit double, IBM double 128-bit float and IEEE 128-bit floating point numbers are all mapped to the DWARF fpr numbers. The issue is the IEEE 128-bit floating point values are actually stored in a vsr not the fprs. This patch changes the register mapping for the vsrs from the fpr to the vsr registers so the value is properly accessed by GDB. The functions rs6000_linux_register_to_value, rs6000_linux_value_to_register, rs6000_linux_value_from_register check if the value is an IEEE 128-bit floating point value and adjust the register number as needed. The test in function rs6000_convert_register_p is fixed so it is only true for floating point values. This patch fixes three regression tests in gdb.base/store.exp. The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE with no regressions. --- gdb/ppc-linux-tdep.c | 4 +++ gdb/rs6000-tdep.c | 65 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index dc03430e2af..fac56d961cb 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "elf-bfd.h" #include "producer.h" +#include "target-float.h" #include "features/rs6000/powerpc-32l.c" #include "features/rs6000/powerpc-altivec32l.c" @@ -2102,6 +2103,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) /* FIXME: jimb/2004-05-05: What should we do when the debug info specifies registers the architecture doesn't have? Our callers don't check the value we return. */ + /* Map dwarf register numbers for floating point, double, IBM double and + IEEE 128-bit floating point to the fpr range. Will have to fix the + mapping for the IEEE 128-bit register numbers later. */ return tdep->ppc_fp0_regnum + (num - 32); else if (77 <= num && num < 77 + 32) return tdep->ppc_vr0_regnum + (num - 77); diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 23397d037ae..186646673f0 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs && type->code () == TYPE_CODE_FLT && (type->length () - != builtin_type (gdbarch)->builtin_double->length ())); + == builtin_type (gdbarch)->builtin_float->length ())); +} + +static int +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type, + int regnum) +{ + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); + + /* If we have the an IEEE 128-bit floating point value, need to map the + register number to the corresponding VSR. */ + if (tdep->ppc_vsr0_regnum != -1 + && regnum >= tdep->ppc_fp0_regnum + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) + && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad) + && (type->length() == 16)) + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; + + return regnum; } static int @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float -- need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + if (!get_frame_register_bytes (frame, regnum, 0, gdb::make_array_view (from, register_size (gdbarch, @@ -2715,11 +2737,51 @@ rs6000_value_to_register (frame_info_ptr frame, gdb_assert (type->code () == TYPE_CODE_FLT); + /* We have an IEEE 128-bit float -- need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + target_float_convert (from, type, to, builtin_type (gdbarch)->builtin_double); put_frame_register (frame, regnum, to); } +static struct value * +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type, + int regnum, struct frame_id frame_id) +{ + int len = type->length (); + struct value *value = value::allocate (type); + + /* We have an IEEE 128-bit float -- need to change regnum mapping from + fpr to vsr. */ + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); + + value->set_lval (lval_register); + frame_info_ptr frame = frame_find_by_id (frame_id); + + if (frame == NULL) + frame_id = null_frame_id; + else + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); + + VALUE_NEXT_FRAME_ID (value) = frame_id; + VALUE_REGNUM (value) = regnum; + + /* Any structure stored in more than one register will always be + an integral number of registers. Otherwise, you need to do + some fiddling with the last register copied here for little + endian machines. */ + if (type_byte_order (type) == BFD_ENDIAN_BIG + && len < register_size (gdbarch, regnum)) + /* Big-endian, and we want less than full size. */ + value->set_offset (register_size (gdbarch, regnum) - len); + else + value->set_offset (0); + + return value; +} + /* The type of a function that moves the value of REG between CACHE or BUF --- in either direction. */ typedef enum register_status (*move_ev_register_func) (struct regcache *, @@ -8337,6 +8399,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p); set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); + set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register); set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum); set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum); -- 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] PowerPC, Fix-test-gdb.base-store.exp 2023-10-20 18:08 ` Carl Love @ 2023-10-24 8:53 ` Andrew Burgess 0 siblings, 0 replies; 27+ messages in thread From: Andrew Burgess @ 2023-10-24 8:53 UTC (permalink / raw) To: Carl Love, Carl Love, gdb-patches, UlrichWeigand, keiths Carl Love <cel@linux.ibm.com> writes: > GDB maintainers: > > Ver 2: In the new function, rs6000_value_from_register, moved the > declaration of frame to where it gets initialized. Patch was retested > on Power10. > > This is the second patch which fixes the 128-bit floating point > register mappings. This fixes the remaining three issues with the > store.exp test. > > Carl > > ----------------------------------------- > rs6000, Fix test gdb.base/store.exp > > The test currently fails for IEEE 128-bit floating point types. PowerPC > supports the IBM double 128-bit floating point format and IEEE 128-bit > format. The IBM double 128-bit floating point format uses two 64-bit > floating point registers to store the 128-bit value. The IEEE 128-bit > floating point format stores the value in a single 128-bit vector-scalar > register (vsr). > > The various floating point values, 32-bit float, 64-bit double, IBM double > 128-bit float and IEEE 128-bit floating point numbers are all mapped to the > DWARF fpr numbers. The issue is the IEEE 128-bit floating point values are > actually stored in a vsr not the fprs. This patch changes the register > mapping for the vsrs from the fpr to the vsr registers so the value is > properly accessed by GDB. The functions rs6000_linux_register_to_value, > rs6000_linux_value_to_register, rs6000_linux_value_from_register check if > the value is an IEEE 128-bit floating point value and adjust the register > number as needed. The test in function rs6000_convert_register_p is fixed > so it is only true for floating point values. > > This patch fixes three regression tests in gdb.base/store.exp. If this doesn't depend on patch #1 then feel free to go ahead an merge. Approved-By: Andrew Burgess <aburgess@redhat.com> Thanks, Andrew > > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE > with no regressions. > --- > gdb/ppc-linux-tdep.c | 4 +++ > gdb/rs6000-tdep.c | 65 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > index dc03430e2af..fac56d961cb 100644 > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -63,6 +63,7 @@ > #include <ctype.h> > #include "elf-bfd.h" > #include "producer.h" > +#include "target-float.h" > > #include "features/rs6000/powerpc-32l.c" > #include "features/rs6000/powerpc-altivec32l.c" > @@ -2102,6 +2103,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num) > /* FIXME: jimb/2004-05-05: What should we do when the debug info > specifies registers the architecture doesn't have? Our > callers don't check the value we return. */ > + /* Map dwarf register numbers for floating point, double, IBM double and > + IEEE 128-bit floating point to the fpr range. Will have to fix the > + mapping for the IEEE 128-bit register numbers later. */ > return tdep->ppc_fp0_regnum + (num - 32); > else if (77 <= num && num < 77 + 32) > return tdep->ppc_vr0_regnum + (num - 77); > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 23397d037ae..186646673f0 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, > && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs > && type->code () == TYPE_CODE_FLT > && (type->length () > - != builtin_type (gdbarch)->builtin_double->length ())); > + == builtin_type (gdbarch)->builtin_float->length ())); > +} > + > +static int > +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type, > + int regnum) > +{ > + ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch); > + > + /* If we have the an IEEE 128-bit floating point value, need to map the > + register number to the corresponding VSR. */ > + if (tdep->ppc_vsr0_regnum != -1 > + && regnum >= tdep->ppc_fp0_regnum > + && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs) > + && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad) > + && (type->length() == 16)) > + regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum; > + > + return regnum; > } > > static int > @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float -- need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > if (!get_frame_register_bytes (frame, regnum, 0, > gdb::make_array_view (from, > register_size (gdbarch, > @@ -2715,11 +2737,51 @@ rs6000_value_to_register (frame_info_ptr frame, > > gdb_assert (type->code () == TYPE_CODE_FLT); > > + /* We have an IEEE 128-bit float -- need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > target_float_convert (from, type, > to, builtin_type (gdbarch)->builtin_double); > put_frame_register (frame, regnum, to); > } > > +static struct value * > +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type, > + int regnum, struct frame_id frame_id) > +{ > + int len = type->length (); > + struct value *value = value::allocate (type); > + > + /* We have an IEEE 128-bit float -- need to change regnum mapping from > + fpr to vsr. */ > + regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum); > + > + value->set_lval (lval_register); > + frame_info_ptr frame = frame_find_by_id (frame_id); > + > + if (frame == NULL) > + frame_id = null_frame_id; > + else > + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); > + > + VALUE_NEXT_FRAME_ID (value) = frame_id; > + VALUE_REGNUM (value) = regnum; > + > + /* Any structure stored in more than one register will always be > + an integral number of registers. Otherwise, you need to do > + some fiddling with the last register copied here for little > + endian machines. */ > + if (type_byte_order (type) == BFD_ENDIAN_BIG > + && len < register_size (gdbarch, regnum)) > + /* Big-endian, and we want less than full size. */ > + value->set_offset (register_size (gdbarch, regnum) - len); > + else > + value->set_offset (0); > + > + return value; > +} > + > /* The type of a function that moves the value of REG between CACHE > or BUF --- in either direction. */ > typedef enum register_status (*move_ev_register_func) (struct regcache *, > @@ -8337,6 +8399,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p); > set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value); > set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register); > + set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register); > > set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum); > set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum); > -- > 2.37.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-11-08 10:54 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-12 14:51 PowerPC, Fix-test-gdb.base-store.exp Carl Love 2023-10-12 14:58 ` [Patch 1/2] " Carl Love 2023-10-13 20:34 ` Keith Seitz 2023-10-13 21:00 ` Carl Love 2023-10-16 11:12 ` Ulrich Weigand 2023-10-16 14:31 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love 2023-10-19 15:54 ` Carl Love 2023-10-24 8:50 ` Andrew Burgess 2023-10-24 16:05 ` Carl Love 2023-10-20 18:08 ` [PATCH 1/2, ver2] " Carl Love 2023-10-24 9:30 ` Andrew Burgess 2023-10-25 13:24 ` Ulrich Weigand 2023-10-30 9:45 ` Andrew Burgess 2023-10-30 16:44 ` Ulrich Weigand 2023-10-30 17:16 ` Carl Love 2023-10-30 17:25 ` [PATCH 1/2, ver3] " Carl Love 2023-11-06 18:24 ` Carl Love 2023-11-08 10:54 ` Andrew Burgess 2023-10-12 15:00 ` [PATCH 2/2] " Carl Love 2023-10-13 20:35 ` Keith Seitz 2023-10-13 21:00 ` Carl Love 2023-10-16 11:13 ` Ulrich Weigand 2023-10-16 14:36 ` Andrew Burgess 2023-10-16 15:51 ` Carl Love 2023-10-20 18:08 ` Carl Love 2023-10-24 8:53 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).