Possible remote.c patch for Z-packet breakpoints + Harvard + SID

Possible remote.c patch for Z-packet breakpoints + Harvard + SID

[Nick Duffek]

Executive summary for the lists I've added to this thread: Z-packet
breakpoints don't work with SID on Harvard architectures.

On 16-Jul-2001, I wrote:

>When a Harvard-architecture GDB sends an address to SID, the address has a
>high bit set indicating whether the address space, e.g. instruction
>vs. data.
>
>However, gdb::add_hw_breakpoint() in sid/component/gdb/gdb.cxx doesn't
>trim the high bit before setting a watchpoint at that address.
>Consequently, hardware breakpoints don't work with Harvard architectures.
>
>It'd be possible to trim the bit in GDB, but I think it's better to fix
>the problem in SID and avoid special-casing hardware breakpoints in the
>GDB <-> SID protocol.
>
>Perhaps a new sid::bus method could be introduced to translate addresses
>without writing or reading memory, and {add,remove}_hw_breakpoint() could
>call that.

On 16-Jul-2001, Frank Ch . Eigler replied:

>: When a Harvard-architecture GDB sends an address to SID, the address has a
>: high bit set indicating whether the address space, e.g. instruction
>: vs. data.
>
>Yes, it should do so, when dealing with memory-related addresses.
>
>: However, gdb::add_hw_breakpoint() in sid/component/gdb/gdb.cxx doesn't
>: trim the high bit before setting a watchpoint at that address. [...]
>
>That's a separate matter - a hardware breakpoint relates to the PC
>register value, which does not contain the address-space tag byte.
>
>: It'd be possible to trim the bit in GDB [...]
>
>Thanks for the other ideas, but methinks GDB is the right place to do
>this.  GDB already knows how to map PC register values to/from memory
>addresses; it needs to do the opposite when requesting hardware
>breakpoints.

I see your point, though I think it's debatable.  For one thing, Z-packet
breakpoints are tagged as "Z_PACKET_SOFTWARE_BP" in gdb/remote.c, so it
looks like they're intended to be a generic protocol speedup rather than a
hardware breakpoint mechanism.

As far as I know, gdb/remote.c has never translated Z-packet breakpoints
as described above, so I doubt we can do ADDRESS_TO_POINTER()
unconditionally on those breakpoints without breaking other targets.

I proposed a patch in January to call BREAKPOINT_FROM_PC() before rather
than after remote.c stores hardware breakpoint addresses in outgoing
packets, but it was decided that such a change should be at a higher
level, e.g. in target_insert_breakpoint().

This problem can't be fixed in target_insert_breakpoint(), because the
distinction between "hardware" (Z-packet) and software breakpoints is made
in remote.c.

So, here's a hook to add to gdb/remote.c.  Comments?

Nick

ChangeLog:

	* gdbarch.sh (ADJUST_REMOTE_Z_BREAKPOINT): New function.
	* remote.c (remote_insert_breakpoint, remote_remove_breakpoint):
	Call ADJUST_REMOTE_Z_BREAKPOINT().

Index: gdb/gdbarch.sh
===================================================================
diff -up gdb/gdbarch.sh gdb/gdbarch.sh
--- gdb/gdbarch.sh	Mon Jul 16 15:25:50 2001
+++ gdb/gdbarch.sh	Mon Jul 16 15:25:42 2001
@@ -488,6 +488,8 @@ f:2:SKIP_PROLOGUE:CORE_ADDR:skip_prologu
 f:2:PROLOGUE_FRAMELESS_P:int:prologue_frameless_p:CORE_ADDR ip:ip::0:generic_prologue_frameless_p::0
 f:2:INNER_THAN:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs::0:0
 f:2:BREAKPOINT_FROM_PC:unsigned char *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr:::legacy_breakpoint_from_pc::0
+# Update remote Z-packet breakpoint ADDR before sending it to the target.
+F:2:ADJUST_REMOTE_Z_BREAKPOINT:void:adjust_remote_z_breakpoint:CORE_ADDR addr:addr::0
 f:2:MEMORY_INSERT_BREAKPOINT:int:memory_insert_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_insert_breakpoint::0
 f:2:MEMORY_REMOVE_BREAKPOINT:int:memory_remove_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_remove_breakpoint::0
 v:2:DECR_PC_AFTER_BREAK:CORE_ADDR:decr_pc_after_break::::0:-1
Index: gdb/remote.c
===================================================================
diff -up gdb/remote.c gdb/remote.c
--- gdb/remote.c	Mon Jul 16 15:25:59 2001
+++ gdb/remote.c	Mon Jul 16 15:25:42 2001
@@ -4513,13 +4513,17 @@ remote_insert_breakpoint (CORE_ADDR addr
     {
       char *buf = alloca (PBUFSIZ);
       char *p = buf;
+      CORE_ADDR addr2;
       
-      addr = remote_address_masked (addr);
+      addr2 = remote_address_masked (addr);
+      if (ADJUST_REMOTE_Z_BREAKPOINT_P ())
+	ADJUST_REMOTE_Z_BREAKPOINT (&addr2);
+
       *(p++) = 'Z';
       *(p++) = '0';
       *(p++) = ',';
-      p += hexnumstr (p, (ULONGEST) addr);
-      BREAKPOINT_FROM_PC (&addr, &bp_size);
+      p += hexnumstr (p, (ULONGEST) addr2);
+      BREAKPOINT_FROM_PC (&addr2, &bp_size);
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);
@@ -4564,14 +4568,18 @@ remote_remove_breakpoint (CORE_ADDR addr
     {
       char *buf = alloca (PBUFSIZ);
       char *p = buf;
+      CORE_ADDR addr2;
       
       *(p++) = 'z';
       *(p++) = '0';
       *(p++) = ',';
 
-      addr = remote_address_masked (addr);
-      p += hexnumstr (p, (ULONGEST) addr);
-      BREAKPOINT_FROM_PC (&addr, &bp_size);
+      addr2 = remote_address_masked (addr);
+      if (ADJUST_REMOTE_Z_BREAKPOINT_P ())
+	ADJUST_REMOTE_Z_BREAKPOINT (&addr2);
+
+      p += hexnumstr (p, (ULONGEST) addr2);
+      BREAKPOINT_FROM_PC (&addr2, &bp_size);
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);

Re: Possible remote.c patch for Z-packet breakpoints + Harvard + SID

[Frank Ch. Eigler]

nsd wrote:

: [...]
: I see your point, though I think it's debatable.  For one thing, Z-packet
: breakpoints are tagged as "Z_PACKET_SOFTWARE_BP" in gdb/remote.c, so it
: looks like they're intended to be a generic protocol speedup rather than a
: hardware breakpoint mechanism.
: [...]

That's true - it makes me rethink my earlier point.

: I proposed a patch in January to call BREAKPOINT_FROM_PC() before rather
: than after remote.c stores hardware breakpoint addresses in outgoing
: packets, but it was decided that such a change should be at a higher
: level, e.g. in target_insert_breakpoint().
: 
: This problem can't be fixed in target_insert_breakpoint(), because the
: distinction between "hardware" (Z-packet) and software breakpoints is made
: in remote.c.
: [...]

If this gdb-side approach is not deemed acceptable to gdb folks, we
may be able to make complementary changes on the sid side without too
much littering.  

One possibility: map hardware breakpoints to mask/value-type sid
"triggerpoints" on the PC register instead of plain value-type ones;
parametrize the gdb-interface-component with the desired mask for a
target.  This sort of thing may be useful for PC-range breakpoints
too.


- FChE

Re: Possible remote.c patch for Z-packet breakpoints + Harvard + SID

[Mark Salter]

>>>>> Nick Duffek writes:

> Executive summary for the lists I've added to this thread: Z-packet
> breakpoints don't work with SID on Harvard architectures.

> On 16-Jul-2001, I wrote:

>> When a Harvard-architecture GDB sends an address to SID, the address has a
>> high bit set indicating whether the address space, e.g. instruction
>> vs. data.

FWIW, I recently had to deal with this in RedBoot. Since the RedBoot stub
already had to deal with differentiating code and data addresses, I just
used the same mechanism to convert from a "gdb" address to a hw code or
data address when decoding the Z packet before the __set_breakpoint() is
called.

--Mark