[v4,2/2] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy

  On 02/01/2017 05:36 PM, Luis Machado wrote:

>> +typedef std::unique_ptr<stdio_file> stdio_file_up;
>> +
>> +/* Like stdio_file, but specifically for stderr.
>> +
>> +   This exists because there is no real line-buffering on Windows, see
>> +   <http://msdn.microsoft.com/en-us/library/86cebhfs%28v=vs.71%29.aspx>
>> +   so the stdout is either fully-buffered or non-buffered.  We can't
>> +   make stdout non-buffered, because of two concerns:
>> +
>> +    1. Non-buffering hurts performance.
>> +    2. Non-buffering may change GDB's behavior when it is interacting
>> +       with a front-end, such as Emacs.
>> +
>> +   We leave stdout as fully buffered, but flush it first when
>> +   something is written to stderr.
>> +
>> +   Note the the 'write_async_safe' method is not overwritten, because
> 
> 
> Extra "the".
> 
> Did you mean overridden instead of overwritten?

Right, fixed.

The existing comment this is being moved from says "overwritten",
and I missed updating it.  ("overwritten" made some sense
in current master, I guess, since to "override" a method
currently you "overwrite" a function pointer.)

>> +class stderr_file : public stdio_file
>> +{
>> +public:
>> +  explicit stderr_file (FILE *stream);
>>
>> -/* Create/open a memory based file.  Can be used as a scratch buffer
>> -   for collecting output.  */
>> -extern struct ui_file *mem_fileopen (void);
>> +  /* Flushes gdb_stdout before writing to the underlying stream.  */
>> +  void write (const char *buf, long length_buf) override;
>>
> 
> I noticed the above declaration and ...
> 
>> +  /* Flushes gdb_stdout before writing to the underlying stream.  */
>> +  void puts (const char *linebuffer) override;
> 
> ... the above declaration both have the same documentation. Do they
> accomplish the same?

They both flush gdb_stdout before deferring to the stdio_file
(the superclass) for the actual writing/outputting.  "puts" exists as
a separate method because for some ui_file types it's more efficient to
call some available puts-like function (e.g. tui_puts), than
having the puts method always always call the write method, which
requires a strlen call.

Would this help?

 gdb/ui-file.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

> Otherwise looks good to me.
> 
> Great cleanup. Thanks!

Thanks!
  

On 02/01/2017 04:49 PM, Pedro Alves wrote:
> On 02/01/2017 05:36 PM, Luis Machado wrote:
>
>>> +typedef std::unique_ptr<stdio_file> stdio_file_up;
>>> +
>>> +/* Like stdio_file, but specifically for stderr.
>>> +
>>> +   This exists because there is no real line-buffering on Windows, see
>>> +   <http://msdn.microsoft.com/en-us/library/86cebhfs%28v=vs.71%29.aspx>
>>> +   so the stdout is either fully-buffered or non-buffered.  We can't
>>> +   make stdout non-buffered, because of two concerns:
>>> +
>>> +    1. Non-buffering hurts performance.
>>> +    2. Non-buffering may change GDB's behavior when it is interacting
>>> +       with a front-end, such as Emacs.
>>> +
>>> +   We leave stdout as fully buffered, but flush it first when
>>> +   something is written to stderr.
>>> +
>>> +   Note the the 'write_async_safe' method is not overwritten, because
>>
>>
>> Extra "the".
>>
>> Did you mean overridden instead of overwritten?
>
> Right, fixed.
>
> The existing comment this is being moved from says "overwritten",
> and I missed updating it.  ("overwritten" made some sense
> in current master, I guess, since to "override" a method
> currently you "overwrite" a function pointer.)
>
>>> +class stderr_file : public stdio_file
>>> +{
>>> +public:
>>> +  explicit stderr_file (FILE *stream);
>>>
>>> -/* Create/open a memory based file.  Can be used as a scratch buffer
>>> -   for collecting output.  */
>>> -extern struct ui_file *mem_fileopen (void);
>>> +  /* Flushes gdb_stdout before writing to the underlying stream.  */
>>> +  void write (const char *buf, long length_buf) override;
>>>
>>
>> I noticed the above declaration and ...
>>
>>> +  /* Flushes gdb_stdout before writing to the underlying stream.  */
>>> +  void puts (const char *linebuffer) override;
>>
>> ... the above declaration both have the same documentation. Do they
>> accomplish the same?
>
> They both flush gdb_stdout before deferring to the stdio_file
> (the superclass) for the actual writing/outputting.  "puts" exists as
> a separate method because for some ui_file types it's more efficient to
> call some available puts-like function (e.g. tui_puts), than
> having the puts method always always call the write method, which
> requires a strlen call.
>
> Would this help?
>
>  gdb/ui-file.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index fc70417..d64cdce 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -58,6 +58,8 @@ public:
>    virtual void write_async_safe (const char *buf, long length_buf)
>    { gdb_assert_not_reached ("write_async_safe"); }
>
> +  /* Some ui_files override this to provide a efficient implementation
> +     that avoids the strlen.  */
>    virtual void puts (const char *str)
>    { this->write (str, strlen (str)); }
>
> @@ -227,10 +229,9 @@ class stderr_file : public stdio_file
>  public:
>    explicit stderr_file (FILE *stream);
>
> -  /* Flushes gdb_stdout before writing to the underlying stream.  */
> +  /* Override the output routines to flush gdb_stdout before deferring
> +     to stdio_file for the actual outputting.  */
>    void write (const char *buf, long length_buf) override;
> -
> -  /* Flushes gdb_stdout before writing to the underlying stream.  */
>    void puts (const char *linebuffer) override;
>  };
>

That looks better. Thanks.
  

On 02/01/2017 11:23 PM, Luis Machado wrote:

> 
> That looks better. Thanks.

Great, thanks.

If there are no further comments, I'll probably push this tomorrow.

Thanks,
Pedro Alves
  

On 02/01/2017 10:01 PM, Pedro Alves wrote:
> On 02/01/2017 11:23 PM, Luis Machado wrote:
> 
>>
>> That looks better. Thanks.
> 
> Great, thanks.
> 
> If there are no further comments, I'll probably push this tomorrow.
> 
> Thanks,
> Pedro Alves
> 
> 

Hi Pedro,

from bisect, it seems commit d7e747 introduced a problem when using
layout regs, that leads gdb to crash when issuing:

./gdb ./a.out -ex 'layout regs' -ex start

From the backtrace, it's caused by this 'delete' on tui_restore_gdbout(): 

(gdb) bt
#0  0x00007ffff6b962b2 in free () from /lib64/libc.so.6
#1  0x000000000059fa47 in tui_restore_gdbout (ui=0x22997b0) at ../../gdb/tui/tui-regs.c:714
#2  0x0000000000619996 in do_my_cleanups (pmy_chain=pmy_chain@entry=0x1e08320 <cleanup_chain>, old_chain=old_chain@entry=0x235b4b0) at ../../gdb/common/cleanups.c:154
#3  0x0000000000619b1d in do_cleanups (old_chain=old_chain@entry=0x235b4b0) at ../../gdb/common/cleanups.c:176
#4  0x000000000059fb0d in tui_register_format (frame=frame@entry=0x22564e0, regnum=regnum@entry=0) at ../../gdb/tui/tui-regs.c:747
#5  0x000000000059ffeb in tui_get_register (data=0x2434d18, changedp=0x0, regnum=0, frame=0x22564e0) at ../../gdb/tui/tui-regs.c:768
#6  tui_show_register_group (refresh_values_only=<optimized out>, frame=0x22564e0, group=0x1e09250 <general_group>) at ../../gdb/tui/tui-regs.c:287
#7  tui_show_registers (group=0x1e09250 <general_group>) at ../../gdb/tui/tui-regs.c:156
#8  0x00000000005a07cf in tui_check_register_values (frame=frame@entry=0x22564e0) at ../../gdb/tui/tui-regs.c:496
#9  0x00000000005a3e65 in tui_check_data_values (frame=frame@entry=0x22564e0) at ../../gdb/tui/tui-windata.c:232
#10 0x000000000059cf65 in tui_refresh_frame_and_register_information (registers_too_p=1) at ../../gdb/tui/tui-hooks.c:156
#11 0x00000000006d5c05 in generic_observer_notify (args=0x7fffffffdbe0, subject=<optimized out>) at ../../gdb/observer.c:167
#12 observer_notify_normal_stop (bs=<optimized out>, print_frame=print_frame@entry=1) at ./observer.inc:61
#13 0x00000000006a6409 in normal_stop () at ../../gdb/infrun.c:8364
#14 0x00000000006af8f5 in fetch_inferior_event (client_data=<optimized out>) at ../../gdb/infrun.c:3990
#15 0x000000000066f0fd in gdb_wait_for_event (block=block@entry=0) at ../../gdb/event-loop.c:859
#16 0x000000000066f237 in gdb_do_one_event () at ../../gdb/event-loop.c:322
#17 0x000000000066f386 in gdb_do_one_event () at ../../gdb/event-loop.c:353
#18 0x00000000007411bc in wait_sync_command_done () at ../../gdb/top.c:570
#19 0x0000000000741426 in maybe_wait_sync_command_done (was_sync=0) at ../../gdb/top.c:587
#20 execute_command (p=<optimized out>, p@entry=0x7fffffffe43a "start", from_tty=from_tty@entry=1) at ../../gdb/top.c:676
#21 0x00000000006c2048 in catch_command_errors (command=0x741200 <execute_command(char*, int)>, arg=0x7fffffffe43a "start", from_tty=1) at ../../gdb/main.c:376
#22 0x00000000006c2b60 in captured_main_1 (context=0x7fffffffde70) at ../../gdb/main.c:1119
#23 captured_main (data=0x7fffffffde70) at ../../gdb/main.c:1140
#24 gdb_main (args=args@entry=0x7fffffffdf90) at ../../gdb/main.c:1158
#25 0x0000000000408cf5 in main (argc=<optimized out>, argv=<optimized out>) at ../../gdb/gdb.c:32
(gdb) f 1 
#1  0x000000000059fa47 in tui_restore_gdbout (ui=0x22997b0) at ../../gdb/tui/tui-regs.c:714
714	  delete gdb_stdout;

--
Edjunior
  

Hi Edjunior,

On 02/27/2017 07:43 PM, Edjunior Barbosa Machado wrote:

> Hi Pedro,
> 
> from bisect, it seems commit d7e747 introduced a problem when using
> layout regs, that leads gdb to crash when issuing:
> 
> ./gdb ./a.out -ex 'layout regs' -ex start
> 
> From the backtrace, it's caused by this 'delete' on tui_restore_gdbout(): 

Thanks for the bisect.  This is fixed in master now.

Thanks,
Pedro Alves