[RFC] Observer mode

[RFC] Observer mode

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

Here is another patch that pushes the envelope a bit, and for which I 
would like to get some prior agreement that it's a good idea.

The basic idea here is that GDB is to be used to look at targets 
installed in the field and in use.  In such a situation it may connect 
to the target and examine state, but not alter that state or interrupt 
anything.  In our particular target, tracing is allowed, but only using 
fast tracepoints, because they are done using jumps and thus act as if 
they were part of the program; taking a trap, even for a slow 
tracepoint, disrupts thread execution unacceptably.  We have been 
calling this "observer mode", and the customer's GDB is actually 
configured to launch in this mode, to reduce the chance of accidental 
stopping - the debugger user has to ask specifically to change it to 
normal debugging mode.

The patch for this is conceptually straightforward, merely introducing a 
set of user-settable variables, such as may-write-memory, which gate all 
user attempts to write memory, being tested at the target vector level.  
Later patches in the series add a "set observer" variable which 
set/clears all of the individual permissions together, and also ensure 
that non-stop mode is on.

Although enabling/disabling at the target vector is somewhat brute 
force, it seems to work reasonably well in practice; GDB doesn't get 
knotted into weird states, and the error messages from failed 
modification attempts (direct or indirect) are reasonable.

If people like this idea, I'll update the old patch and resubmit.

Stan

2009-05-19  Stan Shebs  <stan@codesourcery.com>
 
    * target.h (target_insert_breakpoint): Change macro to function.
    (target_remove_breakpoint): Ditto.
    (target_stop): Ditto.
    * target.c (may_write_registers): New global.
    (may_write_memory): New global.
    (may_insert_breakpoints): New global.
    (may_insert_tracepoints): New global.
    (may_insert_fast_tracepoints): New global.
    (may_stop): New global.
    (target_xfer_partial): Test for write permission.
    (target_store_registers): Ditto.
    (target_insert_breakpoint): New function.
    (target_remove_breakpoint): New function.
    (target_stop): New function.
    (_initialize_targets): Add new set/show variables.
    * tracepoint.c (download_target): Check permission,
    return a success/fail result.
    (trace_start_command): Don't start tracing if no
    tracepoints were downloaded.

    gdb/doc/
    * gdb.texinfo (Observation Mode): New section.


[-- Attachment #2: observer-patch-0 --]
[-- Type: text/plain, Size: 16175 bytes --]

Index: gdb/doc/gdb.texinfo
===================================================================
*** gdb/doc/gdb.texinfo	(revision 250339)
--- gdb/doc/gdb.texinfo	(revision 250340)
*************** you examine the stopped thread in the de
*** 4550,4555 ****
--- 4550,4556 ----
  * Background Execution::	Running your program asynchronously
  * Thread-Specific Breakpoints::	Controlling breakpoints
  * Interrupted System Calls::	GDB may interfere with system calls
+ * Observation Mode::            GDB does not alter program behavior
  @end menu
  
  @node All-Stop Mode
*************** monitor certain events such as thread cr
*** 4880,4885 ****
--- 4881,4970 ----
  When such an event happens, a system call in another thread may return
  prematurely, even though your program does not appear to stop.
  
+ @node Observation Mode
+ @subsection Observation Mode
+ 
+ If you want to build on non-stop mode and observe program behavior
+ without any chance of disruption by @value{GDBN}, you can set
+ variables to disable all of the debugger's attempts to modify state,
+ whether by writing memory, inserting breakpoints, etc.  These operate
+ at a low level, intercepting operations from all commands.
+ 
+ When all of these are set to @code{off}, then @value{GDBN} is said
+ to be @dfn{observation mode}.
+ 
+ Note that @value{GDBN} will not prevent you from making nonsensical
+ combinations of these settings. For instance, if you have enabled
+ @code{may-insert-breakpoints} but disabled @code{may-write-memory},
+ then breakpoints that work by writing trap instructions into the code
+ stream will still not be able to be placed.
+ 
+ @table @code
+ 
+ @kindex may-write-registers
+ @item set may-write-registers on
+ @itemx set may-write-registers off
+ This controls whether @value{GDBN} will attempt to alter the values of
+ registers, such as with assignment expressions in @code{print}, or the
+ @code{jump} command.  It defaults to @code{on}.
+ 
+ @item show may-write-registers
+ Show the current permission to write registers.
+ 
+ @kindex may-write-memory
+ @item set may-write-memory on
+ @itemx set may-write-memory off
+ This controls whether @value{GDBN} will attempt to alter the contents
+ of memory, such as with assignment expressions in @code{print}.  It
+ defaults to @code{on}.
+ 
+ @item show may-write-memory
+ Show the current permission to write memory.
+ 
+ @kindex may-insert-breakpoints
+ @item set may-insert-breakpoints on
+ @itemx set may-insert-breakpoints off
+ This controls whether @value{GDBN} will attempt to insert breakpoints.
+ This affects all breakpoints, including internal breakpoints defined
+ by @value{GDBN}.  It defaults to @code{on}.
+ 
+ @item show may-insert-breakpoints
+ Show the current permission to insert breakpoints.
+ 
+ @kindex may-insert-tracepoints
+ @item set may-insert-tracepoints on
+ @itemx set may-insert-tracepoints off
+ This controls whether @value{GDBN} will attempt to insert (regular)
+ tracepoints at the beginning of a tracing experiment. It affects only
+ non-fast tracepoints, fast tracepoints being under the control of
+ @code{may-insert-fast-tracepoints}. It defaults to @code{on}.
+ 
+ @item show may-insert-tracepoints
+ Show the current permission to insert tracepoints.
+ 
+ @kindex may-insert-fast-tracepoints
+ @item set may-insert-fast-tracepoints on
+ @itemx set may-insert-fast-tracepoints off
+ This controls whether @value{GDBN} will attempt to insert fast
+ tracepoints at the beginning of a tracing experiment. It affects only
+ fast tracepoints, regular (non-fast) tracepoints being under the
+ control of @code{may-insert-tracepoints}. It defaults to @code{on}.
+ 
+ @item show may-insert-fast-tracepoints
+ Show the current permission to insert fast tracepoints.
+ 
+ @kindex may-interrupt
+ @item set may-interrupt on
+ @itemx set may-interrupt off
+ This controls whether @value{GDBN} will attempt to interrupt or stop
+ program execution. When this variable is @code{off}, the
+ @code{interrupt} command will have no effect, nor will
+ @kbd{Ctrl-c}. It defaults to @code{on}.
+ 
+ @item show may-interrupt
+ Show the current permission to interrupt or stop the program.
+ 
+ @end table
  
  @node Reverse Execution
  @chapter Running programs backward
Index: gdb/target.c
===================================================================
*** gdb/target.c	(revision 250339)
--- gdb/target.c	(revision 250340)
*************** static int trust_readonly = 0;
*** 189,194 ****
--- 189,206 ----
  
  static int show_memory_breakpoints = 0;
  
+ static int may_write_registers = 1;
+ 
+ static int may_write_memory = 1;
+ 
+ static int may_insert_breakpoints = 1;
+ 
+ /* static */ int may_insert_tracepoints = 1;
+ 
+ /* static */ int may_insert_fast_tracepoints = 1;
+ 
+ static int may_stop = 1;
+ 
  /* Non-zero if we want to see trace of target level stuff.  */
  
  static int targetdebug = 0;
*************** target_xfer_partial (struct target_ops *
*** 1168,1173 ****
--- 1180,1189 ----
  
    gdb_assert (ops->to_xfer_partial != NULL);
  
+   if (writebuf && !may_write_memory)
+     error (_("Writing to memory is not allowed (addr %s, len %s)"),
+ 	   core_addr_to_string_nz (offset), plongest (len));
+ 
    /* If this is a memory transfer, let the memory-specific code
       have a look at it instead.  Memory transfers are more
       complicated.  */
*************** get_target_memory_unsigned (struct targe
*** 1692,1697 ****
--- 1708,1741 ----
    return extract_unsigned_integer (buf, len);
  }
  
+ int
+ target_insert_breakpoint (struct bp_target_info *bp_tgt)
+ {
+   if (!may_insert_breakpoints)
+     {
+       warning (_("May not insert breakpoints"));
+       return 1;
+     }
+ 
+   return (*current_target.to_insert_breakpoint) (bp_tgt);
+ }
+ 
+ int
+ target_remove_breakpoint (struct bp_target_info *bp_tgt)
+ {
+   /* This is kind of a weird case to handle, but the permission might
+      have been changed after breakpoints were inserted - in which case
+      we should just take the user literally and assume that any
+      breakpoints should be left in place.  */
+   if (!may_insert_breakpoints)
+     {
+       warning (_("May not remove breakpoints"));
+       return 1;
+     }
+ 
+   return (*current_target.to_remove_breakpoint) (bp_tgt);
+ }
+ 
  static void
  target_info (char *args, int from_tty)
  {
*************** target_find_new_threads (void)
*** 2654,2659 ****
--- 2698,2715 ----
      }
  }
  
+ void
+ target_stop (ptid_t ptid)
+ {
+   if (!may_stop)
+     {
+       warning (_("May not interrupt or stop the target, ignoring attempt"));
+       return;
+     }
+ 
+   (*current_target.to_stop) (ptid);
+ }
+ 
  static void
  debug_to_post_attach (int pid)
  {
*************** target_fetch_registers (struct regcache 
*** 2756,2763 ****
  void
  target_store_registers (struct regcache *regcache, int regno)
  {
- 
    struct target_ops *t;
    for (t = current_target.beneath; t != NULL; t = t->beneath)
      {
        if (t->to_store_registers != NULL)
--- 2812,2822 ----
  void
  target_store_registers (struct regcache *regcache, int regno)
  {
    struct target_ops *t;
+ 
+   if (!may_write_registers)
+     error (_("Writing to registers is not allowed (regno %d)"), regno);
+ 
    for (t = current_target.beneath; t != NULL; t = t->beneath)
      {
        if (t->to_store_registers != NULL)
*************** Tells gdb whether to control the inferio
*** 3360,3364 ****
--- 3419,3478 ----
  			   &setlist,
  			   &showlist);
  
+   add_setshow_boolean_cmd ("may-write-registers", class_support,
+ 			   &may_write_registers, _("\
+ Set permission to write into registers."), _("\
+ Show permission to write into registers."), _("\
+ When this permission is on, GDB may write into the target's registers.\n\
+ Otherwise, any sort of write attempt will result in an error."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+   add_setshow_boolean_cmd ("may-write-memory", class_support,
+ 			   &may_write_memory, _("\
+ Set permission to write into target memory."), _("\
+ Show permission to write into target memory."), _("\
+ When this permission is on, GDB may write into the target's memory.\n\
+ Otherwise, any sort of write attempt will result in an error."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+   add_setshow_boolean_cmd ("may-insert-breakpoints", class_support,
+ 			   &may_insert_breakpoints, _("\
+ Set permission to insert breakpoints in the target."), _("\
+ Show permission to insert breakpoints in the target."), _("\
+ When this permission is on, GDB may insert breakpoints in the program.\n\
+ Otherwise, any sort of insertion attempt will result in an error."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+   add_setshow_boolean_cmd ("may-insert-tracepoints", class_support,
+ 			   &may_insert_tracepoints, _("\
+ Set permission to insert tracepoints in the target."), _("\
+ Show permission to insert tracepoints in the target."), _("\
+ When this permission is on, GDB may insert tracepoints in the program.\n\
+ Otherwise, any sort of insertion attempt will result in an error."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+   add_setshow_boolean_cmd ("may-insert-fast-tracepoints", class_support,
+ 			   &may_insert_fast_tracepoints, _("\
+ Set permission to insert fast tracepoints in the target."), _("\
+ Show permission to insert fast tracepoints in the target."), _("\
+ When this permission is on, GDB may insert fast tracepoints.\n\
+ Otherwise, any sort of insertion attempt will result in an error."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+   add_setshow_boolean_cmd ("may-interrupt", class_support,
+ 			   &may_stop, _("\
+ Set permission to interrupt the target."), _("\
+ Show permission to interrupt the target."), _("\
+ When this permission is on, GDB may interrupt/stop the target's execution.\n\
+ Otherwise, any attempt to interrupt or stop will be ignored."),
+ 			   NULL, NULL,
+ 			   &setlist, &showlist);
+ 
+ 
    target_dcache = dcache_init ();
  }
Index: gdb/target.h
===================================================================
*** gdb/target.h	(revision 250339)
--- gdb/target.h	(revision 250340)
*************** extern void print_section_info (struct t
*** 735,748 ****
  /* Insert a breakpoint at address BP_TGT->placed_address in the target
     machine.  Result is 0 for success, or an errno value.  */
  
! #define	target_insert_breakpoint(bp_tgt)	\
!      (*current_target.to_insert_breakpoint) (bp_tgt)
  
  /* Remove a breakpoint at address BP_TGT->placed_address in the target
     machine.  Result is 0 for success, or an errno value.  */
  
! #define	target_remove_breakpoint(bp_tgt)	\
!      (*current_target.to_remove_breakpoint) (bp_tgt)
  
  /* Initialize the terminal settings we record for the inferior,
     before we actually run the inferior.  */
--- 735,746 ----
  /* Insert a breakpoint at address BP_TGT->placed_address in the target
     machine.  Result is 0 for success, or an errno value.  */
  
! extern int target_insert_breakpoint (struct bp_target_info *bp_tgt);
  
  /* Remove a breakpoint at address BP_TGT->placed_address in the target
     machine.  Result is 0 for success, or an errno value.  */
  
! extern int target_remove_breakpoint (struct bp_target_info *bp_tgt);
  
  /* Initialize the terminal settings we record for the inferior,
     before we actually run the inferior.  */
*************** extern void target_find_new_threads (voi
*** 917,923 ****
     Unix, this should act like SIGSTOP).  This function is normally
     used by GUIs to implement a stop button.  */
  
! #define target_stop(ptid) (*current_target.to_stop) (ptid)
  
  /* Send the specified COMMAND to the target's monitor
     (shell,interpreter) for execution.  The result of the query is
--- 915,921 ----
     Unix, this should act like SIGSTOP).  This function is normally
     used by GUIs to implement a stop button.  */
  
! extern void target_stop (ptid_t ptid);
  
  /* Send the specified COMMAND to the target's monitor
     (shell,interpreter) for execution.  The result of the query is
Index: gdb/tracepoint.c
===================================================================
*** gdb/tracepoint.c	(revision 250339)
--- gdb/tracepoint.c	(revision 250340)
***************
*** 26,31 ****
--- 26,33 ----
  #include "gdbcmd.h"
  #include "value.h"
  #include "target.h"
+ extern int may_insert_tracepoints;
+ extern int may_insert_fast_tracepoints;
  #include "language.h"
  #include "gdb_string.h"
  #include "inferior.h"
*************** remote_set_transparent_ranges (void)
*** 1501,1507 ****
     to the target.  If no errors, 
     Tell target to start a new trace experiment.  */
  
! void download_tracepoint (struct breakpoint *t);
  
  static void
  trace_start_command (char *args, int from_tty)
--- 1503,1509 ----
     to the target.  If no errors, 
     Tell target to start a new trace experiment.  */
  
! int download_tracepoint (struct breakpoint *t);
  
  static void
  trace_start_command (char *args, int from_tty)
*************** trace_start_command (char *args, int fro
*** 1511,1516 ****
--- 1513,1519 ----
    int ix;
    struct breakpoint *t;
    struct trace_state_variable *tsv;
+   int any_downloaded = 0;
  
    dont_repeat ();	/* Like "run", dangerous to repeat accidentally.  */
  
*************** trace_start_command (char *args, int fro
*** 1524,1533 ****
        tp_vec = all_tracepoints ();
        for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
  	{
! 	  download_tracepoint (t);
  	}
        VEC_free (breakpoint_p, tp_vec);
  
        /* Init any trace state variables that start with nonzero values.  */
        for (tsv = tvariables; tsv; tsv = tsv->next)
  	{
--- 1527,1541 ----
        tp_vec = all_tracepoints ();
        for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
  	{
! 	  if (download_tracepoint (t))
! 	    any_downloaded = 1;
  	}
        VEC_free (breakpoint_p, tp_vec);
  
+       /* No point in tracing without any tracepoints... */
+       if (!any_downloaded)
+ 	error ("No tracepoints downloaded, not starting trace");
+ 
        /* Init any trace state variables that start with nonzero values.  */
        for (tsv = tvariables; tsv; tsv = tsv->next)
  	{
*************** trace_start_command (char *args, int fro
*** 1559,1567 ****
      error (_("Trace can only be run on remote targets."));
  }
  
! /* Send the definition of a single tracepoint to the target.  */
  
! void
  download_tracepoint (struct breakpoint *t)
  {
    CORE_ADDR tpaddr;
--- 1567,1576 ----
      error (_("Trace can only be run on remote targets."));
  }
  
! /* Send the definition of a single tracepoint to the target. Return 1
!    if successful, 0 if not.  */
  
! int
  download_tracepoint (struct breakpoint *t)
  {
    CORE_ADDR tpaddr;
*************** download_tracepoint (struct breakpoint *
*** 1574,1579 ****
--- 1583,1597 ----
    struct agent_expr *aexpr;
    struct cleanup *aexpr_chain = NULL;
  
+   if ((t->type == bp_fast_tracepoint
+        ? !may_insert_fast_tracepoints
+        : !may_insert_tracepoints))
+     {
+       warning (_("May not insert %stracepoints, skipping tracepoint %d"),
+ 	       (t->type == bp_fast_tracepoint ? "fast " : ""), t->number);
+       return 0;
+     }
+ 
    tpaddr = t->loc->address;
    sprintf_vma (tmp, (t->loc ? tpaddr : 0));
    sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", t->number, 
*************** download_tracepoint (struct breakpoint *
*** 1617,1623 ****
      error (_("Target does not support tracepoints."));
  
    if (!t->actions && !*default_collect)
!     return;
  
    encode_actions (t, &tdp_actions, &stepping_actions);
    old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
--- 1635,1641 ----
      error (_("Target does not support tracepoints."));
  
    if (!t->actions && !*default_collect)
!     return 1;
  
    encode_actions (t, &tdp_actions, &stepping_actions);
    old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
*************** download_tracepoint (struct breakpoint *
*** 1660,1665 ****
--- 1678,1684 ----
  	}
      }
    do_cleanups (old_chain);
+   return 1;
  }
  
  /* tstop command */

Re: [RFC] Observer mode

[Eli Zaretskii]

> Date: Thu, 29 Apr 2010 10:22:31 -0700
> From: Stan Shebs <stan@codesourcery.com>
> 
> + @node Observation Mode
> + @subsection Observation Mode

A suitable index entry would be good here.

> + When all of these are set to @code{off}, then @value{GDBN} is said
> + to be @dfn{observation mode}.

"in observation mode", I presume.

Also, the above sentence is ambiguous: it took me a while to
understand that "all of these" refer to the settings described in this
section.  Please say that explicitly.

> + combinations of these settings. For instance, if you have enabled
                                  ^^
Need two spaces here (and in a few other places in the patch).

> + @itemx set may-write-memory off
> + This controls whether @value{GDBN} will attempt to alter the contents
> + of memory, such as with assignment expressions in @code{print}.

This is also relevant to setting breakpoints, right?  If so, I suggest
to mention them.

> + Otherwise, any sort of write attempt will result in an error."),

Is this useful behavior?  I'd assume that setting these to off would
silently fail the operation.  Otherwise, how would these settings be
useful?  They exist to allow uninterrupted program run, don't they?

> + 	error ("No tracepoints downloaded, not starting trace");

No _() around a message here.

Thanks.

Re: [RFC] Observer mode

[Michael Snyder]

Stan Shebs wrote:
> Here is another patch that pushes the envelope a bit, and for which I 
> would like to get some prior agreement that it's a good idea.
> 
> The basic idea here is that GDB is to be used to look at targets 
> installed in the field and in use.  In such a situation it may connect 
> to the target and examine state, but not alter that state or interrupt 
> anything.  In our particular target, tracing is allowed, but only using 
> fast tracepoints, because they are done using jumps and thus act as if 
> they were part of the program; taking a trap, even for a slow 
> tracepoint, disrupts thread execution unacceptably.  We have been 
> calling this "observer mode", and the customer's GDB is actually 
> configured to launch in this mode, to reduce the chance of accidental 
> stopping - the debugger user has to ask specifically to change it to 
> normal debugging mode.
> 
> The patch for this is conceptually straightforward, merely introducing a 
> set of user-settable variables, such as may-write-memory, which gate all 
> user attempts to write memory, being tested at the target vector level.  
> Later patches in the series add a "set observer" variable which 
> set/clears all of the individual permissions together, and also ensure 
> that non-stop mode is on.
> 
> Although enabling/disabling at the target vector is somewhat brute 
> force, it seems to work reasonably well in practice; GDB doesn't get 
> knotted into weird states, and the error messages from failed 
> modification attempts (direct or indirect) are reasonable.
> 
> If people like this idea, I'll update the old patch and resubmit.

How different is this from async mode?

> 2009-05-19  Stan Shebs  <stan@codesourcery.com>
>  
>     * target.h (target_insert_breakpoint): Change macro to function.
>     (target_remove_breakpoint): Ditto.
>     (target_stop): Ditto.
>     * target.c (may_write_registers): New global.
>     (may_write_memory): New global.
>     (may_insert_breakpoints): New global.
>     (may_insert_tracepoints): New global.
>     (may_insert_fast_tracepoints): New global.
>     (may_stop): New global.
>     (target_xfer_partial): Test for write permission.
>     (target_store_registers): Ditto.
>     (target_insert_breakpoint): New function.
>     (target_remove_breakpoint): New function.
>     (target_stop): New function.
>     (_initialize_targets): Add new set/show variables.
>     * tracepoint.c (download_target): Check permission,
>     return a success/fail result.
>     (trace_start_command): Don't start tracing if no
>     tracepoints were downloaded.
> 
>     gdb/doc/
>     * gdb.texinfo (Observation Mode): New section.
> 

Re: [RFC] Observer mode

[Stan Shebs]

Michael Snyder wrote:
> Stan Shebs wrote:
>> Here is another patch that pushes the envelope a bit, and for which I 
>> would like to get some prior agreement that it's a good idea.
>>
>> The basic idea here is that GDB is to be used to look at targets 
>> installed in the field and in use.  In such a situation it may 
>> connect to the target and examine state, but not alter that state or 
>> interrupt anything.  In our particular target, tracing is allowed, 
>> but only using fast tracepoints, because they are done using jumps 
>> and thus act as if they were part of the program; taking a trap, even 
>> for a slow tracepoint, disrupts thread execution unacceptably.  We 
>> have been calling this "observer mode", and the customer's GDB is 
>> actually configured to launch in this mode, to reduce the chance of 
>> accidental stopping - the debugger user has to ask specifically to 
>> change it to normal debugging mode.
>>
>> [...]
>
> How different is this from async mode?
Observer mode includes async mode.  One could call it a "read-only 
non-stop async mode that allows fast tracepoints", although that's 
rather a mouthful. :-)

Stan

Re: [RFC] Observer mode

[Tom Tromey]

>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:

Stan> We have been calling this "observer mode", and the customer's GDB
Stan> is actually configured to launch in this mode, to reduce the
Stan> chance of accidental stopping - the debugger user has to ask
Stan> specifically to change it to normal debugging mode.

It seems like a reasonable feature to me.

I can't really comment on the implementation approach.

Stan>   #include "gdbcmd.h"
Stan>   #include "value.h"
Stan>   #include "target.h"
Stan> + extern int may_insert_tracepoints;
Stan> + extern int may_insert_fast_tracepoints;
Stan>   #include "language.h"
Stan>   #include "gdb_string.h"
Stan>   #include "inferior.h"

The placement of these declarations seems rather odd.
I seem to keep running across this sort of thing for tracepoints.
I think a tracepoint-implementation-specific header file would be
cleaner.

Tom