[PATCH 0/4] baby step toward multi-target

[PATCH 0/4] baby step toward multi-target

[Tom Tromey]

My multi-target branch is basically dead, I think, mostly due to heavy
conflicts with all the other target changes that have happened in the
meantime.  However recently I realized that there is a less invasive
way to achieve many of the same effects, and a way to salvage some of
the smaller cleanups done there.

In particular on the branch I attempted to split target_ops into a
vtable-like object and a target state object.  As you might imagine
this patch touches a large amount of code and is hard both to keep up
to date, and to test.

This little series, on the other hand, takes a less invasive approach.
The idea here is that rather than doing a huge target_ops split,
instead just sometimes make copies of the target_ops when pushing.
This lets the copies keep their own state; copies are needed in the
long run because multiple target stacks will be active and a given
target_ops only has one "beneath" pointer.

Here corelow is used as the example of how to do a conversion.
There's a bit of cleanup and infrastructure initially, and then the
final patch moves the corelow global state into a new subclass of
target_ops, which is instantiated and pushed.

If this approach seems reasonable then it's not too hard to pull over
some of the other target conversions from the branch.

This series also shows the spots to change if you want to make the
debug target wrap each stratum in the target stack -- each wrapping
debug target would be to_xclose-ish, and spots like target_is_pushed
would be taught to unwrap.

Built and regtested on x86-64 Fedora 20.

[PATCH 1/4] move core_bfd to program space

[Tom Tromey]

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 patch is a basic transformation for eventual multi-target work.

2014-07-18  Tom Tromey  <tromey@redhat.com>

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

diff --git a/gdb/corefile.c b/gdb/corefile.c
index 8a96d75..6b62e13 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -57,10 +57,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;
-
 /* corelow.c target.  It is never NULL after GDB initialization.  */
 
 struct target_ops *core_target;
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 3f81791..c48067e 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -138,7 +138,7 @@ extern void specify_exec_file_hook (void (*hook) (char *filename));
 
 /* Binary File Diddler for the core file.  */
 
-extern bfd *core_bfd;
+#define core_bfd (current_program_space->cbfd)
 
 extern struct target_ops *core_target;
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 08e04eb..852d38a 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -206,6 +206,9 @@ struct program_space
        This is so we can properly report solib changes to the user.  */
     VEC (char_ptr) *deleted_solibs;
 
+    /* Binary file diddling handle for the core file.  */
+    bfd *cbfd;
+
     /* Per pspace data-pointers required by other GDB modules.  */
     REGISTRY_FIELDS;
   };
-- 
1.9.3

[PATCH 2/4] simplify target_is_pushed

[Tom Tromey]

While working on target_is_pushed, I noticed that it is written in a
strange way.  The code currently keeps an extra indirection, where a
simple linked list traversal is all that is needed.  It seems likely
this was done by copying and pasting other code.  However, there is no
reason to do this and the more obvious code is simpler to reason
about.  So, this patch change the implementation.

2014-07-18  Tom Tromey  <tromey@redhat.com>

	* target.c (target_is_pushed): Simplify.
---
 gdb/ChangeLog | 4 ++++
 gdb/target.c  | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index b9310cf..3d28f85 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -654,7 +654,7 @@ pop_all_targets (void)
 int
 target_is_pushed (struct target_ops *t)
 {
-  struct target_ops **cur;
+  struct target_ops *cur;
 
   /* Check magic number.  If wrong, it probably means someone changed
      the struct definition, but not all the places that initialize one.  */
@@ -667,8 +667,8 @@ target_is_pushed (struct target_ops *t)
 		      _("failed internal consistency check"));
     }
 
-  for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
-    if (*cur == t)
+  for (cur = target_stack; cur != NULL; cur = cur->beneath)
+    if (cur == t)
       return 1;
 
   return 0;
-- 
1.9.3

Re: [PATCH 2/4] simplify target_is_pushed

[Joel Brobecker]

> While working on target_is_pushed, I noticed that it is written in a
> strange way.  The code currently keeps an extra indirection, where a
> simple linked list traversal is all that is needed.  It seems likely
> this was done by copying and pasting other code.  However, there is no
> reason to do this and the more obvious code is simpler to reason
> about.  So, this patch change the implementation.
> 
> 2014-07-18  Tom Tromey  <tromey@redhat.com>
> 
> 	* target.c (target_is_pushed): Simplify.

Indeed. It looks like this one could go in on its own...

-- 
Joel

Re: [PATCH 2/4] simplify target_is_pushed

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> 2014-07-18  Tom Tromey  <tromey@redhat.com>
>> 
>> * target.c (target_is_pushed): Simplify.

Joel> Indeed. It looks like this one could go in on its own...

Yeah.  I'll push this one soon.  Thanks.

Tom

[PATCH 3/4] add to_identity

[Tom Tromey]

Right now a target_ops has an odd sense of identity.

Most times some identity is needed, a pointer to a static object is
passed in.  For example, calls to unpush_target generally work like
this:

    unpush_target (&exec_ops);

Conceptually this is a kind of "instanceof" checking.

Now, consider this with "to_xclose" targets.  In this case the
target_ops is allocated on the heap and there's no good way to talk
about the identity.  Code could remember the pointer, of course, but
this usually just begs the question.

For example in a to_open implementation it is reasonably normal to
check target_is_pushed and then do nothing if the target is pushed.
However, there's no reasonable way way to do this with a to_xclose
target.

This patch introduces a to_identity field that just points to the
"prototype" implementation of a target_ops.  This lets us convert
targets to to_xclose without difficulty.

2014-07-18  Tom Tromey  <tromey@redhat.com>

	* bfd-target.c (target_bfd_reopen): Set to_identity.
	* target.c (complete_target_initialization): Set to_identity.
	(unpush_target): Check to_identity.  Call target_close on the real
	target.
	(target_is_pushed): Check to_identity.
	* target.h (struct target_ops) <to_identity>: New field.
---
 gdb/ChangeLog    | 9 +++++++++
 gdb/bfd-target.c | 1 +
 gdb/target.c     | 8 +++++---
 gdb/target.h     | 6 ++++++
 4 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/bfd-target.c b/gdb/bfd-target.c
index 519d5e7..d392762 100644
--- a/gdb/bfd-target.c
+++ b/gdb/bfd-target.c
@@ -98,6 +98,7 @@ target_bfd_reopen (struct bfd *abfd)
   t->to_xclose = target_bfd_xclose;
   t->to_data = data;
   t->to_magic = OPS_MAGIC;
+  t->to_identity = t;
 
   return t;
 }
diff --git a/gdb/target.c b/gdb/target.c
index 3d28f85..6a10c04 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -342,6 +342,8 @@ complete_target_initialization (struct target_ops *t)
   gdb_assert (t->to_can_run == NULL || (t->to_can_async_p != NULL
 					&& t->to_supports_non_stop != NULL));
 
+  t->to_identity = t;
+
   install_delegators (t);
 }
 
@@ -602,7 +604,7 @@ unpush_target (struct target_ops *t)
 
   for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
     {
-      if ((*cur) == t)
+      if ((*cur) == t || (*cur)->to_identity == t)
 	break;
     }
 
@@ -621,7 +623,7 @@ unpush_target (struct target_ops *t)
   /* Finally close the target.  Note we do this after unchaining, so
      any target method calls from within the target_close
      implementation don't end up in T anymore.  */
-  target_close (t);
+  target_close (tmp);
 
   return 1;
 }
@@ -668,7 +670,7 @@ target_is_pushed (struct target_ops *t)
     }
 
   for (cur = target_stack; cur != NULL; cur = cur->beneath)
-    if (cur == t)
+    if (cur == t || cur->to_identity == t)
       return 1;
 
   return 0;
diff --git a/gdb/target.h b/gdb/target.h
index 92572ff..58c7b30 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1135,6 +1135,12 @@ struct target_ops
     void (*to_done_generating_core) (struct target_ops *)
       TARGET_DEFAULT_IGNORE ();
 
+    /* This points to an "original" target_ops from which a particular
+       instance may have been cloned.  This is usedful if a to_xclose
+       target clones some other target_ops, but still wants to call
+       target_is_pushed or unpush_target.  */
+    struct target_ops *to_identity;
+
     int to_magic;
     /* Need sub-structure for target machine related rather than comm related?
      */
-- 
1.9.3

Re: [PATCH 3/4] add to_identity

[Joel Brobecker]

Hi Tom,

I just happened to notice a small typo:

> +    /* This points to an "original" target_ops from which a particular
> +       instance may have been cloned.  This is usedful if a to_xclose
                                                  ^^^^^^^

-- 
Joel

Re: [PATCH 3/4] add to_identity

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I just happened to notice a small typo:

>> +    /* This points to an "original" target_ops from which a particular
>> +       instance may have been cloned.  This is usedful if a to_xclose

Thanks, Joel.
I've fixed this locally.

Tom

[PATCH 4/4] convert corelow to to_xclose

[Tom Tromey]

This converts corelow to be a to_xclose target.  It moves all the
corelow-related globals into a new subclass of target_ops and arranges
for core_open to push an instance of this.

This is preparatory work for multi-target.  After this patch, corelow
doesn't rely on global state and multiple instances can readily be
instantiated.

2014-07-18  Tom Tromey  <tromey@redhat.com>

	* corelow.c (struct core_target_ops_with_data): New struct.
	(core_vec, core_gdbarch, core_data): Remove.  Now fields of
	core_target_ops_with_data.
	(sniff_core_bfd): Add core_gdbarch parameter.
	(get_core_target_ops): New function.
	(core_xclose): Rename from core_close.  Update for
	core_target_ops_with_data.  Free the target.
	(core_close_cleanup): Rename parameter.  Update.
	(core_open): Use TARGET_NEW.  Update.
	(get_core_register_section, get_core_registers, core_files_info)
	(core_xfer_partial, core_read_description, core_pid_to_str): Use
	get_core_target_ops.  Update.
	(init_core_ops): Set to_xclose, not to_close.
	* target.c (allocate_target): New function.
	* target.h (allocate_target): Declare.
	(TARGET_NEW): New macro.
---
 gdb/ChangeLog |  19 +++++++
 gdb/corelow.c | 175 +++++++++++++++++++++++++++++++++++-----------------------
 gdb/target.c  |  20 +++++++
 gdb/target.h  |  15 +++++
 4 files changed, 160 insertions(+), 69 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 1775a66..212665d 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -60,34 +60,39 @@
 
 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.  */
+/* A subclass of target_ops that also holds data for a core
+   target.  */
 
-static struct core_fns *core_vec = NULL;
+struct core_target_ops_with_data
+{
+  /* The base class.  */
 
-/* FIXME: kettenis/20031023: Eventually this variable should
-   disappear.  */
+  struct target_ops base;
 
-static struct gdbarch *core_gdbarch = NULL;
+  /* The core_fns for a core file handler that is prepared to read the
+     core file currently open on core_bfd.  */
 
-/* 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;
+  struct core_fns *core_vec;
 
-static void core_files_info (struct target_ops *);
+  /* FIXME: kettenis/20031023: Eventually this variable should
+     disappear.  */
+
+  struct gdbarch *core_gdbarch;
+
+  /* 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.  */
+  struct target_section_table core_data;
+};
 
-static struct core_fns *sniff_core_bfd (bfd *);
+static void core_files_info (struct target_ops *);
 
 static int gdb_check_format (bfd *);
 
 static void core_open (char *, int);
 
-static void core_close (struct target_ops *self);
-
 static void core_close_cleanup (void *ignore);
 
 static void add_to_thread_list (bfd *, asection *, void *);
@@ -131,7 +136,7 @@ default_core_sniffer (struct core_fns *our_fns, bfd *abfd)
    selected.  */
 
 static struct core_fns *
-sniff_core_bfd (bfd *abfd)
+sniff_core_bfd (bfd *abfd, struct gdbarch *core_gdbarch)
 {
   struct core_fns *cf;
   struct core_fns *yummy = NULL;
@@ -189,12 +194,35 @@ gdb_check_format (bfd *abfd)
   return (0);
 }
 
+\f
+
+/* Return the core_target_ops_with_data for the current target stack,
+   if any.  */
+
+static struct core_target_ops_with_data *
+get_core_target_ops (void)
+{
+  struct target_ops *targ = find_target_at (process_stratum);
+
+  if (targ == NULL || targ->to_identity != &core_ops)
+    return NULL;
+
+  /* Downcast.  */
+  return (struct core_target_ops_with_data *) targ;
+}
+
 /* Discard all vestiges of any previous core file and mark data and
    stack spaces as empty.  */
 
 static void
-core_close (struct target_ops *self)
+core_xclose (struct target_ops *self)
 {
+  /* Downcast.  */
+  struct core_target_ops_with_data *cops
+    = (struct core_target_ops_with_data *) self;
+
+  gdb_assert (self->to_identity == &core_ops);
+
   if (core_bfd)
     {
       int pid = ptid_get_pid (inferior_ptid);
@@ -207,24 +235,18 @@ core_close (struct target_ops *self)
          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;
+
+  xfree (cops->core_data.sections);
+  xfree (cops);
 }
 
 static void
-core_close_cleanup (void *ignore)
+core_close_cleanup (void *arg)
 {
-  core_close (NULL);
+  core_xclose (arg);
 }
 
 /* Look for sections whose names start with `.reg/' so that we can
@@ -285,6 +307,7 @@ core_open (char *filename, int from_tty)
   int scratch_chan;
   int flags;
   volatile struct gdb_exception except;
+  struct core_target_ops_with_data *cops;
 
   target_preopen (from_tty);
   if (!filename)
@@ -340,21 +363,21 @@ core_open (char *filename, int from_tty)
   do_cleanups (old_chain);
   unpush_target (&core_ops);
   core_bfd = temp_bfd;
-  old_chain = make_cleanup (core_close_cleanup, 0 /*ignore*/);
 
-  core_gdbarch = gdbarch_from_bfd (core_bfd);
+  cops = TARGET_NEW (struct core_target_ops_with_data, &core_ops);
+  old_chain = make_cleanup (core_close_cleanup, cops);
+
+  cops->core_gdbarch = gdbarch_from_bfd (core_bfd);
 
   /* Find a suitable core file handler to munch on core_bfd */
-  core_vec = sniff_core_bfd (core_bfd);
+  cops->core_vec = sniff_core_bfd (core_bfd, cops->core_gdbarch);
 
   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))
+			   &cops->core_data.sections,
+			   &cops->core_data.sections_end))
     error (_("\"%s\": Can't find sections: %s"),
 	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
 
@@ -365,7 +388,7 @@ core_open (char *filename, int from_tty)
   if (!exec_bfd)
     set_gdbarch_from_file (core_bfd);
 
-  push_target (&core_ops);
+  push_target (&cops->base);
   discard_cleanups (old_chain);
 
   /* Do this before acknowledging the inferior, so if
@@ -410,7 +433,7 @@ core_open (char *filename, int from_tty)
 	switch_to_thread (thread->ptid);
     }
 
-  post_create_inferior (&core_ops, from_tty);
+  post_create_inferior (&cops->base, 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
@@ -440,11 +463,11 @@ core_open (char *filename, int from_tty)
 	 implementation for that gdbarch, as a fallback measure,
 	 assume the host signal mapping.  It'll be correct for native
 	 cores, but most likely incorrect for cross-cores.  */
-      enum gdb_signal sig = (core_gdbarch != NULL
-			     && gdbarch_gdb_signal_from_target_p (core_gdbarch)
-			     ? gdbarch_gdb_signal_from_target (core_gdbarch,
-							       siggy)
-			     : gdb_signal_from_host (siggy));
+      enum gdb_signal sig
+	= (cops->core_gdbarch != NULL
+	   && gdbarch_gdb_signal_from_target_p (cops->core_gdbarch)
+	   ? gdbarch_gdb_signal_from_target (cops->core_gdbarch, siggy)
+	   : gdb_signal_from_host (siggy));
 
       printf_filtered (_("Program terminated with signal %s, %s.\n"),
 		       gdb_signal_to_name (sig), gdb_signal_to_string (sig));
@@ -502,6 +525,7 @@ get_core_register_section (struct regcache *regcache,
   struct bfd_section *section;
   bfd_size_type size;
   char *contents;
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
 
   xfree (section_name);
 
@@ -530,11 +554,12 @@ get_core_register_section (struct regcache *regcache,
       return;
     }
 
-  if (core_gdbarch && gdbarch_regset_from_core_section_p (core_gdbarch))
+  if (cops->core_gdbarch
+      && gdbarch_regset_from_core_section_p (cops->core_gdbarch))
     {
       const struct regset *regset;
 
-      regset = gdbarch_regset_from_core_section (core_gdbarch,
+      regset = gdbarch_regset_from_core_section (cops->core_gdbarch,
 						 name, size);
       if (regset == NULL)
 	{
@@ -548,10 +573,10 @@ 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 (cops->core_vec);
+  cops->core_vec->core_read_registers (regcache, contents, size, which,
+				       ((CORE_ADDR)
+					bfd_section_vma (core_bfd, section)));
 }
 
 
@@ -568,9 +593,12 @@ get_core_registers (struct target_ops *ops,
 {
   struct core_regset_section *sect_list;
   int i;
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
 
-  if (!(core_gdbarch && gdbarch_regset_from_core_section_p (core_gdbarch))
-      && (core_vec == NULL || core_vec->core_read_registers == NULL))
+  if (!(cops->core_gdbarch
+	&& gdbarch_regset_from_core_section_p (cops->core_gdbarch))
+      && (cops->core_vec == NULL
+	  || cops->core_vec->core_read_registers == NULL))
     {
       fprintf_filtered (gdb_stderr,
 		     "Can't fetch registers from this type of core file\n");
@@ -611,7 +639,9 @@ get_core_registers (struct target_ops *ops,
 static void
 core_files_info (struct target_ops *t)
 {
-  print_section_info (core_data, core_bfd);
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
+
+  print_section_info (&cops->core_data, core_bfd);
 }
 \f
 struct spuid_list
@@ -679,13 +709,15 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
 		   const gdb_byte *writebuf, ULONGEST offset,
 		   ULONGEST len, ULONGEST *xfered_len)
 {
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
+
   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,
+						cops->core_data.sections,
+						cops->core_data.sections_end,
 						NULL);
 
     case TARGET_OBJECT_AUXV:
@@ -759,16 +791,17 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
       return TARGET_XFER_E_IO;
 
     case TARGET_OBJECT_LIBRARIES:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_p (core_gdbarch))
+      if (cops->core_gdbarch
+	  && gdbarch_core_xfer_shared_libraries_p (cops->core_gdbarch))
 	{
 	  if (writebuf)
 	    return TARGET_XFER_E_IO;
 	  else
 	    {
-	      *xfered_len = gdbarch_core_xfer_shared_libraries (core_gdbarch,
-								readbuf,
-								offset, len);
+	      *xfered_len
+		= gdbarch_core_xfer_shared_libraries (cops->core_gdbarch,
+						      readbuf,
+						      offset, len);
 
 	      if (*xfered_len == 0)
 		return TARGET_XFER_EOF;
@@ -779,15 +812,15 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
       /* FALL THROUGH */
 
     case TARGET_OBJECT_LIBRARIES_AIX:
-      if (core_gdbarch
-	  && gdbarch_core_xfer_shared_libraries_aix_p (core_gdbarch))
+      if (cops->core_gdbarch
+	  && gdbarch_core_xfer_shared_libraries_aix_p (cops->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 (cops->core_gdbarch,
 							  readbuf, offset,
 							  len);
 
@@ -913,11 +946,14 @@ core_thread_alive (struct target_ops *ops, ptid_t ptid)
 static const struct target_desc *
 core_read_description (struct target_ops *target)
 {
-  if (core_gdbarch && gdbarch_core_read_description_p (core_gdbarch))
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
+
+  if (cops->core_gdbarch
+      && gdbarch_core_read_description_p (cops->core_gdbarch))
     {
       const struct target_desc *result;
 
-      result = gdbarch_core_read_description (core_gdbarch, 
+      result = gdbarch_core_read_description (cops->core_gdbarch, 
 					      target, core_bfd);
       if (result != NULL)
 	return result;
@@ -932,12 +968,13 @@ core_pid_to_str (struct target_ops *ops, ptid_t ptid)
   static char buf[64];
   struct inferior *inf;
   int pid;
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
 
   /* 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 (cops->core_gdbarch
+      && gdbarch_core_pid_to_str_p (cops->core_gdbarch))
+    return gdbarch_core_pid_to_str (cops->core_gdbarch, ptid);
 
   /* Otherwise, if we don't have one, we'll just fallback to
      "process", with normal_pid_to_str.  */
@@ -1000,7 +1037,7 @@ init_core_ops (void)
   core_ops.to_doc =
     "Use a core file as a target.  Specify the filename of the core file.";
   core_ops.to_open = core_open;
-  core_ops.to_close = core_close;
+  core_ops.to_xclose = core_xclose;
   core_ops.to_detach = core_detach;
   core_ops.to_fetch_registers = get_core_registers;
   core_ops.to_xfer_partial = core_xfer_partial;
diff --git a/gdb/target.c b/gdb/target.c
index 6a10c04..6f5396f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -586,6 +586,26 @@ push_target (struct target_ops *t)
   update_current_target ();
 }
 
+/* See target.h.  */
+
+struct target_ops *
+allocate_target (struct target_ops *ops, size_t alloc)
+{
+  char *mem;
+
+  gdb_assert (ops->to_identity != NULL);
+  gdb_assert (ops->to_xclose != NULL);
+  gdb_assert (alloc >= sizeof (struct target_ops));
+
+  mem = xmalloc (alloc);
+  memcpy (mem, ops, sizeof (struct target_ops));
+  if (alloc > sizeof (struct target_ops))
+    memset (&mem[sizeof (struct target_ops)], 0,
+	    alloc - sizeof (struct target_ops));
+
+  return (struct target_ops *) mem;
+}
+
 /* Remove a target_ops vector from the stack, wherever it may be.
    Return how many times it was removed (0 or 1).  */
 
diff --git a/gdb/target.h b/gdb/target.h
index 58c7b30..60dc287 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2091,6 +2091,21 @@ extern void push_target (struct target_ops *);
 
 extern int unpush_target (struct target_ops *);
 
+/* Allocate a new target object.  OPS is the "prototype"
+   implementation of the new target; it is copied into the new object.
+   ALLOC is the number of bytes to allocate.  It may be larger than
+   sizeof (struct target_ops) if the target subclasses target_ops to
+   store extra data.  Returns the new target object.  Extra space
+   beyond the initial OPS component of the result is zeroed.  */
+
+extern struct target_ops *allocate_target (struct target_ops *ops,
+					   size_t alloc);
+
+/* A typed convenience wrapper for allocate_target.  T is the type of
+   the subclass of target_ops to allocate.  */
+
+#define TARGET_NEW(T, OPS) ((T *) (allocate_target ((OPS), sizeof (T))))
+
 extern void target_pre_inferior (int);
 
 extern void target_preopen (int);
-- 
1.9.3

Re: [PATCH 0/4] baby step toward multi-target

[Pedro Alves]

On 07/18/2014 08:27 PM, Tom Tromey wrote:

> If this approach seems reasonable then it's not too hard to pull over
> some of the other target conversions from the branch.

Seems reasonable.  We may or not end up doing things completely
different wrt to the identify issue and target is pushed/not pushed,
but meanwhile this lets us progress with making it possible to have
different instances of a target, so it seems good forward progress.

The series looked good to me.

-- 
Thanks,
Pedro Alves

Re: [PATCH 0/4] baby step toward multi-target

[Tom Tromey]

Tom> If this approach seems reasonable then it's not too hard to pull over
Tom> some of the other target conversions from the branch.

Pedro> Seems reasonable.  We may or not end up doing things completely
Pedro> different wrt to the identify issue and target is pushed/not pushed,
Pedro> but meanwhile this lets us progress with making it possible to have
Pedro> different instances of a target, so it seems good forward progress.

Pedro> The series looked good to me.

Thanks for looking.

I actually went a bit farther down this path on my branch, adding:

    0249cca turn remote into a to_xclose target
    84e2677 turn linux-thread-db into a to_xclose target
    9fdaa64 fix exec_close to be multi-target-aware
    7fd03ff introduce the target stack
    698a56f convert current_target to be a pointer
    68520c8 convert exec target to to_xclose

These are all things pulled over from the mostly-defunct multi-target
branch.

I probably wouldn't be inclined to put these -- or maybe even the bulk
of the "baby step" series -- in right now.  They aren't really that
useful until some other things are finished, like adding the target ID
to ptid.

However, it's probably useful as a starting point for the next attempt
at multi-target.  In particular it's much less invasive than the earlier
approach, and the "convert current_target to be a pointer" patch (the
biggest one) was mostly done with a perl one-liner.

Tom

Re: [PATCH 0/4] baby step toward multi-target

[Pedro Alves]

On 07/29/2014 03:49 PM, Tom Tromey wrote:

> However, it's probably useful as a starting point for the next attempt
> at multi-target.  In particular it's much less invasive than the earlier
> approach, and the "convert current_target to be a pointer" patch (the
> biggest one) was mostly done with a perl one-liner.

Yeah.

Hmm, kind of orthogonal, but not really, I wonder how far are we
to getting rid of the remaining target stack annoyances.  Like:

 #1 - Convert the remaining data fields to methods.  That seems it'll
      just be boring work.
 #2 - making the debug target a wrapping target around each target layer
      (as you suggested elsewhere).  That'll require make-target-delegates
      hacking.

That'd then make it possible to:

 #3 - eliminate the squashed current_target, replacing it with a pointer
      to the topmost target in the stack (mostly the same as your
      perl one-liner).

Is that in line with what you've been thinking?  I'm wondering whether
you found out in the more invasive branch that it's useful if
current_target is more than a struct target_ops pointer ?
(or "struct target *" after vtable-ification.)

Thanks,
Pedro Alves

Re: [PATCH 0/4] baby step toward multi-target

[Tom Tromey]

Tom> However, it's probably useful as a starting point for the next attempt
Tom> at multi-target.  In particular it's much less invasive than the earlier
Tom> approach, and the "convert current_target to be a pointer" patch (the
Tom> biggest one) was mostly done with a perl one-liner.

Pedro> Yeah.

Pedro> Hmm, kind of orthogonal, but not really, I wonder how far are we
Pedro> to getting rid of the remaining target stack annoyances.  Like:

Pedro>  #1 - Convert the remaining data fields to methods.  That seems it'll
Pedro>       just be boring work.

Agreed.

Pedro>  #2 - making the debug target a wrapping target around each
Pedro>  target layer (as you suggested elsewhere).  That'll require
Pedro>  make-target-delegates hacking.

And a bit of hacking in the same spots touched by the "identity" patch
in this series.

It doesn't seem very hard.

Pedro> That'd then make it possible to:

Pedro>  #3 - eliminate the squashed current_target, replacing it with a pointer
Pedro>       to the topmost target in the stack (mostly the same as your
Pedro>       perl one-liner).

Pedro> Is that in line with what you've been thinking?  I'm wondering whether
Pedro> you found out in the more invasive branch that it's useful if
Pedro> current_target is more than a struct target_ops pointer ?
Pedro> (or "struct target *" after vtable-ification.)

I think it's reasonable to try to get there.
Certainly at this point the squashed target is not extremely useful.
You can see this by looking at how few uses of INHERIT remain.

This would also let us zap the uses of "current_target.beneath" that we
discussed yesterday.


The more invasive branch took a different approach to target identity.
Rather than the to_identity field, it split target_ops into a const
vtable part and a payload part.  Then it aimed to require all targets to
instantiate a new object.

This obviously has to touch a lot of code.  And, it runs into some
difficulties where the target code must work with uninstantiated
targets.

I don't think this is as much of an issue on the new branch.
At least from what I recall the uninstantiated business isn't a big
issue because this only affects a subset of methods, which can simply
avoid looking at the payload.  Also perhaps the target delegation
changes cleaned things up a bit too, I forget.

On the new branch, having current_target be a pointer is preferable just
because it makes target-swapping cheap -- it just means redirecting two
pointers (it could be one pointer, but I chose to keep current_target a
global, always pointing into the current target_stack object; this means
the target stack can be opaque).

I didn't consider making current_target have some other type.

Tom

Re: [PATCH 0/4] baby step toward multi-target

[Doug Evans]

On Tue, Jul 29, 2014 at 10:45 AM, Tom Tromey <tromey@redhat.com> wrote:
> The more invasive branch took a different approach to target identity.
> Rather than the to_identity field, it split target_ops into a const
> vtable part and a payload part.  Then it aimed to require all targets to
> instantiate a new object.

Hi.  I'd like to understand this more.
By way of analogy, are you talking about something akin to RTTI, or
something else?
[How would the problem of target identity be solved if one went with
the "all targets instantiate a new object" approach?]

> This obviously has to touch a lot of code.  And, it runs into some
> difficulties where the target code must work with uninstantiated
> targets.

By way of analogy, is all such code akin to static methods?

> I don't think this is as much of an issue on the new branch.
> At least from what I recall the uninstantiated business isn't a big
> issue because this only affects a subset of methods, which can simply
> avoid looking at the payload.

Avoid looking at the payload how?
Do you mean said "methods" would get passed an argument they are
explicitly not supposed to touch?

Re: [PATCH 0/4] baby step toward multi-target

[Doug Evans]

Doug Evans <dje@google.com> writes:
> On Tue, Jul 29, 2014 at 10:45 AM, Tom Tromey <tromey@redhat.com> wrote:
>> The more invasive branch took a different approach to target identity.
>> Rather than the to_identity field, it split target_ops into a const
>> vtable part and a payload part.  Then it aimed to require all targets to
>> instantiate a new object.
>
> Hi.  I'd like to understand this more.
> By way of analogy, are you talking about something akin to RTTI, or
> something else?
> [How would the problem of target identity be solved if one went with
> the "all targets instantiate a new object" approach?]
>
>> This obviously has to touch a lot of code.  And, it runs into some
>> difficulties where the target code must work with uninstantiated
>> targets.
>
> By way of analogy, is all such code akin to static methods?
>
>> I don't think this is as much of an issue on the new branch.
>> At least from what I recall the uninstantiated business isn't a big
>> issue because this only affects a subset of methods, which can simply
>> avoid looking at the payload.
>
> Avoid looking at the payload how?
> Do you mean said "methods" would get passed an argument they are
> explicitly not supposed to touch?

Regarding avoiding looking at the payload ...

I noticed this in 4/4.  I think(!) it's unrelated, though I'm still
unclear on what you're referring to.  At any rate, the following is odd.

Several "methods" take a "self" or "ops" pointer but then don't use it
and instead go search the target stack for what I'd expect to be the
same thing.

E.g.,

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 1775a66..212665d 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -611,7 +639,9 @@ get_core_registers (struct target_ops *ops,
 static void
 core_files_info (struct target_ops *t)
 {
-  print_section_info (core_data, core_bfd);
+  struct core_target_ops_with_data *cops = get_core_target_ops ();
+
+  print_section_info (&cops->core_data, core_bfd);
 }
 \f
 struct spuid_list

The intuitive thing to do here is assert t is a core ops,
and then use it.  Instead the code calls get_core_target_ops.
Why?

I realize you mentioned in another email that you probably wouldn't
be inclined to check this in.  But if corelow is to be the example
on which other targets are based I'd like to understand all the
oddities I find.  [Maybe the above is just oversight, in which case
no worries.  But gdb's target support isn't always intuitive, and
I don't always trust my intuition with it.]

Re: [PATCH 0/4] baby step toward multi-target

[Doug Evans]

On Tue, Jul 29, 2014 at 7:23 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/18/2014 08:27 PM, Tom Tromey wrote:
>
>> If this approach seems reasonable then it's not too hard to pull over
>> some of the other target conversions from the branch.
>
> Seems reasonable.  We may or not end up doing things completely
> different wrt to the identify issue and target is pushed/not pushed,
> but meanwhile this lets us progress with making it possible to have
> different instances of a target, so it seems good forward progress.
>
> The series looked good to me.

I'd like to review this for a bit more.

I'd like to understand the tradeoffs of this more:

>>This little series, on the other hand, takes a less invasive approach.
>>The idea here is that rather than doing a huge target_ops split,
>>instead just sometimes make copies of the target_ops when pushing.
>>This lets the copies keep their own state; copies are needed in the
>>long run because multiple target stacks will be active and a given
>>target_ops only has one "beneath" pointer.

Thanks.

Re: [PATCH 0/4] baby step toward multi-target

[Doug Evans]

Tom Tromey <tromey@redhat.com> writes:
> My multi-target branch is basically dead, I think, mostly due to heavy
> conflicts with all the other target changes that have happened in the
> meantime.

This is an important bit of functionality that needs to be done.
Since we just released 7.8, and in order to prevent such things
happening again, I think it would be reasonable to put more
resources into getting this done, and delay other things for awhile.
[Assuming various factors like it won't take an inordinate amount
of time, and the end result is the more preferred one, and probably
a few others :-).]

$0.02

> However recently I realized that there is a less invasive
> way to achieve many of the same effects, and a way to salvage some of
> the smaller cleanups done there.
>
> In particular on the branch I attempted to split target_ops into a
> vtable-like object and a target state object.  As you might imagine
> this patch touches a large amount of code and is hard both to keep up
> to date, and to test.

That was my understanding of where we were headed.

> This little series, on the other hand, takes a less invasive approach.
> The idea here is that rather than doing a huge target_ops split,
> instead just sometimes make copies of the target_ops when pushing.
> This lets the copies keep their own state; copies are needed in the
> long run because multiple target stacks will be active and a given
> target_ops only has one "beneath" pointer.

Which version of the long run?
Wouldn't dynamic properties of the object be kept in the object
and not the vtable?

Or is some custom dynamic interitance being done here?
I can imagine that being useful to facilitate inserting
debug targets into the mix.

By way of analogy, I'd be curious how this would be done in c++.

[Just trying to understand the situation better.]

> Here corelow is used as the example of how to do a conversion.
> There's a bit of cleanup and infrastructure initially, and then the
> final patch moves the corelow global state into a new subclass of
> target_ops, which is instantiated and pushed.
>
> If this approach seems reasonable then it's not too hard to pull over
> some of the other target conversions from the branch.
>
> This series also shows the spots to change if you want to make the
> debug target wrap each stratum in the target stack -- each wrapping
> debug target would be to_xclose-ish, and spots like target_is_pushed
> would be taught to unwrap.
>
> Built and regtested on x86-64 Fedora 20.