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

[Andrew Cagney]

Nick,

Have you looked at ``TRANSLATE_XFER_ADDRESS''?  The d10v uses it to 
translate between a virtual CORE_ADDR and a real CORE_ADDR.

However, as a general rule, I think GDB should be consistent and always 
send down down CORE_ADDR's.  Remember the target needs to know about 
CORE_ADDRs anyway.  They are exactly the same as what BFD uses for 
bfd_vma's when specifying things like section addresses.

	Andrew

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

[Nick Duffek]

On 17-Jul-2001, Andrew Cagney wrote:

>Have you looked at ``TRANSLATE_XFER_ADDRESS''?  The d10v uses it to translate
>between a virtual CORE_ADDR and a real CORE_ADDR.

I think that won't work, because only Z-packet breakpoint addresses need
to be translated to real addresses before being sent to SID.
TRANSLATE_XFER_ADDRESS translates memory read/write addresses, which must
not be translated before being sent to SID for the architecture in
question.

>However, as a general rule, I think GDB should be consistent and always send
>down down CORE_ADDR's.

Meaning virtual CORE_ADDRs?  I agree.

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

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

Do we have a consensus that a SID-side approach is preferable to a
GDB-side one?

Nick

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

[Andrew Cagney]

>>However, as a general rule, I think GDB should be consistent and always send
>>down down CORE_ADDR's.
> 
> 
> Meaning virtual CORE_ADDRs?  I agree.


Yes.


> On 16-Jul-2001, Frank Ch . Eigler wrote:
> 
> 
>>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.  
> 
> 
> Do we have a consensus that a SID-side approach is preferable to a
> GDB-side one?


Looks like it (no one else has spoken up).

It means that GDB sends the remote target a CORE_ADDR and that the 
remote target needs to provide a mechanism for mapping between the 
hardware registers and a CORE_ADDR address.  Not unlike 
gdbarch->{read,write}_pc().

	Andrew