[PATCH] Prevent internal-error when computing $pc in ARM assembly code

[PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Martin Simmons]

This patch prevents a gdb internal-error when computing $pc for ARM assembly
code that doesn't use the normal stack frame convention.

It might also help with gdb/12223.

I'm not subscribed to the list so please include me on any replies.


gdb/ChangeLog:

	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
	exception, to allow debugging of assembly code.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 8181f25..d47b4af 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1669,8 +1669,11 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
        current_pc < prologue_end;
        current_pc += 4)
     {
-      unsigned int insn
-	= read_memory_unsigned_integer (current_pc, 4, byte_order_for_code);
+      unsigned int insn;
+      gdb_byte buf[4];
+      if (target_read_memory (current_pc, buf, 4))
+	break;
+      insn = extract_unsigned_integer (buf, 4, byte_order_for_code);
 
       if (insn == 0xe1a0c00d)		/* mov ip, sp */
 	{

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Joel Brobecker]

On Wed, May 20, 2015 at 10:49:47AM +0100, Martin Simmons wrote:
> This patch prevents a gdb internal-error when computing $pc for ARM assembly
> code that doesn't use the normal stack frame convention.
> 
> It might also help with gdb/12223.
> 
> I'm not subscribed to the list so please include me on any replies.
> 
> 
> gdb/ChangeLog:
> 
> 	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
> 	exception, to allow debugging of assembly code.
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 8181f25..d47b4af 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -1669,8 +1669,11 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
>         current_pc < prologue_end;
>         current_pc += 4)
>      {
> -      unsigned int insn
> -	= read_memory_unsigned_integer (current_pc, 4, byte_order_for_code);
> +      unsigned int insn;
> +      gdb_byte buf[4];
> +      if (target_read_memory (current_pc, buf, 4))
> +	break;
> +      insn = extract_unsigned_integer (buf, 4, byte_order_for_code);

It would be good to have a few more details about what causes us
to be in a situation where we'd be failing to read memory at
an address. Perhaps just showing the disassembled code and
a quick explanation of what happens might be sufficient.

Also, for our piece of mind, we normally ask that the change be
validated by running the testsuite. Did you do that? If yes,
on which platform?

Lastly - a small nitpick, due to GDB's Coding style, which requires
that an empty line be inserted between local variable declarations
and the first statement after that. So, would you mind adding an empty
line after "buf"'s declaration?

Other than that, I think the patch looks reasonable. I'm just a little
unsure about how you can be trying to read an instruction at an address
that's unreadable. Looking at the code, and in particular, looking at
the callers, I think I might have some idea, but I'd prefer if you
showed us the issue that you actually hit.

Thank you,
-- 
Joel

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Martin Simmons]

>>>>> On Tue, 9 Jun 2015 14:44:08 -0400, Joel Brobecker said:
> 
> On Wed, May 20, 2015 at 10:49:47AM +0100, Martin Simmons wrote:
> > This patch prevents a gdb internal-error when computing $pc for ARM assembly
> > code that doesn't use the normal stack frame convention.
> > 
> > It might also help with gdb/12223.
> > 
> > I'm not subscribed to the list so please include me on any replies.
> > 
> > 
> > gdb/ChangeLog:
> > 
> > 	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
> > 	exception, to allow debugging of assembly code.
> > 
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index 8181f25..d47b4af 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -1669,8 +1669,11 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
> >         current_pc < prologue_end;
> >         current_pc += 4)
> >      {
> > -      unsigned int insn
> > -	= read_memory_unsigned_integer (current_pc, 4, byte_order_for_code);
> > +      unsigned int insn;
> > +      gdb_byte buf[4];
> > +      if (target_read_memory (current_pc, buf, 4))
> > +	break;
> > +      insn = extract_unsigned_integer (buf, 4, byte_order_for_code);
> 
> It would be good to have a few more details about what causes us
> to be in a situation where we'd be failing to read memory at
> an address. Perhaps just showing the disassembled code and
> a quick explanation of what happens might be sufficient.

Thanks for looking at it.

The code being debugged uses r11 (ARM_FP_REGNUM) as a global pointer rather
than a frame register, so lines 1968...1984 of arm-tdep.c ("We have no symbol
information.") computes bogus values for prologue_start and prologue_end.

 
> Also, for our piece of mind, we normally ask that the change be
> validated by running the testsuite. Did you do that? If yes,
> on which platform?

Yes, I ran make check-gdb on Raspbian GNU/Linux 7, but it produces around 1200
failures with or without my change, including some non-deterministic ones.


> Lastly - a small nitpick, due to GDB's Coding style, which requires
> that an empty line be inserted between local variable declarations
> and the first statement after that. So, would you mind adding an empty
> line after "buf"'s declaration?

Ah, sorry.  Here is the new version.  I've also included a new test, which
passes for me on Raspbian GNU/Linux 7.

gdb/ChangeLog:

	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
	exception, to allow debugging of assembly code.

gdb/testsuite/ChangeLog:
	* gdb.arch/arm-r11-non-pointer.S: New file.
	* gdb.arch/arm-r11-non-pointer.exp: New file.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c99f2a9..fd07bca 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1669,8 +1669,12 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
        current_pc < prologue_end;
        current_pc += 4)
     {
-      unsigned int insn
-	= read_memory_unsigned_integer (current_pc, 4, byte_order_for_code);
+      unsigned int insn;
+      gdb_byte buf[4];
+
+      if (target_read_memory (current_pc, buf, 4))
+	break;
+      insn = extract_unsigned_integer (buf, 4, byte_order_for_code);
 
       if (insn == 0xe1a0c00d)		/* mov ip, sp */
 	{
diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
new file mode 100644
index 0000000..89884e3
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
@@ -0,0 +1,45 @@
+/* Copyright 2010-2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+	.syntax unified
+	.text
+	.type main,%function
+#if defined (__thumb__)
+	.code   16
+	.thumb_func
+#endif
+
+	.align	2
+textdataregion:
+	.word	dataregion
+
+	.globl main
+main:
+	stmfd	sp!, {r3, lr}
+	bl	.L0
+	movs 	r0, #0
+	ldmfd	sp!, {r3, pc}
+	.size main, .-main
+
+.L0:	ldr	r11, textdataregion
+	mov	r11, r11
+	mov	pc, lr
+
+	.data
+	.align 2
+dataregion:
+	.word	64,65,66,67
diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
new file mode 100644
index 0000000..1def957
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
@@ -0,0 +1,69 @@
+# Copyright 2010-2015 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test arm non-pointer in r11.
+
+if {![istarget "arm*-*-*"]} then {
+    verbose "Skipping arm r11 non-pointer tests."
+    return
+}
+
+set testfile "arm-r11-non-pointer"
+set srcfile ${testfile}.S
+set binfile ${objdir}/${subdir}/${testfile}
+
+set additional_flags "-Wa,-g"
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
+    untested arm-r11-non-pointer.exp
+    return -1
+}
+
+# Get things started.
+
+clean_restart ${testfile}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "x/i \$pc" \
+    ".*bl.*0x.*" \
+    "x/i pc"
+
+gdb_test "stepi" \
+    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
+    "stepi"
+
+gdb_test "x/i \$pc" \
+    ".*ldr.*r11,.*" \
+    "x/i pc"
+
+gdb_test "stepi" \
+    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
+    "stepi"
+
+gdb_test "x/i \$pc" \
+    ".*mov.*r11,.*r11.*" \
+    "x/i pc"
+
+##########################################
+
+# Done, run program to exit.
+
+gdb_continue_to_end "arm-r11-non-pointer"

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Joel Brobecker]

> Thanks for looking at it.
> 
> The code being debugged uses r11 (ARM_FP_REGNUM) as a global pointer
> rather than a frame register, so lines 1968...1984 of arm-tdep.c ("We
> have no symbol information.") computes bogus values for prologue_start
> and prologue_end.

OK, so indeed the problem occurs because arm_analyze_prologue
was given a bogus PC range. Great!

> > Also, for our piece of mind, we normally ask that the change be
> > validated by running the testsuite. Did you do that? If yes,
> > on which platform?
> 
> Yes, I ran make check-gdb on Raspbian GNU/Linux 7, but it produces
> around 1200 failures with or without my change, including some
> non-deterministic ones.

That's perfect. What we ask is that the failures before and after
are the same.

> Ah, sorry.  Here is the new version.  I've also included a new test, which
> passes for me on Raspbian GNU/Linux 7.

Awesome, we love tests! Thank you.

> gdb/ChangeLog:
> 
> 	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
> 	exception, to allow debugging of assembly code.
> 
> gdb/testsuite/ChangeLog:
> 	* gdb.arch/arm-r11-non-pointer.S: New file.
> 	* gdb.arch/arm-r11-non-pointer.exp: New file.

Looks good. A few little things in your new testcase...

> +++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
> @@ -0,0 +1,45 @@
> +/* Copyright 2010-2015 Free Software Foundation, Inc.

Did you really mean for the date range to be 2010-2015?
If yes, then great. If not, can you adjust it?

> diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
> new file mode 100644
> index 0000000..1def957
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
> @@ -0,0 +1,69 @@
> +# Copyright 2010-2015 Free Software Foundation, Inc.

Same here...

> +set testfile "arm-r11-non-pointer"
> +set srcfile ${testfile}.S
> +set binfile ${objdir}/${subdir}/${testfile}

Can you use "standard_testfile .S" instead of the 3 commands above?

> +set additional_flags "-Wa,-g"
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
> +    untested arm-r11-non-pointer.exp
> +    return -1
> +}
> +
> +# Get things started.
> +
> +clean_restart ${testfile}

Can you use prepare_for_testing instead of gdb_compile and
clean_restart?

> +gdb_test "x/i \$pc" \
> +    ".*bl.*0x.*" \
> +    "x/i pc"
> +
> +gdb_test "stepi" \
> +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> +    "stepi"
> +
> +gdb_test "x/i \$pc" \
> +    ".*ldr.*r11,.*" \
> +    "x/i pc"
> +
> +gdb_test "stepi" \
> +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> +    "stepi"
> +
> +gdb_test "x/i \$pc" \
> +    ".*mov.*r11,.*r11.*" \
> +    "x/i pc"
> +

We require that all test "labels" be unique. Would you mind ammending
them to make them so. Eg: replace the first "x/i pc" by "x/i pc at
such and such instruction", etc. Same for the "stepi" ones.

Thank you,
-- 
Joel

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Martin Simmons]

>>>>> On Fri, 12 Jun 2015 09:21:13 -0400, Joel Brobecker said:
> 
> Looks good. A few little things in your new testcase...
> 
> > +++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
> > @@ -0,0 +1,45 @@
> > +/* Copyright 2010-2015 Free Software Foundation, Inc.
> 
> Did you really mean for the date range to be 2010-2015?
> If yes, then great. If not, can you adjust it?

I started with the code from another testcase, but it has pretty much all been
rewriten now so I've adjusted it to be 2015.

> > +set testfile "arm-r11-non-pointer"
> > +set srcfile ${testfile}.S
> > +set binfile ${objdir}/${subdir}/${testfile}
> 
> Can you use "standard_testfile .S" instead of the 3 commands above?
> 
> > +set additional_flags "-Wa,-g"
> > +
> > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
> > +    untested arm-r11-non-pointer.exp
> > +    return -1
> > +}
> > +
> > +# Get things started.
> > +
> > +clean_restart ${testfile}
> 
> Can you use prepare_for_testing instead of gdb_compile and
> clean_restart?
> 
> > +gdb_test "x/i \$pc" \
> > +    ".*bl.*0x.*" \
> > +    "x/i pc"
> > +
> > +gdb_test "stepi" \
> > +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> > +    "stepi"
> > +
> > +gdb_test "x/i \$pc" \
> > +    ".*ldr.*r11,.*" \
> > +    "x/i pc"
> > +
> > +gdb_test "stepi" \
> > +    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
> > +    "stepi"
> > +
> > +gdb_test "x/i \$pc" \
> > +    ".*mov.*r11,.*r11.*" \
> > +    "x/i pc"
> > +
> 
> We require that all test "labels" be unique. Would you mind ammending
> them to make them so. Eg: replace the first "x/i pc" by "x/i pc at
> such and such instruction", etc. Same for the "stepi" ones.

OK, all done in the new version below.

gdb/ChangeLog:

	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
	exception, to allow debugging of assembly code.

gdb/testsuite/ChangeLog:
	* gdb.arch/arm-r11-non-pointer.S: New file.
	* gdb.arch/arm-r11-non-pointer.exp: New file.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c99f2a9..fd07bca 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1669,8 +1669,12 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
        current_pc < prologue_end;
        current_pc += 4)
     {
-      unsigned int insn
-	= read_memory_unsigned_integer (current_pc, 4, byte_order_for_code);
+      unsigned int insn;
+      gdb_byte buf[4];
+
+      if (target_read_memory (current_pc, buf, 4))
+	break;
+      insn = extract_unsigned_integer (buf, 4, byte_order_for_code);
 
       if (insn == 0xe1a0c00d)		/* mov ip, sp */
 	{
diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
new file mode 100644
index 0000000..40729fb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.S
@@ -0,0 +1,45 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+	.syntax unified
+	.text
+	.type main,%function
+#if defined (__thumb__)
+	.code   16
+	.thumb_func
+#endif
+
+	.align	2
+textdataregion:
+	.word	dataregion
+
+	.globl main
+main:
+	stmfd	sp!, {r3, lr}
+	bl	.L0
+	movs 	r0, #0
+	ldmfd	sp!, {r3, pc}
+	.size main, .-main
+
+.L0:	ldr	r11, textdataregion
+	mov	r11, r11
+	mov	pc, lr
+
+	.data
+	.align 2
+dataregion:
+	.word	64,65,66,67
diff --git a/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
new file mode 100644
index 0000000..d633a78
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-r11-non-pointer.exp
@@ -0,0 +1,60 @@
+# Copyright 2015 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test arm non-pointer in r11.
+
+if {![istarget "arm*-*-*"]} then {
+    verbose "Skipping arm r11 non-pointer tests."
+    return
+}
+
+standard_testfile .S
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} $srcfile [list debug "additional_flags=-Wa,-g"]] } {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "x/i \$pc" \
+    ".*bl.*0x.*" \
+    "x/i pc at bl"
+
+gdb_test "stepi" \
+    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
+    "stepi from bl to ldr"
+
+gdb_test "x/i \$pc" \
+    ".*ldr.*r11,.*" \
+    "x/i pc at ldr"
+
+gdb_test "stepi" \
+    "\\\?\\\? \\\(\\\) at .*$srcfile:.*" \
+    "stepi from ldr to mov"
+
+gdb_test "x/i \$pc" \
+    ".*mov.*r11,.*r11.*" \
+    "x/i pc at mov"
+
+##########################################
+
+# Done, run program to exit.
+
+gdb_continue_to_end "arm-r11-non-pointer"

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Joel Brobecker]

> gdb/ChangeLog:
> 
> 	* arm-tdep.c (arm_analyze_prologue): Read memory without throwing an
> 	exception, to allow debugging of assembly code.
> 
> gdb/testsuite/ChangeLog:
> 	* gdb.arch/arm-r11-non-pointer.S: New file.
> 	* gdb.arch/arm-r11-non-pointer.exp: New file.

Perfect! And approved :-). Thank you!

-- 
Joel

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Yao Qi]

Joel Brobecker <brobecker@adacore.com> writes:

> It would be good to have a few more details about what causes us
> to be in a situation where we'd be failing to read memory at
> an address. Perhaps just showing the disassembled code and
> a quick explanation of what happens might be sufficient.

I agree with Joel that we need more details about the problem you are
trying to fix.

-- 
Yao (齐尧)

Re: [PATCH] Prevent internal-error when computing $pc in ARM assembly code

[Joel Brobecker]

> > It would be good to have a few more details about what causes us
> > to be in a situation where we'd be failing to read memory at
> > an address. Perhaps just showing the disassembled code and
> > a quick explanation of what happens might be sufficient.
> 
> I agree with Joel that we need more details about the problem you are
> trying to fix.

I think the problem comes from improper unwinding of a function
having an unusual frame, causing us to use a bogus return address
which could be anywhere, including in an unreadable memory area.

-- 
Joel