[RFA] Fix regression in "commands"

[RFA] Fix regression in "commands"

[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

Re: [RFA] Fix regression in "commands"

[Tom Tromey]

>>>>> "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

Re: [RFA] Fix regression in "commands"

[Pedro Alves]

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

Re: [RFA] Fix regression in "commands"

[Tom Tromey]

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"

Re: [RFA] Fix regression in "commands"

[Pedro Alves]

On 12/07/2017 06:13 PM, Tom Tromey wrote:
> Pedro> But I'd start with making "commands" not-error like the
> Pedro> others.
> 
> Ok, how about this?

Great, thanks!

Pedro Alves

Re: [RFA] Fix regression in "commands"

[Tom Tromey]

>>>>> "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

Re: [RFA] Fix regression in "commands"

[Pedro Alves]

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