[rfc] Strip Thumb bit from PC returned by arm_get_longjmp_target

[rfc] Strip Thumb bit from PC returned by arm_get_longjmp_target

[Ulrich Weigand]

Hello,

when stepping over code performing a longjmp on Thumb code,
I'm seeing warning messages along the lines of:
warning: Breakpoint address adjusted from 0x4002d29d to 0x4002d29c.

(This happens e.g. in gdb.threads/threxit-hop-specific.exp.)

This is caused by arm_get_longjmp_target returning a value with
the Thumb bit set, which the rest of the code doesn't expect.

Fixed by calling arm_addr_bits_remove on the PC value returned
from arm_get_longjmp_target.

Tested on armv7l-linux-gnueabi with no regressions.

Any comments?  I'm planning on committing this within a couple
of days.

Bye,
Ulrich


ChangeLog:

	* arm-tdep.c (arm_get_longjmp_target): Strip extra bits from
	returned PC value.


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	16 Aug 2010 19:05:36 -0000
@@ -5852,6 +5852,7 @@ arm_get_longjmp_target (struct frame_inf
     return 0;
 
   *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
+  *pc = arm_addr_bits_remove (gdbarch, *pc);
   return 1;
 }
 
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Matthew Gretton-Dann]

On Tue, 2010-08-17 at 21:15 +0200, Ulrich Weigand wrote:
> Hello,
> 
> when stepping over code performing a longjmp on Thumb code,
> I'm seeing warning messages along the lines of:
> warning: Breakpoint address adjusted from 0x4002d29d to 0x4002d29c.
> 
> (This happens e.g. in gdb.threads/threxit-hop-specific.exp.)
> 
> This is caused by arm_get_longjmp_target returning a value with
> the Thumb bit set, which the rest of the code doesn't expect.
> 
> Fixed by calling arm_addr_bits_remove on the PC value returned
> from arm_get_longjmp_target.
> 
> Tested on armv7l-linux-gnueabi with no regressions.
> 
> Any comments?  I'm planning on committing this within a couple
> of days.

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).

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).

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.

Any thoughts?

Thanks,

Matt

-- 
Matthew Gretton-Dann
Principal Engineer - PDSW Tools
ARM Ltd

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

[Ulrich Weigand]

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

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

[Ulrich Weigand]

> 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?

Actually, it turns out this new patch doesn't work.  It leads to:

Breakpoint 4 at 0x84ec: file ../../../gdb-head/gdb/testsuite/gdb.threads/threxit-hop-specific.c, line 47.^M
(gdb) next^M
^M
Program received signal SIGTRAP, Trace/breakpoint trap.^M

This happens because GDB now no longer recognizes the PC address
when the breakpoint trap arrives.  The PC is compared against
the breakpoint location's loc->address value -- which now has the
Thumb bit set, but the PC doesn't.

Note that while the Thumb bit gets removes in arm_breakpoint_from_pc,
this affects only loc->placed_address, not loc->address.

This seems to indicate that in fact, breakpoint addresses must
*not* have the Thumb bit set ...

Do you have a case where this works for you?

Bye,
Ulrich

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

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

[Matthew Gretton-Dann]

On Thu, 2010-08-19 at 10:49 +0200, Ulrich Weigand wrote:
> > 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?
> 
> Actually, it turns out this new patch doesn't work.  It leads to:
> 
> Breakpoint 4 at 0x84ec: file ../../../gdb-head/gdb/testsuite/gdb.threads/threxit-hop-specific.c, line 47.^M
> (gdb) next^M
> ^M
> Program received signal SIGTRAP, Trace/breakpoint trap.^M
> 
> This happens because GDB now no longer recognizes the PC address
> when the breakpoint trap arrives.  The PC is compared against
> the breakpoint location's loc->address value -- which now has the
> Thumb bit set, but the PC doesn't.
> 
> Note that while the Thumb bit gets removes in arm_breakpoint_from_pc,
> this affects only loc->placed_address, not loc->address.
> 
> This seems to indicate that in fact, breakpoint addresses must
> *not* have the Thumb bit set ...
> 
> Do you have a case where this works for you?

I don't have a case.  I think this means that the original patch is the
correct one for the moment - but I'm not a maintainer so this isn't an
approval.  

Also, looking at the code in arm_adjust_breakpoint_address I think there
are other cases that may cause the failure you are seeing above (for
instance when using Thumb-2 in the absence of mapping symbols and
setting the breakpoint on 0x8001).

We still need to come up with a way to be able to better pass the
instruction set state around with the PC (I'll respond to your earlier
email with some of my thoughts).

Thanks,

Matt

-- 
Matthew Gretton-Dann
Principal Engineer - PDSW Tools
ARM Ltd

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

[Matthew Gretton-Dann]

On Wed, 2010-08-18 at 20:28 +0200, Ulrich Weigand wrote:
> Matthew Gretton-Dann wrote:
> > 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 ...

Using two gdbarch objects seems reasonable, and also extends nicely if
we decide to support the ThumbEE ISA as well (which is intended for JITs
so gdb is unlikely to have any debug info to help it along).

However, I am concerned about a couple of things:

1) What changes does this make to the user interface?  Can I user still
say 'break main' and gdb will determine ARM or Thumb automatically?
Will the user still be able to see a backtrace with some frames in ARM
state and some in Thumb?

2) This moves the decision what instruction set state a breakpoint is
targetting from the time we set the breakpoint to the time the
breakpoint is requested by the user.  Is this a reasonable thing to do?
I think so - I don't expect code to change under the debugger's feet, so
making the decision when we have most information available (at
breakpoint request time) is the correct way to go.

Thanks,

Matt


-- 
Matthew Gretton-Dann
Principal Engineer - PDSW Tools
ARM Ltd

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

[Ulrich Weigand]

Matthew Gretton-Dann wrote:

> Using two gdbarch objects seems reasonable, and also extends nicely if
> we decide to support the ThumbEE ISA as well (which is intended for JITs
> so gdb is unlikely to have any debug info to help it along).
> 
> However, I am concerned about a couple of things:
> 
> 1) What changes does this make to the user interface?  Can I user still
> say 'break main' and gdb will determine ARM or Thumb automatically?

That's the one piece I mentioned where common code support will have to
be added.  Basically, we'd need a gdbarch callback that gets the gdbarch
the breakpoint was parsed in, and the breakpoint's PC address (plus
address space, eventually), and would return the gdbarch to be used to
handle that particular breakpoint location.

> Will the user still be able to see a backtrace with some frames in ARM
> state and some in Thumb?

This mechanism already exists and is used for Cell debugging today: we
can install a frame architecture unwinder that, given a frame, returns
the architecture to be used for the next-outer frame.

> 2) This moves the decision what instruction set state a breakpoint is
> targetting from the time we set the breakpoint to the time the
> breakpoint is requested by the user.  Is this a reasonable thing to do?
> I think so - I don't expect code to change under the debugger's feet, so
> making the decision when we have most information available (at
> breakpoint request time) is the correct way to go.

Actually, it would move the decision to the point where the breakpoint
location structure is allocated.  This happens when the breakpoint is
initially requested, yes, but breakpoint locations are also recomputed
every time the available debug information changes (e.g. shared libraries
are loaded/unloaded).

Bye,
Ulrich

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

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

[Ulrich Weigand]

Matthew Gretton-Dann wrote:
> On Thu, 2010-08-19 at 10:49 +0200, Ulrich Weigand wrote:
> > Actually, it turns out this new patch doesn't work.  It leads to:
> > 
> > Breakpoint 4 at 0x84ec: file ../../../gdb-head/gdb/testsuite/gdb.threads/threxit-hop-specific.c, line 47.^M
> > (gdb) next^M
> > ^M
> > Program received signal SIGTRAP, Trace/breakpoint trap.^M
> > 
> > This happens because GDB now no longer recognizes the PC address
> > when the breakpoint trap arrives.  The PC is compared against
> > the breakpoint location's loc->address value -- which now has the
> > Thumb bit set, but the PC doesn't.
> > 
> > Note that while the Thumb bit gets removes in arm_breakpoint_from_pc,
> > this affects only loc->placed_address, not loc->address.
> > 
> > This seems to indicate that in fact, breakpoint addresses must
> > *not* have the Thumb bit set ...
> > 
> > Do you have a case where this works for you?
> 
> I don't have a case.  I think this means that the original patch is the
> correct one for the moment - but I'm not a maintainer so this isn't an
> approval.  

OK, thanks for your feedback.  I'm happy to wait for Richard's approval ...

> Also, looking at the code in arm_adjust_breakpoint_address I think there
> are other cases that may cause the failure you are seeing above (for
> instance when using Thumb-2 in the absence of mapping symbols and
> setting the breakpoint on 0x8001).

I'm now wondering whether it can in fact ever happen that an address with
the Thumb bit set can come into arm_adjust_breakpoint_address; it seems
that (except for the longjmp target case) no such values are ever exposed.
Am I missing a possible path here?

Bye,
Ulrich

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