[v2,1/3] gdb: Rename gdb_flush to ui_file_flush.

Message ID BuxW80FcA6zgjgKFW9D1hzrw1DLT-p51RpQWQcdTGAzFfs_ci0NWq2uaxztalvDmk-Igt1OFqjNN5nq7SN5WlLUdXKjCYS1E0qVfKVZ886g=@gdcproject.org
State New
Headers show
Series
  • [v2,1/3] gdb: Rename gdb_flush to ui_file_flush.
Related show

Commit Message

Iain Buclaw Nov. 28, 2019, 11:53 p.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 27 November 2019 00:00, Iain Buclaw <ibuclaw@gdcproject.org> wrote:

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> 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 behave differently

> > like that. That sounds like a bug that should be fixed, instead of worked 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_filtered 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 noticed that buffer_clearer is being removed by the compiler as dead code.

>


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.

I've split the patch into two logical parts, each dealing with one function at a time.  Running the testsuitelocally, I can see no new regressions as a result of applying this.

This first patch renames gdb_flush to ui_file_flush.  Redefining gdb_flush in utils.c, with a new behaviour to flush the wrap_buffer if necessary before flushing the stream.

--
Iain

---
gdb/ChangeLog:

2019-11-28  Iain Buclaw  <ibuclaw@gdcproject.org>

        * 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.

---

Comments

Andrew Burgess Nov. 29, 2019, 12:54 a.m. | #1
* Iain Buclaw <ibuclaw@gdcproject.org> [2019-11-28 23:53:48 +0000]:

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> On Wednesday, 27 November 2019 00:00, Iain Buclaw <ibuclaw@gdcproject.org> wrote:

> 

> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> > 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 behave differently

> > > like that. That sounds like a bug that should be fixed, instead of worked 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_filtered 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 noticed that buffer_clearer is being removed by the compiler as dead code.

> >

> 

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

> 

> I've split the patch into two logical parts, each dealing with one function at a time.  Running the testsuitelocally, I can see no new regressions as a result of applying this.

> 

> This first patch renames gdb_flush to ui_file_flush.  Redefining gdb_flush in utils.c, with a new behaviour to flush the wrap_buffer if necessary before flushing the stream.

> 

> --

> Iain

> 

> ---

> gdb/ChangeLog:

> 

> 2019-11-28  Iain Buclaw  <ibuclaw@gdcproject.org>

> 

>         * 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.

> 

> ---

> 


> 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 = 0;

>  

>    /* 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);

>  

>    if (gdb_notifier.num_fds == 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);

>  }

>  

>  /* 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 == 1 ? gdb_stdtarg : gdb_stdtargerr,

>  		       (char *) buffer, length);

> -	gdb_flush (target_fd == 1 ? gdb_stdtarg : gdb_stdtargerr);

> +	ui_file_flush (target_fd == 1 ? gdb_stdtarg : gdb_stdtargerr);

>  	ret = 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 *buf, int len)

>  static void

>  gdb_os_flush_stdout (host_callback *p)

>  {

> -  gdb_flush (gdb_stdtarg);

> +  ui_file_flush (gdb_stdtarg);

>  }

>  

>  /* GDB version of os_write_stderr callback.  */

> @@ -388,7 +388,7 @@ gdb_os_write_stderr (host_callback *p, const char *buf, int len)

>  static void

>  gdb_os_flush_stderr (host_callback *p)

>  {

> -  gdb_flush (gdb_stdtargerr);

> +  ui_file_flush (gdb_stdtargerr);

>  }

>  

>  /* 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] = 0;

>        fputs_unfiltered (tb, gdb_stdtarg);

>      }

> -  gdb_flush (gdb_stdtarg);

> +  ui_file_flush (gdb_stdtarg);

>  }

>  

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

>  

>  

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

>  }

>  

> @@ -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);

>  }

>  

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

>  

> -extern void gdb_flush (ui_file *);

> +extern void ui_file_flush (ui_file *);

>  

>  extern int ui_file_isatty (struct ui_file *);

>  

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

>      }

>  }

>  

> +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 (void);

>  extern struct ui_file **current_ui_gdb_stderr_ptr (void);

>  extern struct ui_file **current_ui_gdb_stdlog_ptr (void);

>  

> +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.  */

>  

>  /* Normal results */

Patch

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 = 0;
 
   /* 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);
 
   if (gdb_notifier.num_fds == 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);
 }
 
 /* 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 == 1 ? gdb_stdtarg : gdb_stdtargerr,
 		       (char *) buffer, length);
-	gdb_flush (target_fd == 1 ? gdb_stdtarg : gdb_stdtargerr);
+	ui_file_flush (target_fd == 1 ? gdb_stdtarg : gdb_stdtargerr);
 	ret = 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 *buf, int len)
 static void
 gdb_os_flush_stdout (host_callback *p)
 {
-  gdb_flush (gdb_stdtarg);
+  ui_file_flush (gdb_stdtarg);
 }
 
 /* GDB version of os_write_stderr callback.  */
@@ -388,7 +388,7 @@  gdb_os_write_stderr (host_callback *p, const char *buf, int len)
 static void
 gdb_os_flush_stderr (host_callback *p)
 {
-  gdb_flush (gdb_stdtargerr);
+  ui_file_flush (gdb_stdtargerr);
 }
 
 /* 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] = 0;
       fputs_unfiltered (tb, gdb_stdtarg);
     }
-  gdb_flush (gdb_stdtarg);
+  ui_file_flush (gdb_stdtarg);
 }
 
 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 sizeof_buf)
 
 
 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);
 }
 
@@ -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);
 }
 
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;
 
-extern void gdb_flush (ui_file *);
+extern void ui_file_flush (ui_file *);
 
 extern int ui_file_isatty (struct ui_file *);
 
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)
     }
 }
 
+void
+gdb_flush (struct ui_file *stream)
+{
+  flush_wrap_buffer (stream);
+  ui_file_flush (stream);
+}
+
 /* 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 (void);
 extern struct ui_file **current_ui_gdb_stderr_ptr (void);
 extern struct ui_file **current_ui_gdb_stdlog_ptr (void);
 
+extern void gdb_flush (struct ui_file *);
+
 /* The current top level's ui_file streams.  */
 
 /* Normal results */