* Fix for prologue processing on PowerPC @ 2017-09-22 12:11 Nikola Prica 2017-10-05 2:01 ` Kevin Buettner 2017-10-26 10:02 ` [PING]Fix " Nikola Prica 0 siblings, 2 replies; 19+ messages in thread From: Nikola Prica @ 2017-09-22 12:11 UTC (permalink / raw) To: gdb-patches Cc: Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic [-- Attachment #1: Type: text/plain, Size: 1538 bytes --] After analyzing dump of ppc program, whose crash occurred after watchdog_force_here () function, GDB couldn't print full back trace because GDB couldn't unwind PC from the watchdog fucntion. The problem is introduced with the following patch: https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html In function skip_prologue(), shifted lr_reg makes below condition always false because non-shifted lr_reg value is expected to be checked. else if (lr_reg >= 0 && /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || /* stw Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x90010000)) || /* stwu Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x94010000)))) Before this fix unwinding was able to work because it relied on unwind directives or on some of the next frames to find PC. Problem came with watchdog_force_here() function which didn't contain unwind directives. I wasn't able to produce test case that would show improvements for end user. I suppose that changes would be visible if watchdog event was called, but I don't have valid ppc board to try this. I have tried this code on simple test case with few functions in back trace. The back trace is printed correctly with and without this fix, but the difference between those two runs is that the body of the upper condition was visited with this patch. After visiting the body there was no need to look for PC counter in next frames nor to use unwind directives. [-- Attachment #2: PowerPC-fix-for-prologue-processing.patch --] [-- Type: text/x-patch, Size: 1733 bytes --] From 93a406efd79552eba7fc55ed3b7f293cdcf324ef Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Tue, 19 Sep 2017 17:52:35 +0200 Subject: [PATCH] PowerPC, fix for prologue processing One of conditions in skip_prologue() is never visited because it expects non shifted `lr_reg`. That condition is supposed to set PC offset. When body of this condition is visited PC offset is set and there will be no need to look for it in next frames nor to use frame unwind directives. gdb/ChangeLog: *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. --- gdb/rs6000-tdep.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 95b2ca7..7f64901 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1652,11 +1652,14 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ - if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; - if (lr_reg == 0) - r0_contains_arg = 0; - continue; + if (lr_reg == -1) + { + lr_reg = (op & 0x03e00000); + fdata->lr_register = lr_reg >> 21; + } + if (lr_reg == 0) + r0_contains_arg = 0; + continue; } else if ((op & 0xfc1fffff) == 0x7c000026) { /* mfcr Rx */ @@ -2178,9 +2181,6 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, } #endif /* 0 */ - if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; - fdata->offset = -fdata->offset; return last_prologue_pc; } -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Fix for prologue processing on PowerPC 2017-09-22 12:11 Fix for prologue processing on PowerPC Nikola Prica @ 2017-10-05 2:01 ` Kevin Buettner 2017-10-26 10:02 ` [PING]Fix " Nikola Prica 1 sibling, 0 replies; 19+ messages in thread From: Kevin Buettner @ 2017-10-05 2:01 UTC (permalink / raw) To: gdb-patches On Fri, 22 Sep 2017 14:11:51 +0200 Nikola Prica <nikola.prica@rt-rk.com> wrote: > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg > and assign shifted lr_reg to fdata->lr_register when lr_reg is > set. > --- > gdb/rs6000-tdep.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 95b2ca7..7f64901 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1652,11 +1652,14 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > > remember just the first one, but skip over additional > ones. */ > - if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > - if (lr_reg == 0) > - r0_contains_arg = 0; > - continue; > + if (lr_reg == -1) > + { > + lr_reg = (op & 0x03e00000); > + fdata->lr_register = lr_reg >> 21; > + } > + if (lr_reg == 0) > + r0_contains_arg = 0; > + continue; > } > else if ((op & 0xfc1fffff) == 0x7c000026) > { /* mfcr Rx */ > @@ -2178,9 +2181,6 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > } > #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > - > fdata->offset = -fdata->offset; > return last_prologue_pc; > } > -- > 2.7.4 > Hi Nikola, The current version of the code causes fdata->lr_register to be set only when lr_reg has been previously set ( >= 0) and when pc == lim_pc. I.e. it won't be set when the loop scanning the prologue is exited early. Just looking over that loop, there are a number of break statements which might cause an early loop exit. My concern about your patch is that by setting fdata->lr_register within the loop, fdata->lr_register could end up having a value which we don't want it to have for one of those early loop exits when pc != lim_pc. That said, I have not done an especially deep analysis of this matter. If you think that fdata->offset should be set for all of those other conditions too, then... Well, I'm willing to consider this proposition. But I'd like to see some analysis of why this is so. Otherwise, it seems to me that the patch below (which I have not tested) might fix the bug that you found, but not perturb the logic regarding pc != lim_pc ? If it looks okay to you, please try it out on your test case (and regression test it using the testsuite too). If it's okay, then go ahead and commit it with a suitably adjusted ChangeLog entry. Thanks, Kevin diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 95b2ca7..86c0915 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1653,7 +1653,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; + lr_reg = op & 0x03e00000; if (lr_reg == 0) r0_contains_arg = 0; continue; @@ -2179,7 +2179,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, #endif /* 0 */ if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + fdata->lr_register = lr_reg >> 21; fdata->offset = -fdata->offset; return last_prologue_pc; ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PING]Fix for prologue processing on PowerPC 2017-09-22 12:11 Fix for prologue processing on PowerPC Nikola Prica 2017-10-05 2:01 ` Kevin Buettner @ 2017-10-26 10:02 ` Nikola Prica 2017-11-08 16:58 ` Kevin Buettner 1 sibling, 1 reply; 19+ messages in thread From: Nikola Prica @ 2017-10-26 10:02 UTC (permalink / raw) To: gdb-patches Cc: Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic Can someone please take a look into this? Thanks, Nikola Prica On 22.09.2017. 14:11, Nikola Prica wrote: > After analyzing dump of ppc program, whose crash occurred after > watchdog_force_here () function, GDB couldn't print full back trace > because GDB couldn't unwind PC from the watchdog fucntion. > > The problem is introduced with the following patch: > > https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html > > In function skip_prologue(), shifted lr_reg makes below condition always > false because non-shifted lr_reg value is expected to be checked. > > else if (lr_reg >= 0 && > /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ > (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || > /* stw Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x90010000)) || > /* stwu Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x94010000)))) > > Before this fix unwinding was able to work because it relied on unwind > directives or on some of the next frames to find PC. Problem came with > watchdog_force_here() function which didn't contain unwind directives. > > I wasn't able to produce test case that would show improvements for end > user. I suppose that changes would be visible if watchdog event was > called, but I don't have valid ppc board to try this. I have tried this > code on simple test case with few functions in back trace. The back > trace is printed correctly with and without this fix, but the difference > between those two runs is that the body of the upper condition was > visited with this patch. After visiting the body there was no need to > look for PC counter in next frames nor to use unwind directives. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING]Fix for prologue processing on PowerPC 2017-10-26 10:02 ` [PING]Fix " Nikola Prica @ 2017-11-08 16:58 ` Kevin Buettner 2017-11-09 18:15 ` [PING][PATCH] Fix " Nikola Prica 0 siblings, 1 reply; 19+ messages in thread From: Kevin Buettner @ 2017-11-08 16:58 UTC (permalink / raw) To: Nikola Prica Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic See: https://sourceware.org/ml/gdb-patches/2017-10/msg00098.html Kevin On Thu, 26 Oct 2017 12:02:19 +0200 Nikola Prica <nikola.prica@rt-rk.com> wrote: > Can someone please take a look into this? > > Thanks, > > Nikola Prica > > On 22.09.2017. 14:11, Nikola Prica wrote: > > After analyzing dump of ppc program, whose crash occurred after > > watchdog_force_here () function, GDB couldn't print full back trace > > because GDB couldn't unwind PC from the watchdog fucntion. > > > > The problem is introduced with the following patch: > > > > https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html > > > > In function skip_prologue(), shifted lr_reg makes below condition always > > false because non-shifted lr_reg value is expected to be checked. > > > > else if (lr_reg >= 0 && > > /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ > > (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || > > /* stw Rx, NUM(r1) */ > > ((op & 0xffff0000) == (lr_reg | 0x90010000)) || > > /* stwu Rx, NUM(r1) */ > > ((op & 0xffff0000) == (lr_reg | 0x94010000)))) > > > > Before this fix unwinding was able to work because it relied on unwind > > directives or on some of the next frames to find PC. Problem came with > > watchdog_force_here() function which didn't contain unwind directives. > > > > I wasn't able to produce test case that would show improvements for end > > user. I suppose that changes would be visible if watchdog event was > > called, but I don't have valid ppc board to try this. I have tried this > > code on simple test case with few functions in back trace. The back > > trace is printed correctly with and without this fix, but the difference > > between those two runs is that the body of the upper condition was > > visited with this patch. After visiting the body there was no need to > > look for PC counter in next frames nor to use unwind directives. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-11-08 16:58 ` Kevin Buettner @ 2017-11-09 18:15 ` Nikola Prica 2017-12-01 19:37 ` pedromfc 0 siblings, 1 reply; 19+ messages in thread From: Nikola Prica @ 2017-11-09 18:15 UTC (permalink / raw) To: Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic Hi Kevin, lr_reg could be also set to -2 in part of code which is reachable after shifting removal. /* Invalidate lr_reg, but don't set it to -1. That would mean that it had never been set. */ lr_reg = -2; This part of the code which depends of non shifted lr_reg, and the part where shifting is removed are only two places where lr_reg is changed. As so, I've added last condition to set fdata->lr_register on -1 if lim_pc is not reached. If it seems fine now could you pleas commit it because I don't have rights to do it. Thanks, Nikola Prica From: Prica <nprica@rt-rk.com> Date: Thu, 9 Nov 2017 13:10:48 +0100 Subject: Fix for prologue processing on PowerPC One of conditions in skip_prologue() is never visited because it expects non shifted `lr_reg`. That condition is supposed to set PC offset. When body of this condition is visited PC offset is set and there will be no need to look for it in next frames nor to use frame unwind directives. gdb/ChangeLog: *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. If iteration do not hit lim_pc lr_register is set as -1. --- gdb/rs6000-tdep.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 6c44995..6f05ef5 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; - if (lr_reg == 0) - r0_contains_arg = 0; + { + lr_reg = (op & 0x03e00000); + fdata->lr_register = lr_reg >> 21; + if (lr_reg == 0) + r0_contains_arg = 0; + } continue; } else if ((op & 0xfc1fffff) == 0x7c000026) @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, } #endif /* 0 */ - if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + if (pc != lim_pc) + fdata->lr_register = -1; fdata->offset = -fdata->offset; return last_prologue_pc; -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-11-09 18:15 ` [PING][PATCH] Fix " Nikola Prica @ 2017-12-01 19:37 ` pedromfc 2017-12-19 15:57 ` Nikola Prica 0 siblings, 1 reply; 19+ messages in thread From: pedromfc @ 2017-12-01 19:37 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand [-- Attachment #1: Type: text/plain, Size: 5120 bytes --] Hello Nikola, Kevin, Thank you for providing these patches. I tested both patches (Nicola's second patch and Kevin's patch) on ppc64le, and there were no regressions, except for some additional expected failures in gdb.threads/attach-many-short-lived-threads.exp. Some comments: - Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within the first if. This is useful for the case when a second mflr Rx with Rx != 0 is detected. Previously this would cause r0_contains_arg to be reset, despite Rx not being 0. However, if the second mflr has Rx == 0, it would make sense to reset r0_contains_arg (and this would work accidentally before the patch, assuming the first mflr also had Rx == 0). Maybe the best solution here is to always check the Rx contained in the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, or leave it as before since it's a separate issue. - Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, and op is an unsigned long. Will the unshifted reg always fit in a int? - I think it's more clear to only set lr_register when needed (pc reaches the limit), as opposed to resetting it to -1 if pc didn't reach the limit. - I wasn't able to directly apply Nikola's patch, I did so manually. I've also attached a reproducer (prologue.c/foo.S) to check if the patches fixed the issue. I had to alter a generated assembly file so that mflr would use a register other than R0 (in which case the old code does work). gcc -g0 -O0 -o prologue prologue.c foo.S (gdb) file prologue Reading symbols from prologue...done. (gdb) break bar Breakpoint 1 at 0x100005b8 (gdb) run Starting program: /home/pedromfc/prologue Before the patch: Breakpoint 1, 0x00000000100005b8 in bar () (gdb) bt #0 0x00000000100005b8 in bar () #1 0x0000000010000644 in foo () Backtrace stopped: frame did not save the PC After (both patches): Breakpoint 1, 0x00000000100005b8 in bar () (gdb) bt #0 0x00000000100005b8 in bar () #1 0x0000000010000644 in foo () #2 0x00000000100005f8 in main () (gdb) info f 1 Stack frame at 0x7fffffffeee0:  pc = 0x10000644 in foo; saved pc = 0x100005f8 Thanks! Pedro On 11/09/2017 04:15 PM, Nikola Prica wrote: > Hi Kevin, > > lr_reg could be also set to -2 in part of code which is reachable > after shifting removal. > >      /* Invalidate lr_reg, but don't set it to -1. >         That would mean that it had never been set. */ >      lr_reg = -2; > > This part of the code which depends of non shifted lr_reg, and the > part where shifting is removed are only two places where lr_reg is > changed. As so, I've added last condition to set fdata->lr_register on > -1 if lim_pc is not reached. > > If it seems fine now could you pleas commit it because I don't have > rights to do it. > > Thanks, > > Nikola Prica > > > From: Prica <nprica@rt-rk.com> > Date: Thu, 9 Nov 2017 13:10:48 +0100 > Subject: Fix for prologue processing on PowerPC > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > >     *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg >      and assign shifted lr_reg to fdata->lr_register when lr_reg is >      set. If iteration do not hit lim_pc lr_register is set as -1. > --- >  gdb/rs6000-tdep.c | 13 ++++++++----- >  1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 6c44995..6f05ef5 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, > CORE_ADDR pc, CORE_ADDR lim_pc, >          remember just the first one, but skip over additional >          ones. */ >       if (lr_reg == -1) > -       lr_reg = (op & 0x03e00000) >> 21; > -         if (lr_reg == 0) > -           r0_contains_arg = 0; > +     { > +       lr_reg = (op & 0x03e00000); > +       fdata->lr_register = lr_reg >> 21; > +       if (lr_reg == 0) > +         r0_contains_arg = 0; > +     } >       continue; >     } >       else if ((op & 0xfc1fffff) == 0x7c000026) > @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, > CORE_ADDR pc, CORE_ADDR lim_pc, >     } >  #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > -   fdata->lr_register = lr_reg; > + if (pc != lim_pc) > +   fdata->lr_register = -1; > >   fdata->offset = -fdata->offset; >   return last_prologue_pc; [-- Attachment #2: prologue.c --] [-- Type: text/x-csrc, Size: 74 bytes --] int bar() { return 0; } int foo(); int main(void) { return foo(); } [-- Attachment #3: foo.S --] [-- Type: text/plain, Size: 486 bytes --] .file "foo.c" .abiversion 2 .section ".text" .align 2 .globl foo .type foo, @function foo: .LCF0: 0: addis 2,12,.TOC.-.LCF0@ha addi 2,2,.TOC.-.LCF0@l .localentry foo,.-foo mflr 3 std 3,16(1) std 31,-8(1) stdu 1,-112(1) mr 31,1 bl bar nop mr 9,3 mr 3,9 addi 1,31,112 ld 0,16(1) mtlr 0 ld 31,-8(1) blr .long 0 .byte 0,0,0,1,128,1,0,1 .size foo,.-foo .ident "GCC: (SUSE Linux) 7.2.1 20170901 [gcc-7-branch revision 251580]" .section .note.GNU-stack,"",@progbits ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-12-01 19:37 ` pedromfc @ 2017-12-19 15:57 ` Nikola Prica 2017-12-29 18:05 ` Pedro Franco de Carvalho 2018-01-02 10:29 ` Yao Qi 0 siblings, 2 replies; 19+ messages in thread From: Nikola Prica @ 2017-12-19 15:57 UTC (permalink / raw) To: pedromfc, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Hello Pedro and Kevi, I've created and tested example based on your example. Thank you for that. > - I think it's more clear to only set lr_register when needed (pc > reaches the limit), as opposed to resetting it to -1 if pc didn't reach > the limit. The body of condition that becomes visitable after this patch invalidates lr_reg by setting it to -2 before reaching the limit. else if (lr_reg >= 0 && /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || /* stw Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x90010000)) || /* stwu Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x94010000)))) { /* where Rx == lr */ fdata->lr_offset = offset; fdata->nosavedpc = 0; /* Invalidate lr_reg, but don't set it to -1. That would mean that it had never been set. */ lr_reg = -2; ... Thanks, Nikola From 9aaddf9670d9f4cb7f088499febd1fa9c6a7076c Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Tue, 19 Dec 2017 14:29:09 +0100 Subject: [PATCH] Fix for prologue processing on PowerPc One of conditions in skip_prologue() is never visited because it expects non shifted `lr_reg`. That condtition is supposed to set PC offset. When body of this condition is visited PC offset is set and there will be no need to look for it in next frames nor to use frame unwind directives. gdb/ChangeLog: *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. If iteration do not hit lim_pc lr_register is set as -1. *testsuite/gdb.arch/ppc-prologue-frame.s: New file. *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise. *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise. --- gdb/rs6000-tdep.c | 14 ++++--- gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 +++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 48 +++++++++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.s | 40 +++++++++++++++++++ 4 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.s diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 456dbcc..f0d2781 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; - if (lr_reg == 0) - r0_contains_arg = 0; + { + lr_reg = (op & 0x03e00000); + fdata->lr_register = lr_reg >> 21; + } + if (lr_reg == 0) + r0_contains_arg = 0; + continue; } else if ((op & 0xfc1fffff) == 0x7c000026) @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, } #endif /* 0 */ - if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + if (pc != lim_pc) + fdata->lr_register = -1; fdata->offset = -fdata->offset; return last_prologue_pc; diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c new file mode 100644 index 0000000..f59210a --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c @@ -0,0 +1,28 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int bar() +{ + return 0; +} + +int foo(); + +int main(void) +{ + return foo(); +} diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp new file mode 100644 index 0000000..e90a8c1 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp @@ -0,0 +1,48 @@ +# Copyright 2017 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/> + +if {![istarget "powerpc*"] } { + verbose "Skipping powerpc back trace test." + return +} + + +set main_testfile ppc-prolog-frame + +if {![istarget "powerpc*-*-*"]} then { + verbose "Skipping PowerPC instructions disassembly." + return -1 +} + + +if {[gdb_compile \ + [list ${srcdir}/${subdir}/$main_testfile.c ${srcdir}/${subdir}/$main_testfile.S] \ + [standard_output_file ${main_testfile}] \ + executable {debug}] != ""} { + untested "failed to build $main_testfile" + return -1 +} + + +clean_restart ${main_testfile} + +if ![runto bar] { + untested "could not run to bar" + return -1 +} + +gdb_test "bt" \ + "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \ + "Backtrace to the main frame" diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s new file mode 100644 index 0000000..16cd7e2 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s @@ -0,0 +1,40 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + + .file "foo.c" + .section ".text" + .align 2 + .globl foo + .type foo, @function +foo: + stwu 1,-32(1) + mflr 3 + stw 3,36(1) + stw 31,28(1) + mr 31,1 + bl bar + mr 9,3 + mr 3,9 + addi 11,31,32 + lwz 0,4(11) + mtlr 0 + lwz 31,-4(11) + mr 1,11 + blr + .size foo,.-foo + .ident "GCC: (Ubuntu 4.8.2-19ubuntu1) 4.8.2" + .section .note.GNU-stack,"",@progbits -- On 01.12.2017. 20:36, pedromfc wrote: > Hello Nikola, Kevin, > > Thank you for providing these patches. > > I tested both patches (Nicola's second patch and Kevin's patch) on > ppc64le, and there were no regressions, except for some additional > expected failures in gdb.threads/attach-many-short-lived-threads.exp. > > Some comments: > > - Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within > the first if. This is useful for the case when a second mflr Rx with Rx > != 0 is detected. Previously this would cause r0_contains_arg to be > reset, despite Rx not being 0. However, if the second mflr has Rx == 0, > it would make sense to reset r0_contains_arg (and this would work > accidentally before the patch, assuming the first mflr also had Rx == > 0). Maybe the best solution here is to always check the Rx contained in > the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, > or leave it as before since it's a separate issue. > > - Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, > and op is an unsigned long. Will the unshifted reg always fit in a int? > > - I think it's more clear to only set lr_register when needed (pc > reaches the limit), as opposed to resetting it to -1 if pc didn't reach > the limit. > > - I wasn't able to directly apply Nikola's patch, I did so manually. > > I've also attached a reproducer (prologue.c/foo.S) to check if the > patches fixed the issue. I had to alter a generated assembly file so > that mflr would use a register other than R0 (in which case the old code > does work). > > gcc -g0 -O0 -o prologue prologue.c foo.S > > (gdb) file prologue > Reading symbols from prologue...done. > (gdb) break bar > Breakpoint 1 at 0x100005b8 > (gdb) run > > Starting program: /home/pedromfc/prologue > > Before the patch: > > Breakpoint 1, 0x00000000100005b8 in bar () > (gdb) bt > #0 0x00000000100005b8 in bar () > #1 0x0000000010000644 in foo () > Backtrace stopped: frame did not save the PC > > After (both patches): > > Breakpoint 1, 0x00000000100005b8 in bar () > (gdb) bt > #0 0x00000000100005b8 in bar () > #1 0x0000000010000644 in foo () > #2 0x00000000100005f8 in main () > > (gdb) info f 1 > Stack frame at 0x7fffffffeee0: > pc = 0x10000644 in foo; saved pc = 0x100005f8 > > Thanks! > Pedro > > On 11/09/2017 04:15 PM, Nikola Prica wrote: >> Hi Kevin, >> >> lr_reg could be also set to -2 in part of code which is reachable >> after shifting removal. >> >> /* Invalidate lr_reg, but don't set it to -1. >> That would mean that it had never been set. */ >> lr_reg = -2; >> >> This part of the code which depends of non shifted lr_reg, and the >> part where shifting is removed are only two places where lr_reg is >> changed. As so, I've added last condition to set fdata->lr_register on >> -1 if lim_pc is not reached. >> >> If it seems fine now could you pleas commit it because I don't have >> rights to do it. >> >> Thanks, >> >> Nikola Prica >> >> >> From: Prica <nprica@rt-rk.com> >> Date: Thu, 9 Nov 2017 13:10:48 +0100 >> Subject: Fix for prologue processing on PowerPC >> >> One of conditions in skip_prologue() is never visited because it >> expects non shifted `lr_reg`. That condition is supposed to set PC >> offset. When body of this condition is visited PC offset is set and >> there will be no need to look for it in next frames nor to use frame >> unwind directives. >> >> gdb/ChangeLog: >> >> *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg >> and assign shifted lr_reg to fdata->lr_register when lr_reg is >> set. If iteration do not hit lim_pc lr_register is set as -1. >> --- >> gdb/rs6000-tdep.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c >> index 6c44995..6f05ef5 100644 >> --- a/gdb/rs6000-tdep.c >> +++ b/gdb/rs6000-tdep.c >> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, >> CORE_ADDR pc, CORE_ADDR lim_pc, >> remember just the first one, but skip over additional >> ones. */ >> if (lr_reg == -1) >> - lr_reg = (op & 0x03e00000) >> 21; >> - if (lr_reg == 0) >> - r0_contains_arg = 0; >> + { >> + lr_reg = (op & 0x03e00000); >> + fdata->lr_register = lr_reg >> 21; >> + if (lr_reg == 0) >> + r0_contains_arg = 0; >> + } >> continue; >> } >> else if ((op & 0xfc1fffff) == 0x7c000026) >> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, >> CORE_ADDR pc, CORE_ADDR lim_pc, >> } >> #endif /* 0 */ >> >> - if (pc == lim_pc && lr_reg >= 0) >> - fdata->lr_register = lr_reg; >> + if (pc != lim_pc) >> + fdata->lr_register = -1; >> >> fdata->offset = -fdata->offset; >> return last_prologue_pc; > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-12-19 15:57 ` Nikola Prica @ 2017-12-29 18:05 ` Pedro Franco de Carvalho 2018-01-09 11:22 ` Nikola Prica 2018-01-02 10:29 ` Yao Qi 1 sibling, 1 reply; 19+ messages in thread From: Pedro Franco de Carvalho @ 2017-12-29 18:05 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Hello Nikola, > > - I think it's more clear to only set lr_register when needed (pc > > reaches the limit), as opposed to resetting it to -1 if pc didn't reach > > the limit. > > The body of condition that becomes visitable after this patch > invalidates lr_reg by setting it to -2 before reaching the limit. Sorry, I was referring to to fdata->lr_register. What I meant is that it seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end of the function (as Kevin had suggested), as opposed to setting it to -1 at the end of the function, in this part: > + if (pc != lim_pc) > + fdata->lr_register = -1; In any case, for this part to be correct, shouldn't the condition be the full complement of the original condition? That is, if (pc != lim_pc || lr_reg < 0) Which is the complement of: if (pc == lim_pc && lr_reg >= 0) Instead of: > + if (pc != lim_pc) Previously if there was a mflr/stw sequence that invalidated lr_reg, fdata->lr_register would be -1 when the function exited, even if pc == lim_pc (because of the second term of the condition). Still, I think it's safer to keep fdata->lr_register == -1 and set it at the end if needed. I wasn't able to apply your patch directly, I think there are some extra whitespaces around there. And thank you for providing a testcase. There's just a few comments: > +if {![istarget "powerpc*"] } { > + verbose "Skipping powerpc back trace test." > + return > +} > + > + > +set main_testfile ppc-prolog-frame > + > +if {![istarget "powerpc*-*-*"]} then { > + verbose "Skipping PowerPC instructions disassembly." > + return -1 > +} There is an extra target check here. Also, because the assembly file uses the 32-bit abi I think the target selection should be: "istarget "powerpc-*-*"", this way it excludes 64-bit targets. The main_testfile name doesn't matche the test file name. In any case, you can use the standard_testfile macro like this: standard_testfile .c .S It will set $srcfile and $srcfile2 to powerpc-prologue-frame.c/.S, respectively. > ${srcdir}/${subdir}/$main_testfile.S] \ > + [standard_output_file ${main_testfile}] \ > + executable {debug}] != ""} { The option flag should be empty ({}), because we want to test the case without debug information. I tried the test and the regular expression didn't seem to match. This is probably because of the "at" parts: > +gdb_test "bt" \ > + "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) > at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \ Does your gdb output "at" instead of in? I tried a recent version and it outputs "in". I adapted one from the powerpc-prologue.exp test test that seemed to work: gdb_test "bt" \ "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ "Backtrace to the main frame" It might be good to also remove unecessary info from the assembly file for inclusion in the testcase, e.g. the .ident directive. I'm not sure of what are the minimum necessary directives according to the abi, but looking at another test, powerpc-stackless.exp, the corresponding assembly source reads: #include <ppc-asm.h> FUNC_START(main) li sp,0 mtlr sp blr FUNC_END(main) These macros could be useful here (in this case the extension should be .S so that gcc will preprocess the file). See below for a diff that incorporate these comments. Feel free to include the changes in your patch. Thanks, Pedro diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S new file mode 100644 index 0000000000..8697213934 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S @@ -0,0 +1,35 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <ppc-asm.h> + +FUNC_START(foo) + stwu 1,-32(1) + mflr 3 + stw 3,36(1) + stw 31,28(1) + mr 31,1 + bl bar + mr 9,3 + mr 3,9 + addi 11,31,32 + lwz 0,4(11) + mtlr 0 + lwz 31,-4(11) + mr 1,11 + blr +FUNC_END(foo) diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c new file mode 100644 index 0000000000..7a743aff8e --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c @@ -0,0 +1,31 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +bar (void) +{ + return 0; +} + +int +foo (void); + +int +main (void) +{ + return foo (); +} diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp new file mode 100644 index 0000000000..f124717169 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp @@ -0,0 +1,43 @@ +# Copyright 2017 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/> + +# Test the PowerPC prologue analyzer when a register rx other than r0 is used +# to save the link register in the mflr rx/stw rx sequence. + +if {![istarget "powerpc*-*-*"]} then { + verbose "Skipping PowerPC prologue tests." + return +} + +standard_testfile .c .S + +set binfile [standard_output_file ${testfile}] + +if {[gdb_compile [list "${srcdir}/${subdir}/${srcfile}" "${srcdir}/${subdir}/${srcfile2}"] \ + "${binfile}" executable {}] != ""} { + untested "Failed to build ${binfile}" + return -1 +} + +clean_restart ${binfile} + +if ![runto bar] { + untested "could not run to bar" + return -1 +} + +gdb_test "bt" \ + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ + "Backtrace to the main frame" Nikola Prica <nikola.prica@rt-rk.com> writes: > Hello Pedro and Kevi, > > I've created and tested example based on your example. Thank you for that. > > > - I think it's more clear to only set lr_register when needed (pc > > reaches the limit), as opposed to resetting it to -1 if pc didn't reach > > the limit. > > The body of condition that becomes visitable after this patch > invalidates lr_reg by setting it to -2 before reaching the limit. > > else if (lr_reg >= 0 && > /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ > (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || > /* stw Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x90010000)) || > /* stwu Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x94010000)))) > { /* where Rx == lr */ > fdata->lr_offset = offset; > fdata->nosavedpc = 0; > /* Invalidate lr_reg, but don't set it to -1. > That would mean that it had never been set. */ > lr_reg = -2; > ... > > Thanks, > > Nikola > > From 9aaddf9670d9f4cb7f088499febd1fa9c6a7076c Mon Sep 17 00:00:00 2001 > From: Prica <nprica@rt-rk.com> > Date: Tue, 19 Dec 2017 14:29:09 +0100 > Subject: [PATCH] Fix for prologue processing on PowerPc > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condtition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign > shifted lr_reg to fdata->lr_register when lr_reg is set. If > iteration do not hit lim_pc lr_register is set as -1. > *testsuite/gdb.arch/ppc-prologue-frame.s: New file. > *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise. > *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise. > --- > gdb/rs6000-tdep.c | 14 ++++--- > gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 +++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 48 > +++++++++++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.s | 40 +++++++++++++++++++ > 4 files changed, 125 insertions(+), 5 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.s > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 456dbcc..f0d2781 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR > pc, CORE_ADDR lim_pc, > remember just the first one, but skip over additional > ones. */ > if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > - if (lr_reg == 0) > - r0_contains_arg = 0; > + { > + lr_reg = (op & 0x03e00000); > + fdata->lr_register = lr_reg >> 21; > + } > + if (lr_reg == 0) > + r0_contains_arg = 0; > + > continue; > } > else if ((op & 0xfc1fffff) == 0x7c000026) > @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR > pc, CORE_ADDR lim_pc, > } > #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > + if (pc != lim_pc) > + fdata->lr_register = -1; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > new file mode 100644 > index 0000000..f59210a > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > @@ -0,0 +1,28 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2017 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > <http://www.gnu.org/licenses/>. */ > + > +int bar() > +{ > + return 0; > +} > + > +int foo(); > + > +int main(void) > +{ > + return foo(); > +} > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > new file mode 100644 > index 0000000..e90a8c1 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > @@ -0,0 +1,48 @@ > +# Copyright 2017 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/> > + > +if {![istarget "powerpc*"] } { > + verbose "Skipping powerpc back trace test." > + return > +} > + > + > +set main_testfile ppc-prolog-frame > + > +if {![istarget "powerpc*-*-*"]} then { > + verbose "Skipping PowerPC instructions disassembly." > + return -1 > +} > + > + > +if {[gdb_compile \ > + [list ${srcdir}/${subdir}/$main_testfile.c > ${srcdir}/${subdir}/$main_testfile.S] \ > + [standard_output_file ${main_testfile}] \ > + executable {debug}] != ""} { > + untested "failed to build $main_testfile" > + return -1 > +} > + > + > +clean_restart ${main_testfile} > + > +if ![runto bar] { > + untested "could not run to bar" > + return -1 > +} > + > +gdb_test "bt" \ > + "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) > at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \ > + "Backtrace to the main frame" > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s > b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s > new file mode 100644 > index 0000000..16cd7e2 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s > @@ -0,0 +1,40 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2017 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > <http://www.gnu.org/licenses/>. */ > + > + .file "foo.c" > + .section ".text" > + .align 2 > + .globl foo > + .type foo, @function > +foo: > + stwu 1,-32(1) > + mflr 3 > + stw 3,36(1) > + stw 31,28(1) > + mr 31,1 > + bl bar > + mr 9,3 > + mr 3,9 > + addi 11,31,32 > + lwz 0,4(11) > + mtlr 0 > + lwz 31,-4(11) > + mr 1,11 > + blr > + .size foo,.-foo > + .ident "GCC: (Ubuntu 4.8.2-19ubuntu1) 4.8.2" > + .section .note.GNU-stack,"",@progbits > -- > > > On 01.12.2017. 20:36, pedromfc wrote: >> Hello Nikola, Kevin, >> >> Thank you for providing these patches. >> >> I tested both patches (Nicola's second patch and Kevin's patch) on >> ppc64le, and there were no regressions, except for some additional >> expected failures in gdb.threads/attach-many-short-lived-threads.exp. >> >> Some comments: >> >> - Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within >> the first if. This is useful for the case when a second mflr Rx with Rx >> != 0 is detected. Previously this would cause r0_contains_arg to be >> reset, despite Rx not being 0. However, if the second mflr has Rx == 0, >> it would make sense to reset r0_contains_arg (and this would work >> accidentally before the patch, assuming the first mflr also had Rx == >> 0). Maybe the best solution here is to always check the Rx contained in >> the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, >> or leave it as before since it's a separate issue. >> >> - Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, >> and op is an unsigned long. Will the unshifted reg always fit in a int? >> >> - I think it's more clear to only set lr_register when needed (pc >> reaches the limit), as opposed to resetting it to -1 if pc didn't reach >> the limit. >> >> - I wasn't able to directly apply Nikola's patch, I did so manually. >> >> I've also attached a reproducer (prologue.c/foo.S) to check if the >> patches fixed the issue. I had to alter a generated assembly file so >> that mflr would use a register other than R0 (in which case the old code >> does work). >> >> gcc -g0 -O0 -o prologue prologue.c foo.S >> >> (gdb) file prologue >> Reading symbols from prologue...done. >> (gdb) break bar >> Breakpoint 1 at 0x100005b8 >> (gdb) run >> >> Starting program: /home/pedromfc/prologue >> >> Before the patch: >> >> Breakpoint 1, 0x00000000100005b8 in bar () >> (gdb) bt >> #0 0x00000000100005b8 in bar () >> #1 0x0000000010000644 in foo () >> Backtrace stopped: frame did not save the PC >> >> After (both patches): >> >> Breakpoint 1, 0x00000000100005b8 in bar () >> (gdb) bt >> #0 0x00000000100005b8 in bar () >> #1 0x0000000010000644 in foo () >> #2 0x00000000100005f8 in main () >> >> (gdb) info f 1 >> Stack frame at 0x7fffffffeee0: >> pc = 0x10000644 in foo; saved pc = 0x100005f8 >> >> Thanks! >> Pedro >> >> On 11/09/2017 04:15 PM, Nikola Prica wrote: >>> Hi Kevin, >>> >>> lr_reg could be also set to -2 in part of code which is reachable >>> after shifting removal. >>> >>> /* Invalidate lr_reg, but don't set it to -1. >>> That would mean that it had never been set. */ >>> lr_reg = -2; >>> >>> This part of the code which depends of non shifted lr_reg, and the >>> part where shifting is removed are only two places where lr_reg is >>> changed. As so, I've added last condition to set fdata->lr_register on >>> -1 if lim_pc is not reached. >>> >>> If it seems fine now could you pleas commit it because I don't have >>> rights to do it. >>> >>> Thanks, >>> >>> Nikola Prica >>> >>> >>> From: Prica <nprica@rt-rk.com> >>> Date: Thu, 9 Nov 2017 13:10:48 +0100 >>> Subject: Fix for prologue processing on PowerPC >>> >>> One of conditions in skip_prologue() is never visited because it >>> expects non shifted `lr_reg`. That condition is supposed to set PC >>> offset. When body of this condition is visited PC offset is set and >>> there will be no need to look for it in next frames nor to use frame >>> unwind directives. >>> >>> gdb/ChangeLog: >>> >>> *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg >>> and assign shifted lr_reg to fdata->lr_register when lr_reg is >>> set. If iteration do not hit lim_pc lr_register is set as -1. >>> --- >>> gdb/rs6000-tdep.c | 13 ++++++++----- >>> 1 file changed, 8 insertions(+), 5 deletions(-) >>> >>> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c >>> index 6c44995..6f05ef5 100644 >>> --- a/gdb/rs6000-tdep.c >>> +++ b/gdb/rs6000-tdep.c >>> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, >>> CORE_ADDR pc, CORE_ADDR lim_pc, >>> remember just the first one, but skip over additional >>> ones. */ >>> if (lr_reg == -1) >>> - lr_reg = (op & 0x03e00000) >> 21; >>> - if (lr_reg == 0) >>> - r0_contains_arg = 0; >>> + { >>> + lr_reg = (op & 0x03e00000); >>> + fdata->lr_register = lr_reg >> 21; >>> + if (lr_reg == 0) >>> + r0_contains_arg = 0; >>> + } >>> continue; >>> } >>> else if ((op & 0xfc1fffff) == 0x7c000026) >>> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, >>> CORE_ADDR pc, CORE_ADDR lim_pc, >>> } >>> #endif /* 0 */ >>> >>> - if (pc == lim_pc && lr_reg >= 0) >>> - fdata->lr_register = lr_reg; >>> + if (pc != lim_pc) >>> + fdata->lr_register = -1; >>> >>> fdata->offset = -fdata->offset; >>> return last_prologue_pc; >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-12-29 18:05 ` Pedro Franco de Carvalho @ 2018-01-09 11:22 ` Nikola Prica 2018-01-10 17:26 ` Pedro Franco de Carvalho 0 siblings, 1 reply; 19+ messages in thread From: Nikola Prica @ 2018-01-09 11:22 UTC (permalink / raw) To: Pedro Franco de Carvalho, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand [-- Attachment #1: Type: text/plain, Size: 3469 bytes --] Hiello Pedro, >> > - I think it's more clear to only set lr_register when needed (pc >> > reaches the limit), as opposed to resetting it to -1 if pc didn't reach >> > the limit. >> >> The body of condition that becomes visitable after this patch >> invalidates lr_reg by setting it to -2 before reaching the limit. > > Sorry, I was referring to to fdata->lr_register. What I meant is that it > seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end > of the function (as Kevin had suggested), as opposed to setting it to -1 > at the end of the function, in this part: > I didn't explain it right. The catch is that fdata->lr_register is set by shifting lr_reg variable which is set only once and it can be invalidated druing the loop iteration. For example, lets say that firstly lr_reg is set to some value, then in some further loop iteration the next code gets visited: else if (lr_reg >= 0 && /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || /* stw Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x90010000)) || /* stwu Rx, NUM(r1) */ ((op & 0xffff0000) == (lr_reg | 0x94010000)))) { /* where Rx == lr */ fdata->lr_offset = offset; fdata->nosavedpc = 0; /* Invalidate lr_reg, but don't set it to -1. That would mean that it had never been set. */ lr_reg = -2; This part of code was previously never visited because of lr_reg shifting. The last line in upper code overwrites value of lr_reg. If in the furtherer iteration lim_pc would be reached, -2 value would be shifted and set to fdata->lr_register and that is not what we want. > > Previously if there was a mflr/stw sequence that invalidated lr_reg, > fdata->lr_register would be -1 when the function exited, even if pc == > lim_pc (because of the second term of the condition). > Previously if there was s mflr/stw sequence lr_reg would never be invalidated (set to -2) because part of the code that does this invalidation was never visitable due to shifting of lr_reg. Invalidation is performed so that the upper code never gets visited again in same iteration. lr_reg is set only once and it's value is used only in the condition of upper part of the code. In other conditions it is only checked whether it is set (different than -1). The comment where lr_reg is changed from -1 to some value says the following: "remember just the first one, but skip over additional ones." Which means that we need to deal only with one value of lr_reg. And the fdata->lr_register basically saves the register number that we get by shifting lr_reg. > > In any case, for this part to be correct, shouldn't the condition be the > full complement of the original condition? > > That is, > > if (pc != lim_pc || lr_reg < 0) > > Which is the complement of: > > if (pc == lim_pc && lr_reg >= 0) > > Instead of: > >> + if (pc != lim_pc) I've changed this condition to following: if (pc != lim_pc && lr_reg != -1) This is more appropriate condition because this condition requires change of lr_reg. > Still, I think it's safer to keep fdata->lr_register == -1 and set it at > the end if needed. > This could be achieved by using one more variable that would be backup for lr_reg in case it is set to -2. But I think that this approach with setting it to -1 is more appropriate than that? Thanks, Nikola Prica [-- Attachment #2: 0001-PATCH-Fix-for-prologue-processing-on-PowerPc.patch --] [-- Type: text/x-patch, Size: 6391 bytes --] From d93689d414137926818429fe0910ad9a9e6ef0dd Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Tue, 19 Dec 2017 14:29:09 +0100 Subject: [PATCH] [PATCH] Fix for prologue processing on PowerPc One of conditions in skip_prologue() is never visited because it expects non shifted `lr_reg`. That condtition is supposed to set PC offset. When body of this condition is visited PC offset is set and there will be no need to look for it in next frames nor to use frame unwind directives. gdb/ChangeLog: *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. If iteration do not hit lim_pc lr_register is set as -1. *testsuite/gdb.arch/ppc-prologue-frame.s: New file. *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise. *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise. --- gdb/rs6000-tdep.c | 14 ++++--- gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 47 +++++++++++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 456dbcc..7bf4f60 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; - if (lr_reg == 0) - r0_contains_arg = 0; + { + lr_reg = (op & 0x03e00000); + fdata->lr_register = lr_reg >> 21; + } + if (lr_reg == 0) + r0_contains_arg = 0; + continue; } else if ((op & 0xfc1fffff) == 0x7c000026) @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, } #endif /* 0 */ - if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + if (pc != lim_pc && lr_reg != -1) + fdata->lr_register = -1; fdata->offset = -fdata->offset; return last_prologue_pc; diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S new file mode 100644 index 0000000..e30ca23 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S @@ -0,0 +1,35 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <ppc-asm.h> + +FUNC_START(foo) + stwu 1,-32(1) + mflr 3 + stw 3,36(1) + stw 31,28(1) + mr 31,1 + bl bar + mr 9,3 + mr 3,9 + addi 11,31,32 + lwz 0,4(11) + mtlr 0 + lwz 31,-4(11) + mr 1,11 + blr +FUNC_END(foo) diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c new file mode 100644 index 0000000..8cab6f2 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c @@ -0,0 +1,28 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int bar() +{ + return 0; +} + +int foo(); + +int main(void) +{ + return foo(); +} diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp new file mode 100644 index 0000000..4f988db --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp @@ -0,0 +1,47 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/> + +if {![istarget "powerpc*"] } { + verbose "Skipping powerpc back trace test." + return +} + +standard_testfile .c .S +set binfile [standard_output_file ${testfile}] + +if {![istarget "powerpc-*-*"]} then { + verbose "Skipping PowerPC instructions disassembly." + return -1 +} + + +if {[gdb_compile \ + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ + "${binfile}" executable {}] != ""} { + untested "failed to build $binfile" + return -1 +} + + +clean_restart ${binfile} + +if ![runto bar] { + untested "could not run to bar" + return -1 +} + +gdb_test "bt" \ + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ + "Backtrace to the main frame" -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-09 11:22 ` Nikola Prica @ 2018-01-10 17:26 ` Pedro Franco de Carvalho 2018-01-11 15:12 ` Nikola Prica 0 siblings, 1 reply; 19+ messages in thread From: Pedro Franco de Carvalho @ 2018-01-10 17:26 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Hello Nikola, > This part of code was previously never visited because of lr_reg > shifting. The last line in upper code overwrites value of lr_reg. If in > the furtherer iteration lim_pc would be reached, -2 value would be > shifted and set to fdata->lr_register and that is not what we want. It could be visited when lr_reg == 0 (because in this case lr_reg >> 21 == lr_reg). I suspect this is a common case, when I used gcc it usually generated the mflr/stw sequence using r0. In the old code, even if the limit was reached, the fdata->lr_register variable would not be set to lr_reg if lr_reg was previously invalidated (set to -2), assuming mflr/stw used r0. You are correct that previously, for any other register than r0, the block that detects the "stw" part of the sequence would never be reached, which would cause fdata->lr_register to be set, if lim_pc was reached, even if LR was saved in the stack by the stw. However, it seems to me that this was unintentional, and that the original intention was to set fdata->lr_register only if it was valid and not yet saved in the stack when skip_prologue was called. So I think it's safer to preserve this behavior, that is, fdata->lr_register should be -1 at function exit if either the limit wasn't reached or lr_reg was saved to the stack or never seen (which would be reflected by -2 or -1, respectively). Note that the comment in the definition of struct rs6000_framedata specifices "if trustworthy". int lr_register; /* register of saved lr, if trustworthy */ Here is an example. Take the sequence: foo: stwu 1,-32(1) mflr 0 nop ... If you break at the nop, and run backtrace, skip_prologue will be called to build the frame_cache and figure out where the link register was saved so that the program counter from the previous stack frame can be found. In this case, skip_prologue will be called with lim_pc pointing to the nop (which is where we are). Because r0 is not saved back to the stack, and because in this case pc reaches lim_pc, fdata->lr_register is valid and should hold lr_reg. Take this other sequence: foo: stwu 1,-32(1) mflr 0 stw 0,36(1) li 0,-1 nop If we break at the nop, and call backtrace, LR will already have been saved in the stack, so fdata->lr_offset should be valid, but not fdata->lr_register. Note that it was overwritten by the li instruction, so r0 can no longer be trusted to hold the value of LR. I suspect the callers of skip_prologue will use lr_offset over lr_register if both are set, but I think it's better to be conservative and ensure fdata is valid. >> Still, I think it's safer to keep fdata->lr_register == -1 and set it at >> the end if needed. >> > > > This could be achieved by using one more variable that would be backup > for lr_reg in case it is set to -2. But I think that this approach with > setting it to -1 is more appropriate than that? It could be done without an additional variable by shifting lr_reg and assigning the result to lr_register only at the end, like in Kevin's suggestion. I'm just not sure that the bitwise operations with lr_reg are safe considering it is an int (e.g., is lr_reg | 0x90010000 well-defined?). Thanks! Pedro Nikola Prica <nikola.prica@rt-rk.com> writes: > Hiello Pedro, > >>> > - I think it's more clear to only set lr_register when needed (pc >>> > reaches the limit), as opposed to resetting it to -1 if pc didn't reach >>> > the limit. >>> >>> The body of condition that becomes visitable after this patch >>> invalidates lr_reg by setting it to -2 before reaching the limit. >> >> Sorry, I was referring to to fdata->lr_register. What I meant is that it >> seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end >> of the function (as Kevin had suggested), as opposed to setting it to -1 >> at the end of the function, in this part: >> > > I didn't explain it right. The catch is that fdata->lr_register is set > by shifting lr_reg variable which is set only once and it can be > invalidated druing the loop iteration. For example, lets say that > firstly lr_reg is set to some value, then in some further loop iteration > the next code gets visited: > > else if (lr_reg >= 0 && > /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */ > (((op & 0xffff0000) == (lr_reg | 0xf8010000)) || > /* stw Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x90010000)) || > /* stwu Rx, NUM(r1) */ > ((op & 0xffff0000) == (lr_reg | 0x94010000)))) > { /* where Rx == lr */ > fdata->lr_offset = offset; > fdata->nosavedpc = 0; > /* Invalidate lr_reg, but don't set it to -1. > That would mean that it had never been set. */ > lr_reg = -2; > > This part of code was previously never visited because of lr_reg > shifting. The last line in upper code overwrites value of lr_reg. If in > the furtherer iteration lim_pc would be reached, -2 value would be > shifted and set to fdata->lr_register and that is not what we want. > > >> >> Previously if there was a mflr/stw sequence that invalidated lr_reg, >> fdata->lr_register would be -1 when the function exited, even if pc == >> lim_pc (because of the second term of the condition). >> > > > Previously if there was s mflr/stw sequence lr_reg would never be > invalidated (set to -2) because part of the code that does this > invalidation was never visitable due to shifting of lr_reg. Invalidation > is performed so that the upper code never gets visited again in same > iteration. lr_reg is set only once and it's value is used only in the > condition of upper part of the code. In other conditions it is only > checked whether it is set (different than -1). The comment where lr_reg > is changed from -1 to some value says the following: > > "remember just the first one, but skip over additional ones." > > Which means that we need to deal only with one value of lr_reg. And the > fdata->lr_register basically saves the register number that we get by > shifting lr_reg. > >> >> In any case, for this part to be correct, shouldn't the condition be the >> full complement of the original condition? >> >> That is, >> >> if (pc != lim_pc || lr_reg < 0) >> >> Which is the complement of: >> >> if (pc == lim_pc && lr_reg >= 0) >> >> Instead of: >> >>> + if (pc != lim_pc) > > > I've changed this condition to following: > > if (pc != lim_pc && lr_reg != -1) > > This is more appropriate condition because this condition requires > change of lr_reg. > > >> Still, I think it's safer to keep fdata->lr_register == -1 and set it at >> the end if needed. >> > > > This could be achieved by using one more variable that would be backup > for lr_reg in case it is set to -2. But I think that this approach with > setting it to -1 is more appropriate than that? > > Thanks, > > Nikola Prica > From d93689d414137926818429fe0910ad9a9e6ef0dd Mon Sep 17 00:00:00 2001 > From: Prica <nprica@rt-rk.com> > Date: Tue, 19 Dec 2017 14:29:09 +0100 > Subject: [PATCH] [PATCH] Fix for prologue processing on PowerPc > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condtition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign > shifted lr_reg to fdata->lr_register when lr_reg is set. If > iteration do not hit lim_pc lr_register is set as -1. > *testsuite/gdb.arch/ppc-prologue-frame.s: New file. > *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise. > *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise. > --- > gdb/rs6000-tdep.c | 14 ++++--- > gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 47 +++++++++++++++++++++++ > 4 files changed, 119 insertions(+), 5 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 456dbcc..7bf4f60 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > remember just the first one, but skip over additional > ones. */ > if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > - if (lr_reg == 0) > - r0_contains_arg = 0; > + { > + lr_reg = (op & 0x03e00000); > + fdata->lr_register = lr_reg >> 21; > + } > + if (lr_reg == 0) > + r0_contains_arg = 0; > + > continue; > } > else if ((op & 0xfc1fffff) == 0x7c000026) > @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > } > #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > + if (pc != lim_pc && lr_reg != -1) > + fdata->lr_register = -1; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > new file mode 100644 > index 0000000..e30ca23 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > @@ -0,0 +1,35 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +#include <ppc-asm.h> > + > +FUNC_START(foo) > + stwu 1,-32(1) > + mflr 3 > + stw 3,36(1) > + stw 31,28(1) > + mr 31,1 > + bl bar > + mr 9,3 > + mr 3,9 > + addi 11,31,32 > + lwz 0,4(11) > + mtlr 0 > + lwz 31,-4(11) > + mr 1,11 > + blr > +FUNC_END(foo) > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > new file mode 100644 > index 0000000..8cab6f2 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > @@ -0,0 +1,28 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +int bar() > +{ > + return 0; > +} > + > +int foo(); > + > +int main(void) > +{ > + return foo(); > +} > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > new file mode 100644 > index 0000000..4f988db > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > @@ -0,0 +1,47 @@ > +# Copyright 2018 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/> > + > +if {![istarget "powerpc*"] } { > + verbose "Skipping powerpc back trace test." > + return > +} > + > +standard_testfile .c .S > +set binfile [standard_output_file ${testfile}] > + > +if {![istarget "powerpc-*-*"]} then { > + verbose "Skipping PowerPC instructions disassembly." > + return -1 > +} > + > + > +if {[gdb_compile \ > + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ > + "${binfile}" executable {}] != ""} { > + untested "failed to build $binfile" > + return -1 > +} > + > + > +clean_restart ${binfile} > + > +if ![runto bar] { > + untested "could not run to bar" > + return -1 > +} > + > +gdb_test "bt" \ > + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ > + "Backtrace to the main frame" > -- > 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-10 17:26 ` Pedro Franco de Carvalho @ 2018-01-11 15:12 ` Nikola Prica 2018-01-19 7:49 ` Nikola Prica 0 siblings, 1 reply; 19+ messages in thread From: Nikola Prica @ 2018-01-11 15:12 UTC (permalink / raw) To: Pedro Franco de Carvalho, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Hi Pedro, I understand it now. Thank you for explanation. > I'm just not sure that the bitwise operations with > lr_reg are safe considering it is an int (e.g., is lr_reg | 0x90010000 > well-defined?). > Regarding that lr_reg is changed to valid value only when it is previously set -1 and that it is changed like lr_reg = (op & 0x03e00000) it means that it only takes bits between 21-26. I would say that it is safe. I suppose that it is set to int for easier way of verifying its validity. Thanks, Nikola Prica ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-11 15:12 ` Nikola Prica @ 2018-01-19 7:49 ` Nikola Prica 2018-01-19 19:01 ` Pedro Franco de Carvalho 2018-01-19 19:08 ` Pedro Franco de Carvalho 0 siblings, 2 replies; 19+ messages in thread From: Nikola Prica @ 2018-01-19 7:49 UTC (permalink / raw) To: Pedro Franco de Carvalho, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand [-- Attachment #1: Type: text/plain, Size: 103 bytes --] Hi Pedro, I've just seen that changed version of patch was not attached. Best regards, Nikola Prica [-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPC.patch --] [-- Type: text/x-patch, Size: 1763 bytes --] From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Thu, 9 Nov 2017 13:10:48 +0100 Subject: [PATCH] Fix for prologue processing on PowerPC One of conditions in skip_prologue() is never visited because it expects non shifted `lr_reg`. That condition is supposed to set PC offset. When body of this condition is visited PC offset is set and there will be no need to look for it in next frames nor to use frame unwind directives. gdb/ChangeLog: *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. If iteration do not hit lim_pc lr_register is set as -1. --- gdb/rs6000-tdep.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index 6c44995..6f05ef5 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; - if (lr_reg == 0) - r0_contains_arg = 0; + { + lr_reg = (op & 0x03e00000); + fdata->lr_register = lr_reg >> 21; + if (lr_reg == 0) + r0_contains_arg = 0; + } continue; } else if ((op & 0xfc1fffff) == 0x7c000026) @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, } #endif /* 0 */ - if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + if (pc != lim_pc) + fdata->lr_register = -1; fdata->offset = -fdata->offset; return last_prologue_pc; -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-19 7:49 ` Nikola Prica @ 2018-01-19 19:01 ` Pedro Franco de Carvalho 2018-01-19 19:08 ` Pedro Franco de Carvalho 1 sibling, 0 replies; 19+ messages in thread From: Pedro Franco de Carvalho @ 2018-01-19 19:01 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Nikola Prica <nikola.prica@rt-rk.com> writes: > Hi Pedro, > > I've just seen that changed version of patch was not attached. > > Best regards, > > Nikola Prica > From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001 > From: Prica <nprica@rt-rk.com> > Date: Thu, 9 Nov 2017 13:10:48 +0100 > Subject: [PATCH] Fix for prologue processing on PowerPC > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg > and assign shifted lr_reg to fdata->lr_register when lr_reg is > set. If iteration do not hit lim_pc lr_register is set as -1. > --- > gdb/rs6000-tdep.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 6c44995..6f05ef5 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > remember just the first one, but skip over additional > ones. */ > if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > - if (lr_reg == 0) > - r0_contains_arg = 0; > + { > + lr_reg = (op & 0x03e00000); > + fdata->lr_register = lr_reg >> 21; > + if (lr_reg == 0) > + r0_contains_arg = 0; > + } > continue; > } > else if ((op & 0xfc1fffff) == 0x7c000026) > @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > } > #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > + if (pc != lim_pc) > + fdata->lr_register = -1; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > -- > 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-19 7:49 ` Nikola Prica 2018-01-19 19:01 ` Pedro Franco de Carvalho @ 2018-01-19 19:08 ` Pedro Franco de Carvalho 2018-01-27 14:32 ` Nikola Prica 1 sibling, 1 reply; 19+ messages in thread From: Pedro Franco de Carvalho @ 2018-01-19 19:08 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Hello Nikola, Is that the version you meant to send? That looks like the same as this one: https://sourceware.org/ml/gdb-patches/2017-11/msg00207.html Thanks, Pedro Nikola Prica <nikola.prica@rt-rk.com> writes: > Hi Pedro, > > I've just seen that changed version of patch was not attached. > > Best regards, > > Nikola Prica > From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001 > From: Prica <nprica@rt-rk.com> > Date: Thu, 9 Nov 2017 13:10:48 +0100 > Subject: [PATCH] Fix for prologue processing on PowerPC > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condition is supposed to set PC > offset. When body of this condition is visited PC offset is set and > there will be no need to look for it in next frames nor to use frame > unwind directives. > > gdb/ChangeLog: > *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg > and assign shifted lr_reg to fdata->lr_register when lr_reg is > set. If iteration do not hit lim_pc lr_register is set as -1. > --- > gdb/rs6000-tdep.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 6c44995..6f05ef5 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > remember just the first one, but skip over additional > ones. */ > if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > - if (lr_reg == 0) > - r0_contains_arg = 0; > + { > + lr_reg = (op & 0x03e00000); > + fdata->lr_register = lr_reg >> 21; > + if (lr_reg == 0) > + r0_contains_arg = 0; > + } > continue; > } > else if ((op & 0xfc1fffff) == 0x7c000026) > @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > } > #endif /* 0 */ > > - if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > + if (pc != lim_pc) > + fdata->lr_register = -1; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > -- > 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-19 19:08 ` Pedro Franco de Carvalho @ 2018-01-27 14:32 ` Nikola Prica 2018-01-31 18:04 ` Pedro Franco de Carvalho 0 siblings, 1 reply; 19+ messages in thread From: Nikola Prica @ 2018-01-27 14:32 UTC (permalink / raw) To: Pedro Franco de Carvalho, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand [-- Attachment #1: Type: text/plain, Size: 121 bytes --] Hi Pedro, Thank you for your review and patience. I'm really grateful. Can someone commit this patch? Thanks, Nikola [-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPc.patch --] [-- Type: text/x-patch, Size: 6127 bytes --] From 4b32b0441739d7daa931b13029f1c8215a4b1f4e Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Thu, 11 Jan 2018 14:49:15 +0100 Subject: [PATCH] Fix for prologue processing on PowerPc One of conditions in skip_prologue() was never visited if there was mflr instruction that moves the link register to a register different thant r0. This condition expects non shifted value of `lr_reg`. Previously offset of link register was never saved for registers different than r0. gdb/ChangeLog: 2018-01-27 Nikola Prica <nikola.prica@rt-rk.com> *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. gdb/testsuite/ChangeLog: 2018-01-27 Nikola Prica <nikola.prica@rt-rk.com> *gdb.arch/ppc-prologue-frame.s: New file. *gdb.arch/ppc-prologue-frame.c: Likewise. *gdb.arch/ppr-prologue-frame.exp: Likewise. --- gdb/rs6000-tdep.c | 4 +-- gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index e5a265d..da5182e 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; + lr_reg = (op & 0x03e00000); if (lr_reg == 0) r0_contains_arg = 0; continue; @@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, #endif /* 0 */ if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + fdata->lr_register = lr_reg >> 21; fdata->offset = -fdata->offset; return last_prologue_pc; diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S new file mode 100644 index 0000000..e30ca23 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S @@ -0,0 +1,35 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <ppc-asm.h> + +FUNC_START(foo) + stwu 1,-32(1) + mflr 3 + stw 3,36(1) + stw 31,28(1) + mr 31,1 + bl bar + mr 9,3 + mr 3,9 + addi 11,31,32 + lwz 0,4(11) + mtlr 0 + lwz 31,-4(11) + mr 1,11 + blr +FUNC_END(foo) diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c new file mode 100644 index 0000000..8cab6f2 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c @@ -0,0 +1,28 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int bar() +{ + return 0; +} + +int foo(); + +int main(void) +{ + return foo(); +} diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp new file mode 100644 index 0000000..d26314b --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp @@ -0,0 +1,41 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/> + +if {![istarget "powerpc-*-*"] } { + verbose "Skipping powerpc back trace test." + return +} + +standard_testfile .c .S +set binfile [standard_output_file ${testfile}] + +if {[gdb_compile \ + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ + "${binfile}" executable {}] != ""} { + untested "failed to build $binfile" + return -1 +} + + +clean_restart ${binfile} + +if ![runto bar] { + untested "could not run to bar" + return -1 +} + +gdb_test "bt" \ + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ + "Backtrace to the main frame" -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-27 14:32 ` Nikola Prica @ 2018-01-31 18:04 ` Pedro Franco de Carvalho 2018-01-31 18:28 ` Ulrich Weigand 0 siblings, 1 reply; 19+ messages in thread From: Pedro Franco de Carvalho @ 2018-01-31 18:04 UTC (permalink / raw) To: Nikola Prica, Kevin Buettner Cc: gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Looks good to me. Just two details for the testsuite changelog, the filenames in the entry don't match the actual filenames, and there's a space between the "*" and the entries. I tested the patch with ppc64le and ppc32, and it works. No regressions. Thanks! Pedro Nikola Prica <nikola.prica@rt-rk.com> writes: > Hi Pedro, > > Thank you for your review and patience. I'm really grateful. > > Can someone commit this patch? > > Thanks, > > Nikola > From 4b32b0441739d7daa931b13029f1c8215a4b1f4e Mon Sep 17 00:00:00 2001 > From: Prica <nprica@rt-rk.com> > Date: Thu, 11 Jan 2018 14:49:15 +0100 > Subject: [PATCH] Fix for prologue processing on PowerPc > > One of conditions in skip_prologue() was never visited if there was mflr > instruction that moves the link register to a register different thant r0. > This condition expects non shifted value of `lr_reg`. Previously offset > of link register was never saved for registers different than r0. > > gdb/ChangeLog: > > 2018-01-27 Nikola Prica <nikola.prica@rt-rk.com> > > *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and > assign shifted lr_reg to fdata->lr_register when lr_reg is set. > > gdb/testsuite/ChangeLog: > > 2018-01-27 Nikola Prica <nikola.prica@rt-rk.com> > > *gdb.arch/ppc-prologue-frame.s: New file. > *gdb.arch/ppc-prologue-frame.c: Likewise. > *gdb.arch/ppr-prologue-frame.exp: Likewise. > --- > gdb/rs6000-tdep.c | 4 +-- > gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++ > 4 files changed, 106 insertions(+), 2 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index e5a265d..da5182e 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > remember just the first one, but skip over additional > ones. */ > if (lr_reg == -1) > - lr_reg = (op & 0x03e00000) >> 21; > + lr_reg = (op & 0x03e00000); > if (lr_reg == 0) > r0_contains_arg = 0; > continue; > @@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, > #endif /* 0 */ > > if (pc == lim_pc && lr_reg >= 0) > - fdata->lr_register = lr_reg; > + fdata->lr_register = lr_reg >> 21; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > new file mode 100644 > index 0000000..e30ca23 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > @@ -0,0 +1,35 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +#include <ppc-asm.h> > + > +FUNC_START(foo) > + stwu 1,-32(1) > + mflr 3 > + stw 3,36(1) > + stw 31,28(1) > + mr 31,1 > + bl bar > + mr 9,3 > + mr 3,9 > + addi 11,31,32 > + lwz 0,4(11) > + mtlr 0 > + lwz 31,-4(11) > + mr 1,11 > + blr > +FUNC_END(foo) > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > new file mode 100644 > index 0000000..8cab6f2 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > @@ -0,0 +1,28 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +int bar() > +{ > + return 0; > +} > + > +int foo(); > + > +int main(void) > +{ > + return foo(); > +} > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > new file mode 100644 > index 0000000..d26314b > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > @@ -0,0 +1,41 @@ > +# Copyright 2018 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/> > + > +if {![istarget "powerpc-*-*"] } { > + verbose "Skipping powerpc back trace test." > + return > +} > + > +standard_testfile .c .S > +set binfile [standard_output_file ${testfile}] > + > +if {[gdb_compile \ > + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ > + "${binfile}" executable {}] != ""} { > + untested "failed to build $binfile" > + return -1 > +} > + > + > +clean_restart ${binfile} > + > +if ![runto bar] { > + untested "could not run to bar" > + return -1 > +} > + > +gdb_test "bt" \ > + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ > + "Backtrace to the main frame" > -- > 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-31 18:04 ` Pedro Franco de Carvalho @ 2018-01-31 18:28 ` Ulrich Weigand 2018-02-01 13:09 ` Nikola Prica 0 siblings, 1 reply; 19+ messages in thread From: Ulrich Weigand @ 2018-01-31 18:28 UTC (permalink / raw) To: Pedro Franco de Carvalho Cc: Nikola Prica, Kevin Buettner, gdb-patches, "Ananthakrishna Sowda (asowda)", "Ivan Baev (ibaev)", 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand Pedro Franco de Carvalho wrote: > Looks good to me. Just two details for the testsuite changelog, > the filenames in the entry don't match the actual filenames, and there's > a space between the "*" and the entries. > > I tested the patch with ppc64le and ppc32, and it works. No regressions. > > Thanks! > Pedro > > Nikola Prica <nikola.prica@rt-rk.com> writes: > > > Hi Pedro, > > > > Thank you for your review and patience. I'm really grateful. > > > > Can someone commit this patch? The patch looks good to me as well. Thanks for the review, Pedro! I've checked this is now. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2018-01-31 18:28 ` Ulrich Weigand @ 2018-02-01 13:09 ` Nikola Prica 0 siblings, 0 replies; 19+ messages in thread From: Nikola Prica @ 2018-02-01 13:09 UTC (permalink / raw) To: Ulrich Weigand, Pedro Franco de Carvalho Cc: Kevin Buettner, gdb-patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), 'Nemanja Popov', Djordje Todorovic, Ulrich.Weigand [-- Attachment #1: Type: text/plain, Size: 50 bytes --] Thank you for your review. Best regards, Nikola [-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPc.patch --] [-- Type: text/x-patch, Size: 6142 bytes --] From d6d907b16d7e7d2a9ee7f147a0d0a1166cf557ae Mon Sep 17 00:00:00 2001 From: Prica <nprica@rt-rk.com> Date: Thu, 11 Jan 2018 14:49:15 +0100 Subject: [PATCH] Fix for prologue processing on PowerPc One of conditions in skip_prologue() was never visited if there was mflr instruction that moves the link register to a register different thant r0. This condition expects non shifted value of `lr_reg`. Previously offset of link register was never saved for registers different than r0. gdb/ChangeLog: 2018-02-01 Nikola Prica <nikola.prica@rt-rk.com> * rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and assign shifted lr_reg to fdata->lr_register when lr_reg is set. gdb/testsuite/ChangeLog: 2018-02-01 Nikola Prica <nikola.prica@rt-rk.com> * gdb.arch/powerpc-prologue-frame.s: New file. * gdb.arch/powerpc-prologue-frame.c: Likewise. * gdb.arch/poweroc-prologue-frame.exp: Likewise. --- gdb/rs6000-tdep.c | 4 +-- gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++++ gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index e5a265d..da5182e 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, remember just the first one, but skip over additional ones. */ if (lr_reg == -1) - lr_reg = (op & 0x03e00000) >> 21; + lr_reg = (op & 0x03e00000); if (lr_reg == 0) r0_contains_arg = 0; continue; @@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc, #endif /* 0 */ if (pc == lim_pc && lr_reg >= 0) - fdata->lr_register = lr_reg; + fdata->lr_register = lr_reg >> 21; fdata->offset = -fdata->offset; return last_prologue_pc; diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S new file mode 100644 index 0000000..e30ca23 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S @@ -0,0 +1,35 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <ppc-asm.h> + +FUNC_START(foo) + stwu 1,-32(1) + mflr 3 + stw 3,36(1) + stw 31,28(1) + mr 31,1 + bl bar + mr 9,3 + mr 3,9 + addi 11,31,32 + lwz 0,4(11) + mtlr 0 + lwz 31,-4(11) + mr 1,11 + blr +FUNC_END(foo) diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c new file mode 100644 index 0000000..8cab6f2 --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c @@ -0,0 +1,28 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int bar() +{ + return 0; +} + +int foo(); + +int main(void) +{ + return foo(); +} diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp new file mode 100644 index 0000000..d26314b --- /dev/null +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp @@ -0,0 +1,41 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/> + +if {![istarget "powerpc-*-*"] } { + verbose "Skipping powerpc back trace test." + return +} + +standard_testfile .c .S +set binfile [standard_output_file ${testfile}] + +if {[gdb_compile \ + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ + "${binfile}" executable {}] != ""} { + untested "failed to build $binfile" + return -1 +} + + +clean_restart ${binfile} + +if ![runto bar] { + untested "could not run to bar" + return -1 +} + +gdb_test "bt" \ + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ + "Backtrace to the main frame" -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PING][PATCH] Fix for prologue processing on PowerPC 2017-12-19 15:57 ` Nikola Prica 2017-12-29 18:05 ` Pedro Franco de Carvalho @ 2018-01-02 10:29 ` Yao Qi 1 sibling, 0 replies; 19+ messages in thread From: Yao Qi @ 2018-01-02 10:29 UTC (permalink / raw) To: Nikola Prica Cc: pedromfc, Kevin Buettner, GDB Patches, Ananthakrishna Sowda (asowda), Ivan Baev (ibaev), Nemanja Popov, Djordje Todorovic, Ulrich.Weigand On Tue, Dec 19, 2017 at 3:57 PM, Nikola Prica <nikola.prica@rt-rk.com> wrote: > Hello Pedro and Kevi, > > I've created and tested example based on your example. Thank you for that. > gdb.arch/powerpc-prologue-frame.exp is useful, but it is only run on powerpc target. I personally prefer using gdb unit test or self test to test powerpc prologue analyser can stop at the the right instruction after scanning a series of instructions. See aarch64-tdep.c:aarch64_analyze_prologue_test. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-02-01 13:09 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-22 12:11 Fix for prologue processing on PowerPC Nikola Prica 2017-10-05 2:01 ` Kevin Buettner 2017-10-26 10:02 ` [PING]Fix " Nikola Prica 2017-11-08 16:58 ` Kevin Buettner 2017-11-09 18:15 ` [PING][PATCH] Fix " Nikola Prica 2017-12-01 19:37 ` pedromfc 2017-12-19 15:57 ` Nikola Prica 2017-12-29 18:05 ` Pedro Franco de Carvalho 2018-01-09 11:22 ` Nikola Prica 2018-01-10 17:26 ` Pedro Franco de Carvalho 2018-01-11 15:12 ` Nikola Prica 2018-01-19 7:49 ` Nikola Prica 2018-01-19 19:01 ` Pedro Franco de Carvalho 2018-01-19 19:08 ` Pedro Franco de Carvalho 2018-01-27 14:32 ` Nikola Prica 2018-01-31 18:04 ` Pedro Franco de Carvalho 2018-01-31 18:28 ` Ulrich Weigand 2018-02-01 13:09 ` Nikola Prica 2018-01-02 10:29 ` Yao Qi
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).