[PATCH 0/3] Torward multiple simultaneous core instances

[PATCH 0/3] Torward multiple simultaneous core instances

[Pedro Alves]

Here's a piece from my multi-target branch that I found was
easily-splittable enough.  This is preparation for being able to debug
multiple cores at once.  E.g., inferior 1 debugging one core, inferior
2 debugging a different core, etc.

There's one patch here that I stole from Tromey's old multi-target
branch early on when I started working on this.  Thanks Tom.  :-)

Pedro Alves (2):
  Eliminate the 'the_core_target' global
  Heap-allocate core_target instances

Tom Tromey (1):
  Move core_bfd to program space

 gdb/corefile.c  |  22 ----
 gdb/corelow.c   | 307 ++++++++++++++++++++++++++++++++------------------------
 gdb/gdbcore.h   |   6 +-
 gdb/progspace.h |   3 +
 4 files changed, 178 insertions(+), 160 deletions(-)

-- 
2.14.3

[PATCH 1/3] Move core_bfd to program space

[Pedro Alves]

From: Tom Tromey <tromey@redhat.com>

This moves the core_bfd global to be a field of the program space.  It
then replaces core_bfd with a macro to avoid a massive patch -- the
same approach taken for various other program space fields.

This is a basic transformation for multi-target work.

yyyy-mm-dd  Tom Tromey  <tromey@redhat.com>
	    Pedro Alves  <tromey@redhat.com>

	* corefile.c (core_bfd): Remove.
	* gdbcore.h (core_bfd): Now a macro.
	* progspace.h (struct program_space) <cbfd>: New field.
---
 gdb/corefile.c  | 4 ----
 gdb/gdbcore.h   | 2 +-
 gdb/progspace.h | 3 +++
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index 114de83640..e0c7540140 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -49,10 +49,6 @@ static hook_type *exec_file_extra_hooks;	/* Array of additional
 						   hooks.  */
 static int exec_file_hook_count = 0;		/* Size of array.  */
 
-/* Binary file diddling handle for the core file.  */
-
-bfd *core_bfd = NULL;
-
 \f
 
 /* Backward compatability with old way of specifying core files.  */
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 401c213d48..d06ccc3507 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -133,7 +133,7 @@ extern void specify_exec_file_hook (void (*hook) (const char *filename));
 
 /* Binary File Diddler for the core file.  */
 
-extern bfd *core_bfd;
+#define core_bfd (current_program_space->cbfd)
 
 /* corelow.c target.  It is never NULL after GDB initialization.  */
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 67c0a240da..835fcfdd5d 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -157,6 +157,9 @@ struct program_space
      It needs to be freed by xfree.  It is not NULL iff EBFD is not NULL.  */
   char *pspace_exec_filename = NULL;
 
+  /* Binary file diddling handle for the core file.  */
+  bfd *cbfd = NULL;
+
   /* The address space attached to this program space.  More than one
      program space may be bound to the same address space.  In the
      traditional unix-like debugging scenario, this will usually
-- 
2.14.3

Re: [PATCH 1/3] Move core_bfd to program space

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This moves the core_bfd global to be a field of the program space.  It
Pedro> then replaces core_bfd with a macro to avoid a massive patch -- the
Pedro> same approach taken for various other program space fields.

I am curious to know whether you would want to remove this macro in the
future.  I don't mean that you should do it -- just more a question of
what direction to go.  There are other macros like this too:
symfile_objfile, object_files, exec_bfd, ...

Also, I can't remember why I moved core_bfd to the progspace.  Would it
be better to have it just be a member of the target?  Or maybe in your
design these end up being basically equivalent, because core targets are
inherently single-process?

Tom

Re: [PATCH 1/3] Move core_bfd to program space

[Pedro Alves]

On 05/04/2018 04:41 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This moves the core_bfd global to be a field of the program space.  It
> Pedro> then replaces core_bfd with a macro to avoid a massive patch -- the
> Pedro> same approach taken for various other program space fields.
> 
> I am curious to know whether you would want to remove this macro in the
> future.  I don't mean that you should do it -- just more a question of
> what direction to go.  There are other macros like this too:
> symfile_objfile, object_files, exec_bfd, ...

Yeah, I have no plans to do that myself, but I wouldn't oppose
changing it.

> Also, I can't remember why I moved core_bfd to the progspace.  Would it
> be better to have it just be a member of the target?  Or maybe in your
> design these end up being basically equivalent, because core targets are
> inherently single-process?
Yeah, I guess program space just felt natural given exec_bfd is there
too.  Not sure about putting it in the target.  Making it a data field of
target_ops I think would be odd.  It might work if we replaced it
with something like (in the multi-target branch):

bfd *
core_bfd ()
{
  if (core_target *core 
      = dynamic_cast<core_target *> 
          (current_inferior ()->process_target ()))
    return core->core_bfd;
  return nullptr;
}

though that's a bit smelly, and when I see dynamic_cast
I can't avoid thinking about how inefficient it is.  :-)

Alternatively, we could make core_bfd() a virtual method of
target_ops instead, that has most targets except the
core_target target return NULL.

Not sure.  Putting it in program space just seemed like an
easy and OK thing to do.

Thanks,
Pedro Alves

Re: [PATCH 1/3] Move core_bfd to program space

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> I am curious to know whether you would want to remove this macro in the
>> future.  I don't mean that you should do it -- just more a question of
>> what direction to go.  There are other macros like this too:
>> symfile_objfile, object_files, exec_bfd, ...

Pedro> Yeah, I have no plans to do that myself, but I wouldn't oppose
Pedro> changing it.

Thanks.

Pedro> Yeah, I guess program space just felt natural given exec_bfd is there
Pedro> too.

That makes sense, thanks.

Tom

[PATCH 3/3] Heap-allocate core_target instances

[Pedro Alves]

This gets rid of the core_ops global, and replaces it with
heap-allocated core_target instances.  In practice, there will only be
one such instance, though that will change further ahead as more
pieces of multi-target support are merged.

Notice that this replaces one heap-allocated object for another, the
number of allocations is the same.  Specifically, currently we
heap-allocate the 'core_data' object, which holds the core's section
table.  With this patch, that object is made a field of the
core_target class, and no longer allocated separately.

Note that this bit:

  -  /* Looks semi-reasonable.  Toss the old core file and work on the
  -     new.  */
  -
  -  unpush_target (&core_ops);

does not need a replacement, because by the time we get here, the
target_preopen call at the top of core_target_open has already
unpushed any previous target.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* corelow.c (core_target) <core_target>: No longer inline.
	Initialize m_core_gdbarch, m_core_vec and build the section table
	here.
	<~core_target>: New.
	<core_gdbarch, get_core_register_section>: New methods.
	<m_core_section_table, m_core_vec, m_core_gdbarch>: New fields,
	factored out from ...
	<core_data, core_vec, core_gdbarch>: ... these deleted globals.
	(core_ops): Delete.
	(sniff_core_bfd): Add gdbarch parameter.
	(core_close): Delete, merged into ...
	(core_target::close): ... here.  Delete self.
	(core_close_cleanup): Delete.
	(struct target_ops_closer): New.
	(core_target_open): Allocate a core_target on the heap.  Use a
	unique_ptr instead of a cleanup.  Bits moved into the core_target
	ctor.  Adjust to use core_target methods instead of globals.
	(get_core_register_section): Rename to ...
	(core_target::get_core_register_section): ... this and adjust.
	(struct get_core_registers_cb_data): New.
	(get_core_registers_cb): Use it.  Use bool.
	(core_target::fetch_registers, core_target::files_info)
	(core_target::xfer_partial, core_target::read_description)
	(core_target::pid_to, core_target::thread_name): Adjust to
	reference class fields instead of globals.
---
 gdb/corelow.c | 267 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 144 insertions(+), 123 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 3b71a1b856..37b1a87b9b 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -50,6 +50,9 @@
 #define O_LARGEFILE 0
 #endif
 
+static core_fns *sniff_core_bfd (gdbarch *core_gdbarch,
+				 bfd *abfd);
+
 /* The core file target.  */
 
 static const target_info core_target_info = {
@@ -61,8 +64,8 @@ static const target_info core_target_info = {
 class core_target final : public target_ops
 {
 public:
-  core_target ()
-  { to_stratum = process_stratum; }
+  core_target ();
+  ~core_target () override;
 
   const target_info &info () const override
   { return core_target_info; }
@@ -90,42 +93,74 @@ public:
   bool has_stack () override;
   bool has_registers () override;
   bool info_proc (const char *, enum info_proc_what) override;
+
+  /* A few helpers.  */
+
+  /* Getter, see variable definition.  */
+  struct gdbarch *core_gdbarch ()
+  {
+    return m_core_gdbarch;
+  }
+
+  /* See definition.  */
+  void get_core_register_section (struct regcache *regcache,
+				  const struct regset *regset,
+				  const char *name,
+				  int min_size,
+				  int which,
+				  const char *human_name,
+				  bool required);
+
+private: /* per-core data */
+  /* The core's section table.  Note that these target sections are
+     *not* mapped in the current address spaces' set of target
+     sections --- those should come only from pure executable or
+     shared library bfds.  The core bfd sections are an implementation
+     detail of the core target, just like ptrace is for unix child
+     targets.  */
+  target_section_table m_core_section_table {};
+
+  /* The core_fns for a core file handler that is prepared to read the
+     core file currently open on core_bfd.  */
+  core_fns *m_core_vec = NULL;
+
+  /* FIXME: kettenis/20031023: Eventually this variable should
+     disappear.  */
+  struct gdbarch *m_core_gdbarch = NULL;
 };
 
+core_target::core_target ()
+{
+  to_stratum = process_stratum;
+
+  m_core_gdbarch = gdbarch_from_bfd (core_bfd);
+
+  /* Find a suitable core file handler to munch on core_bfd */
+  m_core_vec = sniff_core_bfd (m_core_gdbarch, core_bfd);
+
+  /* Find the data section */
+  if (build_section_table (core_bfd,
+			   &m_core_section_table.sections,
+			   &m_core_section_table.sections_end))
+    error (_("\"%s\": Can't find sections: %s"),
+	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
+}
+
+core_target::~core_target ()
+{
+  xfree (m_core_section_table.sections);
+}
+
 /* List of all available core_fns.  On gdb startup, each core file
    register reader calls deprecated_add_core_fns() to register
    information on each core format it is prepared to read.  */
 
 static struct core_fns *core_file_fns = NULL;
 
-/* The core_fns for a core file handler that is prepared to read the
-   core file currently open on core_bfd.  */
-
-static struct core_fns *core_vec = NULL;
-
-/* FIXME: kettenis/20031023: Eventually this variable should
-   disappear.  */
-
-static struct gdbarch *core_gdbarch = NULL;
-
-/* Per-core data.  Currently, only the section table.  Note that these
-   target sections are *not* mapped in the current address spaces' set
-   of target sections --- those should come only from pure executable
-   or shared library bfds.  The core bfd sections are an
-   implementation detail of the core target, just like ptrace is for
-   unix child targets.  */
-static struct target_section_table *core_data;
-
-static struct core_fns *sniff_core_bfd (bfd *);
-
 static int gdb_check_format (bfd *);
 
-static void core_close_cleanup (void *ignore);
-
 static void add_to_thread_list (bfd *, asection *, void *);
 
-static core_target core_ops;
-
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
 
@@ -159,7 +194,7 @@ default_core_sniffer (struct core_fns *our_fns, bfd *abfd)
    selected.  */
 
 static struct core_fns *
-sniff_core_bfd (bfd *abfd)
+sniff_core_bfd (struct gdbarch *core_gdbarch, bfd *abfd)
 {
   struct core_fns *cf;
   struct core_fns *yummy = NULL;
@@ -217,11 +252,8 @@ gdb_check_format (bfd *abfd)
   return (0);
 }
 
-/* Discard all vestiges of any previous core file and mark data and
-   stack spaces as empty.  */
-
-static void
-core_close ()
+void
+core_target::close ()
 {
   if (core_bfd)
     {
@@ -235,30 +267,11 @@ core_close ()
          comments in clear_solib in solib.c.  */
       clear_solib ();
 
-      if (core_data)
-	{
-	  xfree (core_data->sections);
-	  xfree (core_data);
-	  core_data = NULL;
-	}
-
       gdb_bfd_unref (core_bfd);
       core_bfd = NULL;
     }
-  core_vec = NULL;
-  core_gdbarch = NULL;
-}
 
-static void
-core_close_cleanup (void *ignore)
-{
-  core_close ();
-}
-
-void
-core_target::close ()
-{
-  core_close ();
+  delete this;
 }
 
 /* Look for sections whose names start with `.reg/' so that we can
@@ -336,6 +349,16 @@ core_file_command (const char *filename, int from_tty)
     core_target_open (filename, from_tty);
 }
 
+/* Deleter for std::unique_ptr.  Closes the target if an exception is
+   thrown.  */
+struct target_ops_closer
+{
+  void operator() (target_ops *target)
+  {
+    target->close ();
+  }
+};
+
 /* See gdbcore.h.  */
 
 void
@@ -388,29 +411,14 @@ core_target_open (const char *arg, int from_tty)
 	     filename.get (), bfd_errmsg (bfd_get_error ()));
     }
 
-  /* Looks semi-reasonable.  Toss the old core file and work on the
-     new.  */
-
-  unpush_target (&core_ops);
   core_bfd = temp_bfd.release ();
-  old_chain = make_cleanup (core_close_cleanup, 0 /*ignore*/);
 
-  core_gdbarch = gdbarch_from_bfd (core_bfd);
+  core_target *target = new core_target ();
 
-  /* Find a suitable core file handler to munch on core_bfd */
-  core_vec = sniff_core_bfd (core_bfd);
+  std::unique_ptr<target_ops, target_ops_closer> target_holder (target);
 
   validate_files ();
 
-  core_data = XCNEW (struct target_section_table);
-
-  /* Find the data section */
-  if (build_section_table (core_bfd,
-			   &core_data->sections,
-			   &core_data->sections_end))
-    error (_("\"%s\": Can't find sections: %s"),
-	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
-
   /* If we have no exec file, try to set the architecture from the
      core file.  We don't do this unconditionally since an exec file
      typically contains more information that helps us determine the
@@ -418,8 +426,8 @@ core_target_open (const char *arg, int from_tty)
   if (!exec_bfd)
     set_gdbarch_from_file (core_bfd);
 
-  push_target (&core_ops);
-  discard_cleanups (old_chain);
+  push_target (target);
+  target_holder.release ();
 
   /* Do this before acknowledging the inferior, so if
      post_create_inferior throws (can happen easilly if you're loading
@@ -463,7 +471,7 @@ core_target_open (const char *arg, int from_tty)
 	switch_to_thread (thread->ptid);
     }
 
-  post_create_inferior (&core_ops, from_tty);
+  post_create_inferior (target, from_tty);
 
   /* Now go through the target stack looking for threads since there
      may be a thread_stratum target loaded on top of target core by
@@ -490,6 +498,8 @@ core_target_open (const char *arg, int from_tty)
   siggy = bfd_core_file_failing_signal (core_bfd);
   if (siggy > 0)
     {
+      gdbarch *core_gdbarch = target->core_gdbarch ();
+
       /* If we don't have a CORE_GDBARCH to work with, assume a native
 	 core (map gdb_signal from host signals).  If we do have
 	 CORE_GDBARCH to work with, but no gdb_signal_from_target
@@ -544,8 +554,8 @@ core_target::detach (inferior *inf, int from_tty)
 }
 
 /* Try to retrieve registers from a section in core_bfd, and supply
-   them to core_vec->core_read_registers, as the register set numbered
-   WHICH.
+   them to m_core_vec->core_read_registers, as the register set
+   numbered WHICH.
 
    If ptid's lwp member is zero, do the single-threaded
    thing: look for a section named NAME.  If ptid's lwp
@@ -556,18 +566,17 @@ core_target::detach (inferior *inf, int from_tty)
    HUMAN_NAME is a human-readable name for the kind of registers the
    NAME section contains, for use in error messages.
 
-   If REQUIRED is non-zero, print an error if the core file doesn't
-   have a section by the appropriate name.  Otherwise, just do
-   nothing.  */
+   If REQUIRED is true, print an error if the core file doesn't have a
+   section by the appropriate name.  Otherwise, just do nothing.  */
 
-static void
-get_core_register_section (struct regcache *regcache,
-			   const struct regset *regset,
-			   const char *name,
-			   int min_size,
-			   int which,
-			   const char *human_name,
-			   int required)
+void
+core_target::get_core_register_section (struct regcache *regcache,
+					const struct regset *regset,
+					const char *name,
+					int min_size,
+					int which,
+					const char *human_name,
+					bool required)
 {
   struct bfd_section *section;
   bfd_size_type size;
@@ -614,12 +623,19 @@ get_core_register_section (struct regcache *regcache,
       return;
     }
 
-  gdb_assert (core_vec);
-  core_vec->core_read_registers (regcache, contents, size, which,
-				 ((CORE_ADDR)
-				  bfd_section_vma (core_bfd, section)));
+  gdb_assert (m_core_vec != nullptr);
+  m_core_vec->core_read_registers (regcache, contents, size, which,
+				   ((CORE_ADDR)
+				    bfd_section_vma (core_bfd, section)));
 }
 
+/* Data passed to gdbarch_iterate_over_regset_sections's callback.  */
+struct get_core_registers_cb_data
+{
+  core_target *target;
+  struct regcache *regcache;
+};
+
 /* Callback for get_core_registers that handles a single core file
    register note section. */
 
@@ -628,12 +644,12 @@ get_core_registers_cb (const char *sect_name, int size,
 		       const struct regset *regset,
 		       const char *human_name, void *cb_data)
 {
-  struct regcache *regcache = (struct regcache *) cb_data;
-  int required = 0;
+  auto *data = (get_core_registers_cb_data *) cb_data;
+  bool required = false;
 
   if (strcmp (sect_name, ".reg") == 0)
     {
-      required = 1;
+      required = true;
       if (human_name == NULL)
 	human_name = "general-purpose";
     }
@@ -645,8 +661,8 @@ get_core_registers_cb (const char *sect_name, int size,
 
   /* The 'which' parameter is only used when no regset is provided.
      Thus we just set it to -1. */
-  get_core_register_section (regcache, regset, sect_name,
-			     size, -1, human_name, required);
+  data->target->get_core_register_section (data->regcache, regset, sect_name,
+					   size, -1, human_name, required);
 }
 
 /* Get the registers out of a core file.  This is the machine-
@@ -662,8 +678,9 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
   int i;
   struct gdbarch *gdbarch;
 
-  if (!(core_gdbarch && gdbarch_iterate_over_regset_sections_p (core_gdbarch))
-      && (core_vec == NULL || core_vec->core_read_registers == NULL))
+  if (!(m_core_gdbarch != nullptr
+	&& gdbarch_iterate_over_regset_sections_p (m_core_gdbarch))
+      && (m_core_vec == NULL || m_core_vec->core_read_registers == NULL))
     {
       fprintf_filtered (gdb_stderr,
 		     "Can't fetch registers from this type of core file\n");
@@ -672,9 +689,12 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
 
   gdbarch = regcache->arch ();
   if (gdbarch_iterate_over_regset_sections_p (gdbarch))
-    gdbarch_iterate_over_regset_sections (gdbarch,
-					  get_core_registers_cb,
-					  (void *) regcache, NULL);
+    {
+      get_core_registers_cb_data data = { this, regcache };
+      gdbarch_iterate_over_regset_sections (gdbarch,
+					    get_core_registers_cb,
+					    (void *) &data, NULL);
+    }
   else
     {
       get_core_register_section (regcache, NULL,
@@ -692,7 +712,7 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
 void
 core_target::files_info ()
 {
-  print_section_info (core_data, core_bfd);
+  print_section_info (&m_core_section_table, core_bfd);
 }
 \f
 struct spuid_list
@@ -733,11 +753,12 @@ core_target::xfer_partial (enum target_object object, const char *annex,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
-      return section_table_xfer_memory_partial (readbuf, writebuf,
-						offset, len, xfered_len,
-						core_data->sections,
-						core_data->sections_end,
-						NULL);
+      return (section_table_xfer_memory_partial
+	      (readbuf, writebuf,
+	       offset, len, xfered_len,
+	       m_core_section_table.sections,
+	       m_core_section_table.sections_end,
+	       NULL));
 
     case TARGET_OBJECT_AUXV:
       if (readbuf)
@@ -810,14 +831,14 @@ core_target::xfer_partial (enum target_object object, const char *annex,
       return TARGET_XFER_E_IO;
 
     case TARGET_OBJECT_LIBRARIES:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_p (core_gdbarch))
+      if (m_core_gdbarch != nullptr
+	  && gdbarch_core_xfer_shared_libraries_p (m_core_gdbarch))
 	{
 	  if (writebuf)
 	    return TARGET_XFER_E_IO;
 	  else
 	    {
-	      *xfered_len = gdbarch_core_xfer_shared_libraries (core_gdbarch,
+	      *xfered_len = gdbarch_core_xfer_shared_libraries (m_core_gdbarch,
 								readbuf,
 								offset, len);
 
@@ -830,15 +851,15 @@ core_target::xfer_partial (enum target_object object, const char *annex,
       /* FALL THROUGH */
 
     case TARGET_OBJECT_LIBRARIES_AIX:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_aix_p (core_gdbarch))
+      if (m_core_gdbarch != nullptr
+	  && gdbarch_core_xfer_shared_libraries_aix_p (m_core_gdbarch))
 	{
 	  if (writebuf)
 	    return TARGET_XFER_E_IO;
 	  else
 	    {
 	      *xfered_len
-		= gdbarch_core_xfer_shared_libraries_aix (core_gdbarch,
+		= gdbarch_core_xfer_shared_libraries_aix (m_core_gdbarch,
 							  readbuf, offset,
 							  len);
 
@@ -911,10 +932,10 @@ core_target::xfer_partial (enum target_object object, const char *annex,
     case TARGET_OBJECT_SIGNAL_INFO:
       if (readbuf)
 	{
-	  if (core_gdbarch
-	      && gdbarch_core_xfer_siginfo_p (core_gdbarch))
+	  if (m_core_gdbarch != nullptr
+	      && gdbarch_core_xfer_siginfo_p (m_core_gdbarch))
 	    {
-	      LONGEST l = gdbarch_core_xfer_siginfo  (core_gdbarch, readbuf,
+	      LONGEST l = gdbarch_core_xfer_siginfo  (m_core_gdbarch, readbuf,
 						      offset, len);
 
 	      if (l >= 0)
@@ -953,16 +974,16 @@ core_target::thread_alive (ptid_t ptid)
 /* Ask the current architecture what it knows about this core file.
    That will be used, in turn, to pick a better architecture.  This
    wrapper could be avoided if targets got a chance to specialize
-   core_ops.  */
+   core_target.  */
 
 const struct target_desc *
 core_target::read_description ()
 {
-  if (core_gdbarch && gdbarch_core_read_description_p (core_gdbarch))
+  if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
     {
       const struct target_desc *result;
 
-      result = gdbarch_core_read_description (core_gdbarch, this, core_bfd);
+      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
       if (result != NULL)
 	return result;
     }
@@ -979,9 +1000,9 @@ core_target::pid_to_str (ptid_t ptid)
 
   /* The preferred way is to have a gdbarch/OS specific
      implementation.  */
-  if (core_gdbarch
-      && gdbarch_core_pid_to_str_p (core_gdbarch))
-    return gdbarch_core_pid_to_str (core_gdbarch, ptid);
+  if (m_core_gdbarch != nullptr
+      && gdbarch_core_pid_to_str_p (m_core_gdbarch))
+    return gdbarch_core_pid_to_str (m_core_gdbarch, ptid);
 
   /* Otherwise, if we don't have one, we'll just fallback to
      "process", with normal_pid_to_str.  */
@@ -1005,9 +1026,9 @@ core_target::pid_to_str (ptid_t ptid)
 const char *
 core_target::thread_name (struct thread_info *thr)
 {
-  if (core_gdbarch
-      && gdbarch_core_thread_name_p (core_gdbarch))
-    return gdbarch_core_thread_name (core_gdbarch, thr);
+  if (m_core_gdbarch != nullptr
+      && gdbarch_core_thread_name_p (m_core_gdbarch))
+    return gdbarch_core_thread_name (m_core_gdbarch, thr);
   return NULL;
 }
 
-- 
2.14.3

Re: [PATCH 3/3] Heap-allocate core_target instances

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This gets rid of the core_ops global, and replaces it with
Pedro> heap-allocated core_target instances.  In practice, there will only be
Pedro> one such instance, though that will change further ahead as more
Pedro> pieces of multi-target support are merged.

Pedro> +  /* FIXME: kettenis/20031023: Eventually this variable should
Pedro> +     disappear.  */
Pedro> +  struct gdbarch *m_core_gdbarch = NULL;

This comment is very gdb.


It was a bit hard to read the patch but I think I excerpted this correctly:

Pedro> +void
Pedro> +core_target::close ()
[...]
Pedro> +  delete this;

I realize this is what was planned from your c++-ification series, but
today I was wondering if this code path:

    void
    core_target::detach (inferior *inf, int from_tty)
    {
      unpush_target (this);

  =>

    int
    unpush_target (struct target_ops *t)
    ...
      target_close (t);

invokes undefined behavior.

"delete this" seems to be ok if you are careful not to use "this"
afterward, but is this particular use ok?  I don't know the rule here
but I thought I'd mention it just in case.

Pedro> +/* Deleter for std::unique_ptr.  Closes the target if an exception is
Pedro> +   thrown.  */
Pedro> +struct target_ops_closer
Pedro> +{
Pedro> +  void operator() (target_ops *target)
Pedro> +  {
Pedro> +    target->close ();
Pedro> +  }
Pedro> +};

This seems like something to put in target.h.

Tom

Re: [PATCH 3/3] Heap-allocate core_target instances

[Pedro Alves]

Hi Tom,

Thanks for the review.

On 05/04/2018 05:36 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This gets rid of the core_ops global, and replaces it with
> Pedro> heap-allocated core_target instances.  In practice, there will only be
> Pedro> one such instance, though that will change further ahead as more
> Pedro> pieces of multi-target support are merged.
> 
> Pedro> +  /* FIXME: kettenis/20031023: Eventually this variable should
> Pedro> +     disappear.  */
> Pedro> +  struct gdbarch *m_core_gdbarch = NULL;
> 
> This comment is very gdb.

The joys of a codebase with a long history.  :-)

> 
> 
> It was a bit hard to read the patch 

Note the patch is harder to read if you read it locally using the
default diff algorithm.  The version posted to the list was generated
with git's --patience diff algorithm, which understands this
patch better than the default algorithm.

but I think I excerpted this correctly:

> 
> Pedro> +void
> Pedro> +core_target::close ()
> [...]
> Pedro> +  delete this;
> 
> I realize this is what was planned from your c++-ification series, but
> today I was wondering if this code path:
> 
>     void
>     core_target::detach (inferior *inf, int from_tty)
>     {
>       unpush_target (this);
> 
>   =>
> 
>     int
>     unpush_target (struct target_ops *t)
>     ...
>       target_close (t);
> 
> invokes undefined behavior.

Hmm, not seeing a problem.  T is not used by unpush_target after
the target_close call, and nor is "this" used after the unpush_target
call in core_target::detach.

> 
> "delete this" seems to be ok if you are careful not to use "this"
> afterward, 

Yeah, it's a common idiom in a decref()/release() method of reference
counted types, for example (when the reference count reaches 0).
As long as you don't use 'this' afterwards, it's OK.

> but is this particular use ok?  I don't know the rule here
> but I thought I'd mention it just in case.

It seems OK to me.  The core_target::detach case might be less
obvious?  I'm adding some comments.

> 
> Pedro> +/* Deleter for std::unique_ptr.  Closes the target if an exception is
> Pedro> +   thrown.  */
> Pedro> +struct target_ops_closer
> Pedro> +{
> Pedro> +  void operator() (target_ops *target)
> Pedro> +  {
> Pedro> +    target->close ();
> Pedro> +  }
> Pedro> +};
> 
> This seems like something to put in target.h.

Done.

Let me know what you think of this version.

From 18891f9cf0d403f93448cdded9d7c1fbe33186e7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 6 May 2018 16:24:21 +0100
Subject: [PATCH 3/3] Heap-allocate core_target instances

This gets rid of the core_ops global, and replaces it with
heap-allocated core_target instances.  In practice, there will only be
one such instance, though that will change further ahead as more
pieces of multi-target support are merged.

Notice that this replaces one heap-allocated object for another, the
number of allocations is the same.  Specifically, currently we
heap-allocate the 'core_data' object, which holds the core's section
table.  With this patch, that object is made a field of the
core_target class, and no longer allocated separately.

Note that this bit:

  -  /* Looks semi-reasonable.  Toss the old core file and work on the
  -     new.  */
  -
  -  unpush_target (&core_ops);

does not need a replacement, because by the time we get here, the
target_preopen call at the top of core_target_open has already
unpushed any previous target.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* corelow.c (core_target) <core_target>: No longer inline.
	Initialize m_core_gdbarch, m_core_vec and build the section table
	here.
	<~core_target>: New.
	<core_gdbarch, get_core_register_section>: New methods.
	<m_core_section_table, m_core_vec, m_core_gdbarch>: New fields,
	factored out from ...
	<core_data, core_vec, core_gdbarch>: ... these deleted globals.
	(core_ops): Delete.
	(sniff_core_bfd): Add gdbarch parameter.
	(core_close): Delete, merged into ...
	(core_target::close): ... here.  Delete self.
	(core_close_cleanup): Delete.
	(core_target_open): Allocate a core_target on the heap.  Use a
	unique_ptr instead of a cleanup.  Bits moved into the core_target
	ctor.  Adjust to use core_target methods instead of globals.
	(get_core_register_section): Rename to ...
	(core_target::get_core_register_section): ... this and adjust.
	(struct get_core_registers_cb_data): New.
	(get_core_registers_cb): Use it.  Use bool.
	(core_target::fetch_registers, core_target::files_info)
	(core_target::xfer_partial, core_target::read_description)
	(core_target::pid_to, core_target::thread_name): Adjust to
	reference class fields instead of globals.
	* target.h (struct target_ops_deleter, target_ops_up): New.
---
 gdb/corelow.c | 265 +++++++++++++++++++++++++++++++---------------------------
 gdb/target.h  |  14 ++++
 2 files changed, 157 insertions(+), 122 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 3b71a1b856..439fe1338a 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -50,6 +50,9 @@
 #define O_LARGEFILE 0
 #endif
 
+static core_fns *sniff_core_bfd (gdbarch *core_gdbarch,
+				 bfd *abfd);
+
 /* The core file target.  */
 
 static const target_info core_target_info = {
@@ -61,8 +64,8 @@ static const target_info core_target_info = {
 class core_target final : public target_ops
 {
 public:
-  core_target ()
-  { to_stratum = process_stratum; }
+  core_target ();
+  ~core_target () override;
 
   const target_info &info () const override
   { return core_target_info; }
@@ -90,42 +93,75 @@ public:
   bool has_stack () override;
   bool has_registers () override;
   bool info_proc (const char *, enum info_proc_what) override;
+
+  /* A few helpers.  */
+
+  /* Getter, see variable definition.  */
+  struct gdbarch *core_gdbarch ()
+  {
+    return m_core_gdbarch;
+  }
+
+  /* See definition.  */
+  void get_core_register_section (struct regcache *regcache,
+				  const struct regset *regset,
+				  const char *name,
+				  int min_size,
+				  int which,
+				  const char *human_name,
+				  bool required);
+
+private: /* per-core data */
+
+  /* The core's section table.  Note that these target sections are
+     *not* mapped in the current address spaces' set of target
+     sections --- those should come only from pure executable or
+     shared library bfds.  The core bfd sections are an implementation
+     detail of the core target, just like ptrace is for unix child
+     targets.  */
+  target_section_table m_core_section_table {};
+
+  /* The core_fns for a core file handler that is prepared to read the
+     core file currently open on core_bfd.  */
+  core_fns *m_core_vec = NULL;
+
+  /* FIXME: kettenis/20031023: Eventually this field should
+     disappear.  */
+  struct gdbarch *m_core_gdbarch = NULL;
 };
 
+core_target::core_target ()
+{
+  to_stratum = process_stratum;
+
+  m_core_gdbarch = gdbarch_from_bfd (core_bfd);
+
+  /* Find a suitable core file handler to munch on core_bfd */
+  m_core_vec = sniff_core_bfd (m_core_gdbarch, core_bfd);
+
+  /* Find the data section */
+  if (build_section_table (core_bfd,
+			   &m_core_section_table.sections,
+			   &m_core_section_table.sections_end))
+    error (_("\"%s\": Can't find sections: %s"),
+	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
+}
+
+core_target::~core_target ()
+{
+  xfree (m_core_section_table.sections);
+}
+
 /* List of all available core_fns.  On gdb startup, each core file
    register reader calls deprecated_add_core_fns() to register
    information on each core format it is prepared to read.  */
 
 static struct core_fns *core_file_fns = NULL;
 
-/* The core_fns for a core file handler that is prepared to read the
-   core file currently open on core_bfd.  */
-
-static struct core_fns *core_vec = NULL;
-
-/* FIXME: kettenis/20031023: Eventually this variable should
-   disappear.  */
-
-static struct gdbarch *core_gdbarch = NULL;
-
-/* Per-core data.  Currently, only the section table.  Note that these
-   target sections are *not* mapped in the current address spaces' set
-   of target sections --- those should come only from pure executable
-   or shared library bfds.  The core bfd sections are an
-   implementation detail of the core target, just like ptrace is for
-   unix child targets.  */
-static struct target_section_table *core_data;
-
-static struct core_fns *sniff_core_bfd (bfd *);
-
 static int gdb_check_format (bfd *);
 
-static void core_close_cleanup (void *ignore);
-
 static void add_to_thread_list (bfd *, asection *, void *);
 
-static core_target core_ops;
-
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
 
@@ -159,7 +195,7 @@ default_core_sniffer (struct core_fns *our_fns, bfd *abfd)
    selected.  */
 
 static struct core_fns *
-sniff_core_bfd (bfd *abfd)
+sniff_core_bfd (struct gdbarch *core_gdbarch, bfd *abfd)
 {
   struct core_fns *cf;
   struct core_fns *yummy = NULL;
@@ -217,11 +253,10 @@ gdb_check_format (bfd *abfd)
   return (0);
 }
 
-/* Discard all vestiges of any previous core file and mark data and
-   stack spaces as empty.  */
+/* Close the core target.  */
 
-static void
-core_close ()
+void
+core_target::close ()
 {
   if (core_bfd)
     {
@@ -235,30 +270,13 @@ core_close ()
          comments in clear_solib in solib.c.  */
       clear_solib ();
 
-      if (core_data)
-	{
-	  xfree (core_data->sections);
-	  xfree (core_data);
-	  core_data = NULL;
-	}
-
       gdb_bfd_unref (core_bfd);
       core_bfd = NULL;
     }
-  core_vec = NULL;
-  core_gdbarch = NULL;
-}
 
-static void
-core_close_cleanup (void *ignore)
-{
-  core_close ();
-}
-
-void
-core_target::close ()
-{
-  core_close ();
+  /* Core targets are heap-allocated (see core_target_open), so here
+     we delete ourselves.  */
+  delete this;
 }
 
 /* Look for sections whose names start with `.reg/' so that we can
@@ -388,29 +406,15 @@ core_target_open (const char *arg, int from_tty)
 	     filename.get (), bfd_errmsg (bfd_get_error ()));
     }
 
-  /* Looks semi-reasonable.  Toss the old core file and work on the
-     new.  */
-
-  unpush_target (&core_ops);
   core_bfd = temp_bfd.release ();
-  old_chain = make_cleanup (core_close_cleanup, 0 /*ignore*/);
 
-  core_gdbarch = gdbarch_from_bfd (core_bfd);
+  core_target *target = new core_target ();
 
-  /* Find a suitable core file handler to munch on core_bfd */
-  core_vec = sniff_core_bfd (core_bfd);
+  /* Own the target until it is successfully pushed.  */
+  target_ops_up target_holder (target);
 
   validate_files ();
 
-  core_data = XCNEW (struct target_section_table);
-
-  /* Find the data section */
-  if (build_section_table (core_bfd,
-			   &core_data->sections,
-			   &core_data->sections_end))
-    error (_("\"%s\": Can't find sections: %s"),
-	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
-
   /* If we have no exec file, try to set the architecture from the
      core file.  We don't do this unconditionally since an exec file
      typically contains more information that helps us determine the
@@ -418,8 +422,8 @@ core_target_open (const char *arg, int from_tty)
   if (!exec_bfd)
     set_gdbarch_from_file (core_bfd);
 
-  push_target (&core_ops);
-  discard_cleanups (old_chain);
+  push_target (target);
+  target_holder.release ();
 
   /* Do this before acknowledging the inferior, so if
      post_create_inferior throws (can happen easilly if you're loading
@@ -463,7 +467,7 @@ core_target_open (const char *arg, int from_tty)
 	switch_to_thread (thread->ptid);
     }
 
-  post_create_inferior (&core_ops, from_tty);
+  post_create_inferior (target, from_tty);
 
   /* Now go through the target stack looking for threads since there
      may be a thread_stratum target loaded on top of target core by
@@ -490,6 +494,8 @@ core_target_open (const char *arg, int from_tty)
   siggy = bfd_core_file_failing_signal (core_bfd);
   if (siggy > 0)
     {
+      gdbarch *core_gdbarch = target->core_gdbarch ();
+
       /* If we don't have a CORE_GDBARCH to work with, assume a native
 	 core (map gdb_signal from host signals).  If we do have
 	 CORE_GDBARCH to work with, but no gdb_signal_from_target
@@ -538,14 +544,18 @@ core_target_open (const char *arg, int from_tty)
 void
 core_target::detach (inferior *inf, int from_tty)
 {
+  /* Note that 'this' is dangling after this call.  unpush_target
+     closes the target, and our close implementation deletes
+     'this'.  */
   unpush_target (this);
+
   reinit_frame_cache ();
   maybe_say_no_core_file_now (from_tty);
 }
 
 /* Try to retrieve registers from a section in core_bfd, and supply
-   them to core_vec->core_read_registers, as the register set numbered
-   WHICH.
+   them to m_core_vec->core_read_registers, as the register set
+   numbered WHICH.
 
    If ptid's lwp member is zero, do the single-threaded
    thing: look for a section named NAME.  If ptid's lwp
@@ -556,18 +566,17 @@ core_target::detach (inferior *inf, int from_tty)
    HUMAN_NAME is a human-readable name for the kind of registers the
    NAME section contains, for use in error messages.
 
-   If REQUIRED is non-zero, print an error if the core file doesn't
-   have a section by the appropriate name.  Otherwise, just do
-   nothing.  */
+   If REQUIRED is true, print an error if the core file doesn't have a
+   section by the appropriate name.  Otherwise, just do nothing.  */
 
-static void
-get_core_register_section (struct regcache *regcache,
-			   const struct regset *regset,
-			   const char *name,
-			   int min_size,
-			   int which,
-			   const char *human_name,
-			   int required)
+void
+core_target::get_core_register_section (struct regcache *regcache,
+					const struct regset *regset,
+					const char *name,
+					int min_size,
+					int which,
+					const char *human_name,
+					bool required)
 {
   struct bfd_section *section;
   bfd_size_type size;
@@ -614,12 +623,19 @@ get_core_register_section (struct regcache *regcache,
       return;
     }
 
-  gdb_assert (core_vec);
-  core_vec->core_read_registers (regcache, contents, size, which,
-				 ((CORE_ADDR)
-				  bfd_section_vma (core_bfd, section)));
+  gdb_assert (m_core_vec != nullptr);
+  m_core_vec->core_read_registers (regcache, contents, size, which,
+				   ((CORE_ADDR)
+				    bfd_section_vma (core_bfd, section)));
 }
 
+/* Data passed to gdbarch_iterate_over_regset_sections's callback.  */
+struct get_core_registers_cb_data
+{
+  core_target *target;
+  struct regcache *regcache;
+};
+
 /* Callback for get_core_registers that handles a single core file
    register note section. */
 
@@ -628,12 +644,12 @@ get_core_registers_cb (const char *sect_name, int size,
 		       const struct regset *regset,
 		       const char *human_name, void *cb_data)
 {
-  struct regcache *regcache = (struct regcache *) cb_data;
-  int required = 0;
+  auto *data = (get_core_registers_cb_data *) cb_data;
+  bool required = false;
 
   if (strcmp (sect_name, ".reg") == 0)
     {
-      required = 1;
+      required = true;
       if (human_name == NULL)
 	human_name = "general-purpose";
     }
@@ -645,8 +661,8 @@ get_core_registers_cb (const char *sect_name, int size,
 
   /* The 'which' parameter is only used when no regset is provided.
      Thus we just set it to -1. */
-  get_core_register_section (regcache, regset, sect_name,
-			     size, -1, human_name, required);
+  data->target->get_core_register_section (data->regcache, regset, sect_name,
+					   size, -1, human_name, required);
 }
 
 /* Get the registers out of a core file.  This is the machine-
@@ -662,8 +678,9 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
   int i;
   struct gdbarch *gdbarch;
 
-  if (!(core_gdbarch && gdbarch_iterate_over_regset_sections_p (core_gdbarch))
-      && (core_vec == NULL || core_vec->core_read_registers == NULL))
+  if (!(m_core_gdbarch != nullptr
+	&& gdbarch_iterate_over_regset_sections_p (m_core_gdbarch))
+      && (m_core_vec == NULL || m_core_vec->core_read_registers == NULL))
     {
       fprintf_filtered (gdb_stderr,
 		     "Can't fetch registers from this type of core file\n");
@@ -672,9 +689,12 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
 
   gdbarch = regcache->arch ();
   if (gdbarch_iterate_over_regset_sections_p (gdbarch))
-    gdbarch_iterate_over_regset_sections (gdbarch,
-					  get_core_registers_cb,
-					  (void *) regcache, NULL);
+    {
+      get_core_registers_cb_data data = { this, regcache };
+      gdbarch_iterate_over_regset_sections (gdbarch,
+					    get_core_registers_cb,
+					    (void *) &data, NULL);
+    }
   else
     {
       get_core_register_section (regcache, NULL,
@@ -692,7 +712,7 @@ core_target::fetch_registers (struct regcache *regcache, int regno)
 void
 core_target::files_info ()
 {
-  print_section_info (core_data, core_bfd);
+  print_section_info (&m_core_section_table, core_bfd);
 }
 \f
 struct spuid_list
@@ -733,11 +753,12 @@ core_target::xfer_partial (enum target_object object, const char *annex,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
-      return section_table_xfer_memory_partial (readbuf, writebuf,
-						offset, len, xfered_len,
-						core_data->sections,
-						core_data->sections_end,
-						NULL);
+      return (section_table_xfer_memory_partial
+	      (readbuf, writebuf,
+	       offset, len, xfered_len,
+	       m_core_section_table.sections,
+	       m_core_section_table.sections_end,
+	       NULL));
 
     case TARGET_OBJECT_AUXV:
       if (readbuf)
@@ -810,14 +831,14 @@ core_target::xfer_partial (enum target_object object, const char *annex,
       return TARGET_XFER_E_IO;
 
     case TARGET_OBJECT_LIBRARIES:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_p (core_gdbarch))
+      if (m_core_gdbarch != nullptr
+	  && gdbarch_core_xfer_shared_libraries_p (m_core_gdbarch))
 	{
 	  if (writebuf)
 	    return TARGET_XFER_E_IO;
 	  else
 	    {
-	      *xfered_len = gdbarch_core_xfer_shared_libraries (core_gdbarch,
+	      *xfered_len = gdbarch_core_xfer_shared_libraries (m_core_gdbarch,
 								readbuf,
 								offset, len);
 
@@ -830,15 +851,15 @@ core_target::xfer_partial (enum target_object object, const char *annex,
       /* FALL THROUGH */
 
     case TARGET_OBJECT_LIBRARIES_AIX:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_aix_p (core_gdbarch))
+      if (m_core_gdbarch != nullptr
+	  && gdbarch_core_xfer_shared_libraries_aix_p (m_core_gdbarch))
 	{
 	  if (writebuf)
 	    return TARGET_XFER_E_IO;
 	  else
 	    {
 	      *xfered_len
-		= gdbarch_core_xfer_shared_libraries_aix (core_gdbarch,
+		= gdbarch_core_xfer_shared_libraries_aix (m_core_gdbarch,
 							  readbuf, offset,
 							  len);
 
@@ -911,10 +932,10 @@ core_target::xfer_partial (enum target_object object, const char *annex,
     case TARGET_OBJECT_SIGNAL_INFO:
       if (readbuf)
 	{
-	  if (core_gdbarch
-	      && gdbarch_core_xfer_siginfo_p (core_gdbarch))
+	  if (m_core_gdbarch != nullptr
+	      && gdbarch_core_xfer_siginfo_p (m_core_gdbarch))
 	    {
-	      LONGEST l = gdbarch_core_xfer_siginfo  (core_gdbarch, readbuf,
+	      LONGEST l = gdbarch_core_xfer_siginfo  (m_core_gdbarch, readbuf,
 						      offset, len);
 
 	      if (l >= 0)
@@ -953,16 +974,16 @@ core_target::thread_alive (ptid_t ptid)
 /* Ask the current architecture what it knows about this core file.
    That will be used, in turn, to pick a better architecture.  This
    wrapper could be avoided if targets got a chance to specialize
-   core_ops.  */
+   core_target.  */
 
 const struct target_desc *
 core_target::read_description ()
 {
-  if (core_gdbarch && gdbarch_core_read_description_p (core_gdbarch))
+  if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
     {
       const struct target_desc *result;
 
-      result = gdbarch_core_read_description (core_gdbarch, this, core_bfd);
+      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
       if (result != NULL)
 	return result;
     }
@@ -979,9 +1000,9 @@ core_target::pid_to_str (ptid_t ptid)
 
   /* The preferred way is to have a gdbarch/OS specific
      implementation.  */
-  if (core_gdbarch
-      && gdbarch_core_pid_to_str_p (core_gdbarch))
-    return gdbarch_core_pid_to_str (core_gdbarch, ptid);
+  if (m_core_gdbarch != nullptr
+      && gdbarch_core_pid_to_str_p (m_core_gdbarch))
+    return gdbarch_core_pid_to_str (m_core_gdbarch, ptid);
 
   /* Otherwise, if we don't have one, we'll just fallback to
      "process", with normal_pid_to_str.  */
@@ -1005,9 +1026,9 @@ core_target::pid_to_str (ptid_t ptid)
 const char *
 core_target::thread_name (struct thread_info *thr)
 {
-  if (core_gdbarch
-      && gdbarch_core_thread_name_p (core_gdbarch))
-    return gdbarch_core_thread_name (core_gdbarch, thr);
+  if (m_core_gdbarch != nullptr
+      && gdbarch_core_thread_name_p (m_core_gdbarch))
+    return gdbarch_core_thread_name (m_core_gdbarch, thr);
   return NULL;
 }
 
diff --git a/gdb/target.h b/gdb/target.h
index b37702b501..e2d1e61cdc 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1243,6 +1243,20 @@ struct target_ops
       TARGET_DEFAULT_IGNORE ();
   };
 
+/* Deleter for std::unique_ptr.  See comments in
+   target_ops::~target_ops and target_ops::close about heap-allocated
+   targets.  */
+struct target_ops_deleter
+{
+  void operator() (target_ops *target)
+  {
+    target->close ();
+  }
+};
+
+/* A unique pointer for target_ops.  */
+typedef std::unique_ptr<target_ops, target_ops_deleter> target_ops_up;
+
 /* Native target backends call this once at initialization time to
    inform the core about which is the target that can respond to "run"
    or "attach".  Note: native targets are always singletons.  */
-- 
2.14.3

Re: [PATCH 3/3] Heap-allocate core_target instances

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> It seems OK to me.  The core_target::detach case might be less
Pedro> obvious?  I'm adding some comments.

Thanks.

Pedro> Let me know what you think of this version.

Looks reasonable to me, thanks.

Tom

Re: [PATCH 3/3] Heap-allocate core_target instances

[Pedro Alves]

On 05/07/2018 03:26 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> It seems OK to me.  The core_target::detach case might be less
> Pedro> obvious?  I'm adding some comments.
> 
> Thanks.
> 
> Pedro> Let me know what you think of this version.
> 
> Looks reasonable to me, thanks.
Thanks Tom.  I'm pushing this in.

Pedro Alves

[PATCH 2/3] Eliminate the 'the_core_target' global

[Pedro Alves]

(previously called 'core_target', but since renamed because
'core_target' is the name of the target_ops class now.)

This eliminates the "the_core_target" global, as preparation for being
able to have more than one core loaded.  When we get there, we will
instantiate one core_target object per core instead.

Essentially, this replaces the reference to the_core_target in
core_file_command by a reference to core_bfd, which is per
program_space.

Currently, core_file_command calls 'the_core_target->detach()' even if
the core target is not open and pushed on the target stack.  If it is
indeed not open, then the practical effect is that
core_target::detach() prints "No core file now.".  That is preserved
by printing that directly from within core_file_command if not
debugging a core.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* corefile.c (core_file_command): Move to corelow.c.
	* corelow.c (the_core_target): Delete.
	(core_file_command): Moved from corefile.c.  Check exec_bfd
	instead of the_core_target.  Use target_detach instead of calling
	into the_core_target directly.
	(maybe_say_no_core_file_now): New.
	(core_target::detach): Use it.
	(_initialize_corelow): Remove references to the_core_target.
	* gdbcore.h (the_core_target): Delete.
---
 gdb/corefile.c | 18 ------------------
 gdb/corelow.c  | 42 +++++++++++++++++++++++++++++++-----------
 gdb/gdbcore.h  |  4 ----
 3 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index e0c7540140..06b26863f3 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -51,24 +51,6 @@ static int exec_file_hook_count = 0;		/* Size of array.  */
 
 \f
 
-/* Backward compatability with old way of specifying core files.  */
-
-void
-core_file_command (const char *filename, int from_tty)
-{
-  dont_repeat ();		/* Either way, seems bogus.  */
-
-  if (!filename)
-    {
-      gdb_assert (the_core_target != NULL);
-
-      the_core_target->detach (current_inferior (), from_tty);
-    }
-  else
-    core_target_open (filename, from_tty);
-}
-\f
-
 /* If there are two or more functions that wish to hook into
    exec_file_command, this function will call all of the hook
    functions.  */
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 97a957c8fc..3b71a1b856 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -92,9 +92,6 @@ public:
   bool info_proc (const char *, enum info_proc_what) override;
 };
 
-/* See gdbcore.h.  */
-struct target_ops *the_core_target;
-
 /* List of all available core_fns.  On gdb startup, each core file
    register reader calls deprecated_add_core_fns() to register
    information on each core format it is prepared to read.  */
@@ -309,6 +306,36 @@ add_to_thread_list (bfd *abfd, asection *asect, void *reg_sect_arg)
     inferior_ptid = ptid;			/* Yes, make it current.  */
 }
 
+/* Issue a message saying we have no core to debug, if FROM_TTY.  */
+
+static void
+maybe_say_no_core_file_now (int from_tty)
+{
+  if (from_tty)
+    printf_filtered (_("No core file now.\n"));
+}
+
+/* Backward compatability with old way of specifying core files.  */
+
+void
+core_file_command (const char *filename, int from_tty)
+{
+  dont_repeat ();		/* Either way, seems bogus.  */
+
+  if (filename == NULL)
+    {
+      if (core_bfd != NULL)
+	{
+	  target_detach (current_inferior (), from_tty);
+	  gdb_assert (core_bfd == NULL);
+	}
+      else
+	maybe_say_no_core_file_now (from_tty);
+    }
+  else
+    core_target_open (filename, from_tty);
+}
+
 /* See gdbcore.h.  */
 
 void
@@ -513,8 +540,7 @@ core_target::detach (inferior *inf, int from_tty)
 {
   unpush_target (this);
   reinit_frame_cache ();
-  if (from_tty)
-    printf_filtered (_("No core file now.\n"));
+  maybe_say_no_core_file_now (from_tty);
 }
 
 /* Try to retrieve registers from a section in core_bfd, and supply
@@ -1021,11 +1047,5 @@ core_target::info_proc (const char *args, enum info_proc_what request)
 void
 _initialize_corelow (void)
 {
-  if (the_core_target != NULL)
-    internal_error (__FILE__, __LINE__,
-		    _("core target already exists (\"%s\")."),
-		    the_core_target->longname ());
-  the_core_target = &core_ops;
-
   add_target (core_target_info, core_target_open, filename_completer);
 }
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index d06ccc3507..04a4b4767a 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -135,10 +135,6 @@ extern void specify_exec_file_hook (void (*hook) (const char *filename));
 
 #define core_bfd (current_program_space->cbfd)
 
-/* corelow.c target.  It is never NULL after GDB initialization.  */
-
-extern struct target_ops *the_core_target;
-
 /* Whether to open exec and core files read-only or read-write.  */
 
 extern int write_files;
-- 
2.14.3

Re: [PATCH 2/3] Eliminate the 'the_core_target' global

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Currently, core_file_command calls 'the_core_target->detach()' even if
Pedro> the core target is not open and pushed on the target stack.  If it is
Pedro> indeed not open, then the practical effect is that
Pedro> core_target::detach() prints "No core file now.".  That is preserved
Pedro> by printing that directly from within core_file_command if not
Pedro> debugging a core.

I read this patch & it seemed fine to me.

Tom