diff mbox

[RFC,v5] fix regressions with target-async

Message ID 1394642546-10555-1-git-send-email-tromey@redhat.com
State Committed
Headers show

Commit Message

Tom Tromey March 12, 2014, 4:42 p.m. UTC
A patch in the target cleanup series caused a regression when using
record with target-async.  Version 4 of the patch is here:

    https://sourceware.org/ml/gdb-patches/2014-03/msg00159.html

The immediate problem is that record supplies to_can_async_p and
to_is_async_p methods, but does not supply a to_async method.  So,
when target-async is set, record claims to support async -- but if the
underlying target does not support async, then the to_async method
call will end up in that method's default implementation, namely
tcomplain.

This worked previously because the record target used to provide a
to_async method; one that (erroneously, only at push time) checked the
other members of the target stack, and then simply dropped to_async
calls in the "does not implement async" case.

My first thought was to simply drop tcomplain as the default for
to_async.  This works, but Pedro pointed out that the only reason
record has to supply to_can_async_p and to_is_async_p is that these
default to using the find_default_run_target machinery -- and these
defaults are only needed by "run" and "attach".

So, a nicer solution presents itself: change run and attach to
explicitly call into the default run target when needed; and change
to_is_async_p and to_can_async_p to default to "return 0".  This makes
the target stack simpler to use and lets us remove the method
implementations from record.  This is also in harmony with other plans
for the target stack; namely trying to reduce the impact of
find_default_run_target.  This approach makes it clear that
find_default_is_async_p is not needed -- it is asking whether a target
that may not even be pushed is actually async, which seems like a
nonsensical question.

While an improvement, this approach proved to introduce the same bug
when using the core target.  Looking a bit deeper, the issue is that
code in "attach" and "run" may need to use either the current target
stack or the default run target -- but different calls into the target
API in those functions could wind up querying different targets.

This new patch makes the target to use more explicit in "run" and
"attach".  Then these commands explicitly make the needed calls
against that target.  This ensures that a single target is used for
all relevant operations.  This lets us remove a couple find_default_*
functions from various targets, including the dummy target.  I think
this is a decent understandability improvement.

One issue I see with this patch is that the new calls in "run" and
"attach" are not very much like the rest of the target API.  I think
fundamentally this is due to bad factoring in the target API, which
may need to be fixed for multi-target.  Tackling that seemed ambitious
for a regression fix.

While working on this I noticed that there don't seem to be any test
cases that involve both target-async and record, so this patch changes
break-precsave.exp to add some.  It also changes corefile.exp to add
some target-async tests; these pass with current trunk and with this
patch applied, but fail with the v1 patch.

This patch differs from v4 in that it moves initialization of
to_can_async_p and to_supports_non_stop into inf-child, adds some
assertions to complete_target_initialization, and adds some comments
to target.h.

Built and regtested on x86-64 Fedora 20.

2014-03-12  Tom Tromey  <tromey@redhat.com>

	* inf-child.c (return_zero): New function.
	(inf_child_target): Set to_can_async_p, to_supports_non_stop.
	* aix-thread.c (aix_thread_inferior_created): New function.
	(aix_thread_attach): Remove.
	(init_aix_thread_ops): Don't set to_attach.
	(_initialize_aix_thread): Register inferior_created observer.
	* corelow.c (init_core_ops): Don't set to_attach or
	to_create_inferior.
	* exec.c (init_exec_ops): Don't set to_attach or
	to_create_inferior.
	* infcmd.c (run_command_1): Use find_run_target.  Make direct
	target calls.
	(attach_command): Use find_attach_target.  Make direct target
	calls.
	* record-btrace.c (init_record_btrace_ops): Don't set
	to_create_inferior.
	* record-full.c (record_full_can_async_p, record_full_is_async_p):
	Remove.
	(init_record_full_ops, init_record_full_core_ops): Update.  Don't
	set to_create_inferior.
	* target.c (complete_target_initialization): Add assertion.
	(target_create_inferior): Remove.
	(find_default_attach, find_default_create_inferior): Remove.
	(find_attach_target, find_run_target): New functions.
	(find_default_is_async_p, find_default_can_async_p)
	(target_supports_non_stop, target_attach): Remove.
	(init_dummy_target): Don't set to_create_inferior or
	to_supports_non_stop.
	* target.h (struct target_ops) <to_attach>: Add comment.  Remove
	TARGET_DEFAULT_FUNC.
	<to_create_inferior>: Add comment.
	<to_can_async_p, to_is_async_p, to_supports_non_stop>: Use
	TARGET_DEFAULT_RETURN.
	<to_can_async_p, to_supports_non_stop, to_can_run>: Add comments.
	(find_attach_target, find_run_target): Declare.
	(target_create_inferior): Remove.
	(target_has_execution_1): Update comment.
	(target_supports_non_stop): Remove.
	* target-delegates.c: Rebuild.

2014-03-10  Tom Tromey  <tromey@redhat.com>

	* gdb.base/corefile.exp (corefile_test_run, corefile_test_attach):
	New procs.  Add target-async tests.
	* gdb.reverse/break-precsave.exp (precsave_tests): New proc.
	Add target-async tests.
---
 gdb/ChangeLog                                |  42 +++++++++
 gdb/aix-thread.c                             |  14 ++-
 gdb/corelow.c                                |   2 -
 gdb/exec.c                                   |   2 -
 gdb/inf-child.c                              |  13 +++
 gdb/infcmd.c                                 |  34 ++++---
 gdb/record-btrace.c                          |   1 -
 gdb/record-full.c                            |  19 ----
 gdb/target-delegates.c                       |  42 ++++++---
 gdb/target.c                                 | 129 +++++++--------------------
 gdb/target.h                                 |  64 +++++++------
 gdb/testsuite/ChangeLog                      |   7 ++
 gdb/testsuite/gdb.base/corefile.exp          | 115 +++++++++++++++---------
 gdb/testsuite/gdb.reverse/break-precsave.exp | 115 +++++++++++++-----------
 14 files changed, 325 insertions(+), 274 deletions(-)

Comments

Pedro Alves March 12, 2014, 5:18 p.m. UTC | #1
This is OK, please push it in.

On 03/12/2014 04:42 PM, Tom Tromey wrote:

> While working on this I noticed that there don't seem to be any test
> cases that involve both target-async and record, so this patch changes
> break-precsave.exp to add some.  It also changes corefile.exp to add
> some target-async tests; these pass with current trunk and with this
> patch applied, but fail with the v1 patch.

I was looking at the async-by-default series, and pondering
this --- after the "target async by default" series,
"set target-async" is really an MI setting, so it'll be irrelevant
and really useless for these tests.  We'll have an alternative "maint
set target-async off/on", but I don't think we should use it to
force specific tests to run in sync mode -- rather my idea for it
was to it from a board file for the occasional comfortable testing and
emulation of !Linux targets.

So in the end, after the target-async series, I think we should go
through non-MI tests and remove all that do "set target-async on".
Tom Tromey March 12, 2014, 7:04 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This is OK, please push it in.

Thanks.

Pedro> So in the end, after the target-async series, I think we should go
Pedro> through non-MI tests and remove all that do "set target-async on".

That approach is fine with me too.  Then you can remove the odd linkage
between the "maint" and target-async settings.

Tom
Pedro Alves March 12, 2014, 7:27 p.m. UTC | #3
On 03/12/2014 07:04 PM, Tom Tromey wrote:
> 
> That approach is fine with me too.  Then you can remove the odd linkage
> between the "maint" and target-async settings.

I would like remove that also for another reason:

The idea of "maint set target-async off" is to disable
_target_ async support, to emulate, say sync Windows testing,
on Linux.

Currently, on a non-async-capable target, "set target-async on"
itself does not error, but subsequent (CLI or MI) execution commands
fail with "Asynchronous execution not supported on this target.".

After the async-by-default series, "set target-async" becomes
exclusively an MI command.  After "set target-async on",
MI execution commands are background execution commands.

Therefore, the combination:

 "maint set target-async off" + "set target-async on"

should also result in the same exact error as today
you get when you try "set target-async on" against a
target that doesn't support async.

The linkage in v5 means that "set target-async on"
in the testsuite re-enables "maint set target-async on",
which defeats the purpose of the maint setting.

(btw, I'm considering adding "set mi-async", and making
"set target-async" a deprecated alias to that, for clarity)
Tom Tromey March 12, 2014, 7:45 p.m. UTC | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Therefore, the combination:
Pedro>  "maint set target-async off" + "set target-async on"
Pedro> should also result in the same exact error as today
Pedro> you get when you try "set target-async on" against a
Pedro> target that doesn't support async.

Yeah, that approach makes sense to me.

Pedro> (btw, I'm considering adding "set mi-async", and making
Pedro> "set target-async" a deprecated alias to that, for clarity)

Sounds reasonable.

Tom
diff mbox

Patch

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 444a025..3865cba 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -959,12 +959,9 @@  new_objfile (struct objfile *objfile)
 /* Attach to process specified by ARGS.  */
 
 static void
-aix_thread_attach (struct target_ops *ops, char *args, int from_tty)
+aix_thread_inferior_created (struct target_ops *ops, int from_tty)
 {
-  struct target_ops *beneath = find_target_beneath (ops);
-  
-  beneath->to_attach (beneath, args, from_tty);
-  pd_activate (1);
+  pd_enable ();
 }
 
 /* Detach from the process attached to by aix_thread_attach().  */
@@ -1823,15 +1820,12 @@  init_aix_thread_ops (void)
   aix_thread_ops.to_longname = _("AIX pthread support");
   aix_thread_ops.to_doc = _("AIX pthread support");
 
-  aix_thread_ops.to_attach = aix_thread_attach;
   aix_thread_ops.to_detach = aix_thread_detach;
   aix_thread_ops.to_resume = aix_thread_resume;
   aix_thread_ops.to_wait = aix_thread_wait;
   aix_thread_ops.to_fetch_registers = aix_thread_fetch_registers;
   aix_thread_ops.to_store_registers = aix_thread_store_registers;
   aix_thread_ops.to_xfer_partial = aix_thread_xfer_partial;
-  /* No need for aix_thread_ops.to_create_inferior, because we activate thread
-     debugging when the inferior reaches pd_brk_addr.  */
   aix_thread_ops.to_mourn_inferior = aix_thread_mourn_inferior;
   aix_thread_ops.to_thread_alive = aix_thread_thread_alive;
   aix_thread_ops.to_pid_to_str = aix_thread_pid_to_str;
@@ -1855,6 +1849,10 @@  _initialize_aix_thread (void)
   /* Notice when object files get loaded and unloaded.  */
   observer_attach_new_objfile (new_objfile);
 
+  /* Add ourselves to inferior_created event chain.
+     This is needed to enable the thread target on "attach".  */
+  observer_attach_inferior_created (aix_thread_inferior_created);
+
   add_setshow_boolean_cmd ("aix-thread", class_maintenance, &debug_aix_thread,
 			   _("Set debugging of AIX thread module."),
 			   _("Show debugging of AIX thread module."),
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 05c3e4e..02caeef 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -999,14 +999,12 @@  init_core_ops (void)
     "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_attach = find_default_attach;
   core_ops.to_detach = core_detach;
   core_ops.to_fetch_registers = get_core_registers;
   core_ops.to_xfer_partial = core_xfer_partial;
   core_ops.to_files_info = core_files_info;
   core_ops.to_insert_breakpoint = ignore;
   core_ops.to_remove_breakpoint = ignore;
-  core_ops.to_create_inferior = find_default_create_inferior;
   core_ops.to_thread_alive = core_thread_alive;
   core_ops.to_read_description = core_read_description;
   core_ops.to_pid_to_str = core_pid_to_str;
diff --git a/gdb/exec.c b/gdb/exec.c
index 908858e..ca59c72 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -959,13 +959,11 @@  init_exec_ops (void)
 Specify the filename of the executable file.";
   exec_ops.to_open = exec_open;
   exec_ops.to_close = exec_close_1;
-  exec_ops.to_attach = find_default_attach;
   exec_ops.to_xfer_partial = exec_xfer_partial;
   exec_ops.to_get_section_table = exec_get_section_table;
   exec_ops.to_files_info = exec_files_info;
   exec_ops.to_insert_breakpoint = ignore;
   exec_ops.to_remove_breakpoint = ignore;
-  exec_ops.to_create_inferior = find_default_create_inferior;
   exec_ops.to_stratum = file_stratum;
   exec_ops.to_has_memory = exec_has_memory;
   exec_ops.to_make_corefile_notes = exec_make_note_section;
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index ee63dd1..c6483f9 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -392,6 +392,15 @@  inf_child_can_use_agent (struct target_ops *self)
   return agent_loaded_p ();
 }
 
+/* Default implementation of the to_can_async_p and
+   to_supports_non_stop methods.  */
+
+static int
+return_zero (struct target_ops *ignore)
+{
+  return 0;
+}
+
 struct target_ops *
 inf_child_target (void)
 {
@@ -416,6 +425,10 @@  inf_child_target (void)
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
   t->to_follow_fork = inf_child_follow_fork;
   t->to_can_run = inf_child_can_run;
+  /* We must default these because they must be implemented by any
+     target that can run.  */
+  t->to_can_async_p = return_zero;
+  t->to_supports_non_stop = return_zero;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
   t->to_has_all_memory = default_child_has_all_memory;
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c59d0be..75d37fd 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -503,6 +503,7 @@  run_command_1 (char *args, int from_tty, int tbreak_at_main)
   struct cleanup *old_chain;
   ptid_t ptid;
   struct ui_out *uiout = current_uiout;
+  struct target_ops *run_target;
 
   dont_repeat ();
 
@@ -531,7 +532,9 @@  run_command_1 (char *args, int from_tty, int tbreak_at_main)
 
   exec_file = (char *) get_exec_file (0);
 
-  if (non_stop && !target_supports_non_stop ())
+  run_target = find_run_target ();
+
+  if (non_stop && !run_target->to_supports_non_stop (run_target))
     error (_("The target does not support running in non-stop mode."));
 
   /* We keep symbols from add-symbol-file, on the grounds that the
@@ -544,7 +547,7 @@  run_command_1 (char *args, int from_tty, int tbreak_at_main)
 
   if (!args)
     {
-      if (target_can_async_p ())
+      if (run_target->to_can_async_p (run_target))
 	async_disable_stdin ();
     }
   else
@@ -553,12 +556,12 @@  run_command_1 (char *args, int from_tty, int tbreak_at_main)
 
       /* If we get a request for running in the bg but the target
          doesn't support it, error out.  */
-      if (async_exec && !target_can_async_p ())
+      if (async_exec && !run_target->to_can_async_p (run_target))
 	error (_("Asynchronous execution not supported on this target."));
 
       /* If we don't get a request of running in the bg, then we need
          to simulate synchronous (fg) execution.  */
-      if (!async_exec && target_can_async_p ())
+      if (!async_exec && run_target->to_can_async_p (run_target))
 	{
 	  /* Simulate synchronous execution.  */
 	  async_disable_stdin ();
@@ -585,9 +588,12 @@  run_command_1 (char *args, int from_tty, int tbreak_at_main)
 
   /* We call get_inferior_args() because we might need to compute
      the value now.  */
-  target_create_inferior (exec_file, get_inferior_args (),
-			  environ_vector (current_inferior ()->environment),
-			  from_tty);
+  run_target->to_create_inferior (run_target, exec_file, get_inferior_args (),
+				  environ_vector (current_inferior ()->environment),
+				  from_tty);
+  /* to_create_inferior should push the target, so after this point we
+     shouldn't refer to run_target again.  */
+  run_target = NULL;
 
   /* We're starting off a new process.  When we get out of here, in
      non-stop mode, finish the state of all threads of that process,
@@ -2515,6 +2521,7 @@  attach_command (char *args, int from_tty)
 {
   int async_exec = 0;
   struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
+  struct target_ops *attach_target;
 
   dont_repeat ();		/* Not for the faint of heart */
 
@@ -2534,7 +2541,9 @@  attach_command (char *args, int from_tty)
      this function should probably be moved into target_pre_inferior.  */
   target_pre_inferior (from_tty);
 
-  if (non_stop && !target_supports_non_stop ())
+  attach_target = find_attach_target ();
+
+  if (non_stop && !attach_target->to_supports_non_stop (attach_target))
     error (_("Cannot attach to this target in non-stop mode"));
 
   if (args)
@@ -2543,20 +2552,23 @@  attach_command (char *args, int from_tty)
 
       /* If we get a request for running in the bg but the target
          doesn't support it, error out.  */
-      if (async_exec && !target_can_async_p ())
+      if (async_exec && !attach_target->to_can_async_p (attach_target))
 	error (_("Asynchronous execution not supported on this target."));
     }
 
   /* If we don't get a request of running in the bg, then we need
      to simulate synchronous (fg) execution.  */
-  if (!async_exec && target_can_async_p ())
+  if (!async_exec && attach_target->to_can_async_p (attach_target))
     {
       /* Simulate synchronous execution.  */
       async_disable_stdin ();
       make_cleanup ((make_cleanup_ftype *)async_enable_stdin, NULL);
     }
 
-  target_attach (args, from_tty);
+  attach_target->to_attach (attach_target, args, from_tty);
+  /* to_attach should push the target, so after this point we
+     shouldn't refer to attach_target again.  */
+  attach_target = NULL;
 
   /* Set up the "saved terminal modes" of the inferior
      based on what modes we are starting it with.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d4c0d42..bcac165 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1893,7 +1893,6 @@  init_record_btrace_ops (void)
   ops->to_disconnect = record_disconnect;
   ops->to_mourn_inferior = record_mourn_inferior;
   ops->to_kill = record_kill;
-  ops->to_create_inferior = find_default_create_inferior;
   ops->to_stop_recording = record_btrace_stop_recording;
   ops->to_info_record = record_btrace_info;
   ops->to_insn_history = record_btrace_insn_history;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index d35165b..39db910 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1750,20 +1750,6 @@  record_full_goto_bookmark (struct target_ops *self,
   return;
 }
 
-static int
-record_full_can_async_p (struct target_ops *ops)
-{
-  /* We only enable async when the user specifically asks for it.  */
-  return target_async_permitted;
-}
-
-static int
-record_full_is_async_p (struct target_ops *ops)
-{
-  /* We only enable async when the user specifically asks for it.  */
-  return target_async_permitted;
-}
-
 static enum exec_direction_kind
 record_full_execution_direction (struct target_ops *self)
 {
@@ -1916,7 +1902,6 @@  init_record_full_ops (void)
   record_full_ops.to_detach = record_detach;
   record_full_ops.to_mourn_inferior = record_mourn_inferior;
   record_full_ops.to_kill = record_kill;
-  record_full_ops.to_create_inferior = find_default_create_inferior;
   record_full_ops.to_store_registers = record_full_store_registers;
   record_full_ops.to_xfer_partial = record_full_xfer_partial;
   record_full_ops.to_insert_breakpoint = record_full_insert_breakpoint;
@@ -1928,8 +1913,6 @@  init_record_full_ops (void)
   /* Add bookmark target methods.  */
   record_full_ops.to_get_bookmark = record_full_get_bookmark;
   record_full_ops.to_goto_bookmark = record_full_goto_bookmark;
-  record_full_ops.to_can_async_p = record_full_can_async_p;
-  record_full_ops.to_is_async_p = record_full_is_async_p;
   record_full_ops.to_execution_direction = record_full_execution_direction;
   record_full_ops.to_info_record = record_full_info;
   record_full_ops.to_save_record = record_full_save;
@@ -2174,8 +2157,6 @@  init_record_full_core_ops (void)
   /* Add bookmark target methods.  */
   record_full_core_ops.to_get_bookmark = record_full_get_bookmark;
   record_full_core_ops.to_goto_bookmark = record_full_goto_bookmark;
-  record_full_core_ops.to_can_async_p = record_full_can_async_p;
-  record_full_core_ops.to_is_async_p = record_full_is_async_p;
   record_full_core_ops.to_execution_direction
     = record_full_execution_direction;
   record_full_core_ops.to_info_record = record_full_info;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 3ca24b7..87ce0d1 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -4,13 +4,6 @@ 
 /* To regenerate this file, run:*/
 /*      make-target-delegates target.h > target-delegates.c */
 static void
-delegate_attach (struct target_ops *self, char *arg1, int arg2)
-{
-  self = self->beneath;
-  self->to_attach (self, arg1, arg2);
-}
-
-static void
 delegate_post_attach (struct target_ops *self, int arg1)
 {
   self = self->beneath;
@@ -690,12 +683,24 @@  delegate_can_async_p (struct target_ops *self)
 }
 
 static int
+tdefault_can_async_p (struct target_ops *self)
+{
+  return 0;
+}
+
+static int
 delegate_is_async_p (struct target_ops *self)
 {
   self = self->beneath;
   return self->to_is_async_p (self);
 }
 
+static int
+tdefault_is_async_p (struct target_ops *self)
+{
+  return 0;
+}
+
 static void
 delegate_async (struct target_ops *self, async_callback_ftype *arg1, void *arg2)
 {
@@ -710,6 +715,19 @@  tdefault_async (struct target_ops *self, async_callback_ftype *arg1, void *arg2)
 }
 
 static int
+delegate_supports_non_stop (struct target_ops *self)
+{
+  self = self->beneath;
+  return self->to_supports_non_stop (self);
+}
+
+static int
+tdefault_supports_non_stop (struct target_ops *self)
+{
+  return 0;
+}
+
+static int
 delegate_find_memory_regions (struct target_ops *self, find_memory_region_ftype arg1, void *arg2)
 {
   self = self->beneath;
@@ -1608,8 +1626,6 @@  delegate_decr_pc_after_break (struct target_ops *self, struct gdbarch *arg1)
 static void
 install_delegators (struct target_ops *ops)
 {
-  if (ops->to_attach == NULL)
-    ops->to_attach = delegate_attach;
   if (ops->to_post_attach == NULL)
     ops->to_post_attach = delegate_post_attach;
   if (ops->to_detach == NULL)
@@ -1730,6 +1746,8 @@  install_delegators (struct target_ops *ops)
     ops->to_is_async_p = delegate_is_async_p;
   if (ops->to_async == NULL)
     ops->to_async = delegate_async;
+  if (ops->to_supports_non_stop == NULL)
+    ops->to_supports_non_stop = delegate_supports_non_stop;
   if (ops->to_find_memory_regions == NULL)
     ops->to_find_memory_regions = delegate_find_memory_regions;
   if (ops->to_make_corefile_notes == NULL)
@@ -1881,7 +1899,6 @@  install_delegators (struct target_ops *ops)
 static void
 install_dummy_methods (struct target_ops *ops)
 {
-  ops->to_attach = find_default_attach;
   ops->to_post_attach = tdefault_post_attach;
   ops->to_detach = tdefault_detach;
   ops->to_disconnect = tdefault_disconnect;
@@ -1939,9 +1956,10 @@  install_dummy_methods (struct target_ops *ops)
   ops->to_pid_to_exec_file = tdefault_pid_to_exec_file;
   ops->to_log_command = tdefault_log_command;
   ops->to_get_section_table = tdefault_get_section_table;
-  ops->to_can_async_p = find_default_can_async_p;
-  ops->to_is_async_p = find_default_is_async_p;
+  ops->to_can_async_p = tdefault_can_async_p;
+  ops->to_is_async_p = tdefault_is_async_p;
   ops->to_async = tdefault_async;
+  ops->to_supports_non_stop = tdefault_supports_non_stop;
   ops->to_find_memory_regions = dummy_find_memory_regions;
   ops->to_make_corefile_notes = dummy_make_corefile_notes;
   ops->to_get_bookmark = tdefault_get_bookmark;
diff --git a/gdb/target.c b/gdb/target.c
index f7868c0..0d22297 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -95,10 +95,6 @@  static char *dummy_make_corefile_notes (struct target_ops *self,
 
 static char *default_pid_to_str (struct target_ops *ops, ptid_t ptid);
 
-static int find_default_can_async_p (struct target_ops *ignore);
-
-static int find_default_is_async_p (struct target_ops *ignore);
-
 static enum exec_direction_kind default_execution_direction
     (struct target_ops *self);
 
@@ -387,6 +383,12 @@  complete_target_initialization (struct target_ops *t)
   if (t->to_has_execution == NULL)
     t->to_has_execution = return_zero_has_execution;
 
+  /* These methods can be called on an unpushed target and so require
+     a default implementation if the target might plausibly be the
+     default run target.  */
+  gdb_assert (t->to_can_run == NULL || (t->to_can_async_p != NULL
+					&& t->to_supports_non_stop != NULL));
+
   install_delegators (t);
 }
 
@@ -473,29 +475,6 @@  target_load (char *arg, int from_tty)
 }
 
 void
-target_create_inferior (char *exec_file, char *args,
-			char **env, int from_tty)
-{
-  struct target_ops *t;
-
-  for (t = current_target.beneath; t != NULL; t = t->beneath)
-    {
-      if (t->to_create_inferior != NULL)	
-	{
-	  t->to_create_inferior (t, exec_file, args, env, from_tty);
-	  if (targetdebug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"target_create_inferior (%s, %s, xxx, %d)\n",
-				exec_file, args, from_tty);
-	  return;
-	}
-    }
-
-  internal_error (__FILE__, __LINE__,
-		  _("could not find a target to create inferior"));
-}
-
-void
 target_terminal_inferior (void)
 {
   /* A background resume (``run&'') should leave GDB in control of the
@@ -2632,79 +2611,46 @@  find_default_run_target (char *do_mesg)
   return runable;
 }
 
-void
-find_default_attach (struct target_ops *ops, char *args, int from_tty)
-{
-  struct target_ops *t;
-
-  t = find_default_run_target ("attach");
-  (t->to_attach) (t, args, from_tty);
-  return;
-}
+/* See target.h.  */
 
-void
-find_default_create_inferior (struct target_ops *ops,
-			      char *exec_file, char *allargs, char **env,
-			      int from_tty)
+struct target_ops *
+find_attach_target (void)
 {
   struct target_ops *t;
 
-  t = find_default_run_target ("run");
-  (t->to_create_inferior) (t, exec_file, allargs, env, from_tty);
-  return;
-}
+  /* If a target on the current stack can attach, use it.  */
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    {
+      if (t->to_attach != NULL)
+	break;
+    }
 
-static int
-find_default_can_async_p (struct target_ops *ignore)
-{
-  struct target_ops *t;
+  /* Otherwise, use the default run target for attaching.  */
+  if (t == NULL)
+    t = find_default_run_target ("attach");
 
-  /* This may be called before the target is pushed on the stack;
-     look for the default process stratum.  If there's none, gdb isn't
-     configured with a native debugger, and target remote isn't
-     connected yet.  */
-  t = find_default_run_target (NULL);
-  if (t && t->to_can_async_p != delegate_can_async_p)
-    return (t->to_can_async_p) (t);
-  return 0;
+  return t;
 }
 
-static int
-find_default_is_async_p (struct target_ops *ignore)
-{
-  struct target_ops *t;
-
-  /* This may be called before the target is pushed on the stack;
-     look for the default process stratum.  If there's none, gdb isn't
-     configured with a native debugger, and target remote isn't
-     connected yet.  */
-  t = find_default_run_target (NULL);
-  if (t && t->to_is_async_p != delegate_is_async_p)
-    return (t->to_is_async_p) (t);
-  return 0;
-}
+/* See target.h.  */
 
-static int
-find_default_supports_non_stop (struct target_ops *self)
+struct target_ops *
+find_run_target (void)
 {
   struct target_ops *t;
 
-  t = find_default_run_target (NULL);
-  if (t && t->to_supports_non_stop)
-    return (t->to_supports_non_stop) (t);
-  return 0;
-}
-
-int
-target_supports_non_stop (void)
-{
-  struct target_ops *t;
+  /* If a target on the current stack can attach, use it.  */
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    {
+      if (t->to_create_inferior != NULL)
+	break;
+    }
 
-  for (t = &current_target; t != NULL; t = t->beneath)
-    if (t->to_supports_non_stop)
-      return t->to_supports_non_stop (t);
+  /* Otherwise, use the default run target.  */
+  if (t == NULL)
+    t = find_default_run_target ("run");
 
-  return 0;
+  return t;
 }
 
 /* Implement the "info proc" command.  */
@@ -3256,8 +3202,6 @@  init_dummy_target (void)
   dummy_target.to_shortname = "None";
   dummy_target.to_longname = "None";
   dummy_target.to_doc = "";
-  dummy_target.to_create_inferior = find_default_create_inferior;
-  dummy_target.to_supports_non_stop = find_default_supports_non_stop;
   dummy_target.to_supports_disable_randomization
     = find_default_supports_disable_randomization;
   dummy_target.to_stratum = dummy_stratum;
@@ -3293,15 +3237,6 @@  target_close (struct target_ops *targ)
     fprintf_unfiltered (gdb_stdlog, "target_close ()\n");
 }
 
-void
-target_attach (char *args, int from_tty)
-{
-  current_target.to_attach (&current_target, args, from_tty);
-  if (targetdebug)
-    fprintf_unfiltered (gdb_stdlog, "target_attach (%s, %d)\n",
-			args, from_tty);
-}
-
 int
 target_thread_alive (ptid_t ptid)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 0d4e5ff..d7c6c3d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -401,8 +401,15 @@  struct target_ops
        to xfree everything (including the "struct target_ops").  */
     void (*to_xclose) (struct target_ops *targ);
     void (*to_close) (struct target_ops *);
-    void (*to_attach) (struct target_ops *ops, char *, int)
-      TARGET_DEFAULT_FUNC (find_default_attach);
+    /* Attaches to a process on the target side.  Arguments are as
+       passed to the `attach' command by the user.  This routine can
+       be called when the target is not on the target-stack, if the
+       target_can_run routine returns 1; in that case, it must push
+       itself onto the stack.  Upon exit, the target should be ready
+       for normal operations, and should be ready to deliver the
+       status of the process immediately (without waiting) to an
+       upcoming target_wait call.  */
+    void (*to_attach) (struct target_ops *ops, char *, int);
     void (*to_post_attach) (struct target_ops *, int)
       TARGET_DEFAULT_IGNORE ();
     void (*to_detach) (struct target_ops *ops, const char *, int)
@@ -494,6 +501,11 @@  struct target_ops
       TARGET_DEFAULT_NORETURN (noprocess ());
     void (*to_load) (struct target_ops *, char *, int)
       TARGET_DEFAULT_NORETURN (tcomplain ());
+    /* Start an inferior process and set inferior_ptid to its pid.
+       EXEC_FILE is the file to run.
+       ALLARGS is a string containing the arguments to the program.
+       ENV is the environment vector to pass.  Errors reported with error().
+       On VxWorks and various standalone systems, we ignore exec_file.  */
     void (*to_create_inferior) (struct target_ops *, 
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (struct target_ops *, ptid_t)
@@ -519,6 +531,9 @@  struct target_ops
       TARGET_DEFAULT_RETURN (0);
     void (*to_mourn_inferior) (struct target_ops *)
       TARGET_DEFAULT_FUNC (default_mourn_inferior);
+    /* Note that to_can_run is special and can be invoked on an
+       unpushed target.  Targets defining this method must also define
+       to_can_async_p and to_supports_non_stop.  */
     int (*to_can_run) (struct target_ops *)
       TARGET_DEFAULT_RETURN (0);
 
@@ -561,14 +576,18 @@  struct target_ops
     int (*to_has_execution) (struct target_ops *, ptid_t);
     int to_has_thread_control;	/* control thread execution */
     int to_attach_no_wait;
-    /* ASYNC target controls */
+    /* This method must be implemented in some situations.  See the
+       comment on 'to_can_run'.  */
     int (*to_can_async_p) (struct target_ops *)
-      TARGET_DEFAULT_FUNC (find_default_can_async_p);
+      TARGET_DEFAULT_RETURN (0);
     int (*to_is_async_p) (struct target_ops *)
-      TARGET_DEFAULT_FUNC (find_default_is_async_p);
+      TARGET_DEFAULT_RETURN (0);
     void (*to_async) (struct target_ops *, async_callback_ftype *, void *)
       TARGET_DEFAULT_NORETURN (tcomplain ());
-    int (*to_supports_non_stop) (struct target_ops *);
+    /* This method must be implemented in some situations.  See the
+       comment on 'to_can_run'.  */
+    int (*to_supports_non_stop) (struct target_ops *)
+      TARGET_DEFAULT_RETURN (0);
     /* find_memory_regions support method for gcore */
     int (*to_find_memory_regions) (struct target_ops *,
 				   find_memory_region_ftype func, void *data)
@@ -1117,15 +1136,17 @@  extern struct target_ops current_target;
 
 void target_close (struct target_ops *targ);
 
-/* Attaches to a process on the target side.  Arguments are as passed
-   to the `attach' command by the user.  This routine can be called
-   when the target is not on the target-stack, if the target_can_run
-   routine returns 1; in that case, it must push itself onto the stack.
-   Upon exit, the target should be ready for normal operations, and
-   should be ready to deliver the status of the process immediately
-   (without waiting) to an upcoming target_wait call.  */
+/* Find the correct target to use for "attach".  If a target on the
+   current stack supports attaching, then it is returned.  Otherwise,
+   the default run target is returned.  */
+
+extern struct target_ops *find_attach_target (void);
 
-void target_attach (char *, int);
+/* Find the correct target to use for "run".  If a target on the
+   current stack supports creating a new inferior, then it is
+   returned.  Otherwise, the default run target is returned.  */
+
+extern struct target_ops *find_run_target (void);
 
 /* Some targets don't generate traps when attaching to the inferior,
    or their target_attach implementation takes care of the waiting.
@@ -1393,15 +1414,6 @@  extern void target_kill (void);
 
 extern void target_load (char *arg, int from_tty);
 
-/* Start an inferior process and set inferior_ptid to its pid.
-   EXEC_FILE is the file to run.
-   ALLARGS is a string containing the arguments to the program.
-   ENV is the environment vector to pass.  Errors reported with error().
-   On VxWorks and various standalone systems, we ignore exec_file.  */
-
-void target_create_inferior (char *exec_file, char *args,
-			     char **env, int from_tty);
-
 /* Some targets (such as ttrace-based HPUX) don't allow us to request
    notification of inferior events such as fork and vork immediately
    after the inferior is created.  (This because of how gdb gets an
@@ -1579,8 +1591,8 @@  extern int target_has_registers_1 (void);
    target is currently executing; for some targets, that's the same as
    whether or not the target is capable of execution, but there are
    also targets which can be current while not executing.  In that
-   case this will become true after target_create_inferior or
-   target_attach.  */
+   case this will become true after to_create_inferior or
+   to_attach.  */
 
 extern int target_has_execution_1 (ptid_t);
 
@@ -1616,8 +1628,6 @@  extern int target_async_permitted;
 /* Is the target in asynchronous execution mode?  */
 #define target_is_async_p() (current_target.to_is_async_p (&current_target))
 
-int target_supports_non_stop (void);
-
 /* Put the target in async mode with the specified callback function.  */
 #define target_async(CALLBACK,CONTEXT) \
      (current_target.to_async (&current_target, (CALLBACK), (CONTEXT)))
diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 61c13c4..6eb1f02 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -185,35 +185,41 @@  gdb_test "core" "No core file now."
 
 # Test a run (start) command will clear any loaded core file.
 
-gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
-gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
-
-set test "run: with core"
-if [runto_main] {
-    pass $test
-} else {
-    fail $test
-}
+proc corefile_test_run {} {
+    global corefile gdb_prompt
+
+    gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
+    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
 
-set test "run: core file is cleared"
-gdb_test_multiple "info files" $test {
-    -re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+    set test "run: with core"
+    if [runto_main] {
+	pass $test
+    } else {
 	fail $test
     }
-    -re "\r\n$gdb_prompt $" {
-	pass $test
+
+    set test "run: core file is cleared"
+    gdb_test_multiple "info files" $test {
+	-re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	    fail $test
+	}
+	-re "\r\n$gdb_prompt $" {
+	    pass $test
+	}
     }
-}
 
-set test "quit with a process"
-gdb_test_multiple "quit" $test {
-    -re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
-	pass $test
-	gdb_test "n" {Not confirmed\.} "quit with processes: n"
+    set test "quit with a process"
+    gdb_test_multiple "quit" $test {
+	-re "A debugging session is active.\r\n.*\r\nQuit anyway\\? \\(y or n\\) $" {
+	    pass $test
+	    gdb_test "n" {Not confirmed\.} "quit with processes: n"
+	}
     }
+
+    gdb_exit
 }
 
-gdb_exit
+corefile_test_run
 
 # Verify there is no question if only a core file is loaded.
 
@@ -235,37 +241,48 @@  gdb_exit
 
 # Test an attach command will clear any loaded core file.
 
-if ![is_remote target] {
-    set test "attach: spawn sleep"
-    set res [remote_spawn host "$binfile sleep"]
-    if { $res < 0 || $res == "" } {
-	fail $test
-	return
-    }
-    set pid [exp_pid -i $res]
-    # We don't care whether the program is still in the startup phase when we
-    # attach.
-
-    gdb_start
+proc corefile_test_attach {{async 0}} {
+    global binfile corefile gdb_prompt
 
-    gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
-    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+    if ![is_remote target] {
+	set test "attach: spawn sleep"
+	set res [remote_spawn host "$binfile sleep"]
+	if { $res < 0 || $res == "" } {
+	    fail $test
+	    return
+	}
+	set pid [exp_pid -i $res]
+	# We don't care whether the program is still in the startup phase when we
+	# attach.
 
-    gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+	gdb_start
 
-    set test "attach: core file is cleared"
-    gdb_test_multiple "info files" $test {
-	-re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
-	    fail $test
+	if {$async} {
+	    gdb_test_no_output "set target-async on" \
+		"enable target-async for attach tests"
 	}
-	-re "\r\n$gdb_prompt $" {
-	    pass $test
+
+	gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
+	gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+
+	gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+
+	set test "attach: core file is cleared"
+	gdb_test_multiple "info files" $test {
+	    -re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+		fail $test
+	    }
+	    -re "\r\n$gdb_prompt $" {
+		pass $test
+	    }
 	}
-    }
 
-    gdb_exit
+	gdb_exit
+    }
 }
 
+corefile_test_attach
+
 # Test warning-free core file load.  E.g., a Linux vDSO used to
 # trigger this warning:
 #     warning: Can't read pathname for load map: Input/output error.
@@ -281,3 +298,13 @@  gdb_test_multiple "core-file $corefile" $test {
 	pass $test
     }
 }
+
+
+# Try a couple tests again with target-async.
+with_test_prefix "target-async" {
+    clean_restart ${testfile}
+
+    gdb_test_no_output "set target-async on"
+    corefile_test_run
+    corefile_test_attach 1
+}
diff --git a/gdb/testsuite/gdb.reverse/break-precsave.exp b/gdb/testsuite/gdb.reverse/break-precsave.exp
index cc05d43..f1e6c69 100644
--- a/gdb/testsuite/gdb.reverse/break-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/break-precsave.exp
@@ -33,73 +33,86 @@  set bar_location  [gdb_get_line_number "break in bar" ]
 set main_location [gdb_get_line_number "break in main"]
 set end_location  [gdb_get_line_number "end of main"  ]
 
-runto main
+proc precsave_tests {} {
+    global foo_location bar_location main_location end_location
+    global decimal srcfile precsave gdb_prompt
 
-if [supports_process_record] {
-    # Activate process record/replay
-    gdb_test_no_output "record" "Turn on process record"
-}
+    runto main
 
-gdb_test "break $end_location" \
-    "Breakpoint $decimal at .*/$srcfile, line $end_location\." \
-    "BP at end of main"
+    if [supports_process_record] {
+	# Activate process record/replay
+	gdb_test_no_output "record" "Turn on process record"
+    }
 
-gdb_test "continue" "Breakpoint .* end of main .*" "run to end of main"
+    gdb_test "break $end_location" \
+	"Breakpoint $decimal at .*/$srcfile, line $end_location\." \
+	"BP at end of main"
 
-gdb_test "record save $precsave" \
-    "Saved core file $precsave with execution log\."  \
-    "save process recfile"
+    gdb_test "continue" "Breakpoint .* end of main .*" "run to end of main"
 
-gdb_test "kill" "" "Kill process, prepare to debug log file" \
-    "Kill the program being debugged\\? \\(y or n\\) " "y"
+    gdb_test "record save $precsave" \
+	"Saved core file $precsave with execution log\."  \
+	"save process recfile"
 
-gdb_test "record restore $precsave" \
-    "Program terminated with signal .*" \
-    "reload precord save file"
+    gdb_test "kill" "" "Kill process, prepare to debug log file" \
+	"Kill the program being debugged\\? \\(y or n\\) " "y"
 
-gdb_test "break foo" \
-    "Breakpoint $decimal at .* line $foo_location\." \
-    "set breakpoint on foo"
+    gdb_test "record restore $precsave" \
+	"Program terminated with signal .*" \
+	"reload precord save file"
 
-gdb_test "break bar" \
-    "Breakpoint $decimal at .* line $bar_location\." \
-    "set breakpoint on bar"
+    gdb_test "break foo" \
+	"Breakpoint $decimal at .* line $foo_location\." \
+	"set breakpoint on foo"
 
-gdb_continue_to_breakpoint "foo" ".*/$srcfile:$foo_location.*"
-gdb_continue_to_breakpoint "bar" ".*/$srcfile:$bar_location.*"
-gdb_test_multiple "continue" "go to end of main forward" {
-    -re ".*Breakpoint $decimal,.*/$srcfile:$end_location.*$gdb_prompt $"  {
-	pass "go to end of main forward"
-    }
-    -re "No more reverse-execution history.* end of main .*$gdb_prompt $" {
-	pass "go to end of main forward"
+    gdb_test "break bar" \
+	"Breakpoint $decimal at .* line $bar_location\." \
+	"set breakpoint on bar"
+
+    gdb_continue_to_breakpoint "foo" ".*/$srcfile:$foo_location.*"
+    gdb_continue_to_breakpoint "bar" ".*/$srcfile:$bar_location.*"
+    gdb_test_multiple "continue" "go to end of main forward" {
+	-re ".*Breakpoint $decimal,.*/$srcfile:$end_location.*$gdb_prompt $"  {
+	    pass "go to end of main forward"
+	}
+	-re "No more reverse-execution history.* end of main .*$gdb_prompt $" {
+	    pass "go to end of main forward"
+	}
     }
-}
 
-gdb_test_no_output "set exec-direction reverse" "set reverse"
+    gdb_test_no_output "set exec-direction reverse" "set reverse"
 
-gdb_continue_to_breakpoint "bar backward"  ".*/$srcfile:$bar_location.*"
-gdb_continue_to_breakpoint "foo backward"  ".*/$srcfile:$foo_location.*"
+    gdb_continue_to_breakpoint "bar backward"  ".*/$srcfile:$bar_location.*"
+    gdb_continue_to_breakpoint "foo backward"  ".*/$srcfile:$foo_location.*"
 
-gdb_test_multiple "continue" "main backward" {
-    -re ".*Breakpoint $decimal,.*/$srcfile:$main_location.*$gdb_prompt $" {
-	pass "main backward"
-    }
-    -re "No more reverse-execution history.* break in main .*$gdb_prompt $" {
-	pass "main backward"
+    gdb_test_multiple "continue" "main backward" {
+	-re ".*Breakpoint $decimal,.*/$srcfile:$main_location.*$gdb_prompt $" {
+	    pass "main backward"
+	}
+	-re "No more reverse-execution history.* break in main .*$gdb_prompt $" {
+	    pass "main backward"
+	}
     }
-}
 
-gdb_test_no_output "set exec-direction forward" "set forward"
+    gdb_test_no_output "set exec-direction forward" "set forward"
 
-gdb_continue_to_breakpoint "foo" ".*/$srcfile:$foo_location.*"
-gdb_continue_to_breakpoint "bar" ".*/$srcfile:$bar_location.*"
+    gdb_continue_to_breakpoint "foo" ".*/$srcfile:$foo_location.*"
+    gdb_continue_to_breakpoint "bar" ".*/$srcfile:$bar_location.*"
 
-gdb_test_multiple "continue" "end of record log" {
-    -re ".*Breakpoint $decimal,.*/$srcfile:$end_location.*$gdb_prompt $" {
-	pass "end of record log"
-    }
-    -re "No more reverse-execution history.* end of main .*$gdb_prompt $" {
-	pass "end of record log"
+    gdb_test_multiple "continue" "end of record log" {
+	-re ".*Breakpoint $decimal,.*/$srcfile:$end_location.*$gdb_prompt $" {
+	    pass "end of record log"
+	}
+	-re "No more reverse-execution history.* end of main .*$gdb_prompt $" {
+	    pass "end of record log"
+	}
     }
 }
+
+precsave_tests
+
+with_test_prefix "target-async" {
+    clean_restart $testfile
+    gdb_test_no_output "set target-async on"
+    precsave_tests
+}