[RFA/doc] Redefine the length argument in remote Z packets

[Daniel Jacobowitz <dan at codesourcery dot com>]

We currently have five kinds of "Z" packets, for software/hardware
breakpoints and write/read/access watchpoints.  They all take an
address and a length.  For watchpoints, the length is obvious: how
large an area to watch.  For breakpoints, it's a convenient shortcut
that I'm running into trouble with.

The reason for the length argument on breakpoints, particularly
software breakpoints, is that some platforms have more than one
breakpoint instruction.  For example, on ARM targets, you need
to use an ARM mode breakpoint if you're replacing an ARM instruction
and a Thumb mode breakpoint if you're replacing a Thumb instruction.
Conveniently, on both ARM and MIPS the compact ISA has smaller
breakpoints than the normal ISA.  So we use Z0,ADDR,4 for one
and Z0,ADDR,2 for the other.

On recent ARM processors there's an extended version of Thumb mode,
called Thumb-2.  This has both 16-bit and 32-bit instructions.  In
most situations we can replace the first half of a 32-bit instruction
with the 16-bit breakpoint, and everything will work out fine - that
is what we also do on i386.  But there's one corner case, which
I'll go into more in the next patch, where we need to replace 32-bit
Thumb instructions with a 32-bit Thumb breakpoint.

Unfortunately, it doesn't *quite* line up with the ARM mode
breakpoints; just a few bits off.  So we need a third kind.

This new breakpoint has an actual length of four bytes.  So
gdbarch_breakpoint_from_pc correctly reports that it is a four byte
breakpoint.  For native debugging, this is as far as we have to go;
everything will work.  But for remote targets this will cause us
to insert the wrong breakpoint instruction.  We can't use Z0,ADDR,4
for this breakpoint.

I had two options for the new breakpoint.  We could either send
Z0,ADDR,2 and have the target figure out whether to use the 16-bit or
32-bit breakpoint, or we could send Z0,ADDR,3 [number chosen
arbitrarily] and have the target decide based on that to use a
32-bit breakpoint.

Both are feasible; it's easy to tell if a Thumb instruction is the
first half of a 32-bit sequence.  I opted for the new number as
a safeguard.  The failure if a 16-bit breakpoint is used will be
quite hard to identify, and could silently corrupt program execution
in some cases.  So let's make sure stubs that don't implement the
right breakpoint fail.

This is mostly academic.  It only applies to Linux, and similar OS
environments; bare metal can use 16-bit breakpoints all the time.  And
gdbserver doesn't support Z0 yet.  I'm only fixing this thoroughly
because I really don't want it to come back to haunt me.

Eli, could you review the documentation patch?  I rearranged a little;
this isn't the only target-specific knob in the remote protocol, so
I made a place to further document such knobs.

Tested on arm-none-linux-gnueabi.

-- 
Daniel Jacobowitz
CodeSourcery

2010-01-28  Daniel Jacobowitz  <dan@codesourcery.com>

	gdb/
	* arch-utils.c (default_remote_breakpoint_from_pc): New function.
	* arch-utils.h (default_remote_breakpoint_from_pc): Declare.
	* gdbarch.c, gdbarch.h: Regenerated.
	* gdbarch.sh (remote_breakpoint_from_pc): New architecture method.
	* remote.c (remote_insert_breakpoint, remote_insert_hw_breakpoint): Use
	gdbarch_remote_breakpoint_from_pc.

	gdb/doc/
	* gdb.texinfo (Architecture-Specific Protocol Details): New section.
	Document ARM breakpoint types.
	(Register Packet Format): Move into the new section.
	(Packets): Describe the KIND argument for Z0, z0, Z1, and z1 packets.

---
 gdb/arch-utils.c    |    7 ++++++
 gdb/arch-utils.h    |    3 ++
 gdb/doc/gdb.texinfo |   60 +++++++++++++++++++++++++++++++++++++++-------------
 gdb/gdbarch.c       |   25 +++++++++++++++++++++
 gdb/gdbarch.h       |    8 ++++++
 gdb/gdbarch.sh      |    4 +++
 gdb/remote.c        |    4 +--
 7 files changed, 95 insertions(+), 16 deletions(-)

Index: gdb-mainline/gdb/arch-utils.c
===================================================================
--- gdb-mainline.orig/gdb/arch-utils.c	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/arch-utils.c	2010-01-11 15:04:43.000000000 -0800
@@ -776,6 +776,13 @@ default_fast_tracepoint_valid_at (struct
   return 1;
 }
 
+void
+default_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
+				   int *kindptr)
+{
+  gdbarch_breakpoint_from_pc (gdbarch, pcptr, kindptr);
+}
+
 /* */
 
 extern initialize_file_ftype _initialize_gdbarch_utils; /* -Wmissing-prototypes */
Index: gdb-mainline/gdb/arch-utils.h
===================================================================
--- gdb-mainline.orig/gdb/arch-utils.h	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/arch-utils.h	2010-01-11 15:04:43.000000000 -0800
@@ -159,4 +159,7 @@ extern int default_fast_tracepoint_valid
 					     CORE_ADDR addr,
 					     int *isize, char **msg);
 
+extern void default_remote_breakpoint_from_pc (struct gdbarch *,
+					       CORE_ADDR *pcptr, int *kindptr);
+
 #endif
Index: gdb-mainline/gdb/doc/gdb.texinfo
===================================================================
--- gdb-mainline.orig/gdb/doc/gdb.texinfo	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/doc/gdb.texinfo	2010-01-11 15:04:43.000000000 -0800
@@ -28114,7 +28114,7 @@ Show the current setting of the target w
 * Packets::
 * Stop Reply Packets::
 * General Query Packets::
-* Register Packet Format::
+* Architecture-Specific Protocol Details::
 * Tracepoint Packets::
 * Host I/O Packets::
 * Interrupts::
@@ -28867,7 +28867,8 @@ for an error
 @cindex @samp{Z} packets
 Insert (@samp{Z}) or remove (@samp{z}) a @var{type} breakpoint or
 watchpoint starting at address @var{address} and covering the next
-@var{length} bytes.
+@var{length} bytes.  @var{length} may also be a @var{kind} value
+for breakpoint packets.
 
 Each breakpoint and watchpoint packet @var{type} is documented
 separately.
@@ -28879,18 +28880,21 @@ remote target shall support either both 
 avoid potential problems with duplicate packets, the operations should
 be implemented in an idempotent way.}
 
-@item z0,@var{addr},@var{length}
-@itemx Z0,@var{addr},@var{length}
+@item z0,@var{addr},@var{kind}
+@itemx Z0,@var{addr},@var{kind}
 @cindex @samp{z0} packet
 @cindex @samp{Z0} packet
 Insert (@samp{Z0}) or remove (@samp{z0}) a memory breakpoint at address
-@var{addr} of size @var{length}.
+@var{addr} of type @var{kind}.
 
 A memory breakpoint is implemented by replacing the instruction at
 @var{addr} with a software breakpoint or trap instruction.  The
-@var{length} is used by targets that indicates the size of the
-breakpoint (in bytes) that should be inserted (e.g., the @sc{arm} and
-@sc{mips} can insert either a 2 or 4 byte breakpoint).
+@var{kind} is used by targets that have multiple breakpoint
+instructions.  It typically indicates the size of the breakpoint in
+bytes that should be inserted (e.g., the @sc{arm} and @sc{mips} can
+insert either a 2 or 4 byte breakpoint).  Some architectures have
+additional meanings for @var{kind}; @xref{Architecture-Specific
+Protocol Details}.
 
 @emph{Implementation note: It is possible for a target to copy or move
 code that contains memory breakpoints (e.g., when implementing
@@ -28907,15 +28911,16 @@ not supported
 for an error
 @end table
 
-@item z1,@var{addr},@var{length}
-@itemx Z1,@var{addr},@var{length}
+@item z1,@var{addr},@var{kind}
+@itemx Z1,@var{addr},@var{kind}
 @cindex @samp{z1} packet
 @cindex @samp{Z1} packet
 Insert (@samp{Z1}) or remove (@samp{z1}) a hardware breakpoint at
-address @var{addr} of size @var{length}.
+address @var{addr} of size @var{kind}.
 
 A hardware breakpoint is implemented using a mechanism that is not
-dependant on being able to modify the target's memory.
+dependant on being able to modify the target's memory.  @var{kind}
+has the same meaning as in @samp{Z0} packets.
 
 @emph{Implementation note: A hardware breakpoint is not affected by code
 movement.}
@@ -30004,8 +30009,35 @@ A badly formed request or an error was e
 
 @end table
 
-@node Register Packet Format
-@section Register Packet Format
+@node Architecture-Specific Protocol Details
+@section Architecture-Specific Protocol Details
+
+This section describes how the remote protocol is applied to specific
+target architectures.  Also see @ref{Standard Target Features}, for
+details of XML target descriptions for each architecture.
+
+@subsection ARM
+
+@subsubsection Breakpoint Kinds
+
+These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets.
+
+@table @r
+
+@item 2
+16-bit Thumb mode breakpoint.
+
+@item 3
+32-bit Thumb mode (Thumb-2) breakpoint.
+
+@item 4
+32-bit ARM mode breakpoint.
+
+@end table
+
+@subsection MIPS
+
+@subsubsection Register Packet Format
 
 The following @code{g}/@code{G} packets have previously been defined.
 In the below, some thirty-two bit registers are transferred as
Index: gdb-mainline/gdb/gdbarch.c
===================================================================
--- gdb-mainline.orig/gdb/gdbarch.c	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/gdbarch.c	2010-01-11 15:04:43.000000000 -0800
@@ -188,6 +188,7 @@ struct gdbarch
   gdbarch_skip_main_prologue_ftype *skip_main_prologue;
   gdbarch_inner_than_ftype *inner_than;
   gdbarch_breakpoint_from_pc_ftype *breakpoint_from_pc;
+  gdbarch_remote_breakpoint_from_pc_ftype *remote_breakpoint_from_pc;
   gdbarch_adjust_breakpoint_address_ftype *adjust_breakpoint_address;
   gdbarch_memory_insert_breakpoint_ftype *memory_insert_breakpoint;
   gdbarch_memory_remove_breakpoint_ftype *memory_remove_breakpoint;
@@ -330,6 +331,7 @@ struct gdbarch startup_gdbarch =
   0,  /* skip_main_prologue */
   0,  /* inner_than */
   0,  /* breakpoint_from_pc */
+  default_remote_breakpoint_from_pc,  /* remote_breakpoint_from_pc */
   0,  /* adjust_breakpoint_address */
   default_memory_insert_breakpoint,  /* memory_insert_breakpoint */
   default_memory_remove_breakpoint,  /* memory_remove_breakpoint */
@@ -456,6 +458,7 @@ gdbarch_alloc (const struct gdbarch_info
   gdbarch->value_from_register = default_value_from_register;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
+  gdbarch->remote_breakpoint_from_pc = default_remote_breakpoint_from_pc;
   gdbarch->memory_insert_breakpoint = default_memory_insert_breakpoint;
   gdbarch->memory_remove_breakpoint = default_memory_remove_breakpoint;
   gdbarch->remote_register_number = default_remote_register_number;
@@ -593,6 +596,8 @@ verify_gdbarch (struct gdbarch *gdbarch)
     fprintf_unfiltered (log, "\n\tinner_than");
   if (gdbarch->breakpoint_from_pc == 0)
     fprintf_unfiltered (log, "\n\tbreakpoint_from_pc");
+  if (gdbarch->remote_breakpoint_from_pc == default_remote_breakpoint_from_pc)
+    fprintf_unfiltered (log, "\n\tremote_breakpoint_from_pc");
   /* Skip verify of adjust_breakpoint_address, has predicate */
   /* Skip verify of memory_insert_breakpoint, invalid_p == 0 */
   /* Skip verify of memory_remove_breakpoint, invalid_p == 0 */
@@ -1067,6 +1072,9 @@ gdbarch_dump (struct gdbarch *gdbarch, s
                       "gdbarch_dump: regset_from_core_section = <%s>\n",
                       host_address_to_string (gdbarch->regset_from_core_section));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: remote_breakpoint_from_pc = <%s>\n",
+                      host_address_to_string (gdbarch->remote_breakpoint_from_pc));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: remote_register_number = <%s>\n",
                       host_address_to_string (gdbarch->remote_register_number));
   fprintf_unfiltered (file,
@@ -2284,6 +2292,23 @@ set_gdbarch_breakpoint_from_pc (struct g
   gdbarch->breakpoint_from_pc = breakpoint_from_pc;
 }
 
+void
+gdbarch_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *kindptr)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->remote_breakpoint_from_pc != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_remote_breakpoint_from_pc called\n");
+  gdbarch->remote_breakpoint_from_pc (gdbarch, pcptr, kindptr);
+}
+
+void
+set_gdbarch_remote_breakpoint_from_pc (struct gdbarch *gdbarch,
+                                       gdbarch_remote_breakpoint_from_pc_ftype remote_breakpoint_from_pc)
+{
+  gdbarch->remote_breakpoint_from_pc = remote_breakpoint_from_pc;
+}
+
 int
 gdbarch_adjust_breakpoint_address_p (struct gdbarch *gdbarch)
 {
Index: gdb-mainline/gdb/gdbarch.h
===================================================================
--- gdb-mainline.orig/gdb/gdbarch.h	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/gdbarch.h	2010-01-11 15:04:43.000000000 -0800
@@ -407,6 +407,14 @@ typedef const gdb_byte * (gdbarch_breakp
 extern const gdb_byte * gdbarch_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr);
 extern void set_gdbarch_breakpoint_from_pc (struct gdbarch *gdbarch, gdbarch_breakpoint_from_pc_ftype *breakpoint_from_pc);
 
+/* Return the adjusted address and kind to use for Z0/Z1 packets.
+   KIND is usually the memory length of the breakpoint, but may have a
+   different target-specific meaning. */
+
+typedef void (gdbarch_remote_breakpoint_from_pc_ftype) (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *kindptr);
+extern void gdbarch_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *kindptr);
+extern void set_gdbarch_remote_breakpoint_from_pc (struct gdbarch *gdbarch, gdbarch_remote_breakpoint_from_pc_ftype *remote_breakpoint_from_pc);
+
 extern int gdbarch_adjust_breakpoint_address_p (struct gdbarch *gdbarch);
 
 typedef CORE_ADDR (gdbarch_adjust_breakpoint_address_ftype) (struct gdbarch *gdbarch, CORE_ADDR bpaddr);
Index: gdb-mainline/gdb/gdbarch.sh
===================================================================
--- gdb-mainline.orig/gdb/gdbarch.sh	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/gdbarch.sh	2010-01-11 15:04:43.000000000 -0800
@@ -486,6 +486,10 @@ m:CORE_ADDR:skip_prologue:CORE_ADDR ip:i
 M:CORE_ADDR:skip_main_prologue:CORE_ADDR ip:ip
 f:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs:0:0
 m:const gdb_byte *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr::0:
+# Return the adjusted address and kind to use for Z0/Z1 packets.
+# KIND is usually the memory length of the breakpoint, but may have a
+# different target-specific meaning.
+m:void:remote_breakpoint_from_pc:CORE_ADDR *pcptr, int *kindptr:pcptr, kindptr::default_remote_breakpoint_from_pc:
 M:CORE_ADDR:adjust_breakpoint_address:CORE_ADDR bpaddr:bpaddr
 m:int:memory_insert_breakpoint:struct bp_target_info *bp_tgt:bp_tgt:0:default_memory_insert_breakpoint::0
 m:int:memory_remove_breakpoint:struct bp_target_info *bp_tgt:bp_tgt:0:default_memory_remove_breakpoint::0
Index: gdb-mainline/gdb/remote.c
===================================================================
--- gdb-mainline.orig/gdb/remote.c	2010-01-11 15:04:14.000000000 -0800
+++ gdb-mainline/gdb/remote.c	2010-01-11 15:04:43.000000000 -0800
@@ -7043,7 +7043,7 @@ remote_insert_breakpoint (struct gdbarch
       char *p;
       int bpsize;
 
-      gdbarch_breakpoint_from_pc (gdbarch, &addr, &bpsize);
+      gdbarch_remote_breakpoint_from_pc (gdbarch, &addr, &bpsize);
 
       rs = get_remote_state ();
       p = rs->buf;
@@ -7245,7 +7245,7 @@ remote_insert_hw_breakpoint (struct gdba
   /* The length field should be set to the size of a breakpoint
      instruction, even though we aren't inserting one ourselves.  */
 
-  gdbarch_breakpoint_from_pc
+  gdbarch_remote_breakpoint_from_pc
     (gdbarch, &bp_tgt->placed_address, &bp_tgt->placed_size);
 
   if (remote_protocol_packets[PACKET_Z1].support == PACKET_DISABLE)