From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7200 invoked by alias); 13 Aug 2018 21:32:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 7188 invoked by uid 89); 13 Aug 2018 21:32:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=told, heres, here's, UD:be X-HELO: mailsec108.isp.belgacom.be Received: from mailsec108.isp.belgacom.be (HELO mailsec108.isp.belgacom.be) (195.238.20.104) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 13 Aug 2018 21:32:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1534195950; x=1565731950; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=5DkkcvwisemwpF71R4bZSj4pmImT143841iFCPIucsc=; b=HkRxj600IOWDLDiro0WxWfOOdLxb7+nG4E0alSYhKIncAgDlthZecGj2 VAa9QghBsuzbhCvwVa6iExFQGTjjUw==; Received: from 217.24-133-109.adsl-dyn.isp.belgacom.be (HELO md) ([109.133.24.217]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 13 Aug 2018 23:32:28 +0200 Message-ID: <1534195947.15655.2.camel@skynet.be> Subject: Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing From: Philippe Waroquiers To: Tom Tromey Cc: gdb-patches@sourceware.org Date: Mon, 13 Aug 2018 21:32:00 -0000 In-Reply-To: <8736vktz97.fsf@tromey.com> References: <20180802212613.29813-1-philippe.waroquiers@skynet.be> <20180802212613.29813-2-philippe.waroquiers@skynet.be> <87sh3v1ezc.fsf@tromey.com> <87lg9gi1c4.fsf@tromey.com> <1533845999.1860.1.camel@skynet.be> <878t5fhxdl.fsf@tromey.com> <87ftzmvs42.fsf@tromey.com> <87bmaavrr6.fsf@tromey.com> <87ftzm5hp3.fsf@tromey.com> <8736vktz97.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00333.txt.bz2 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, >