[RFA] New qRelocInsn RSP packet, docs and NEWS.

[RFA] New qRelocInsn RSP packet, docs and NEWS.

[Pedro Alves]

This extracts the new qRelocInsn packet from this larger patch:

 [GDBserver fast tracepoints support]
 http://sourceware.org/ml/gdb-patches/2010-05/msg00149.html

It also extends the documentation a bit compared to that other
patch, and also adds a NEWS entry.

Okay?

-- 
Pedro Alves

gdb/
2010-05-24  Pedro Alves  <pedro@codesourcery.com>

	* NEWS: Mention the `qRelocInsn' feature.
	* gdbarch.sh (relocate_instruction): New.
	* amd64-tdep.c (rip_relative_offset): New.
	(append_insns): New.
	(amd64_relocate_instruction): New.
	(amd64_init_abi): Install it.
	* i386-tdep.c (append_insns): New.
	(i386_relocate_instruction): New.
	(i386_gdbarch_init): Install it.
	* remote.c (remote_get_noisy_reply): Handle qRelocInsn requests.
	* gdbarch.h, gdbarch.c: Regenerate.

gdb/doc/
2010-05-24  Pedro Alves  <pedro@codesourcery.com>

	* gdb.texinfo (General Query Packets) <qSupported>: Describe the
	`qRelocInsn' feature.
	(Relocate instruction reply packet): New subsection
	of `Tracepoint Packets'.
	(Tracepoint Packets): Mention that packets QTDP and QTStart
	support the qRelocInsn request, and add cross reference to new
	subsection.

---
 gdb/NEWS            |    9 +++
 gdb/amd64-tdep.c    |  119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/doc/gdb.texinfo |   53 ++++++++++++++++++++++-
 gdb/gdbarch.c       |   35 +++++++++++++++
 gdb/gdbarch.h       |   18 +++++++
 gdb/gdbarch.sh      |   13 +++++
 gdb/i386-tdep.c     |   82 +++++++++++++++++++++++++++++++++++
 gdb/remote.c        |   62 ++++++++++++++++++++++++---
 8 files changed, 382 insertions(+), 9 deletions(-)

Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS	2010-05-24 14:22:02.000000000 +0100
+++ src/gdb/NEWS	2010-05-24 14:29:38.000000000 +0100
@@ -18,6 +18,15 @@ qGetTIBAddr
 
   Return the address of the Windows Thread Information Block of a given thread.
 
+qRelocInsn
+
+  In response to several of the tracepoint packets, the target may now
+  also respond with a number of intermediate `qRelocInsn' request
+  packets before the final result packet, to have GDB handle
+  relocatating an instruction to execute at a different address.  This
+  is particularly useful for stubs that support fast tracepoints.  GDB
+  reports support for this feature in the qSupported packet.
+
 * The source command now accepts a -s option to force searching for the
   script in the source search path even if the script name specifies
   a directory.
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2010-05-24 12:28:55.000000000 +0100
+++ src/gdb/doc/gdb.texinfo	2010-05-24 14:21:56.000000000 +0100
@@ -31408,6 +31408,11 @@ This feature indicates that @value{GDBN}
 description.  If the stub sees @samp{xmlRegisters=} with target
 specific strings separated by a comma, it will report register
 description.
+
+@item qRelocInsn
+This feature indicates whether @value{GDBN} supports the
+@samp{qRelocInsn} packet (@pxref{Tracepoint Packets,,Relocate
+instruction reply packet}).
 @end table
 
 Stubs should ignore any unknown values for
@@ -32016,6 +32021,8 @@ Replies:
 @table @samp
 @item OK
 The packet was understood and carried out.
+@item qRelocInsn
+@xref{Tracepoint Packets,,Relocate instruction reply packet}.
 @item 
 The packet was not recognized.
 @end table
@@ -32080,6 +32087,8 @@ Replies:
 @table @samp
 @item OK
 The packet was understood and carried out.
+@item qRelocInsn
+@xref{Tracepoint Packets,,Relocate instruction reply packet}.
 @item 
 The packet was not recognized.
 @end table
@@ -32171,8 +32180,10 @@ Like @samp{QTFrame:range:@var{start}:@va
 frame @emph{outside} the given range of addresses (exclusive).
 
 @item QTStart
-Begin the tracepoint experiment.  Begin collecting data from tracepoint
-hits in the trace frame buffer.
+Begin the tracepoint experiment.  Begin collecting data from
+tracepoint hits in the trace frame buffer.  This packet supports the
+@samp{qRelocInsn} reply (@pxref{Tracepoint Packets,,Relocate
+instruction reply packet}).
 
 @item QTStop
 End the tracepoint experiment.  Stop collecting trace frames.
@@ -32333,6 +32344,44 @@ This packet directs the target to use a 
 
 @end table
 
+@subsection Relocate instruction reply packet
+When installing fast tracepoints in memory, the target may need to
+relocate the instruction currently at the tracepoint address to a
+different address in memory.  For most instructions, a simple copy is
+enough, but, for example, call instructions that implicitly push the
+return address on the stack, and relative branches or other
+PC-relative instructions require offset adjustment, so that the effect
+of executing the instruction at a different address is the same as if
+it had executed in the original location.
+
+In response to several of the tracepoint packets, the target may also
+respond with a number of intermediate @samp{qRelocInsn} request
+packets before the final result packet, to have @value{GDBN} handle
+this relocation operation.  If a packet supports this mechanism, its
+documentation will explicitly say so.  See for example the above
+descriptions for the @samp{QTStart} and @samp{QTDP} packets.  The
+format of the request is:
+
+@table @samp
+@item qRelocInsn:@var{from};@var{to}
+
+This requests @value{GDBN} to copy instruction at address @var{from}
+to address @var{to}, possibly adjusted so that executing the
+instruction at @var{to} has the same effect as executing it at
+@var{from}.  @value{GDBN} writes the adjusted instruction to target
+memory starting at @var{to}.
+@end table
+
+Replies:
+@table @samp
+@item qRelocInsn:@var{adjusted_size}
+Informs the stub the relocation is complete.  @var{adjusted_size} is
+the length in bytes of resulting relocated instruction sequence.
+@item E @var{NN}
+A badly formed request was detected, or an error was encountered while
+relocating the instruction.
+@end table
+
 @node Host I/O Packets
 @section Host I/O Packets
 @cindex Host I/O, remote protocol
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-05-24 12:28:42.000000000 +0100
+++ src/gdb/remote.c	2010-05-24 13:14:56.000000000 +0100
@@ -242,6 +242,8 @@ static void remote_terminal_ours (void);
 
 static int remote_read_description_p (struct target_ops *target);
 
+char *unpack_varlen_hex (char *buff, ULONGEST *result);
+
 /* The non-stop remote protocol provisions for one pending stop reply.
    This is where we keep it until it is acknowledged.  */
 
@@ -433,6 +435,55 @@ remote_get_noisy_reply (char **buf_p,
       buf = *buf_p;
       if (buf[0] == 'E')
 	trace_error (buf);
+      else if (strncmp (buf, "qRelocInsn:", strlen ("qRelocInsn:")) == 0)
+	{
+	  ULONGEST ul;
+	  CORE_ADDR from, to, org_to;
+	  char *p, *pp;
+	  int adjusted_size = 0;
+	  volatile struct gdb_exception ex;
+
+	  p = buf + strlen ("qRelocInsn:");
+	  pp = unpack_varlen_hex (p, &ul);
+	  if (*pp != ';')
+	    error (_("invalid qRelocInsn packet: %s\n"), buf);
+	  from = ul;
+
+	  p = pp + 1;
+	  pp = unpack_varlen_hex (p, &ul);
+	  to = ul;
+
+	  org_to = to;
+
+	  TRY_CATCH (ex, RETURN_MASK_ALL)
+	    {
+	      gdbarch_relocate_instruction (target_gdbarch, &to, from);
+	    }
+	  if (ex.reason >= 0)
+	    {
+	      adjusted_size = to - org_to;
+
+	      sprintf (buf, "qRelocInsn:%x", adjusted_size);
+	      putpkt (buf);
+	    }
+	  else if (ex.reason < 0 && ex.error == MEMORY_ERROR)
+	    {
+	      /* Propagate memory errors silently back to the target.
+		 The stub may have limited the range of addresses we
+		 can write to, for example.  */
+	      putpkt ("E01");
+	    }
+	  else
+	    {
+	      /* Something unexpectedly bad happened.  Be verbose so
+		 we can tell what, and propagate the error back to the
+		 stub, so it doesn't get stuck waiting for a
+		 response.  */
+	      exception_fprintf (gdb_stderr, ex,
+				 _("warning: relocating instruction: "));
+	      putpkt ("E01");
+	    }
+	}
       else if (buf[0] == 'O' && buf[1] != 'K')
 	remote_console_output (buf + 1);	/* 'O' message from stub */
       else
@@ -3584,13 +3635,10 @@ remote_query_supported (void)
       if (remote_support_xml)
 	q = remote_query_supported_append (q, remote_support_xml);
 
-      if (q)
-	{
-	  q = reconcat (q, "qSupported:", q, (char *) NULL);
-	  putpkt (q);
-	}
-      else
-	putpkt ("qSupported");
+      q = remote_query_supported_append (q, "qRelocInsn+");
+
+      q = reconcat (q, "qSupported:", q, (char *) NULL);
+      putpkt (q);
 
       do_cleanups (old_chain);
 
Index: src/gdb/amd64-tdep.c
===================================================================
--- src.orig/gdb/amd64-tdep.c	2010-05-24 13:00:23.000000000 +0100
+++ src/gdb/amd64-tdep.c	2010-05-24 14:14:21.000000000 +0100
@@ -1507,6 +1507,123 @@ amd64_displaced_step_fixup (struct gdbar
 			    paddress (gdbarch, retaddr));
     }
 }
+
+/* If the instruction INSN uses RIP-relative addressing, return the
+   offset into the raw INSN where the displacement to be adjusted is
+   found.  Returns 0 if the instruction doesn't use RIP-relative
+   addressing.  */
+
+static int
+rip_relative_offset (struct amd64_insn *insn)
+{
+  if (insn->modrm_offset != -1)
+    {
+      gdb_byte modrm = insn->raw_insn[insn->modrm_offset];
+
+      if ((modrm & 0xc7) == 0x05)
+	{
+	  /* The displacement is found right after the ModRM byte.  */
+	  return insn->modrm_offset + 1;
+	}
+    }
+
+  return 0;
+}
+
+static void
+append_insns (CORE_ADDR *to, ULONGEST len, const gdb_byte *buf)
+{
+  target_write_memory (*to, buf, len);
+  *to += len;
+}
+
+void
+amd64_relocate_instruction (struct gdbarch *gdbarch,
+			    CORE_ADDR *to, CORE_ADDR oldloc)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  int len = gdbarch_max_insn_length (gdbarch);
+  /* Extra space for sentinels.  */
+  int fixup_sentinel_space = len;
+  gdb_byte *buf = xmalloc (len + fixup_sentinel_space);
+  struct amd64_insn insn_details;
+  int offset = 0;
+  LONGEST rel32, newrel;
+  gdb_byte *insn;
+  int insn_length;
+
+  read_memory (oldloc, buf, len);
+
+  /* Set up the sentinel space so we don't have to worry about running
+     off the end of the buffer.  An excessive number of leading prefixes
+     could otherwise cause this.  */
+  memset (buf + len, 0, fixup_sentinel_space);
+
+  insn = buf;
+  amd64_get_insn_details (insn, &insn_details);
+
+  insn_length = gdb_buffered_insn_length (gdbarch, insn, len, oldloc);
+
+  /* Skip legacy instruction prefixes.  */
+  insn = amd64_skip_prefixes (insn);
+
+  /* Adjust calls with 32-bit relative addresses as push/jump, with
+     the address pushed being the location where the original call in
+     the user program would return to.  */
+  if (insn[0] == 0xe8)
+    {
+      gdb_byte push_buf[16];
+      unsigned int ret_addr;
+
+      /* Where "ret" in the original code will return to.  */
+      ret_addr = oldloc + insn_length;
+      push_buf[0] = 0x68; /* pushq $... */
+      memcpy (&push_buf[1], &ret_addr, 4);
+      /* Push the push.  */
+      append_insns (to, 5, push_buf);
+
+      /* Convert the relative call to a relative jump.  */
+      insn[0] = 0xe9;
+
+      /* Adjust the destination offset.  */
+      rel32 = extract_signed_integer (insn + 1, 4, byte_order);
+      newrel = (oldloc - *to) + rel32;
+      store_signed_integer (insn + 1, 4, newrel, byte_order);
+
+      /* Write the adjusted jump into its displaced location.  */
+      append_insns (to, 5, insn);
+      return;
+    }
+
+  offset = rip_relative_offset (&insn_details);
+  if (!offset)
+    {
+      /* Adjust jumps with 32-bit relative addresses.  Calls are
+	 already handled above.  */
+      if (insn[0] == 0xe9)
+	offset = 1;
+      /* Adjust conditional jumps.  */
+      else if (insn[0] == 0x0f && (insn[1] & 0xf0) == 0x80)
+	offset = 2;
+    }
+
+  if (offset)
+    {
+      rel32 = extract_signed_integer (insn + offset, 4, byte_order);
+      newrel = (oldloc - *to) + rel32;
+      store_signed_integer (insn + offset, 4, newrel, byte_order);
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Adjusted insn rel32=0x%s at 0x%s to"
+			    " rel32=0x%s at 0x%s\n",
+			    hex_string (rel32), paddress (gdbarch, oldloc),
+			    hex_string (newrel), paddress (gdbarch, *to));
+    }
+
+  /* Write the adjusted instruction into its displaced location.  */
+  append_insns (to, insn_length, buf);
+}
+
 \f
 /* The maximum number of saved registers.  This should include %rip.  */
 #define AMD64_NUM_SAVED_REGS	AMD64_NUM_GREGS
@@ -2363,6 +2480,8 @@ amd64_init_abi (struct gdbarch_info info
 					  amd64_regset_from_core_section);
 
   set_gdbarch_get_longjmp_target (gdbarch, amd64_get_longjmp_target);
+
+  set_gdbarch_relocate_instruction (gdbarch, amd64_relocate_instruction);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
Index: src/gdb/gdbarch.c
===================================================================
--- src.orig/gdb/gdbarch.c	2010-05-24 13:00:42.000000000 +0100
+++ src/gdb/gdbarch.c	2010-05-24 14:14:44.000000000 +0100
@@ -245,6 +245,7 @@ struct gdbarch
   gdbarch_displaced_step_fixup_ftype *displaced_step_fixup;
   gdbarch_displaced_step_free_closure_ftype *displaced_step_free_closure;
   gdbarch_displaced_step_location_ftype *displaced_step_location;
+  gdbarch_relocate_instruction_ftype *relocate_instruction;
   gdbarch_overlay_update_ftype *overlay_update;
   gdbarch_core_read_description_ftype *core_read_description;
   gdbarch_static_transform_name_ftype *static_transform_name;
@@ -392,6 +393,7 @@ struct gdbarch startup_gdbarch =
   0,  /* displaced_step_fixup */
   NULL,  /* displaced_step_free_closure */
   NULL,  /* displaced_step_location */
+  0,  /* relocate_instruction */
   0,  /* overlay_update */
   0,  /* core_read_description */
   0,  /* static_transform_name */
@@ -493,6 +495,7 @@ gdbarch_alloc (const struct gdbarch_info
   gdbarch->displaced_step_fixup = NULL;
   gdbarch->displaced_step_free_closure = NULL;
   gdbarch->displaced_step_location = NULL;
+  gdbarch->relocate_instruction = NULL;
   gdbarch->target_signal_from_host = default_target_signal_from_host;
   gdbarch->target_signal_to_host = default_target_signal_to_host;
   gdbarch->has_shared_address_space = default_has_shared_address_space;
@@ -667,6 +670,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
     fprintf_unfiltered (log, "\n\tdisplaced_step_free_closure");
   if ((! gdbarch->displaced_step_location) != (! gdbarch->displaced_step_copy_insn))
     fprintf_unfiltered (log, "\n\tdisplaced_step_location");
+  /* Skip verify of relocate_instruction, has predicate */
   /* Skip verify of overlay_update, has predicate */
   /* Skip verify of core_read_description, has predicate */
   /* Skip verify of static_transform_name, has predicate */
@@ -1105,6 +1109,12 @@ 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: gdbarch_relocate_instruction_p() = %d\n",
+                      gdbarch_relocate_instruction_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: relocate_instruction = <%s>\n",
+                      host_address_to_string (gdbarch->relocate_instruction));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: remote_breakpoint_from_pc = <%s>\n",
                       host_address_to_string (gdbarch->remote_breakpoint_from_pc));
   fprintf_unfiltered (file,
@@ -3303,6 +3313,31 @@ set_gdbarch_displaced_step_location (str
 }
 
 int
+gdbarch_relocate_instruction_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->relocate_instruction != NULL;
+}
+
+void
+gdbarch_relocate_instruction (struct gdbarch *gdbarch, CORE_ADDR *to, CORE_ADDR from)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->relocate_instruction != NULL);
+  /* Do not check predicate: gdbarch->relocate_instruction != NULL, allow call.  */
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_relocate_instruction called\n");
+  gdbarch->relocate_instruction (gdbarch, to, from);
+}
+
+void
+set_gdbarch_relocate_instruction (struct gdbarch *gdbarch,
+                                  gdbarch_relocate_instruction_ftype relocate_instruction)
+{
+  gdbarch->relocate_instruction = relocate_instruction;
+}
+
+int
 gdbarch_overlay_update_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
Index: src/gdb/gdbarch.h
===================================================================
--- src.orig/gdb/gdbarch.h	2010-05-24 13:00:42.000000000 +0100
+++ src/gdb/gdbarch.h	2010-05-24 14:14:43.000000000 +0100
@@ -806,6 +806,24 @@ typedef CORE_ADDR (gdbarch_displaced_ste
 extern CORE_ADDR gdbarch_displaced_step_location (struct gdbarch *gdbarch);
 extern void set_gdbarch_displaced_step_location (struct gdbarch *gdbarch, gdbarch_displaced_step_location_ftype *displaced_step_location);
 
+/* Relocate an instruction to execute at a different address.  OLDLOC
+   is the address in the inferior memory where the instruction to
+   relocate is currently at.  On input, TO points to the destination
+   where we want the instruction to be copied (and possibly adjusted)
+   to.  On output, it points to one past the end of the resulting
+   instruction(s).  The effect of executing the instruction at TO shall
+   be the same as if executing it at FROM.  For example, call
+   instructions that implicitly push the return address on the stack
+   should be adjusted to return to the instruction after OLDLOC;
+   relative branches, and other PC-relative instructions need the
+   offset adjusted; etc. */
+
+extern int gdbarch_relocate_instruction_p (struct gdbarch *gdbarch);
+
+typedef void (gdbarch_relocate_instruction_ftype) (struct gdbarch *gdbarch, CORE_ADDR *to, CORE_ADDR from);
+extern void gdbarch_relocate_instruction (struct gdbarch *gdbarch, CORE_ADDR *to, CORE_ADDR from);
+extern void set_gdbarch_relocate_instruction (struct gdbarch *gdbarch, gdbarch_relocate_instruction_ftype *relocate_instruction);
+
 /* Refresh overlay mapped state for section OSECT. */
 
 extern int gdbarch_overlay_update_p (struct gdbarch *gdbarch);
Index: src/gdb/gdbarch.sh
===================================================================
--- src.orig/gdb/gdbarch.sh	2010-05-24 13:00:42.000000000 +0100
+++ src/gdb/gdbarch.sh	2010-05-24 14:14:47.000000000 +0100
@@ -708,6 +708,19 @@ m:void:displaced_step_free_closure:struc
 # see the comments in infrun.c.
 m:CORE_ADDR:displaced_step_location:void:::NULL::(! gdbarch->displaced_step_location) != (! gdbarch->displaced_step_copy_insn)
 
+# Relocate an instruction to execute at a different address.  OLDLOC
+# is the address in the inferior memory where the instruction to
+# relocate is currently at.  On input, TO points to the destination
+# where we want the instruction to be copied (and possibly adjusted)
+# to.  On output, it points to one past the end of the resulting
+# instruction(s).  The effect of executing the instruction at TO shall
+# be the same as if executing it at FROM.  For example, call
+# instructions that implicitly push the return address on the stack
+# should be adjusted to return to the instruction after OLDLOC;
+# relative branches, and other PC-relative instructions need the
+# offset adjusted; etc.
+M:void:relocate_instruction:CORE_ADDR *to, CORE_ADDR from:to, from::NULL
+
 # Refresh overlay mapped state for section OSECT.
 F:void:overlay_update:struct obj_section *osect:osect
 
Index: src/gdb/i386-tdep.c
===================================================================
--- src.orig/gdb/i386-tdep.c	2010-05-24 12:58:48.000000000 +0100
+++ src/gdb/i386-tdep.c	2010-05-24 14:14:36.000000000 +0100
@@ -658,6 +658,86 @@ i386_displaced_step_fixup (struct gdbarc
                             paddress (gdbarch, retaddr));
     }
 }
+
+static void
+append_insns (CORE_ADDR *to, ULONGEST len, const gdb_byte *buf)
+{
+  target_write_memory (*to, buf, len);
+  *to += len;
+}
+
+static void
+i386_relocate_instruction (struct gdbarch *gdbarch,
+			   CORE_ADDR *to, CORE_ADDR oldloc)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  gdb_byte buf[I386_MAX_INSN_LEN];
+  int offset = 0, rel32, newrel;
+  int insn_length;
+  gdb_byte *insn = buf;
+
+  read_memory (oldloc, buf, I386_MAX_INSN_LEN);
+
+  insn_length = gdb_buffered_insn_length (gdbarch, insn,
+					  I386_MAX_INSN_LEN, oldloc);
+
+  /* Get past the prefixes.  */
+  insn = i386_skip_prefixes (insn, I386_MAX_INSN_LEN);
+
+  /* Adjust calls with 32-bit relative addresses as push/jump, with
+     the address pushed being the location where the original call in
+     the user program would return to.  */
+  if (insn[0] == 0xe8)
+    {
+      gdb_byte push_buf[16];
+      unsigned int ret_addr;
+
+      /* Where "ret" in the original code will return to.  */
+      ret_addr = oldloc + insn_length;
+      push_buf[0] = 0x68; /* pushq $... */
+      memcpy (&push_buf[1], &ret_addr, 4);
+      /* Push the push.  */
+      append_insns (to, 5, push_buf);
+
+      /* Convert the relative call to a relative jump.  */
+      insn[0] = 0xe9;
+
+      /* Adjust the destination offset.  */
+      rel32 = extract_signed_integer (insn + 1, 4, byte_order);
+      newrel = (oldloc - *to) + rel32;
+      store_signed_integer (insn + 1, 4, newrel, byte_order);
+
+      /* Write the adjusted jump into its displaced location.  */
+      append_insns (to, 5, insn);
+      return;
+    }
+
+  /* Adjust jumps with 32-bit relative addresses.  Calls are already
+     handled above.  */
+  if (insn[0] == 0xe9)
+    offset = 1;
+  /* Adjust conditional jumps.  */
+  else if (insn[0] == 0x0f && (insn[1] & 0xf0) == 0x80)
+    offset = 2;
+
+  if (offset)
+    {
+      rel32 = extract_signed_integer (insn + offset, 4, byte_order);
+      newrel = (oldloc - *to) + rel32;
+      store_signed_integer (insn + offset, 4, newrel, byte_order);
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Adjusted insn rel32=0x%s at 0x%s to"
+			    " rel32=0x%s at 0x%s\n",
+			    hex_string (rel32), paddress (gdbarch, oldloc),
+			    hex_string (newrel), paddress (gdbarch, *to));
+    }
+
+  /* Write the adjusted instructions into their displaced
+     location.  */
+  append_insns (to, insn_length, buf);
+}
+
 \f
 #ifdef I386_REGNO_TO_SYMMETRY
 #error "The Sequent Symmetry is no longer supported."
@@ -6909,6 +6989,8 @@ i386_gdbarch_init (struct gdbarch_info i
 
   tdesc_data = tdesc_data_alloc ();
 
+  set_gdbarch_relocate_instruction (gdbarch, i386_relocate_instruction);
+
   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.tdep_info = (void *) tdesc_data;
   gdbarch_init_osabi (info, gdbarch);

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Mon, 24 May 2010 14:35:45 +0100
> 
> This extracts the new qRelocInsn packet from this larger patch:
> 
>  [GDBserver fast tracepoints support]
>  http://sourceware.org/ml/gdb-patches/2010-05/msg00149.html
> 
> It also extends the documentation a bit compared to that other
> patch, and also adds a NEWS entry.

The documentation parts of the patch, including the NEWS entry, are
fine with me.

Thanks.

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Joel Brobecker]

No real comment on the patch, just one typo that I spotted:

> +  In response to several of the tracepoint packets, the target may now
> +  also respond with a number of intermediate `qRelocInsn' request
> +  packets before the final result packet, to have GDB handle
> +  relocatating an instruction to execute at a different address.  This
     ^^^^^^^^^^^^
     relocating.

-- 
Joel

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Pedro Alves]

On Monday 24 May 2010 19:06:51, Eli Zaretskii wrote:

> The documentation parts of the patch, including the NEWS entry, are
> fine with me.
> 
> Thanks.

Thanks for the review.  I've applied it after fixing the typo
Joel spotted.

-- 
Pedro Alves

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Index: src/gdb/remote.c
Pedro> ===================================================================
Pedro> --- src.orig/gdb/remote.c	2010-05-24 12:28:42.000000000 +0100
Pedro> +++ src/gdb/remote.c	2010-05-24 13:14:56.000000000 +0100
Pedro> @@ -242,6 +242,8 @@ static void remote_terminal_ours (void);
 
Pedro>  static int remote_read_description_p (struct target_ops *target);
 
Pedro> +char *unpack_varlen_hex (char *buff, ULONGEST *result);

It seems like this declaration could go in tracepoint.h.

Tom

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Pedro Alves]

On Thursday 27 May 2010 20:58:02, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Index: src/gdb/remote.c
> Pedro> ===================================================================
> Pedro> --- src.orig/gdb/remote.c        2010-05-24 12:28:42.000000000 +0100
> Pedro> +++ src/gdb/remote.c     2010-05-24 13:14:56.000000000 +0100
> Pedro> @@ -242,6 +242,8 @@ static void remote_terminal_ours (void);
>  
> Pedro>  static int remote_read_description_p (struct target_ops *target);
>  
> Pedro> +char *unpack_varlen_hex (char *buff, ULONGEST *result);
> 
> It seems like this declaration could go in tracepoint.h.

This function does live in remote.c, but it's defined further down, close to
its siblings.  tracepoint.c is also reusing it since not to long ago (since
tracing moved to target_ops).  remote.h would make a bit more sense though
one could argue it could also live in a shared utils.c-kind of file nowadays.

-- 
Pedro Alves

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> +char *unpack_varlen_hex (char *buff, ULONGEST *result);

Tom> It seems like this declaration could go in tracepoint.h.

Pedro> This function does live in remote.c, but it's defined further
Pedro> down, close to its siblings.  tracepoint.c is also reusing it
Pedro> since not to long ago (since tracing moved to target_ops).
Pedro> remote.h would make a bit more sense though one could argue it
Pedro> could also live in a shared utils.c-kind of file nowadays.

Oops, sorry about the mistake.  I misread which file the definition is in.

My concern is that duplicate declarations lead to bugs, because they can
insulate a module from changes to an API it uses.  Ideally, I think that
all non-static objects ought to have a single declaration in a single
header file, which is included by all users.

Exactly where something lives is secondary to me.  I do think it is
generally better for generic functions to be somewhere like utils.c.
The only real failure mode to a bad placement is a bit of code
duplication, though, and that isn't as serious a problem, at least not
for "leaf" things like this.

thanks,
Tom

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Pedro Alves]

On Thursday 27 May 2010 21:09:22, Pedro Alves wrote:
> On Thursday 27 May 2010 20:58:02, Tom Tromey wrote:
> > >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> > 
> > Pedro> Index: src/gdb/remote.c
> > Pedro> ===================================================================
> > Pedro> --- src.orig/gdb/remote.c        2010-05-24 12:28:42.000000000 +0100
> > Pedro> +++ src/gdb/remote.c     2010-05-24 13:14:56.000000000 +0100
> > Pedro> @@ -242,6 +242,8 @@ static void remote_terminal_ours (void);
> >  
> > Pedro>  static int remote_read_description_p (struct target_ops *target);
> >  
> > Pedro> +char *unpack_varlen_hex (char *buff, ULONGEST *result);
> > 
> > It seems like this declaration could go in tracepoint.h.
> 
> This function does live in remote.c, but it's defined further down, close to
> its siblings.  tracepoint.c is also reusing it since not to long ago (since
> tracing moved to target_ops).  remote.h would make a bit more sense though
> one could argue it could also live in a shared utils.c-kind of file nowadays.

I've gone with remote.h.  There's a bit of cruft there too.

A bit going backwards to be including remote.h in tracepoint.c,
but, this is only having the includes match reality.

I think this is obvious, and so I'll apply it in a bit.

-- 
Pedro Alves

2010-05-27  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (unpack_varlen_hex): Remove forward declaration.
	(remote_console_output): Make static, and add forward declaration.
	* remote.h: Drop FIXME comment.
	(remote_console_output, remote_cisco_objfile_relocate)
	(deprecated_target_resume_hook, deprecated_target_wait_loop_hook):
	Delete declarations.
	* tracepoint.c: Include "remote.h".
	(unpack_varlen_hex): Delete declaration.

---
 gdb/remote.c     |    4 ++--
 gdb/remote.h     |   19 +------------------
 gdb/tracepoint.c |    3 +--
 3 files changed, 4 insertions(+), 22 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-05-27 21:23:02.000000000 +0100
+++ src/gdb/remote.c	2010-05-27 21:31:55.000000000 +0100
@@ -242,7 +242,7 @@ static void remote_terminal_ours (void);
 
 static int remote_read_description_p (struct target_ops *target);
 
-char *unpack_varlen_hex (char *buff, ULONGEST *result);
+static void remote_console_output (char *msg);
 
 /* The non-stop remote protocol provisions for one pending stop reply.
    This is where we keep it until it is acknowledged.  */
@@ -4668,7 +4668,7 @@ remote_terminal_ours (void)
   remote_async_terminal_ours_p = 1;
 }
 
-void
+static void
 remote_console_output (char *msg)
 {
   char *p;
Index: src/gdb/remote.h
===================================================================
--- src.orig/gdb/remote.h	2010-05-27 21:20:58.000000000 +0100
+++ src/gdb/remote.h	2010-05-27 21:30:56.000000000 +0100
@@ -22,8 +22,6 @@
 
 struct target_desc;
 
-/* FIXME?: move this interface down to tgt vector) */
-
 /* Read a packet from the remote machine, with error checking, and
    store it in *BUF.  Resize *BUF using xrealloc if necessary to hold
    the result, and update *SIZEOF_BUF.  If FOREVER, wait forever
@@ -40,19 +38,7 @@ extern void getpkt (char **buf, long *si
 
 extern int putpkt (char *buf);
 
-/* Send HEX encoded string to the target console. (gdb_stdtarg) */
-
-extern void remote_console_output (char *);
-
-
-/* FIXME: cagney/1999-09-20: The remote cisco stuff in remote.c needs
-   to be broken out into a separate file (remote-cisco.[hc]?).  Before
-   that can happen, a remote protocol stack framework needs to be
-   implemented. */
-
-extern void remote_cisco_objfile_relocate (bfd_signed_vma text_off,
-					   bfd_signed_vma data_off,
-					   bfd_signed_vma bss_off);
+extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
 
 extern void async_remote_interrupt_twice (void *arg);
 
@@ -61,9 +47,6 @@ extern int remote_write_bytes (CORE_ADDR
 
 extern int remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
 
-extern void (*deprecated_target_resume_hook) (void);
-extern void (*deprecated_target_wait_loop_hook) (void);
-
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
 void register_remote_support_xml (const char *);
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-05-27 21:21:02.000000000 +0100
+++ src/gdb/tracepoint.c	2010-05-27 21:29:27.000000000 +0100
@@ -46,6 +46,7 @@
 #include "gdbthread.h"
 #include "stack.h"
 #include "gdbcore.h"
+#include "remote.h"
 
 #include "ax.h"
 #include "ax-gdb.h"
@@ -3307,8 +3308,6 @@ tfile_interp_line (char *line,
 /* Parse the part of trace status syntax that is shared between
    the remote protocol and the trace file reader.  */
 
-extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
-
 void
 parse_trace_status (char *line, struct trace_status *ts)
 {

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Pedro Alves]

On Thursday 27 May 2010 21:37:07, Tom Tromey wrote:
> My concern is that duplicate declarations lead to bugs, because they can
> insulate a module from changes to an API it uses.  Ideally, I think that
> all non-static objects ought to have a single declaration in a single
> header file, which is included by all users.

Oh, you know I agree with that! :-)  I didn't remember that tracepoint.c
was using this function, and since I had written this a while ago, I
had forgotten/missed why the declaration I was adding didn't
have a "static" qualifier.

> Exactly where something lives is secondary to me.  I do think it is
> generally better for generic functions to be somewhere like utils.c.
> The only real failure mode to a bad placement is a bit of code
> duplication, though, and that isn't as serious a problem, at least not
> for "leaf" things like this.

Yep, agreed on all accounts; I've written a patch to move the
declaration to a header.  I've left moving the functions somewhere
else for another day; it doesn't seem to add much benefit today.

-- 
Pedro Alves

Re: [RFA] New qRelocInsn RSP packet, docs and NEWS.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I've gone with remote.h.  There's a bit of cruft there too.
Pedro> A bit going backwards to be including remote.h in tracepoint.c,
Pedro> but, this is only having the includes match reality.
Pedro> I think this is obvious, and so I'll apply it in a bit.

Thanks for doing this.

Tom