[v2,2/3] gdb: Rename fputs_unfiltered to ui_file_puts.

Message ID XQGyMnfalxm1OOKSZo3TrFUyNA0xu3oTmFB4ITVbCxtNngUKDgi0LlhRPG5-6BD2j30Hxpxyii45hKT4y0yrUM6uxTtNQ4WCQzPqgVZy_Qk=@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. 29, 2019, 12:03 a.m.
This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.

Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.

This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.

--
Iain

---
gdb/ChangeLog:

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

        * gdb/remote-sim.c (gdb_os_write_stderr): Update.
        * gdb/remote.c (remote_console_output): Update.
        * gdb/ui-file.c (fputs_unfiltered): Rename to...
        (ui_file_puts): ...this.
        * gdb/ui-file.h (ui_file_puts): Add declaration.
        * gdb/utils.c (emit_style_escape): Update.
        (flush_wrap_buffer): Update.
        (fputs_maybe_filtered): Update.
        (fputs_unfiltered): Add function.

---

Comments

Simon Marchi Dec. 1, 2019, 3:12 p.m. | #1
On 2019-11-28 7:03 p.m., Iain Buclaw wrote:
> This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.

> 

> Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.

> 

> This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.

> 

> --

> Iain

> 

> ---

> gdb/ChangeLog:

> 

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

> 

>         * gdb/remote-sim.c (gdb_os_write_stderr): Update.

>         * gdb/remote.c (remote_console_output): Update.

>         * gdb/ui-file.c (fputs_unfiltered): Rename to...

>         (ui_file_puts): ...this.

>         * gdb/ui-file.h (ui_file_puts): Add declaration.

>         * gdb/utils.c (emit_style_escape): Update.

>         (flush_wrap_buffer): Update.

>         (fputs_maybe_filtered): Update.

>         (fputs_unfiltered): Add function.

> 

> ---

> 

> 


Hi Iain,

The patch does not apply for me on current master:

$ git apply ~/patches/uifileputs.patch
error: patch failed: gdb/remote.c:6782
error: gdb/remote.c: patch does not apply

How did you generate this patch?  Could you please send your patches using git-send-email?

Simon
Simon Marchi Dec. 1, 2019, 3:15 p.m. | #2
On 2019-12-01 10:12 a.m., Simon Marchi wrote:
> On 2019-11-28 7:03 p.m., Iain Buclaw wrote:

>> This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.

>>

>> Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.

>>

>> This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.

>>

>> --

>> Iain

>>

>> ---

>> gdb/ChangeLog:

>>

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

>>

>>         * gdb/remote-sim.c (gdb_os_write_stderr): Update.

>>         * gdb/remote.c (remote_console_output): Update.

>>         * gdb/ui-file.c (fputs_unfiltered): Rename to...

>>         (ui_file_puts): ...this.

>>         * gdb/ui-file.h (ui_file_puts): Add declaration.

>>         * gdb/utils.c (emit_style_escape): Update.

>>         (flush_wrap_buffer): Update.

>>         (fputs_maybe_filtered): Update.

>>         (fputs_unfiltered): Add function.

>>

>> ---

>>

>>

> 

> Hi Iain,

> 

> The patch does not apply for me on current master:

> 

> $ git apply ~/patches/uifileputs.patch

> error: patch failed: gdb/remote.c:6782

> error: gdb/remote.c: patch does not apply

> 

> How did you generate this patch?  Could you please send your patches using git-send-email?

> 

> Simon


My bad, I had not seen that this was part of a series (the email is not threaded with the others
of the series).  After applying 1/3, this one applies fine.

Simon
Joel Brobecker Jan. 26, 2020, 11:31 a.m. | #3
Hi Iain,

> This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.

> 

> Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.

> 

> This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.

> 

> --

> Iain

> 

> ---

> gdb/ChangeLog:

> 

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

> 

>         * gdb/remote-sim.c (gdb_os_write_stderr): Update.

>         * gdb/remote.c (remote_console_output): Update.

>         * gdb/ui-file.c (fputs_unfiltered): Rename to...

>         (ui_file_puts): ...this.

>         * gdb/ui-file.h (ui_file_puts): Add declaration.

>         * gdb/utils.c (emit_style_escape): Update.

>         (flush_wrap_buffer): Update.

>         (fputs_maybe_filtered): Update.

>         (fputs_unfiltered): Add function.


As far as I can tell, no one has reviewed this patch. This is a call
for help from other Global Maintainers, as I don't feel qualified
to review it all on my own.

As far as I can tell from reading this patch, it really feels like
you are trying to turn the turn the relationship of filtered vs
unfiltered on its head. Right now, fputs_maybe_filtered is the one
that depends on fputs_unfiltered, and this is realy what feels
natural to me.

For some reason, you decided to do the other way around, but without
a real explanation of why we are getting text being printed in the wrong
order, and why your patch fixes it or why you think this is the best
approach.


> 

> ---

> 

> 


> diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c

> index f9c2f605c3..b7eeccf91b 100644

> --- a/gdb/remote-sim.c

> +++ b/gdb/remote-sim.c

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

>      {

>        b[0] = buf[i];

>        b[1] = 0;

> -      fputs_unfiltered (b, gdb_stdtargerr);

> +      ui_file_puts (gdb_stdtargerr, b);

>      }

>    return len;

>  }

> diff --git a/gdb/remote.c b/gdb/remote.c

> index 054802f744..7cc2a0470b 100644

> --- a/gdb/remote.c

> +++ b/gdb/remote.c

> @@ -6782,7 +6782,7 @@ remote_console_output (const char *msg)

>  

>        tb[0] = c;

>        tb[1] = 0;

> -      fputs_unfiltered (tb, gdb_stdtarg);

> +      ui_file_puts (gdb_stdtarg, tb);

>      }

>    ui_file_flush (gdb_stdtarg);

>  }

> diff --git a/gdb/ui-file.c b/gdb/ui-file.c

> index d7d113856e..6a3ebfb6a9 100644

> --- a/gdb/ui-file.c

> +++ b/gdb/ui-file.c

> @@ -150,7 +150,7 @@ ui_file_read (struct ui_file *file, char *buf, long length_buf)

>  }

>  

>  void

> -fputs_unfiltered (const char *buf, struct ui_file *file)

> +ui_file_puts (struct ui_file *file, const char *buf)

>  {

>    file->puts (buf);

>  }

> diff --git a/gdb/ui-file.h b/gdb/ui-file.h

> index 711a888a2e..8b57622a09 100644

> --- a/gdb/ui-file.h

> +++ b/gdb/ui-file.h

> @@ -112,6 +112,8 @@ extern void ui_file_write_async_safe (struct ui_file *file, const char *buf,

>  

>  extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);

>  

> +extern void ui_file_puts (struct ui_file *file, const char *buf);

> +

>  extern int gdb_console_fputs (const char *, FILE *);

>  

>  /* A std::string-based ui_file.  Can be used as a scratch buffer for

> diff --git a/gdb/utils.c b/gdb/utils.c

> index 5d6f680bce..0e09f646bf 100644

> --- a/gdb/utils.c

> +++ b/gdb/utils.c

> @@ -1405,7 +1405,7 @@ emit_style_escape (const ui_file_style &style,

>    if (stream == nullptr)

>      wrap_buffer.append (style.to_ansi ());

>    else

> -    fputs_unfiltered (style.to_ansi ().c_str (), stream);

> +    ui_file_puts (stream, style.to_ansi ().c_str ());

>  }

>  

>  /* Set the current output style.  This will affect future uses of the

> @@ -1539,7 +1539,7 @@ flush_wrap_buffer (struct ui_file *stream)

>  {

>    if (stream == gdb_stdout && !wrap_buffer.empty ())

>      {

> -      fputs_unfiltered (wrap_buffer.c_str (), stream);

> +      ui_file_puts (stream, wrap_buffer.c_str ());

>        wrap_buffer.clear ();

>      }

>  }

> @@ -1695,7 +1695,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,

>        || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())

>      {

>        flush_wrap_buffer (stream);

> -      fputs_unfiltered (linebuffer, stream);

> +      ui_file_puts (stream, linebuffer);

>        return;

>      }

>  

> @@ -1795,7 +1795,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,

>  	      /* Now output indentation and wrapped string.  */

>  	      if (wrap_column)

>  		{

> -		  fputs_unfiltered (wrap_indent, stream);

> +		  ui_file_puts (stream, wrap_indent);

>  		  if (stream->can_emit_style_escape ())

>  		    emit_style_escape (save_style, stream);

>  		  /* FIXME, this strlen is what prevents wrap_indent from

> @@ -1833,6 +1833,12 @@ fputs_filtered (const char *linebuffer, struct ui_file *stream)

>    fputs_maybe_filtered (linebuffer, stream, 1);

>  }

>  

> +void

> +fputs_unfiltered (const char *linebuffer, struct ui_file *stream)

> +{

> +  fputs_maybe_filtered (linebuffer, stream, 0);

> +}

> +

>  /* See utils.h.  */

>  

>  void



-- 
Joel
Eli Zaretskii via Gdb-patches Aug. 7, 2020, 10:15 p.m. | #4
Excerpts from Joel Brobecker's message of January 26, 2020 12:31 pm:
> Hi Iain,

> 

>> This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.

>>

>> Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.

>>

>> This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.

>>

>> --

>> Iain

>>

>> ---

>> gdb/ChangeLog:

>>

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

>>

>>         * gdb/remote-sim.c (gdb_os_write_stderr): Update.

>>         * gdb/remote.c (remote_console_output): Update.

>>         * gdb/ui-file.c (fputs_unfiltered): Rename to...

>>         (ui_file_puts): ...this.

>>         * gdb/ui-file.h (ui_file_puts): Add declaration.

>>         * gdb/utils.c (emit_style_escape): Update.

>>         (flush_wrap_buffer): Update.

>>         (fputs_maybe_filtered): Update.

>>         (fputs_unfiltered): Add function.

> 

> As far as I can tell, no one has reviewed this patch. This is a call

> for help from other Global Maintainers, as I don't feel qualified

> to review it all on my own.

> 

> As far as I can tell from reading this patch, it really feels like

> you are trying to turn the turn the relationship of filtered vs

> unfiltered on its head. Right now, fputs_maybe_filtered is the one

> that depends on fputs_unfiltered, and this is realy what feels

> natural to me.

> 

> For some reason, you decided to do the other way around, but without

> a real explanation of why we are getting text being printed in the wrong

> order, and why your patch fixes it or why you think this is the best

> approach.

> 

> 


Hi Joel,

I guess that someone answered your question, as it appears that this
patch got committed anyway.

To give my best explanation on what I saw anyway.  There are a number of
print related functions that each followed a consistent behaviour except
fputs_unfiltered.

 - printf_filtered called vfprintf_maybe_filtered(/*filter=*/true).
 - printf_unfiltered called vfprintf_maybe_filtered(false).
 - fprintf_filtered called vfprintf_maybe_filtered(true).
 - fprintf_unfiltered called vfprintf_maybe_filtered(false).
 - fputs_styled called fputs_maybe_filtered(/*filter=*/1).
 - fputs_styled_unfiltered called fputs_maybe_filtered(0).
 - fputs_filtered called fputs_maybe_filtered(1).
 - fputs_unfiltered called ui_file::puts()

Before 2a3c1174c3c0db1140180fb3fc56ac324d1c0a7c, all *printf_unfiltered
functions used to call fputs_unfiltered(), so there was no problems
mixing printf_unfiltered() and fputs_unfiltered().

After the change though, printf_unfiltered() now got written to a buffer
awaiting to be flushed whilst fputs_unfiltered() continued to write
directly to the output file.

This is the part that caused the regression:
---
@@ -2064,13 +2096,13 @@ vfprintf_unfiltered (struct ui_file *stream, const char *format, va_list args)
       fputs_unfiltered (timestamp.c_str (), stream);
     }
   else
-    fputs_unfiltered (linebuffer.c_str (), stream);
+    vfprintf_maybe_filtered (stream, format, args, false, true);
 }

 void
 vprintf_filtered (const char *format, va_list args)
---

Reverting the one line caused other problems however, so the only way I
saw to reasonably move forward was to bring fputs_unfiltered in line
with all others I've listed above, so now it calls fputs_maybe_filtered(0),
and no one has to worry about printf_unfiltered and fputs_unfiltered
being flushed in a different order to the one they were called in.

Hope this is clear.

Iain.

Patch

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index f9c2f605c3..b7eeccf91b 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -378,7 +378,7 @@  gdb_os_write_stderr (host_callback *p, const char *buf, int len)
     {
       b[0] = buf[i];
       b[1] = 0;
-      fputs_unfiltered (b, gdb_stdtargerr);
+      ui_file_puts (gdb_stdtargerr, b);
     }
   return len;
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index 054802f744..7cc2a0470b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6782,7 +6782,7 @@  remote_console_output (const char *msg)
 
       tb[0] = c;
       tb[1] = 0;
-      fputs_unfiltered (tb, gdb_stdtarg);
+      ui_file_puts (gdb_stdtarg, tb);
     }
   ui_file_flush (gdb_stdtarg);
 }
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index d7d113856e..6a3ebfb6a9 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -150,7 +150,7 @@  ui_file_read (struct ui_file *file, char *buf, long length_buf)
 }
 
 void
-fputs_unfiltered (const char *buf, struct ui_file *file)
+ui_file_puts (struct ui_file *file, const char *buf)
 {
   file->puts (buf);
 }
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 711a888a2e..8b57622a09 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -112,6 +112,8 @@  extern void ui_file_write_async_safe (struct ui_file *file, const char *buf,
 
 extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);
 
+extern void ui_file_puts (struct ui_file *file, const char *buf);
+
 extern int gdb_console_fputs (const char *, FILE *);
 
 /* A std::string-based ui_file.  Can be used as a scratch buffer for
diff --git a/gdb/utils.c b/gdb/utils.c
index 5d6f680bce..0e09f646bf 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1405,7 +1405,7 @@  emit_style_escape (const ui_file_style &style,
   if (stream == nullptr)
     wrap_buffer.append (style.to_ansi ());
   else
-    fputs_unfiltered (style.to_ansi ().c_str (), stream);
+    ui_file_puts (stream, style.to_ansi ().c_str ());
 }
 
 /* Set the current output style.  This will affect future uses of the
@@ -1539,7 +1539,7 @@  flush_wrap_buffer (struct ui_file *stream)
 {
   if (stream == gdb_stdout && !wrap_buffer.empty ())
     {
-      fputs_unfiltered (wrap_buffer.c_str (), stream);
+      ui_file_puts (stream, wrap_buffer.c_str ());
       wrap_buffer.clear ();
     }
 }
@@ -1695,7 +1695,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
       || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
     {
       flush_wrap_buffer (stream);
-      fputs_unfiltered (linebuffer, stream);
+      ui_file_puts (stream, linebuffer);
       return;
     }
 
@@ -1795,7 +1795,7 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	      /* Now output indentation and wrapped string.  */
 	      if (wrap_column)
 		{
-		  fputs_unfiltered (wrap_indent, stream);
+		  ui_file_puts (stream, wrap_indent);
 		  if (stream->can_emit_style_escape ())
 		    emit_style_escape (save_style, stream);
 		  /* FIXME, this strlen is what prevents wrap_indent from
@@ -1833,6 +1833,12 @@  fputs_filtered (const char *linebuffer, struct ui_file *stream)
   fputs_maybe_filtered (linebuffer, stream, 1);
 }
 
+void
+fputs_unfiltered (const char *linebuffer, struct ui_file *stream)
+{
+  fputs_maybe_filtered (linebuffer, stream, 0);
+}
+
 /* See utils.h.  */
 
 void