Re: [patch] Fix PR tdep/12352: Handle str pc, [Rd, #imm] in displaced stepping

[Yao Qi <yao@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

On 01/22/2011 11:07 PM, Richard Earnshaw wrote:
>> > If it is a bug here, this patch is to address it.  These two lines of
>> > code is to compute the offset of `str pc'.  In this patch, we can do
>> > this in a different way,
>> > 
>> > 	str pc, [sp, #-4]
>> > 	ldr r4, [sp, #-4]
>> > 
>> > 
> No, code must not write below the stack -- the value can get corrupted
> if an interrupt occurs.  (I'm not sure if that's possible in this
> specific case as the debugger ought to be in control; but it's bad
> practice to violate the ABI in this way).

When these two instructions are running, debugger is not in control.

How about this insn sequence, which should comply with ABI?

	sub sp,	#4
	str pc, [sp]
	ldr r4, [sp]
	add sp, #4

Tested new patch in ARM native GDB with arm-disp-step.exp.  No failures.

-- 
Yao (齐尧)

[-- Attachment #2: pr12352-0127.patch --]
[-- Type: text/x-patch, Size: 4818 bytes --]

gdb/
        PR tdep/12352
        * arm-tdep.c (copy_ldr_str_ldrb_strb): Replace PC with SP in order to
        store PC value on stack instead of text section.
	* arm-tdep.h (DISPLACED_MODIFIED_INSNS): Increase it to 10.

gdb/testsuite/
        PR tdep/12352
        * gdb.arch/arm-disp-step.S : New test for str instruction.
        * gdb.arch/arm-disp-step.exp : Likewise.


diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 1f05b7a..a37ab15 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -5152,7 +5152,7 @@ copy_ldr_str_ldrb_strb (struct gdbarch *gdbarch, uint32_t insn,
      scratch+12: add r4, r4, #8  (r4 = offset)
      scratch+16: add r0, r0, r4
      scratch+20: str r0, [r2, #imm] (or str r0, [r2, r3])
-     scratch+24: <temp>
+     temp can be [sp, -4]
 
      Otherwise we don't know what value to write for PC, since the offset is
      architecture-dependent (sometimes PC+8, sometimes PC+12).  */
@@ -5176,23 +5176,25 @@ copy_ldr_str_ldrb_strb (struct gdbarch *gdbarch, uint32_t insn,
     {
       /* We need to use r4 as scratch.  Make sure it's restored afterwards.  */
       dsc->u.ldst.restore_r4 = 1;
+      dsc->modinsn[0] = 0xe24dd004;  /* sub sp, sp, #4 */
+      dsc->modinsn[1] = 0xe58df000;  /* str pc, [sp] */
+      dsc->modinsn[2] = 0xe59d4000;  /* ldr r4, [sp] */
+      dsc->modinsn[3] = 0xe28dd004;  /* add sp, sp, #4 */
 
-      dsc->modinsn[0] = 0xe58ff014;  /* str pc, [pc, #20].  */
-      dsc->modinsn[1] = 0xe59f4010;  /* ldr r4, [pc, #16].  */
-      dsc->modinsn[2] = 0xe044400f;  /* sub r4, r4, pc.  */
-      dsc->modinsn[3] = 0xe2844008;  /* add r4, r4, #8.  */
-      dsc->modinsn[4] = 0xe0800004;  /* add r0, r0, r4.  */
+      dsc->modinsn[4] = 0xe044400f;  /* sub r4, r4, pc.  */
+      dsc->modinsn[5] = 0xe2844008;  /* add r4, r4, #8.  */
+      dsc->modinsn[6] = 0xe0800004;  /* add r0, r0, r4.  */
 
       /* As above.  */
       if (immed)
-	dsc->modinsn[5] = (insn & 0xfff00fff) | 0x20000;
+	dsc->modinsn[7] = (insn & 0xfff00fff) | 0x20000;
       else
-	dsc->modinsn[5] = (insn & 0xfff00ff0) | 0x20003;
+	dsc->modinsn[7] = (insn & 0xfff00ff0) | 0x20003;
 
-      dsc->modinsn[6] = 0x0;  /* breakpoint location.  */
-      dsc->modinsn[7] = 0x0;  /* scratch space.  */
+      dsc->modinsn[8] = 0x0;  /* breakpoint location.  */
+      dsc->modinsn[9] = 0x0;  /* scratch space.  */
 
-      dsc->numinsns = 6;
+      dsc->numinsns = 8;
     }
 
   dsc->cleanup = load ? &cleanup_load : &cleanup_store;
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 61cdb5d..dbf2c14 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -209,7 +209,7 @@ struct gdbarch_tdep
 /* The maximum number of modified instructions generated for one single-stepped
    instruction, including the breakpoint (usually at the end of the instruction
    sequence) and any scratch words, etc.  */
-#define DISPLACED_MODIFIED_INSNS	8
+#define DISPLACED_MODIFIED_INSNS	10
 
 struct displaced_step_closure
 {
diff --git a/gdb/testsuite/gdb.arch/arm-disp-step.S b/gdb/testsuite/gdb.arch/arm-disp-step.S
index d748718..1aa7df1 100644
--- a/gdb/testsuite/gdb.arch/arm-disp-step.S
+++ b/gdb/testsuite/gdb.arch/arm-disp-step.S
@@ -48,6 +48,10 @@ test_ret_end:
 	bl test_ldm_stm_pc
 #endif
 
+	/* Test str in ARM mode and Thumb-2 */
+#if !defined(__thumb__)
+	bl test_str_pc
+#endif
 	/* Return */
 	mov     sp, r7
 	sub     sp, sp, #4
@@ -118,3 +122,17 @@ test_ldm_stm_pc_ret:
 	.word	test_ldm_stm_pc_ret
 	.size test_ldm_stm_pc, .-test_ldm_stm_pc
 #endif
+
+#if !defined(__thumb__)
+#if defined (__thumb2__)
+	.code   16
+	.thumb_func
+#endif
+	.global test_str_pc
+	.type test_str_pc, %function
+test_str_pc:
+	str     pc, [sp, #-4]
+	.global test_str_pc_end
+test_str_pc_end:
+	bx lr
+#endif
diff --git a/gdb/testsuite/gdb.arch/arm-disp-step.exp b/gdb/testsuite/gdb.arch/arm-disp-step.exp
index 826f728..f2df388 100644
--- a/gdb/testsuite/gdb.arch/arm-disp-step.exp
+++ b/gdb/testsuite/gdb.arch/arm-disp-step.exp
@@ -126,6 +126,29 @@ proc test_ldr_from_pc {} {
 	".*bx lr.*"
 }
 
+###########################################
+
+proc test_str_pc {} {
+    global srcfile
+    gdb_test_multiple "break *test_str_pc" "break test_str_pc" {
+	-re "Breakpoint.*at.* file .*$srcfile, line.*" {
+	    pass "break test_str_pc"
+	}
+	-re "No symbol.*" {
+	    pass "break test_str_pc"
+	    return
+	}
+    }
+    gdb_test "break *test_str_pc_end" \
+	"Breakpoint.*at.* file .*$srcfile, line.*" \
+	"break test_str_pc_end"
+
+    gdb_continue_to_breakpoint "continue to test_str_pc" \
+	".*str.*pc\,.*\[sp, #-4\].*"
+    gdb_continue_to_breakpoint "continue to test_str_pc_end" \
+	".*bx lr.*"
+}
+
 # Get things started.
 
 clean_restart ${testfile}
@@ -165,6 +188,7 @@ test_ldr_from_pc
 
 test_ldm_stm_pc
 
+test_str_pc
 ##########################################
 
 # Done, run program to exit.