[PATCH v3 08/10] gdbserver: define and use regcache::reset

[Aktemur, Tankut Baris]

On Tuesday, January 28, 2025 7:01 PM, Simon Marchi wrote:
> On 1/28/25 3:16 AM, Tankut Baris Aktemur wrote:
> > Define a `reset` method for a regcache and use it for code
> > simplification.  This patch allows further simplification in the next
> > patch.
> >
> > The reset method fills the register data with zeroes.  For the use in
> > get_thread_regcache, this is added behavior, making the patch not a
> > pure refactoring, and may look like extra overhead.  However, it is
> > better to avoid having arbitrary values left in the data buffer.
> > Hence, it is considered a behavioral improvement.
> > ---
> >  gdbserver/regcache.cc | 30 ++++++++++++++++--------------
> >  gdbserver/regcache.h  |  4 ++++
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> > index
> 11ce2b730d025d47981dce815856ea8e908a8dc6..a09e981773f76d6fc50c7a52e6dc1763ce945ff5 100644
> > --- a/gdbserver/regcache.cc
> > +++ b/gdbserver/regcache.cc
> > @@ -52,8 +52,7 @@ get_thread_regcache (thread_info *thread, bool fetch)
> >
> >        switch_to_thread (thread);
> >        /* Invalidate all registers, to prevent stale left-overs.  */
> > -      memset (regcache->register_status, REG_UNKNOWN,
> > -	      regcache->tdesc->reg_defs.size ());
> > +      regcache->reset (REG_UNKNOWN);
> >        fetch_inferior_registers (regcache, -1);
> >        regcache->registers_fetched = true;
> >      }
> > @@ -130,11 +129,10 @@ regcache::regcache (const target_desc *tdesc)
> >       fetches.  This way they'll read as zero instead of
> >       garbage.  */
> >    this->registers
> > -    = (unsigned char *) xcalloc (1, tdesc->registers_size);
> > +    = (unsigned char *) xmalloc (tdesc->registers_size);
> >    this->register_status
> >      = (unsigned char *) xmalloc (tdesc->reg_defs.size ());
> > -  memset ((void *) this->register_status, REG_UNKNOWN,
> > -	  tdesc->reg_defs.size ());
> > +  reset (REG_UNKNOWN);
> >  }
> >
> >  regcache::~regcache ()
> > @@ -146,6 +144,18 @@ regcache::~regcache ()
> >
> >  #endif
> >
> > +void
> > +regcache::reset (enum register_status status)
> > +{
> > +  /* Zero-initialize the register cache, in case there are registers
> > +     the target never fetches.  This way they'll read as zero instead
> > +     of garbage.  */
> > +  memset (this->registers, 0, this->tdesc->registers_size);
> > +#ifndef IN_PROCESS_AGENT
> > +  memset (this->register_status, status, this->tdesc->reg_defs.size ());
> > +#endif
> 
> I think that we need to have a null check for
> register_status:
> 
>   if (this->register_status != nullptr)
>     memset (this->register_status, status, this->tdesc->reg_defs.size ());
> 
> When this constructor gets called:
> 
>   regcache (const target_desc *tdesc, unsigned char *regbuf);
> 
> by gdbserver (not the IPA), then register_status stays nullptr.  We
> may not hit the problem right now, but if one were to construct a
> regcache using that constructor and call reset, I think it would crash.

In the last patch, we replace the memset call with

    for (int i = 0; i < this->tdesc->reg_defs.size (); i++)
      set_register_status (i, status);

and in set_register_status we have

   if (m_register_status != nullptr)
     m_register_status[regnum] = status;

I know, these hidden forward changes are difficult for review.  I'll
nevertheless update this patch with your suggestion, even if it's
removed soon, so that the patch is improved in isolation.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928