[PATCH 0/1] ARM: Fix crash when frame cannot be found

[PATCH 0/1] ARM: Fix crash when frame cannot be found

[Meador Inge]

This patch addresses an issue with the ARM prologue analyzer that occurs
when the frame pointer cannot be deduced.  The way things are currently
written GDB crashes in some cases.

For example, a crash can occur when the first instruction in the assembly
function in question is 'mov sp, r0'.  When stepping GDB kicks off a prologue
analysis from 'arm-tdep.c:arm_make_prologue_cache'.  However, it can't actually
find the frame, so it sets the cached frame reg to -1.  Later on in
'arm_make_prologue_cache' GDB tries to pass 'cache->framereg' to
'get_frame_register_unsigned' and crashes.

This patch fixes the problem by always falling back on the SP register when
the frame cannot be computed.  A similar strategy is used on other
architectures.  GDB testsuite run with 'target sim'; no regressions.  Some
ad hoc testing done on actual hardware as well.

OK?

Meador Inge (1):
  arm: Change prologue analyzer to always fallback on SP.

 gdb/arm-tdep.c                            |   16 +------------
 gdb/testsuite/gdb.arch/thumb-prologue.c   |   34 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/thumb-prologue.exp |   27 +++++++++++++++++++++++
 3 files changed, 63 insertions(+), 14 deletions(-)

[PATCH 1/1] ARM: Change prologue analyzer to always fallback on SP.

[Meador Inge]

2011-07-19  Meador Inge  <meadori@codesourcery.com>

	* arm-tdep.c (thumb_analyze_prologue): Always fallback on the SP
	register when the frame can't be determined.
	* arm-tdep.c (arm_analyze_prologue): Ditto.

2011-07-19  Meador Inge  <meadori@codesourcery.com>

	* gdb.arch/thumb-prologue.c (switch_stack_to_same): New test function.
	(switch_stack_to_other): New test function.
	* gdb.arch/thumb-prologue.exp: New test cases.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 gdb/arm-tdep.c                            |   16 +------------
 gdb/testsuite/gdb.arch/thumb-prologue.c   |   34 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/thumb-prologue.exp |   27 +++++++++++++++++++++++
 3 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 1a75af1..9aeec48 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1150,18 +1150,12 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
       cache->framereg = THUMB_FP_REGNUM;
       cache->framesize = -regs[THUMB_FP_REGNUM].k;
     }
-  else if (pv_is_register (regs[ARM_SP_REGNUM], ARM_SP_REGNUM))
+  else
     {
       /* Try the stack pointer... this is a bit desperate.  */
       cache->framereg = ARM_SP_REGNUM;
       cache->framesize = -regs[ARM_SP_REGNUM].k;
     }
-  else
-    {
-      /* We're just out of luck.  We don't know where the frame is.  */
-      cache->framereg = -1;
-      cache->framesize = 0;
-    }
 
   for (i = 0; i < 16; i++)
     if (pv_area_find_reg (stack, gdbarch, i, &offset))
@@ -1881,18 +1875,12 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
       framereg = ARM_FP_REGNUM;
       framesize = -regs[ARM_FP_REGNUM].k;
     }
-  else if (pv_is_register (regs[ARM_SP_REGNUM], ARM_SP_REGNUM))
+  else
     {
       /* Try the stack pointer... this is a bit desperate.  */
       framereg = ARM_SP_REGNUM;
       framesize = -regs[ARM_SP_REGNUM].k;
     }
-  else
-    {
-      /* We're just out of luck.  We don't know where the frame is.  */
-      framereg = -1;
-      framesize = 0;
-    }
 
   if (cache)
     {
diff --git a/gdb/testsuite/gdb.arch/thumb-prologue.c b/gdb/testsuite/gdb.arch/thumb-prologue.c
index bb24df0..a726149 100644
--- a/gdb/testsuite/gdb.arch/thumb-prologue.c
+++ b/gdb/testsuite/gdb.arch/thumb-prologue.c
@@ -18,11 +18,15 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 void tpcs_frame (void);
+void switch_stack_to_same (void);
+void switch_stack_to_other (void);
 
 int
 main (void)
 {
   tpcs_frame ();
+  switch_stack_to_same ();
+  switch_stack_to_other ();
   return 0;
 }
 
@@ -104,3 +108,33 @@ asm(".text\n"
     "	mov	lr, r3\n"
     "	bx	lr\n"
 );
+
+asm(".text\n"
+    "	.align 2\n"
+    "	.thumb_func\n"
+    "	.code 16\n"
+    "write_sp:\n"
+    "	mov	sp, r0\n"
+    "	bx	lr\n"
+
+    "	.align 2\n"
+    "	.thumb_func\n"
+    "	.code 16\n"
+    "switch_stack_to_same:\n"
+    "	push	{lr}\n"
+    "	mov	r0, sp\n"
+    "	bl	write_sp\n"
+    "	pop	{r1}\n"
+    "	bx	r1\n"
+
+    "	.align 2\n"
+    "	.thumb_func\n"
+    "	.code 16\n"
+    "switch_stack_to_other:\n"
+    "	push	{lr}\n"
+    "	mov	r7, sp\n"
+    "	mov	r0, #128\n"
+    "	bl	write_sp\n"
+    "	mov	sp, r7\n"
+    "	pop	{r1}\n"
+    "	bx	r1\n");
diff --git a/gdb/testsuite/gdb.arch/thumb-prologue.exp b/gdb/testsuite/gdb.arch/thumb-prologue.exp
index e685bc5..39b61c4 100644
--- a/gdb/testsuite/gdb.arch/thumb-prologue.exp
+++ b/gdb/testsuite/gdb.arch/thumb-prologue.exp
@@ -59,3 +59,30 @@ gdb_test "backtrace 10" \
 gdb_test "info frame" \
 	".*Saved registers:.*r7 at.*r10 at.*r11 at.*lr at.*" \
 	"saved registers in TPCS"
+
+
+# Testcase for "switching" the stack to the same stack in the prologue.
+
+gdb_breakpoint "switch_stack_to_same"
+
+gdb_test "continue" "Breakpoint .*, $hex in switch_stack_to_same \\(\\)" \
+	"continue to switch_stack_to_same"
+
+gdb_test "stepi 2" "in write_sp \\(\\)" "stepi over mov sp, sp"
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in write_sp .*\r\n#1\[ \t\]*$hex in switch_stack_to_same .*\r\n#2\[ \t\]*$hex in main.*" \
+	"backtrace in write_sp"
+
+# Testcase for switching to another stack in the prologue.
+
+gdb_breakpoint "switch_stack_to_other"
+
+gdb_test "continue" "Breakpoint .*, $hex in switch_stack_to_other \\(\\)" \
+	"continue to switch_stack_to_other"
+
+gdb_test "stepi 2" "in write_sp \\(\\)" "stepi over mov sp, 128"
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in write_sp .*\r\n#1\[ \t\]*$hex in switch_stack_to_other .*\r\n#2\[ \t\]*$hex in main.*" \
+	"backtrace in write_sp"
-- 
1.7.0.4

Re: [PATCH 1/1] ARM: Change prologue analyzer to always fallback on SP.

[Daniel Jacobowitz]

On Tue, Jul 19, 2011 at 2:43 PM, Meador Inge <meadori@codesourcery.com> wrote:
> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>
>        * arm-tdep.c (thumb_analyze_prologue): Always fallback on the SP
>        register when the frame can't be determined.
>        * arm-tdep.c (arm_analyze_prologue): Ditto.
>
> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>
>        * gdb.arch/thumb-prologue.c (switch_stack_to_same): New test function.
>        (switch_stack_to_other): New test function.
>        * gdb.arch/thumb-prologue.exp: New test cases.

This looks OK to me.

-- 
Thanks,
Daniel

Re: [PATCH 1/1] ARM: Change prologue analyzer to always fallback on SP.

[Meador Inge]

On 07/19/2011 03:30 PM, Daniel Jacobowitz wrote:

> On Tue, Jul 19, 2011 at 2:43 PM, Meador Inge <meadori@codesourcery.com> wrote:
>> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>>
>>        * arm-tdep.c (thumb_analyze_prologue): Always fallback on the SP
>>        register when the frame can't be determined.
>>        * arm-tdep.c (arm_analyze_prologue): Ditto.
>>
>> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>>
>>        * gdb.arch/thumb-prologue.c (switch_stack_to_same): New test function.
>>        (switch_stack_to_other): New test function.
>>        * gdb.arch/thumb-prologue.exp: New test cases.
> 
> This looks OK to me.

Thanks.  I just now got my FSF copyright assignment worked
out (on file as "William Meador Inge").  Could someone please
commit this for me?  The same patch still applies.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

Re: [PATCH 1/1] ARM: Change prologue analyzer to always fallback on SP.

[Meador Inge]

On 10/24/2011 07:58 PM, Meador Inge wrote:
> On 07/19/2011 03:30 PM, Daniel Jacobowitz wrote:
> 
>> On Tue, Jul 19, 2011 at 2:43 PM, Meador Inge <meadori@codesourcery.com> wrote:
>>> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>>>
>>>        * arm-tdep.c (thumb_analyze_prologue): Always fallback on the SP
>>>        register when the frame can't be determined.
>>>        * arm-tdep.c (arm_analyze_prologue): Ditto.
>>>
>>> 2011-07-19  Meador Inge  <meadori@codesourcery.com>
>>>
>>>        * gdb.arch/thumb-prologue.c (switch_stack_to_same): New test function.
>>>        (switch_stack_to_other): New test function.
>>>        * gdb.arch/thumb-prologue.exp: New test cases.
>>
>> This looks OK to me.
> 
> Thanks.  I just now got my FSF copyright assignment worked
> out (on file as "William Meador Inge").  Could someone please
> commit this for me?  The same patch still applies.

I have access now.  Committed.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software