From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55293 invoked by alias); 29 Nov 2019 00:54:27 -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 55051 invoked by uid 89); 29 Nov 2019 00:54:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.7 required=5.0 tests=AWL,BAYES_00,BODY_8BITS,GARBLED_BODY,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Nov 2019 00:54:25 +0000 Received: by mail-wm1-f68.google.com with SMTP id y23so3998371wma.0 for ; Thu, 28 Nov 2019 16:54:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=TEgQpEEyOFRXDQ70GO1vTtobKmPOtk4TOncWA5TtfXA=; b=IDZvMIG/k++in8xZ+Vw28lvfHF1g/Rz5OUAOv3ddlp9Vy8Fv3Srkkilb93+HFpucDY 3ONtKPvno0uvloocLtY/oGkWtPeSJ/banyGzbeEpm/voRyP1VSkz+/7oOgCtbwCThArv FwwLO+PRYaduM0/FE27dH3betMskTGoHoWcaB8+bkk+dZkTojJrnFIAK0yNmIc0TKanD e5xiB1FX2FMtY8GEAJA0TWrgsARD7qHKLR/AgvvO9q5eNiYeMN7/yqWGxnrb+SyMRDRR YhIzIC1l5LZcPRD8SaIf2MlaGkGsNFP6zv444EnOopAsbPeiA5jD3rwgx2EFRphWOjF6 m5nQ== Return-Path: Received: from localhost (host109-151-46-117.range109-151.btcentralplus.com. [109.151.46.117]) by smtp.gmail.com with ESMTPSA id t16sm12602822wmt.38.2019.11.28.16.54.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 28 Nov 2019 16:54:21 -0800 (PST) Date: Fri, 29 Nov 2019 00:54:00 -0000 From: Andrew Burgess To: Iain Buclaw Cc: Pedro Alves , "gdb-patches@sourceware.org" Subject: Re: [PATCH v2 1/3] gdb: Rename gdb_flush to ui_file_flush. Message-ID: <20191129005420.GF3410@embecosm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: X-Fortune: Every journalist has a novel in him, which is an excellent place for it. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg01123.txt.bz2 * Iain Buclaw [2019-11-28 23:53:48 +0000]: > =E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90 Original = Message =E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90 > On Wednesday, 27 November 2019 00:00, Iain Buclaw wrote: >=20 > > =E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90 Origina= l Message =E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90=E2=80=90 > > On Tuesday, 26 November 2019 21:24, Pedro Alves palves@redhat.com wrote: > > > > > On 11/26/19 12:49 PM, Iain Buclaw wrote: > > > > > > > The significance of this is that printf_unfiltered writes messages = to wrap_buffer, whereas puts_unfiltered pushes them immediately to stdout, = resulting in "post-" messages being printed out of order. > > > > > > It sounds quite surprising that two _unfiltered functions could behav= e differently > > > like that. That sounds like a bug that should be fixed, instead of wo= rked around > > > by having to recall to use printf vs puts. > > > Thanks, > > > Pedro Alves > > > > I think the best way to avoid the discrepancy is to treat both fputs_fi= ltered and fputs_unfiltered equally by forwarding both calls to fputs_maybe= _filtered. > > > > To avoid recursion, flush_wrap_buffer and fputs_maybe_filtered have had= calls to fputs_unfiltered replaced with stream->puts(). > > > > While attempting to grok my head around fputs_maybe_filtered, I also no= ticed that buffer_clearer is being removed by the compiler as dead code. > > >=20 > Except that doesn't work as some parts of gdb require that direct > writing/flushing stdout is kept in place, so what I ultimately ended > up doing is defining two new functions for this, then repurposing > fputs_unfiltered and gdb_flush to interact with > fputs_maybe_unfiltered. >=20 > I've split the patch into two logical parts, each dealing with one functi= on at a time. Running the testsuitelocally, I can see no new regressions a= s a result of applying this. >=20 > This first patch renames gdb_flush to ui_file_flush. Redefining gdb_flus= h in utils.c, with a new behaviour to flush the wrap_buffer if necessary be= fore flushing the stream. >=20 > -- > Iain >=20 > --- > gdb/ChangeLog: >=20 > 2019-11-28 Iain Buclaw >=20 > * gdb/event-loop.c (gdb_wait_for_event): Update. > * gdb/printcmd.c (printf_command): Update. > * gdb/remote-fileio.c (remote_fileio_func_write): Update. > * gdb/remote-sim.c (gdb_os_flush_stdout): Update. > (gdb_os_flush_stderr): Update. > * gdb/remote.c (remote_console_output): Update. > * gdb/ui-file.c (gdb_flush): Rename to... > (ui_file_flush): ...this. > (stderr_file::write): Update. > (stderr_file::puts): Update. > * gdb/ui-file.h (gdb_flush): Rename to... > (ui_file_flush): ...this. > * gdb/utils.c (gdb_flush): Add function. > * gdb/utils.h (gdb_flush): Add declaration. Iain, thanks for looking at this issue. When posting patches for review you should include the commit message for review too. This should consist of a concise but informative title, a description of the problem, maybe how you discovered it, or what problems it was causing, and then an overview of the solution. >=20 > --- >=20 > diff --git a/gdb/event-loop.c b/gdb/event-loop.c > index affa00b4aa..0d12ac9d41 100644 > --- a/gdb/event-loop.c > +++ b/gdb/event-loop.c > @@ -747,8 +747,8 @@ gdb_wait_for_event (int block) > int num_found =3D 0; >=20=20 > /* Make sure all output is done before getting another event. */ > - gdb_flush (gdb_stdout); > - gdb_flush (gdb_stderr); > + ui_file_flush (gdb_stdout); > + ui_file_flush (gdb_stderr); >=20=20 > if (gdb_notifier.num_fds =3D=3D 0) > return -1; > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index fe0efd371a..e0295940d8 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -2718,7 +2718,7 @@ printf_command (const char *arg, int from_tty) > ui_printf (arg, gdb_stdout); > reset_terminal_style (gdb_stdout); > wrap_here (""); > - gdb_flush (gdb_stdout); > + ui_file_flush (gdb_stdout); > } >=20=20 > /* Implement the "eval" command. */ > diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c > index bc7c71ffdd..e2ee311bf3 100644 > --- a/gdb/remote-fileio.c > +++ b/gdb/remote-fileio.c > @@ -641,7 +641,7 @@ remote_fileio_func_write (remote_target *remote, char= *buf) > case FIO_FD_CONSOLE_OUT: > ui_file_write (target_fd =3D=3D 1 ? gdb_stdtarg : gdb_stdtargerr, > (char *) buffer, length); > - gdb_flush (target_fd =3D=3D 1 ? gdb_stdtarg : gdb_stdtargerr); > + ui_file_flush (target_fd =3D=3D 1 ? gdb_stdtarg : gdb_stdtargerr); > ret =3D length; > break; > default: > diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c > index 67b4690945..f9c2f605c3 100644 > --- a/gdb/remote-sim.c > +++ b/gdb/remote-sim.c > @@ -363,7 +363,7 @@ gdb_os_write_stdout (host_callback *p, const char *bu= f, int len) > static void > gdb_os_flush_stdout (host_callback *p) > { > - gdb_flush (gdb_stdtarg); > + ui_file_flush (gdb_stdtarg); > } >=20=20 > /* GDB version of os_write_stderr callback. */ > @@ -388,7 +388,7 @@ gdb_os_write_stderr (host_callback *p, const char *bu= f, int len) > static void > gdb_os_flush_stderr (host_callback *p) > { > - gdb_flush (gdb_stdtargerr); > + ui_file_flush (gdb_stdtargerr); > } >=20=20 > /* GDB version of printf_filtered callback. */ > diff --git a/gdb/remote.c b/gdb/remote.c > index 3fc9a2608e..054802f744 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -6784,7 +6784,7 @@ remote_console_output (const char *msg) > tb[1] =3D 0; > fputs_unfiltered (tb, gdb_stdtarg); > } > - gdb_flush (gdb_stdtarg); > + ui_file_flush (gdb_stdtarg); > } >=20=20 > struct stop_reply : public notif_event > diff --git a/gdb/ui-file.c b/gdb/ui-file.c > index 71b74bba19..d7d113856e 100644 > --- a/gdb/ui-file.c > +++ b/gdb/ui-file.c > @@ -91,7 +91,7 @@ null_file::write_async_safe (const char *buf, long size= of_buf) > =0C >=20=20 > void > -gdb_flush (struct ui_file *file) > +ui_file_flush (struct ui_file *file) > { > file->flush (); > } > @@ -315,7 +315,7 @@ stdio_file::can_emit_style_escape () > void > stderr_file::write (const char *buf, long length_buf) > { > - gdb_flush (gdb_stdout); > + ui_file_flush (gdb_stdout); > stdio_file::write (buf, length_buf); > } >=20=20 > @@ -325,7 +325,7 @@ stderr_file::write (const char *buf, long length_buf) > void > stderr_file::puts (const char *linebuffer) > { > - gdb_flush (gdb_stdout); > + ui_file_flush (gdb_stdout); > stdio_file::puts (linebuffer); > } >=20=20 > diff --git a/gdb/ui-file.h b/gdb/ui-file.h > index 3f6f38a68f..711a888a2e 100644 > --- a/gdb/ui-file.h > +++ b/gdb/ui-file.h > @@ -100,7 +100,7 @@ public: > /* A preallocated null_file stream. */ > extern null_file null_stream; >=20=20 > -extern void gdb_flush (ui_file *); > +extern void ui_file_flush (ui_file *); >=20=20 > extern int ui_file_isatty (struct ui_file *); >=20=20 > diff --git a/gdb/utils.c b/gdb/utils.c > index f7fae35729..5d6f680bce 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -1544,6 +1544,13 @@ flush_wrap_buffer (struct ui_file *stream) > } > } >=20=20 > +void > +gdb_flush (struct ui_file *stream) > +{ > + flush_wrap_buffer (stream); > + ui_file_flush (stream); > +} All new functions need to have a header comment. The GDB style would be to add this comment here: /* See utils.h. */ And then add a full description of the function in the header file. > + > /* Indicate that if the next sequence of characters overflows the line, > a newline should be inserted here rather than when it hits the end. > If INDENT is non-null, it is a string to be printed to indent the > diff --git a/gdb/utils.h b/gdb/utils.h > index 79c8a6fc8d..185f156a0a 100644 > --- a/gdb/utils.h > +++ b/gdb/utils.h > @@ -323,6 +323,8 @@ extern struct ui_file **current_ui_gdb_stdin_ptr (voi= d); > extern struct ui_file **current_ui_gdb_stderr_ptr (void); > extern struct ui_file **current_ui_gdb_stdlog_ptr (void); >=20=20 > +extern void gdb_flush (struct ui_file *); So you should add a comment here, ideally this would make it clear when I would call this instead of ui_file_flush. Ideally, given you've gone to the effort of understanding all this, it would be nice to have a commented added on ui_file_flush too as that was missing comments before. Thanks, Andrew > + > /* The current top level's ui_file streams. */ >=20=20 > /* Normal results */