diff mbox

Make '{putchar,fputc}_unfiltered' use 'fputs_unfiltered'

Message ID 20200220041214.155369-1-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Feb. 20, 2020, 4:12 a.m. UTC 
  There is currently a regression when using
'{putchar,fputc}_unfiltered' with 'puts_unfiltered' which was
introduced by one of the commits that reworked the unfiltered print
code.

The regression makes it impossible to use '{putchar,fputc}_unfiltered'
with 'puts_unfiltered', because the former writes directly to the
ui_file stream using 'stream->write', while the latter uses a buffered
mechanism (see 'wrap_buffer') and delays the printing.

If you do a quick & dirty hack on e.g. top.c:show_gdb_datadir:

  @@ -2088,6 +2088,13 @@ static void
   show_gdb_datadir (struct ui_file *file, int from_tty,
		    struct cmd_list_element *c, const char *value)
   {
  +  putchar_unfiltered ('\n');
  +  puts_unfiltered ("TEST");
  +  putchar_unfiltered ('>');
  +  puts_unfiltered ("PUTS");
  +  putchar_unfiltered ('\n');

rebuild GDB and invoke the "show data-directory" command, you will
see:

  (gdb) show data-directory

  >
  TESTPUTSGDB's data directory is "/usr/local/share/gdb".

Note how the '>' was printed before the output, and "TEST" and "PUTS"
were printed together.

My first attempt to fix this was to always call 'flush_wrap_buffer' at
the end of 'fputs_maybe_filtered', since it seemed to me that the
function should always print what was requested.  But I wasn't sure
this was the right thing to do, so I talked to Tom on IRC and he gave
me another, simpler idea: make '{putchar,fputc}_unfiltered' call into
the already existing 'fputs_unfiltered' function.

This patch implements the idea.  I regtested it on the Buildbot, and
no regressions were detected.

gdb/ChangeLog:
2020-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Tom Tromey  <tom@tromey.com>
	* utils.c (fputs_maybe_filtered): Call 'stream->puts' instead
	of 'fputc_unfiltered'.
	(putchar_unfiltered): Call 'fputc_unfiltered'.
	(fputc_unfiltered): Call 'fputs_unfiltered'.
---
 gdb/utils.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

Comments

Tom Tromey Feb. 20, 2020, 7:59 p.m. UTC | #1 
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> gdb/ChangeLog:
Sergio> 2020-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
Sergio> 	    Tom Tromey  <tom@tromey.com>
Sergio> 	* utils.c (fputs_maybe_filtered): Call 'stream->puts' instead
Sergio> 	of 'fputc_unfiltered'.
Sergio> 	(putchar_unfiltered): Call 'fputc_unfiltered'.
Sergio> 	(fputc_unfiltered): Call 'fputs_unfiltered'.

Looks good to me.  Thanks for doing this.

Tom
Sergio Durigan Junior Feb. 20, 2020, 9:04 p.m. UTC | #2 
On Thursday, February 20 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> gdb/ChangeLog:
> Sergio> 2020-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
> Sergio> 	    Tom Tromey  <tom@tromey.com>
> Sergio> 	* utils.c (fputs_maybe_filtered): Call 'stream->puts' instead
> Sergio> 	of 'fputc_unfiltered'.
> Sergio> 	(putchar_unfiltered): Call 'fputc_unfiltered'.
> Sergio> 	(fputc_unfiltered): Call 'fputs_unfiltered'.
>
> Looks good to me.  Thanks for doing this.

Thanks, pushed.

3f702acd7d562d3a33c59d6398ae74058438d2c7
Joel Brobecker Feb. 21, 2020, 3:57 a.m. UTC | #3 
Hi Sergio and Tom,

On Thu, Feb 20, 2020 at 04:04:05PM -0500, Sergio Durigan Junior wrote:
> On Thursday, February 20 2020, Tom Tromey wrote:
> 
> >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
> >
> > Sergio> gdb/ChangeLog:
> > Sergio> 2020-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
> > Sergio> 	    Tom Tromey  <tom@tromey.com>
> > Sergio> 	* utils.c (fputs_maybe_filtered): Call 'stream->puts' instead
> > Sergio> 	of 'fputc_unfiltered'.
> > Sergio> 	(putchar_unfiltered): Call 'fputc_unfiltered'.
> > Sergio> 	(fputc_unfiltered): Call 'fputs_unfiltered'.
> >
> > Looks good to me.  Thanks for doing this.
> 
> Thanks, pushed.
> 
> 3f702acd7d562d3a33c59d6398ae74058438d2c7

I think the commit that Sergio incriminated is also in gdb-9-branch.
Do you confirm?

If yes, what about creating a GDB PR, and backporting this change
to the gdb-9-branch (all changes after the .1 must have a PR)?
The change looks relatively safe to me, but maybe it's not as simple
as we might think, especially since this section of the code _is_ quite
central...

Thank you!
Sergio Durigan Junior Feb. 21, 2020, 5:53 a.m. UTC | #4 
On Thursday, February 20 2020, Joel Brobecker wrote:

> Hi Sergio and Tom,

Hey Joel,

> On Thu, Feb 20, 2020 at 04:04:05PM -0500, Sergio Durigan Junior wrote:
>> On Thursday, February 20 2020, Tom Tromey wrote:
>> 
>> >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>> >
>> > Sergio> gdb/ChangeLog:
>> > Sergio> 2020-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
>> > Sergio> 	    Tom Tromey  <tom@tromey.com>
>> > Sergio> 	* utils.c (fputs_maybe_filtered): Call 'stream->puts' instead
>> > Sergio> 	of 'fputc_unfiltered'.
>> > Sergio> 	(putchar_unfiltered): Call 'fputc_unfiltered'.
>> > Sergio> 	(fputc_unfiltered): Call 'fputs_unfiltered'.
>> >
>> > Looks good to me.  Thanks for doing this.
>> 
>> Thanks, pushed.
>> 
>> 3f702acd7d562d3a33c59d6398ae74058438d2c7
>
> I think the commit that Sergio incriminated is also in gdb-9-branch.
> Do you confirm?

Yes, the bug also happens in gdb-9-branch.

> If yes, what about creating a GDB PR, and backporting this change
> to the gdb-9-branch (all changes after the .1 must have a PR)?

I can do that.  Just not right now; I need to be in my bed sleeping 30
minutes ago ;-).

> The change looks relatively safe to me, but maybe it's not as simple
> as we might think, especially since this section of the code _is_ quite
> central...

Yeah, this code is quite messy, and even though the change is somewhat
"self-contained".  What if we wait a few days/weeks before I open the PR
and do the backport?  Due to this code being quite central as you said,
I think it should not take long for complaints to start coming our way,
assuming there is a bug to be found.

Thanks,
Joel Brobecker Feb. 21, 2020, 10:13 a.m. UTC | #5 
> > I think the commit that Sergio incriminated is also in gdb-9-branch.
> > Do you confirm?
> 
> Yes, the bug also happens in gdb-9-branch.
> 
> > If yes, what about creating a GDB PR, and backporting this change
> > to the gdb-9-branch (all changes after the .1 must have a PR)?
> 
> I can do that.  Just not right now; I need to be in my bed sleeping 30
> minutes ago ;-).

;-)

> > The change looks relatively safe to me, but maybe it's not as simple
> > as we might think, especially since this section of the code _is_ quite
> > central...
> 
> Yeah, this code is quite messy, and even though the change is somewhat
> "self-contained".  What if we wait a few days/weeks before I open the PR
> and do the backport?  Due to this code being quite central as you said,
> I think it should not take long for complaints to start coming our way,
> assuming there is a bug to be found.

That definitely works. Unless we discover this is a crippling issue
that deserves an emergency release, we have a solid couple of months
to decide, really. More if we need...

Thanks Sergio!
Sergio Durigan Junior Feb. 21, 2020, 6:31 p.m. UTC | #6 
On Friday, February 21 2020, Joel Brobecker wrote:

>> > The change looks relatively safe to me, but maybe it's not as simple
>> > as we might think, especially since this section of the code _is_ quite
>> > central...
>> 
>> Yeah, this code is quite messy, and even though the change is somewhat
>> "self-contained".  What if we wait a few days/weeks before I open the PR
>> and do the backport?  Due to this code being quite central as you said,
>> I think it should not take long for complaints to start coming our way,
>> assuming there is a bug to be found.
>
> That definitely works. Unless we discover this is a crippling issue
> that deserves an emergency release, we have a solid couple of months
> to decide, really. More if we need...

Good deal :-).

> Thanks Sergio!

Thank *you* for bringing this to my attention!
diff mbox

Patch

diff --git a/gdb/utils.c b/gdb/utils.c
index 0200a8621f..0b470120a2 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1776,7 +1776,12 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 		     newline -- if chars_per_line is right, we
 		     probably just overflowed anyway; if it's wrong,
 		     let us keep going.  */
-		  fputc_unfiltered ('\n', stream);
+		  /* XXX: The ideal thing would be to call
+		     'stream->putc' here, but we can't because it
+		     currently calls 'fputc_unfiltered', which ends up
+		     calling us, which generates an infinite
+		     recursion.  */
+		  stream->puts ("\n");
 		}
 	      else
 		{
@@ -1821,7 +1826,12 @@  fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  wrap_here ((char *) 0);	/* Spit out chars, cancel
 					   further wraps.  */
 	  lines_printed++;
-	  fputc_unfiltered ('\n', stream);
+	  /* XXX: The ideal thing would be to call
+	     'stream->putc' here, but we can't because it
+	     currently calls 'fputc_unfiltered', which ends up
+	     calling us, which generates an infinite
+	     recursion.  */
+	  stream->puts ("\n");
 	  lineptr++;
 	}
     }
@@ -1916,10 +1926,7 @@  fputs_highlighted (const char *str, const compiled_regex &highlight,
 int
 putchar_unfiltered (int c)
 {
-  char buf = c;
-
-  gdb_stdout->write (&buf, 1);
-  return c;
+  return fputc_unfiltered (c, gdb_stdout);
 }
 
 /* Write character C to gdb_stdout using GDB's paging mechanism and return C.
@@ -1934,9 +1941,11 @@  putchar_filtered (int c)
 int
 fputc_unfiltered (int c, struct ui_file *stream)
 {
-  char buf = c;
+  char buf[2];
 
-  stream->write (&buf, 1);
+  buf[0] = c;
+  buf[1] = 0;
+  fputs_unfiltered (buf, stream);
   return c;
 }