* [PATCH 4/5] get_number returns status in details
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
@ 2014-03-13 2:35 ` Yao Qi
2014-03-13 2:35 ` [PATCH 5/5] Accept convenience variable in commands -break-passcount and -break-commands Yao Qi
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Yao Qi @ 2014-03-13 2:35 UTC (permalink / raw)
To: gdb-patches
Hi,
get_number doesn't return status in details, which prevent callers
printing error or warning in details. This patch change get_number
interface to return status in return value and return parsed number in
a new argument '*number'. It is quite similar to to_xfer_partial
interface.
After this patch, from the return value of get_number, we can tell the
input is invalid or the input is zero. Then, we can add the checking
to "enable count" command, in which, zero is a valid input.
GDB is quiet for these invalid input like this
(gdb) enable count 1.1 1
(gdb)
with this patch, GDB emits error if input number isn't valid:
(gdb) enable count 1.1 1
Invalid count number '1.1 1'
zero is still a valid number, so I add a test about
"enable count 0 1" and this input is valid. However, the semantics of
setting count to zero is out of the scope of this patch.
gdb:
2014-03-13 Yao Qi <yao@codesourcery.com>
* breakpoint.c (enable_count_command): Error out if
get_number returns error.
* cli/cli-utils.c (get_number_trailer): Update comments.
(get_number_trailer): Add argument number and change return
type to enum get_number_status. Return status instead of
printing messages.
(print_get_number_status): New function.
(get_number): Add argument number and call
print_get_number_status. All callers updated.
(get_number_or_range): Call print_get_number_status.
* cli/cli-utils.h (enum get_number_status): New.
(get_number): Update declaration.
* reverse.c (goto_bookmark_command): Change 'num' to type int.
gdb/testsuite:
2014-03-13 Yao Qi <yao@codesourcery.com>
* gdb.base/ena-dis-br.exp: Test invalid count number and zero.
---
gdb/breakpoint.c | 22 ++++-----
gdb/cli/cli-utils.c | 84 +++++++++++++++++++++-----------
gdb/cli/cli-utils.h | 29 ++++++++++-
gdb/reverse.c | 6 +--
gdb/testsuite/gdb.base/ena-dis-br.exp | 9 ++++
5 files changed, 102 insertions(+), 48 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 211c2aa..6ccc0e7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1040,8 +1040,7 @@ condition_command (char *arg, int from_tty)
error_no_arg (_("breakpoint number"));
p = arg;
- bnum = get_number (&p);
- if (bnum == 0)
+ if (get_number (&p, &bnum) != GET_NUMBER_OK)
error (_("Bad breakpoint argument: '%s'"), arg);
ALL_BREAKPOINTS (b)
@@ -14564,8 +14563,7 @@ ignore_command (char *args, int from_tty)
if (p == 0)
error_no_arg (_("a breakpoint number"));
- num = get_number (&p);
- if (num == 0)
+ if (get_number (&p, &num) != GET_NUMBER_OK)
error (_("bad breakpoint number: '%s'"), args);
if (*p == 0)
error (_("Second argument (specified ignore-count) is missing."));
@@ -14634,8 +14632,7 @@ find_location_by_number (char *number)
*dot = '\0';
p1 = number;
- bp_num = get_number (&p1);
- if (bp_num == 0)
+ if (get_number (&p1, &bp_num) != GET_NUMBER_OK)
error (_("Bad breakpoint number '%s'"), number);
ALL_BREAKPOINTS (b)
@@ -14648,8 +14645,7 @@ find_location_by_number (char *number)
error (_("Bad breakpoint number '%s'"), number);
p1 = dot+1;
- loc_num = get_number (&p1);
- if (loc_num == 0)
+ if (get_number (&p1, &loc_num) != GET_NUMBER_OK)
error (_("Bad breakpoint location number '%s'"), number);
--loc_num;
@@ -14934,7 +14930,11 @@ do_map_enable_count_breakpoint (struct breakpoint *bpt, void *countptr)
static void
enable_count_command (char *args, int from_tty)
{
- int count = get_number (&args);
+ char *p = args;
+ int count;
+
+ if (get_number (&args, &count) != GET_NUMBER_OK)
+ error (_("Invalid count number '%s'"), p);
map_breakpoint_numbers (args, do_map_enable_count_breakpoint, &count);
}
@@ -15611,9 +15611,7 @@ get_tracepoint_by_number (char *arg)
{
char *instring = arg;
- tpnum = get_number (&arg);
-
- if (tpnum == 0)
+ if (get_number (&arg, &tpnum) != GET_NUMBER_OK)
{
printf_filtered (_("bad tracepoint number at or near '%s'\n"),
instring);
diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index d59a231..53211a4 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -25,19 +25,15 @@
#include <ctype.h>
-/* *PP is a string denoting a number. Get the number of the. Advance
- *PP after the string and any trailing whitespace.
+/* TRAILER is a character which can be found after the number; most
+ commonly this is `-'. If you don't want a trailer, use \0. For
+ PP and function return value, please see documentation of
+ get_number in cli-utils.h. */
- Currently the string can either be a number, or "$" followed by the
- name of a convenience variable, or ("$" or "$$") followed by digits.
-
- TRAILER is a character which can be found after the number; most
- commonly this is `-'. If you don't want a trailer, use \0. */
-
-static int
-get_number_trailer (char **pp, int trailer)
+static enum get_number_status
+get_number_trailer (char **pp, int trailer, int *number)
{
- int retval = 0; /* default */
+ enum get_number_status retval; /* default */
char *p = *pp;
if (*p == '$')
@@ -47,12 +43,13 @@ get_number_trailer (char **pp, int trailer)
if (val) /* Value history reference */
{
if (TYPE_CODE (value_type (val)) == TYPE_CODE_INT)
- retval = value_as_long (val);
- else
{
- printf_filtered (_("History value must have integer type.\n"));
- retval = 0;
+ retval = GET_NUMBER_OK;
+ *number = (int) value_as_long (val);
}
+ else
+ retval = GET_NUMBER_E_HISTORY_VALUE_TYPE;
+
}
else /* Convenience variable */
{
@@ -68,13 +65,12 @@ get_number_trailer (char **pp, int trailer)
strncpy (varname, start, p - start);
varname[p - start] = '\0';
if (get_internalvar_integer (lookup_internalvar (varname), &val))
- retval = (int) val;
- else
{
- printf_filtered (_("Convenience variable must "
- "have integer value.\n"));
- retval = 0;
+ *number = (int) val;
+ retval = GET_NUMBER_OK;
}
+ else
+ retval = GET_NUMBER_E_CONVENIENCE_TYPE;
}
}
else
@@ -90,29 +86,54 @@ get_number_trailer (char **pp, int trailer)
while (*p && !isspace((int) *p))
++p;
/* Return zero, which caller must interpret as error. */
- retval = 0;
+ retval = GET_NUMBER_E_NOT_NUMBER;
}
else
- retval = atoi (*pp);
+ {
+ *number = atoi (*pp);
+ retval = GET_NUMBER_OK;
+ }
}
if (!(isspace (*p) || *p == '\0' || *p == trailer))
{
/* Trailing junk: return 0 and let caller print error msg. */
while (!(isspace (*p) || *p == '\0' || *p == trailer))
++p;
- retval = 0;
+
+ retval = GET_NUMBER_E_TRAILING_JUNK;
}
p = skip_spaces (p);
*pp = p;
return retval;
}
+/* Print STATUS. */
+
+static void
+print_get_number_status (enum get_number_status status)
+{
+ if (status == GET_NUMBER_E_HISTORY_VALUE_TYPE)
+ printf_filtered (_("History value must have integer type.\n"));
+ else if (status == GET_NUMBER_E_CONVENIENCE_TYPE)
+ printf_filtered (_("Convenience variable must have integer value.\n"));
+ else if (status == GET_NUMBER_E_NOT_NUMBER)
+ printf_filtered (_("Not a number.\n"));
+ else if (status == GET_NUMBER_E_TRAILING_JUNK)
+ printf_filtered (_("Junk at the end of number.\n"));
+}
+
/* See documentation in cli-utils.h. */
-int
-get_number (char **pp)
+enum get_number_status
+get_number (char **pp, int *number)
{
- return get_number_trailer (pp, '\0');
+ enum get_number_status status;
+
+ status = get_number_trailer (pp, '\0', number);
+
+ print_get_number_status (status);
+
+ return status;
}
/* See documentation in cli-utils.h. */
@@ -132,9 +153,15 @@ get_number_or_range (struct get_number_or_range_state *state)
{
if (*state->string != '-')
{
+ enum get_number_status status;
+
+ status = get_number_trailer (&state->string, '-', &state->last_retval);
/* Default case: state->string is pointing either to a solo
number, or to the first number of a range. */
- state->last_retval = get_number_trailer (&state->string, '-');
+ if (GET_NUMBER_OK != status)
+ state->last_retval = 0;
+
+ print_get_number_status (status);
if (*state->string == '-')
{
char **temp;
@@ -145,8 +172,7 @@ get_number_or_range (struct get_number_or_range_state *state)
temp = &state->end_ptr;
state->end_ptr = skip_spaces (state->string + 1);
- state->end_value = get_number (temp);
- if (state->end_value == 0)
+ if (get_number (temp, &state->end_value) != GET_NUMBER_OK)
error (_("wrong value"));
else if (state->end_value < state->last_retval)
{
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index fed876b..eee23f4 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -20,13 +20,36 @@
#ifndef CLI_UTILS_H
#define CLI_UTILS_H
-/* *PP is a string denoting a number. Get the number of the. Advance
- *PP after the string and any trailing whitespace.
+/* The returned status of get_number. */
+
+enum get_number_status
+{
+ /* The input string is correctly parsed. */
+ GET_NUMBER_OK = 0,
+
+ /* The input string is a history value, but its type isn't
+ expected. */
+ GET_NUMBER_E_HISTORY_VALUE_TYPE = -1,
+
+ /* The input string is a convenience variable, but its type
+ isn't expected. */
+ GET_NUMBER_E_CONVENIENCE_TYPE = -2,
+
+ /* The input string is not a number. */
+ GET_NUMBER_E_NOT_NUMBER = -3,
+
+ /* There is junk at the end of the input string. */
+ GET_NUMBER_E_TRAILING_JUNK = -4,
+};
+
+/* *PP is a string denoting a number. Return the status of
+ 'enum get_number_status' and save the number in *NUMBER on
+ success. Advance *PP after the string and any trailing whitespace.
Currently the string can either be a number, or "$" followed by the
name of a convenience variable, or ("$" or "$$") followed by digits. */
-extern int get_number (char **);
+extern enum get_number_status get_number (char **pp, int *number);
/* An object of this type is passed to get_number_or_range. It must
be initialized by calling init_number_or_range. This type is
diff --git a/gdb/reverse.c b/gdb/reverse.c
index 30a0328..7828555 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -249,7 +249,7 @@ static void
goto_bookmark_command (char *args, int from_tty)
{
struct bookmark *b;
- unsigned long num;
+ int num;
char *p = args;
if (args == NULL || args[0] == '\0')
@@ -274,9 +274,7 @@ goto_bookmark_command (char *args, int from_tty)
}
/* General case. Bookmark identified by bookmark number. */
- num = get_number (&args);
-
- if (num == 0)
+ if (get_number (&args, &num) != GET_NUMBER_OK)
error (_("goto-bookmark: invalid bookmark number '%s'."), p);
ALL_BOOKMARKS (b)
diff --git a/gdb/testsuite/gdb.base/ena-dis-br.exp b/gdb/testsuite/gdb.base/ena-dis-br.exp
index 6f2c469..7d0e3e1 100644
--- a/gdb/testsuite/gdb.base/ena-dis-br.exp
+++ b/gdb/testsuite/gdb.base/ena-dis-br.exp
@@ -155,6 +155,15 @@ set bp [break_at $bp_location7 "line $bp_location7"]
set bp2 [break_at marker1 " line ($bp_location15|$bp_location16)"]
+# Test enable count with invalid count number.
+
+gdb_test "enable count 1.1 $bp" "Invalid count number '1.1 $bp'.*" \
+ "enable count with invalid number"
+
+# Test enable count with zero, which is valid.
+
+gdb_test_no_output "enable count 0 $bp" "enable count with zero"
+
gdb_test_no_output "enable count 2 $bp" "disable break with count"
gdb_test "continue" \
--
1.7.7.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 5/5] Accept convenience variable in commands -break-passcount and -break-commands
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
2014-03-13 2:35 ` [PATCH 4/5] get_number returns status in details Yao Qi
@ 2014-03-13 2:35 ` Yao Qi
2014-03-14 8:22 ` Eli Zaretskii
2014-03-13 2:35 ` [PATCH 2/5] Change argument 'args' of get_tracepoint_by_number to char * Yao Qi
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2014-03-13 2:35 UTC (permalink / raw)
To: gdb-patches
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.
This patch teaches 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.
gdb:
2014-03-13 Yao Qi <yao@codesourcery.com>
* cli/cli-utils.c (get_number_quiet): New function.
* cli/cli-utils.h (get_number_quiet): Declare.
* mi/mi-cmd-break.c: Include "cli/cli-utils.h".
(mi_cmd_break_passcount): Use get_number_quiet instead of atoi.
(mi_cmd_break_commands): Likewise.
* NEWS: Mention that commands -break-passcount and
-break-commands accept convenience variable.
gdb/testsuite:
2014-03-13 Yao Qi <yao@codesourcery.com>
* gdb.mi/mi-break.exp: Test the error messages to invalid
input for -break-commands.
* 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/cli/cli-utils.c | 8 ++++++
gdb/cli/cli-utils.h | 4 +++
gdb/mi/mi-cmd-break.c | 26 +++++++++++++++----
gdb/testsuite/gdb.mi/mi-break.exp | 8 ++++++
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 +-
8 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index 2a384ba..f667538 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -95,6 +95,11 @@ PowerPC64 GNU/Linux little-endian powerpc64le-*-linux*
and "assf"), have been deprecated. Use the "sharedlibrary" command, or
its alias "share", instead.
+* 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/cli/cli-utils.c b/gdb/cli/cli-utils.c
index 53211a4..b906a0f 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -125,6 +125,14 @@ print_get_number_status (enum get_number_status status)
/* See documentation in cli-utils.h. */
enum get_number_status
+get_number_quiet (char **pp, int *number)
+{
+ return get_number_trailer (pp, '\0', number);
+}
+
+/* See documentation in cli-utils.h. */
+
+enum get_number_status
get_number (char **pp, int *number)
{
enum get_number_status status;
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index eee23f4..fc938ec 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -51,6 +51,10 @@ enum get_number_status
extern enum get_number_status get_number (char **pp, int *number);
+/* Similar to get_number, but it doesn't print messages. */
+
+extern enum get_number_status get_number_quiet (char **pp, int *number);
+
/* An object of this type is passed to get_number_or_range. It must
be initialized by calling init_number_or_range. This type is
defined here so that it can be stack-allocated, but all members
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 29e3fb3..4ef8eb5 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -32,6 +32,7 @@
#include "mi-cmd-break.h"
#include "gdb_obstack.h"
#include <ctype.h>
+#include "cli/cli-utils.h"
enum
{
@@ -330,12 +331,18 @@ mi_cmd_break_passcount (char *command, char **argv, int argc)
int n;
int p;
struct tracepoint *t;
+ char *s;
if (argc != 2)
error (_("Usage: tracepoint-number passcount"));
- n = atoi (argv[0]);
- p = atoi (argv[1]);
+ s = argv[0];
+ if (get_number_quiet (&s, &n) != GET_NUMBER_OK)
+ error (_("Invalid tracepoint number \"%s\"."), argv[0]);
+
+ s = argv[1];
+ if (get_number_quiet (&s, &p) != GET_NUMBER_OK)
+ error (_("Invalid passcount number \"%s\"."), argv[1]);
t = get_tracepoint (n);
if (t)
@@ -437,18 +444,25 @@ void
mi_cmd_break_commands (char *command, char **argv, int argc)
{
struct command_line *break_command;
- char *endptr;
+
int bnum;
struct breakpoint *b;
+ char *p = argv[0];
+ enum get_number_status status;
if (argc < 1)
error (_("USAGE: %s <BKPT> [<COMMAND> [<COMMAND>...]]"), command);
- bnum = strtol (argv[0], &endptr, 0);
- if (endptr == argv[0])
+ status = get_number_quiet (&p, &bnum);
+
+ if (status == GET_NUMBER_E_HISTORY_VALUE_TYPE)
+ error (_("history value must have integer type"));
+ else if (status == GET_NUMBER_E_CONVENIENCE_TYPE)
+ error (_("convenience variable must have integer value"));
+ else if (status == GET_NUMBER_E_NOT_NUMBER)
error (_("breakpoint number argument \"%s\" is not a number."),
argv[0]);
- else if (*endptr != '\0')
+ else if (status == GET_NUMBER_E_TRAILING_JUNK)
error (_("junk at the end of breakpoint number argument \"%s\"."),
argv[0]);
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index a9064fd..ea8fb37 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -232,6 +232,14 @@ proc test_breakpoint_commands {} {
mi_create_breakpoint "basics.c:callee2" 7 keep callee2 ".*basics.c" $line_callee2_body $hex \
"breakpoint commands: insert breakpoint at basics.c:callee2"
+ mi_gdb_test "-break-commands xyz \"print 10\" \"continue\"" \
+ "\\^error,msg=\"breakpoint number argument \\\\\"xyz\\\\\" is not a number\..*" \
+ "breakpoint commands: not a number"
+
+ mi_gdb_test "-break-commands 7.7 \"print 10\" \"continue\"" \
+ "\\^error,msg=\"junk at the end of breakpoint number argument \\\\\"7\.7\\\\\".*" \
+ "breakpoint commands: junk at the end"
+
mi_gdb_test "-break-commands 7 \"print 10\" \"continue\"" \
"\\^done" \
"breakpoint commands: set commands"
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 2/5] Change argument 'args' of get_tracepoint_by_number to char *
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
2014-03-13 2:35 ` [PATCH 4/5] get_number returns status in details Yao Qi
2014-03-13 2:35 ` [PATCH 5/5] Accept convenience variable in commands -break-passcount and -break-commands Yao Qi
@ 2014-03-13 2:35 ` Yao Qi
2014-03-13 2:35 ` [PATCH 3/5] Emit wrong value error when parsing range Yao Qi
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Yao Qi @ 2014-03-13 2:35 UTC (permalink / raw)
To: gdb-patches
We pass the reference to a char pointer to get_tracepoint_by_number,
but the char pointer isn't used afterwards, so looks it should be
fine to pass a char pointer to get_tracepoint_by_number.
gdb:
2014-03-13 Yao Qi <yao@codesourcery.com>
* breakpoint.c (get_tracepoint_by_number): Change type of
'args' to 'char *'. Update pointer checking. All callers
updated.
* breakpoint.h (get_tracepoint_by_number): Update declaration.
---
gdb/breakpoint.c | 10 +++++-----
gdb/breakpoint.h | 2 +-
gdb/tracepoint.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 800cead..211c2aa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15532,7 +15532,7 @@ trace_pass_command (char *args, int from_tty)
}
else if (*args == '\0')
{
- t1 = get_tracepoint_by_number (&args);
+ t1 = get_tracepoint_by_number (args);
if (t1)
trace_pass_set_count (t1, count, from_tty);
}
@@ -15592,12 +15592,12 @@ get_tracepoint_by_number_on_target (int num)
(tracepoint_count) is returned. */
struct tracepoint *
-get_tracepoint_by_number (char **arg)
+get_tracepoint_by_number (char *arg)
{
struct breakpoint *t;
int tpnum;
- if (arg == NULL || *arg == NULL || ! **arg)
+ if (arg == NULL || *arg == 0)
{
if (tracepoint_count == 0)
{
@@ -15609,9 +15609,9 @@ get_tracepoint_by_number (char **arg)
}
else
{
- char *instring = *arg;
+ char *instring = arg;
- tpnum = get_number (arg);
+ tpnum = get_number (&arg);
if (tpnum == 0)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c1ed2e6..c240516 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1490,7 +1490,7 @@ extern struct tracepoint *get_tracepoint_by_number_on_target (int num);
/* Find a tracepoint by parsing a number in the supplied string. */
extern struct tracepoint *
- get_tracepoint_by_number (char **arg);
+ get_tracepoint_by_number (char *arg);
/* Return a vector of all tracepoints currently defined. The vector
is newly allocated; the caller should free when done with it. */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index cea40e3..a5bc7d1 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -653,7 +653,7 @@ trace_actions_command (char *args, int from_tty)
struct tracepoint *t;
struct command_line *l;
- t = get_tracepoint_by_number (&args);
+ t = get_tracepoint_by_number (args);
if (t)
{
char *tmpbuf =
--
1.7.7.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 3/5] Emit wrong value error when parsing range
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
` (2 preceding siblings ...)
2014-03-13 2:35 ` [PATCH 2/5] Change argument 'args' of get_tracepoint_by_number to char * Yao Qi
@ 2014-03-13 2:35 ` Yao Qi
2014-03-13 2:35 ` [PATCH 1/5] Remove argument 'state' from get_tracepoint_by_number Yao Qi
2014-03-13 15:14 ` [PATCH 0/5] Return error code in get_number Tom Tromey
5 siblings, 0 replies; 12+ messages in thread
From: Yao Qi @ 2014-03-13 2:35 UTC (permalink / raw)
To: gdb-patches
The current code in get_number_or_range doesn't check the return value
of get_number, so current gdb does:
(gdb) thread apply 1-1.2 bt
inverted range
which is a little bit confusing. This patch adds a check to
get_number's return value, and emit error "wrong value" if return
value is zero. With this patch applied, gdb prints:
(gdb) thread apply 1-1.2 bt
wrong value
gdb:
2014-03-13 Yao Qi <yao@codesourcery.com>
* cli/cli-utils.c (get_number_or_range): Error out if get_number
returns zero.
---
gdb/cli/cli-utils.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index a0ebc11..d59a231 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -146,7 +146,9 @@ get_number_or_range (struct get_number_or_range_state *state)
temp = &state->end_ptr;
state->end_ptr = skip_spaces (state->string + 1);
state->end_value = get_number (temp);
- if (state->end_value < state->last_retval)
+ if (state->end_value == 0)
+ error (_("wrong value"));
+ else if (state->end_value < state->last_retval)
{
error (_("inverted range"));
}
--
1.7.7.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/5] Remove argument 'state' from get_tracepoint_by_number
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
` (3 preceding siblings ...)
2014-03-13 2:35 ` [PATCH 3/5] Emit wrong value error when parsing range Yao Qi
@ 2014-03-13 2:35 ` Yao Qi
2014-03-13 15:14 ` [PATCH 0/5] Return error code in get_number Tom Tromey
5 siblings, 0 replies; 12+ messages in thread
From: Yao Qi @ 2014-03-13 2:35 UTC (permalink / raw)
To: gdb-patches
Nowadays, get_tracepoint_by_number gets number from two sources, ARG
and STATE, which looks a little odd. It's better to change
get_tracepoint_by_number to get number only from ARG, and use
get_number_or_range to get number from STATE.
This patch removes 'state' argument, get get_tracepoint_by_number
cleaner.
gdb:
2014-03-13 Yao Qi <yao@codesourcery.com>
* breakpoint.c (trace_pass_command): Get tracepoint number by
get_number_or_range and get tracepoint get_tracepoint.
(get_tracepoint_by_number): Remove argument 'state'. Update
comments. Don't check 'state'. All callers updated.
* breakpoint.h (get_tracepoint_by_number): Update declaration.
---
gdb/breakpoint.c | 46 ++++++++++++++++++++++++++--------------------
gdb/breakpoint.h | 3 +--
gdb/tracepoint.c | 2 +-
3 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1551b99..800cead 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15532,7 +15532,7 @@ trace_pass_command (char *args, int from_tty)
}
else if (*args == '\0')
{
- t1 = get_tracepoint_by_number (&args, NULL);
+ t1 = get_tracepoint_by_number (&args);
if (t1)
trace_pass_set_count (t1, count, from_tty);
}
@@ -15543,9 +15543,14 @@ trace_pass_command (char *args, int from_tty)
init_number_or_range (&state, args);
while (!state.finished)
{
- t1 = get_tracepoint_by_number (&args, &state);
+ int tpnum = get_number_or_range (&state);
+
+ t1 = get_tracepoint (tpnum);
+
if (t1)
trace_pass_set_count (t1, count, from_tty);
+ else
+ printf_unfiltered ("No tracepoint number %d.\n", tpnum);
}
}
}
@@ -15583,36 +15588,37 @@ get_tracepoint_by_number_on_target (int num)
}
/* Utility: parse a tracepoint number and look it up in the list.
- If STATE is not NULL, use, get_number_or_range_state and ignore ARG.
If the argument is missing, the most recent tracepoint
(tracepoint_count) is returned. */
struct tracepoint *
-get_tracepoint_by_number (char **arg,
- struct get_number_or_range_state *state)
+get_tracepoint_by_number (char **arg)
{
struct breakpoint *t;
int tpnum;
- char *instring = arg == NULL ? NULL : *arg;
- if (state)
+ if (arg == NULL || *arg == NULL || ! **arg)
{
- gdb_assert (!state->finished);
- tpnum = get_number_or_range (state);
+ if (tracepoint_count == 0)
+ {
+ printf_filtered (_("No previous tracepoint\n"));
+ return NULL;
+ }
+
+ tpnum = tracepoint_count;
}
- else if (arg == NULL || *arg == NULL || ! **arg)
- tpnum = tracepoint_count;
else
- tpnum = get_number (arg);
-
- if (tpnum <= 0)
{
- if (instring && *instring)
- printf_filtered (_("bad tracepoint number at or near '%s'\n"),
- instring);
- else
- printf_filtered (_("No previous tracepoint\n"));
- return NULL;
+ char *instring = *arg;
+
+ tpnum = get_number (arg);
+
+ if (tpnum == 0)
+ {
+ printf_filtered (_("bad tracepoint number at or near '%s'\n"),
+ instring);
+ return NULL;
+ }
}
ALL_TRACEPOINTS (t)
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index bf1f52a..c1ed2e6 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1490,8 +1490,7 @@ extern struct tracepoint *get_tracepoint_by_number_on_target (int num);
/* Find a tracepoint by parsing a number in the supplied string. */
extern struct tracepoint *
- get_tracepoint_by_number (char **arg,
- struct get_number_or_range_state *state);
+ get_tracepoint_by_number (char **arg);
/* Return a vector of all tracepoints currently defined. The vector
is newly allocated; the caller should free when done with it. */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 673fddd..cea40e3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -653,7 +653,7 @@ trace_actions_command (char *args, int from_tty)
struct tracepoint *t;
struct command_line *l;
- t = get_tracepoint_by_number (&args, NULL);
+ t = get_tracepoint_by_number (&args);
if (t)
{
char *tmpbuf =
--
1.7.7.6
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/5] Return error code in get_number
2014-03-13 2:35 [PATCH 0/5] Return error code in get_number Yao Qi
` (4 preceding siblings ...)
2014-03-13 2:35 ` [PATCH 1/5] Remove argument 'state' from get_tracepoint_by_number Yao Qi
@ 2014-03-13 15:14 ` Tom Tromey
2014-03-14 0:59 ` Yao Qi
5 siblings, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2014-03-13 15:14 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
Yao> Patch #5 tries to teach GDB to accept convenience variable in MI
Yao> commands, in which we use get_number to parse string and get number.
Yao> However, get_number returns zero for any kinds of error, and callers
Yao> can't tell from it. Patch #4 changes get_number's interface to
Yao> return parsing status and parsed number. This patch also moves
Yao> some printing messages out of get_number, and let the callers to
Yao> print messages according to returned status.
Could you say what you want this for?
I don't understand why an MI client would ever use it.
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/5] Return error code in get_number
2014-03-13 15:14 ` [PATCH 0/5] Return error code in get_number Tom Tromey
@ 2014-03-14 0:59 ` Yao Qi
2014-03-14 14:38 ` Tom Tromey
0 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2014-03-14 0:59 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 03/13/2014 11:14 PM, Tom Tromey wrote:
> Could you say what you want this for?
> I don't understand why an MI client would ever use it.
I'd like to teach mi_cmd_break_commands to accept convenience variable,
so get_number is suggested to use
(https://sourceware.org/ml/gdb-patches/2014-02/msg00595.html).
mi_cmd_break_commands may throw two errors when it parses a number,
and I'd like to keep its behaviour after I change to get_number.
I want get_number returns the error code and mi_cmd_break_commands
can throws errors accordingly.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] Return error code in get_number
2014-03-14 0:59 ` Yao Qi
@ 2014-03-14 14:38 ` Tom Tromey
2014-03-17 0:51 ` Yao Qi
0 siblings, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2014-03-14 14:38 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao> I'd like to teach mi_cmd_break_commands to accept convenience variable,
Yeah -- but why?
It seems to me that an MI client already knows all the breakpoint
numbers. I'm having trouble picturing the scenario where I'd want my MI
client to use a convenience variable instead.
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] Return error code in get_number
2014-03-14 14:38 ` Tom Tromey
@ 2014-03-17 0:51 ` Yao Qi
2014-05-15 17:58 ` Tom Tromey
0 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2014-03-17 0:51 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 03/14/2014 10:38 PM, Tom Tromey wrote:
> It seems to me that an MI client already knows all the breakpoint
> numbers. I'm having trouble picturing the scenario where I'd want my MI
> client to use a convenience variable instead.
This facilitates writing MI test cases that we don't have to hard-code
breakpoint numbers anymore. Keith pointed this out in the review
https://sourceware.org/ml/gdb-patches/2014-01/msg00824.html and I think
that is good to have, so I write this patch series. Can this justify
the changes in this series?
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] Return error code in get_number
2014-03-17 0:51 ` Yao Qi
@ 2014-05-15 17:58 ` Tom Tromey
0 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2014-05-15 17:58 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Tom> It seems to me that an MI client already knows all the breakpoint
Tom> numbers. I'm having trouble picturing the scenario where I'd want my MI
Tom> client to use a convenience variable instead.
Yao> This facilitates writing MI test cases that we don't have to hard-code
Yao> breakpoint numbers anymore. Keith pointed this out in the review
Yao> https://sourceware.org/ml/gdb-patches/2014-01/msg00824.html and I think
Yao> that is good to have, so I write this patch series. Can this justify
Yao> the changes in this series?
I am not opposed to it but I would like to gently push back a little.
It seems to me that if the proposed patches are there to help with a
deficiency in the test suite, then it would be better to fix the test
suite. Say, by implementing a way to extract the breakpoint number and
make it available to the Tcl code. Did you consider this approach? And
if so what made you reject it?
Tom
^ permalink raw reply [flat|nested] 12+ messages in thread