[PATCH 0/2] Small memory-writing gdbserver improvements

[PATCH 0/2] Small memory-writing gdbserver improvements

[Tom Tromey]

I noticed a comment in gdbserver referring to cleanups, so I fixed
that; and then added on a patch to remove write_inferior_memory in
favor of target_write_memory.

Tested by the buildbot.

Tom

[PATCH 2/2] Replace write_inferior_memory with target_write_memory

[Tom Tromey]

target_write_memory is just a simple wrapper for
write_inferior_memory.  Because target_write_memory is needed for
gdbsupport, and because gdb uses the name "target_write_memory"
everywhere, this patch renames write_inferior_memory and removes the
wrapper.  I think this brings gdb and gdbserver slightly more in sync.

gdb/gdbserver/ChangeLog
2019-08-14  Tom Tromey  <tromey@adacore.com>

	* tracepoint.c (write_inferior_data_pointer)
	(write_inferior_integer, write_inferior_int8)
	(write_inferior_uinteger, m_tracepoint_action_download)
	(r_tracepoint_action_download, x_tracepoint_action_download)
	(l_tracepoint_action_download, clear_inferior_trace_buffer)
	(download_agent_expr, download_tracepoint_1)
	(download_trace_state_variables, upload_fast_traceframes): Update.
	* server.c (gdb_write_memory): Update.
	* remote-utils.c (relocate_instruction): Update.
	* proc-service.c (ps_pdwrite): Update.
	* mem-break.c (remove_memory_breakpoint)
	(delete_fast_tracepoint_jump, set_fast_tracepoint_jump)
	(uninsert_fast_tracepoint_jumps_at)
	(reinsert_fast_tracepoint_jumps_at): Update.
	* linux-x86-low.c (append_insns)
	(i386_install_fast_tracepoint_jump_pad)
	(amd64_write_goto_address, i386_write_goto_address): Update.
	* linux-s390-low.c (append_insns, s390_write_goto_address):
	Update.
	* linux-ppc-low.c (ppc_relocate_instruction)
	(ppc_install_fast_tracepoint_jump_pad, emit_insns)
	(ppc_write_goto_address): Update.
	* linux-aarch64-low.c (append_insns): Update.
	* target.h (struct target_ops): Update.
	(write_inferior_memory): Don't declare.
	* target.c (target_write_memory): Rename from
	write_inferior_memory.  Remove old target_write_memory.
---
 gdb/gdbserver/ChangeLog           | 30 +++++++++++++++++++++++++++++
 gdb/gdbserver/linux-aarch64-low.c |  4 ++--
 gdb/gdbserver/linux-ppc-low.c     | 22 ++++++++++-----------
 gdb/gdbserver/linux-s390-low.c    |  4 ++--
 gdb/gdbserver/linux-x86-low.c     |  8 ++++----
 gdb/gdbserver/mem-break.c         | 30 ++++++++++++++---------------
 gdb/gdbserver/proc-service.c      |  2 +-
 gdb/gdbserver/remote-utils.c      |  4 ++--
 gdb/gdbserver/server.c            |  2 +-
 gdb/gdbserver/target.c            | 14 ++++----------
 gdb/gdbserver/target.h            |  5 +----
 gdb/gdbserver/tracepoint.c        | 32 +++++++++++++++----------------
 12 files changed, 89 insertions(+), 68 deletions(-)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index ab2f40ea98c..33095ea752d 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -1626,11 +1626,11 @@ append_insns (CORE_ADDR *to, size_t len, const uint32_t *buf)
   for (i = 0; i < len; i++)
     le_buf[i] = htole32 (buf[i]);
 
-  write_inferior_memory (*to, (const unsigned char *) le_buf, byte_len);
+  target_write_memory (*to, (const unsigned char *) le_buf, byte_len);
 
   xfree (le_buf);
 #else
-  write_inferior_memory (*to, (const unsigned char *) buf, byte_len);
+  target_write_memory (*to, (const unsigned char *) buf, byte_len);
 #endif
 
   *to += byte_len;
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index f17f05a0a32..8a0965bd20a 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -1535,12 +1535,12 @@ ppc_relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
 
 	  /* Jump over the unconditional branch.  */
 	  insn = (insn & ~0xfffc) | 0x8;
-	  write_inferior_memory (*to, (unsigned char *) &insn, 4);
+	  target_write_memory (*to, (unsigned char *) &insn, 4);
 	  *to += 4;
 
 	  /* Build a unconditional branch and copy LK bit.  */
 	  insn = (18 << 26) | (0x3fffffc & newrel) | (insn & 0x3);
-	  write_inferior_memory (*to, (unsigned char *) &insn, 4);
+	  target_write_memory (*to, (unsigned char *) &insn, 4);
 	  *to += 4;
 
 	  return;
@@ -1563,14 +1563,14 @@ ppc_relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
 	  bdnz_insn |= (insn ^ (1 << 22)) & (1 << 22);
 	  bf_insn |= (insn ^ (1 << 24)) & (1 << 24);
 
-	  write_inferior_memory (*to, (unsigned char *) &bdnz_insn, 4);
+	  target_write_memory (*to, (unsigned char *) &bdnz_insn, 4);
 	  *to += 4;
-	  write_inferior_memory (*to, (unsigned char *) &bf_insn, 4);
+	  target_write_memory (*to, (unsigned char *) &bf_insn, 4);
 	  *to += 4;
 
 	  /* Build a unconditional branch and copy LK bit.  */
 	  insn = (18 << 26) | (0x3fffffc & newrel) | (insn & 0x3);
-	  write_inferior_memory (*to, (unsigned char *) &insn, 4);
+	  target_write_memory (*to, (unsigned char *) &insn, 4);
 	  *to += 4;
 
 	  return;
@@ -1583,14 +1583,14 @@ ppc_relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
 
 	  /* Build a unconditional branch and copy LK bit.  */
 	  insn = (18 << 26) | (0x3fffffc & newrel) | (insn & 0x3);
-	  write_inferior_memory (*to, (unsigned char *) &insn, 4);
+	  target_write_memory (*to, (unsigned char *) &insn, 4);
 	  *to += 4;
 
 	  return;
 	}
     }
 
-  write_inferior_memory (*to, (unsigned char *) &insn, 4);
+  target_write_memory (*to, (unsigned char *) &insn, 4);
   *to += 4;
 }
 
@@ -1750,7 +1750,7 @@ ppc_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
   p += GEN_ADDI (p, 1, 1, frame_size);
 
   /* Flush instructions to inferior memory.  */
-  write_inferior_memory (buildaddr, (unsigned char *) buf, (p - buf) * 4);
+  target_write_memory (buildaddr, (unsigned char *) buf, (p - buf) * 4);
 
   /* Now, insert the original instruction to execute in the jump pad.  */
   *adjusted_insn_addr = buildaddr + (p - buf) * 4;
@@ -1780,7 +1780,7 @@ ppc_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
     }
   /* b <tpaddr+4> */
   p += GEN_B (p, offset);
-  write_inferior_memory (buildaddr, (unsigned char *) buf, (p - buf) * 4);
+  target_write_memory (buildaddr, (unsigned char *) buf, (p - buf) * 4);
   *jump_entry = buildaddr + (p - buf) * 4;
 
   /* The jump pad is now built.  Wire in a jump to our jump pad.  This
@@ -1816,7 +1816,7 @@ static void
 emit_insns (uint32_t *buf, int n)
 {
   n = n * sizeof (uint32_t);
-  write_inferior_memory (current_insn_ptr, (unsigned char *) buf, n);
+  target_write_memory (current_insn_ptr, (unsigned char *) buf, n);
   current_insn_ptr += n;
 }
 
@@ -2604,7 +2604,7 @@ ppc_write_goto_address (CORE_ADDR from, CORE_ADDR to, int size)
     }
 
   if (!emit_error)
-    write_inferior_memory (from, (unsigned char *) &insn, 4);
+    target_write_memory (from, (unsigned char *) &insn, 4);
 }
 
 /* Table of emit ops for 32-bit.  */
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index b23e3b1af6f..cd901813081 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -1036,7 +1036,7 @@ static const unsigned char s390_ft_exit_gpr_zarch[] = {
 static void
 append_insns (CORE_ADDR *to, size_t len, const unsigned char *buf)
 {
-  write_inferior_memory (*to, buf, len);
+  target_write_memory (*to, buf, len);
   *to += len;
 }
 
@@ -1793,7 +1793,7 @@ s390_write_goto_address (CORE_ADDR from, CORE_ADDR to, int size)
     }
 
   memcpy (buf, &sdiff, sizeof sdiff);
-  write_inferior_memory (from, buf, sizeof sdiff);
+  target_write_memory (from, buf, sizeof sdiff);
 }
 
 /* Preparation for emitting a literal pool of given size.  Loads the address
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index cb0169c4bd5..cafff6b109b 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -1007,7 +1007,7 @@ x86_supports_tracepoints (void)
 static void
 append_insns (CORE_ADDR *to, size_t len, const unsigned char *buf)
 {
-  write_inferior_memory (*to, buf, len);
+  target_write_memory (*to, buf, len);
   *to += len;
 }
 
@@ -1385,7 +1385,7 @@ i386_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr,
       offset = *jump_entry - (*trampoline + sizeof (jump_insn));
       memcpy (buf, jump_insn, sizeof (jump_insn));
       memcpy (buf + 1, &offset, 4);
-      write_inferior_memory (*trampoline, buf, sizeof (jump_insn));
+      target_write_memory (*trampoline, buf, sizeof (jump_insn));
 
       /* Use a 16-bit relative jump instruction to jump to the trampoline.  */
       offset = (*trampoline - (tpaddr + sizeof (small_jump_insn))) & 0xffff;
@@ -1780,7 +1780,7 @@ amd64_write_goto_address (CORE_ADDR from, CORE_ADDR to, int size)
     }
 
   memcpy (buf, &diff, sizeof (int));
-  write_inferior_memory (from, buf, sizeof (int));
+  target_write_memory (from, buf, sizeof (int));
 }
 
 static void
@@ -2398,7 +2398,7 @@ i386_write_goto_address (CORE_ADDR from, CORE_ADDR to, int size)
     }
 
   memcpy (buf, &diff, sizeof (int));
-  write_inferior_memory (from, buf, sizeof (int));
+  target_write_memory (from, buf, sizeof (int));
 }
 
 static void
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 6327218cf66..582fcac1632 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -401,15 +401,15 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
   int err;
 
   /* Since there can be trap breakpoints inserted in the same address
-     range, we use `write_inferior_memory', which takes care of
+     range, we use `target_write_memory', which takes care of
      layering breakpoints on top of fast tracepoints, and on top of
      the buffer we pass it.  This works because the caller has already
      either unlinked the breakpoint or marked it uninserted.  Also
      note that we need to pass the current shadow contents, because
-     write_inferior_memory updates any shadow memory with what we pass
+     target_write_memory updates any shadow memory with what we pass
      here, and we want that to be a nop.  */
   memcpy (buf, bp->old_data, bp_size (bp));
-  err = write_inferior_memory (bp->pc, buf, bp_size (bp));
+  err = target_write_memory (bp->pc, buf, bp_size (bp));
   if (err != 0)
     {
       if (debug_threads)
@@ -578,17 +578,17 @@ delete_fast_tracepoint_jump (struct fast_tracepoint_jump *todel)
 	      *bp_link = bp->next;
 
 	      /* Since there can be breakpoints inserted in the same
-		 address range, we use `write_inferior_memory', which
+		 address range, we use `target_write_memory', which
 		 takes care of layering breakpoints on top of fast
 		 tracepoints, and on top of the buffer we pass it.
 		 This works because we've already unlinked the fast
 		 tracepoint jump above.  Also note that we need to
 		 pass the current shadow contents, because
-		 write_inferior_memory updates any shadow memory with
+		 target_write_memory updates any shadow memory with
 		 what we pass here, and we want that to be a nop.  */
 	      buf = (unsigned char *) alloca (bp->length);
 	      memcpy (buf, fast_tracepoint_jump_shadow (bp), bp->length);
-	      ret = write_inferior_memory (bp->pc, buf, bp->length);
+	      ret = target_write_memory (bp->pc, buf, bp->length);
 	      if (ret != 0)
 		{
 		  /* Something went wrong, relink the jump.  */
@@ -672,14 +672,14 @@ set_fast_tracepoint_jump (CORE_ADDR where,
   proc->fast_tracepoint_jumps = jp;
 
   /* Since there can be trap breakpoints inserted in the same address
-     range, we use use `write_inferior_memory', which takes care of
+     range, we use use `target_write_memory', which takes care of
      layering breakpoints on top of fast tracepoints, on top of the
      buffer we pass it.  This works because we've already linked in
      the fast tracepoint jump above.  Also note that we need to pass
-     the current shadow contents, because write_inferior_memory
+     the current shadow contents, because target_write_memory
      updates any shadow memory with what we pass here, and we want
      that to be a nop.  */
-  err = write_inferior_memory (where, buf, length);
+  err = target_write_memory (where, buf, length);
   if (err != 0)
     {
       if (debug_threads)
@@ -721,17 +721,17 @@ uninsert_fast_tracepoint_jumps_at (CORE_ADDR pc)
       jp->inserted = 0;
 
       /* Since there can be trap breakpoints inserted in the same
-	 address range, we use use `write_inferior_memory', which
+	 address range, we use use `target_write_memory', which
 	 takes care of layering breakpoints on top of fast
 	 tracepoints, and on top of the buffer we pass it.  This works
 	 because we've already marked the fast tracepoint fast
 	 tracepoint jump uninserted above.  Also note that we need to
 	 pass the current shadow contents, because
-	 write_inferior_memory updates any shadow memory with what we
+	 target_write_memory updates any shadow memory with what we
 	 pass here, and we want that to be a nop.  */
       buf = (unsigned char *) alloca (jp->length);
       memcpy (buf, fast_tracepoint_jump_shadow (jp), jp->length);
-      err = write_inferior_memory (jp->pc, buf, jp->length);
+      err = target_write_memory (jp->pc, buf, jp->length);
       if (err != 0)
 	{
 	  jp->inserted = 1;
@@ -769,16 +769,16 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
   jp->inserted = 1;
 
   /* Since there can be trap breakpoints inserted in the same address
-     range, we use `write_inferior_memory', which takes care of
+     range, we use `target_write_memory', which takes care of
      layering breakpoints on top of fast tracepoints, and on top of
      the buffer we pass it.  This works because we've already marked
      the fast tracepoint jump inserted above.  Also note that we need
      to pass the current shadow contents, because
-     write_inferior_memory updates any shadow memory with what we pass
+     target_write_memory updates any shadow memory with what we pass
      here, and we want that to be a nop.  */
   buf = (unsigned char *) alloca (jp->length);
   memcpy (buf, fast_tracepoint_jump_shadow (jp), jp->length);
-  err = write_inferior_memory (where, buf, jp->length);
+  err = target_write_memory (where, buf, jp->length);
   if (err != 0)
     {
       jp->inserted = 0;
diff --git a/gdb/gdbserver/proc-service.c b/gdb/gdbserver/proc-service.c
index a8bd1e88a54..c5ebff208a9 100644
--- a/gdb/gdbserver/proc-service.c
+++ b/gdb/gdbserver/proc-service.c
@@ -91,7 +91,7 @@ ps_err_e
 ps_pdwrite (gdb_ps_prochandle_t ph, psaddr_t addr,
 	    gdb_ps_write_buf_t buf, gdb_ps_size_t size)
 {
-  if (write_inferior_memory ((uintptr_t) addr, (const gdb_byte *) buf, size)
+  if (target_write_memory ((uintptr_t) addr, (const gdb_byte *) buf, size)
       != 0)
     return PS_ERR;
   return PS_OK;
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index d965b6fe644..665fc66c53d 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1636,7 +1636,7 @@ relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
 	{
 	  if (decode_X_packet (&cs.own_buf[1], len - 1, &mem_addr,
 			       &mem_len, &mem_buf) < 0
-	      || write_inferior_memory (mem_addr, mem_buf, mem_len) != 0)
+	      || target_write_memory (mem_addr, mem_buf, mem_len) != 0)
 	    write_enn (cs.own_buf);
 	  else
 	    write_ok (cs.own_buf);
@@ -1644,7 +1644,7 @@ relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
       else
 	{
 	  decode_M_packet (&cs.own_buf[1], &mem_addr, &mem_len, &mem_buf);
-	  if (write_inferior_memory (mem_addr, mem_buf, mem_len) == 0)
+	  if (target_write_memory (mem_addr, mem_buf, mem_len) == 0)
 	    write_ok (cs.own_buf);
 	  else
 	    write_enn (cs.own_buf);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5b14883f7a8..127cd3840bc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1026,7 +1026,7 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
       if (ret == 0)
 	{
 	  if (set_desired_thread ())
-	    ret = write_inferior_memory (memaddr, myaddr, len);
+	    ret = target_write_memory (memaddr, myaddr, len);
 	  else
 	    ret = EIO;
 	  done_accessing_memory ();
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 2587d8aa550..6f6b448ecf4 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -146,9 +146,11 @@ target_read_uint32 (CORE_ADDR memaddr, uint32_t *result)
   return read_inferior_memory (memaddr, (gdb_byte *) result, sizeof (*result));
 }
 
+/* See target/target.h.  */
+
 int
-write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
-		       int len)
+target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
+		     ssize_t len)
 {
   /* Make a copy of the data because check_mem_write may need to
      update it.  */
@@ -157,14 +159,6 @@ write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
   return (*the_target->write_memory) (memaddr, buffer.data (), len);
 }
 
-/* See target/target.h.  */
-
-int
-target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
-{
-  return write_inferior_memory (memaddr, myaddr, len);
-}
-
 ptid_t
 mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
 	int connected_wait)
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 434bfa9f3cb..67167cca2d3 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -167,7 +167,7 @@ struct target_ops
   int (*read_memory) (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
   /* Write memory to the inferior process.  This should generally be
-     called through write_inferior_memory, which handles breakpoint shadowing.
+     called through target_write_memory, which handles breakpoint shadowing.
 
      Write LEN bytes from the buffer at MYADDR to MEMADDR.
 
@@ -726,9 +726,6 @@ void done_accessing_memory (void);
 
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
-int write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
-			   int len);
-
 int set_desired_thread ();
 
 const char *target_pid_to_str (ptid_t);
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 09e7d5f8fec..0d0263956d3 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -440,26 +440,26 @@ static int
 write_inferior_data_pointer (CORE_ADDR symaddr, CORE_ADDR val)
 {
   void *pval = (void *) (uintptr_t) val;
-  return write_inferior_memory (symaddr,
+  return target_write_memory (symaddr,
 				(unsigned char *) &pval, sizeof (pval));
 }
 
 static int
 write_inferior_integer (CORE_ADDR symaddr, int val)
 {
-  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
+  return target_write_memory (symaddr, (unsigned char *) &val, sizeof (val));
 }
 
 static int
 write_inferior_int8 (CORE_ADDR symaddr, int8_t val)
 {
-  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
+  return target_write_memory (symaddr, (unsigned char *) &val, sizeof (val));
 }
 
 static int
 write_inferior_uinteger (CORE_ADDR symaddr, unsigned int val)
 {
-  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
+  return target_write_memory (symaddr, (unsigned char *) &val, sizeof (val));
 }
 
 static CORE_ADDR target_malloc (ULONGEST size);
@@ -517,7 +517,7 @@ m_tracepoint_action_download (const struct tracepoint_action *action)
 {
   CORE_ADDR ipa_action = target_malloc (sizeof (struct collect_memory_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) action,
+  target_write_memory (ipa_action, (unsigned char *) action,
 			 sizeof (struct collect_memory_action));
 
   return ipa_action;
@@ -540,7 +540,7 @@ r_tracepoint_action_download (const struct tracepoint_action *action)
 {
   CORE_ADDR ipa_action = target_malloc (sizeof (struct collect_registers_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) action,
+  target_write_memory (ipa_action, (unsigned char *) action,
 			 sizeof (struct collect_registers_action));
 
   return ipa_action;
@@ -560,7 +560,7 @@ x_tracepoint_action_download (const struct tracepoint_action *action)
   CORE_ADDR ipa_action = target_malloc (sizeof (struct eval_expr_action));
   CORE_ADDR expr;
 
-  write_inferior_memory (ipa_action, (unsigned char *) action,
+  target_write_memory (ipa_action, (unsigned char *) action,
 			 sizeof (struct eval_expr_action));
   expr = download_agent_expr (((struct eval_expr_action *) action)->expr);
   write_inferior_data_pointer (ipa_action
@@ -608,7 +608,7 @@ l_tracepoint_action_download (const struct tracepoint_action *action)
   CORE_ADDR ipa_action
     = target_malloc (sizeof (struct collect_static_trace_data_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) action,
+  target_write_memory (ipa_action, (unsigned char *) action,
 			 sizeof (struct collect_static_trace_data_action));
 
   return ipa_action;
@@ -1458,14 +1458,14 @@ clear_inferior_trace_buffer (void)
   ipa_trace_buffer_ctrl.wrap = ipa_trace_buffer_hi;
 
   /* A traceframe with zeroed fields marks the end of trace data.  */
-  write_inferior_memory (ipa_sym_addrs.addr_trace_buffer_ctrl,
+  target_write_memory (ipa_sym_addrs.addr_trace_buffer_ctrl,
 			 (unsigned char *) &ipa_trace_buffer_ctrl,
 			 sizeof (ipa_trace_buffer_ctrl));
 
   write_inferior_uinteger (ipa_sym_addrs.addr_trace_buffer_ctrl_curr, 0);
 
   /* A traceframe with zeroed fields marks the end of trace data.  */
-  write_inferior_memory (ipa_trace_buffer_lo,
+  target_write_memory (ipa_trace_buffer_lo,
 			 (unsigned char *) &ipa_traceframe,
 			 sizeof (ipa_traceframe));
 
@@ -6009,12 +6009,12 @@ download_agent_expr (struct agent_expr *expr)
   CORE_ADDR expr_bytes;
 
   expr_addr = target_malloc (sizeof (*expr));
-  write_inferior_memory (expr_addr, (unsigned char *) expr, sizeof (*expr));
+  target_write_memory (expr_addr, (unsigned char *) expr, sizeof (*expr));
 
   expr_bytes = target_malloc (expr->length);
   write_inferior_data_pointer (expr_addr + offsetof (struct agent_expr, bytes),
 			       expr_bytes);
-  write_inferior_memory (expr_bytes, expr->bytes, expr->length);
+  target_write_memory (expr_bytes, expr->bytes, expr->length);
 
   return expr_addr;
 }
@@ -6067,7 +6067,7 @@ download_tracepoint_1 (struct tracepoint *tpoint)
      tracepoints before clearing our own copy.  */
   target_tracepoint.hit_count = 0;
 
-  write_inferior_memory (tpptr, (unsigned char *) &target_tracepoint,
+  target_write_memory (tpptr, (unsigned char *) &target_tracepoint,
 			 sizeof (target_tracepoint));
 
   if (tpoint->cond)
@@ -6279,14 +6279,14 @@ download_trace_state_variables (void)
 	 Assume no next, fixup when needed.  */
       target_tsv.next = NULL;
 
-      write_inferior_memory (ptr, (unsigned char *) &target_tsv,
+      target_write_memory (ptr, (unsigned char *) &target_tsv,
 			     sizeof (target_tsv));
 
       if (tsv->name != NULL)
 	{
 	  size_t size = strlen (tsv->name) + 1;
 	  CORE_ADDR name_addr = target_malloc (size);
-	  write_inferior_memory (name_addr,
+	  target_write_memory (name_addr,
 				 (unsigned char *) tsv->name, size);
 	  write_inferior_data_pointer (ptr
 				       + offsetof (struct trace_state_variable,
@@ -6548,7 +6548,7 @@ upload_fast_traceframes (void)
 		   (int) (ipa_trace_buffer_hi - ipa_trace_buffer_lo));
     }
 
-  if (write_inferior_memory (ipa_trace_buffer_ctrl_addr,
+  if (target_write_memory (ipa_trace_buffer_ctrl_addr,
 			     (unsigned char *) &ipa_trace_buffer_ctrl,
 			     sizeof (struct ipa_trace_buffer_control)))
     return;
-- 
2.20.1

[PATCH 1/2] Simplify write_inferior_memory

[Tom Tromey]

gdbserver's write_inferior_memory uses a static variable to avoid
memory leaks, and has a comment referring to the lack of cleanups.
This patch removes this comment and the code in favor of a
straightforward use of std::vector.

gdb/gdbserver/ChangeLog
2019-08-14  Tom Tromey  <tromey@adacore.com>

	* target.c (write_inferior_memory): Use std::vector.
---
 gdb/gdbserver/ChangeLog |  4 ++++
 gdb/gdbserver/target.c  | 22 +++++-----------------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 25f91eeba73..2587d8aa550 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -150,23 +150,11 @@ int
 write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
 		       int len)
 {
-  /* Lacking cleanups, there is some potential for a memory leak if the
-     write fails and we go through error().  Make sure that no more than
-     one buffer is ever pending by making BUFFER static.  */
-  static unsigned char *buffer = 0;
-  int res;
-
-  if (buffer != NULL)
-    free (buffer);
-
-  buffer = (unsigned char *) xmalloc (len);
-  memcpy (buffer, myaddr, len);
-  check_mem_write (memaddr, buffer, myaddr, len);
-  res = (*the_target->write_memory) (memaddr, buffer, len);
-  free (buffer);
-  buffer = NULL;
-
-  return res;
+  /* Make a copy of the data because check_mem_write may need to
+     update it.  */
+  std::vector<unsigned char> buffer (myaddr, myaddr + len);
+  check_mem_write (memaddr, buffer.data (), myaddr, len);
+  return (*the_target->write_memory) (memaddr, buffer.data (), len);
 }
 
 /* See target/target.h.  */
-- 
2.20.1

Re: [PATCH 1/2] Simplify write_inferior_memory

[Kevin Buettner]

On Wed, 14 Aug 2019 10:48:13 -0600
Tom Tromey <tromey@adacore.com> wrote:

> gdbserver's write_inferior_memory uses a static variable to avoid
> memory leaks, and has a comment referring to the lack of cleanups.
> This patch removes this comment and the code in favor of a
> straightforward use of std::vector.
> 
> gdb/gdbserver/ChangeLog
> 2019-08-14  Tom Tromey  <tromey@adacore.com>
> 
> 	* target.c (write_inferior_memory): Use std::vector.

LGTM.

Kevin

Re: [PATCH 2/2] Replace write_inferior_memory with target_write_memory

[Kevin Buettner]

On Wed, 14 Aug 2019 10:48:14 -0600
Tom Tromey <tromey@adacore.com> wrote:

> target_write_memory is just a simple wrapper for
> write_inferior_memory.  Because target_write_memory is needed for
> gdbsupport, and because gdb uses the name "target_write_memory"
> everywhere, this patch renames write_inferior_memory and removes the
> wrapper.  I think this brings gdb and gdbserver slightly more in sync.
> 
> gdb/gdbserver/ChangeLog
> 2019-08-14  Tom Tromey  <tromey@adacore.com>
> 
> 	* tracepoint.c (write_inferior_data_pointer)
> 	(write_inferior_integer, write_inferior_int8)
> 	(write_inferior_uinteger, m_tracepoint_action_download)
> 	(r_tracepoint_action_download, x_tracepoint_action_download)
> 	(l_tracepoint_action_download, clear_inferior_trace_buffer)
> 	(download_agent_expr, download_tracepoint_1)
> 	(download_trace_state_variables, upload_fast_traceframes): Update.
> 	* server.c (gdb_write_memory): Update.
> 	* remote-utils.c (relocate_instruction): Update.
> 	* proc-service.c (ps_pdwrite): Update.
> 	* mem-break.c (remove_memory_breakpoint)
> 	(delete_fast_tracepoint_jump, set_fast_tracepoint_jump)
> 	(uninsert_fast_tracepoint_jumps_at)
> 	(reinsert_fast_tracepoint_jumps_at): Update.
> 	* linux-x86-low.c (append_insns)
> 	(i386_install_fast_tracepoint_jump_pad)
> 	(amd64_write_goto_address, i386_write_goto_address): Update.
> 	* linux-s390-low.c (append_insns, s390_write_goto_address):
> 	Update.
> 	* linux-ppc-low.c (ppc_relocate_instruction)
> 	(ppc_install_fast_tracepoint_jump_pad, emit_insns)
> 	(ppc_write_goto_address): Update.
> 	* linux-aarch64-low.c (append_insns): Update.
> 	* target.h (struct target_ops): Update.
> 	(write_inferior_memory): Don't declare.
> 	* target.c (target_write_memory): Rename from
> 	write_inferior_memory.  Remove old target_write_memory.

LGTM.

Kevin

Re: [PATCH 1/2] Simplify write_inferior_memory

[Pedro Alves]

On 8/14/19 5:48 PM, Tom Tromey wrote:
> +  std::vector<unsigned char> buffer (myaddr, myaddr + len);

gdb::byte_vector 

> +  check_mem_write (memaddr, buffer.data (), myaddr, len);
> +  return (*the_target->write_memory) (memaddr, buffer.data (), len);

Thanks,
Pedro Alves

Re: [PATCH 1/2] Simplify write_inferior_memory

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 8/14/19 5:48 PM, Tom Tromey wrote:
>> +  std::vector<unsigned char> buffer (myaddr, myaddr + len);

Pedro> gdb::byte_vector 

Thanks, I forgot about that.  I'll fix it momentarily.

Tom