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

[Iain Buclaw <ibuclaw@gdcproject.org>]

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.