[57/58] gdbserver: use unique_ptr for 'the_target'

The target op vector is about to be replaced with a process_target
object.  In the current state, the 'set_target_ops' function takes a
target op vector and creates a clone of it via XNEW and memcpy.  As we
are now switching to using classes and polymorphism, this memcpy'ing
will no longer be possible.  For proper memory managament, switch to
using a unique_ptr.

gdbserver/ChangeLog:
2020-02-10  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* target.h (set_target_ops): Remove.
	(<the_target>): Change the type to
	std::unique_ptr<process_stratum_target>.
	* target.c (set_target_ops): Remove.
	(done_accessing_memory): Update the usage of the_target.
	* fork-child.c (post_fork_inferior): Update the usage of
	the_target.
	* remote-utils.c (prepare_resume_reply): Ditto.
	* linux-low.c (initialize_low): Set the value of the_target.
	(struct linux_target_ops): Delete.
	* lynx-low.c (initialize_low): Set the value of the_target.
	(struct lynx_target_ops): Delete.
	* nto-low.c (initialize_low): Set the value of the_target.
	(struct nto_target_ops): Delete.
	* win32-low.c (initialize_low): Set the value of the_target.
	(struct win32_target_ops): Delete.
---
 gdbserver/fork-child.c   |  2 +-
 gdbserver/linux-low.c    |  7 ++-----
 gdbserver/lynx-low.c     |  9 ++-------
 gdbserver/nto-low.c      |  8 ++------
 gdbserver/remote-utils.c |  2 +-
 gdbserver/target.c       | 11 ++---------
 gdbserver/target.h       |  4 +---
 gdbserver/win32-low.c    |  7 ++-----
 8 files changed, 13 insertions(+), 37 deletions(-)

On Tue, Feb 11, 2020 at 3:08 AM Tankut Baris Aktemur
<tankut.baris.aktemur@intel.com> wrote:
>
> The target op vector is about to be replaced with a process_target
> object.  In the current state, the 'set_target_ops' function takes a
> target op vector and creates a clone of it via XNEW and memcpy.  As we
> are now switching to using classes and polymorphism, this memcpy'ing
> will no longer be possible.  For proper memory managament, switch to
> using a unique_ptr.
>
> gdbserver/ChangeLog:
> 2020-02-10  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
>
>         * target.h (set_target_ops): Remove.
>         (<the_target>): Change the type to
>         std::unique_ptr<process_stratum_target>.
>         * target.c (set_target_ops): Remove.
>         (done_accessing_memory): Update the usage of the_target.
>         * fork-child.c (post_fork_inferior): Update the usage of
>         the_target.
>         * remote-utils.c (prepare_resume_reply): Ditto.
>         * linux-low.c (initialize_low): Set the value of the_target.
>         (struct linux_target_ops): Delete.
>         * lynx-low.c (initialize_low): Set the value of the_target.
>         (struct lynx_target_ops): Delete.
>         * nto-low.c (initialize_low): Set the value of the_target.
>         (struct nto_target_ops): Delete.
>         * win32-low.c (initialize_low): Set the value of the_target.
>         (struct win32_target_ops): Delete.
> ---
>  gdbserver/fork-child.c   |  2 +-
>  gdbserver/linux-low.c    |  7 ++-----
>  gdbserver/lynx-low.c     |  9 ++-------
>  gdbserver/nto-low.c      |  8 ++------
>  gdbserver/remote-utils.c |  2 +-
>  gdbserver/target.c       | 11 ++---------
>  gdbserver/target.h       |  4 +---
>  gdbserver/win32-low.c    |  7 ++-----
>  8 files changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/gdbserver/fork-child.c b/gdbserver/fork-child.c
> index 7a71ec0b1ca..3702c1a8c2e 100644
> --- a/gdbserver/fork-child.c
> +++ b/gdbserver/fork-child.c
> @@ -107,7 +107,7 @@ post_fork_inferior (int pid, const char *program)
>    atexit (restore_old_foreground_pgrp);
>  #endif
>
> -  startup_inferior (the_target, pid,
> +  startup_inferior (the_target.get (), pid,
>                     START_INFERIOR_TRAPS_EXPECTED,
>                     &cs.last_status, &cs.last_ptid);
>    current_thread->last_resume_kind = resume_stop;
> diff --git a/gdbserver/linux-low.c b/gdbserver/linux-low.c
> index cd84cef326a..87cf2fd8afb 100644
> --- a/gdbserver/linux-low.c
> +++ b/gdbserver/linux-low.c
> @@ -7512,10 +7512,6 @@ linux_get_hwcap2 (int wordsize)
>
>  static linux_process_target the_linux_target;
>
> -static process_stratum_target linux_target_ops = {
> -  &the_linux_target,
> -};
> -
>  #ifdef HAVE_LINUX_REGSETS
>  void
>  initialize_regsets_info (struct regsets_info *info)
> @@ -7533,7 +7529,8 @@ initialize_low (void)
>    struct sigaction sigchld_action;
>
>    memset (&sigchld_action, 0, sizeof (sigchld_action));
> -  set_target_ops (&linux_target_ops);
> +  the_target.reset (new process_stratum_target);
> +  the_target->pt = &the_linux_target;

Should this now be a constructor argument?

>
>    linux_ptrace_init_warnings ();
>    linux_proc_init_warnings ();
> diff --git a/gdbserver/lynx-low.c b/gdbserver/lynx-low.c
> index 778afd190b6..1377ccaf4ae 100644
> --- a/gdbserver/lynx-low.c
> +++ b/gdbserver/lynx-low.c
> @@ -739,16 +739,11 @@ lynx_process_target::sw_breakpoint_from_kind (int kind, int *size)
>
>  static lynx_process_target the_lynx_target;
>
> -/* The LynxOS target_ops vector.  */
> -
> -static process_stratum_target lynx_target_ops = {
> -  &the_lynx_target,
> -};
> -
>  void
>  initialize_low (void)
>  {
> -  set_target_ops (&lynx_target_ops);
> +  the_target.reset (new process_stratum_target);
> +  the_target->pt = &the_lynx_target;
>    the_low_target.arch_setup ();
>  }
>
> diff --git a/gdbserver/nto-low.c b/gdbserver/nto-low.c
> index cd461be8378..bbcc5046659 100644
> --- a/gdbserver/nto-low.c
> +++ b/gdbserver/nto-low.c
> @@ -946,11 +946,6 @@ nto_process_target::sw_breakpoint_from_kind (int kind, int *size)
>
>  static nto_process_target the_nto_target;
>
> -static process_stratum_target nto_target_ops = {
> -  &the_nto_target,
> -};
> -
> -
>  /* Global function called by server.c.  Initializes QNX Neutrino
>     gdbserver.  */
>
> @@ -960,7 +955,8 @@ initialize_low (void)
>    sigset_t set;
>
>    TRACE ("%s\n", __func__);
> -  set_target_ops (&nto_target_ops);
> +  the_target.reset (new process_stratum_target);
> +  the_target->pt = &the_nto_target;
>
>    /* We use SIGUSR1 to gain control after we block waiting for a process.
>       We use sigwaitevent to wait.  */
> diff --git a/gdbserver/remote-utils.c b/gdbserver/remote-utils.c
> index 316f04e32ee..14e89332db4 100644
> --- a/gdbserver/remote-utils.c
> +++ b/gdbserver/remote-utils.c
> @@ -1208,7 +1208,7 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>
>         saved_thread = current_thread;
>
> -       switch_to_thread (the_target, ptid);
> +       switch_to_thread (the_target.get (), ptid);
>
>         regp = current_target_desc ()->expedite_regs;
>
> diff --git a/gdbserver/target.c b/gdbserver/target.c
> index 5c80a379f65..048fd5ad6e9 100644
> --- a/gdbserver/target.c
> +++ b/gdbserver/target.c
> @@ -27,7 +27,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>
> -process_stratum_target *the_target;
> +std::unique_ptr<process_stratum_target> the_target;
>
>  int
>  set_desired_thread ()
> @@ -118,7 +118,7 @@ done_accessing_memory (void)
>
>    /* Restore the previous selected thread.  */
>    cs.general_thread = prev_general_thread;
> -  switch_to_thread (the_target, cs.general_thread);
> +  switch_to_thread (the_target.get (), cs.general_thread);
>  }
>
>  int
> @@ -267,13 +267,6 @@ target_supports_multi_process (void)
>    return the_target->pt->supports_multi_process ();
>  }
>
> -void
> -set_target_ops (process_stratum_target *target)
> -{
> -  the_target = XNEW (process_stratum_target);
> -  memcpy (the_target, target, sizeof (*the_target));
> -}
> -
>  /* Convert pid to printable format.  */
>
>  const char *
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index 1b5059c1beb..ac0ada0cd38 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -506,9 +506,7 @@ public:
>    virtual int get_ipa_tdesc_idx ();
>  };
>
> -extern process_stratum_target *the_target;
> -
> -void set_target_ops (process_stratum_target *);
> +extern std::unique_ptr<process_stratum_target> the_target;
>
>  #define target_create_inferior(program, program_args)  \
>    the_target->pt->create_inferior (program, program_args)
> diff --git a/gdbserver/win32-low.c b/gdbserver/win32-low.c
> index 567e2ebe3db..5b4dd36f5d0 100644
> --- a/gdbserver/win32-low.c
> +++ b/gdbserver/win32-low.c
> @@ -1847,14 +1847,11 @@ win32_process_target::sw_breakpoint_from_kind (int kind, int *size)
>
>  static win32_process_target the_win32_target;
>
> -static process_stratum_target win32_target_ops = {
> -  &the_win32_target,
> -};
> -
>  /* Initialize the Win32 backend.  */
>  void
>  initialize_low (void)
>  {
> -  set_target_ops (&win32_target_ops);
> +  the_target.reset (new process_stratum_target);
> +  the_target->pt = &the_win32_target;
>    the_low_target.arch_setup ();
>  }
> --
> 2.17.1
>

On Tuesday, February 11, 2020 5:43 PM, Christian Biesinger wrote:
> On Tue, Feb 11, 2020 at 3:08 AM Tankut Baris Aktemur

> <tankut.baris.aktemur@intel.com> wrote:

> >

> > The target op vector is about to be replaced with a process_target

> > object.  In the current state, the 'set_target_ops' function takes a

> > target op vector and creates a clone of it via XNEW and memcpy.  As we

> > are now switching to using classes and polymorphism, this memcpy'ing

> > will no longer be possible.  For proper memory managament, switch to

> > using a unique_ptr.

> >

> > gdbserver/ChangeLog:

> > 2020-02-10  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> >

> >         * target.h (set_target_ops): Remove.

> >         (<the_target>): Change the type to

> >         std::unique_ptr<process_stratum_target>.

> >         * target.c (set_target_ops): Remove.

> >         (done_accessing_memory): Update the usage of the_target.

> >         * fork-child.c (post_fork_inferior): Update the usage of

> >         the_target.

> >         * remote-utils.c (prepare_resume_reply): Ditto.

> >         * linux-low.c (initialize_low): Set the value of the_target.

> >         (struct linux_target_ops): Delete.

> >         * lynx-low.c (initialize_low): Set the value of the_target.

> >         (struct lynx_target_ops): Delete.

> >         * nto-low.c (initialize_low): Set the value of the_target.

> >         (struct nto_target_ops): Delete.

> >         * win32-low.c (initialize_low): Set the value of the_target.

> >         (struct win32_target_ops): Delete.

> > ---

> >  gdbserver/fork-child.c   |  2 +-

> >  gdbserver/linux-low.c    |  7 ++-----

> >  gdbserver/lynx-low.c     |  9 ++-------

> >  gdbserver/nto-low.c      |  8 ++------

> >  gdbserver/remote-utils.c |  2 +-

> >  gdbserver/target.c       | 11 ++---------

> >  gdbserver/target.h       |  4 +---

> >  gdbserver/win32-low.c    |  7 ++-----

> >  8 files changed, 13 insertions(+), 37 deletions(-)

> >

> > diff --git a/gdbserver/fork-child.c b/gdbserver/fork-child.c

> > index 7a71ec0b1ca..3702c1a8c2e 100644

> > --- a/gdbserver/fork-child.c

> > +++ b/gdbserver/fork-child.c

> > @@ -107,7 +107,7 @@ post_fork_inferior (int pid, const char *program)

> >    atexit (restore_old_foreground_pgrp);

> >  #endif

> >

> > -  startup_inferior (the_target, pid,

> > +  startup_inferior (the_target.get (), pid,

> >                     START_INFERIOR_TRAPS_EXPECTED,

> >                     &cs.last_status, &cs.last_ptid);

> >    current_thread->last_resume_kind = resume_stop;

> > diff --git a/gdbserver/linux-low.c b/gdbserver/linux-low.c

> > index cd84cef326a..87cf2fd8afb 100644

> > --- a/gdbserver/linux-low.c

> > +++ b/gdbserver/linux-low.c

> > @@ -7512,10 +7512,6 @@ linux_get_hwcap2 (int wordsize)

> >

> >  static linux_process_target the_linux_target;

> >

> > -static process_stratum_target linux_target_ops = {

> > -  &the_linux_target,

> > -};

> > -

> >  #ifdef HAVE_LINUX_REGSETS

> >  void

> >  initialize_regsets_info (struct regsets_info *info)

> > @@ -7533,7 +7529,8 @@ initialize_low (void)

> >    struct sigaction sigchld_action;

> >

> >    memset (&sigchld_action, 0, sizeof (sigchld_action));

> > -  set_target_ops (&linux_target_ops);

> > +  the_target.reset (new process_stratum_target);

> > +  the_target->pt = &the_linux_target;

> 

> Should this now be a constructor argument?


Yes, this would make it better; however, to be frank, I did not care too
much because the next patch in the series (PATCH 58/58) deletes the
process_target class and the 'pt' field, making the following change:

  -  the_target.reset (new process_stratum_target);
  -  the_target->pt = &the_linux_target;
  +  the_target.reset (new linux_process_target);

So the code above is temporary -- if I introduce a constructor here, it
would have to be deleted/modified in the next patch.  Given this, is it OK
to leave this patch as it is?

An alternative is to squash PATCH 57 and 58, but 58 is already the longest
patch in the series.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

On 2/11/20 9:02 AM, Tankut Baris Aktemur wrote:
> The target op vector is about to be replaced with a process_target
> object.  In the current state, the 'set_target_ops' function takes a
> target op vector and creates a clone of it via XNEW and memcpy.  As we
> are now switching to using classes and polymorphism, this memcpy'ing
> will no longer be possible.  For proper memory managament, switch to
> using a unique_ptr.

Typo: "management"

I find it unfortunate to expose the unique_ptr-ness throughtout, since
the target object is really a singleton.  I don't immediately see why
do we need to heap-allocate and copy the target object?
On the gdb side, we define the target as a global instance and then
register a pointer as the native target.  How about we do that in
gdbserver too?

I.e., just make set_target_ops do (with whatever intermediate steps
to get there):

void
set_target_ops (process_stratum_target *target)
{
  the_target = target;
}

Thanks,
Pedro Alves