Message ID | 5b7dfd6c-483e-618b-0d25-7f5b12ad32de@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 26569 invoked by alias); 1 Feb 2017 22:49:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 26548 invoked by uid 89); 1 Feb 2017 22:49:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=BAYES_50, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=586, deferring, flushes, aspx X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 01 Feb 2017 22:49:05 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BEA92369C4; Wed, 1 Feb 2017 22:49:05 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v11Mn4F9018856; Wed, 1 Feb 2017 17:49:04 -0500 Subject: Re: [PATCH v4 2/2] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy To: Luis Machado <lgustavo@codesourcery.com>, gdb-patches@sourceware.org References: <1485909045-30285-1-git-send-email-palves@redhat.com> <1485909045-30285-3-git-send-email-palves@redhat.com> <a1194bed-b33a-d874-2fa2-b0a1ff793eea@codesourcery.com> From: Pedro Alves <palves@redhat.com> Message-ID: <5b7dfd6c-483e-618b-0d25-7f5b12ad32de@redhat.com> Date: Wed, 1 Feb 2017 22:49:02 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <a1194bed-b33a-d874-2fa2-b0a1ff793eea@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit |
Commit Message
Pedro Alves
Feb. 1, 2017, 10:49 p.m. UTC
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!
Comments
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
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; };