remote.c Z0 packet detection robustness

remote.c Z0 packet detection robustness

[Jonathan Larmour]

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

Hi,

The attached patch improves the robustness of Z0 packet detection. In some
rare cases on some targets, the remote_insert_breakpoint behaves
differently for the first breakpoint attempted, as a result of the Z0
packet support detection. The problem is that GDB modifies the address
information used as part of the detection, by passing in the address to use
to gdbarch_breakpoint_from_pc, which on some target vectors (in my case
MIPS16, although it's also conceivable for Thumb to be affected) modifies
the address.

If the Z0 packet is unknown, it drops through to memory_insert_breakpoint
passing it the modified address. This can cause a change in later
heuristics within the target support when the target tries to detect the
form of breakpoint required (e.g. 2 versus 4 byte).

You can see this in the implementation of e.g. mips_breakpoint_from_pc,
which will look at the PC first, to determine the breakpoint style to use,
not the length. Most of the times this works, but in some particular cases,
depending on particular characteristics of the symbol, it can fail to
detect mips16 addresses. Whereas it could detect them if the address had
been unmodified.

I have check-in permissions, so can commit after approval if that helps.

Jifl

2008-06-27  Jonathan Larmour  <jifl@eCosCentric.com>

	* remote.c (remote_insert_breakpoint): Ensure that if Z0
	unsupported and we fall back to memory_insert_breakpoint, we
	use the unmodified requested address.


-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

[-- Attachment #2: remotebp.z0.detect.tweak.patch --]
[-- Type: text/x-patch, Size: 1025 bytes --]

--- gdb/remote.c~	2008-06-11 13:56:36.000000000 +0100
+++ gdb/remote.c	2008-06-20 19:56:54.000000000 +0100
@@ -5250,12 +5250,13 @@ remote_insert_breakpoint (struct bp_targ
 
   if (remote_protocol_packets[PACKET_Z0].support != PACKET_DISABLE)
     {
-      CORE_ADDR addr;
+      CORE_ADDR addr = bp_tgt->placed_address;
       struct remote_state *rs;
       char *p;
+      int bpsize;
 
       gdbarch_breakpoint_from_pc
-	(current_gdbarch, &bp_tgt->placed_address, &bp_tgt->placed_size);
+	(current_gdbarch, &addr, &bpsize);
 
       rs = get_remote_state ();
       p = rs->buf;
@@ -5263,9 +5264,9 @@ remote_insert_breakpoint (struct bp_targ
       *(p++) = 'Z';
       *(p++) = '0';
       *(p++) = ',';
-      addr = (ULONGEST) remote_address_masked (bp_tgt->placed_address);
+      addr = (ULONGEST) remote_address_masked (addr);
       p += hexnumstr (p, addr);
-      sprintf (p, ",%d", bp_tgt->placed_size);
+      sprintf (p, ",%d", bpsize);
 
       putpkt (rs->buf);
       getpkt (&rs->buf, &rs->buf_size, 0);

Re: remote.c Z0 packet detection robustness

[Daniel Jacobowitz]

On Fri, Jun 27, 2008 at 04:55:37PM +0100, Jonathan Larmour wrote:
> 2008-06-27  Jonathan Larmour  <jifl@eCosCentric.com>
> 
> 	* remote.c (remote_insert_breakpoint): Ensure that if Z0
> 	unsupported and we fall back to memory_insert_breakpoint, we
> 	use the unmodified requested address.

Thanks for the clear explanation.  This is almost OK - but you need to
update bp_tgt if Z0 succeeds.  Otherwise we may remove the breakpoint
incorrectly.


-- 
Daniel Jacobowitz
CodeSourcery

Re: remote.c Z0 packet detection robustness

[Jonathan Larmour]

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

Daniel Jacobowitz wrote:
> On Fri, Jun 27, 2008 at 04:55:37PM +0100, Jonathan Larmour wrote:
>> 2008-06-27  Jonathan Larmour  <jifl@eCosCentric.com>
>>
>> 	* remote.c (remote_insert_breakpoint): Ensure that if Z0
>> 	unsupported and we fall back to memory_insert_breakpoint, we
>> 	use the unmodified requested address.
> 
> Thanks for the clear explanation.  This is almost OK - but you need to
> update bp_tgt if Z0 succeeds.  Otherwise we may remove the breakpoint
> incorrectly.

Is this better?

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

[-- Attachment #2: remote.c.v2.patch --]
[-- Type: text/x-patch, Size: 1302 bytes --]

--- gdb-6.8.50.20080620/gdb/remote.c.old	2008-06-11 13:56:36.000000000 +0100
+++ gdb-6.8.50.20080620/gdb/remote.c	2008-06-27 18:26:41.000000000 +0100
@@ -5250,12 +5250,13 @@ remote_insert_breakpoint (struct bp_targ
 
   if (remote_protocol_packets[PACKET_Z0].support != PACKET_DISABLE)
     {
-      CORE_ADDR addr;
+      CORE_ADDR addr = bp_tgt->placed_address;
       struct remote_state *rs;
       char *p;
+      int bpsize;
 
       gdbarch_breakpoint_from_pc
-	(current_gdbarch, &bp_tgt->placed_address, &bp_tgt->placed_size);
+	(current_gdbarch, &addr, &bpsize);
 
       rs = get_remote_state ();
       p = rs->buf;
@@ -5263,9 +5264,9 @@ remote_insert_breakpoint (struct bp_targ
       *(p++) = 'Z';
       *(p++) = '0';
       *(p++) = ',';
-      addr = (ULONGEST) remote_address_masked (bp_tgt->placed_address);
+      addr = (ULONGEST) remote_address_masked (addr);
       p += hexnumstr (p, addr);
-      sprintf (p, ",%d", bp_tgt->placed_size);
+      sprintf (p, ",%d", bpsize);
 
       putpkt (rs->buf);
       getpkt (&rs->buf, &rs->buf_size, 0);
@@ -5275,6 +5276,8 @@ remote_insert_breakpoint (struct bp_targ
 	case PACKET_ERROR:
 	  return -1;
 	case PACKET_OK:
+	  bp_tgt->placed_address = addr;
+	  bp_tgt->placed_size = bpsize;
 	  return 0;
 	case PACKET_UNKNOWN:
 	  break;

Re: remote.c Z0 packet detection robustness

[Daniel Jacobowitz]

On Fri, Jun 27, 2008 at 07:17:25PM +0100, Jonathan Larmour wrote:
> Daniel Jacobowitz wrote:
> > On Fri, Jun 27, 2008 at 04:55:37PM +0100, Jonathan Larmour wrote:
> >> 2008-06-27  Jonathan Larmour  <jifl@eCosCentric.com>
> >>
> >> 	* remote.c (remote_insert_breakpoint): Ensure that if Z0
> >> 	unsupported and we fall back to memory_insert_breakpoint, we
> >> 	use the unmodified requested address.
> > 
> > Thanks for the clear explanation.  This is almost OK - but you need to
> > update bp_tgt if Z0 succeeds.  Otherwise we may remove the breakpoint
> > incorrectly.
> 
> Is this better?

Yes, this version is fine to commit.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery