[rfa] Allow a target to adjust target_has_execution while remaining active

[Daniel Jacobowitz <drow at false dot org>]

I posted this patch in the middle of a conversation with Fred, and then
forgot about it; here it is as a proper RFA.  This area is a bit
complex, and basicly not documented at all, so I added a page or two to
gdbint about it.

Here's Fred's report of trouble:
  http://sourceware.org/ml/gdb-patches/2006-08/msg00014.html

The explanation of the patch, quoted from that thread, is below.  If
you're wondering why the extra complexity matters for remote/sim
targets, the only reason it isn't necessary for native targets too is
find_default_run_target, which loops through every registered
target vector looking for the first one which offers to_can_run.
Fortunately that's usually only one and it doesn't require any
explicit to_open operation.  But you can't just say "run" without
connecting to an extended-remote target: we won't know where it
is to connect to!  So, there's advantage to staying connected
and keeping it current, even if there's nothing running at the moment.

Tested x86_64-pc-linux-gnu, and with various remote/sim configurations.
Does this look OK?  How about the docs?

===
  A target vector can be completely inactive (not pushed on the target
  stack), active but not running (pushed, but not connected to a fully
  manifested inferior), or completely active (pushed, with an accessible
  inferior).  Most targets are only completely inactive or completely
  active, but some support persistant connections to a target even
  when the target has exited or not yet started.
  
  For example, connecting to the simulator using @code{target sim} does
  not create a running program.  Neither registers nor memory are
  accessible until @code{run}.  Similarly, after @code{kill}, the
  program can not continue executing.  But in both cases @value{GDBN}
  remains connected to the simulator, and target-specific commands
  are directed to the simulator.
  
  A target which only supports complete activation should push itself
  onto the stack in its @code{to_open} routine (by calling
  @code{push_target}), and unpush itself from the stack in its
  @code{to_mourn_inferior} routine (by calling @code{unpush_target}).
  
  A target which supports both partial and complete activation should
  still call @code{push_target} in @code{to_open}, but not call
  @code{unpush_target} in @code{to_mourn_inferior}.  Instead, it should
  call one of @code{target_mark_running} and @code{target_mark_exited}
  in its @code{to_open}, depending on whether the target is fully active
  after connection.  It should also call @code{target_mark_running} any
  time the inferior becomes fully active (e.g.@: in
  @code{to_create_inferior} and @code{to_attach}), and
  @code{target_mark_exited} when the inferior becomes inactive (in
  @code{to_mourn_inferior}).  The target should also make sure to call
  @code{target_mourn_inferior} from its @code{to_kill}, to return the
  target to inactive state.

This is a bit different than we had before, but I think it's fairly
clear and more useful.  I tested this on arm-sim, where it removes a
bunch of ERRORs, by fixing "target sim; kill; file" - Fred reported
that "file" in that case warned that the target was already running,
even though we'd just issued a "kill".

I'll be using this approach for the extended-remote target, or a
similar variant, soon.
===

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdbint.texinfo (Target Vector Definition): Move most
	content into Existing Targets.  Add a menu.
	(Existing Targets): New section, moved from Target Vector
	Definition.  Use @subsection.
	(Managing Execution State): New section.

2006-08-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* remote-sim.c (gdbsim_kill): Call target_mourn_inferior.
	(gdbsim_load): Don't bother to adjust inferior_ptid here.
	(gdbsim_create_inferior): Mark the simulator as running.
	(gdbsim_open): Don't bother fetching registers.  Mark
	the target as not running.
	(gdbsim_xfer): When the program is not running, pass memory
	requests down.
	(gdbsim_mourn_inferior): Mark the target as not running.
	* target.c (target_mark_running, target_mark_exited): New.
	* target.h (target_has_execution): Update the comment.
	(target_mark_running, target_mark_exited): New prototypes.

Index: gdb-mainline/gdb/doc/gdbint.texinfo
===================================================================
--- gdb-mainline.orig/gdb/doc/gdbint.texinfo	2006-05-15 00:20:01.000000000 -0700
+++ gdb-mainline/gdb/doc/gdbint.texinfo	2006-08-16 19:02:19.000000000 -0700
@@ -4405,11 +4405,56 @@ actually exercises control over a proces
 @value{GDBN} includes some 30-40 different target vectors; however,
 each configuration of @value{GDBN} includes only a few of them.
 
-@section File Targets
+@menu
+* Managing Execution State::
+* Existing Targets::
+@end menu
+
+@node Managing Execution State
+@section Managing Execution State
+@cindex execution state
+
+A target vector can be completely inactive (not pushed on the target
+stack), active but not running (pushed, but not connected to a fully
+manifested inferior), or completely active (pushed, with an accessible
+inferior).  Most targets are only completely inactive or completely
+active, but some support persistant connections to a target even
+when the target has exited or not yet started.
+
+For example, connecting to the simulator using @code{target sim} does
+not create a running program.  Neither registers nor memory are
+accessible until @code{run}.  Similarly, after @code{kill}, the
+program can not continue executing.  But in both cases @value{GDBN}
+remains connected to the simulator, and target-specific commands
+are directed to the simulator.
+
+A target which only supports complete activation should push itself
+onto the stack in its @code{to_open} routine (by calling
+@code{push_target}), and unpush itself from the stack in its
+@code{to_mourn_inferior} routine (by calling @code{unpush_target}).
+
+A target which supports both partial and complete activation should
+still call @code{push_target} in @code{to_open}, but not call
+@code{unpush_target} in @code{to_mourn_inferior}.  Instead, it should
+call one of @code{target_mark_running} and @code{target_mark_exited}
+in its @code{to_open}, depending on whether the target is fully active
+after connection.  It should also call @code{target_mark_running} any
+time the inferior becomes fully active (e.g.@: in
+@code{to_create_inferior} and @code{to_attach}), and
+@code{target_mark_exited} when the inferior becomes inactive (in
+@code{to_mourn_inferior}).  The target should also make sure to call
+@code{target_mourn_inferior} from its @code{to_kill}, to return the
+target to inactive state.
+
+@node Existing Targets
+@section Existing Targets
+@cindex targets
+
+@subsection File Targets
 
 Both executables and core files have target vectors.
 
-@section Standard Protocol and Remote Stubs
+@subsection Standard Protocol and Remote Stubs
 
 @value{GDBN}'s file @file{remote.c} talks a serial protocol to code
 that runs in the target system.  @value{GDBN} provides several sample
@@ -4454,13 +4499,13 @@ of the debugger/stub.
 From reading the stub, it's probably not obvious how breakpoints work.
 They are simply done by deposit/examine operations from @value{GDBN}.
 
-@section ROM Monitor Interface
+@subsection ROM Monitor Interface
 
-@section Custom Protocols
+@subsection Custom Protocols
 
-@section Transport Layer
+@subsection Transport Layer
 
-@section Builtin Simulator
+@subsection Builtin Simulator
 
 
 @node Native Debugging
Index: gdb-mainline/gdb/remote-sim.c
===================================================================
--- gdb-mainline.orig/gdb/remote-sim.c	2006-04-19 00:18:09.000000000 -0700
+++ gdb-mainline/gdb/remote-sim.c	2006-08-16 18:42:28.000000000 -0700
@@ -383,8 +383,8 @@ gdbsim_kill (void)
     printf_filtered ("gdbsim_kill\n");
 
   /* There is no need to `kill' running simulator - the simulator is
-     not running */
-  inferior_ptid = null_ptid;
+     not running.  Mourning it is enough.  */
+  target_mourn_inferior ();
 }
 
 /* Load an executable file into the target process.  This is expected to
@@ -410,8 +410,6 @@ gdbsim_load (char *args, int fromtty)
   if (sr_get_debug ())
     printf_filtered ("gdbsim_load: prog \"%s\"\n", prog);
 
-  inferior_ptid = null_ptid;
-
   /* FIXME: We will print two messages on error.
      Need error to either not print anything if passed NULL or need
      another routine that doesn't take any arguments.  */
@@ -469,6 +467,7 @@ gdbsim_create_inferior (char *exec_file,
   sim_create_inferior (gdbsim_desc, exec_bfd, argv, env);
 
   inferior_ptid = pid_to_ptid (42);
+  target_mark_running (&gdbsim_ops);
   insert_breakpoints ();	/* Needed to get correct instruction in cache */
 
   clear_proceed_status ();
@@ -543,8 +542,12 @@ gdbsim_open (char *args, int from_tty)
     error (_("unable to create simulator instance"));
 
   push_target (&gdbsim_ops);
-  target_fetch_registers (-1);
   printf_filtered ("Connected to the simulator.\n");
+
+  /* There's nothing running after "target sim" or "load"; not until
+     "run".  */
+  inferior_ptid = null_ptid;
+  target_mark_exited (&gdbsim_ops);
 }
 
 /* Does whatever cleanup is required for a target that we are no longer
@@ -747,6 +750,12 @@ gdbsim_xfer_inferior_memory (CORE_ADDR m
 			     int write, struct mem_attrib *attrib,
 			     struct target_ops *target)
 {
+  /* If no program is running yet, then ignore the simulator for
+     memory.  Pass the request down to the next target, hopefully
+     an exec file.  */
+  if (!target_has_execution)
+    return 0;
+
   if (!program_loaded)
     error (_("No program loaded."));
 
@@ -802,6 +811,7 @@ gdbsim_mourn_inferior (void)
     printf_filtered ("gdbsim_mourn_inferior:\n");
 
   remove_breakpoints ();
+  target_mark_exited (&gdbsim_ops);
   generic_mourn_inferior ();
 }
 
Index: gdb-mainline/gdb/target.c
===================================================================
--- gdb-mainline.orig/gdb/target.c	2006-08-16 00:22:21.000000000 -0700
+++ gdb-mainline/gdb/target.c	2006-08-16 17:58:58.000000000 -0700
@@ -645,6 +645,56 @@ update_current_target (void)
   current_target.beneath = target_stack;
 }
 
+/* Mark OPS as a running target.  This reverses the effect
+   of target_mark_exited.  */
+
+void
+target_mark_running (struct target_ops *ops)
+{
+  struct target_ops *t;
+
+  for (t = target_stack; t != NULL; t = t->beneath)
+    if (t == ops)
+      break;
+  if (t == NULL)
+    internal_error (__FILE__, __LINE__,
+		    "Attempted to mark unpushed target \"%s\" as running",
+		    ops->to_shortname);
+
+  ops->to_has_execution = 1;
+  ops->to_has_all_memory = 1;
+  ops->to_has_memory = 1;
+  ops->to_has_stack = 1;
+  ops->to_has_registers = 1;
+
+  update_current_target ();
+}
+
+/* Mark OPS as a non-running target.  This reverses the effect
+   of target_mark_running.  */
+
+void
+target_mark_exited (struct target_ops *ops)
+{
+  struct target_ops *t;
+
+  for (t = target_stack; t != NULL; t = t->beneath)
+    if (t == ops)
+      break;
+  if (t == NULL)
+    internal_error (__FILE__, __LINE__,
+		    "Attempted to mark unpushed target \"%s\" as running",
+		    ops->to_shortname);
+
+  ops->to_has_execution = 0;
+  ops->to_has_all_memory = 0;
+  ops->to_has_memory = 0;
+  ops->to_has_stack = 0;
+  ops->to_has_registers = 0;
+
+  update_current_target ();
+}
+
 /* Push a new target type into the stack of the existing target accessors,
    possibly superseding some of the existing accessors.
 
Index: gdb-mainline/gdb/target.h
===================================================================
--- gdb-mainline.orig/gdb/target.h	2006-08-16 00:22:21.000000000 -0700
+++ gdb-mainline/gdb/target.h	2006-08-16 18:26:51.000000000 -0700
@@ -873,11 +873,12 @@ int target_follow_fork (int follow_child
      (current_target.to_has_registers)
 
 /* Does the target have execution?  Can we make it jump (through
-   hoops), or pop its stack a few times?  FIXME: If this is to work that
-   way, it needs to check whether an inferior actually exists.
-   remote-udi.c and probably other targets can be the current target
-   when the inferior doesn't actually exist at the moment.  Right now
-   this just tells us whether this target is *capable* of execution.  */
+   hoops), or pop its stack a few times?  This means that the current
+   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.  */
 
 #define	target_has_execution	\
      (current_target.to_has_execution)
@@ -1131,6 +1132,13 @@ extern void target_preopen (int);
 
 extern void pop_target (void);
 
+/* Mark a pushed target as running or exited, for targets which do not
+   automatically pop when not active.  */
+
+void target_mark_running (struct target_ops *);
+
+void target_mark_exited (struct target_ops *);
+
 /* Struct section_table maps address ranges to file sections.  It is
    mostly used with BFD files, but can be used without (e.g. for handling
    raw disks, or files not in formats handled by BFD).  */