[PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[Tankut Baris Aktemur]

Hello,

Gdbserver's regcache is defined and used in a way that it is either
invalid or fetches all the registers from the target prior to being
used.  It also seems some regcache functions have two different contracts
based on argument values (e.g. a buffer being nullptr).

This is an attempt to refactor the regcache in gdbserver to

  - convert several free functions to methods.
  - use and update register statuses more consistently.
  - allow populating register values gradually, instead of having to
    fetch all register values from the target.

The last item above is utilized in our (Intel) downstream debugger.
No gdbserver low target is modified to utilize that feature in this
series.

Regression-tested on X86_64 Linux using the native-gdbserver and
native-extended-gdbserver board files.

Tankut Baris Aktemur (26):
  gdbserver: convert init_register_cache into regcache::initialize
  gdbserver: convert new_register_cache into a regcache constructor
  gdbserver: by-pass regcache to access tdesc only
  gdbserver: boolify and defaultize the 'fetch' parameter of
    get_thread_regcache
  gdbserver: add a pointer to the owner thread in regcache
  gdbserver: turn part of get_thread_regcache into regcache::fetch
  gdbserver: convert regcache_cpy into regcache::copy_from
  gdbserver: convert free_register_cache into a destructor of regcache
  gdbserver: extract out regcache::invalidate and regcache::discard
  gdbserver: convert registers_to_string into
    regcache::registers_to_string
  gdbserver: convert registers_from_string into
    regcache::registers_from_string
  gdbserver: convert supply_regblock to regcache::supply_regblock
  gdbserver: convert register_data into regcache::register_data
  gdbserver: introduce and use regcache::set_register_status
  gdbserver: check for nullptr condition in
    regcache::get_register_status
  gdbserver: boolify regcache fields
  gdbserver: rename regcache's registers_valid to registers_fetched
  gdbsupport: fix a typo in a comment in common-regcache.h
  gdbserver: fix the declared type of register_status in regcache
  gdbserver: make some regcache fields private
  gdbserver: use REG_UNKNOWN for a regcache's register statuses
  gdbserver: zero-out register values in regcache-discard
  gdbserver: set register statuses in registers_from_string
  gdbserver: return tracked register status in
    regcache_raw_read_unsigned
  gdbserver: refuse null argument in regcache::supply_regblock
  gdbserver: allow gradually populating and selectively storing a
    regcache

 gdbserver/gdbthread.h          |   2 +-
 gdbserver/linux-aarch32-low.cc |   2 +-
 gdbserver/linux-low.cc         |  18 +-
 gdbserver/linux-ppc-low.cc     |  12 +-
 gdbserver/linux-s390-low.cc    |  14 +-
 gdbserver/linux-x86-low.cc     |   9 +-
 gdbserver/mem-break.cc         |   4 +-
 gdbserver/proc-service.cc      |   2 +-
 gdbserver/regcache.cc          | 305 ++++++++++++++++++++-------------
 gdbserver/regcache.h           |  92 ++++++----
 gdbserver/remote-utils.cc      |   2 +-
 gdbserver/server.cc            |  16 +-
 gdbserver/tracepoint.cc        |  25 ++-
 gdbserver/win32-low.cc         |   2 +-
 gdbsupport/common-regcache.h   |  11 +-
 15 files changed, 302 insertions(+), 214 deletions(-)

-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 01/26] gdbserver: convert init_register_cache into regcache::initialize

[Tankut Baris Aktemur]

This is a refactoring that converts the `init_register_cache` function
to a method of the regcache struct.  During this conversion, we also
change the return type to void.
---
 gdbserver/regcache.cc   | 32 +++++++++++++++-----------------
 gdbserver/regcache.h    |  7 +++----
 gdbserver/tracepoint.cc |  9 ++++-----
 3 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 0b1141662ac..7987c406ab4 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -113,10 +113,9 @@ regcache_invalidate (void)
 
 #endif
 
-struct regcache *
-init_register_cache (struct regcache *regcache,
-		     const struct target_desc *tdesc,
-		     unsigned char *regbuf)
+void
+regcache::initialize (const target_desc *tdesc,
+		      unsigned char *regbuf)
 {
   if (regbuf == NULL)
     {
@@ -125,13 +124,13 @@ init_register_cache (struct regcache *regcache,
 	 created, in case there are registers the target never
 	 fetches.  This way they'll read as zero instead of
 	 garbage.  */
-      regcache->tdesc = tdesc;
-      regcache->registers
+      this->tdesc = tdesc;
+      this->registers
 	= (unsigned char *) xcalloc (1, tdesc->registers_size);
-      regcache->registers_owned = 1;
-      regcache->register_status
+      this->registers_owned = 1;
+      this->register_status
 	= (unsigned char *) xmalloc (tdesc->reg_defs.size ());
-      memset ((void *) regcache->register_status, REG_UNAVAILABLE,
+      memset ((void *) this->register_status, REG_UNAVAILABLE,
 	      tdesc->reg_defs.size ());
 #else
       gdb_assert_not_reached ("can't allocate memory from the heap");
@@ -139,17 +138,15 @@ init_register_cache (struct regcache *regcache,
     }
   else
     {
-      regcache->tdesc = tdesc;
-      regcache->registers = regbuf;
-      regcache->registers_owned = 0;
+      this->tdesc = tdesc;
+      this->registers = regbuf;
+      this->registers_owned = 0;
 #ifndef IN_PROCESS_AGENT
-      regcache->register_status = NULL;
+      this->register_status = nullptr;
 #endif
     }
 
-  regcache->registers_valid = 0;
-
-  return regcache;
+  this->registers_valid = 0;
 }
 
 #ifndef IN_PROCESS_AGENT
@@ -160,8 +157,9 @@ new_register_cache (const struct target_desc *tdesc)
   struct regcache *regcache = new struct regcache;
 
   gdb_assert (tdesc->registers_size != 0);
+  regcache->initialize (tdesc, nullptr);
 
-  return init_register_cache (regcache, tdesc, NULL);
+  return regcache;
 }
 
 void
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 7248bcf5808..15b7e2b4dff 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -46,6 +46,9 @@ struct regcache : public reg_buffer_common
   unsigned char *register_status = nullptr;
 #endif
 
+  /* Init the regcache data.  */
+  void initialize (const target_desc *tdesc, unsigned char *regbuf);
+
   /* See gdbsupport/common-regcache.h.  */
   enum register_status get_register_status (int regnum) const override;
 
@@ -59,10 +62,6 @@ struct regcache : public reg_buffer_common
   bool raw_compare (int regnum, const void *buf, int offset) const override;
 };
 
-struct regcache *init_register_cache (struct regcache *regcache,
-				      const struct target_desc *tdesc,
-				      unsigned char *regbuf);
-
 void regcache_cpy (struct regcache *dst, struct regcache *src);
 
 /* Create a new register cache for INFERIOR.  */
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 3f60989e4c7..e4715b95eb3 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4706,7 +4706,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
       if (!fctx->regcache_initted)
 	{
 	  fctx->regcache_initted = 1;
-	  init_register_cache (&fctx->regcache, ipa_tdesc, fctx->regspace);
+	  fctx->regcache.initialize (ipa_tdesc, fctx->regspace);
 	  supply_regblock (&fctx->regcache, NULL);
 	  supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs);
 	}
@@ -4721,7 +4721,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
       if (!sctx->regcache_initted)
 	{
 	  sctx->regcache_initted = 1;
-	  init_register_cache (&sctx->regcache, ipa_tdesc, sctx->regspace);
+	  sctx->regcache.initialize (ipa_tdesc, sctx->regspace);
 	  supply_regblock (&sctx->regcache, NULL);
 	  /* Pass down the tracepoint address, because REGS doesn't
 	     include the PC, but we know what it must have been.  */
@@ -4799,8 +4799,7 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 
 	/* Wrap the regblock in a register cache (in the stack, we
 	   don't want to malloc here).  */
-	init_register_cache (&tregcache, context_regcache->tdesc,
-			     regspace + 1);
+	tregcache.initialize (context_regcache->tdesc, regspace + 1);
 
 	/* Copy the register data to the regblock.  */
 	regcache_cpy (&tregcache, context_regcache);
@@ -5207,7 +5206,7 @@ traceframe_get_pc (struct traceframe *tframe)
   if (dataptr == NULL)
     return 0;
 
-  init_register_cache (&regcache, tdesc, dataptr);
+  regcache.initialize (tdesc, dataptr);
   return regcache_read_pc (&regcache);
 }
 
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 02/26] gdbserver: convert new_register_cache into a regcache constructor

[Tankut Baris Aktemur]

Convert the `new_register_cache` function into a constructor of the
regcache struct.  Also define a default ctor because it is used in
other places, in particular, tracepoint.cc.
---
 gdbserver/regcache.cc | 11 +++--------
 gdbserver/regcache.h  |  8 ++++----
 gdbserver/server.cc   |  2 +-
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 7987c406ab4..1410dd96551 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -43,7 +43,7 @@ get_thread_regcache (struct thread_info *thread, int fetch)
 
       gdb_assert (proc->tdesc != NULL);
 
-      regcache = new_register_cache (proc->tdesc);
+      regcache = new struct regcache (proc->tdesc);
       set_thread_regcache_data (thread, regcache);
     }
 
@@ -151,15 +151,10 @@ regcache::initialize (const target_desc *tdesc,
 
 #ifndef IN_PROCESS_AGENT
 
-struct regcache *
-new_register_cache (const struct target_desc *tdesc)
+regcache::regcache (const target_desc *tdesc)
 {
-  struct regcache *regcache = new struct regcache;
-
   gdb_assert (tdesc->registers_size != 0);
-  regcache->initialize (tdesc, nullptr);
-
-  return regcache;
+  initialize (tdesc, nullptr);
 }
 
 void
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 15b7e2b4dff..0002726ed00 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -44,6 +44,10 @@ struct regcache : public reg_buffer_common
 #ifndef IN_PROCESS_AGENT
   /* One of REG_UNAVAILABLE or REG_VALID.  */
   unsigned char *register_status = nullptr;
+
+  /* Constructors.  */
+  regcache () = default;
+  regcache (const target_desc *tdesc);
 #endif
 
   /* Init the regcache data.  */
@@ -64,10 +68,6 @@ struct regcache : public reg_buffer_common
 
 void regcache_cpy (struct regcache *dst, struct regcache *src);
 
-/* Create a new register cache for INFERIOR.  */
-
-struct regcache *new_register_cache (const struct target_desc *tdesc);
-
 struct regcache *get_thread_regcache (struct thread_info *thread, int fetch);
 
 /* Release all memory associated with the register cache for INFERIOR.  */
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 949849b63a2..f85d7ed9cf5 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -4238,7 +4238,7 @@ process_serial_event (void)
       if (cs.current_traceframe >= 0)
 	{
 	  struct regcache *regcache
-	    = new_register_cache (current_target_desc ());
+	    = new struct regcache (current_target_desc ());
 
 	  if (fetch_traceframe_registers (cs.current_traceframe,
 					  regcache, -1) == 0)
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 03/26] gdbserver: by-pass regcache to access tdesc only

[Tankut Baris Aktemur]

The `get_thread_regcache` function has a `fetch` option to skip
fetching the registers from the target.  It seems this option is set
to false only at uses where we just need to access the tdesc through
the regcache of the current thread, as in

  struct regcache *regcache = get_thread_regcache (current_thread, 0);
  ... regcache->tdesc ...

Since the tdesc of a regcache is set from the process of the thread
that owns the regcache, we can simplify the code to access the tdesc
via the process, as in

  ... current_process ()->tdesc ...

This is intended to be a refactoring with no behavioral change.

Tested only for the linux-x86-low target.
---
 gdbserver/linux-ppc-low.cc  | 10 +++-------
 gdbserver/linux-s390-low.cc | 14 ++++----------
 gdbserver/linux-x86-low.cc  |  7 ++-----
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index f8dd770b8eb..96c1da4d905 100644
--- a/gdbserver/linux-ppc-low.cc
+++ b/gdbserver/linux-ppc-low.cc
@@ -1609,8 +1609,7 @@ ppc_target::install_fast_tracepoint_jump_pad (CORE_ADDR tpoint,
   const CORE_ADDR entryaddr = *jump_entry;
   int rsz, min_frame, frame_size, tp_reg;
 #ifdef __powerpc64__
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-  int is_64 = register_size (regcache->tdesc, 0) == 8;
+  int is_64 = register_size (current_process ()->tdesc, 0) == 8;
   int is_opd = is_64 && !is_elfv2_inferior ();
 #else
   int is_64 = 0, is_opd = 0;
@@ -3381,9 +3380,7 @@ emit_ops *
 ppc_target::emit_ops ()
 {
 #ifdef __powerpc64__
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-
-  if (register_size (regcache->tdesc, 0) == 8)
+  if (register_size (current_process ()->tdesc, 0) == 8)
     {
       if (is_elfv2_inferior ())
 	return &ppc64v2_emit_ops_impl;
@@ -3399,8 +3396,7 @@ ppc_target::emit_ops ()
 int
 ppc_target::get_ipa_tdesc_idx ()
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-  const struct target_desc *tdesc = regcache->tdesc;
+  const struct target_desc *tdesc = current_process ()->tdesc;
 
 #ifdef __powerpc64__
   if (tdesc == tdesc_powerpc_64l)
diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
index 48f64ef7bb3..11f207ea2f1 100644
--- a/gdbserver/linux-s390-low.cc
+++ b/gdbserver/linux-s390-low.cc
@@ -797,9 +797,7 @@ s390_target::low_get_thread_area (int lwpid, CORE_ADDR *addrp)
 {
   CORE_ADDR res = ptrace (PTRACE_PEEKUSER, lwpid, (long) PT_ACR0, (long) 0);
 #ifdef __s390x__
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-
-  if (register_size (regcache->tdesc, 0) == 4)
+  if (register_size (current_process ()->tdesc, 0) == 4)
     res &= 0xffffffffull;
 #endif
   *addrp = res;
@@ -1287,8 +1285,7 @@ s390_target::install_fast_tracepoint_jump_pad
   unsigned char jbuf[6] = { 0xc0, 0xf4, 0, 0, 0, 0 };	/* jg ... */
   CORE_ADDR buildaddr = *jump_entry;
 #ifdef __s390x__
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-  int is_64 = register_size (regcache->tdesc, 0) == 8;
+  int is_64 = register_size (current_process ()->tdesc, 0) == 8;
   int is_zarch = is_64 || have_hwcap_s390_high_gprs;
   int has_vx = have_hwcap_s390_vx;
 #else
@@ -1452,8 +1449,7 @@ s390_target::get_min_fast_tracepoint_insn_len ()
 int
 s390_target::get_ipa_tdesc_idx ()
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-  const struct target_desc *tdesc = regcache->tdesc;
+  const struct target_desc *tdesc = current_process ()->tdesc;
 
 #ifdef __s390x__
   if (tdesc == tdesc_s390x_linux64)
@@ -2840,9 +2836,7 @@ emit_ops *
 s390_target::emit_ops ()
 {
 #ifdef __s390x__
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-
-  if (register_size (regcache->tdesc, 0) == 8)
+  if (register_size (current_process ()->tdesc, 0) == 8)
     return &s390x_emit_ops;
   else
 #endif
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index 4a538b107be..e08ebacee05 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -276,9 +276,7 @@ static /*const*/ int i386_regmap[] =
 static int
 is_64bit_tdesc (thread_info *thread)
 {
-  struct regcache *regcache = get_thread_regcache (thread, 0);
-
-  return register_size (regcache->tdesc, 0) == 8;
+  return register_size (get_thread_process (thread)->tdesc, 0) == 8;
 }
 
 #endif
@@ -2958,8 +2956,7 @@ x86_target::low_supports_range_stepping ()
 int
 x86_target::get_ipa_tdesc_idx ()
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 0);
-  const struct target_desc *tdesc = regcache->tdesc;
+  const struct target_desc *tdesc = current_process ()->tdesc;
 
 #ifdef __x86_64__
   return amd64_get_ipa_tdesc_idx (tdesc);
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 04/26] gdbserver: boolify and defaultize the 'fetch' parameter of get_thread_regcache

[Tankut Baris Aktemur]

Boolify the 'fetch' parameter of the get_thread_regcache function.

All of the current uses pass true for this parameter.  Therefore, define
its default value as true and remove the argument from the uses.

We still keep the parameter, though, to give downstream targets the
option to obtain a regcache without having to fetch the whole
contents.  Our (Intel) downstream target is an example.
---
 gdbserver/linux-aarch32-low.cc |  2 +-
 gdbserver/linux-low.cc         | 18 +++++++++---------
 gdbserver/linux-ppc-low.cc     |  2 +-
 gdbserver/linux-x86-low.cc     |  2 +-
 gdbserver/mem-break.cc         |  4 ++--
 gdbserver/proc-service.cc      |  2 +-
 gdbserver/regcache.cc          |  4 ++--
 gdbserver/regcache.h           |  2 +-
 gdbserver/remote-utils.cc      |  2 +-
 gdbserver/server.cc            |  4 ++--
 gdbserver/tracepoint.cc        |  4 ++--
 gdbserver/win32-low.cc         |  2 +-
 12 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/gdbserver/linux-aarch32-low.cc b/gdbserver/linux-aarch32-low.cc
index 990a352af74..6508f587288 100644
--- a/gdbserver/linux-aarch32-low.cc
+++ b/gdbserver/linux-aarch32-low.cc
@@ -171,7 +171,7 @@ struct regs_info regs_info_aarch32 =
 int
 arm_is_thumb_mode (void)
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 1);
+  struct regcache *regcache = get_thread_regcache (current_thread);
   unsigned long cpsr;
 
   collect_register_by_name (regcache, "cpsr", &cpsr);
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index dec1944d45f..9e17e384c03 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -756,7 +756,7 @@ linux_process_target::get_pc (lwp_info *lwp)
   scoped_restore_current_thread restore_thread;
   switch_to_thread (get_lwp_thread (lwp));
 
-  struct regcache *regcache = get_thread_regcache (current_thread, 1);
+  struct regcache *regcache = get_thread_regcache (current_thread);
   CORE_ADDR pc = low_get_pc (regcache);
 
   threads_debug_printf ("pc is 0x%lx", (long) pc);
@@ -772,7 +772,7 @@ linux_process_target::get_syscall_trapinfo (lwp_info *lwp, int *sysno)
   scoped_restore_current_thread restore_thread;
   switch_to_thread (get_lwp_thread (lwp));
 
-  regcache = get_thread_regcache (current_thread, 1);
+  regcache = get_thread_regcache (current_thread);
   low_get_syscall_trapinfo (regcache, sysno);
 
   threads_debug_printf ("get_syscall_trapinfo sysno %d", *sysno);
@@ -880,7 +880,7 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
       if (pc != sw_breakpoint_pc)
 	{
 	  struct regcache *regcache
-	    = get_thread_regcache (current_thread, 1);
+	    = get_thread_regcache (current_thread);
 	  low_set_pc (regcache, sw_breakpoint_pc);
 	}
 
@@ -2052,7 +2052,7 @@ linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
 			  (PTRACE_TYPE_ARG3) 0, &info);
 		}
 
-	      regcache = get_thread_regcache (current_thread, 1);
+	      regcache = get_thread_regcache (current_thread);
 	      low_set_pc (regcache, status.tpoint_addr);
 	      lwp->stop_pc = status.tpoint_addr;
 
@@ -3053,7 +3053,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
       if (increment_pc != 0)
 	{
 	  struct regcache *regcache
-	    = get_thread_regcache (current_thread, 1);
+	    = get_thread_regcache (current_thread);
 
 	  event_child->stop_pc += increment_pc;
 	  low_set_pc (regcache, event_child->stop_pc);
@@ -3341,7 +3341,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
       if (low_supports_breakpoints ())
 	{
 	  struct regcache *regcache
-	    = get_thread_regcache (current_thread, 1);
+	    = get_thread_regcache (current_thread);
 	  low_set_pc (regcache, event_child->stop_pc);
 	}
 
@@ -3577,7 +3577,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
       if (decr_pc != 0)
 	{
 	  struct regcache *regcache
-	    = get_thread_regcache (current_thread, 1);
+	    = get_thread_regcache (current_thread);
 	  low_set_pc (regcache, event_child->stop_pc + decr_pc);
 	}
     }
@@ -3912,7 +3912,7 @@ void
 linux_process_target::install_software_single_step_breakpoints (lwp_info *lwp)
 {
   struct thread_info *thread = get_lwp_thread (lwp);
-  struct regcache *regcache = get_thread_regcache (thread, 1);
+  struct regcache *regcache = get_thread_regcache (thread);
 
   scoped_restore_current_thread restore_thread;
 
@@ -4087,7 +4087,7 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
 
   if (proc->tdesc != NULL && low_supports_breakpoints ())
     {
-      struct regcache *regcache = get_thread_regcache (current_thread, 1);
+      struct regcache *regcache = get_thread_regcache (current_thread);
 
       lwp->stop_pc = low_get_pc (regcache);
 
diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index 96c1da4d905..5c1dd6c078d 100644
--- a/gdbserver/linux-ppc-low.cc
+++ b/gdbserver/linux-ppc-low.cc
@@ -1055,7 +1055,7 @@ ppc_target::low_get_thread_area (int lwpid, CORE_ADDR *addr)
 {
   struct lwp_info *lwp = find_lwp_pid (ptid_t (lwpid));
   struct thread_info *thr = get_lwp_thread (lwp);
-  struct regcache *regcache = get_thread_regcache (thr, 1);
+  struct regcache *regcache = get_thread_regcache (thr);
   ULONGEST tp = 0;
 
 #ifdef __powerpc64__
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index e08ebacee05..c012cadcb54 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -353,7 +353,7 @@ x86_target::low_get_thread_area (int lwpid, CORE_ADDR *addr)
 
   {
     struct thread_info *thr = get_lwp_thread (lwp);
-    struct regcache *regcache = get_thread_regcache (thr, 1);
+    struct regcache *regcache = get_thread_regcache (thr);
     unsigned int desc[4];
     ULONGEST gs = 0;
     const int reg_thread_area = 3; /* bits to scale down register value.  */
diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
index c669842228d..73eeb8114eb 100644
--- a/gdbserver/mem-break.cc
+++ b/gdbserver/mem-break.cc
@@ -1228,7 +1228,7 @@ gdb_condition_true_at_breakpoint_z_type (char z_type, CORE_ADDR addr)
   if (bp->cond_list == NULL)
     return 1;
 
-  ctx.regcache = get_thread_regcache (current_thread, 1);
+  ctx.regcache = get_thread_regcache (current_thread);
   ctx.tframe = NULL;
   ctx.tpoint = NULL;
 
@@ -1350,7 +1350,7 @@ run_breakpoint_commands_z_type (char z_type, CORE_ADDR addr)
   if (bp == NULL)
     return 1;
 
-  ctx.regcache = get_thread_regcache (current_thread, 1);
+  ctx.regcache = get_thread_regcache (current_thread);
   ctx.tframe = NULL;
   ctx.tpoint = NULL;
 
diff --git a/gdbserver/proc-service.cc b/gdbserver/proc-service.cc
index 2d516a0cad7..031e9607241 100644
--- a/gdbserver/proc-service.cc
+++ b/gdbserver/proc-service.cc
@@ -113,7 +113,7 @@ ps_lgetregs (gdb_ps_prochandle_t ph, lwpid_t lwpid, prgregset_t gregset)
 
   scoped_restore_current_thread restore_thread;
   switch_to_thread (get_lwp_thread (lwp));
-  regcache = get_thread_regcache (current_thread, 1);
+  regcache = get_thread_regcache (current_thread);
   gregset_info ()->fill_function (regcache, gregset);
 
   return PS_OK;
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 1410dd96551..21c2207822c 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -24,7 +24,7 @@
 #ifndef IN_PROCESS_AGENT
 
 struct regcache *
-get_thread_regcache (struct thread_info *thread, int fetch)
+get_thread_regcache (struct thread_info *thread, bool fetch)
 {
   struct regcache *regcache;
 
@@ -67,7 +67,7 @@ get_thread_regcache (struct thread_info *thread, int fetch)
 struct regcache *
 get_thread_regcache_for_ptid (ptid_t ptid)
 {
-  return get_thread_regcache (find_thread_ptid (ptid), 1);
+  return get_thread_regcache (find_thread_ptid (ptid));
 }
 
 void
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 0002726ed00..5b2066ced4d 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -68,7 +68,7 @@ struct regcache : public reg_buffer_common
 
 void regcache_cpy (struct regcache *dst, struct regcache *src);
 
-struct regcache *get_thread_regcache (struct thread_info *thread, int fetch);
+regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
 
 /* Release all memory associated with the register cache for INFERIOR.  */
 
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 80310bc2c70..26ca621e2f1 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -1156,7 +1156,7 @@ prepare_resume_reply (char *buf, ptid_t ptid, const target_waitstatus &status)
 
 	regp = current_target_desc ()->expedite_regs;
 
-	regcache = get_thread_regcache (current_thread, 1);
+	regcache = get_thread_regcache (current_thread);
 
 	if (the_target->stopped_by_watchpoint ())
 	  {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index f85d7ed9cf5..4be12ec1c66 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -4255,7 +4255,7 @@ process_serial_event (void)
 	    write_enn (cs.own_buf);
 	  else
 	    {
-	      regcache = get_thread_regcache (current_thread, 1);
+	      regcache = get_thread_regcache (current_thread);
 	      registers_to_string (regcache, cs.own_buf);
 	    }
 	}
@@ -4272,7 +4272,7 @@ process_serial_event (void)
 	    write_enn (cs.own_buf);
 	  else
 	    {
-	      regcache = get_thread_regcache (current_thread, 1);
+	      regcache = get_thread_regcache (current_thread);
 	      registers_from_string (regcache, &cs.own_buf[1]);
 	      write_ok (cs.own_buf);
 	    }
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index e4715b95eb3..82bb74addd1 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4382,7 +4382,7 @@ tracepoint_finished_step (struct thread_info *tinfo, CORE_ADDR stop_pc)
 	       wstep->tp_number, paddress (wstep->tp_address));
 
   ctx.base.type = trap_tracepoint;
-  ctx.regcache = get_thread_regcache (tinfo, 1);
+  ctx.regcache = get_thread_regcache (tinfo);
 
   while (wstep != NULL)
     {
@@ -4543,7 +4543,7 @@ tracepoint_was_hit (struct thread_info *tinfo, CORE_ADDR stop_pc)
     return 0;
 
   ctx.base.type = trap_tracepoint;
-  ctx.regcache = get_thread_regcache (tinfo, 1);
+  ctx.regcache = get_thread_regcache (tinfo);
 
   for (tpoint = tracepoints; tpoint; tpoint = tpoint->next)
     {
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 756507709d8..5cc6a49e1f6 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -971,7 +971,7 @@ gdbserver_windows_process::handle_access_violation
 static void
 maybe_adjust_pc ()
 {
-  struct regcache *regcache = get_thread_regcache (current_thread, 1);
+  struct regcache *regcache = get_thread_regcache (current_thread);
   child_fetch_inferior_registers (regcache, -1);
 
   windows_thread_info *th
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 05/26] gdbserver: add a pointer to the owner thread in regcache

[Tankut Baris Aktemur]

Add a back-link in regcache to the thread that owns the regcache.
This will help us in future patches to refer to the right thread
object without having to rely on the global current_thread pointer.
---
 gdbserver/regcache.cc | 1 +
 gdbserver/regcache.h  | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 21c2207822c..2a8dc17ed6a 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -45,6 +45,7 @@ get_thread_regcache (struct thread_info *thread, bool fetch)
 
       regcache = new struct regcache (proc->tdesc);
       set_thread_regcache_data (thread, regcache);
+      regcache->thread = thread;
     }
 
   if (fetch && regcache->registers_valid == 0)
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 5b2066ced4d..053ed08b20f 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -33,6 +33,9 @@ struct regcache : public reg_buffer_common
   /* The regcache's target description.  */
   const struct target_desc *tdesc = nullptr;
 
+  /* Back-link to the thread to which this regcache belongs.  */
+  thread_info *thread = nullptr;
+
   /* Whether the REGISTERS buffer's contents are valid.  If false, we
      haven't fetched the registers from the target yet.  Not that this
      register cache is _not_ pass-through, unlike GDB's.  Note that
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 06/26] gdbserver: turn part of get_thread_regcache into regcache::fetch

[Tankut Baris Aktemur]

Extract out a piece of code in get_thread_regcache that fetches the
registers from the target, and turn it into a method of 'regcache'.
---
 gdbserver/regcache.cc | 23 +++++++++++++++--------
 gdbserver/regcache.h  |  3 +++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 2a8dc17ed6a..89ecdfec6f3 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -48,19 +48,26 @@ get_thread_regcache (struct thread_info *thread, bool fetch)
       regcache->thread = thread;
     }
 
-  if (fetch && regcache->registers_valid == 0)
+  if (fetch)
+    regcache->fetch ();
+
+  return regcache;
+}
+
+void
+regcache::fetch ()
+{
+  if (registers_valid == 0)
     {
       scoped_restore_current_thread restore_thread;
+      gdb_assert (this->thread != nullptr);
+      switch_to_thread (this->thread);
 
-      switch_to_thread (thread);
       /* Invalidate all registers, to prevent stale left-overs.  */
-      memset (regcache->register_status, REG_UNAVAILABLE,
-	      regcache->tdesc->reg_defs.size ());
-      fetch_inferior_registers (regcache, -1);
-      regcache->registers_valid = 1;
+      memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
+      fetch_inferior_registers (this, -1);
+      registers_valid = 1;
     }
-
-  return regcache;
 }
 
 /* See gdbsupport/common-regcache.h.  */
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 053ed08b20f..7eae6cb724a 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -67,6 +67,9 @@ struct regcache : public reg_buffer_common
 
   /* See gdbsupport/common-regcache.h.  */
   bool raw_compare (int regnum, const void *buf, int offset) const override;
+
+  /* Fetch the registers from the target, if not done already.  */
+  void fetch ();
 };
 
 void regcache_cpy (struct regcache *dst, struct regcache *src);
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 07/26] gdbserver: convert regcache_cpy into regcache::copy_from

[Tankut Baris Aktemur]

Convert the free `regcache_cpy` function to a method of the
regcache struct.
---
 gdbserver/regcache.cc   | 16 ++++++++--------
 gdbserver/regcache.h    |  5 +++--
 gdbserver/tracepoint.cc |  2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 89ecdfec6f3..be01df342bb 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -180,19 +180,19 @@ free_register_cache (struct regcache *regcache)
 #endif
 
 void
-regcache_cpy (struct regcache *dst, struct regcache *src)
+regcache::copy_from (regcache *src)
 {
-  gdb_assert (src != NULL && dst != NULL);
-  gdb_assert (src->tdesc == dst->tdesc);
-  gdb_assert (src != dst);
+  gdb_assert (src != nullptr);
+  gdb_assert (src->tdesc == this->tdesc);
+  gdb_assert (src != this);
 
-  memcpy (dst->registers, src->registers, src->tdesc->registers_size);
+  memcpy (this->registers, src->registers, src->tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
-  if (dst->register_status != NULL && src->register_status != NULL)
-    memcpy (dst->register_status, src->register_status,
+  if (this->register_status != nullptr && src->register_status != nullptr)
+    memcpy (this->register_status, src->register_status,
 	    src->tdesc->reg_defs.size ());
 #endif
-  dst->registers_valid = src->registers_valid;
+  this->registers_valid = src->registers_valid;
 }
 
 /* Return a reference to the description of register N.  */
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 7eae6cb724a..32b3a8dccfc 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -70,9 +70,10 @@ struct regcache : public reg_buffer_common
 
   /* Fetch the registers from the target, if not done already.  */
   void fetch ();
-};
 
-void regcache_cpy (struct regcache *dst, struct regcache *src);
+  /* Copy the contents of SRC into this regcache.  */
+  void copy_from (regcache *src);
+};
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
 
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 82bb74addd1..26003422a70 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4802,7 +4802,7 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 	tregcache.initialize (context_regcache->tdesc, regspace + 1);
 
 	/* Copy the register data to the regblock.  */
-	regcache_cpy (&tregcache, context_regcache);
+	tregcache.copy_from (context_regcache);
 
 #ifndef IN_PROCESS_AGENT
 	/* On some platforms, trap-based tracepoints will have the PC
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 08/26] gdbserver: convert free_register_cache into a destructor of regcache

[Tankut Baris Aktemur]

Convert the `free_register_cache` function into a destructor of the
regcache struct.  In one place, we completely remove the call to free
the regcache object by stack-allocating the object.

We also delete the copy constructor explicitly to prevent the risk of
copying regcaches and then double-freeing buffers when they are
destructed.
---
 gdbserver/gdbthread.h |  2 +-
 gdbserver/regcache.cc | 15 +++++----------
 gdbserver/regcache.h  |  9 +++++----
 gdbserver/server.cc   |  8 +++-----
 4 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 493e1dbf6cb..cfd81870af9 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -35,7 +35,7 @@ struct thread_info
 
   ~thread_info ()
   {
-    free_register_cache (this->regcache_data);
+    delete this->regcache_data;
   }
 
   /* The id of this thread.  */
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index be01df342bb..7b6337166ad 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -165,16 +165,11 @@ regcache::regcache (const target_desc *tdesc)
   initialize (tdesc, nullptr);
 }
 
-void
-free_register_cache (struct regcache *regcache)
+regcache::~regcache ()
 {
-  if (regcache)
-    {
-      if (regcache->registers_owned)
-	free (regcache->registers);
-      free (regcache->register_status);
-      delete regcache;
-    }
+  if (registers_owned)
+    free (registers);
+  free (register_status);
 }
 
 #endif
@@ -280,7 +275,7 @@ free_register_cache_thread (struct thread_info *thread)
   if (regcache != NULL)
     {
       regcache_invalidate_thread (thread);
-      free_register_cache (regcache);
+      delete regcache;
       set_thread_regcache_data (thread, NULL);
     }
 }
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 32b3a8dccfc..614d5a2561f 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -51,6 +51,11 @@ struct regcache : public reg_buffer_common
   /* Constructors.  */
   regcache () = default;
   regcache (const target_desc *tdesc);
+  regcache (const regcache &rhs) = delete;
+  regcache &operator= (const regcache &rhs) = delete;
+
+  /* Deconstructor.  */
+  virtual ~regcache ();
 #endif
 
   /* Init the regcache data.  */
@@ -77,10 +82,6 @@ struct regcache : public reg_buffer_common
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
 
-/* Release all memory associated with the register cache for INFERIOR.  */
-
-void free_register_cache (struct regcache *regcache);
-
 /* Invalidate cached registers for one thread.  */
 
 void regcache_invalidate_thread (struct thread_info *);
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 4be12ec1c66..521f13c07d7 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -4237,15 +4237,13 @@ process_serial_event (void)
       require_running_or_break (cs.own_buf);
       if (cs.current_traceframe >= 0)
 	{
-	  struct regcache *regcache
-	    = new struct regcache (current_target_desc ());
+	  regcache a_regcache (current_target_desc ());
 
 	  if (fetch_traceframe_registers (cs.current_traceframe,
-					  regcache, -1) == 0)
-	    registers_to_string (regcache, cs.own_buf);
+					  &a_regcache, -1) == 0)
+	    registers_to_string (&a_regcache, cs.own_buf);
 	  else
 	    write_enn (cs.own_buf);
-	  free_register_cache (regcache);
 	}
       else
 	{
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 09/26] gdbserver: extract out regcache::invalidate and regcache::discard

[Tankut Baris Aktemur]

Extract out a piece of code from the `regcache_invalidate_thread`
function and turn into a new method of regcache named 'invalidate'.

We also introduce a small method named 'discard' to give the clients
an option to discard the cache without storing the contents.  This
method is utilized in a downstream debugger.
---
 gdbserver/regcache.cc | 24 +++++++++++++++++-------
 gdbserver/regcache.h  |  6 ++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 7b6337166ad..1db78951cc2 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -85,18 +85,22 @@ regcache_invalidate_thread (struct thread_info *thread)
 
   regcache = thread_regcache_data (thread);
 
-  if (regcache == NULL)
-    return;
+  if (regcache != nullptr)
+    regcache->invalidate ();
+}
 
-  if (regcache->registers_valid)
+void
+regcache::invalidate ()
+{
+  if (registers_valid)
     {
       scoped_restore_current_thread restore_thread;
-
-      switch_to_thread (thread);
-      store_inferior_registers (regcache, -1);
+      gdb_assert (this->thread != nullptr);
+      switch_to_thread (this->thread);
+      store_inferior_registers (this, -1);
     }
 
-  regcache->registers_valid = 0;
+  discard ();
 }
 
 /* See regcache.h.  */
@@ -121,6 +125,12 @@ regcache_invalidate (void)
 
 #endif
 
+void
+regcache::discard ()
+{
+  registers_valid = 0;
+}
+
 void
 regcache::initialize (const target_desc *tdesc,
 		      unsigned char *regbuf)
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 614d5a2561f..b4a9d8864ae 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -78,6 +78,12 @@ struct regcache : public reg_buffer_common
 
   /* Copy the contents of SRC into this regcache.  */
   void copy_from (regcache *src);
+
+  /* Store the cached registers to the target and then discard the cache.  */
+  void invalidate ();
+
+  /* Discard the cache without storing the registers to the target.  */
+  void discard ();
 };
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 10/26] gdbserver: convert registers_to_string into regcache::registers_to_string

[Tankut Baris Aktemur]

Convert the free `registers_to_string` function into a method of
the regcache struct.
---
 gdbserver/regcache.cc | 12 +++++-------
 gdbserver/regcache.h  |  9 ++++-----
 gdbserver/server.cc   |  4 ++--
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 1db78951cc2..92dc6fc921c 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -214,16 +214,14 @@ find_register_by_number (const struct target_desc *tdesc, int n)
 #ifndef IN_PROCESS_AGENT
 
 void
-registers_to_string (struct regcache *regcache, char *buf)
+regcache::registers_to_string (char *buf)
 {
-  unsigned char *registers = regcache->registers;
-  const struct target_desc *tdesc = regcache->tdesc;
-
+  unsigned char *regs = registers;
   for (int i = 0; i < tdesc->reg_defs.size (); ++i)
     {
-      if (regcache->register_status[i] == REG_VALID)
+      if (register_status[i] == REG_VALID)
 	{
-	  bin2hex (registers, buf, register_size (tdesc, i));
+	  bin2hex (regs, buf, register_size (tdesc, i));
 	  buf += register_size (tdesc, i) * 2;
 	}
       else
@@ -231,7 +229,7 @@ registers_to_string (struct regcache *regcache, char *buf)
 	  memset (buf, 'x', register_size (tdesc, i) * 2);
 	  buf += register_size (tdesc, i) * 2;
 	}
-      registers += register_size (tdesc, i);
+      regs += register_size (tdesc, i);
     }
   *buf = '\0';
 }
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index b4a9d8864ae..bed3151dcad 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -84,6 +84,10 @@ struct regcache : public reg_buffer_common
 
   /* Discard the cache without storing the registers to the target.  */
   void discard ();
+
+  /* Convert all registers to a string in the currently specified remote
+     format.  */
+  void registers_to_string (char *buf);
 };
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
@@ -106,11 +110,6 @@ void regcache_invalidate (void);
 
 void regcache_release (void);
 
-/* Convert all registers to a string in the currently specified remote
-   format.  */
-
-void registers_to_string (struct regcache *regcache, char *buf);
-
 /* Convert a string to register values and fill our register cache.  */
 
 void registers_from_string (struct regcache *regcache, char *buf);
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 521f13c07d7..f540cc86f7b 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -4241,7 +4241,7 @@ process_serial_event (void)
 
 	  if (fetch_traceframe_registers (cs.current_traceframe,
 					  &a_regcache, -1) == 0)
-	    registers_to_string (&a_regcache, cs.own_buf);
+	    a_regcache.registers_to_string (cs.own_buf);
 	  else
 	    write_enn (cs.own_buf);
 	}
@@ -4254,7 +4254,7 @@ process_serial_event (void)
 	  else
 	    {
 	      regcache = get_thread_regcache (current_thread);
-	      registers_to_string (regcache, cs.own_buf);
+	      regcache->registers_to_string (cs.own_buf);
 	    }
 	}
       break;
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 11/26] gdbserver: convert registers_from_string into regcache::registers_from_string

[Tankut Baris Aktemur]

Convert the registers_from_string function into a method of the regcache
struct.  Also constify the buffer argument.
---
 gdbserver/regcache.cc | 4 +---
 gdbserver/regcache.h  | 7 +++----
 gdbserver/server.cc   | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 92dc6fc921c..156fda7068d 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -235,11 +235,9 @@ regcache::registers_to_string (char *buf)
 }
 
 void
-registers_from_string (struct regcache *regcache, char *buf)
+regcache::registers_from_string (const char *buf)
 {
   int len = strlen (buf);
-  unsigned char *registers = regcache->registers;
-  const struct target_desc *tdesc = regcache->tdesc;
 
   if (len != tdesc->registers_size * 2)
     {
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index bed3151dcad..6aa03a70352 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -88,6 +88,9 @@ struct regcache : public reg_buffer_common
   /* Convert all registers to a string in the currently specified remote
      format.  */
   void registers_to_string (char *buf);
+
+  /* Convert a string to register values and fill our register cache.  */
+  void registers_from_string (const char *buf);
 };
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
@@ -110,10 +113,6 @@ void regcache_invalidate (void);
 
 void regcache_release (void);
 
-/* Convert a string to register values and fill our register cache.  */
-
-void registers_from_string (struct regcache *regcache, char *buf);
-
 /* For regcache_read_pc see gdbsupport/common-regcache.h.  */
 
 void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index f540cc86f7b..5e639d6ac57 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -4271,7 +4271,7 @@ process_serial_event (void)
 	  else
 	    {
 	      regcache = get_thread_regcache (current_thread);
-	      registers_from_string (regcache, &cs.own_buf[1]);
+	      regcache->registers_from_string (&cs.own_buf[1]);
 	      write_ok (cs.own_buf);
 	    }
 	}
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 12/26] gdbserver: convert supply_regblock to regcache::supply_regblock

[Tankut Baris Aktemur]

Convert the supply_regblock function into a method of the regcache
struct.  Also do some minor code modernization.
---
 gdbserver/regcache.cc   | 32 ++++++++------------------------
 gdbserver/regcache.h    |  7 +++++--
 gdbserver/tracepoint.cc |  8 ++++----
 3 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 156fda7068d..31f1e7bb3dc 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -376,39 +376,23 @@ supply_register_by_name_zeroed (struct regcache *regcache,
 
 #endif
 
-/* Supply the whole register set whose contents are stored in BUF, to
-   REGCACHE.  If BUF is NULL, all the registers' values are recorded
-   as unavailable.  */
-
 void
-supply_regblock (struct regcache *regcache, const void *buf)
+regcache::supply_regblock (const void *buf)
 {
-  if (buf)
+  if (buf != nullptr)
     {
-      const struct target_desc *tdesc = regcache->tdesc;
-
-      memcpy (regcache->registers, buf, tdesc->registers_size);
+      memcpy (registers, buf, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
-      {
-	int i;
-
-	for (i = 0; i < tdesc->reg_defs.size (); i++)
-	  regcache->register_status[i] = REG_VALID;
-      }
+      for (int i = 0; i < tdesc->reg_defs.size (); i++)
+	register_status[i] = REG_VALID;
 #endif
     }
   else
     {
-      const struct target_desc *tdesc = regcache->tdesc;
-
-      memset (regcache->registers, 0, tdesc->registers_size);
+      memset (registers, 0, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
-      {
-	int i;
-
-	for (i = 0; i < tdesc->reg_defs.size (); i++)
-	  regcache->register_status[i] = REG_UNAVAILABLE;
-      }
+      for (int i = 0; i < tdesc->reg_defs.size (); i++)
+	register_status[i] = REG_UNAVAILABLE;
 #endif
     }
 }
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 6aa03a70352..944718070b4 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -91,6 +91,11 @@ struct regcache : public reg_buffer_common
 
   /* Convert a string to register values and fill our register cache.  */
   void registers_from_string (const char *buf);
+
+  /* Supply the whole register set whose contents are stored in BUF,
+     to this regcache.  If BUF is NULL, all the registers' values are
+     recorded as unavailable.  */
+  void supply_regblock (const void *buf);
 };
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
@@ -138,8 +143,6 @@ void supply_register_by_name (struct regcache *regcache,
 void supply_register_by_name_zeroed (struct regcache *regcache,
 				     const char *name);
 
-void supply_regblock (struct regcache *regcache, const void *buf);
-
 void collect_register (struct regcache *regcache, int n, void *buf);
 
 void collect_register_as_string (struct regcache *regcache, int n, char *buf);
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 26003422a70..9833e7c3b0f 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4707,7 +4707,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
 	{
 	  fctx->regcache_initted = 1;
 	  fctx->regcache.initialize (ipa_tdesc, fctx->regspace);
-	  supply_regblock (&fctx->regcache, NULL);
+	  fctx->regcache.supply_regblock (nullptr);
 	  supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs);
 	}
       regcache = &fctx->regcache;
@@ -4722,7 +4722,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
 	{
 	  sctx->regcache_initted = 1;
 	  sctx->regcache.initialize (ipa_tdesc, sctx->regspace);
-	  supply_regblock (&sctx->regcache, NULL);
+	  sctx->regcache.supply_regblock (nullptr);
 	  /* Pass down the tracepoint address, because REGS doesn't
 	     include the PC, but we know what it must have been.  */
 	  supply_static_tracepoint_registers (&sctx->regcache,
@@ -5180,7 +5180,7 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum)
   if (dataptr == NULL)
     {
       /* Mark registers unavailable.  */
-      supply_regblock (regcache, NULL);
+      regcache->supply_regblock (nullptr);
 
       /* We can generally guess at a PC, although this will be
 	 misleading for while-stepping frames and multi-location
@@ -5190,7 +5190,7 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum)
 	regcache_write_pc (regcache, tpoint->address);
     }
   else
-    supply_regblock (regcache, dataptr);
+    regcache->supply_regblock (dataptr);
 
   return 0;
 }
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 13/26] gdbserver: convert register_data into regcache::register_data

[Tankut Baris Aktemur]

Convert the register_data function to a method of the regcache struct.
---
 gdbserver/regcache.cc | 20 ++++++++++----------
 gdbserver/regcache.h  |  3 +++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 31f1e7bb3dc..79835ef4ff1 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -314,11 +314,11 @@ regcache_register_size (const struct regcache *regcache, int n)
   return register_size (regcache->tdesc, n);
 }
 
-static unsigned char *
-register_data (const struct regcache *regcache, int n)
+unsigned char *
+regcache::register_data (int regnum) const
 {
-  return (regcache->registers
-	  + find_register_by_number (regcache->tdesc, n).offset / 8);
+  return (registers
+	  + find_register_by_number (tdesc, regnum).offset / 8);
 }
 
 void
@@ -334,7 +334,7 @@ regcache::raw_supply (int n, const void *buf)
 {
   if (buf)
     {
-      memcpy (register_data (this, n), buf, register_size (tdesc, n));
+      memcpy (register_data (n), buf, register_size (tdesc, n));
 #ifndef IN_PROCESS_AGENT
       if (register_status != NULL)
 	register_status[n] = REG_VALID;
@@ -342,7 +342,7 @@ regcache::raw_supply (int n, const void *buf)
     }
   else
     {
-      memset (register_data (this, n), 0, register_size (tdesc, n));
+      memset (register_data (n), 0, register_size (tdesc, n));
 #ifndef IN_PROCESS_AGENT
       if (register_status != NULL)
 	register_status[n] = REG_UNAVAILABLE;
@@ -355,7 +355,7 @@ regcache::raw_supply (int n, const void *buf)
 void
 supply_register_zeroed (struct regcache *regcache, int n)
 {
-  memset (register_data (regcache, n), 0,
+  memset (regcache->register_data (n), 0,
 	  register_size (regcache->tdesc, n));
 #ifndef IN_PROCESS_AGENT
   if (regcache->register_status != NULL)
@@ -419,7 +419,7 @@ collect_register (struct regcache *regcache, int n, void *buf)
 void
 regcache::raw_collect (int n, void *buf) const
 {
-  memcpy (buf, register_data (this, n), register_size (tdesc, n));
+  memcpy (buf, register_data (n), register_size (tdesc, n));
 }
 
 enum register_status
@@ -458,7 +458,7 @@ regcache_raw_get_unsigned_by_name (struct regcache *regcache,
 void
 collect_register_as_string (struct regcache *regcache, int n, char *buf)
 {
-  bin2hex (register_data (regcache, n), buf,
+  bin2hex (regcache->register_data (n), buf,
 	   register_size (regcache->tdesc, n));
 }
 
@@ -505,7 +505,7 @@ regcache::raw_compare (int regnum, const void *buf, int offset) const
 {
   gdb_assert (buf != NULL);
 
-  const unsigned char *regbuf = register_data (this, regnum);
+  const unsigned char *regbuf = register_data (regnum);
   int size = register_size (tdesc, regnum);
   gdb_assert (size >= offset);
 
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 944718070b4..88e6ac32bae 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -96,6 +96,9 @@ struct regcache : public reg_buffer_common
      to this regcache.  If BUF is NULL, all the registers' values are
      recorded as unavailable.  */
   void supply_regblock (const void *buf);
+
+  /* Return the pointer to the register with number REGNUM.  */
+  unsigned char *register_data (int regnum) const;
 };
 
 regcache *get_thread_regcache (thread_info *thread, bool fetch = true);
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 14/26] gdbserver: introduce and use regcache::set_register_status

[Tankut Baris Aktemur]

Introduce and use a setter method in regcache to set the status of a
register.  There already exists get_register_status.  So, it made
sense to add the setter to control access to the register_status
field.
---
 gdbserver/regcache.cc | 23 +++++++++++++++--------
 gdbserver/regcache.h  |  3 +++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 79835ef4ff1..ec11082be6f 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -336,16 +336,14 @@ regcache::raw_supply (int n, const void *buf)
     {
       memcpy (register_data (n), buf, register_size (tdesc, n));
 #ifndef IN_PROCESS_AGENT
-      if (register_status != NULL)
-	register_status[n] = REG_VALID;
+      set_register_status (n, REG_VALID);
 #endif
     }
   else
     {
       memset (register_data (n), 0, register_size (tdesc, n));
 #ifndef IN_PROCESS_AGENT
-      if (register_status != NULL)
-	register_status[n] = REG_UNAVAILABLE;
+      set_register_status (n, REG_UNAVAILABLE);
 #endif
     }
 }
@@ -358,8 +356,7 @@ supply_register_zeroed (struct regcache *regcache, int n)
   memset (regcache->register_data (n), 0,
 	  register_size (regcache->tdesc, n));
 #ifndef IN_PROCESS_AGENT
-  if (regcache->register_status != NULL)
-    regcache->register_status[n] = REG_VALID;
+  regcache->set_register_status (n, REG_VALID);
 #endif
 }
 
@@ -384,7 +381,7 @@ regcache::supply_regblock (const void *buf)
       memcpy (registers, buf, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
       for (int i = 0; i < tdesc->reg_defs.size (); i++)
-	register_status[i] = REG_VALID;
+	set_register_status (i, REG_VALID);
 #endif
     }
   else
@@ -392,7 +389,7 @@ regcache::supply_regblock (const void *buf)
       memset (registers, 0, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
       for (int i = 0; i < tdesc->reg_defs.size (); i++)
-	register_status[i] = REG_UNAVAILABLE;
+	set_register_status (i, REG_UNAVAILABLE);
 #endif
     }
 }
@@ -498,6 +495,16 @@ regcache::get_register_status (int regnum) const
 #endif
 }
 
+void
+regcache::set_register_status (int regnum, enum register_status status)
+{
+#ifndef IN_PROCESS_AGENT
+  gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
+  if (register_status != nullptr)
+    register_status[regnum] = status;
+#endif
+}
+
 /* See gdbsupport/common-regcache.h.  */
 
 bool
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 88e6ac32bae..ceef28086ce 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -64,6 +64,9 @@ struct regcache : public reg_buffer_common
   /* See gdbsupport/common-regcache.h.  */
   enum register_status get_register_status (int regnum) const override;
 
+  /* Set the status of register REGNUM to STATUS.  */
+  void set_register_status (int regnum, enum register_status status);
+
   /* See gdbsupport/common-regcache.h.  */
   void raw_supply (int regnum, const void *buf) override;
 
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 15/26] gdbserver: check for nullptr condition in regcache::get_register_status

[Tankut Baris Aktemur]

A regcache can be initialized with a register value buffer, in which
case, the register_status pointer is null.  This condition is checked
in set_register_status, but not in get_register_status.  Do this check
for consistence and safety.
---
 gdbserver/regcache.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index ec11082be6f..0c6f1eb392b 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -489,7 +489,10 @@ regcache::get_register_status (int regnum) const
 {
 #ifndef IN_PROCESS_AGENT
   gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
-  return (enum register_status) (register_status[regnum]);
+  if (register_status != nullptr)
+    return (enum register_status) (register_status[regnum]);
+  else
+    return REG_VALID;
 #else
   return REG_VALID;
 #endif
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 16/26] gdbserver: boolify regcache fields

[Tankut Baris Aktemur]

The registers_valid and registers_owned fields of the regcache struct
are of type int.  Make them bool.
---
 gdbserver/regcache.cc | 12 ++++++------
 gdbserver/regcache.h  |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 0c6f1eb392b..4b56750d06a 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -57,7 +57,7 @@ get_thread_regcache (struct thread_info *thread, bool fetch)
 void
 regcache::fetch ()
 {
-  if (registers_valid == 0)
+  if (!registers_valid)
     {
       scoped_restore_current_thread restore_thread;
       gdb_assert (this->thread != nullptr);
@@ -66,7 +66,7 @@ regcache::fetch ()
       /* Invalidate all registers, to prevent stale left-overs.  */
       memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
       fetch_inferior_registers (this, -1);
-      registers_valid = 1;
+      registers_valid = true;
     }
 }
 
@@ -128,7 +128,7 @@ regcache_invalidate (void)
 void
 regcache::discard ()
 {
-  registers_valid = 0;
+  registers_valid = false;
 }
 
 void
@@ -145,7 +145,7 @@ regcache::initialize (const target_desc *tdesc,
       this->tdesc = tdesc;
       this->registers
 	= (unsigned char *) xcalloc (1, tdesc->registers_size);
-      this->registers_owned = 1;
+      this->registers_owned = true;
       this->register_status
 	= (unsigned char *) xmalloc (tdesc->reg_defs.size ());
       memset ((void *) this->register_status, REG_UNAVAILABLE,
@@ -158,13 +158,13 @@ regcache::initialize (const target_desc *tdesc,
     {
       this->tdesc = tdesc;
       this->registers = regbuf;
-      this->registers_owned = 0;
+      this->registers_owned = false;
 #ifndef IN_PROCESS_AGENT
       this->register_status = nullptr;
 #endif
     }
 
-  this->registers_valid = 0;
+  this->registers_valid = false;
 }
 
 #ifndef IN_PROCESS_AGENT
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index ceef28086ce..f155ac631eb 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -41,8 +41,8 @@ struct regcache : public reg_buffer_common
      register cache is _not_ pass-through, unlike GDB's.  Note that
      "valid" here is unrelated to whether the registers are available
      in a traceframe.  For that, check REGISTER_STATUS below.  */
-  int registers_valid = 0;
-  int registers_owned = 0;
+  bool registers_valid = false;
+  bool registers_owned = false;
   unsigned char *registers = nullptr;
 #ifndef IN_PROCESS_AGENT
   /* One of REG_UNAVAILABLE or REG_VALID.  */
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 17/26] gdbserver: rename regcache's registers_valid to registers_fetched

[Tankut Baris Aktemur]

The registers_valid field of the regcache struct is used for tracking
whether we have attempted to fetch all the registers from the target.
Its name does not reflect this well, I think.  It falsely gives the
impression that all the registers are valid.  This may conflict an
individual register status, which could be REG_UNAVAILABLE.  To better
reflect the purpose, rename the field to "registers_fetched".
---
 gdbserver/regcache.cc | 12 ++++++------
 gdbserver/regcache.h  | 13 +++++++------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 4b56750d06a..0e21c1aa7d1 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -57,7 +57,7 @@ get_thread_regcache (struct thread_info *thread, bool fetch)
 void
 regcache::fetch ()
 {
-  if (!registers_valid)
+  if (!registers_fetched)
     {
       scoped_restore_current_thread restore_thread;
       gdb_assert (this->thread != nullptr);
@@ -66,7 +66,7 @@ regcache::fetch ()
       /* Invalidate all registers, to prevent stale left-overs.  */
       memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
       fetch_inferior_registers (this, -1);
-      registers_valid = true;
+      registers_fetched = true;
     }
 }
 
@@ -92,7 +92,7 @@ regcache_invalidate_thread (struct thread_info *thread)
 void
 regcache::invalidate ()
 {
-  if (registers_valid)
+  if (registers_fetched)
     {
       scoped_restore_current_thread restore_thread;
       gdb_assert (this->thread != nullptr);
@@ -128,7 +128,7 @@ regcache_invalidate (void)
 void
 regcache::discard ()
 {
-  registers_valid = false;
+  registers_fetched = false;
 }
 
 void
@@ -164,7 +164,7 @@ regcache::initialize (const target_desc *tdesc,
 #endif
     }
 
-  this->registers_valid = false;
+  this->registers_fetched = false;
 }
 
 #ifndef IN_PROCESS_AGENT
@@ -197,7 +197,7 @@ regcache::copy_from (regcache *src)
     memcpy (this->register_status, src->register_status,
 	    src->tdesc->reg_defs.size ());
 #endif
-  this->registers_valid = src->registers_valid;
+  this->registers_fetched = src->registers_fetched;
 }
 
 /* Return a reference to the description of register N.  */
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index f155ac631eb..3fcd4643a42 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -36,12 +36,13 @@ struct regcache : public reg_buffer_common
   /* Back-link to the thread to which this regcache belongs.  */
   thread_info *thread = nullptr;
 
-  /* Whether the REGISTERS buffer's contents are valid.  If false, we
-     haven't fetched the registers from the target yet.  Not that this
-     register cache is _not_ pass-through, unlike GDB's.  Note that
-     "valid" here is unrelated to whether the registers are available
-     in a traceframe.  For that, check REGISTER_STATUS below.  */
-  bool registers_valid = false;
+  /* Whether the REGISTERS buffer's contents are fetched.  If false,
+     we haven't fetched the registers from the target yet.  Note that
+     this register cache is _not_ pass-through, unlike GDB's.  Also,
+     note that "fetched" here is unrelated to whether the registers
+     are available in a traceframe.  For that, check REGISTER_STATUS
+     below.  */
+  bool registers_fetched = false;
   bool registers_owned = false;
   unsigned char *registers = nullptr;
 #ifndef IN_PROCESS_AGENT
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 18/26] gdbsupport: fix a typo in a comment in common-regcache.h

[Tankut Baris Aktemur]

Fix a typo.
---
 gdbsupport/common-regcache.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
index e462f532407..bf14610f98f 100644
--- a/gdbsupport/common-regcache.h
+++ b/gdbsupport/common-regcache.h
@@ -33,10 +33,10 @@ enum register_status : signed char
 
     /* The register value is unavailable.  E.g., we're inspecting a
        traceframe, and this register wasn't collected.  Note that this
-       is different a different "unavailable" from saying the register
-       does not exist in the target's architecture --- in that case,
-       the target should have given us a target description that does
-       not include the register in the first place.  */
+       "unavailable" is different from saying the register does not
+       exist in the target's architecture --- in that case, the target
+       should have given us a target description that does not include
+       the register in the first place.  */
     REG_UNAVAILABLE = -1
   };
 
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 19/26] gdbserver: fix the declared type of register_status in regcache

[Tankut Baris Aktemur]

The register_status field of regcache is declared as `unsigned char *`.
This is incorrect, because `enum register_status` from
gdbsupport/common-regcache.h is based on signed char and
REG_UNAVAILABLE is defined as -1.  Fix the declared type.

The get/set methods already use the correct type, but we update cast
operations in two places.
---
 gdbserver/regcache.cc | 4 ++--
 gdbserver/regcache.h  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 0e21c1aa7d1..09ea58bdbd6 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -147,7 +147,7 @@ regcache::initialize (const target_desc *tdesc,
 	= (unsigned char *) xcalloc (1, tdesc->registers_size);
       this->registers_owned = true;
       this->register_status
-	= (unsigned char *) xmalloc (tdesc->reg_defs.size ());
+	= (enum register_status *) xmalloc (tdesc->reg_defs.size ());
       memset ((void *) this->register_status, REG_UNAVAILABLE,
 	      tdesc->reg_defs.size ());
 #else
@@ -490,7 +490,7 @@ regcache::get_register_status (int regnum) const
 #ifndef IN_PROCESS_AGENT
   gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
   if (register_status != nullptr)
-    return (enum register_status) (register_status[regnum]);
+    return register_status[regnum];
   else
     return REG_VALID;
 #else
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 3fcd4643a42..ad71a9acaec 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -46,8 +46,8 @@ struct regcache : public reg_buffer_common
   bool registers_owned = false;
   unsigned char *registers = nullptr;
 #ifndef IN_PROCESS_AGENT
-  /* One of REG_UNAVAILABLE or REG_VALID.  */
-  unsigned char *register_status = nullptr;
+  /* See gdbsupport/common-regcache.h.  */
+  enum register_status *register_status = nullptr;
 
   /* Constructors.  */
   regcache () = default;
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 20/26] gdbserver: make some regcache fields private

[Tankut Baris Aktemur]

Some fields of the regcache struct are used for internal state
tracking.  Prevent direct access to them from outside by making them
private.
---
 gdbserver/regcache.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index ad71a9acaec..be412bc3765 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -36,6 +36,8 @@ struct regcache : public reg_buffer_common
   /* Back-link to the thread to which this regcache belongs.  */
   thread_info *thread = nullptr;
 
+private:
+
   /* Whether the REGISTERS buffer's contents are fetched.  If false,
      we haven't fetched the registers from the target yet.  Note that
      this register cache is _not_ pass-through, unlike GDB's.  Also,
@@ -49,6 +51,8 @@ struct regcache : public reg_buffer_common
   /* See gdbsupport/common-regcache.h.  */
   enum register_status *register_status = nullptr;
 
+public:
+
   /* Constructors.  */
   regcache () = default;
   regcache (const target_desc *tdesc);
@@ -59,6 +63,8 @@ struct regcache : public reg_buffer_common
   virtual ~regcache ();
 #endif
 
+public:
+
   /* Init the regcache data.  */
   void initialize (const target_desc *tdesc, unsigned char *regbuf);
 
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 21/26] gdbserver: use REG_UNKNOWN for a regcache's register statuses

[Tankut Baris Aktemur]

When a regcache is initialized, the values of registers are not
fetched yet.  Thus, initialize the register statuses to REG_UNKNOWN
instead of REG_UNAVAILABLE, because the latter rather means "we
attempted to fetch but could not obtain the value".

The definitions of the reg status enums (from
gdbsupport/common-regcache.h) as a reminder:

    /* The register value is not in the cache, and we don't know yet
       whether it's available in the target (or traceframe).  */
    REG_UNKNOWN = 0,

    /* The register value is valid and cached.  */
    REG_VALID = 1,

    /* The register value is unavailable.  E.g., we're inspecting a
       traceframe, and this register wasn't collected.  Note that this
       "unavailable" is different from saying the register does not
       exist in the target's architecture --- in that case, the target
       should have given us a target description that does not include
       the register in the first place.  */
    REG_UNAVAILABLE = -1

Similarly, when the regcache is invalidated, change all the statuses
back to REG_UNKNOWN.
---
 gdbserver/regcache.cc | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 09ea58bdbd6..2befb30e337 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -64,9 +64,17 @@ regcache::fetch ()
       switch_to_thread (this->thread);
 
       /* Invalidate all registers, to prevent stale left-overs.  */
-      memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
+      discard ();
       fetch_inferior_registers (this, -1);
       registers_fetched = true;
+
+      /* Make sure that the registers that could not be fetched are
+	 now unavailable.  */
+      for (int i = 0; i < tdesc->reg_defs.size (); ++i)
+	{
+	  if (register_status[i] == REG_UNKNOWN)
+	    set_register_status (i, REG_UNAVAILABLE);
+	}
     }
 }
 
@@ -128,6 +136,9 @@ regcache_invalidate (void)
 void
 regcache::discard ()
 {
+#ifndef IN_PROCESS_AGENT
+  memset ((void *) register_status, REG_UNKNOWN, tdesc->reg_defs.size ());
+#endif
   registers_fetched = false;
 }
 
@@ -148,8 +159,7 @@ regcache::initialize (const target_desc *tdesc,
       this->registers_owned = true;
       this->register_status
 	= (enum register_status *) xmalloc (tdesc->reg_defs.size ());
-      memset ((void *) this->register_status, REG_UNAVAILABLE,
-	      tdesc->reg_defs.size ());
+      discard ();
 #else
       gdb_assert_not_reached ("can't allocate memory from the heap");
 #endif
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 22/26] gdbserver: zero-out register values in regcache-discard

[Tankut Baris Aktemur]

Zero-out register values when a regcache is discarded so that we avoid
garbage values left in the buffer.
---
 gdbserver/regcache.cc | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 2befb30e337..644f436c681 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -136,6 +136,7 @@ regcache_invalidate (void)
 void
 regcache::discard ()
 {
+  memset (registers, 0, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
   memset ((void *) register_status, REG_UNKNOWN, tdesc->reg_defs.size ());
 #endif
@@ -149,16 +150,17 @@ regcache::initialize (const target_desc *tdesc,
   if (regbuf == NULL)
     {
 #ifndef IN_PROCESS_AGENT
-      /* Make sure to zero-initialize the register cache when it is
-	 created, in case there are registers the target never
-	 fetches.  This way they'll read as zero instead of
-	 garbage.  */
       this->tdesc = tdesc;
       this->registers
-	= (unsigned char *) xcalloc (1, tdesc->registers_size);
+	= (unsigned char *) xmalloc (tdesc->registers_size);
       this->registers_owned = true;
       this->register_status
 	= (enum register_status *) xmalloc (tdesc->reg_defs.size ());
+
+      /* Make sure to zero-initialize the register cache when it is
+	 created, in case there are registers the target never
+	 fetches.  This way they'll read as zero instead of
+	 garbage.  */
       discard ();
 #else
       gdb_assert_not_reached ("can't allocate memory from the heap");
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 23/26] gdbserver: set register statuses in registers_from_string

[Tankut Baris Aktemur]

The registers_from_string function uses hex2bin to set the values of
all registers.  Set the register statuses to REG_VALID to reflect this
change.
---
 gdbserver/regcache.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 644f436c681..32f0e1109e6 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -259,6 +259,8 @@ regcache::registers_from_string (const char *buf)
 	len = tdesc->registers_size * 2;
     }
   hex2bin (buf, registers, len / 2);
+  /* All register data have been re-written.  Update the statuses.  */
+  memset (register_status, REG_VALID, tdesc->reg_defs.size ());
 }
 
 /* See regcache.h */
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 24/26] gdbserver: return tracked register status in regcache_raw_read_unsigned

[Tankut Baris Aktemur]

In regcache_raw_read_unsigned, we unconditionally return REG_VALID as
the register status.  This does not seem right, since the register may
in fact be in another state, such as REG_UNAVAILABLE.  Return the
tracked status.
---
 gdbserver/regcache.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 32f0e1109e6..4533e9e9b12 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -451,7 +451,7 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
   *val = 0;
   collect_register (regcache, regnum, val);
 
-  return REG_VALID;
+  return regcache->get_register_status (regnum);
 }
 
 #ifndef IN_PROCESS_AGENT
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 25/26] gdbserver: refuse null argument in regcache::supply_regblock

[Tankut Baris Aktemur]

The regcache::supply_regblock method implements two different
behaviors based on whether its 'buf' argument is null.  The null
behavior is used only in tracepoint.cc.  Refuse the null pointer
argument and use the 'discard' method to obtain that second behavior.

Note that the original code resets register statuses to
REG_UNAVAILABLE, but 'discard' sets them to REG_UNKNOWN.  For the
current purposes of resetting the regcache, the difference does not
seem to be important.
---
 gdbserver/regcache.cc   | 19 +++++--------------
 gdbserver/regcache.h    |  3 +--
 gdbserver/tracepoint.cc |  8 ++++----
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 4533e9e9b12..cfb68774402 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -390,22 +390,13 @@ supply_register_by_name_zeroed (struct regcache *regcache,
 void
 regcache::supply_regblock (const void *buf)
 {
-  if (buf != nullptr)
-    {
-      memcpy (registers, buf, tdesc->registers_size);
-#ifndef IN_PROCESS_AGENT
-      for (int i = 0; i < tdesc->reg_defs.size (); i++)
-	set_register_status (i, REG_VALID);
-#endif
-    }
-  else
-    {
-      memset (registers, 0, tdesc->registers_size);
+  gdb_assert (buf != nullptr);
+
+  memcpy (registers, buf, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
-      for (int i = 0; i < tdesc->reg_defs.size (); i++)
-	set_register_status (i, REG_UNAVAILABLE);
+  for (int i = 0; i < tdesc->reg_defs.size (); i++)
+    set_register_status (i, REG_VALID);
 #endif
-    }
 }
 
 #ifndef IN_PROCESS_AGENT
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index be412bc3765..216044889ec 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -103,8 +103,7 @@ struct regcache : public reg_buffer_common
   void registers_from_string (const char *buf);
 
   /* Supply the whole register set whose contents are stored in BUF,
-     to this regcache.  If BUF is NULL, all the registers' values are
-     recorded as unavailable.  */
+     to this regcache.  BUF cannot be NULL.  */
   void supply_regblock (const void *buf);
 
   /* Return the pointer to the register with number REGNUM.  */
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 9833e7c3b0f..9622d53cd82 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4707,7 +4707,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
 	{
 	  fctx->regcache_initted = 1;
 	  fctx->regcache.initialize (ipa_tdesc, fctx->regspace);
-	  fctx->regcache.supply_regblock (nullptr);
+	  fctx->regcache.discard ();
 	  supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs);
 	}
       regcache = &fctx->regcache;
@@ -4722,7 +4722,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
 	{
 	  sctx->regcache_initted = 1;
 	  sctx->regcache.initialize (ipa_tdesc, sctx->regspace);
-	  sctx->regcache.supply_regblock (nullptr);
+	  sctx->regcache.discard ();
 	  /* Pass down the tracepoint address, because REGS doesn't
 	     include the PC, but we know what it must have been.  */
 	  supply_static_tracepoint_registers (&sctx->regcache,
@@ -5179,8 +5179,8 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum)
   dataptr = traceframe_find_regblock (tframe, tfnum);
   if (dataptr == NULL)
     {
-      /* Mark registers unavailable.  */
-      regcache->supply_regblock (nullptr);
+      /* Wipe out the cache.  */
+      regcache->discard ();
 
       /* We can generally guess at a PC, although this will be
 	 misleading for while-stepping frames and multi-location
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 26/26] gdbserver: allow gradually populating and selectively storing a regcache

[Tankut Baris Aktemur]

Currently, the regcache can be used by fetching all the registers from
the target.  For some targets, this could be a costly operation
because there is a large number of threads with a large register file
each.  In this patch, we revise the regcache implementation to allow
populating the contents gradually and storing the registers only when
they have updated values.

To this aim, we introduce a new register status: REG_DIRTY.  This
status denotes that a register value has been cached and also updated.
When invalidating the cache, only the dirty registers are stored to
the target.  In a typical debug session, it is more likely that only a
small subset of the register file has changed.  Therefore, selectively
storing the registers on targets with many threads and registers can
save substantial costs, with respect to storing the whole set.

The collect_register function now performs a lazy fetch.  If the
requested register value is not cached yet, it is requested from the
target.

The supply_register function updates the status of the supplied
register as follows: if the register was not available in the cache,
its status becomes REG_VALID, denoting that the value is now cached.
If the register is supplied again, it becomes REG_DIRTY.

The function that supply the whole register set (supply_regblock and
registers_from_string) are also updated to compare the present and new
values of each register, so that we can track the register statuses
(i.e.  dirty or not) properly.

Regression-tested on an X86_64 Linux target using the native-gdbserver
and native-extended-gdbserver board files.
---
 gdbserver/regcache.cc        | 96 ++++++++++++++++++++++++++++++------
 gdbserver/regcache.h         |  6 +++
 gdbsupport/common-regcache.h |  3 ++
 3 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index cfb68774402..cf020985c31 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -63,6 +63,18 @@ regcache::fetch ()
       gdb_assert (this->thread != nullptr);
       switch_to_thread (this->thread);
 
+      /* If there are individually-fetched dirty registers, first
+	 store them, then fetch all.  We prefer this to doing
+	 individual fetch for each registers, if needed, because it is
+	 more likely that very few registers are individually-fetched
+	 at this moment and that fetching all in one go is more
+	 efficient than fetching each reg one by one.  */
+      for (int i = 0; i < tdesc->reg_defs.size (); ++i)
+	{
+	  if (register_status[i] == REG_DIRTY)
+	    store_inferior_registers (this, i);
+	}
+
       /* Invalidate all registers, to prevent stale left-overs.  */
       discard ();
       fetch_inferior_registers (this, -1);
@@ -100,12 +112,17 @@ regcache_invalidate_thread (struct thread_info *thread)
 void
 regcache::invalidate ()
 {
-  if (registers_fetched)
+  scoped_restore_current_thread restore_thread;
+  gdb_assert (this->thread != nullptr);
+  switch_to_thread (this->thread);
+
+  /* Store dirty registers individually.  We prefer this to a
+     store-all, because it is more likely that a small number of
+     registers have changed.  */
+  for (int i = 0; i < tdesc->reg_defs.size (); ++i)
     {
-      scoped_restore_current_thread restore_thread;
-      gdb_assert (this->thread != nullptr);
-      switch_to_thread (this->thread);
-      store_inferior_registers (this, -1);
+      if (register_status[i] == REG_DIRTY)
+	store_inferior_registers (this, i);
     }
 
   discard ();
@@ -231,7 +248,8 @@ regcache::registers_to_string (char *buf)
   unsigned char *regs = registers;
   for (int i = 0; i < tdesc->reg_defs.size (); ++i)
     {
-      if (register_status[i] == REG_VALID)
+      if (register_status[i] == REG_VALID
+	  || register_status[i] == REG_DIRTY)
 	{
 	  bin2hex (regs, buf, register_size (tdesc, i));
 	  buf += register_size (tdesc, i) * 2;
@@ -258,9 +276,12 @@ regcache::registers_from_string (const char *buf)
       if (len > tdesc->registers_size * 2)
 	len = tdesc->registers_size * 2;
     }
-  hex2bin (buf, registers, len / 2);
-  /* All register data have been re-written.  Update the statuses.  */
-  memset (register_status, REG_VALID, tdesc->reg_defs.size ());
+
+  unsigned char *new_regs =
+    (unsigned char *) alloca (tdesc->registers_size);
+
+  hex2bin (buf, new_regs, len / 2);
+  supply_regblock (new_regs);
 }
 
 /* See regcache.h */
@@ -350,7 +371,7 @@ regcache::raw_supply (int n, const void *buf)
     {
       memcpy (register_data (n), buf, register_size (tdesc, n));
 #ifndef IN_PROCESS_AGENT
-      set_register_status (n, REG_VALID);
+      bump_register_status (n);
 #endif
     }
   else
@@ -370,7 +391,7 @@ supply_register_zeroed (struct regcache *regcache, int n)
   memset (regcache->register_data (n), 0,
 	  register_size (regcache->tdesc, n));
 #ifndef IN_PROCESS_AGENT
-  regcache->set_register_status (n, REG_VALID);
+  regcache->bump_register_status (n);
 #endif
 }
 
@@ -392,11 +413,26 @@ regcache::supply_regblock (const void *buf)
 {
   gdb_assert (buf != nullptr);
 
-  memcpy (registers, buf, tdesc->registers_size);
 #ifndef IN_PROCESS_AGENT
-  for (int i = 0; i < tdesc->reg_defs.size (); i++)
-    set_register_status (i, REG_VALID);
+  /* First, update the statuses.  Mark dirty only those that have
+     changed.  */
+  unsigned char *regs = registers;
+  unsigned char *new_regs = (unsigned char *) buf;
+  for (int i = 0; i < tdesc->reg_defs.size (); ++i)
+    {
+      int size = register_size (tdesc, i);
+      bool first_time = (get_register_status (i) == REG_UNKNOWN);
+      bool valid = (get_register_status (i) == REG_VALID);
+
+      if (first_time
+	  || (valid && (memcmp (new_regs, regs, size) != 0)))
+	bump_register_status (i);
+
+      regs += size;
+      new_regs += size;
+    }
 #endif
+  memcpy (registers, buf, tdesc->registers_size);
 }
 
 #ifndef IN_PROCESS_AGENT
@@ -413,6 +449,15 @@ supply_register_by_name (struct regcache *regcache,
 void
 collect_register (struct regcache *regcache, int n, void *buf)
 {
+#ifndef IN_PROCESS_AGENT
+  if (regcache->get_register_status (n) == REG_UNKNOWN)
+    {
+      /* This register has not been fetched from the target, yet.
+	 Do it now.  */
+      fetch_inferior_registers (regcache, n);
+    }
+#endif
+
   regcache->raw_collect (n, buf);
 }
 
@@ -513,6 +558,29 @@ regcache::set_register_status (int regnum, enum register_status status)
 #endif
 }
 
+void
+regcache::bump_register_status (int regnum)
+{
+#ifndef IN_PROCESS_AGENT
+  if (register_status == nullptr)
+    return;
+#endif
+
+  switch (get_register_status (regnum))
+    {
+    case REG_UNKNOWN:
+      set_register_status (regnum, REG_VALID);
+      break;
+
+    case REG_VALID:
+      set_register_status (regnum, REG_DIRTY);
+      break;
+
+    default:
+      break;
+    }
+}
+
 /* See gdbsupport/common-regcache.h.  */
 
 bool
diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
index 216044889ec..132709fa71f 100644
--- a/gdbserver/regcache.h
+++ b/gdbserver/regcache.h
@@ -74,6 +74,12 @@ struct regcache : public reg_buffer_common
   /* Set the status of register REGNUM to STATUS.  */
   void set_register_status (int regnum, enum register_status status);
 
+  /* Shift the register status "one level" towards REG_DIRTY.
+     REG_UNKNOWN becomes REG_VALID;
+     REG_VALID becomes REG_DIRTY;
+     REG_DIRTY and REG_UNAVAILABLE stay the same.  */
+  void bump_register_status (int regnum);
+
   /* See gdbsupport/common-regcache.h.  */
   void raw_supply (int regnum, const void *buf) override;
 
diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
index bf14610f98f..e81238fe7e0 100644
--- a/gdbsupport/common-regcache.h
+++ b/gdbsupport/common-regcache.h
@@ -31,6 +31,9 @@ enum register_status : signed char
     /* The register value is valid and cached.  */
     REG_VALID = 1,
 
+    /* The register value is valid, cached, and has been changed.  */
+    REG_DIRTY = 2,
+
     /* The register value is unavailable.  E.g., we're inspecting a
        traceframe, and this register wasn't collected.  Note that this
        "unavailable" is different from saying the register does not
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[Tom Tromey]

>>>>> Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

> Gdbserver's regcache is defined and used in a way that it is either
> invalid or fetches all the registers from the target prior to being
> used.  It also seems some regcache functions have two different contracts
> based on argument values (e.g. a buffer being nullptr).

> This is an attempt to refactor the regcache in gdbserver to

>   - convert several free functions to methods.
>   - use and update register statuses more consistently.
>   - allow populating register values gradually, instead of having to
>     fetch all register values from the target.

I haven't read these patches, but I wanted to mention that, over time,
we've been trying to bring gdb and gdbserver closer together where
possible.  And, I'm wondering how this series fits into this.  At the
end, are the two register caches more similar?  More divergent?

I'm not necessarily saying this is the most important thing, but for
example what would be unfortunate is if the two ended up with similar
functionality but very different expressions, which would make the
sharing of other code even harder.

Tom

RE: [PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[Aktemur, Tankut Baris]

On Tuesday, March 7, 2023 9:40 PM, Tom Tromey wrote:
> >>>>> Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > Gdbserver's regcache is defined and used in a way that it is either
> > invalid or fetches all the registers from the target prior to being
> > used.  It also seems some regcache functions have two different contracts
> > based on argument values (e.g. a buffer being nullptr).
> 
> > This is an attempt to refactor the regcache in gdbserver to
> 
> >   - convert several free functions to methods.
> >   - use and update register statuses more consistently.
> >   - allow populating register values gradually, instead of having to
> >     fetch all register values from the target.
> 
> I haven't read these patches, but I wanted to mention that, over time,
> we've been trying to bring gdb and gdbserver closer together where
> possible.  And, I'm wondering how this series fits into this.  At the
> end, are the two register caches more similar?  More divergent?
> 
> I'm not necessarily saying this is the most important thing, but for
> example what would be unfortunate is if the two ended up with similar
> functionality but very different expressions, which would make the
> sharing of other code even harder.
> 
> Tom

Hello Tom,

The regcache implementation in gdb is a lot more involved with its relation
to gdbarch, pseudo registers, values, etc. that do not exist at the gdbserver
side.  As far as I can tell, gdbserver's regcache can/should be compared with
gdb's reg_buffer.  The series I posted does not strictly aim at sharing code.
But from a general perspective, I think it would be fair to say that the
refactorings I posted bring gdbserver's regcache's behavior closer to the definitions
in gdb.  E.g. the use of REG_VALID/UNKNOWN/UNAVAILABLE statuses and the moving
of free functions to class methods.  For true code sharing, however, there is
much work to do, in my opinion.

The series at the end introduces a REG_DIRTY status as a new state. This does
not exist in the gdb side and could be considered a divergence.

Regards
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[Aktemur, Tankut Baris]

Kindly pinging.

Thanks,
-Baris

On Tuesday, February 28, 2023 12:28 PM, Aktemur, Tankut Baris wrote:
> Hello,
> 
> Gdbserver's regcache is defined and used in a way that it is either
> invalid or fetches all the registers from the target prior to being
> used.  It also seems some regcache functions have two different contracts
> based on argument values (e.g. a buffer being nullptr).
> 
> This is an attempt to refactor the regcache in gdbserver to
> 
>   - convert several free functions to methods.
>   - use and update register statuses more consistently.
>   - allow populating register values gradually, instead of having to
>     fetch all register values from the target.
> 
> The last item above is utilized in our (Intel) downstream debugger.
> No gdbserver low target is modified to utilize that feature in this
> series.
> 
> Regression-tested on X86_64 Linux using the native-gdbserver and
> native-extended-gdbserver board files.
> 
> Tankut Baris Aktemur (26):
>   gdbserver: convert init_register_cache into regcache::initialize
>   gdbserver: convert new_register_cache into a regcache constructor
>   gdbserver: by-pass regcache to access tdesc only
>   gdbserver: boolify and defaultize the 'fetch' parameter of
>     get_thread_regcache
>   gdbserver: add a pointer to the owner thread in regcache
>   gdbserver: turn part of get_thread_regcache into regcache::fetch
>   gdbserver: convert regcache_cpy into regcache::copy_from
>   gdbserver: convert free_register_cache into a destructor of regcache
>   gdbserver: extract out regcache::invalidate and regcache::discard
>   gdbserver: convert registers_to_string into
>     regcache::registers_to_string
>   gdbserver: convert registers_from_string into
>     regcache::registers_from_string
>   gdbserver: convert supply_regblock to regcache::supply_regblock
>   gdbserver: convert register_data into regcache::register_data
>   gdbserver: introduce and use regcache::set_register_status
>   gdbserver: check for nullptr condition in
>     regcache::get_register_status
>   gdbserver: boolify regcache fields
>   gdbserver: rename regcache's registers_valid to registers_fetched
>   gdbsupport: fix a typo in a comment in common-regcache.h
>   gdbserver: fix the declared type of register_status in regcache
>   gdbserver: make some regcache fields private
>   gdbserver: use REG_UNKNOWN for a regcache's register statuses
>   gdbserver: zero-out register values in regcache-discard
>   gdbserver: set register statuses in registers_from_string
>   gdbserver: return tracked register status in
>     regcache_raw_read_unsigned
>   gdbserver: refuse null argument in regcache::supply_regblock
>   gdbserver: allow gradually populating and selectively storing a
>     regcache
> 
>  gdbserver/gdbthread.h          |   2 +-
>  gdbserver/linux-aarch32-low.cc |   2 +-
>  gdbserver/linux-low.cc         |  18 +-
>  gdbserver/linux-ppc-low.cc     |  12 +-
>  gdbserver/linux-s390-low.cc    |  14 +-
>  gdbserver/linux-x86-low.cc     |   9 +-
>  gdbserver/mem-break.cc         |   4 +-
>  gdbserver/proc-service.cc      |   2 +-
>  gdbserver/regcache.cc          | 305 ++++++++++++++++++++-------------
>  gdbserver/regcache.h           |  92 ++++++----
>  gdbserver/remote-utils.cc      |   2 +-
>  gdbserver/server.cc            |  16 +-
>  gdbserver/tracepoint.cc        |  25 ++-
>  gdbserver/win32-low.cc         |   2 +-
>  gdbsupport/common-regcache.h   |  11 +-
>  15 files changed, 302 insertions(+), 214 deletions(-)
> 
> --
> 2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 00/26] gdbserver: refactor regcache and allow gradually populating

[Aktemur, Tankut Baris]

Kindly pinging.

Thanks,
-Baris

On Tuesday, February 28, 2023 12:28 PM, Aktemur, Tankut Baris wrote:
> Hello,
> 
> Gdbserver's regcache is defined and used in a way that it is either
> invalid or fetches all the registers from the target prior to being
> used.  It also seems some regcache functions have two different contracts
> based on argument values (e.g. a buffer being nullptr).
> 
> This is an attempt to refactor the regcache in gdbserver to
> 
>   - convert several free functions to methods.
>   - use and update register statuses more consistently.
>   - allow populating register values gradually, instead of having to
>     fetch all register values from the target.
> 
> The last item above is utilized in our (Intel) downstream debugger.
> No gdbserver low target is modified to utilize that feature in this
> series.
> 
> Regression-tested on X86_64 Linux using the native-gdbserver and
> native-extended-gdbserver board files.
> 
> Tankut Baris Aktemur (26):
>   gdbserver: convert init_register_cache into regcache::initialize
>   gdbserver: convert new_register_cache into a regcache constructor
>   gdbserver: by-pass regcache to access tdesc only
>   gdbserver: boolify and defaultize the 'fetch' parameter of
>     get_thread_regcache
>   gdbserver: add a pointer to the owner thread in regcache
>   gdbserver: turn part of get_thread_regcache into regcache::fetch
>   gdbserver: convert regcache_cpy into regcache::copy_from
>   gdbserver: convert free_register_cache into a destructor of regcache
>   gdbserver: extract out regcache::invalidate and regcache::discard
>   gdbserver: convert registers_to_string into
>     regcache::registers_to_string
>   gdbserver: convert registers_from_string into
>     regcache::registers_from_string
>   gdbserver: convert supply_regblock to regcache::supply_regblock
>   gdbserver: convert register_data into regcache::register_data
>   gdbserver: introduce and use regcache::set_register_status
>   gdbserver: check for nullptr condition in
>     regcache::get_register_status
>   gdbserver: boolify regcache fields
>   gdbserver: rename regcache's registers_valid to registers_fetched
>   gdbsupport: fix a typo in a comment in common-regcache.h
>   gdbserver: fix the declared type of register_status in regcache
>   gdbserver: make some regcache fields private
>   gdbserver: use REG_UNKNOWN for a regcache's register statuses
>   gdbserver: zero-out register values in regcache-discard
>   gdbserver: set register statuses in registers_from_string
>   gdbserver: return tracked register status in
>     regcache_raw_read_unsigned
>   gdbserver: refuse null argument in regcache::supply_regblock
>   gdbserver: allow gradually populating and selectively storing a
>     regcache
> 
>  gdbserver/gdbthread.h          |   2 +-
>  gdbserver/linux-aarch32-low.cc |   2 +-
>  gdbserver/linux-low.cc         |  18 +-
>  gdbserver/linux-ppc-low.cc     |  12 +-
>  gdbserver/linux-s390-low.cc    |  14 +-
>  gdbserver/linux-x86-low.cc     |   9 +-
>  gdbserver/mem-break.cc         |   4 +-
>  gdbserver/proc-service.cc      |   2 +-
>  gdbserver/regcache.cc          | 305 ++++++++++++++++++++-------------
>  gdbserver/regcache.h           |  92 ++++++----
>  gdbserver/remote-utils.cc      |   2 +-
>  gdbserver/server.cc            |  16 +-
>  gdbserver/tracepoint.cc        |  25 ++-
>  gdbserver/win32-low.cc         |   2 +-
>  gdbsupport/common-regcache.h   |  11 +-
>  15 files changed, 302 insertions(+), 214 deletions(-)
> 
> --
> 2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 01/26] gdbserver: convert init_register_cache into regcache::initialize

[Simon Marchi]

On 2/28/23 06:27, Tankut Baris Aktemur via Gdb-patches wrote:
> This is a refactoring that converts the `init_register_cache` function
> to a method of the regcache struct.  During this conversion, we also
> change the return type to void.

I think this is fine.  However, with a bit more effort, I think
this could actually become a constructor, which would be nicer than
having to call an initialize function.

The only more "tricky" spot would be the use of regcache in
fast_tracepoint_ctx.  I think you could replace the regcache_initted and
regcache fields with an std::optional.  When instantiating the regcache,
you would emplace the optional, which would call the constructor at that
point.  fast_tracepoint_ctx is only ever allocated as a local variable
in gdb_collect, so using a gdb::optional there should be fine.

Another improvement to try (for a subsequent patch) would be to pass an
array_view to the initialize method (or constructor), and assert that
the register size described by the tdesc matches the size of the
buffer array_view passed.

Simon

Re: [PATCH 02/26] gdbserver: convert new_register_cache into a regcache constructor

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the `new_register_cache` function into a constructor of the
> regcache struct.  Also define a default ctor because it is used in
> other places, in particular, tracepoint.cc.
> ---
>  gdbserver/regcache.cc | 11 +++--------
>  gdbserver/regcache.h  |  8 ++++----
>  gdbserver/server.cc   |  2 +-
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 7987c406ab4..1410dd96551 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -43,7 +43,7 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>  
>        gdb_assert (proc->tdesc != NULL);
>  
> -      regcache = new_register_cache (proc->tdesc);
> +      regcache = new struct regcache (proc->tdesc);
>        set_thread_regcache_data (thread, regcache);
>      }
>  
> @@ -151,15 +151,10 @@ regcache::initialize (const target_desc *tdesc,
>  
>  #ifndef IN_PROCESS_AGENT
>  
> -struct regcache *
> -new_register_cache (const struct target_desc *tdesc)
> +regcache::regcache (const target_desc *tdesc)
>  {
> -  struct regcache *regcache = new struct regcache;
> -
>    gdb_assert (tdesc->registers_size != 0);
> -  regcache->initialize (tdesc, nullptr);
> -
> -  return regcache;
> +  initialize (tdesc, nullptr);
>  }
>  
>  void
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 15b7e2b4dff..0002726ed00 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -44,6 +44,10 @@ struct regcache : public reg_buffer_common
>  #ifndef IN_PROCESS_AGENT
>    /* One of REG_UNAVAILABLE or REG_VALID.  */
>    unsigned char *register_status = nullptr;
> +
> +  /* Constructors.  */
> +  regcache () = default;
> +  regcache (const target_desc *tdesc);

Ok, so with this patch I understood better the two modes regcache can
operate, it can allocate its own memory or use a buffer passed to the
initialize method.  I think my previous suggestion still stands.  You
could have:

  regcache (const target_desc *tdesc,  unsigned char *regbuf);
  regcache (const target_desc *tdesc);

... former using the passed-in buffer and the latter allocating its own
buffer.  I don't think we would need a default constructor then (and we
should use DISALLOW_COPY_AND_ASSIGN).

Simon

Re: [PATCH 03/26] gdbserver: by-pass regcache to access tdesc only

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The `get_thread_regcache` function has a `fetch` option to skip
> fetching the registers from the target.  It seems this option is set
> to false only at uses where we just need to access the tdesc through
> the regcache of the current thread, as in
> 
>   struct regcache *regcache = get_thread_regcache (current_thread, 0);
>   ... regcache->tdesc ...
> 
> Since the tdesc of a regcache is set from the process of the thread
> that owns the regcache, we can simplify the code to access the tdesc
> via the process, as in
> 
>   ... current_process ()->tdesc ...
> 
> This is intended to be a refactoring with no behavioral change.
> 
> Tested only for the linux-x86-low target.

Thanks, that LGTM.  You can apply it immediately, I think, it's good on
its own.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 04/26] gdbserver: boolify and defaultize the 'fetch' parameter of get_thread_regcache

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Boolify the 'fetch' parameter of the get_thread_regcache function.
> 
> All of the current uses pass true for this parameter.  Therefore, define
> its default value as true and remove the argument from the uses.
> 
> We still keep the parameter, though, to give downstream targets the
> option to obtain a regcache without having to fetch the whole
> contents.  Our (Intel) downstream target is an example.

Just to understand better, can you explain what is the rationale of your
usage of fetch == false?

In any case, this LGTM, I think you can push it on its own, it's a good
cleanup.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 05/26] gdbserver: add a pointer to the owner thread in regcache

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Add a back-link in regcache to the thread that owns the regcache.
> This will help us in future patches to refer to the right thread
> object without having to rely on the global current_thread pointer.

I'm a bit hesitant with this change as-is, because it sets the thread
field only in some instances of regcache.  There are code paths that
create regcaches but don't set it.  It adds a bit of cognitive load to
have to think about when it is set and when it isn't.

I'll have to see how it's used in the rest of the series.  Perhaps that
doesn't work, but intuitively I'm thinking that it might be better to
get the thread from the context in another way, like passing it down to
functions as an argument.

Simon

Re: [PATCH 06/26] gdbserver: turn part of get_thread_regcache into regcache::fetch

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Extract out a piece of code in get_thread_regcache that fetches the
> registers from the target, and turn it into a method of 'regcache'.
> ---
>  gdbserver/regcache.cc | 23 +++++++++++++++--------
>  gdbserver/regcache.h  |  3 +++
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 2a8dc17ed6a..89ecdfec6f3 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -48,19 +48,26 @@ get_thread_regcache (struct thread_info *thread, bool fetch)
>        regcache->thread = thread;
>      }
>  
> -  if (fetch && regcache->registers_valid == 0)
> +  if (fetch)
> +    regcache->fetch ();
> +
> +  return regcache;
> +}
> +
> +void
> +regcache::fetch ()
> +{
> +  if (registers_valid == 0)
>      {
>        scoped_restore_current_thread restore_thread;
> +      gdb_assert (this->thread != nullptr);
> +      switch_to_thread (this->thread);
>  
> -      switch_to_thread (thread);
>        /* Invalidate all registers, to prevent stale left-overs.  */
> -      memset (regcache->register_status, REG_UNAVAILABLE,
> -	      regcache->tdesc->reg_defs.size ());
> -      fetch_inferior_registers (regcache, -1);
> -      regcache->registers_valid = 1;
> +      memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
> +      fetch_inferior_registers (this, -1);
> +      registers_valid = 1;
>      }
> -
> -  return regcache;

Ok, so this is one of the uses of the regcache::thread field.  Given
that, as of today (before your series), a thread knows its regcache, but
a regcache doesn't know its thread (if any), it could be the other way
around.  You could have a thread_info::fetch_registers method that does
the above.  This way, the regcache stays a relatively dumb box of
register data, oblivious to where the bytes come from.  Again, I don't
have yet the full context of the rest of the series.

Another improvement I'd like to see: would it be relatively easy to
change fetch_inferior_registers to pass the thread down, and avoid the
switch_to_thread?  I guess you wouldn't need to pass the regcache down
anymore, because the target function would know it needs to put the
register data in thread->regcache.  Doing the same in GDB is an enormous
amount of work, but I peeked at the GDBserver code, and it seems
do-able.

Simon

Re: [PATCH 07/26] gdbserver: convert regcache_cpy into regcache::copy_from

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the free `regcache_cpy` function to a method of the
> regcache struct.

I think this is ok to push right away, it's a good cleanup on its own.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 08/26] gdbserver: convert free_register_cache into a destructor of regcache

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the `free_register_cache` function into a destructor of the
> regcache struct.  In one place, we completely remove the call to free
> the regcache object by stack-allocating the object.
> 
> We also delete the copy constructor explicitly to prevent the risk of
> copying regcaches and then double-freeing buffers when they are
> destructed.
> ---
>  gdbserver/gdbthread.h |  2 +-
>  gdbserver/regcache.cc | 15 +++++----------
>  gdbserver/regcache.h  |  9 +++++----
>  gdbserver/server.cc   |  8 +++-----
>  4 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 493e1dbf6cb..cfd81870af9 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -35,7 +35,7 @@ struct thread_info
>  
>    ~thread_info ()
>    {
> -    free_register_cache (this->regcache_data);
> +    delete this->regcache_data;

Perhaps in another patch, but you could make regcache_data a unique_ptr
(I quickly skimmed the subjects of the rest of the series and didn't see
a patch that would do that).

>    }
>  
>    /* The id of this thread.  */
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index be01df342bb..7b6337166ad 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -165,16 +165,11 @@ regcache::regcache (const target_desc *tdesc)
>    initialize (tdesc, nullptr);
>  }
>  
> -void
> -free_register_cache (struct regcache *regcache)
> +regcache::~regcache ()
>  {
> -  if (regcache)
> -    {
> -      if (regcache->registers_owned)
> -	free (regcache->registers);
> -      free (regcache->register_status);
> -      delete regcache;
> -    }
> +  if (registers_owned)
> +    free (registers);
> +  free (register_status);
>  }
>  
>  #endif
> @@ -280,7 +275,7 @@ free_register_cache_thread (struct thread_info *thread)
>    if (regcache != NULL)
>      {
>        regcache_invalidate_thread (thread);
> -      free_register_cache (regcache);
> +      delete regcache;
>        set_thread_regcache_data (thread, NULL);
>      }
>  }
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 32b3a8dccfc..614d5a2561f 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -51,6 +51,11 @@ struct regcache : public reg_buffer_common
>    /* Constructors.  */
>    regcache () = default;
>    regcache (const target_desc *tdesc);
> +  regcache (const regcache &rhs) = delete;
> +  regcache &operator= (const regcache &rhs) = delete;

Use DISALLOW_COPY_AND_ASSIGN.  And I think it wouldn't hurt to put it
outside of the `#ifndef IN_PROCESS_AGENT`.  If you follow my previous
suggestions, the constructor with 2 arguments would also need to be
outside the ifndef.

> +
> +  /* Deconstructor.  */
> +  virtual ~regcache ();

Deconstructor -> Destructor :)

But really, I don't think it's necessary, anybody slightly familiar with
C++ will know this is the destructor... as you wish.

I guess you might need in the rest of the series, but as of now the
destructor doesn't need to be virtual, so I wouldn't make it virtual
in this patch.

Simon

Re: [PATCH 09/26] gdbserver: extract out regcache::invalidate and regcache::discard

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Extract out a piece of code from the `regcache_invalidate_thread`
> function and turn into a new method of regcache named 'invalidate'.

For clarity, if you can think of a better name than invalidate, it would
be nice.  To me, invalidate sounds like we just mark the registers as
stale (what you discard method does, basically).  This method writes the
registers back to the target and invalidates / discard.

> We also introduce a small method named 'discard' to give the clients
> an option to discard the cache without storing the contents.  This
> method is utilized in a downstream debugger.

I would perhaps suggest keeping the discard method as a downstream
modification, since it's trivial (and therefore doesn't need much
maintenance effort to keep up to date).

> ---
>  gdbserver/regcache.cc | 24 +++++++++++++++++-------
>  gdbserver/regcache.h  |  6 ++++++
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 7b6337166ad..1db78951cc2 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -85,18 +85,22 @@ regcache_invalidate_thread (struct thread_info *thread)
>  
>    regcache = thread_regcache_data (thread);
>  
> -  if (regcache == NULL)
> -    return;
> +  if (regcache != nullptr)
> +    regcache->invalidate ();
> +}
>  
> -  if (regcache->registers_valid)
> +void
> +regcache::invalidate ()

Add the typical:

/* See regcache.h.  */

> +{
> +  if (registers_valid)
>      {
>        scoped_restore_current_thread restore_thread;
> -
> -      switch_to_thread (thread);
> -      store_inferior_registers (regcache, -1);
> +      gdb_assert (this->thread != nullptr);
> +      switch_to_thread (this->thread);
> +      store_inferior_registers (this, -1);

Ok, so I note that this is another place where you use regcache::thread
(which makes sense, it's the mirror operation to fetch).  I guess that
the same idea could be use there, it could be either a method on
thread_info, or event stay a free function.

Similar comment as earlier, as a general improvement, it would be nice
if we could get rid of this switch_to_thread.

>      }
>  
> -  regcache->registers_valid = 0;
> +  discard ();

Why is it necessary to mark the register content invalid here?  In
theory, it should be valid as long as we don't resume the thread.  It's
already done like that, so it's fine to keep it as-is, I'm just
wondering.

Simon

Re: [PATCH 10/26] gdbserver: convert registers_to_string into regcache::registers_to_string

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the free `registers_to_string` function into a method of
> the regcache struct.

I'm not strongly opposed to this change, but I think that it works just
as well as a free function.  Not everything working on a regcache needs
to be a method of regcache.  If you imagine that all the members of
regcache are private, and it offers an interface to access the register
contents and statuses, then registers_to_string can very well be
implemented using that public interface.

Simon

Re: [PATCH 10/26] gdbserver: convert registers_to_string into regcache::registers_to_string

[Simon Marchi]

On 12/21/23 16:13, Simon Marchi wrote:
> On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
>> Convert the free `registers_to_string` function into a method of
>> the regcache struct.
> 
> I'm not strongly opposed to this change, but I think that it works just
> as well as a free function.  Not everything working on a regcache needs
> to be a method of regcache.  If you imagine that all the members of
> regcache are private, and it offers an interface to access the register
> contents and statuses, then registers_to_string can very well be
> implemented using that public interface.

To clarify, I think that this functionality might belong more in the
remote-utils.cc's area of responsibility, since it's serializing the
regcache's content specifically for the remote protocol.

Simon

Re: [PATCH 12/26] gdbserver: convert supply_regblock to regcache::supply_regblock

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the supply_regblock function into a method of the regcache
> struct.  Also do some minor code modernization.
> ---
>  gdbserver/regcache.cc   | 32 ++++++++------------------------
>  gdbserver/regcache.h    |  7 +++++--
>  gdbserver/tracepoint.cc |  8 ++++----
>  3 files changed, 17 insertions(+), 30 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 156fda7068d..31f1e7bb3dc 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -376,39 +376,23 @@ supply_register_by_name_zeroed (struct regcache *regcache,
>  
>  #endif
>  
> -/* Supply the whole register set whose contents are stored in BUF, to
> -   REGCACHE.  If BUF is NULL, all the registers' values are recorded
> -   as unavailable.  */
> -
>  void
> -supply_regblock (struct regcache *regcache, const void *buf)
> +regcache::supply_regblock (const void *buf)

Add `/* See regcache.h.  */`.

Can you investigate if the buf == nullptr case is really needed?  It
seems to me like the only times we call supply_regblock with nullptr is
just after creating a fresh regcache.  Since the regcache is created
with all registers in the REG_UNAVAILABLE state, perhaps those spots can
just omit calling supply_regblock, and supply_regblock can be
simplified?

Simon

Re: [PATCH 13/26] gdbserver: convert register_data into regcache::register_data

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Convert the register_data function to a method of the regcache struct.
> ---
>  gdbserver/regcache.cc | 20 ++++++++++----------
>  gdbserver/regcache.h  |  3 +++
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 31f1e7bb3dc..79835ef4ff1 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -314,11 +314,11 @@ regcache_register_size (const struct regcache *regcache, int n)
>    return register_size (regcache->tdesc, n);
>  }
>  
> -static unsigned char *
> -register_data (const struct regcache *regcache, int n)
> +unsigned char *
> +regcache::register_data (int regnum) const

Just a note: post-rebasing, this would return a gdb::array_view<gdb_byte>.

Simon

Re: [PATCH 14/26] gdbserver: introduce and use regcache::set_register_status

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Introduce and use a setter method in regcache to set the status of a
> register.  There already exists get_register_status.  So, it made
> sense to add the setter to control access to the register_status
> field.

Does this method (and eventually the register_status field) need to be
public?  Or is it only set internally, through some other methods?

> @@ -498,6 +495,16 @@ regcache::get_register_status (int regnum) const
>  #endif
>  }
>  
> +void
> +regcache::set_register_status (int regnum, enum register_status status)
> +{
> +#ifndef IN_PROCESS_AGENT
> +  gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
> +  if (register_status != nullptr)
> +    register_status[regnum] = status;
> +#endif

I don't understand why some spots (like here) check for register_status
being nullptr, but others (like get_register_status) don't.

Simon

Re: [PATCH 15/26] gdbserver: check for nullptr condition in regcache::get_register_status

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> A regcache can be initialized with a register value buffer, in which
> case, the register_status pointer is null.  This condition is checked
> in set_register_status, but not in get_register_status.  Do this check
> for consistence and safety.

Ok, thanks, that answers my question on the previous patch :).

Simon

Re: [PATCH 15/26] gdbserver: check for nullptr condition in regcache::get_register_status

[Simon Marchi]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> A regcache can be initialized with a register value buffer, in which
> case, the register_status pointer is null.  This condition is checked
> in set_register_status, but not in get_register_status.  Do this check
> for consistence and safety.
> ---
>  gdbserver/regcache.cc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index ec11082be6f..0c6f1eb392b 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -489,7 +489,10 @@ regcache::get_register_status (int regnum) const
>  {
>  #ifndef IN_PROCESS_AGENT
>    gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
> -  return (enum register_status) (register_status[regnum]);
> +  if (register_status != nullptr)
> +    return (enum register_status) (register_status[regnum]);
> +  else
> +    return REG_VALID;
>  #else
>    return REG_VALID;
>  #endif
> -- 
> 2.25.1

Thanks, that LGTM, I think it can be pushed on its own.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 16/26] gdbserver: boolify regcache fields

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The registers_valid and registers_owned fields of the regcache struct
> are of type int.  Make them bool.

I think that can be pushed right away (well, perhaps it doesn't apply
cleanly without the previous patches, but some equivalent version of
this can be pushed right away).

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 17/26] gdbserver: rename regcache's registers_valid to registers_fetched

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The registers_valid field of the regcache struct is used for tracking
> whether we have attempted to fetch all the registers from the target.
> Its name does not reflect this well, I think.  It falsely gives the
> impression that all the registers are valid.  This may conflict an
> individual register status, which could be REG_UNAVAILABLE.  To better
> reflect the purpose, rename the field to "registers_fetched".

I agree that the term valid is used for both things here.  I'm fine with
the change.  Again, I think that can be pushed right away.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 18/26] gdbsupport: fix a typo in a comment in common-regcache.h

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Fix a typo.
> ---
>  gdbsupport/common-regcache.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
> index e462f532407..bf14610f98f 100644
> --- a/gdbsupport/common-regcache.h
> +++ b/gdbsupport/common-regcache.h
> @@ -33,10 +33,10 @@ enum register_status : signed char
>  
>      /* The register value is unavailable.  E.g., we're inspecting a
>         traceframe, and this register wasn't collected.  Note that this
> -       is different a different "unavailable" from saying the register
> -       does not exist in the target's architecture --- in that case,
> -       the target should have given us a target description that does
> -       not include the register in the first place.  */
> +       "unavailable" is different from saying the register does not
> +       exist in the target's architecture --- in that case, the target
> +       should have given us a target description that does not include
> +       the register in the first place.  */
>      REG_UNAVAILABLE = -1
>    };
>  

LGTM, I think that can be pushed right away.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 19/26] gdbserver: fix the declared type of register_status in regcache

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The register_status field of regcache is declared as `unsigned char *`.
> This is incorrect, because `enum register_status` from
> gdbsupport/common-regcache.h is based on signed char and
> REG_UNAVAILABLE is defined as -1.  Fix the declared type.
> 
> The get/set methods already use the correct type, but we update cast
> operations in two places.
> ---
>  gdbserver/regcache.cc | 4 ++--
>  gdbserver/regcache.h  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 0e21c1aa7d1..09ea58bdbd6 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -147,7 +147,7 @@ regcache::initialize (const target_desc *tdesc,
>  	= (unsigned char *) xcalloc (1, tdesc->registers_size);
>        this->registers_owned = true;
>        this->register_status
> -	= (unsigned char *) xmalloc (tdesc->reg_defs.size ());
> +	= (enum register_status *) xmalloc (tdesc->reg_defs.size ());

The malloc'ed size assumes that a register_status value is 1 byte long.
register_status is indeed 1 byte long, but since it's hidden behind
another name, it's not obvious.  You could perhaps switch to:

  this->register_status = XNEWVEC (enum register_status, tdesc->reg_defs.size ());

Or C++ify it:

  this->register_status = new enum register_status[tdesc->reg_defs.size ()];

But then you have to change the free to a delete[], or store it in an
std::unique_ptr<enum register_status[]>.

Simon

Re: [PATCH 20/26] gdbserver: make some regcache fields private

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Some fields of the regcache struct are used for internal state
> tracking.  Prevent direct access to them from outside by making them
> private.

This is a good step, but it would be nice to:

 - rename private fields to prefix them with `m_`.  When accessing
   fields that are not prefixed with `m_`, I like to use `this->`,
   otherwise it's not obvious what you are acessing.  With the prefix
   though, I don't think it's necessary, as the naming makes it obvious.
   It then makes the code a bit more concise.
 - reorder members of the class to be somewhat in standard order.  I
   don't think we have a strict rule about this, but it's typically:
   constructor, destructor, public methods, public fields, private
   methods, private fields.

I don't mind if this is done by editing the patches in this series, or
as a change on top, if it's easier.

Simon

Re: [PATCH 21/26] gdbserver: use REG_UNKNOWN for a regcache's register statuses

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> When a regcache is initialized, the values of registers are not
> fetched yet.  Thus, initialize the register statuses to REG_UNKNOWN
> instead of REG_UNAVAILABLE, because the latter rather means "we
> attempted to fetch but could not obtain the value".
> 
> The definitions of the reg status enums (from
> gdbsupport/common-regcache.h) as a reminder:
> 
>     /* The register value is not in the cache, and we don't know yet
>        whether it's available in the target (or traceframe).  */
>     REG_UNKNOWN = 0,
> 
>     /* The register value is valid and cached.  */
>     REG_VALID = 1,
> 
>     /* The register value is unavailable.  E.g., we're inspecting a
>        traceframe, and this register wasn't collected.  Note that this
>        "unavailable" is different from saying the register does not
>        exist in the target's architecture --- in that case, the target
>        should have given us a target description that does not include
>        the register in the first place.  */
>     REG_UNAVAILABLE = -1
> 
> Similarly, when the regcache is invalidated, change all the statuses
> back to REG_UNKNOWN.

That makes sense to me.

> ---
>  gdbserver/regcache.cc | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 09ea58bdbd6..2befb30e337 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -64,9 +64,17 @@ regcache::fetch ()
>        switch_to_thread (this->thread);
>  
>        /* Invalidate all registers, to prevent stale left-overs.  */
> -      memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
> +      discard ();
>        fetch_inferior_registers (this, -1);
>        registers_fetched = true;
> +
> +      /* Make sure that the registers that could not be fetched are
> +	 now unavailable.  */
> +      for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> +	{
> +	  if (register_status[i] == REG_UNKNOWN)
> +	    set_register_status (i, REG_UNAVAILABLE);
> +	}

The braces are not needed.

But is it really needed to do this?  Unavailable is only meaningful for
tracing, in a regcache that is the result of tracepoint collection.  For
a regcache created by reading registers from the target, I don't think
that setting statuses to unavailable makes sense.

Also, if asking the target to fetch all registers, why would some
registers still be unknown?  It's the target that provides the target
description, it's supposed to only include registers that exist (like
the comment on top of REG_UNAVAILABLE says).  So it should be able to
read them all.  In other words, I think this loop should be asserting
that all statuses are REG_VALID.

But then for the tracing case, I wonder who sets the statuses to
REG_UNAVAILABLE for those statuses that should really be
REG_UNAVAILABLE.  From what I can see, all regcaches used on the tracing
code paths use the regcache constructor with 2 arguments, which means
regcaches that don't track the status of registers
(regcache::register_status is nullptr).  So... now that you changed
regcache to use REG_UNKNOWN by default, is REG_UNAVAILABLE really
used in GDBserver?

I noticed that do_action_at_tracepoint has a comment that says "Collect
all registers for now.".  I guess that if it didn't collect all
registers, this would be a spot that would explicitly set some registers
to REG_UNAVAILABLE.

I see that for fast tracepoints, for instance
supply_fast_tracepoint_registers in linux-amd64-ipa.cc, we supply a
fixed set of registers.  Clearly, these are not all the registers that
an amd64 machine could have.  But it just writes a register buffer that
ends up in the trace, there doesn't seem to be a way to communicate
which registers were collected and which weren't.  When we create a new
regcache to read these registers, when handling the 'g' packet in
server.cc, there's no way to know which individual registers were
collected and which weren't.

Simon

Re: [PATCH 22/26] gdbserver: zero-out register values in regcache-discard

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Zero-out register values when a regcache is discarded so that we avoid
> garbage values left in the buffer.
> ---
>  gdbserver/regcache.cc | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 2befb30e337..644f436c681 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -136,6 +136,7 @@ regcache_invalidate (void)
>  void
>  regcache::discard ()
>  {
> +  memset (registers, 0, tdesc->registers_size);
>  #ifndef IN_PROCESS_AGENT
>    memset ((void *) register_status, REG_UNKNOWN, tdesc->reg_defs.size ());
>  #endif
> @@ -149,16 +150,17 @@ regcache::initialize (const target_desc *tdesc,
>    if (regbuf == NULL)
>      {
>  #ifndef IN_PROCESS_AGENT
> -      /* Make sure to zero-initialize the register cache when it is
> -	 created, in case there are registers the target never
> -	 fetches.  This way they'll read as zero instead of
> -	 garbage.  */
>        this->tdesc = tdesc;
>        this->registers
> -	= (unsigned char *) xcalloc (1, tdesc->registers_size);
> +	= (unsigned char *) xmalloc (tdesc->registers_size);
>        this->registers_owned = true;
>        this->register_status
>  	= (enum register_status *) xmalloc (tdesc->reg_defs.size ());
> +
> +      /* Make sure to zero-initialize the register cache when it is
> +	 created, in case there are registers the target never
> +	 fetches.  This way they'll read as zero instead of
> +	 garbage.  */
>        discard ();
>  #else
>        gdb_assert_not_reached ("can't allocate memory from the heap");

Just curious, if we read and use the contents of a register that isn't
REG_VALID, it's a bug, right?  If so, shouldn't we instead make sure
this never happens?  After all, for a register that is not REG_VALID, a
value of 0 is just as much a "garbage value" as any other value.

Simon

Re: [PATCH 23/26] gdbserver: set register statuses in registers_from_string

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The registers_from_string function uses hex2bin to set the values of
> all registers.  Set the register statuses to REG_VALID to reflect this
> change.
> ---
>  gdbserver/regcache.cc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 644f436c681..32f0e1109e6 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -259,6 +259,8 @@ regcache::registers_from_string (const char *buf)
>  	len = tdesc->registers_size * 2;
>      }
>    hex2bin (buf, registers, len / 2);
> +  /* All register data have been re-written.  Update the statuses.  */
> +  memset (register_status, REG_VALID, tdesc->reg_defs.size ());
>  }
>  
>  /* See regcache.h */

Well... technically, registers_from_string is written in a way that if
the input buffer is too short, the registers at the end will not be
written (don't know why it's written like that, instead of erroring out,
but that's how it is right now).  So you should only set to REG_VALID the
registers that are actually completely written, setting the rest to
REG_UNKNOWN.

Simon

Re: [PATCH 24/26] gdbserver: return tracked register status in regcache_raw_read_unsigned

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> In regcache_raw_read_unsigned, we unconditionally return REG_VALID as
> the register status.  This does not seem right, since the register may
> in fact be in another state, such as REG_UNAVAILABLE.  Return the
> tracked status.
> ---
>  gdbserver/regcache.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 32f0e1109e6..4533e9e9b12 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -451,7 +451,7 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
>    *val = 0;
>    collect_register (regcache, regnum, val);
>  
> -  return REG_VALID;
> +  return regcache->get_register_status (regnum);
>  }
>  
>  #ifndef IN_PROCESS_AGENT

LGTM.  You can push it right away, if you think it makes sense.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 25/26] gdbserver: refuse null argument in regcache::supply_regblock

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> The regcache::supply_regblock method implements two different
> behaviors based on whether its 'buf' argument is null.  The null
> behavior is used only in tracepoint.cc.  Refuse the null pointer
> argument and use the 'discard' method to obtain that second behavior.
> 
> Note that the original code resets register statuses to
> REG_UNAVAILABLE, but 'discard' sets them to REG_UNKNOWN.  For the
> current purposes of resetting the regcache, the difference does not
> seem to be important.

Ah, glad to see this, it matches one of my comment on an earlier patch
(and as you can see, I read patch series in a very linear way with no
lookahead).

In the spots inside the in-process agent, the regcache doesn't even
track statuses, so it doesn't make a difference indeed.

> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index 9833e7c3b0f..9622d53cd82 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -4707,7 +4707,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
>  	{
>  	  fctx->regcache_initted = 1;
>  	  fctx->regcache.initialize (ipa_tdesc, fctx->regspace);
> -	  fctx->regcache.supply_regblock (nullptr);
> +	  fctx->regcache.discard ();
>  	  supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs);

I think the call to discard is pointless.  The call to discard does (and
should) pretty much bring the regcache back to its state just after
initialization.  In this case, it was initialized at the line just before.

>  	}
>        regcache = &fctx->regcache;
> @@ -4722,7 +4722,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
>  	{
>  	  sctx->regcache_initted = 1;
>  	  sctx->regcache.initialize (ipa_tdesc, sctx->regspace);
> -	  sctx->regcache.supply_regblock (nullptr);
> +	  sctx->regcache.discard ();

Same here.

>  	  /* Pass down the tracepoint address, because REGS doesn't
>  	     include the PC, but we know what it must have been.  */
>  	  supply_static_tracepoint_registers (&sctx->regcache,
> @@ -5179,8 +5179,8 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum)
>    dataptr = traceframe_find_regblock (tframe, tfnum);
>    if (dataptr == NULL)
>      {
> -      /* Mark registers unavailable.  */
> -      regcache->supply_regblock (nullptr);
> +      /* Wipe out the cache.  */
> +      regcache->discard ();

So, I think this is a spot where you would actually want to make the
registers REG_UNAVAILABLE.  We are filling a regcache meant to represent
the registers that were collected when hitting a given tracepoint.  If
traceframe_find_regblock returns nullptr, I guess it means that we
didn't collect the registers.  So all registers are unavailable.

In practice, it won't make a difference, because that regcache is then
fed to registers_to_string, which will have the same behavior for
REG_UNAVAILABLE and REG_UNKNOWN.  But if you want to be accurate, I
think that REG_UNAVAILABLE makes more sense here.

Simon

Re: [PATCH 26/26] gdbserver: allow gradually populating and selectively storing a regcache

[Simon Marchi]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Currently, the regcache can be used by fetching all the registers from
> the target.  For some targets, this could be a costly operation
> because there is a large number of threads with a large register file
> each.  In this patch, we revise the regcache implementation to allow
> populating the contents gradually and storing the registers only when
> they have updated values.

When reading the subject (gradually populating) and the commit message
(especially the paragraph above), I expected that the patch would make
fetching registers from the target (and filling up the regcache) more
lazy.  This is the picture I had in mind: the regcache would start with
all REG_UNKNOWN statuses.  When a caller asks for a given register
value, we would fetch that register value from the target, and its
status would become REG_VALID.

However, I see that regcache::fetch still asks the target to fetch all
registers from the start.  Is my understanding of what you're trying to
achieve wrong?

> 
> To this aim, we introduce a new register status: REG_DIRTY.  This
> status denotes that a register value has been cached and also updated.
> When invalidating the cache, only the dirty registers are stored to
> the target.  In a typical debug session, it is more likely that only a
> small subset of the register file has changed.  Therefore, selectively
> storing the registers on targets with many threads and registers can
> save substantial costs, with respect to storing the whole set.

Just curious, can you share some real world experience about those cost
savings?  I'm guessing we're talking about GPU registers here?

> The collect_register function now performs a lazy fetch.  If the
> requested register value is not cached yet, it is requested from the
> target.
> 
> The supply_register function updates the status of the supplied
> register as follows: if the register was not available in the cache,
> its status becomes REG_VALID, denoting that the value is now cached.
> If the register is supplied again, it becomes REG_DIRTY.

I don't understand the logic here.  If a register is in the REG_UNKNOWN
state and I supply a value through regcache->raw_supply, I think it
should go to the REG_DIRTY status.  We don't know what the value of the
register on the target is, it is only safe to assume that it's not the
same value as what was supplied.

> 
> The function that supply the whole register set (supply_regblock and
> registers_from_string) are also updated to compare the present and new
> values of each register, so that we can track the register statuses
> (i.e.  dirty or not) properly.
> 
> Regression-tested on an X86_64 Linux target using the native-gdbserver
> and native-extended-gdbserver board files.
> ---
>  gdbserver/regcache.cc        | 96 ++++++++++++++++++++++++++++++------
>  gdbserver/regcache.h         |  6 +++
>  gdbsupport/common-regcache.h |  3 ++
>  3 files changed, 91 insertions(+), 14 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index cfb68774402..cf020985c31 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -63,6 +63,18 @@ regcache::fetch ()
>        gdb_assert (this->thread != nullptr);
>        switch_to_thread (this->thread);
>  
> +      /* If there are individually-fetched dirty registers, first
> +	 store them, then fetch all.  We prefer this to doing
> +	 individual fetch for each registers, if needed, because it is
> +	 more likely that very few registers are individually-fetched
> +	 at this moment and that fetching all in one go is more
> +	 efficient than fetching each reg one by one.  */
> +      for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> +	{
> +	  if (register_status[i] == REG_DIRTY)
> +	    store_inferior_registers (this, i);
> +	}
> +
>        /* Invalidate all registers, to prevent stale left-overs.  */
>        discard ();
>        fetch_inferior_registers (this, -1);
> @@ -100,12 +112,17 @@ regcache_invalidate_thread (struct thread_info *thread)
>  void
>  regcache::invalidate ()
>  {
> -  if (registers_fetched)
> +  scoped_restore_current_thread restore_thread;
> +  gdb_assert (this->thread != nullptr);
> +  switch_to_thread (this->thread);
> +
> +  /* Store dirty registers individually.  We prefer this to a
> +     store-all, because it is more likely that a small number of
> +     registers have changed.  */
> +  for (int i = 0; i < tdesc->reg_defs.size (); ++i)
>      {
> -      scoped_restore_current_thread restore_thread;
> -      gdb_assert (this->thread != nullptr);
> -      switch_to_thread (this->thread);
> -      store_inferior_registers (this, -1);
> +      if (register_status[i] == REG_DIRTY)
> +	store_inferior_registers (this, i);
>      }

I think there's a design problem here: from what I understand, it's
possible to get in a situation where all registers are REG_UNKNOWN,
except one that is REG_DIRTY.  When you "invalidate" the regcache, we'll
call store_inferior_registers for the register that is REG_DIRTY.
However, linux_process_target::fetch_registers, for instance, will write
all registers in a given regset when asked to store one register
contained in that regset.  So it will end up writing garbage data for
all the registers in that regset that were REG_UNKNOWN.

>  
>    discard ();
> @@ -231,7 +248,8 @@ regcache::registers_to_string (char *buf)
>    unsigned char *regs = registers;
>    for (int i = 0; i < tdesc->reg_defs.size (); ++i)
>      {
> -      if (register_status[i] == REG_VALID)
> +      if (register_status[i] == REG_VALID
> +	  || register_status[i] == REG_DIRTY)
>  	{
>  	  bin2hex (regs, buf, register_size (tdesc, i));
>  	  buf += register_size (tdesc, i) * 2;
> @@ -258,9 +276,12 @@ regcache::registers_from_string (const char *buf)
>        if (len > tdesc->registers_size * 2)
>  	len = tdesc->registers_size * 2;
>      }
> -  hex2bin (buf, registers, len / 2);
> -  /* All register data have been re-written.  Update the statuses.  */
> -  memset (register_status, REG_VALID, tdesc->reg_defs.size ());
> +
> +  unsigned char *new_regs =
> +    (unsigned char *) alloca (tdesc->registers_size);

I would prefer to use a gdb::byte_vector here, instead of alloca.

Simon