gdb: Use puts_unfiltered instead of printf_unfiltered

Message ID Bp1z1H0Uk-JcW1j7NMWgeijH4pFLAM5zyY6-OMDHyXwIBLc_qKy0LFyx15VMcg6gEd4IKqxbhN1mBXE5Hd0jY0U9qZj0-Poz4hQHHoYlKDc=@gdcproject.org
State New
Headers show
Series
  • gdb: Use puts_unfiltered instead of printf_unfiltered
Related show

Commit Message

Iain Buclaw Nov. 26, 2019, 12:49 p.m.
Hi,

This patch fixes a regression, possibly introduced by 2a3c1174c3c0db1140180fb3fc56ac324d1c0a7c, in this part of the change:

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

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.

Not sure about how to go about testing this, looking at the testsuite, such as gdb.base/annota1.exp, everything appears to be in order.  Perhaps this is because the testsuite triggers one of these conditions in fputs_maybe_filtered() though.

---
if (stream != gdb_stdout
    || !pagination_enabled
    || pagination_disabled_for_command
    || batch_flag
    || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
    || top_level_interpreter () == NULL
    || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
---


However, the actual observed behaviour in gdb is:
---
Reading symbols from a.out...
(gdb) set annotate 2

\032\032pre-prompt
(gdb)
\032\032prompt
start

prompt\032\032post-
Temporary breakpoint 1 at 0x13716: file test.c, line 3.
---

With this patch applied, instead "\032\032post-prompt" is printed.

I think this can be applied as obvious, but wanted to have someone else have a quick check, just in case it would be preferred to change fputs_maybe_filtered instead to flush the buffer on scope exit for unfiltered messages.

--
Iain

---

Comments

Eli Zaretskii via Gdb-patches Nov. 26, 2019, 8:13 p.m. | #1
Sounds like this fixes
https://sourceware.org/bugzilla/show_bug.cgi?id=25190, could you
mention that in the patch description?

(Those three lines could probably be a single printf...)

Christian

On Tue, Nov 26, 2019 at 6:49 AM Iain Buclaw <ibuclaw@gdcproject.org> wrote:
>

> Hi,

>

> This patch fixes a regression, possibly introduced by 2a3c1174c3c0db1140180fb3fc56ac324d1c0a7c, in this part of the change:

>

> ---

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

> ---

>

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

>

> Not sure about how to go about testing this, looking at the testsuite, such as gdb.base/annota1.exp, everything appears to be in order.  Perhaps this is because the testsuite triggers one of these conditions in fputs_maybe_filtered() though.

>

> ---

> if (stream != gdb_stdout

>     || !pagination_enabled

>     || pagination_disabled_for_command

>     || batch_flag

>     || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)

>     || top_level_interpreter () == NULL

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

> ---

>

>

> However, the actual observed behaviour in gdb is:

> ---

> Reading symbols from a.out...

> (gdb) set annotate 2

>

> \032\032pre-prompt

> (gdb)

> \032\032prompt

> start

>

> prompt\032\032post-

> Temporary breakpoint 1 at 0x13716: file test.c, line 3.

> ---

>

> With this patch applied, instead "\032\032post-prompt" is printed.

>

> I think this can be applied as obvious, but wanted to have someone else have a quick check, just in case it would be preferred to change fputs_maybe_filtered instead to flush the buffer on scope exit for unfiltered messages.

>

> --

> Iain

>

> ---

>

>

>
Pedro Alves Nov. 26, 2019, 8:24 p.m. | #2
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
Iain Buclaw Nov. 26, 2019, 11 p.m. | #3
‐‐‐‐‐‐‐ 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.

--
Iain

---
gdb/ChangeLog:

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

	* gdb/ui-file.c (fputs_unfiltered): Move to utils.c.
	* gdb/utils.c (flush_wrap_buffer): Call ui_file::puts instead of
	fputs_unfiltered.
	(fputs_maybe_filtered): Likewise.  Remove unused buffer_clearer.
	(fputs_unfiltered): Moved from utils.c; call fputs_maybe_filtered.

---
 gdb/ui-file.c |  6 ------
 gdb/utils.c   | 22 +++++++++-------------
 2 files changed, 9 insertions(+), 19 deletions(-)

---
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 71b74bba19..31664d5d65 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -149,12 +149,6 @@ ui_file_read (struct ui_file *file, char *buf, long length_buf)
   return file->read (buf, length_buf);
 }
 
-void
-fputs_unfiltered (const char *buf, struct ui_file *file)
-{
-  file->puts (buf);
-}
-
 
 
 string_file::~string_file ()
diff --git a/gdb/utils.c b/gdb/utils.c
index f7fae35729..e8cc21c8c4 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -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);
+      stream->puts (wrap_buffer.c_str ());
       wrap_buffer.clear ();
     }
 }
@@ -1688,18 +1688,10 @@ 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);
+      stream->puts (linebuffer);
       return;
     }
 
-  auto buffer_clearer
-    = make_scope_exit ([&] ()
-		       {
-			 wrap_buffer.clear ();
-			 wrap_column = 0;
-			 wrap_indent = "";
-		       });
-
   /* Go through and output each character.  Show line extension
      when this is necessary; prompt user for new page when this is
      necessary.  */
@@ -1788,7 +1780,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);
+		  stream->puts (wrap_indent);
 		  if (stream->can_emit_style_escape ())
 		    emit_style_escape (save_style, stream);
 		  /* FIXME, this strlen is what prevents wrap_indent from
@@ -1816,8 +1808,6 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  lineptr++;
 	}
     }
-
-  buffer_clearer.release ();
 }
 
 void
@@ -1826,6 +1816,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
Iain Buclaw Jan. 18, 2020, 2:46 p.m. | #4
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, January 17, 2020 6:56 PM, Joel Brobecker <brobecker@adacore.com> wrote:

> Hi Iain, hi Pedro,

>

> Is this message the latest one on this subject? Christian helped us

> identify this issue as a regression, and I am wondering whether we want

> to try to fix it before the 9.1 release or whether we accept it...

>

> Thanks!

>


Hi,

As I've already said, there was a follow-up that instead attempted to fix the filtered vs unfiltered inconsistencies that caused the bug.

So rather than this one, I'd suggest looking at the other two here.

https://sourceware.org/ml/gdb-patches/2019-11/msg01120.html
https://sourceware.org/ml/gdb-patches/2019-11/msg01121.html

Regards
Iain.

Patch

gdb/ChangeLog:

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

	* gdb/event-top.c (handle_line_of_input): Use puts_unfiltered instead
	of printf_unfiltered.

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 0396dbcc52..df6a4095fb 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -663,9 +663,9 @@  handle_line_of_input (struct buffer *cmd_line_buffer,
 
   if (from_tty && annotation_level > 1)
     {
-      printf_unfiltered (("\n\032\032post-"));
+      puts_unfiltered ("\n\032\032post-");
       puts_unfiltered (annotation_suffix);
-      printf_unfiltered (("\n"));
+      puts_unfiltered ("\n");
     }
 
 #define SERVER_COMMAND_PREFIX "server "