[PATCH 0/3] Improve disassemble command completion

[PATCH 0/3] Improve disassemble command completion

[Andrew Burgess]

I wanted better tab completion for the disassemble command.

---

Andrew Burgess (3):
  gdb: error if /r and /b are used with disassemble command
  gdb: make skip_over_slash_fmt available outside printcmd.c
  gdb: add a custom command completer for disassemble command

 gdb/NEWS                              |  7 +++
 gdb/cli/cli-cmds.c                    | 20 ++++++++-
 gdb/completer.c                       | 53 ++++++++++++++++++++++
 gdb/completer.h                       | 17 +++++++
 gdb/doc/gdb.texinfo                   | 20 ++++++---
 gdb/printcmd.c                        | 65 ---------------------------
 gdb/testsuite/gdb.base/completion.exp | 30 +++++++++++++
 gdb/testsuite/gdb.disasm/basics.c     | 22 +++++++++
 gdb/testsuite/gdb.disasm/basics.exp   | 43 ++++++++++++++++++
 9 files changed, 205 insertions(+), 72 deletions(-)
 create mode 100644 gdb/testsuite/gdb.disasm/basics.c
 create mode 100644 gdb/testsuite/gdb.disasm/basics.exp


base-commit: 1c37b30945073f34bbb685d2ac47ab01e0c93d45
-- 
2.25.4

[PATCH 1/3] gdb: error if /r and /b are used with disassemble command

[Andrew Burgess]

The disassembler gained a new /b flag in this commit:

  commit d4ce49b7ac077a9882d6a5e689e260300045ca88
  Date:   Tue Jun 21 20:23:35 2022 +0100

      gdb: disassembler opcode display formatting

The /b and /r flags result in the instruction opcodes displayed in
different formats, so it's not possible to have both at the same
time.  Currently the /b flag overrides the /r flag.

We have a similar situation with the /m and /s flags, but here, if the
user tries to use both flags then they will get an error.

I think the error is clearer, so in this commit I propose that we add
an error if /r and /b are both used.

Obviously this change breaks backwards compatibility.  I don't have a
compelling argument for why we should make the change beyond my
feeling that it was a mistake not to add this error from the start,
and that the new behaviour is better.
---
 gdb/NEWS                            |  7 ++++++
 gdb/cli/cli-cmds.c                  |  4 +++
 gdb/doc/gdb.texinfo                 | 20 ++++++++++-----
 gdb/testsuite/gdb.disasm/basics.c   | 22 ++++++++++++++++
 gdb/testsuite/gdb.disasm/basics.exp | 39 +++++++++++++++++++++++++++++
 5 files changed, 86 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.disasm/basics.c
 create mode 100644 gdb/testsuite/gdb.disasm/basics.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 08d779010f0..4dd00d6fa01 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -6,6 +6,13 @@
 * GDB index now contains information about the main function. This speeds up
   startup when it is being used for some large binaries.
 
+* Changed commands
+
+disassemble
+  Attempting to use both the 'r' and 'b' flags with the disassemble
+  command will now give an error.  Previously the 'b' flag would
+  always override the 'r' flag.
+
 * Python API
 
   ** New function gdb.notify_mi(NAME, DATA), that emits custom
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 8cadd637151..90989588bab 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1644,6 +1644,10 @@ disassemble_command (const char *arg, int from_tty)
       == (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
     error (_("Cannot specify both /m and /s."));
 
+  if ((flags & (DISASSEMBLY_RAW_INSN | DISASSEMBLY_RAW_BYTES))
+      == (DISASSEMBLY_RAW_INSN | DISASSEMBLY_RAW_BYTES))
+    error (_("Cannot specify both /r and /b."));
+
   if (! p || ! *p)
     {
       flags |= DISASSEMBLY_OMIT_FNAME;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index db1a82ec838..2cd565ed5b4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10060,12 +10060,20 @@
 instructions.  It can also print mixed source+disassembly by specifying
 the @code{/m} or @code{/s} modifier and print the raw instructions in
 hex as well as in symbolic form by specifying the @code{/r} or @code{/b}
-modifier.  The default memory range is the function surrounding the
-program counter of the selected frame.  A single argument to this
-command is a program counter value; @value{GDBN} dumps the function
-surrounding this value.  When two arguments are given, they should be
-separated by a comma, possibly surrounded by whitespace.  The arguments
-specify a range of addresses to dump, in one of two forms:
+modifier.
+
+Only one of @code{/m} and @code{/s} can be used, attempting to use
+both flag will give an error.
+
+Only one of @code{/r} and @code{/b} can be used, attempting to use
+both flag will give an error.
+
+The default memory range is the function surrounding the program
+counter of the selected frame.  A single argument to this command is a
+program counter value; @value{GDBN} dumps the function surrounding
+this value.  When two arguments are given, they should be separated by
+a comma, possibly surrounded by whitespace.  The arguments specify a
+range of addresses to dump, in one of two forms:
 
 @table @code
 @item @var{start},@var{end}
diff --git a/gdb/testsuite/gdb.disasm/basics.c b/gdb/testsuite/gdb.disasm/basics.c
new file mode 100644
index 00000000000..002b411fd37
--- /dev/null
+++ b/gdb/testsuite/gdb.disasm/basics.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.disasm/basics.exp b/gdb/testsuite/gdb.disasm/basics.exp
new file mode 100644
index 00000000000..78df18d9e96
--- /dev/null
+++ b/gdb/testsuite/gdb.disasm/basics.exp
@@ -0,0 +1,39 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Some basic tests of the disassemble command.  Tests in this script
+# should be architecture independent.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile ${srcfile}] == -1 } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Check the '/s' and '/m' flags can't be used together.
+gdb_test "disassemble /sm main" \
+    "Cannot specify both /m and /s\\."
+gdb_test "disassemble /ms main" \
+    "Cannot specify both /m and /s\\."
+
+# Check the '/r' and '/b' flags can't be used together.
+gdb_test "disassemble /rb main" \
+    "Cannot specify both /r and /b\\."
+gdb_test "disassemble /br main" \
+    "Cannot specify both /r and /b\\."
-- 
2.25.4

[PATCH 2/3] gdb: make skip_over_slash_fmt available outside printcmd.c

[Andrew Burgess]

Move the function skip_over_slash_fmt into completer.c, and make it
extern, with a declaration in completer.h.

This is a refactor in order to support the next commit.  I've not
changed any of the code in skip_over_slash_fmt.

There should be no user visible changes after this commit.
---
 gdb/completer.c | 53 ++++++++++++++++++++++++++++++++++++++++
 gdb/completer.h | 17 +++++++++++++
 gdb/printcmd.c  | 65 -------------------------------------------------
 3 files changed, 70 insertions(+), 65 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 2abf3998345..42ac7be297a 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2991,6 +2991,59 @@ gdb_display_match_list (char **matches, int len, int max,
     }
 }
 
+/* See completer.h.  */
+
+bool
+skip_over_slash_fmt (completion_tracker &tracker, const char **args)
+{
+  const char *text = *args;
+
+  if (text[0] == '/')
+    {
+      bool in_fmt;
+      tracker.set_use_custom_word_point (true);
+
+      if (text[1] == '\0')
+	{
+	  /* The user tried to complete after typing just the '/' character
+	     of the /FMT string.  Step the completer past the '/', but we
+	     don't offer any completions.  */
+	  in_fmt = true;
+	  ++text;
+	}
+      else
+	{
+	  /* The user has typed some characters after the '/', we assume
+	     this is a complete /FMT string, first skip over it.  */
+	  text = skip_to_space (text);
+
+	  if (*text == '\0')
+	    {
+	      /* We're at the end of the input string.  The user has typed
+		 '/FMT' and asked for a completion.  Push an empty
+		 completion string, this will cause readline to insert a
+		 space so the user now has '/FMT '.  */
+	      in_fmt = true;
+	      tracker.add_completion (make_unique_xstrdup (text));
+	    }
+	  else
+	    {
+	      /* The user has already typed things after the /FMT, skip the
+		 whitespace and return false.  Whoever called this function
+		 should then try to complete what comes next.  */
+	      in_fmt = false;
+	      text = skip_spaces (text);
+	    }
+	}
+
+      tracker.advance_custom_word_point_by (text - *args);
+      *args = text;
+      return in_fmt;
+    }
+
+  return false;
+}
+
 void _initialize_completer ();
 void
 _initialize_completer ()
diff --git a/gdb/completer.h b/gdb/completer.h
index 67d2fbf9d38..d96c4b515d3 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -657,6 +657,23 @@ extern const char *skip_quoted_chars (const char *, const char *,
 
 extern const char *skip_quoted (const char *);
 
+/* Called from command completion function to skip over /FMT
+   specifications, allowing the rest of the line to be completed.  Returns
+   true if the /FMT is at the end of the current line and there is nothing
+   left to complete, otherwise false is returned.
+
+   In either case *ARGS can be updated to point after any part of /FMT that
+   is present.
+
+   This function is designed so that trying to complete '/' will offer no
+   completions, the user needs to insert the format specification
+   themselves.  Trying to complete '/FMT' (where FMT is any non-empty set
+   of alpha-numeric characters) will cause readline to insert a single
+   space, setting the user up to enter the expression.  */
+
+extern bool skip_over_slash_fmt (completion_tracker &tracker,
+				 const char **args);
+
 /* Maximum number of candidates to consider before the completer
    bails by throwing MAX_COMPLETIONS_REACHED_ERROR.  Negative values
    disable limiting.  */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 5e9c8a5b222..fd6e74ec61c 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1378,71 +1378,6 @@ print_command_1 (const char *args, int voidprint)
     }
 }
 
-/* Called from command completion function to skip over /FMT
-   specifications, allowing the rest of the line to be completed.  Returns
-   true if the /FMT is at the end of the current line and there is nothing
-   left to complete, otherwise false is returned.
-
-   In either case *ARGS can be updated to point after any part of /FMT that
-   is present.
-
-   This function is designed so that trying to complete '/' will offer no
-   completions, the user needs to insert the format specification
-   themselves.  Trying to complete '/FMT' (where FMT is any non-empty set
-   of alpha-numeric characters) will cause readline to insert a single
-   space, setting the user up to enter the expression.  */
-
-static bool
-skip_over_slash_fmt (completion_tracker &tracker, const char **args)
-{
-  const char *text = *args;
-
-  if (text[0] == '/')
-    {
-      bool in_fmt;
-      tracker.set_use_custom_word_point (true);
-
-      if (text[1] == '\0')
-	{
-	  /* The user tried to complete after typing just the '/' character
-	     of the /FMT string.  Step the completer past the '/', but we
-	     don't offer any completions.  */
-	  in_fmt = true;
-	  ++text;
-	}
-      else
-	{
-	  /* The user has typed some characters after the '/', we assume
-	     this is a complete /FMT string, first skip over it.  */
-	  text = skip_to_space (text);
-
-	  if (*text == '\0')
-	    {
-	      /* We're at the end of the input string.  The user has typed
-		 '/FMT' and asked for a completion.  Push an empty
-		 completion string, this will cause readline to insert a
-		 space so the user now has '/FMT '.  */
-	      in_fmt = true;
-	      tracker.add_completion (make_unique_xstrdup (text));
-	    }
-	  else
-	    {
-	      /* The user has already typed things after the /FMT, skip the
-		 whitespace and return false.  Whoever called this function
-		 should then try to complete what comes next.  */
-	      in_fmt = false;
-	      text = skip_spaces (text);
-	    }
-	}
-
-      tracker.advance_custom_word_point_by (text - *args);
-      *args = text;
-      return in_fmt;
-    }
-
-  return false;
-}
-
 /* See valprint.h.  */
 
 void
-- 
2.25.4

[PATCH 3/3] gdb: add a custom command completer for disassemble command

[Andrew Burgess]

Add a new command completer function for the disassemble command.
There are two things that this completion function changes.  First,
after the previous commit, the new function calls skip_over_slash_fmt,
which means that hitting tab after entering a /OPT flag now inserts a
space ready to start typing the address to disassemble at:

  (gdb) disassemble /r<TAB>
  (gdb) disassemble /r <CURSOR>

But also, we now get symbol completion after a /OPT option set,
previously this would do nothing:

  (gdb) disassemble /r mai<TAB>

But now:

  (gdb) disassemble /r mai<TAB>
  (gdb) disassemble /r main <CURSOR>

Which was my main motivation for working on this commit.

However, I have made a second change in the completion function.
Currently, the disassemble command calls the generic
location_completer function, however, the disassemble docs say:

     Note that the 'disassemble' command's address arguments are specified
  using expressions in your programming language (*note Expressions:
  Expressions.), not location specs (*note Location Specifications::).
  So, for example, if you want to disassemble function 'bar' in file
  'foo.c', you must type 'disassemble 'foo.c'::bar' and not 'disassemble
  foo.c:bar'.

And indeed, if I try:

  (gdb) disassemble hello.c:main
  No symbol "hello" in current context.
  (gdb) disassemble hello.c::main
  No symbol "hello" in current context.
  (gdb) disassemble 'hello.c'::main
  Dump of assembler code for function main:
  ... snip ...

But, if I do this:

  (gdb) disassemble hell<TAB>
  (gdb) disassemble hello.c:<CURSOR>

which is a consequence of using the location_completer function.  So
in this commit, after calling skip_over_slash_fmt, I forward the bulk
of the disassemble command completion to expression_completer.  Now
when I try this:

  (gdb) disassemble hell<TAB>

gives nothing, which I think is an improvement.  There is one slight
disappointment, if I do:

  (gdb) disassemble 'hell<TAB>

I still get nothing.  I had hoped that this would expand to:
'hello.c':: but I guess this is a limitation of the current
expression_completer implementation, however, I don't think this is a
regression, the previous expansion was just wrong.  Fixing
expression_completer is out of scope for this commit.

I've added some disassembler command completion tests, and also a test
that disassembling using 'FILE'::FUNC syntax works, as I don't think
that is tested anywhere.
---
 gdb/cli/cli-cmds.c                    | 16 +++++++++++++-
 gdb/testsuite/gdb.base/completion.exp | 30 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.disasm/basics.exp   |  4 ++++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 90989588bab..fd93e5b4ccb 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1698,6 +1698,20 @@ disassemble_command (const char *arg, int from_tty)
   print_disassembly (gdbarch, name, low, high, block, flags);
 }
 
+/* Command completion for the disassemble command.  */
+
+static void
+disassemble_command_completer (struct cmd_list_element *ignore,
+			       completion_tracker &tracker,
+			       const char *text, const char * /* word */)
+{
+  if (skip_over_slash_fmt (tracker, &text))
+    return;
+
+  const char *word = advance_to_expression_complete_word_point (tracker, text);
+  expression_completer (ignore, tracker, text, word);
+}
+
 static void
 make_command (const char *arg, int from_tty)
 {
@@ -2855,7 +2869,7 @@ Note that the address is interpreted as an expression, not as a location\n\
 like in the \"break\" command.\n\
 So, for example, if you want to disassemble function bar in file foo.c\n\
 you must type \"disassemble 'foo.c'::bar\" and not \"disassemble foo.c:bar\"."));
-  set_cmd_completer (c, location_completer);
+  set_cmd_completer_handle_brkchars (c, disassemble_command_completer);
 
   c = add_com ("make", class_support, make_command, _("\
 Run the ``make'' program using the rest of the line as arguments."));
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 4686e6f8f34..15937810d2f 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -975,3 +975,33 @@ test_gdb_complete_unique "xxx_yyy_" "xxx_yyy_zzz"
 gdb_test_no_output "alias set aaa_bbb_ccc=set debug"
 gdb_test_no_output "maint deprecate set aaa_bbb_ccc"
 test_gdb_complete_unique "set aaa_bbb_" "set aaa_bbb_ccc"
+
+# Test command completion for the disassemble command.
+test_gdb_complete_unique \
+    "disassemble multi_line_if" \
+    "disassemble multi_line_if_conditional"
+
+test_gdb_complete_multiple "disassemble " \
+    "multi_li" "ne_" {
+	"multi_line_if_conditional"
+	"multi_line_while_conditional"
+    }
+
+foreach_with_prefix spc { " " "" } {
+    test_gdb_complete_none "disassemble${spc}/"
+
+    foreach_with_prefix flg { "r" "b" "rb" "s" "m" "ms" } {
+	test_gdb_complete_unique "disassemble${spc}/${flg}" \
+	    "disassemble${spc}/${flg}"
+
+	test_gdb_complete_unique \
+	    "disassemble${spc}/${flg} multi_line_if" \
+	    "disassemble${spc}/${flg} multi_line_if_conditional"
+
+	test_gdb_complete_multiple "disassemble${spc}/${flg} " \
+	    "multi_li" "ne_" {
+		"multi_line_if_conditional"
+		"multi_line_while_conditional"
+	    }
+    }
+}
diff --git a/gdb/testsuite/gdb.disasm/basics.exp b/gdb/testsuite/gdb.disasm/basics.exp
index 78df18d9e96..3dfc16e04cf 100644
--- a/gdb/testsuite/gdb.disasm/basics.exp
+++ b/gdb/testsuite/gdb.disasm/basics.exp
@@ -37,3 +37,7 @@ gdb_test "disassemble /rb main" \
     "Cannot specify both /r and /b\\."
 gdb_test "disassemble /br main" \
     "Cannot specify both /r and /b\\."
+
+# Check disassembly using 'FILE'::FUNC syntax.
+gdb_test "disassemble '$srcfile'::main" \
+    "Dump of assembler code for function main:.*End of assembler dump\\."
-- 
2.25.4

Re: [PATCH 1/3] gdb: error if /r and /b are used with disassemble command

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 20 Oct 2023 22:33:09 +0100
> 
> The disassembler gained a new /b flag in this commit:
> 
>   commit d4ce49b7ac077a9882d6a5e689e260300045ca88
>   Date:   Tue Jun 21 20:23:35 2022 +0100
> 
>       gdb: disassembler opcode display formatting
> 
> The /b and /r flags result in the instruction opcodes displayed in
> different formats, so it's not possible to have both at the same
> time.  Currently the /b flag overrides the /r flag.
> 
> We have a similar situation with the /m and /s flags, but here, if the
> user tries to use both flags then they will get an error.
> 
> I think the error is clearer, so in this commit I propose that we add
> an error if /r and /b are both used.
> 
> Obviously this change breaks backwards compatibility.  I don't have a
> compelling argument for why we should make the change beyond my
> feeling that it was a mistake not to add this error from the start,
> and that the new behaviour is better.
> ---
>  gdb/NEWS                            |  7 ++++++
>  gdb/cli/cli-cmds.c                  |  4 +++
>  gdb/doc/gdb.texinfo                 | 20 ++++++++++-----
>  gdb/testsuite/gdb.disasm/basics.c   | 22 ++++++++++++++++
>  gdb/testsuite/gdb.disasm/basics.exp | 39 +++++++++++++++++++++++++++++
>  5 files changed, 86 insertions(+), 6 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.disasm/basics.c
>  create mode 100644 gdb/testsuite/gdb.disasm/basics.exp

Thanks, the documentation parts are OK.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 0/3] Improve disassemble command completion

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> I wanted better tab completion for the disassemble command.

I've gone ahead and checked this series in.

Thanks,
Andrew

>
> ---
>
> Andrew Burgess (3):
>   gdb: error if /r and /b are used with disassemble command
>   gdb: make skip_over_slash_fmt available outside printcmd.c
>   gdb: add a custom command completer for disassemble command
>
>  gdb/NEWS                              |  7 +++
>  gdb/cli/cli-cmds.c                    | 20 ++++++++-
>  gdb/completer.c                       | 53 ++++++++++++++++++++++
>  gdb/completer.h                       | 17 +++++++
>  gdb/doc/gdb.texinfo                   | 20 ++++++---
>  gdb/printcmd.c                        | 65 ---------------------------
>  gdb/testsuite/gdb.base/completion.exp | 30 +++++++++++++
>  gdb/testsuite/gdb.disasm/basics.c     | 22 +++++++++
>  gdb/testsuite/gdb.disasm/basics.exp   | 43 ++++++++++++++++++
>  9 files changed, 205 insertions(+), 72 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.disasm/basics.c
>  create mode 100644 gdb/testsuite/gdb.disasm/basics.exp
>
>
> base-commit: 1c37b30945073f34bbb685d2ac47ab01e0c93d45
> -- 
> 2.25.4