[PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[Yao Qi]

Hi,
When I modify a MI test case to avoid hard code breakpoint number in
the testcase, commands -break-passcount and -break-commands only
accepts numbers and rejects convenience variables, such as $bpnum, as
invalid input.

IWBN to teach these commands to accept convenience variables, and
avoid hard code breakpoint number in test case.

I add a NEWS entry since it is user visible.  However, I didn't update
the manual because manual doesn't mention that commands 'enable' 'disable'
accept convenience variable explicitly, likewise, doesn't mention this
for MI commands.  Some test cases are updated to replace hard code
number with convenience variables.

Regression tested on x86_64-linux.

gdb:

2014-01-24  Yao Qi  <yao@codesourcery.com>

	* mi/mi-cmd-break.c (parse_number): New function.
	(mi_cmd_break_passcount): Use parse_number instead of atoi.
	(mi_cmd_break_commands): Move some code to parse_number.  Call
	parse_number.
	* NEWS: Mention that commands -break-passcount and
	-break-commands accept convenience variable.

gdb/testsuite:

2014-01-24  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Test command -break-passcount accepts convenience variable.
	* gdb.trace/mi-trace-frame-collected.exp: Test command
	-break-commands accepts convenience variable.
	* gdb.trace/mi-trace-unavailable.exp: Likewise.
---
 gdb/NEWS                                           |    5 ++
 gdb/mi/mi-cmd-break.c                              |   46 +++++++++++++++----
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp     |    2 +
 .../gdb.trace/mi-trace-frame-collected.exp         |    4 +-
 gdb/testsuite/gdb.trace/mi-trace-unavailable.exp   |    4 +-
 5 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 6b1aacb..32539b2 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -42,6 +42,11 @@ qXfer:btrace:read's annex
   The qXfer:btrace:read packet supports a new annex 'delta' to read
   branch trace incrementally.
 
+* MI changes
+
+  ** The commands -break-passcount and -break-commands accept convenience
+     variable as breakpoint number.
+
 *** Changes in GDB 7.7
 
 * Improved support for process record-replay and reverse debugging on
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 29e3fb3..7d9e360 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -324,6 +324,38 @@ enum wp_type
   ACCESS_WP
 };
 
+/* Get the number of NUMBER, which can be a number or "$" followed by
+   the name of a convenience variable.  */
+
+static int
+parse_number (const char *number)
+{
+  int bnum;
+
+  if (*number == '$') /* Convenience variable */
+    {
+      LONGEST val;
+
+      if (get_internalvar_integer (lookup_internalvar (&number[1]), &val))
+	bnum = (int) val;
+      else
+	error (_("Convenience variable must have integer value."));
+    }
+  else
+    {
+      char *endptr;
+
+      bnum = strtol (number, &endptr, 0);
+      if (endptr == number)
+	error (_("number argument \"%s\" is not a number."),
+	       number);
+      else if (*endptr != '\0')
+	error (_("junk at the end of number argument \"%s\"."),
+	       number);
+    }
+  return bnum;
+}
+
 void
 mi_cmd_break_passcount (char *command, char **argv, int argc)
 {
@@ -334,8 +366,8 @@ mi_cmd_break_passcount (char *command, char **argv, int argc)
   if (argc != 2)
     error (_("Usage: tracepoint-number passcount"));
 
-  n = atoi (argv[0]);
-  p = atoi (argv[1]);
+  n = parse_number (argv[0]);
+  p = parse_number (argv[1]);
   t = get_tracepoint (n);
 
   if (t)
@@ -437,20 +469,14 @@ void
 mi_cmd_break_commands (char *command, char **argv, int argc)
 {
   struct command_line *break_command;
-  char *endptr;
+
   int bnum;
   struct breakpoint *b;
 
   if (argc < 1)
     error (_("USAGE: %s <BKPT> [<COMMAND> [<COMMAND>...]]"), command);
 
-  bnum = strtol (argv[0], &endptr, 0);
-  if (endptr == argv[0])
-    error (_("breakpoint number argument \"%s\" is not a number."),
-	   argv[0]);
-  else if (*endptr != '\0')
-    error (_("junk at the end of breakpoint number argument \"%s\"."),
-	   argv[0]);
+  bnum = parse_number (argv[0]);
 
   b = get_breakpoint (bnum);
   if (b == NULL)
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index aa991cf..6049d2c 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -141,6 +141,8 @@ proc test_insert_delete_modify { } {
     # notification.
     mi_gdb_test "-break-passcount 4 1" "\\^done" \
 	"-break-passcount 4 1"
+    mi_gdb_test "-break-passcount \$tpnum 2" "\\^done" \
+	"-break-passcount \$tpnum 2"
 
     # Delete some breakpoints and verify that '=breakpoint-deleted
     # notification is correctly emitted.
diff --git a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
index e0f5f8d..9b1c638 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-frame-collected.exp
@@ -63,7 +63,7 @@ if [is_amd64_regs_target] {
     return -1
 }
 
-mi_gdb_test "-break-commands 3 \"collect gdb_char_test\" \"collect gdb_union1_test\" \"collect gdb_struct1_test.l\" \"collect gdb_arr_test\[0\]\" \"collect $${pcreg}\" \"teval \$tsv += 1\" \"collect \$tsv\" \"end\" " \
+mi_gdb_test "-break-commands \$bpnum \"collect gdb_char_test\" \"collect gdb_union1_test\" \"collect gdb_struct1_test.l\" \"collect gdb_arr_test\[0\]\" \"collect $${pcreg}\" \"teval \$tsv += 1\" \"collect \$tsv\" \"end\" " \
     {\^done} "set action"
 
 mi_gdb_test "-break-insert -a gdb_c_test" \
@@ -73,7 +73,7 @@ mi_gdb_test "-break-insert -a gdb_c_test" \
 # Define an action.
 # Collect a global variable to be sure no registers are collected
 # except PC.
-mi_gdb_test "-break-commands 4 \"collect gdb_char_test\" \"end\" " \
+mi_gdb_test "-break-commands \$bpnum \"collect gdb_char_test\" \"end\" " \
     {\^done} "set action on tracepoint 4"
 
 mi_gdb_test "-trace-start" {.*\^done} "trace start"
diff --git a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
index 6dd0415..4f5dd22 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-unavailable.exp
@@ -49,7 +49,7 @@ mi_gdb_test "-break-insert -a bar" \
     "insert tracepoint on bar"
 
 # Define an action.
-mi_gdb_test "-break-commands 3 \"collect array\" \"collect j\" \"end\" " \
+mi_gdb_test "-break-commands \$bpnum \"collect array\" \"collect j\" \"end\" " \
     {\^done} "set action"
 
 mi_gdb_test "-break-insert -a foo" \
@@ -57,7 +57,7 @@ mi_gdb_test "-break-insert -a foo" \
     "insert tracepoint on foo"
 
 # Collect 'main' to make sure no registers are collected except PC.
-mi_gdb_test "-break-commands 4 \"collect main\" \"end\" " \
+mi_gdb_test "-break-commands \$bpnum \"collect main\" \"end\" " \
     {\^done} "set action on tracepoint 4"
 
 mi_gdb_test "-trace-start" {.*\^done} "trace start"
-- 
1.7.7.6

Re: [PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[Yao Qi]

On 01/24/2014 08:20 PM, Yao Qi wrote:
> When I modify a MI test case to avoid hard code breakpoint number in
> the testcase, commands -break-passcount and -break-commands only
> accepts numbers and rejects convenience variables, such as $bpnum, as
> invalid input.
> 
> IWBN to teach these commands to accept convenience variables, and
> avoid hard code breakpoint number in test case.
> 
> I add a NEWS entry since it is user visible.  However, I didn't update
> the manual because manual doesn't mention that commands 'enable' 'disable'
> accept convenience variable explicitly, likewise, doesn't mention this
> for MI commands.  Some test cases are updated to replace hard code
> number with convenience variables.
> 
> Regression tested on x86_64-linux.
> 
> gdb:
> 
> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* mi/mi-cmd-break.c (parse_number): New function.
> 	(mi_cmd_break_passcount): Use parse_number instead of atoi.
> 	(mi_cmd_break_commands): Move some code to parse_number.  Call
> 	parse_number.
> 	* NEWS: Mention that commands -break-passcount and
> 	-break-commands accept convenience variable.
> 
> gdb/testsuite:
> 
> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> 	Test command -break-passcount accepts convenience variable.
> 	* gdb.trace/mi-trace-frame-collected.exp: Test command
> 	-break-commands accepts convenience variable.
> 	* gdb.trace/mi-trace-unavailable.exp: Likewise.

Ping.  https://sourceware.org/ml/gdb-patches/2014-01/msg00936.html

-- 
Yao (齐尧)

ping: [PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[Yao Qi]

On 02/06/2014 08:36 PM, Yao Qi wrote:
> On 01/24/2014 08:20 PM, Yao Qi wrote:
>> When I modify a MI test case to avoid hard code breakpoint number in
>> the testcase, commands -break-passcount and -break-commands only
>> accepts numbers and rejects convenience variables, such as $bpnum, as
>> invalid input.
>>
>> IWBN to teach these commands to accept convenience variables, and
>> avoid hard code breakpoint number in test case.
>>
>> I add a NEWS entry since it is user visible.  However, I didn't update
>> the manual because manual doesn't mention that commands 'enable' 'disable'
>> accept convenience variable explicitly, likewise, doesn't mention this
>> for MI commands.  Some test cases are updated to replace hard code
>> number with convenience variables.
>>
>> Regression tested on x86_64-linux.
>>
>> gdb:
>>
>> 2014-01-24  Yao Qi  <yao@codesourcery.com>
>>
>> 	* mi/mi-cmd-break.c (parse_number): New function.
>> 	(mi_cmd_break_passcount): Use parse_number instead of atoi.
>> 	(mi_cmd_break_commands): Move some code to parse_number.  Call
>> 	parse_number.
>> 	* NEWS: Mention that commands -break-passcount and
>> 	-break-commands accept convenience variable.
>>
>> gdb/testsuite:
>>
>> 2014-01-24  Yao Qi  <yao@codesourcery.com>
>>
>> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
>> 	Test command -break-passcount accepts convenience variable.
>> 	* gdb.trace/mi-trace-frame-collected.exp: Test command
>> 	-break-commands accepts convenience variable.
>> 	* gdb.trace/mi-trace-unavailable.exp: Likewise.
> 
> Ping.  https://sourceware.org/ml/gdb-patches/2014-01/msg00936.html
> 

Ping.

-- 
Yao (齐尧)

Re: ping: [PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[Joel Brobecker]

> >> When I modify a MI test case to avoid hard code breakpoint number in
> >> the testcase, commands -break-passcount and -break-commands only
> >> accepts numbers and rejects convenience variables, such as $bpnum, as
> >> invalid input.
> >>
> >> IWBN to teach these commands to accept convenience variables, and
> >> avoid hard code breakpoint number in test case.
> >>
> >> I add a NEWS entry since it is user visible.  However, I didn't update
> >> the manual because manual doesn't mention that commands 'enable' 'disable'
> >> accept convenience variable explicitly, likewise, doesn't mention this
> >> for MI commands.  Some test cases are updated to replace hard code
> >> number with convenience variables.
> >>
> >> Regression tested on x86_64-linux.
> >>
> >> gdb:
> >>
> >> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> >>
> >> 	* mi/mi-cmd-break.c (parse_number): New function.
> >> 	(mi_cmd_break_passcount): Use parse_number instead of atoi.
> >> 	(mi_cmd_break_commands): Move some code to parse_number.  Call
> >> 	parse_number.
> >> 	* NEWS: Mention that commands -break-passcount and
> >> 	-break-commands accept convenience variable.
> >>
> >> gdb/testsuite:
> >>
> >> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> >>
> >> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> >> 	Test command -break-passcount accepts convenience variable.
> >> 	* gdb.trace/mi-trace-frame-collected.exp: Test command
> >> 	-break-commands accepts convenience variable.
> >> 	* gdb.trace/mi-trace-unavailable.exp: Likewise.
> > 
> > Ping.  https://sourceware.org/ml/gdb-patches/2014-01/msg00936.html
> > 
> 
> Ping.

No objection to the functionality extension itself, but the
implementatoin made me wonder why you had to implement convenience
variable extension, and I went looking for what the equivalent CLI
commands were doing. They are using get_number_or_range.

Would it make sense, in this case, to extend the interface of
those GDB/MI commands to match the interface of their CLI counterparts?
Depending on the answer, you might want to factorize a bit the code
so that both use the same code (ie the CLI split the args first
and then call the factorized function, while the MI would just call
the factorized function). Otherwise, cli/cli-utils.c::get_number
seems to be doing what you want.

-- 
Joel

Re: ping: [PATCH] Accept convenience variable in commands -break-passcount and -break-commands

[Yao Qi]

On 02/19/2014 04:55 PM, Joel Brobecker wrote:
> No objection to the functionality extension itself, but the
> implementatoin made me wonder why you had to implement convenience
> variable extension, and I went looking for what the equivalent CLI
> commands were doing. They are using get_number_or_range.
> 
> Would it make sense, in this case, to extend the interface of
> those GDB/MI commands to match the interface of their CLI counterparts?

I thought about using get_number_or_range in MI parts too when I wrote
this patch.  I didn't pursue this approach because of the different
error report in CLI and MI.  Function get_number_trailer doesn't emit
any errors, and only returns 0 if something wrong.  Callers of
get_number_or_range in CLI may emit warnings if 0 is returned.  At
present, GDB MI emits error immediately when parsing is wrong.

> Depending on the answer, you might want to factorize a bit the code
> so that both use the same code (ie the CLI split the args first
> and then call the factorized function, while the MI would just call
> the factorized function). Otherwise, cli/cli-utils.c::get_number
> seems to be doing what you want.

I agree that we need some code factor here, so that both CLI and MI can
use the same interface.  I'll post an updated patch.

-- 
Yao (齐尧)