[rfc, arm] Always use correct execution state for single-step breakpoints

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hello,

about a year ago, Matthew Gretton-Dann added improved heuristics in an
attempt to ensure the breakpoints for software single-singlestepping
are correct for the target execution mode:
http://sourceware.org/ml/gdb-patches/2010-03/msg00168.html

Now in general, GDB needs to make some assumptions (usually by consulting
symbol tables) to determine the target execution mode for an arbitrary
code address.  However, when single-stepping, GDB detects the target
address by decoding the branch instruction anyway, and can at the same
time reliably detect the target execution mode.

With Matthew's patch, this information is used whenever GDB does not
have symbol information for the target address.  When GDB *does* have
symbol information, that still takes precedence.  Now usually this is
OK because the execution mode determined from symbol tables is still
correct.

However, we've had a bug report where symbols tables were present,
but contained the *wrong* execution mode for a symbol (due to an
error in hand-written assembler code).  Now this is certainly a bug
in the code (and led to the program crashing at run-time).  However,
you'd still expect a debugger to get this right -- bugs in the program
are why you need a debugger in the first place :-)

The following patch attempts to fix this test case by making sure that
for single-step breakpoints, the symbol table is *never* consulted,
but instead the known target execution mode is used.  This is done
by means of a new global "arm_override_mode" which is respected in
arm_pc_is_thumb unconditionally (similar to arm_force_mode_string),
and set by a new routine arm_insert_single_step_breakpoint which is
then used whenever setting a single-step breakpoint.  This routine
operates on the execution mode encoded in the LSB of the address;
to get at that, arm_get_next_pc is modified to return a PC in that
form.

(The global is a bit ugly, but the concept still seems right to me:
if at some point in the future, we actually do use two different
gdbarch's to represent Thumb vs. ARM, the single-step routines would
determine the proper gdbarch to set the breakpoint in and simply
use insert_single_step_breakpoint with that arch.)

The old single-step heuristics in arm_pc_is_thumb introduced by
Matthew is no longer needed, and removed by the patch.

As a final part of the patch, there was one case where 
arm_linux_syscall_next_pc would not properly set the Thumb bit;
this is fixed as well.

Tested on armv7l-unknown-linux-gnueabi with no regressions;
fixes the newly added test case.

Any comments?   I'm planning on committing this in a week or so.

Bye,
Ulrich


ChangeLog:

gdb/
	* arm-tdep.h (arm_insert_single_step_breakpoint): Add prototype.
	* arm-tdep.c (arm_override_mode): New global.
	(arm_pc_is_thumb): Respect arm_override_mode.  Remove single-step
	execution mode heuristics.
	(thumb_get_next_pc_raw): Remove INSERT_BKTP argument; always insert
	second single-step breakpoint if needed, using
	arm_insert_single_step_breakpoint.
	(arm_get_next_pc_raw): Remove INSERT_BKTP argument.  Only handle
	ARM execution mode, do not call thumb_get_next_pc_raw.
	(arm_get_next_pc): Encode execution mode in return value.  Call
	either arm_get_next_pc_raw or thumb_get_next_pc_raw.
	(arm_insert_single_step_breakpoint): New function.
	(arm_software_single_step): Call it.
	* arm-linux-tdep.c (arm_linux_sigreturn_return_addr): Add IS_THUMB
	argument to return execution mode of sigreturn target.
	(arm_linux_syscall_next_pc): Use it.
	(arm_linux_copy_svc): Update call.
	(arm_linux_software_single_step): Call
	arm_insert_single_step_breakpoint.

gdb/testsuite/
	* gdb.arch/thumb-singlestep.S: New file.
	* gdb.arch/thumb-singlestep.exp: Likewise.


Index: gdb/arm-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-linux-tdep.c,v
retrieving revision 1.84
diff -u -p -r1.84 arm-linux-tdep.c
--- gdb/arm-linux-tdep.c	8 Mar 2011 01:04:35 -0000	1.84
+++ gdb/arm-linux-tdep.c	23 Mar 2011 18:36:17 -0000
@@ -669,18 +669,24 @@ arm_linux_regset_from_core_section (stru
 }
 
 /* Copy the value of next pc of sigreturn and rt_sigrturn into PC,
-   and return 1.  Return 0 if it is not a rt_sigreturn/sigreturn
-   syscall.  */
+   return 1.  In addition, set IS_THUMB depending on whether we
+   will return to ARM or Thumb code.  Return 0 if it is not a
+   rt_sigreturn/sigreturn syscall.  */
 static int
 arm_linux_sigreturn_return_addr (struct frame_info *frame,
 				 unsigned long svc_number,
-				 CORE_ADDR *pc)
+				 CORE_ADDR *pc, int *is_thumb)
 {
   /* Is this a sigreturn or rt_sigreturn syscall?  */
   if (svc_number == 119 || svc_number == 173)
     {
       if (get_frame_type (frame) == SIGTRAMP_FRAME)
 	{
+	  ULONGEST t_bit = arm_psr_thumb_bit (frame_unwind_arch (frame));
+	  CORE_ADDR cpsr
+	    = frame_unwind_register_unsigned (frame, ARM_PS_REGNUM);
+
+	  *is_thumb = (cpsr & t_bit) != 0;
 	  *pc = frame_unwind_caller_pc (frame);
 	  return 1;
 	}
@@ -698,11 +704,11 @@ arm_linux_syscall_next_pc (struct frame_
   CORE_ADDR return_addr = 0;
   int is_thumb = arm_frame_is_thumb (frame);
   ULONGEST svc_number = 0;
-  int is_sigreturn = 0;
 
   if (is_thumb)
     {
       svc_number = get_frame_register_unsigned (frame, 7);
+      return_addr = pc + 2;
     }
   else
     {
@@ -721,24 +727,15 @@ arm_linux_syscall_next_pc (struct frame_
 	{
 	  svc_number = get_frame_register_unsigned (frame, 7);
 	}
+
+      return_addr = pc + 4;
     }
 
-  is_sigreturn = arm_linux_sigreturn_return_addr (frame, svc_number, 
-						  &return_addr);
+  arm_linux_sigreturn_return_addr (frame, svc_number, &return_addr, &is_thumb);
 
-  if (is_sigreturn)
-    return return_addr;
-  
+  /* Addresses for calling Thumb functions have the bit 0 set.  */
   if (is_thumb)
-    {
-      return_addr = pc + 2;
-      /* Addresses for calling Thumb functions have the bit 0 set.  */
-      return_addr |= 1;
-    }
-  else
-    {
-      return_addr = pc + 4;
-    }
+    return_addr |= 1;
 
   return return_addr;
 }
@@ -761,7 +758,7 @@ arm_linux_software_single_step (struct f
   if (next_pc > 0xffff0000)
     next_pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM);
 
-  insert_single_step_breakpoint (gdbarch, aspace, next_pc);
+  arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc);
 
   return 1;
 }
@@ -806,6 +803,7 @@ arm_linux_copy_svc (struct gdbarch *gdba
   struct frame_info *frame;
   unsigned int svc_number = displaced_read_reg (regs, dsc, 7);
   int is_sigreturn = 0;
+  int is_thumb;
 
   if (debug_displaced)
     fprintf_unfiltered (gdb_stdlog, "displaced: copying Linux svc insn %.8lx\n",
@@ -814,7 +812,7 @@ arm_linux_copy_svc (struct gdbarch *gdba
   frame = get_current_frame ();
 
   is_sigreturn = arm_linux_sigreturn_return_addr(frame, svc_number,
-						 &return_to);
+						 &return_to, &is_thumb);
   if (is_sigreturn)
     {
 	  struct symtab_and_line sal;
Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.338
diff -u -p -r1.338 arm-tdep.c
--- gdb/arm-tdep.c	21 Mar 2011 17:28:04 -0000	1.338
+++ gdb/arm-tdep.c	23 Mar 2011 18:36:18 -0000
@@ -133,6 +133,13 @@ static const char *arm_mode_strings[] =
 static const char *arm_fallback_mode_string = "auto";
 static const char *arm_force_mode_string = "auto";
 
+/* Internal override of the execution mode.  -1 means no override,
+   0 means override to ARM mode, 1 means override to Thumb mode.
+   The effect is the same as if arm_force_mode has been set by the
+   user (except the internal override has precedence over a user's
+   arm_force_mode override).  */
+static int arm_override_mode = -1;
+
 /* Number of different reg name sets (options).  */
 static int num_disassembly_options;
 
@@ -356,9 +363,6 @@ arm_find_mapping_symbol (CORE_ADDR memad
   return 0;
 }
 
-static CORE_ADDR arm_get_next_pc_raw (struct frame_info *frame, 
-				      CORE_ADDR pc, int insert_bkpt);
-
 /* Determine if the program counter specified in MEMADDR is in a Thumb
    function.  This function should be called for addresses unrelated to
    any executing frame; otherwise, prefer arm_frame_is_thumb.  */
@@ -388,6 +392,10 @@ arm_pc_is_thumb (struct gdbarch *gdbarch
   if (IS_THUMB_ADDR (memaddr))
     return 1;
 
+  /* Respect internal mode override if active.  */
+  if (arm_override_mode != -1)
+    return arm_override_mode;
+
   /* If the user wants to override the symbol table, let him.  */
   if (strcmp (arm_force_mode_string, "arm") == 0)
     return 0;
@@ -418,29 +426,9 @@ arm_pc_is_thumb (struct gdbarch *gdbarch
      target, then trust the current value of $cpsr.  This lets
      "display/i $pc" always show the correct mode (though if there is
      a symbol table we will not reach here, so it still may not be
-     displayed in the mode it will be executed).  
-   
-     As a further heuristic if we detect that we are doing a single-step we
-     see what state executing the current instruction ends up with us being
-     in.  */
+     displayed in the mode it will be executed).  */
   if (target_has_registers)
-    {
-      struct frame_info *current_frame = get_current_frame ();
-      CORE_ADDR current_pc = get_frame_pc (current_frame);
-      int is_thumb = arm_frame_is_thumb (current_frame);
-      CORE_ADDR next_pc;
-      if (memaddr == current_pc)
-	return is_thumb;
-      else
-	{
-	  struct gdbarch *gdbarch = get_frame_arch (current_frame);
-	  next_pc = arm_get_next_pc_raw (current_frame, current_pc, FALSE);
-	  if (memaddr == gdbarch_addr_bits_remove (gdbarch, next_pc))
-	    return IS_THUMB_ADDR (next_pc);
-	  else
-	    return is_thumb;
-	}
-    }
+    return arm_frame_is_thumb (get_current_frame ());
 
   /* Otherwise we're out of luck; we assume ARM.  */
   return 0;
@@ -4216,7 +4204,7 @@ thumb_advance_itstate (unsigned int itst
    another breakpoint by our caller.  */
 
 static CORE_ADDR
-thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc, int insert_bkpt)
+thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct address_space *aspace = get_frame_address_space (frame);
@@ -4314,8 +4302,8 @@ thumb_get_next_pc_raw (struct frame_info
 
 	      /* Set a breakpoint on the following instruction.  */
 	      gdb_assert ((itstate & 0x0f) != 0);
-	      if (insert_bkpt)
-	        insert_single_step_breakpoint (gdbarch, aspace, pc);
+	      arm_insert_single_step_breakpoint (gdbarch, aspace,
+						 MAKE_THUMB_ADDR (pc));
 	      cond_negated = (itstate >> 4) & 1;
 
 	      /* Skip all following instructions with the same
@@ -4587,8 +4575,7 @@ thumb_get_next_pc_raw (struct frame_info
 }
 
 /* Get the raw next address.  PC is the current program counter, in 
-   FRAME.  INSERT_BKPT should be TRUE if we want a breakpoint set on 
-   the alternative next instruction if there are two options.
+   FRAME, which is assumed to be executing in ARM mode.
 
    The value returned has the execution state of the next instruction 
    encoded in it.  Use IS_THUMB_ADDR () to see whether the instruction is
@@ -4596,7 +4583,7 @@ thumb_get_next_pc_raw (struct frame_info
    address.  */
 
 static CORE_ADDR
-arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc, int insert_bkpt)
+arm_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
@@ -4606,9 +4593,6 @@ arm_get_next_pc_raw (struct frame_info *
   unsigned long status;
   CORE_ADDR nextpc;
 
-  if (arm_frame_is_thumb (frame))
-    return thumb_get_next_pc_raw (frame, pc, insert_bkpt);
-
   pc_val = (unsigned long) pc;
   this_instr = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
 
@@ -4861,18 +4845,51 @@ arm_get_next_pc_raw (struct frame_info *
   return nextpc;
 }
 
+/* Determine next PC after current instruction executes.  Will call either
+   arm_get_next_pc_raw or thumb_get_next_pc_raw.  Error out if infinite
+   loop is detected.  */
+
 CORE_ADDR
 arm_get_next_pc (struct frame_info *frame, CORE_ADDR pc)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
-  CORE_ADDR nextpc = 
-    gdbarch_addr_bits_remove (gdbarch, 
-			      arm_get_next_pc_raw (frame, pc, TRUE));
-  if (nextpc == pc)
-    error (_("Infinite loop detected"));
+  CORE_ADDR nextpc;
+
+  if (arm_frame_is_thumb (frame))
+    {
+      nextpc = thumb_get_next_pc_raw (frame, pc);
+      if (nextpc == MAKE_THUMB_ADDR (pc))
+	error (_("Infinite loop detected"));
+    }
+  else
+    {
+      nextpc = arm_get_next_pc_raw (frame, pc);
+      if (nextpc == pc)
+	error (_("Infinite loop detected"));
+    }
+
   return nextpc;
 }
 
+/* Like insert_single_step_breakpoint, but make sure we use a breakpoint
+   of the appropriate mode (as encoded in the PC value), even if this
+   differs from what would be expected according to the symbol tables.  */
+
+void
+arm_insert_single_step_breakpoint (struct gdbarch *gdbarch,
+				   struct address_space *aspace,
+				   CORE_ADDR pc)
+{
+  struct cleanup *old_chain
+    = make_cleanup_restore_integer (&arm_override_mode);
+
+  arm_override_mode = IS_THUMB_ADDR (pc);
+  pc = gdbarch_addr_bits_remove (gdbarch, pc);
+
+  insert_single_step_breakpoint (gdbarch, aspace, pc);
+
+  do_cleanups (old_chain);
+}
+
 /* single_step() is called just before we want to resume the inferior,
    if we want to single-step it but there is no hardware or kernel
    single-step support.  We find the target of the coming instruction
@@ -4883,13 +4900,9 @@ arm_software_single_step (struct frame_i
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct address_space *aspace = get_frame_address_space (frame);
-
-  /* NOTE: This may insert the wrong breakpoint instruction when
-     single-stepping over a mode-changing instruction, if the
-     CPSR heuristics are used.  */
-
   CORE_ADDR next_pc = arm_get_next_pc (frame, get_frame_pc (frame));
-  insert_single_step_breakpoint (gdbarch, aspace, next_pc);
+
+  arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc);
 
   return 1;
 }
Index: gdb/arm-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.h,v
retrieving revision 1.49
diff -u -p -r1.49 arm-tdep.h
--- gdb/arm-tdep.h	8 Mar 2011 01:04:35 -0000	1.49
+++ gdb/arm-tdep.h	23 Mar 2011 18:36:18 -0000
@@ -311,6 +311,8 @@ extern void
 
 CORE_ADDR arm_skip_stub (struct frame_info *, CORE_ADDR);
 CORE_ADDR arm_get_next_pc (struct frame_info *, CORE_ADDR);
+void arm_insert_single_step_breakpoint (struct gdbarch *,
+					struct address_space *, CORE_ADDR);
 int arm_software_single_step (struct frame_info *);
 int arm_frame_is_thumb (struct frame_info *frame);
 
--- /dev/null	2011-03-24 13:56:45.925176537 +0100
+++ gdb/testsuite/gdb.arch/thumb-singlestep.S	2011-03-24 19:13:29.000000000 +0100
@@ -0,0 +1,40 @@
+/* Test program with deliberately incorrect execution mode transition
+
+   Copyright 2011 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/>.  */
+
+	.text
+	.align 2
+	.global foo
+	.thumb
+	/* .thumb_func deliberately omitted */
+foo:
+	mov r0,#42
+	bx lr
+
+        .text
+	.align  2
+	.global main
+	.thumb
+	.thumb_func
+	.type	main, %function
+main:
+        push	{r3, lr}
+	blx	foo
+        pop	{r3, pc}
+	.size	main, .-main
+
--- /dev/null	2011-03-24 13:56:45.925176537 +0100
+++ gdb/testsuite/gdb.arch/thumb-singlestep.exp	2011-03-24 19:13:29.000000000 +0100
@@ -0,0 +1,43 @@
+# Copyright 2011 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/>.
+
+# Test single-stepping into incorrectly marked Thumb routine
+
+if {![istarget arm*-*]} then {
+    verbose "Skipping ARM tests."
+    return
+}
+
+set testfile "thumb-singlestep"
+set srcfile ${testfile}.S
+set binfile ${objdir}/${subdir}/${testfile}
+
+set additional_flags "additional_flags=-mthumb"
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug $additional_flags]] != "" } {
+    untested thumb-singlestep.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    gdb_suppress_tests
+}
+
+gdb_test "si" "foo \\(\\) at .*${srcfile}.*mov r0,#42.*" "step into foo"
+
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com