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

["Aktemur, Tankut Baris" <tankut dot baris dot aktemur at intel dot com>]

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