This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PING][PATCH] Fix for prologue processing on PowerPC
- From: Nikola Prica <nikola dot prica at rt-rk dot com>
- To: pedromfc <pedromfc at linux dot vnet dot ibm dot com>, Kevin Buettner <kevinb at redhat dot com>
- Cc: gdb-patches at sourceware dot org, "Ananthakrishna Sowda (asowda)" <asowda at cisco dot com>, "Ivan Baev (ibaev)" <ibaev at cisco dot com>, 'Nemanja Popov' <nemanja dot popov at rt-rk dot com>, Djordje Todorovic <Djordje dot Todorovic at rt-rk dot com>, Ulrich dot Weigand at de dot ibm dot com
- Date: Tue, 19 Dec 2017 16:57:34 +0100
- Subject: Re: [PING][PATCH] Fix for prologue processing on PowerPC
- Authentication-results: sourceware.org; auth=none
- References: <f90d189e-7ec5-34f9-c776-8af42a3c07a6@rt-rk.com> <e3274bee-eb2d-3706-3af0-59f28c9c0899@rt-rk.com> <20171108095850.394a48ca@pinnacle.lan> <8bf0014c-e83c-5988-4d06-173572f21186@rt-rk.com> <7ba16b14-9384-34d9-937e-531a2192842a@linux.vnet.ibm.com>
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;