arm software watchpoint: return to epilogue

Message ID 87zjepf4qd.fsf@codesourcery.com
State New, archived
Headers

Commit Message

Yao Qi Aug. 28, 2014, 7:33 a.m. UTC
  Pedro Alves <palves@redhat.com> writes:

> This gdbarch hook's name is a bit misleading -- your comment above kind
> of makes it sound like the patch is doing some kind of target specific

I write down a lot to describe the problem on thumb mode and it gives a
feeling that it is a target specific hack.

> hack, while this is exactly how the gdbarch hook is specified:
>
> # A target might have problems with watchpoints as soon as the stack
> # frame of the current function has been destroyed.  This mostly happens
> # as the first action in a funtion's epilogue.  in_function_epilogue_p()
> # is defined to return a non-zero value if either the given addr is one
>                                                                  ^^^^^^
> # instruction after the stack destroying instruction up to the trailing
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # return instruction or if we can figure out that the stack frame has
> # already been invalidated regardless of the value of addr.  Targets
> # which don't suffer from that problem could just let this functionality
> # untouched.
> m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0
>

Yes, this gdbarch hook's name is misleading.  How about renaming it to
stack_frame_destroyed_p?

>> The patch is tested in arm-none-eabi and arm-none-linux-gnueabi with
>> various multilibs.  OK to apply?
>
> This is OK with Will's comment addressed.

The changelog entry is updated, and the assignment to found_stack_adjust
is removed from the patch.  Patch below is pushed in.
  

Comments

Pedro Alves Aug. 28, 2014, 9:05 a.m. UTC | #1
On 08/28/2014 08:33 AM, Yao Qi wrote:
> Yes, this gdbarch hook's name is misleading.  How about renaming it to
> stack_frame_destroyed_p?

I like that.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c7ccb41..b6d8469 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@ 
 2014-08-28  Yao Qi  <yao@codesourcery.com>
 
+	* arm-tdep.c (thumb_in_function_epilogue_p): Don't set
+	found_stack_adjust in forward scan.  Remove condition check
+	on found_stack_adjust which is always true.  Indent the code.
+
+2014-08-28  Yao Qi  <yao@codesourcery.com>
+
 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
 	(handle_DW_AT_stmt_list): Remove comment about WANT_LINE_INFO.
 	(dwarf_decode_lines): Remove argument
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 9bc6507..f9feb52 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3273,7 +3273,6 @@  thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	found_return = 1;
       else if (thumb_instruction_restores_sp (insn))
 	{
-	  found_stack_adjust = 1;
 	  if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
 	    found_return = 1;
 	}
@@ -3287,20 +3286,18 @@  thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 
 	  if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
 	    {
-	      found_stack_adjust = 1;
 	      if (insn2 & 0x8000)  /* <registers> include PC.  */
 		found_return = 1;
 	    }
 	  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
 		   && (insn2 & 0x0fff) == 0x0b04)
 	    {
-	      found_stack_adjust = 1;
 	      if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
 		found_return = 1;
 	    }
 	  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
 		   && (insn2 & 0x0e00) == 0x0a00)
-	    found_stack_adjust = 1;
+	    ;
 	  else
 	    break;
 	}
@@ -3317,27 +3314,24 @@  thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
      a 32-bit instruction.  This is just a heuristic, so we do not worry
      too much about false positives.  */
 
-  if (!found_stack_adjust)
-    {
-      if (pc - 4 < func_start)
-	return 0;
-      if (target_read_memory (pc - 4, buf, 4))
-	return 0;
-
-      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
-      insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
+  if (pc - 4 < func_start)
+    return 0;
+  if (target_read_memory (pc - 4, buf, 4))
+    return 0;
 
-      if (thumb_instruction_restores_sp (insn2))
-	found_stack_adjust = 1;
-      else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
-	found_stack_adjust = 1;
-      else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
-	       && (insn2 & 0x0fff) == 0x0b04)
-	found_stack_adjust = 1;
-      else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
-	       && (insn2 & 0x0e00) == 0x0a00)
-	found_stack_adjust = 1;
-    }
+  insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
+  insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
+
+  if (thumb_instruction_restores_sp (insn2))
+    found_stack_adjust = 1;
+  else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
+    found_stack_adjust = 1;
+  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
+	   && (insn2 & 0x0fff) == 0x0b04)
+    found_stack_adjust = 1;
+  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
+	   && (insn2 & 0x0e00) == 0x0a00)
+    found_stack_adjust = 1;
 
   return found_stack_adjust;
 }