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

  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(-)
  

>>>>> 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
  

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
  

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
  

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