This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Fix regression in "commands"


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]