This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Fix regression in "commands"
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>, gdb-patches at sourceware dot org
- Date: Tue, 7 Nov 2017 10:49:15 +0000
- Subject: Re: [RFA] Fix regression in "commands"
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 14BCE81E01
- References: <20171103190747.389-1-tom@tromey.com>
On 11/03/2017 07:07 PM, Tom Tromey wrote:
> Pedro pointed out a regression in "commands", where trying to clear a
> breakpoint's command list would fail:
>
> (top-gdb) commands
> Type commands for breakpoint(s) 3, one per line.
> End with a line saying just "end".
> >end
> No breakpoints specified.
> (top-gdb)
>
> I believe the bug was introduced by my patch that changes
> counted_command_line to be a shared_ptr. This causes the problem
> because now the counted_command_line in commands_command_1 can be NULL,
> whereas previously it never could be.
>
> The fix here is to track whether commands have been read using a
> separate flag.
>
> 2017-11-03 Tom Tromey <tom@tromey.com>
>
> * breakpoint.c (commands_command_1): Use a flag to track whether
> commands have been read.
>
> 2017-11-03 Tom Tromey <tom@tromey.com>
>
> * gdb.base/break.exp: Add test for empty "commands".
> ---
> gdb/ChangeLog | 5 +++++
> gdb/breakpoint.c | 7 +++++--
> gdb/testsuite/ChangeLog | 4 ++++
> gdb/testsuite/gdb.base/break.exp | 5 +++++
> 4 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 1f823ca..0dc20bf 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-11-03 Tom Tromey <tom@tromey.com>
> +
> + * breakpoint.c (commands_command_1): Use a flag to track whether
> + commands have been read.
> +
> 2017-11-03 Ulrich Weigand <uweigand@de.ibm.com>
>
> * doublest.c (convert_doublest_to_floatformat): Fix uninitialized
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 0bf47d5..609f1ed 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1255,6 +1255,7 @@ commands_command_1 (const char *arg, int from_tty,
> struct command_line *control)
> {
> counted_command_line cmd;
> + bool read_commands = false;
>
> std::string new_arg;
>
> @@ -1271,7 +1272,7 @@ commands_command_1 (const char *arg, int from_tty,
> map_breakpoint_numbers
> (arg, [&] (breakpoint *b)
> {
> - if (cmd == NULL)
> + if (!read_commands)
> {
> if (control != NULL)
> cmd = copy_command_lines (control->body_list[0]);
> @@ -1288,6 +1289,8 @@ commands_command_1 (const char *arg, int from_tty,
> ? check_tracepoint_command : 0),
> b);
> }
> +
> + read_commands = true;
> }
>
> /* If a breakpoint was on the list more than once, we don't need to
> @@ -1300,7 +1303,7 @@ commands_command_1 (const char *arg, int from_tty,
> }
> });
>
> - if (cmd == NULL)
> + if (!read_commands)
> error (_("No breakpoints specified."));
Hmm, for the truly "no breakpoints specified" case,
it seems to me that before we can reach this error, we've already
hit the error at the top of map_breakpoint_numbers:
if (args == 0 || *args == '\0')
error_no_arg (_("one or more breakpoint numbers"));
and for the case where the user specifies some argument
that doesn't match any breakpoint, map_breakpoint_numbers
already printed one of:
warning (_("bad breakpoint number at or near '%s'"), p);
printf_unfiltered (_("No breakpoint number %d.\n"), num);
when the "No breakpoints specified." error is reached.
E.g.:
(gdb) commands asdf
warning: bad breakpoint number at or near 'asdf'
No breakpoints specified.
(gdb) commands 123
No breakpoint number 123.
No breakpoints specified.
The "No breakpoints specified" error here looks a bit
strange to me, since I did specify something.
So I'm wondering what's the actual command line that we'd
want to reach this error call here. I note that "disable",
"delete", etc. don't have this error. Maybe it predated use of
map_breakpoint_numbers and/or error_no_arg? Shouldn't we
just remove it?
> diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
> index 96e2f35..604d957 100644
> --- a/gdb/testsuite/gdb.base/break.exp
> +++ b/gdb/testsuite/gdb.base/break.exp
> @@ -854,3 +854,8 @@ gdb_test_no_output "set \$foo=81.5" \
> gdb_test "break $srcfile:\$foo" \
> "Convenience variables used in line specs must have integer values.*" \
> "set breakpoint via non-integer convenience variable disallowed"
> +
> +
> +# Test that commands can be cleared without error.
> +gdb_test "commands\nend" ">end" "clear breakpoint commands"
> +
It'd be nice to extend this testcase a little bit more, to
check whether this works on a breakpoint that actually
had commands set (as opposed to silently doing nothing) and also
to make sure that the commands were actually cleared, from
"info breakpoints"s output, for example.
I wonder whether gdb.base/commands.exp is a better home for this.
Thanks,
Pedro Alves