Re: [rfc] Strip Thumb bit from PC returned by arm_get_longjmp_target

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

Matthew Gretton-Dann wrote:

> The problem with the patch is it removes what may be the only way we
> have of telling the instruction state of the longjmp target.  If you
> have debugging information (mapping symbols at the very least)
> everything is fine, but if you don't then how do you tell what the state
> is?  arm_pc_is_thumb does use this bit to detect the instruction state
> (and arm_breakpoint_from_pc then uses this result to determine the
> breakpoint type).

Ah, I see.  I was confused about just where the Thumb bit was supposed
to be present and where not, sorry ...

> In the case above I think the correct fix is to make
> arm_adjust_breakpoint_address not strip out the address bits (which it
> does when trying to work out whether we are single stepping through an
> IT block).

Does the patch below seem reasonable to you?

> However, I think there is a more fundamental issue to be solved in the
> ARM backend.  For ARM (and possibly other architectures) you do not have
> enough information in the PC to be able to accurately set a breakpoint,
> you also need to know the instruction set in use at that address.
> 
> Currently this is being done in a fairly ad hoc and inconsistent fashion
> by using bit-0 to indicate Thumb or ARM state.  But this troublesome to
> maintain, and can make the debugger behave in a non-obvious manner to
> the user.  I think it would be cleaner to change the interfaces to
> various functions (for example breakpoint_from_pc) to take an
> 'instruction set state' parameter as well as the pc (or change the pc
> from a CORE_ADDR to a { CORE_ADDR pc; enum instruction_set isa; } pair),
> but this would be a large scale change with impacts throughout the code
> base - which probably makes it impractical, and certainly in need of
> discussion.

Good point.  I think there are two potential longer-term options.  On
the one hand, we've been planning to add support for multiple address
spaces within a single process; this would imply that all gdbarch
callbacks that get an address today also receive an address space
identifier.  If we have that, we could create two address spaces to 
represent the Thumb vs. ARM instruction space ...

Another option would be to actually have two separate *gdbarch*
implementations for Thumb vs. ARM, and use the multi-architecture
support to choose the proper architecture to use.  For frames,
this would already be possible today using the frame-arch unwinder
I've added for Cell support.  There is even a breakpoint-location
architecture, but no way yet for the back-end to influence how
this is selected ... that can be added, though.  (There may be
other places where support is currently insufficient, of course.)

In fact, the latter option seems nice anyway since in several gdbarch
callback we already basically have a big "if ARM vs. Thumb" switching
between two quite different implementations.  Just having two different
gdbarch's in the first place might actually simplify this ...

Bye,
Ulrich


ChangeLog:

	* arm-tdep.c (arm_adjust_breakpoint_address): Do not strip out
	Thumb bit when returning incoming address unchanged.  Set the
	Thumb bit when actually adjusting address.

Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.304
diff -u -p -r1.304 arm-tdep.c
--- gdb/arm-tdep.c	27 May 2010 19:06:12 -0000	1.304
+++ gdb/arm-tdep.c	18 Aug 2010 17:55:54 -0000
@@ -3346,6 +3346,7 @@ arm_adjust_breakpoint_address (struct gd
   int buf_len, buf2_len;
   enum bfd_endian order = gdbarch_byte_order_for_code (gdbarch);
   int i, any, last_it, last_it_count;
+  CORE_ADDR orig_bpaddr;
 
   /* If we are using BKPT breakpoints, none of this is necessary.  */
   if (gdbarch_tdep (gdbarch)->thumb2_breakpoint == NULL)
@@ -3364,7 +3365,14 @@ arm_adjust_breakpoint_address (struct gd
     /* Thumb-2 code must have mapping symbols to have a chance.  */
     return bpaddr;
 
-  bpaddr = gdbarch_addr_bits_remove (gdbarch, bpaddr);
+  /* From here on, we always should return addresses with the Thumb
+     bit set.  However, the incoming address may not consistently
+     have that bit set -- if it is clear, and we set the bit when
+     returning the original address otherwise unchanged, common code
+     would erroneously assume we adjusted the breakpoint.  Just return
+     the original address as-is in those cases.  */
+  orig_bpaddr = bpaddr;
+  bpaddr = gdbarch_addr_bits_remove (gdbarch, orig_bpaddr);
 
   if (find_pc_partial_function (bpaddr, NULL, &func_start, NULL)
       && func_start > boundary)
@@ -3377,11 +3385,11 @@ arm_adjust_breakpoint_address (struct gd
   buf_len = min (bpaddr - boundary, MAX_IT_BLOCK_PREFIX);
   if (buf_len == 0)
     /* No room for an IT instruction.  */
-    return bpaddr;
+    return orig_bpaddr;
 
   buf = xmalloc (buf_len);
   if (target_read_memory (bpaddr - buf_len, buf, buf_len) != 0)
-    return bpaddr;
+    return orig_bpaddr;
   any = 0;
   for (i = 0; i < buf_len; i += 2)
     {
@@ -3395,7 +3403,7 @@ arm_adjust_breakpoint_address (struct gd
   if (any == 0)
     {
       xfree (buf);
-      return bpaddr;
+      return orig_bpaddr;
     }
 
   /* OK, the code bytes before this instruction contain at least one
@@ -3437,7 +3445,7 @@ arm_adjust_breakpoint_address (struct gd
 	{
 	  buf = extend_buffer_earlier (buf, bpaddr, buf_len, bpaddr - boundary);
 	  if (buf == NULL)
-	    return bpaddr;
+	    return orig_bpaddr;
 	  buf_len = bpaddr - boundary;
 	  i = 0;
 	}
@@ -3446,7 +3454,7 @@ arm_adjust_breakpoint_address (struct gd
     {
       buf = extend_buffer_earlier (buf, bpaddr, buf_len, bpaddr - boundary);
       if (buf == NULL)
-	return bpaddr;
+	return orig_bpaddr;
       buf_len = bpaddr - boundary;
       i = 0;
     }
@@ -3477,15 +3485,15 @@ arm_adjust_breakpoint_address (struct gd
 
   if (last_it == -1)
     /* There wasn't really an IT instruction after all.  */
-    return bpaddr;
+    return orig_bpaddr;
 
   if (last_it_count < 1)
     /* It was too far away.  */
-    return bpaddr;
+    return orig_bpaddr;
 
   /* This really is a trouble spot.  Move the breakpoint to the IT
      instruction.  */
-  return bpaddr - buf_len + last_it;
+  return MAKE_THUMB_ADDR (bpaddr - buf_len + last_it);
 }
 
 /* ARM displaced stepping support.


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com