[RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing

[Philippe Waroquiers]

On Sat, 2018-08-11 at 14:38 -0600, Tom Tromey wrote:
> Tom> Further testing shows that this regresses some MI tests when compiled
> Tom> with -fsanitize=address.
> 
> Here's another, more convoluted, variant that seems to fix all the
> problems.
> 
> Let me know what you think.  I'm eager to be rid of this patch :)
Yes, that is understandable, the patch starts to be impressive :).

I cannot really give any comment about the patch approach,
as all the command line handling/repeating/... is rather mysterious
to me, and it is not very clear to me which problem in MI had to
be solved.

But find below some minor comments or questions ...
Thanks
Philippe


...
> diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
> index 2aa41d6c8b..8deb308ef8 100644
> --- a/gdb/cli/cli-interp.c
> +++ b/gdb/cli/cli-interp.c
> @@ -30,6 +30,7 @@
>  #include "gdbthread.h"
>  #include "thread-fsm.h"
>  #include "inferior.h"
> +#include "gdbcmd.h"
As I understand, this is needed to have the new overload
execute_command.

Why is this new overload not defined in top.h ?
(where the first execute_command was defined ?)
Wouldn't that avoid to have to include gdbcmd.h in many
places ?
So, unclear why the execute_command was moved from top.h
to gdbcmd.h (but the implementation of execute_command
with the new can_repeat bool is still in top.c).
It would seem more natural to have all the execute_command
in top.h (and top.c).
(maybe the above is very much Ada minded, as Ada
enforces a consistency between the declaration and
the implementation of a function, including where
the declaration is located).

Assuming this is better placed in gdbcmd.h, then there is
at least one place which says in cli-interp.c:
#include "top.h"		/* for "execute_command" */

>  
>  cli_interp_base::cli_interp_base (const char *name)
>    : interp (name)
> 
...

> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 5852089f09..b2abb6aa62 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -565,7 +565,7 @@ async_disable_stdin (void)
>     a whole command.  */
>  
>  void
> -command_handler (const char *command)
> +command_handler (const char *command, bool can_repeat)
>  {
>    struct ui *ui = current_ui;
>    const char *c;
> @@ -580,7 +580,7 @@ command_handler (const char *command)
>      ;
>    if (c[0] != '#')
>      {
> -      execute_command (command, ui->instream == ui->stdin_stream);
> +      execute_command (command, ui->instream == ui->stdin_stream, can_repeat);
>  
>        /* Do any commands attached to breakpoint we stopped at.  */
>        bpstat_do_actions ();
> @@ -631,19 +631,24 @@ command_line_append_input_line (struct buffer *cmd_line_buffer, char *rl)
>  
>     Returns EOF on end of file.
>  
> -   If REPEAT, handle command repetitions:
> +   REPEAT is both an input and output flag.
*REPEAT is only set to true, never set to false.

So, the idea is that the caller initialises it to false ?
(the 'input flag' is then a little bit confusing, because
the flag value is never read, even if it has to be initialised).

Or told otherwise, code like the below
  repeat = false;
   /* this might do repeat */
  handle_line_of_input (..., &repeat, ....);
is a little bit confusing ...

Maybe the arg should be called everywhere can_repeat
(and should be in output set to false/true) ?

> +   If REPEAT is NULL, do not handle command repetitions.
> +   Otherwise (if it is not NULL):
> +
> +     - If the input can possibly be repeated (basically, no "server"
> +       prefix), then *REPEAT is set to true.
>  
>       - If the input command line is NOT empty, the command returned is
>         copied into the global 'saved_command_line' var so that it can
>         be repeated later.
>  
> -     - OTOH, if the input command line IS empty, return the previously
> -       saved command instead of the empty input line.
> +     - OTOH, if the input command line IS empty, return a copy the
Typo:   a copy of the




> +       previously saved command instead of the empty input line.
>  */
>  
>  char *
>  handle_line_of_input (struct buffer *cmd_line_buffer,
> -		      char *rl, int repeat, const char *annotation_suffix)
> +		      char *rl, bool *repeat, const char *annotation_suffix)
>  {
>    struct ui *ui = current_ui;
>    int from_tty = ui->instream == ui->stdin_stream;
> @@ -713,8 +718,14 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
>       previous command, return the previously saved command.  */
>    for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++)
>      ;
> -  if (repeat && *p1 == '\0')
> -    return saved_command_line;
> +  if (repeat != nullptr && *p1 == '\0')
> +    {
> +      *repeat = true;
> +      xfree (buffer_finish (cmd_line_buffer));
> +      buffer_grow_str0 (cmd_line_buffer, saved_command_line);
> +      cmd_line_buffer->used_size = 0;
> +      return cmd_line_buffer->buffer;
> +    }
>  
>    /* Add command to history if appropriate.  Note: lines consisting
>       solely of comments are also added to the command history.  This
> @@ -727,14 +738,13 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
>      gdb_add_history (cmd);
>  
>    /* Save into global buffer if appropriate.  */
> -  if (repeat)
> +  if (repeat != nullptr)
>      {
> +      *repeat = true;
>        xfree (saved_command_line);
>        saved_command_line = xstrdup (cmd);
> -      return saved_command_line;
>      }
> -  else
> -    return cmd;
> +  return cmd;
>  }
>  
>  /* Handle a complete line of input.  This is called by the callback
> @@ -751,8 +761,9 @@ command_line_handler (char *rl)
>    struct buffer *line_buffer = get_command_line_buffer ();
>    struct ui *ui = current_ui;
>    char *cmd;
> +  bool can_repeat = false;
>  
> -  cmd = handle_line_of_input (line_buffer, rl, 1, "prompt");
> +  cmd = handle_line_of_input (line_buffer, rl, &can_repeat, "prompt");
>    if (cmd == (char *) EOF)
>      {
>        /* stdin closed.  The connection with the terminal is gone.
> @@ -771,7 +782,7 @@ command_line_handler (char *rl)
>      {
>        ui->prompt_state = PROMPT_NEEDED;
>  
> -      command_handler (cmd);
> +      command_handler (cmd, can_repeat);
>  
>        if (ui->prompt_state != PROMPTED)
>  	display_gdb_prompt (0);
> 

...

> diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h
> index b675ae8618..cbb42249d0 100644
> --- a/gdb/gdbcmd.h
> +++ b/gdb/gdbcmd.h
> @@ -132,7 +132,14 @@ extern struct cmd_list_element *showchecklist;
>  
>  extern struct cmd_list_element *save_cmdlist;
>  
> -extern void execute_command (const char *, int);
> +extern void execute_command (const char *, int, bool);
> +
> +static inline void
> +execute_command (const char *arg, int from_tty)
> +{
> +  return execute_command (arg, from_tty, false);
function is void, no need for the return ?



> +}
> +
>  extern std::string execute_command_to_string (const char *p, int from_tty);
>  
>  extern void print_command_line (struct command_line *, unsigned int,
>