[RFA] 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.

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(-)
  

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
  

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

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

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
  

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