This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC 2/5] Implement frame apply [all | COUNT | -COUNT] [-FLAGS...] COMMAND


On 2018-05-05 03:28 PM, Philippe Waroquiers wrote:
> Also implement the command 'faas COMMAND', a shortcut for
> 'frame apply all -s COMMAND'
> ---
>  gdb/stack.c | 239 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 214 insertions(+), 25 deletions(-)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index ecf1ee8379..61e86ab18b 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -777,7 +777,7 @@ do_gdb_disassembly (struct gdbarch *gdbarch,
>     
>     SRC_LINE: Print only source line.
>     LOCATION: Print only location.
> -   LOC_AND_SRC: Print location and source line.
> +   SRC_AND_LOC: Print location and source line.

Can you push this as a separate, obvious patch?

>  
>     Used in "where" output, and to emit breakpoint or step
>     messages.  */
> @@ -1687,6 +1687,39 @@ info_frame_command (const char *addr_exp, int from_tty)
>    }
>  }
>  
> +/* COUNT specifies the nr of outermost frames we are interested in.

nr -> number.  But actually I think the description is clear enough with only
the second sentence:

/* Return the starting frame needed to handle the COUNT outermost frames.  */

> +   trailing_outermost_frame returns the starting frame
> +   needed to handle COUNT outermost frames.  */
> +
> +static struct frame_info*

Space before *.

> +trailing_outermost_frame (int count)
> +{
> +  struct frame_info *current;
> +  struct frame_info *trailing;
> +
> +  trailing = get_current_frame ();
> +
> +  gdb_assert (count > 0);
> +
> +  current = trailing;
> +  while (current && count--)

current != nullptr

> +    {
> +      QUIT;
> +      current = get_prev_frame (current);
> +    }
> +
> +  /* Will stop when CURRENT reaches the top of the stack.
> +     TRAILING will be COUNT below it.  */
> +  while (current)

current != nullptr

> +    {
> +      QUIT;
> +      trailing = get_prev_frame (trailing);
> +      current = get_prev_frame (current);
> +    }
> +
> +  return trailing;
> +}
> +
>  /* Print briefly all stack frames or just the innermost COUNT_EXP
>     frames.  */
>  
> @@ -1696,7 +1729,6 @@ backtrace_command_1 (const char *count_exp, frame_filter_flags flags,
>  {
>    struct frame_info *fi;
>    int count;
> -  int i;
>    int py_start = 0, py_end = 0;
>    enum ext_lang_bt_status result = EXT_LANG_BT_ERROR;
>  
> @@ -1756,30 +1788,13 @@ backtrace_command_1 (const char *count_exp, frame_filter_flags flags,
>  
>        if (count_exp != NULL && count < 0)
>  	{
> -	  struct frame_info *current;
> -
> -	  count = -count;
> -
> -	  current = trailing;
> -	  while (current && count--)
> -	    {
> -	      QUIT;
> -	      current = get_prev_frame (current);
> -	    }
> -
> -	  /* Will stop when CURRENT reaches the top of the stack.
> -	     TRAILING will be COUNT below it.  */
> -	  while (current)
> -	    {
> -	      QUIT;
> -	      trailing = get_prev_frame (trailing);
> -	      current = get_prev_frame (current);
> -	    }
> -
> +	  trailing = trailing_outermost_frame (-count);
>  	  count = -1;
>  	}
> +      else
> +	trailing = get_current_frame ();

We now do

  trailing = get_current_frame ();

both before the if and in the else.  We just need one, I don't mind which one.

>  
> -      for (i = 0, fi = trailing; fi && count--; i++, fi = get_prev_frame (fi))
> +      for (fi = trailing; fi && count--; fi = get_prev_frame (fi))

Removing the i variable seems to be something you could push as an obvious patch.

>  	{
>  	  QUIT;
>  
> @@ -2506,9 +2521,148 @@ func_command (const char *arg, int from_tty)
>      select_and_print_frame (frame);
>  }
>  
> +/* Apply a GDB command to a all stack frames, or innermost COUNT frames.
> +   With a negative COUNT, apply command on outermost -COUNT frames.
> +
> +   frame apply 3 info frame     Apply 'info frame' to frames 0, 1, 2
> +   frame apply -3 info frame    Apply 'info frame' to outermost 3 frames.
> +   frame apply all x/i $pc      Apply 'x/i $pc' cmd to all frames.
> +   frame apply all -s p local_var_no_idea_in_which_frame
> +                If a frame has a local variable called
> +                local_var_no_idea_in_which_frame, print frame
> +                and value of local_var_no_idea_in_which_frame.
> +   frame apply all -sqq p local_var_no_idea_in_which_frame
> +                Same as before, but only print the variable value.  */
> +
> +/* Apply a GDB command to COUNT stack frames, starting at trailing.
> +   COUNT -1 means all frames starting at trailing.  WHICH_COMMAND is used

trailing -> TRAILING

> +   for error messages.  */
> +static void
> +frame_apply_command_count (const char* which_command,
> +			   const char *cmd, int from_tty,
> +			   struct frame_info *trailing, int count)
> +{
> +  int print_what_v = 2; /* Corresponding to LOCATION.  */
> +  enum print_what print_what[5] =
> +    {
> +      LOC_AND_ADDRESS, /* Should never be used, this is verbosity 0.  */
> +      SRC_LINE,
> +      LOCATION,
> +      LOC_AND_ADDRESS,
> +      SRC_AND_LOC
> +    };
> +  bool cont;
> +  bool silent;
> +  struct frame_info *fi;
> +
> +  if (cmd != NULL)
> +    check_for_flags_vqcs (which_command, &cmd,
> +			  &print_what_v, 4,
> +			  &cont, &silent);
> +
> +  if (cmd == NULL || *cmd == '\000')

'\0' ?

> +    error (_("Please specify a command to apply on the selected frames"));
> +
> +  for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
> +    {
> +      struct frame_id frame_id = get_frame_id (fi);
> +
> +      QUIT;
> +
> +      select_frame (fi);
> +      TRY
> +	{
> +	  std::string cmd_result = execute_command_to_string (cmd, from_tty);
> +	  if (!silent || cmd_result.length() > 0)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what[print_what_v], 0);
> +	      printf_filtered ("%s", cmd_result.c_str ());
> +	    }
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  if (!silent)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what [print_what_v], 0);
> +	      if (cont)
> +		printf_filtered ("%s\n", ex.message);
> +	      else
> +		throw_exception (ex);
> +	    }
> +	}
> +      END_CATCH;
> +
> +      /* execute_command_to_string might invalidate FI.  */
> +      fi = frame_find_by_id (frame_id);
> +      if (fi == NULL)
> +	{
> +	  trailing = NULL;
> +	  warning (_("Unable to restore previously selected frame."));
> +	  break;
> +	}
> +    }
> +}
> +
> +static void
> +frame_apply_all_command (const char *cmd, int from_tty)
> +{
> +  struct frame_info *fi;
> +  struct frame_info *trailing = NULL;
> +  int count;
> +  int i;

These variables are unused.

> +
> +  if (!target_has_stack)
> +    error (_("No stack."));
> +
> +  frame_apply_command_count ("frame apply all", cmd, from_tty,
> +			     get_current_frame(), INT_MAX);

Space after get_current_frame.

> +}
> +
> +/* Implementation of the "frame apply" command.  */
> +
> +static void
> +frame_apply_command (const char* cmd, int from_tty)
> +{
> +  int count;
> +  struct frame_info *trailing;
> +
> +  if (!target_has_stack)
> +    error (_("No stack."));
> +
> +  count = get_number_trailer (&cmd, 0);
> +
> +  if (count < 0)
> +    {
> +      trailing = trailing_outermost_frame (-count);
> +      count = -1;
> +    }
> +  else
> +    trailing = get_current_frame ();
> +
> +  frame_apply_command_count ("frame apply", cmd, from_tty,
> +			     trailing, count);
> +}

While testing this, I intuitively expected that this command would
work the same way as thread apply: take a frame number of frame number
range.  I had to think for a second when this did not do as I expected:

(gdb) bt
#0  break_here () at test.c:6
#1  0x000055555555461a in main (argc=1, argv=0x7fffffffde78) at test.c:12
(gdb) frame apply 0 print 123
(gdb) frame apply 1 print 123
#0  break_here () at test.c:6
$6 = 123

So I'm wondering whether it would make more sense to have both thread apply
and frame apply work in a similar way, to avoid confusing users.

> +
> +/* Implementation of the "faas" command.  */
> +
> +static void
> +faas_command (const char *cmd, int from_tty)
> +{
> +  std::string expanded = std::string ("frame apply all -s ") + std::string (cmd);
> +  execute_command (expanded.c_str (), from_tty);
> +}
> +
> +
> +/* Commands with a prefix of `frame'.  */
> +struct cmd_list_element *frame_cmd_list = NULL;
> +
>  void
>  _initialize_stack (void)
>  {
> +  static struct cmd_list_element *frame_apply_list = NULL;
> +
>    add_com ("return", class_stack, return_command, _("\
>  Make selected stack frame return to its caller.\n\
>  Control remains in the debugger, but when you continue\n\
> @@ -2531,14 +2685,49 @@ An argument says how many frames down to go."));
>  Same as the `down' command, but does not print anything.\n\
>  This is useful in command scripts."));
>  
> -  add_com ("frame", class_stack, frame_command, _("\
> +  add_prefix_cmd ("frame", class_stack, frame_command, _("\
>  Select and print a stack frame.\nWith no argument, \
>  print the selected stack frame.  (See also \"info frame\").\n\
>  An argument specifies the frame to select.\n\
> -It can be a stack frame number or the address of the frame."));
> +It can be a stack frame number or the address of the frame."),
> +		  &frame_cmd_list, "frame ", 1, &cmdlist);
>  
>    add_com_alias ("f", "frame", class_stack, 1);
>  
> +#define FRAME_APPLY_FLAGS_HELP "\
> +FLAGS letters are v(increase verbosity), q(decrease verbosity)\n\
> +  c(continue), s(silent).\n\
> +Verbosity (default 2) controls what to print for a frame:\n\
> +  0 : no frame info is printed\n\
> +  1 : source line\n\
> +  2 : location\n\
> +  3 : location and address\n\
> +  4 : source line and location\n\
> +By default, if a COMMAND raises an error, frame apply is aborted.\n\
> +Flag c indicates to print the error and continue.\n\
> +Flag s indicates to silently ignore a COMMAND that raises an error\n\
> +or produces no output."

I would prefer if this was a const char [] rather than a macro.

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]