* [RFA] Fix regression in "commands"
@ 2017-11-03 19:07 Tom Tromey
2017-11-07 10:49 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2017-11-03 19:07 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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."));
}
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 158fea4..e82d2b9 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-11-03 Tom Tromey <tom@tromey.com>
+
+ * gdb.base/break.exp: Add test for empty "commands".
+
2017-11-03 Yao Qi <yao.qi@linaro.org>
* gdb.mi/list-thread-groups-available.exp: Skip it if XML parsing
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"
+
--
2.9.5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-11-03 19:07 [RFA] Fix regression in "commands" Tom Tromey
@ 2017-11-07 10:49 ` Pedro Alves
2017-12-01 16:45 ` Tom Tromey
2017-12-01 16:56 ` Tom Tromey
0 siblings, 2 replies; 8+ messages in thread
From: Pedro Alves @ 2017-11-07 10:49 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-11-07 10:49 ` Pedro Alves
@ 2017-12-01 16:45 ` Tom Tromey
2017-12-04 15:50 ` Pedro Alves
2017-12-01 16:56 ` Tom Tromey
1 sibling, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2017-12-01 16:45 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Hmm, for the truly "no breakpoints specified" case,
Pedro> it seems to me that before we can reach this error, we've already
Pedro> hit the error at the top of map_breakpoint_numbers:
Pedro> if (args == 0 || *args == '\0')
Pedro> error_no_arg (_("one or more breakpoint numbers"));
Pedro> and for the case where the user specifies some argument
Pedro> that doesn't match any breakpoint, map_breakpoint_numbers
Pedro> already printed one of:
Pedro> warning (_("bad breakpoint number at or near '%s'"), p);
Pedro> printf_unfiltered (_("No breakpoint number %d.\n"), num);
Pedro> when the "No breakpoints specified." error is reached.
I think the problem is that if the error call is removed from
commands_command_1, then a failure here won't throw at all, because
map_breakpoint_number_range just prints a message (either with warning
or printf_unfiltered -- not sure why the discrepancy), and doesn't
throw.
But, this would mean that an script erroneously using "commands" would
not be interrupted, which seems like maybe a bad result. Though, as you
point out, "delete" doesn't do this, so maybe it is ok?
Making map_breakpoint_number_range throw would mean that the case where
you have breakpoint 2 and do "commands 1-3" would now fail -- although
that's worked with a warning since ranges were added.
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-12-01 16:45 ` Tom Tromey
@ 2017-12-04 15:50 ` Pedro Alves
2017-12-07 18:13 ` Tom Tromey
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2017-12-04 15:50 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 12/01/2017 04:45 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> Hmm, for the truly "no breakpoints specified" case,
> Pedro> it seems to me that before we can reach this error, we've already
> Pedro> hit the error at the top of map_breakpoint_numbers:
> Pedro> if (args == 0 || *args == '\0')
> Pedro> error_no_arg (_("one or more breakpoint numbers"));
> Pedro> and for the case where the user specifies some argument
> Pedro> that doesn't match any breakpoint, map_breakpoint_numbers
> Pedro> already printed one of:
> Pedro> warning (_("bad breakpoint number at or near '%s'"), p);
> Pedro> printf_unfiltered (_("No breakpoint number %d.\n"), num);
> Pedro> when the "No breakpoints specified." error is reached.
>
> I think the problem is that if the error call is removed from
> commands_command_1, then a failure here won't throw at all, because
> map_breakpoint_number_range just prints a message (either with warning
> or printf_unfiltered -- not sure why the discrepancy), and doesn't
> throw.
>
> But, this would mean that an script erroneously using "commands" would
> not be interrupted, which seems like maybe a bad result. Though, as you
> point out, "delete" doesn't do this, so maybe it is ok?
I think it's OK in the principle that "delete", "disable", etc.
which seem like used-more-frequently commands, don't do this.
I think that if we want to error out, then we should do that
to all the commands that take command lists/ranges, and then
maybe do it directly in map_breakpoint_number_range, perhaps.
But I'd start with making "commands" not-error like the
others.
>
> Making map_breakpoint_number_range throw would mean that the case where
> you have breakpoint 2 and do "commands 1-3" would now fail -- although
> that's worked with a warning since ranges were added.
Right, doesn't seem very friently to make those error out.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-12-04 15:50 ` Pedro Alves
@ 2017-12-07 18:13 ` Tom Tromey
2017-12-07 18:27 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2017-12-07 18:13 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
Pedro> But I'd start with making "commands" not-error like the
Pedro> others.
Ok, how about this?
Tom
commit 657cbb04bd450d2217679d33a91d2342fe02c4b9
Author: Tom Tromey <tom@tromey.com>
Date: Fri Nov 3 10:26:11 2017 -0600
Fix regression in "commands"
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.
After some discussion, we agreed to simply remove the error case from
commands_command_1.
2017-12-07 Tom Tromey <tom@tromey.com>
PR breakpoints/22511:
* breakpoint.c (commands_command_1): Don't throw an exception when
no commands have been read.
2017-12-07 Tom Tromey <tom@tromey.com>
* gdb.base/break.exp: Add test for empty "commands".
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 54aec7f7d9..7c1dff1c35 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2017-12-07 Tom Tromey <tom@tromey.com>
+
+ PR breakpoints/22511:
+ * breakpoint.c (commands_command_1): Don't throw an exception when
+ no commands have been read.
+
2017-12-07 Simon Marchi <simon.marchi@ericsson.com>
* common/selftest.h (struct selftest): Add virtual destructor.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 76bfd53a43..59a4dad3cf 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1272,9 +1272,6 @@ commands_command_1 (const char *arg, int from_tty,
observer_notify_breakpoint_modified (b);
}
});
-
- if (cmd == NULL)
- error (_("No breakpoints specified."));
}
static void
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 87547cbe13..d52625bd74 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-12-07 Tom Tromey <tom@tromey.com>
+
+ * gdb.base/break.exp: Add test for empty "commands".
+
2017-12-07 Phil Muldoon <pmuldoon@redhat.com>
* gdb.python/py-breakpoint.exp (test_bkpt_explicit_loc): Add new
diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
index 96e2f35003..87db2dc0d3 100644
--- a/gdb/testsuite/gdb.base/break.exp
+++ b/gdb/testsuite/gdb.base/break.exp
@@ -854,3 +854,15 @@ 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\nprint 232323\nend" ">end" "set some breakpoint commands"
+gdb_test "commands\nend" ">end" "clear breakpoint commands"
+# We verify that the commands were cleared by ensuring that the last
+# breakpoint's location ends the output -- if there were commands,
+# they would have been printed after the location.
+gdb_test "info break" "$srcfile:$line" "verify that they were cleared"
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-11-07 10:49 ` Pedro Alves
2017-12-01 16:45 ` Tom Tromey
@ 2017-12-01 16:56 ` Tom Tromey
2017-12-04 15:55 ` Pedro Alves
1 sibling, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2017-12-01 16:56 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> I wonder whether gdb.base/commands.exp is a better home for this.
That file has some tests of "commands" but also is a grab-bag of testing
random commands. I don't think the name specifically has to do with the
"commands" command; so unless you feel strongly about it, I'll just
leave it in break.exp.
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA] Fix regression in "commands"
2017-12-01 16:56 ` Tom Tromey
@ 2017-12-04 15:55 ` Pedro Alves
0 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2017-12-04 15:55 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 12/01/2017 04:56 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> I wonder whether gdb.base/commands.exp is a better home for this.
>
> That file has some tests of "commands" but also is a grab-bag of testing
> random commands. I don't think the name specifically has to do with the
> "commands" command; so unless you feel strongly about it, I'll just
> leave it in break.exp.
Right, I wasn't thinking about the "commands" command specifically --
I think that testcase is more about the support for user-defined
commands / scripting than about random commands, and there are
some tests in there about breakpoint commands.
But really I don't feel strongly about it at all. break.exp is fine.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-07 18:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-03 19:07 [RFA] Fix regression in "commands" Tom Tromey
2017-11-07 10:49 ` Pedro Alves
2017-12-01 16:45 ` Tom Tromey
2017-12-04 15:50 ` Pedro Alves
2017-12-07 18:13 ` Tom Tromey
2017-12-07 18:27 ` Pedro Alves
2017-12-01 16:56 ` Tom Tromey
2017-12-04 15:55 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).