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

[Martin Simmons <martin at lispworks dot com>]

>>>>> 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"