[PATCH] gdb: reintroduce reg_buffer::m_readonly in place of m_has_pseudo

[PATCH] gdb: reintroduce reg_buffer::m_readonly in place of m_has_pseudo

[Guinevere Larsen]

A series of commits obfuscated the meaning of reg_buffer:m_has_pseudo,
most notably for the current code being commit
daf6667d1f94c7e74df4076daf021cd28a2, that changed the checks in
regcache::save and regcache::restore from checking if the cache was
readonly to checking if the cache had registers.

There was no reason provided for that change, but following commits
started to use a parameters called "readonly" to set m_has_pseudo, until
a following commit only used the literal "true" and "false" values, but
still mapping perfectly on whether the register cache will be read only,
and not taking the gdbarch's pseudo registers or specific needs of a
cache's location into account.

The current usages in regcache::save and ::restore were just changed to
use m_readonly, making the code more understandable.  Two other uses, in
readable_regcache::cooked_read and ::cooked_read_value, have just been
removed, and the final 2 uses, in reg_buffer's constructor and in
reg_buffer::assert_regnum, were changed to always act as if the buffer
has pseudo registers.

I chose to make reg_buffer always allocate space for pseudo-registers
because the cooker_read methods act as if it was possible to cache a
pseudo register's value, but as far as I could see, there wasn't any
code doing it. Since it would be done in some tdep file and I didn't
feel confident in saying no code at all does this caching, and thought
it would be safer to allocate space for it.

Tested on x86_64 Fedora 40
---
 gdb/regcache-dump.c | 18 +++++++-----------
 gdb/regcache.c      | 35 ++++++++++++-----------------------
 gdb/regcache.h      | 19 +++++++++++--------
 3 files changed, 30 insertions(+), 42 deletions(-)

diff --git a/gdb/regcache-dump.c b/gdb/regcache-dump.c
index bc665dc08a6..6a6633e49dd 100644
--- a/gdb/regcache-dump.c
+++ b/gdb/regcache-dump.c
@@ -32,8 +32,7 @@ class register_dump_regcache : public register_dump
 {
 public:
   register_dump_regcache (regcache *regcache, bool dump_pseudo)
-    : register_dump (regcache->arch ()), m_regcache (regcache),
-      m_dump_pseudo (dump_pseudo)
+    : register_dump (regcache->arch (), dump_pseudo), m_regcache (regcache)
   {
   }
 
@@ -80,9 +79,6 @@ class register_dump_regcache : public register_dump
 
 private:
   regcache *m_regcache;
-
-  /* Dump pseudo registers or not.  */
-  const bool m_dump_pseudo;
 };
 
 /* Dump from reg_buffer, used when there is no thread or
@@ -92,7 +88,7 @@ class register_dump_reg_buffer : public register_dump, reg_buffer
 {
 public:
   register_dump_reg_buffer (gdbarch *gdbarch, bool dump_pseudo)
-    : register_dump (gdbarch), reg_buffer (gdbarch, dump_pseudo)
+    : register_dump (gdbarch, dump_pseudo), reg_buffer (gdbarch, true)
   {
   }
 
@@ -101,14 +97,14 @@ class register_dump_reg_buffer : public register_dump, reg_buffer
   {
     if (regnum < 0)
       {
-	if (m_has_pseudo)
+	if (m_dump_pseudo)
 	  gdb_printf (file, "Cooked value");
 	else
 	  gdb_printf (file, "Raw value");
       }
     else
       {
-	if (regnum < gdbarch_num_regs (m_gdbarch) || m_has_pseudo)
+	if (regnum < gdbarch_num_regs (m_gdbarch) || m_dump_pseudo)
 	  {
 	    auto size = register_size (m_gdbarch, regnum);
 
@@ -140,7 +136,7 @@ class register_dump_none : public register_dump
 {
 public:
   register_dump_none (gdbarch *arch)
-    : register_dump (arch)
+    : register_dump (arch, false)
   {}
 
 protected:
@@ -154,7 +150,7 @@ class register_dump_remote : public register_dump
 {
 public:
   register_dump_remote (gdbarch *arch)
-    : register_dump (arch)
+    : register_dump (arch, false)
   {}
 
 protected:
@@ -181,7 +177,7 @@ class register_dump_groups : public register_dump
 {
 public:
   register_dump_groups (gdbarch *arch)
-    : register_dump (arch)
+    : register_dump (arch, true)
   {}
 
 protected:
diff --git a/gdb/regcache.c b/gdb/regcache.c
index f04354d822f..135238e5be3 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -185,8 +185,8 @@ regcache_register_size (const reg_buffer_common *regcache, int n)
     (gdb::checked_static_cast<const struct regcache *> (regcache)->arch (), n);
 }
 
-reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
-  : m_has_pseudo (has_pseudo)
+reg_buffer::reg_buffer (gdbarch *gdbarch, bool readonly)
+  : m_readonly (readonly)
 {
   gdb_assert (gdbarch != NULL);
   m_descr = regcache_descr (gdbarch);
@@ -194,18 +194,9 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
   /* We don't zero-initialize the M_REGISTERS array, as the bytes it contains
      aren't meaningful as long as the corresponding register status is not
      REG_VALID.  */
-  if (has_pseudo)
-    {
-      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
-      m_register_status.reset
-	(new register_status[m_descr->nr_cooked_registers] ());
-    }
-  else
-    {
-      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers]);
-      m_register_status.reset
-	(new register_status[gdbarch_num_regs (gdbarch)] ());
-    }
+  m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
+  m_register_status.reset
+    (new register_status[m_descr->nr_cooked_registers] ());
 }
 
 regcache::regcache (inferior *inf_for_target_calls, gdbarch *gdbarch)
@@ -264,8 +255,9 @@ reg_buffer::save (register_read_ftype cooked_read)
 {
   struct gdbarch *gdbarch = m_descr->gdbarch;
 
-  /* It should have pseudo registers.  */
-  gdb_assert (m_has_pseudo);
+  /* This method should only be called when we're creating a
+     readonly cache.  */
+  gdb_assert (m_readonly);
   /* Clear the dest.  */
   memset (m_registers.get (), 0, m_descr->sizeof_cooked_registers);
   memset (m_register_status.get (), REG_UNKNOWN, m_descr->nr_cooked_registers);
@@ -297,7 +289,7 @@ regcache::restore (readonly_detached_regcache *src)
   int regnum;
 
   gdb_assert (src != NULL);
-  gdb_assert (src->m_has_pseudo);
+  gdb_assert (src->m_readonly);
 
   gdb_assert (gdbarch == src->arch ());
 
@@ -336,10 +328,7 @@ void
 reg_buffer::assert_regnum (int regnum) const
 {
   gdb_assert (regnum >= 0);
-  if (m_has_pseudo)
-    gdb_assert (regnum < m_descr->nr_cooked_registers);
-  else
-    gdb_assert (regnum < gdbarch_num_regs (arch ()));
+  gdb_assert (regnum < m_descr->nr_cooked_registers);
 }
 
 /* Type to map a ptid to a list of regcaches (one thread may have multiple
@@ -723,7 +712,7 @@ readable_regcache::cooked_read (int regnum, gdb::array_view<gdb_byte> dst)
 
   gdb_assert (dst.size () == m_descr->sizeof_register[regnum]);
 
-  if (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN)
+  if (m_register_status[regnum] != REG_UNKNOWN)
     {
       if (m_register_status[regnum] == REG_VALID)
 	copy (register_buffer (regnum), dst);
@@ -774,7 +763,7 @@ readable_regcache::cooked_read_value (int regnum)
   gdb_assert (regnum < m_descr->nr_cooked_registers);
 
   if (regnum < num_raw_registers ()
-      || (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN)
+      || (m_register_status[regnum] != REG_UNKNOWN)
       || !gdbarch_pseudo_register_read_value_p (m_descr->gdbarch))
     {
       value *result = value::allocate_register
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 2f4b7d94c69..3ecf07020be 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -188,7 +188,7 @@ struct cached_reg_t
 class reg_buffer : public reg_buffer_common
 {
 public:
-  reg_buffer (gdbarch *gdbarch, bool has_pseudo);
+  reg_buffer (gdbarch *gdbarch, bool readonly);
 
   DISABLE_COPY_AND_ASSIGN (reg_buffer);
 
@@ -275,7 +275,7 @@ class reg_buffer : public reg_buffer_common
 
   struct regcache_descr *m_descr;
 
-  bool m_has_pseudo;
+  bool m_readonly;
   /* The register buffers.  */
   std::unique_ptr<gdb_byte[]> m_registers;
   /* Register cache status.  */
@@ -290,8 +290,8 @@ class reg_buffer : public reg_buffer_common
 class readable_regcache : public reg_buffer
 {
 public:
-  readable_regcache (gdbarch *gdbarch, bool has_pseudo)
-    : reg_buffer (gdbarch, has_pseudo)
+  readable_regcache (gdbarch *gdbarch, bool readonly)
+    : reg_buffer (gdbarch, readonly)
   {}
 
   /* Transfer a raw register [0..NUM_REGS) from core-gdb to this regcache,
@@ -349,8 +349,8 @@ class readable_regcache : public reg_buffer
 class detached_regcache : public readable_regcache
 {
 public:
-  detached_regcache (gdbarch *gdbarch, bool has_pseudo)
-    : readable_regcache (gdbarch, has_pseudo)
+  detached_regcache (gdbarch *gdbarch, bool readonly)
+    : readable_regcache (gdbarch, readonly)
   {}
 
   void raw_update (int regnum) override
@@ -537,8 +537,8 @@ class register_dump
   virtual ~register_dump () = default;
 
 protected:
-  register_dump (gdbarch *arch)
-    : m_gdbarch (arch)
+  register_dump (gdbarch *arch, bool dump_pseudo)
+    : m_gdbarch (arch), m_dump_pseudo (dump_pseudo)
   {}
 
   /* Dump the register REGNUM contents.  If REGNUM is -1, print the
@@ -546,6 +546,9 @@ class register_dump
   virtual void dump_reg (ui_file *file, int regnum) = 0;
 
   gdbarch *m_gdbarch;
+
+  /* Dump pseudo registers or not.  */
+  const bool m_dump_pseudo;
 };
 
 #endif /* REGCACHE_H */
-- 
2.45.2

Re: [PATCH] gdb: reintroduce reg_buffer::m_readonly in place of m_has_pseudo

[Andrew Burgess]

Guinevere Larsen <blarsen@redhat.com> writes:

> A series of commits obfuscated the meaning of reg_buffer:m_has_pseudo,
> most notably for the current code being commit
> daf6667d1f94c7e74df4076daf021cd28a2, that changed the checks in
> regcache::save and regcache::restore from checking if the cache was
> readonly to checking if the cache had registers.
>
> There was no reason provided for that change, but following commits
> started to use a parameters called "readonly" to set m_has_pseudo, until
> a following commit only used the literal "true" and "false" values, but
> still mapping perfectly on whether the register cache will be read only,
> and not taking the gdbarch's pseudo registers or specific needs of a
> cache's location into account.
>
> The current usages in regcache::save and ::restore were just changed to
> use m_readonly, making the code more understandable.  Two other uses, in
> readable_regcache::cooked_read and ::cooked_read_value, have just been
> removed, and the final 2 uses, in reg_buffer's constructor and in
> reg_buffer::assert_regnum, were changed to always act as if the buffer
> has pseudo registers.

This last bit confuses me.  A read/write reg cache can't really hold the
pseudo registers I think.  If we wrote to a raw-register and then read a
pseudo that is made (in part) from the raw register value, we'd miss the
updated value, wouldn't we?   Or maybe I'm missing something...

I'm not sure moving to a situation where we carry around some unused
space, and just rely on GDB not filling it in is not the best situation.

Can you explain a little more what the newly allocated space is expected
to be used for, and why it's safe?

Thanks,
Andrew


>
> I chose to make reg_buffer always allocate space for pseudo-registers
> because the cooker_read methods act as if it was possible to cache a
> pseudo register's value, but as far as I could see, there wasn't any
> code doing it. Since it would be done in some tdep file and I didn't
> feel confident in saying no code at all does this caching, and thought
> it would be safer to allocate space for it.
>
> Tested on x86_64 Fedora 40
> ---
>  gdb/regcache-dump.c | 18 +++++++-----------
>  gdb/regcache.c      | 35 ++++++++++++-----------------------
>  gdb/regcache.h      | 19 +++++++++++--------
>  3 files changed, 30 insertions(+), 42 deletions(-)
>
> diff --git a/gdb/regcache-dump.c b/gdb/regcache-dump.c
> index bc665dc08a6..6a6633e49dd 100644
> --- a/gdb/regcache-dump.c
> +++ b/gdb/regcache-dump.c
> @@ -32,8 +32,7 @@ class register_dump_regcache : public register_dump
>  {
>  public:
>    register_dump_regcache (regcache *regcache, bool dump_pseudo)
> -    : register_dump (regcache->arch ()), m_regcache (regcache),
> -      m_dump_pseudo (dump_pseudo)
> +    : register_dump (regcache->arch (), dump_pseudo), m_regcache (regcache)
>    {
>    }
>  
> @@ -80,9 +79,6 @@ class register_dump_regcache : public register_dump
>  
>  private:
>    regcache *m_regcache;
> -
> -  /* Dump pseudo registers or not.  */
> -  const bool m_dump_pseudo;
>  };
>  
>  /* Dump from reg_buffer, used when there is no thread or
> @@ -92,7 +88,7 @@ class register_dump_reg_buffer : public register_dump, reg_buffer
>  {
>  public:
>    register_dump_reg_buffer (gdbarch *gdbarch, bool dump_pseudo)
> -    : register_dump (gdbarch), reg_buffer (gdbarch, dump_pseudo)
> +    : register_dump (gdbarch, dump_pseudo), reg_buffer (gdbarch, true)
>    {
>    }
>  
> @@ -101,14 +97,14 @@ class register_dump_reg_buffer : public register_dump, reg_buffer
>    {
>      if (regnum < 0)
>        {
> -	if (m_has_pseudo)
> +	if (m_dump_pseudo)
>  	  gdb_printf (file, "Cooked value");
>  	else
>  	  gdb_printf (file, "Raw value");
>        }
>      else
>        {
> -	if (regnum < gdbarch_num_regs (m_gdbarch) || m_has_pseudo)
> +	if (regnum < gdbarch_num_regs (m_gdbarch) || m_dump_pseudo)
>  	  {
>  	    auto size = register_size (m_gdbarch, regnum);
>  
> @@ -140,7 +136,7 @@ class register_dump_none : public register_dump
>  {
>  public:
>    register_dump_none (gdbarch *arch)
> -    : register_dump (arch)
> +    : register_dump (arch, false)
>    {}
>  
>  protected:
> @@ -154,7 +150,7 @@ class register_dump_remote : public register_dump
>  {
>  public:
>    register_dump_remote (gdbarch *arch)
> -    : register_dump (arch)
> +    : register_dump (arch, false)
>    {}
>  
>  protected:
> @@ -181,7 +177,7 @@ class register_dump_groups : public register_dump
>  {
>  public:
>    register_dump_groups (gdbarch *arch)
> -    : register_dump (arch)
> +    : register_dump (arch, true)
>    {}
>  
>  protected:
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index f04354d822f..135238e5be3 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -185,8 +185,8 @@ regcache_register_size (const reg_buffer_common *regcache, int n)
>      (gdb::checked_static_cast<const struct regcache *> (regcache)->arch (), n);
>  }
>  
> -reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
> -  : m_has_pseudo (has_pseudo)
> +reg_buffer::reg_buffer (gdbarch *gdbarch, bool readonly)
> +  : m_readonly (readonly)
>  {
>    gdb_assert (gdbarch != NULL);
>    m_descr = regcache_descr (gdbarch);
> @@ -194,18 +194,9 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
>    /* We don't zero-initialize the M_REGISTERS array, as the bytes it contains
>       aren't meaningful as long as the corresponding register status is not
>       REG_VALID.  */
> -  if (has_pseudo)
> -    {
> -      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
> -      m_register_status.reset
> -	(new register_status[m_descr->nr_cooked_registers] ());
> -    }
> -  else
> -    {
> -      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers]);
> -      m_register_status.reset
> -	(new register_status[gdbarch_num_regs (gdbarch)] ());
> -    }
> +  m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
> +  m_register_status.reset
> +    (new register_status[m_descr->nr_cooked_registers] ());
>  }
>  
>  regcache::regcache (inferior *inf_for_target_calls, gdbarch *gdbarch)
> @@ -264,8 +255,9 @@ reg_buffer::save (register_read_ftype cooked_read)
>  {
>    struct gdbarch *gdbarch = m_descr->gdbarch;
>  
> -  /* It should have pseudo registers.  */
> -  gdb_assert (m_has_pseudo);
> +  /* This method should only be called when we're creating a
> +     readonly cache.  */
> +  gdb_assert (m_readonly);
>    /* Clear the dest.  */
>    memset (m_registers.get (), 0, m_descr->sizeof_cooked_registers);
>    memset (m_register_status.get (), REG_UNKNOWN, m_descr->nr_cooked_registers);
> @@ -297,7 +289,7 @@ regcache::restore (readonly_detached_regcache *src)
>    int regnum;
>  
>    gdb_assert (src != NULL);
> -  gdb_assert (src->m_has_pseudo);
> +  gdb_assert (src->m_readonly);
>  
>    gdb_assert (gdbarch == src->arch ());
>  
> @@ -336,10 +328,7 @@ void
>  reg_buffer::assert_regnum (int regnum) const
>  {
>    gdb_assert (regnum >= 0);
> -  if (m_has_pseudo)
> -    gdb_assert (regnum < m_descr->nr_cooked_registers);
> -  else
> -    gdb_assert (regnum < gdbarch_num_regs (arch ()));
> +  gdb_assert (regnum < m_descr->nr_cooked_registers);
>  }
>  
>  /* Type to map a ptid to a list of regcaches (one thread may have multiple
> @@ -723,7 +712,7 @@ readable_regcache::cooked_read (int regnum, gdb::array_view<gdb_byte> dst)
>  
>    gdb_assert (dst.size () == m_descr->sizeof_register[regnum]);
>  
> -  if (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN)
> +  if (m_register_status[regnum] != REG_UNKNOWN)
>      {
>        if (m_register_status[regnum] == REG_VALID)
>  	copy (register_buffer (regnum), dst);
> @@ -774,7 +763,7 @@ readable_regcache::cooked_read_value (int regnum)
>    gdb_assert (regnum < m_descr->nr_cooked_registers);
>  
>    if (regnum < num_raw_registers ()
> -      || (m_has_pseudo && m_register_status[regnum] != REG_UNKNOWN)
> +      || (m_register_status[regnum] != REG_UNKNOWN)
>        || !gdbarch_pseudo_register_read_value_p (m_descr->gdbarch))
>      {
>        value *result = value::allocate_register
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 2f4b7d94c69..3ecf07020be 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -188,7 +188,7 @@ struct cached_reg_t
>  class reg_buffer : public reg_buffer_common
>  {
>  public:
> -  reg_buffer (gdbarch *gdbarch, bool has_pseudo);
> +  reg_buffer (gdbarch *gdbarch, bool readonly);
>  
>    DISABLE_COPY_AND_ASSIGN (reg_buffer);
>  
> @@ -275,7 +275,7 @@ class reg_buffer : public reg_buffer_common
>  
>    struct regcache_descr *m_descr;
>  
> -  bool m_has_pseudo;
> +  bool m_readonly;
>    /* The register buffers.  */
>    std::unique_ptr<gdb_byte[]> m_registers;
>    /* Register cache status.  */
> @@ -290,8 +290,8 @@ class reg_buffer : public reg_buffer_common
>  class readable_regcache : public reg_buffer
>  {
>  public:
> -  readable_regcache (gdbarch *gdbarch, bool has_pseudo)
> -    : reg_buffer (gdbarch, has_pseudo)
> +  readable_regcache (gdbarch *gdbarch, bool readonly)
> +    : reg_buffer (gdbarch, readonly)
>    {}
>  
>    /* Transfer a raw register [0..NUM_REGS) from core-gdb to this regcache,
> @@ -349,8 +349,8 @@ class readable_regcache : public reg_buffer
>  class detached_regcache : public readable_regcache
>  {
>  public:
> -  detached_regcache (gdbarch *gdbarch, bool has_pseudo)
> -    : readable_regcache (gdbarch, has_pseudo)
> +  detached_regcache (gdbarch *gdbarch, bool readonly)
> +    : readable_regcache (gdbarch, readonly)
>    {}
>  
>    void raw_update (int regnum) override
> @@ -537,8 +537,8 @@ class register_dump
>    virtual ~register_dump () = default;
>  
>  protected:
> -  register_dump (gdbarch *arch)
> -    : m_gdbarch (arch)
> +  register_dump (gdbarch *arch, bool dump_pseudo)
> +    : m_gdbarch (arch), m_dump_pseudo (dump_pseudo)
>    {}
>  
>    /* Dump the register REGNUM contents.  If REGNUM is -1, print the
> @@ -546,6 +546,9 @@ class register_dump
>    virtual void dump_reg (ui_file *file, int regnum) = 0;
>  
>    gdbarch *m_gdbarch;
> +
> +  /* Dump pseudo registers or not.  */
> +  const bool m_dump_pseudo;
>  };
>  
>  #endif /* REGCACHE_H */
> -- 
> 2.45.2