[RFA,01/10] Rationalize "backtrace" command line parsing

Message ID 20170425194113.17862-2-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey April 25, 2017, 7:41 p.m. UTC
  The backtrace command has peculiar command-line parsing.  In
particular, it splits the command line, then loops over the arguments.
If it sees a word it recognizes, like "full", it effectively drops
this word from the argument vector.  Then, it pastes together the
remaining arguments, passing them on to backtrace_command_1, which in
turn passes the resulting string to parse_and_eval_long.

The documentation doesn't mention the parse_and_eval_long at all, so
it is a bit of a hidden feature that you can "bt 3*2".  The strange
algorithm above also means you can "bt 3 * no-filters 2" and get 6
frames...

This patch changes backtrace's command line parsing to be a bit more
rational.  Now, special words like "full" are only recognized at the
start of the command.

This also updates the documentation to describe the various bt options
individually.

ChangeLog
2017-04-25  Tom Tromey  <tom@tromey.com>

	* stack.c (backtrace_command): Rewrite command line parsing.

doc/ChangeLog
2017-04-25  Tom Tromey  <tom@tromey.com>

	* gdb.texinfo (Backtrace): Describe options individually.
---
 gdb/ChangeLog       |  4 +++
 gdb/doc/ChangeLog   |  4 +++
 gdb/doc/gdb.texinfo | 50 ++++++++++++++-----------------------
 gdb/stack.c         | 71 ++++++++++++++++++++---------------------------------
 4 files changed, 53 insertions(+), 76 deletions(-)
  

Comments

Eli Zaretskii April 26, 2017, 10:33 a.m. UTC | #1
> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Tue, 25 Apr 2017 13:41:04 -0600
> 
> The backtrace command has peculiar command-line parsing.  In
> particular, it splits the command line, then loops over the arguments.
> If it sees a word it recognizes, like "full", it effectively drops
> this word from the argument vector.  Then, it pastes together the
> remaining arguments, passing them on to backtrace_command_1, which in
> turn passes the resulting string to parse_and_eval_long.
> 
> The documentation doesn't mention the parse_and_eval_long at all, so
> it is a bit of a hidden feature that you can "bt 3*2".  The strange
> algorithm above also means you can "bt 3 * no-filters 2" and get 6
> frames...
> 
> This patch changes backtrace's command line parsing to be a bit more
> rational.  Now, special words like "full" are only recognized at the
> start of the command.

Do we really want to make backward-incompatible changes in such a
veteran command?

> +@code{backtrace} can accept some arguments:

This sentence will look erroneous in print, because it starts with a
lower-case letter.  Maybe use "The @code{backtrace} command can ..."
instead.

Also, you've removed the @table and the corresponding @item, so now
the description text describes something that was not yet called out:

  @node Backtrace
  @section Backtraces

  @cindex traceback
  @cindex call stack traces
  A backtrace is a summary of how your program got where it is.  It shows one
  line per frame, for many frames, starting with the currently executing
  frame (frame zero), followed by its caller (frame one), and on up the
  stack.

  @anchor{backtrace-command}
  @kindex backtrace
  @kindex bt @r{(@code{backtrace})}
  Print a backtrace of the entire stack, use the @code{backtrace}
  command, or its alias @code{bt}.  This command will print one line ...

Since @kindex leaves no traces in the manual, I think this will look
weird, don't you agree?

Thanks.
  
Pedro Alves June 27, 2017, 4:24 p.m. UTC | #2
I noticed now that this series is still pending.

On 04/25/2017 08:41 PM, Tom Tromey wrote:
> The backtrace command has peculiar command-line parsing.  In
> particular, it splits the command line, then loops over the arguments.
> If it sees a word it recognizes, like "full", it effectively drops
> this word from the argument vector.  Then, it pastes together the
> remaining arguments, passing them on to backtrace_command_1, which in
> turn passes the resulting string to parse_and_eval_long.
> 
> The documentation doesn't mention the parse_and_eval_long at all, so
> it is a bit of a hidden feature that you can "bt 3*2".  The strange
> algorithm above also means you can "bt 3 * no-filters 2" and get 6
> frames...
> 

Funny.

> This patch changes backtrace's command line parsing to be a bit more
> rational.  Now, special words like "full" are only recognized at the
> start of the command.
> 
> This also updates the documentation to describe the various bt options
> individually.
> 
> ChangeLog
> 2017-04-25  Tom Tromey  <tom@tromey.com>
> 
> 	* stack.c (backtrace_command): Rewrite command line parsing.

LGTM.

It might be good to clarify "help bt", to show something around:

  Usage: backtrace [QUALIFIERS]... COUNT


I'd support deprecating the existing non-hyphenated qualifiers,
and start supporting hyphenated options:

  bt -full -no-filters -whatever-new-option -- COUNT

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 90ce6ae..48f2063 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2017-04-25  Tom Tromey  <tom@tromey.com>
+
+	* stack.c (backtrace_command): Rewrite command line parsing.
+
 2017-04-25  Yao Qi  <yao.qi@linaro.org>
 
 	* aarch64-tdep.c (aarch64_gdbarch_init): Don't call
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 9be035c..cf2226c 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@ 
+2017-04-25  Tom Tromey  <tom@tromey.com>
+
+	* gdb.texinfo (Backtrace): Describe options individually.
+
 2017-04-21  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* gdb.texinfo (GDB/MI Thread Information): Add missing
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f2e6156..a9f12fd 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7199,39 +7199,27 @@  frame (frame zero), followed by its caller (frame one), and on up the
 stack.
 
 @anchor{backtrace-command}
-@table @code
 @kindex backtrace
 @kindex bt @r{(@code{backtrace})}
-@item backtrace
-@itemx bt
-Print a backtrace of the entire stack: one line per frame for all
-frames in the stack.
-
-You can stop the backtrace at any time by typing the system interrupt
-character, normally @kbd{Ctrl-c}.
-
-@item backtrace @var{n}
-@itemx bt @var{n}
-Similar, but print only the innermost @var{n} frames.
-
-@item backtrace -@var{n}
-@itemx bt -@var{n}
-Similar, but print only the outermost @var{n} frames.
-
-@item backtrace full
-@itemx bt full
-@itemx bt full @var{n}
-@itemx bt full -@var{n}
-Print the values of the local variables also.  As described above,
-@var{n} specifies the number of frames to print.
-
-@item backtrace no-filters
-@itemx bt no-filters
-@itemx bt no-filters @var{n}
-@itemx bt no-filters -@var{n}
-@itemx bt no-filters full
-@itemx bt no-filters full @var{n}
-@itemx bt no-filters full -@var{n}
+Print a backtrace of the entire stack, use the @code{backtrace}
+command, or its alias @code{bt}.  This command will print one line per
+frame for all frames in the stack.  You can stop the backtrace at any
+time by typing the system interrupt character, normally @kbd{Ctrl-c}.
+@code{backtrace} can accept some arguments:
+
+@table @code
+@item @var{n}
+@itemx @var{n}
+Print only the innermost @var{n} frames.
+
+@item -@var{n}
+@itemx -@var{n}
+Print only the outermost @var{n} frames.
+
+@item full
+Print the values of the local variables also.
+
+@item no-filters
 Do not run Python frame filters on this backtrace.  @xref{Frame
 Filter API}, for more information.  Additionally use @ref{disable
 frame-filter all} to turn off all frame filters.  This is only
diff --git a/gdb/stack.c b/gdb/stack.c
index 7f8a51c..c2fc3f1 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1868,65 +1868,46 @@  backtrace_command_1 (char *count_exp, int show_locals, int no_filters,
 static void
 backtrace_command (char *arg, int from_tty)
 {
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
-  int fulltrace_arg = -1, arglen = 0, argc = 0, no_filters  = -1;
-  int user_arg = 0;
+  bool fulltrace = false;
+  bool filters = true;
 
   if (arg)
     {
-      char **argv;
-      int i;
+      bool done = false;
 
-      argv = gdb_buildargv (arg);
-      make_cleanup_freeargv (argv);
-      argc = 0;
-      for (i = 0; argv[i]; i++)
+      while (!done)
 	{
 	  unsigned int j;
+	  char *this_arg = extract_arg (&arg);
 
-	  for (j = 0; j < strlen (argv[i]); j++)
-	    argv[i][j] = TOLOWER (argv[i][j]);
+	  if (this_arg == NULL)
+	    break;
+
+	  struct cleanup *arg_cleanup = make_cleanup (xfree, this_arg);
+
+	  for (j = 0; j < strlen (this_arg); j++)
+	    this_arg[j] = TOLOWER (this_arg[j]);
 
-	  if (no_filters < 0 && subset_compare (argv[i], "no-filters"))
-	    no_filters = argc;
+	  if (subset_compare (this_arg, "no-filters"))
+	    filters = false;
+	  else if (subset_compare (this_arg, "full"))
+	    fulltrace = true;
 	  else
 	    {
-	      if (fulltrace_arg < 0 && subset_compare (argv[i], "full"))
-		fulltrace_arg = argc;
-	      else
-		{
-		  user_arg++;
-		  arglen += strlen (argv[i]);
-		}
+	      /* Not a recognized argument, so stop.  */
+	      done = true;
 	    }
-	  argc++;
-	}
-      arglen += user_arg;
-      if (fulltrace_arg >= 0 || no_filters >= 0)
-	{
-	  if (arglen > 0)
-	    {
-	      arg = (char *) xmalloc (arglen + 1);
-	      make_cleanup (xfree, arg);
-	      arg[0] = 0;
-	      for (i = 0; i < argc; i++)
-		{
-		  if (i != fulltrace_arg && i != no_filters)
-		    {
-		      strcat (arg, argv[i]);
-		      strcat (arg, " ");
-		    }
-		}
-	    }
-	  else
-	    arg = NULL;
+
+	  do_cleanups (arg_cleanup);
 	}
-    }
 
-  backtrace_command_1 (arg, fulltrace_arg >= 0 /* show_locals */,
-		       no_filters >= 0 /* no frame-filters */, from_tty);
+      arg = skip_spaces (arg);
+      if (*arg == '\0')
+	arg = NULL;
+    }
 
-  do_cleanups (old_chain);
+  backtrace_command_1 (arg, fulltrace /* show_locals */,
+		       !filters /* no frame-filters */, from_tty);
 }
 
 /* Iterate over the local variables of a block B, calling CB with