* [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
@ 2019-11-26 12:49 Iain Buclaw
2019-11-26 20:13 ` Christian Biesinger via gdb-patches
2019-11-26 20:25 ` Pedro Alves
0 siblings, 2 replies; 6+ messages in thread
From: Iain Buclaw @ 2019-11-26 12:49 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]
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
---
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: postprompt.patch --]
[-- Type: text/x-patch; name="postprompt.patch", Size: 665 bytes --]
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 "
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
2019-11-26 12:49 [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered Iain Buclaw
@ 2019-11-26 20:13 ` Christian Biesinger via gdb-patches
2019-11-26 20:25 ` Pedro Alves
1 sibling, 0 replies; 6+ messages in thread
From: Christian Biesinger via gdb-patches @ 2019-11-26 20:13 UTC (permalink / raw)
To: Iain Buclaw; +Cc: gdb-patches
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
>
> ---
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
2019-11-26 12:49 [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered Iain Buclaw
2019-11-26 20:13 ` Christian Biesinger via gdb-patches
@ 2019-11-26 20:25 ` Pedro Alves
2019-11-26 23:00 ` Iain Buclaw
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2019-11-26 20:25 UTC (permalink / raw)
To: Iain Buclaw, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
2019-11-26 20:25 ` Pedro Alves
@ 2019-11-26 23:00 ` Iain Buclaw
2020-01-17 17:57 ` Joel Brobecker
0 siblings, 1 reply; 6+ messages in thread
From: Iain Buclaw @ 2019-11-26 23:00 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]
‐‐‐‐‐‐‐ 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(-)
---
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: postprompt-2.patch --]
[-- Type: text/x-patch; name="postprompt-2.patch", Size: 2245 bytes --]
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);
-}
-
\f
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
2019-11-26 23:00 ` Iain Buclaw
@ 2020-01-17 17:57 ` Joel Brobecker
2020-01-18 18:53 ` Iain Buclaw
0 siblings, 1 reply; 6+ messages in thread
From: Joel Brobecker @ 2020-01-17 17:57 UTC (permalink / raw)
To: Iain Buclaw; +Cc: Pedro Alves, gdb-patches
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!
On Tue, Nov 26, 2019 at 11:00:21PM +0000, Iain Buclaw 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.
>
> --
> 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);
> -}
> -
> \f
>
> 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
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered
2020-01-17 17:57 ` Joel Brobecker
@ 2020-01-18 18:53 ` Iain Buclaw
0 siblings, 0 replies; 6+ messages in thread
From: Iain Buclaw @ 2020-01-18 18:53 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Pedro Alves, gdb-patches
‐‐‐‐‐‐‐ 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-18 14:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-26 12:49 [PATCH] gdb: Use puts_unfiltered instead of printf_unfiltered Iain Buclaw
2019-11-26 20:13 ` Christian Biesinger via gdb-patches
2019-11-26 20:25 ` Pedro Alves
2019-11-26 23:00 ` Iain Buclaw
2020-01-17 17:57 ` Joel Brobecker
2020-01-18 18:53 ` Iain Buclaw
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox