[RFC] mi: add -a option to the "-data-disassemble" command

Message ID 20180727075447.16243-1-jan.vrany@fit.cvut.cz
State Superseded
Headers show
Series
  • [RFC] mi: add -a option to the "-data-disassemble" command
Related show

Commit Message

Jan Vrany July 27, 2018, 7:54 a.m.
A CLI command "disassemble" allows use to specify a single
address - in that case function surrounding that address is
disassembled.

This commit adds this feature to MI command "-data-disassemble".

gdb/ChangeLog:

	* mi/mi-cmd-disas.c (mi_cmd_disassemble): Added -a option.
	If used, use find_pc_partial_function to find address range
	to disassemble.
	* mi/mi-main.c (mi/mi-main.c): Add new feature
	data-disassemble-a-option to indicate that -data-disassemble
	supports -a.
	* NEWS: Mention new -data-disassemble option -a

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Data Manipulation): Document
	"-data-disassemble -a addr".
	(GDB/MI Support Commands): Document data-disassemble-a-option
	feature

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-disassemble.exp (test_disassembly_only): Added
	tests for -data-disassemble -a.
	(test_disassembly_bogus_args): Likewise.
---
 gdb/ChangeLog                           | 10 +++++++++
 gdb/NEWS                                |  3 +++
 gdb/doc/ChangeLog                       |  7 +++++++
 gdb/doc/gdb.texinfo                     |  9 ++++++++
 gdb/mi/mi-cmd-disas.c                   | 28 ++++++++++++++++++-------
 gdb/mi/mi-main.c                        |  1 +
 gdb/testsuite/ChangeLog                 |  6 ++++++
 gdb/testsuite/gdb.mi/mi-disassemble.exp | 20 ++++++++++++++++--
 8 files changed, 75 insertions(+), 9 deletions(-)

-- 
2.18.0

Comments

Tom Tromey July 30, 2018, 3:04 p.m. | #1
>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:


Jan> A CLI command "disassemble" allows use to specify a single
Jan> address - in that case function surrounding that address is
Jan> disassembled.

Jan>  	  high = parse_and_eval_address (oarg);
Jan>  	  end_seen = 1;
Jan>  	  break;
Jan> +        case ADDR_OPT:
Jan> +          addr = parse_and_eval_address (oarg);
Jan> +          addr_seen = 1;
Jan> +          break;

The indentation looked slightly off here.
It could just be how the patch looked here, but could you double-check
just in case?

Jan> +  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen && !addr_seen)
Jan> +	|| (line_seen && file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)
Jan> +	|| (!line_seen && !file_seen && !num_seen && start_seen && end_seen && !addr_seen)
Jan> +	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen && addr_seen)))

I suppose this ought to check for the case where either -s or -e is
given along with -a.  That seems like an error.

This "if" made me laugh.

Jan> +    mi_gdb_test "112-data-disassemble -a \$pc -- 0" \
Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
Jan> +             "data-disassemble function around pc assembly only"
Jan> +
Jan> +    mi_gdb_test "112-data-disassemble -a callee4 -- 0" \
Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
Jan> +             "data-disassemble function callee4 assembly only"

Probably the second "112" should be "113".  I don't know if you can
reuse tokens or not, but it seems simple and safe not to.

Thanks for doing this.  It seems like a good addition to me.

Tom
Jan Vrany July 31, 2018, 10:23 a.m. | #2
On Mon, 2018-07-30 at 09:04 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

> 

> Jan> A CLI command "disassemble" allows use to specify a single

> Jan> address - in that case function surrounding that address is

> Jan> disassembled.

> 

> Jan>  	  high = parse_and_eval_address (oarg);

> Jan>  	  end_seen = 1;

> Jan>  	  break;

> Jan> +        case ADDR_OPT:

> Jan> +          addr = parse_and_eval_address (oarg);

> Jan> +          addr_seen = 1;

> Jan> +          break;

> 

> The indentation looked slightly off here.

> It could just be how the patch looked here, but could you double-check

> just in case?


My fault. My editor put 8 spaces instead of tab (\t) at the beginning of
my lines. Will fix. 

> 

> Jan> +  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen && !addr_seen)

> Jan> +	|| (line_seen && file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)

> Jan> +	|| (!line_seen && !file_seen && !num_seen && start_seen && end_seen && !addr_seen)

> Jan> +	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen && addr_seen)))

> 

> I suppose this ought to check for the case where either -s or -e is

> given along with -a.  That seems like an error.

> 


I'm sorry I'm confused. Let me try to explain. There are (now) four forms:

1a) -f filename -l linenum 
1b) -f filename -l linenum -n lines 
 2) -s start-addr -e end-addr
 3) -a addr

Command  must have one of the above four forms, otherwise it's invalid. Each
"line" in the code above checks one of the above form (in order as written here). 
Note, that there's negation at the very beginning, so the condition holds 
if the command has none of the four forms. Seems "good" to me. Makes sense? 

Perhaps it's easier to see with little reformatting (hope email won't screw it):

  if (!   (( line_seen &&  file_seen &&  num_seen && !start_seen && !end_seen && !addr_seen)
	|| ( line_seen &&  file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)
	|| (!line_seen && !file_seen && !num_seen &&  start_seen &&  end_seen && !addr_seen)
	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen &&  addr_seen)))
    error (_("-data-disassemble: Usage: ( [-f filename -l linenum [-n "
	     "howmany]] | [-s startaddr -e endaddr] | [-a addr] ) [--] mode."));

Now, it appears to me that first two lines can be merged as forms 1a and 1b differ only
in presence of lines. Also, I forgot to update the comment above. So, what about:

   /* Allow only filename + linenum (with how_many which is not
-     required) OR start_addr + end_addr.  */
+     required) OR start_addr + end_addr OR addr  */
 
-  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen)
-       || (line_seen && file_seen && !num_seen && !start_seen && !end_seen)
-       || (!line_seen && !file_seen && !num_seen && start_seen && end_seen)))
+  if (!(  ( line_seen &&  file_seen &&              !start_seen && !end_seen && !addr_seen)
+       || (!line_seen && !file_seen && !num_seen &&  start_seen &&  end_seen && !addr_seen)
+       || (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen &&  addr_seen)))

this passes mi-disassemble.exp on my machine. 


> This "if" made me laugh.


:-) I cannot say I was exactly laughing. But followed the crowed anyway.

> 

> Jan> +    mi_gdb_test "112-data-disassemble -a \$pc -- 0" \

> Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}.*\]" \

> Jan> +             "data-disassemble function around pc assembly only"

> Jan> +

> Jan> +    mi_gdb_test "112-data-disassemble -a callee4 -- 0" \

> Jan> +	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\}.*\]" 

> \

> Jan> +             "data-disassemble function callee4 assembly only"

> 

> Probably the second "112" should be "113".  I don't know if you can

> reuse tokens or not, but it seems simple and safe not to.


Sure, will fix and send a patch once we agree on the "if". 

Thanks, Jan
Tom Tromey July 31, 2018, 3:48 p.m. | #3
>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:


>> The indentation looked slightly off here.

>> It could just be how the patch looked here, but could you double-check

>> just in case?


Jan> My fault. My editor put 8 spaces instead of tab (\t) at the beginning of
Jan> my lines. Will fix. 

Thanks.  According to .dir-locals.el, tabs are fine, assuming things
line up properly.  (But there's been talk over the years of switching
purely to spaces, and IIRC using only spaces is also fine.)

Jan> I'm sorry I'm confused. Let me try to explain. There are (now) four forms:

Jan> 1a) -f filename -l linenum 
Jan> 1b) -f filename -l linenum -n lines 
Jan>  2) -s start-addr -e end-addr
Jan>  3) -a addr

Jan> Command  must have one of the above four forms, otherwise it's invalid. Each
Jan> "line" in the code above checks one of the above form (in order as written here). 
Jan> Note, that there's negation at the very beginning, so the condition holds 
Jan> if the command has none of the four forms. Seems "good" to me. Makes sense? 

Yes, thanks for explaining this.  I was misreading the new code here.

Jan> Now, it appears to me that first two lines can be merged as forms 1a and 1b differ only
Jan> in presence of lines. Also, I forgot to update the comment above. So, what about:
[...]

Thank you, that seems like a good improvement.

Tom

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0f601bdbf0..53a417706f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@ 
+2018-07-05  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmd-disas.c (mi_cmd_disassemble): Added -a option.
+	If used, use find_pc_partial_function to find address range
+	to disassemble.
+	* mi/mi-main.c (mi/mi-main.c): Add new feature
+	data-disassemble-a-option to indicate that -data-disassemble
+	supports -a.
+	* NEWS: Mention new -data-disassemble option -a
+
 2018-07-02  Sebastian Huber  <sebastian.huber@embedded-brains.de>
 
 	* riscv-tdep.c (riscv_register_aliases): Swap "fp" and "s0" entries.
diff --git a/gdb/NEWS b/gdb/NEWS
index 839466e7e0..19f353495b 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@ 
 
 *** Changes since GDB 8.1
 
+* The '-data-disassemble' MI command now accepts the '-a' option to disassemble
+  the whole function surrounding given program counter value or function name.
+
 * The 'symbol-file' command now accepts an '-o' option to add a relative
   offset to all sections.
 
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 6954398e43..f52074f2df 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,10 @@ 
+2018-07-05  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.texinfo (GDB/MI Data Manipulation): Document
+	"-data-disassemble -a addr".
+	(GDB/MI Support Commands): Document data-disassemble-a-option
+	feature
+
 2018-06-28  Petr Tesarik  <ptesarik@suse.cz>
 
 	* gdb.texinfo (Files): Document "add-symbol-file -o offset".
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7fb6ac5636..47acf6cdd0 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30947,6 +30947,7 @@  For details about what an addressable memory unit is,
 @smallexample
  -data-disassemble
     [ -s @var{start-addr} -e @var{end-addr} ]
+  | [ -a @var{addr} ]
   | [ -f @var{filename} -l @var{linenum} [ -n @var{lines} ] ]
   -- @var{mode}
 @end smallexample
@@ -30959,6 +30960,11 @@  Where:
 is the beginning address (or @code{$pc})
 @item @var{end-addr}
 is the end address
+@item @var{addr}
+is the address anywhere in function code (or function name).  If @code{-a}
+@var{addr} is used, the whole function surrounding that address will be
+disassembled. If @var{addr} is a function name, the whole function with that
+name will be disassembled.
 @item @var{filename}
 is the name of the file to disassemble
 @item @var{linenum}
@@ -33095,6 +33101,9 @@  records, produced when trying to execute an undefined @sc{gdb/mi} command
 @item exec-run-start-option
 Indicates that the @code{-exec-run} command supports the @option{--start}
 option (@pxref{GDB/MI Program Execution}).
+@item data-disassemble-a-option
+Indicates that the @code{-data-disassemble} command supports the @option{-a}
+option (@pxref{GDB/MI Data Manipulation}).
 @end ftable
 
 @subheading The @code{-list-target-features} Command
diff --git a/gdb/mi/mi-cmd-disas.c b/gdb/mi/mi-cmd-disas.c
index e704e35b34..51f9aa2594 100644
--- a/gdb/mi/mi-cmd-disas.c
+++ b/gdb/mi/mi-cmd-disas.c
@@ -67,6 +67,7 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
   int num_seen = 0;
   int start_seen = 0;
   int end_seen = 0;
+  int addr_seen = 0;
 
   /* ... and their corresponding value. */
   char *file_string = NULL;
@@ -74,13 +75,14 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
   int how_many = -1;
   CORE_ADDR low = 0;
   CORE_ADDR high = 0;
+  CORE_ADDR addr = 0;
 
   /* Options processing stuff.  */
   int oind = 0;
   char *oarg;
   enum opt
   {
-    FILE_OPT, LINE_OPT, NUM_OPT, START_OPT, END_OPT
+    FILE_OPT, LINE_OPT, NUM_OPT, START_OPT, END_OPT, ADDR_OPT
   };
   static const struct mi_opt opts[] =
     {
@@ -89,6 +91,7 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
       {"n", NUM_OPT, 1},
       {"s", START_OPT, 1},
       {"e", END_OPT, 1},
+      {"a", ADDR_OPT, 1},
       { 0, 0, 0 }
     };
 
@@ -122,6 +125,10 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
 	  high = parse_and_eval_address (oarg);
 	  end_seen = 1;
 	  break;
+        case ADDR_OPT:
+          addr = parse_and_eval_address (oarg);
+          addr_seen = 1;
+          break;
 	}
     }
   argv += oind;
@@ -130,15 +137,16 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
   /* Allow only filename + linenum (with how_many which is not
      required) OR start_addr + end_addr.  */
 
-  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen)
-	|| (line_seen && file_seen && !num_seen && !start_seen && !end_seen)
-	|| (!line_seen && !file_seen && !num_seen && start_seen && end_seen)))
+  if (!((line_seen && file_seen && num_seen && !start_seen && !end_seen && !addr_seen)
+	|| (line_seen && file_seen && !num_seen && !start_seen && !end_seen && !addr_seen)
+	|| (!line_seen && !file_seen && !num_seen && start_seen && end_seen && !addr_seen)
+	|| (!line_seen && !file_seen && !num_seen && !start_seen && !end_seen && addr_seen)))
     error (_("-data-disassemble: Usage: ( [-f filename -l linenum [-n "
-	     "howmany]] | [-s startaddr -e endaddr]) [--] mode."));
+	     "howmany]] | [-s startaddr -e endaddr] | [-a addr] ) [--] mode."));
 
   if (argc != 1)
-    error (_("-data-disassemble: Usage: [-f filename -l linenum "
-	     "[-n howmany]] [-s startaddr -e endaddr] [--] mode."));
+    error (_("-data-disassemble: Usage: ( [-f filename -l linenum [-n "
+             "howmany]] | [-s startaddr -e endaddr] | [-a addr] ) [--] mode."));
 
   mode = atoi (argv[0]);
   if (mode < 0 || mode > 5)
@@ -184,6 +192,12 @@  mi_cmd_disassemble (const char *command, char **argv, int argc)
 	error (_("-data-disassemble: "
 		 "No function contains specified address"));
     }
+  else if (addr_seen) 
+    {
+      if (find_pc_partial_function (addr, NULL, &low, &high) == 0)
+        error (_("-data-disassemble: "
+                 "No function contains specified address"));
+    }
 
   gdb_disassembly (gdbarch, uiout,
   		   disasm_flags,
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 2177abd0ed..f503b3149e 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1683,6 +1683,7 @@  mi_cmd_list_features (const char *command, char **argv, int argc)
       uiout->field_string (NULL, "info-gdb-mi-command");
       uiout->field_string (NULL, "undefined-command-error-code");
       uiout->field_string (NULL, "exec-run-start-option");
+      uiout->field_string (NULL, "data-disassemble-a-option");
 
       if (ext_lang_initialized_p (get_ext_lang_defn (EXT_LANG_PYTHON)))
 	uiout->field_string (NULL, "python");
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 93c849c040..11d1440535 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-07-05  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.mi/mi-disassemble.exp (test_disassembly_only): Added
+	tests for -data-disassemble -a.
+	(test_disassembly_bogus_args): Likewise.
+
 2018-06-29  Pedro Alves  <palves@redhat.com>
 
 	* gdb.threads/names.exp: Adjust expected "info threads" output.
diff --git a/gdb/testsuite/gdb.mi/mi-disassemble.exp b/gdb/testsuite/gdb.mi/mi-disassemble.exp
index 6b1fcf5884..bbf29d7b63 100644
--- a/gdb/testsuite/gdb.mi/mi-disassemble.exp
+++ b/gdb/testsuite/gdb.mi/mi-disassemble.exp
@@ -46,13 +46,24 @@  proc test_disassembly_only {} {
     # Test disassembly more only for the current function.
     # Tests:
     # -data-disassemble -s $pc -e "$pc+8" -- 0
+    # -data-disassemble -a $pc -- 0
+    # -data-disassemble -a callee4 -- 0
     # -data-disassembly -f basics.c -l $line_main_body -- 0
 
+
     mi_gdb_test "print/x \$pc" "" ""
     mi_gdb_test "111-data-disassemble -s \$pc -e \"\$pc + 12\" -- 0" \
 	    "111\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
              "data-disassemble from pc to pc+12 assembly only"
 
+    mi_gdb_test "112-data-disassemble -a \$pc -- 0" \
+	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
+             "data-disassemble function around pc assembly only"
+
+    mi_gdb_test "112-data-disassemble -a callee4 -- 0" \
+	    "112\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\},\{address=\"$hex\",func-name=\"callee4\",offset=\"$decimal\",inst=\".*\"\}.*\]" \
+             "data-disassemble function callee4 assembly only"
+
     mi_gdb_test "222-data-disassemble  -f basics.c -l $line_main_body -- 0" \
 	    "222\\^done,asm_insns=\\\[\{address=\"$hex\",func-name=\"main\",offset=\"0\",inst=\".*\"\},.*,\{address=\"$hex\",func-name=\"main\",offset=\"$decimal\",inst=\".*\"\}\\\]" \
               "data-disassemble file & line, assembly only"
@@ -207,6 +218,7 @@  proc test_disassembly_bogus_args {} {
     # Tests:
     # -data-disassembly -f foo -l abc -n 0 -- 0
     # -data-disassembly -s foo -e bar -- 0
+    # -data-disassembly -a foo -- 0
     # -data-disassembly -s $pc -f basics.c -- 0
     # -data-disassembly -f basics.c -l 32 -- 9
 
@@ -216,10 +228,14 @@  proc test_disassembly_bogus_args {} {
 
     mi_gdb_test "321-data-disassemble -s foo -e bar -- 0" \
              "321\\^error,msg=\"No symbol \\\\\"foo\\\\\" in current context.\"" \
-             "data-disassemble bogus address"
+             "data-disassemble bogus address (-s -e)"
+
+    mi_gdb_test "322-data-disassemble -a foo -- 0" \
+             "322\\^error,msg=\"No symbol \\\\\"foo\\\\\" in current context.\"" \
+             "data-disassemble bogus address (-a)"
 
     mi_gdb_test "456-data-disassemble -s \$pc -f basics.c -- 0" \
-             "456\\^error,msg=\"-data-disassemble: Usage: \\( .-f filename -l linenum .-n howmany.. \\| .-s startaddr -e endaddr.\\) .--. mode.\"" \
+             "456\\^error,msg=\"-data-disassemble: Usage: \\( .-f filename -l linenum .-n howmany.. \\| .-s startaddr -e endaddr. \\| .-a addr. \\) .--. mode.\"" \
              "data-disassemble mix different args"
 
     mi_gdb_test "789-data-disassemble -f basics.c -l $line_main_body -- 9" \