Message ID | M5oE0jdfEHsib_abZiBsAsHhGE_PVRPMYX-hTKYd_gNkaUaBKnKwqYbPAFCduTZkk2HEunw_U178cb8TLd_eYmckpiWQ0gI-MMqlvv05eZI=@gdcproject.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 111911 invoked by alias); 29 Nov 2019 00:12:11 -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 111893 invoked by uid 89); 29 Nov 2019 00:12:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-40134.protonmail.ch Received: from mail-40134.protonmail.ch (HELO mail-40134.protonmail.ch) (185.70.40.134) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Nov 2019 00:12:08 +0000 Date: Fri, 29 Nov 2019 00:12:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gdcproject.org; s=protonmail; t=1574986325; bh=nyM+aP7sey9beZYOzt52thjEUi+ElyecGdUypy+yXN0=; h=Date:To:From:Cc:Reply-To:Subject:Feedback-ID:From; b=gqaMlDXSl2t0wbtGGMiBQO093PcafF4Ow1u6mNYuwMK6e0vjG1xiC7gOaXmuBsit5 0NEXzNdw4YLWG6w5nCjMz9TYvdC6XVJ1CbSMbC51XIZiY9uwyoUlCT6b7f24ki9teR xG9qS8ZAgIOqxPWxntEbiUf0M/07Ycq/oMpIICS4= To: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org> From: Iain Buclaw <ibuclaw@gdcproject.org> Cc: Pedro Alves <palves@redhat.com> Reply-To: Iain Buclaw <ibuclaw@gdcproject.org> Subject: [PATCH v2 3/3] gdb: Remove unused buffer_clearer variable. Message-ID: <M5oE0jdfEHsib_abZiBsAsHhGE_PVRPMYX-hTKYd_gNkaUaBKnKwqYbPAFCduTZkk2HEunw_U178cb8TLd_eYmckpiWQ0gI-MMqlvv05eZI=@gdcproject.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="b1_0c7d671a64ea364de09da75343228403" X-IsSubscribed: yes |
Commit Message
Iain Buclaw
Nov. 29, 2019, 12:12 a.m. UTC
When reviewing how fputs_maybe_unfiltered works, I noticed that the compiler was removing this variable and assigned lambda expression as dead code. There are no early returns in this function, so release() is always called. -- Iain --- gdb/ChangeLog: 2019-11-29 Iain Buclaw <ibuclaw@gdcproject.org> * gdb/utils.c (fputs_maybe_filtered): Remove buffer_clearer variable. ---
Comments
On 2019-11-28 7:12 p.m., Iain Buclaw wrote: > When reviewing how fputs_maybe_unfiltered works, I noticed that the compiler was removing this variable and assigned lambda expression as dead code. > > There are no early returns in this function, so release() is always called. > > -- > Iain The goal of this variable is to cleanup on scope exit, in the event an exception is raised, for instance by prompt_for_continue if the user presses q to stop the command. So it's normal that there is nothing using the variable directly. It would be alarming if the compiled was really optimizing this out. Can you give more details on how you reached that conclusion? Simon
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Saturday, 30 November 2019 04:28, Simon Marchi <simark@simark.ca> wrote: > On 2019-11-28 7:12 p.m., Iain Buclaw wrote: > > > When reviewing how fputs_maybe_unfiltered works, I noticed that the compiler was removing this variable and assigned lambda expression as dead code. > > There are no early returns in this function, so release() is always called. > > -- > > Iain > > The goal of this variable is to cleanup on scope exit, in the > event an exception is raised, for instance by prompt_for_continue > if the user presses q to stop the command. So it's normal that > there is nothing using the variable directly. > > It would be alarming if the compiled was really optimizing this > out. Can you give more details on how you reached that conclusion? > When initially stepping through this code to work out what this was doing, I noticed this variable was optimized out completely. When compiling with -fdump-tree-optimized, I couldn't see at a glance any buffer_clearer nor lambda functions relating to fputs_maybe_filtered. Maybe the scope_exit body got inlined somewhere and I missed it completely. If this is expected to be lowered to a 'try{...} finally{...}' statement, I'm fine ignoring this. It's not related to what I was trying to fix, this is just something that initially threw me off. -- Iain
On 2019-11-30 4:57 a.m., Iain Buclaw wrote: > When initially stepping through this code to work out what this was doing, I noticed this variable was optimized out completely. When compiling with -fdump-tree-optimized, I couldn't see at a glance any buffer_clearer nor lambda functions relating to fputs_maybe_filtered. > > Maybe the scope_exit body got inlined somewhere and I missed it completely. If this is expected to be lowered to a 'try{...} finally{...}' statement, I'm fine ignoring this. It's not related to what I was trying to fix, this is just something that initially threw me off. I'm not saying you are wrong, there might indeed be a problem. I was just saying what this is expected to accomplish. It would be worth pursuing this just to make sure it does what we intend it to do. Simon
On Sun, Dec 1, 2019 at 9:06 AM Simon Marchi <simark@simark.ca> wrote: > > On 2019-11-30 4:57 a.m., Iain Buclaw wrote: > > When initially stepping through this code to work out what this was doing, I noticed this variable was optimized out completely. When compiling with -fdump-tree-optimized, I couldn't see at a glance any buffer_clearer nor lambda functions relating to fputs_maybe_filtered. > > > > Maybe the scope_exit body got inlined somewhere and I missed it completely. If this is expected to be lowered to a 'try{...} finally{...}' statement, I'm fine ignoring this. It's not related to what I was trying to fix, this is just something that initially threw me off. > > I'm not saying you are wrong, there might indeed be a problem. I was just saying what > this is expected to accomplish. It would be worth pursuing this just to make sure it > does what we intend it to do. I played with this a bit... a simple version does show it being called: @@ -1695,10 +1695,12 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, auto buffer_clearer = make_scope_exit ([&] () { + fprintf(stderr, "called!\n"); wrap_buffer.clear (); wrap_column = 0; wrap_indent = ""; }); + throw gdb_exception(); Even a more indirect version (closer to what this is meant for) works: @@ -1446,6 +1446,7 @@ reset_terminal_style (struct ui_file *stream) static void prompt_for_continue (void) { + throw gdb_exception(); char cont_prompt[120]; /* Used to add duration we waited for user to respond to prompt_for_continue_wait_time. */ @@ -1695,6 +1696,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, auto buffer_clearer = make_scope_exit ([&] () { + fprintf(stderr, "called!\n"); wrap_buffer.clear (); wrap_column = 0; wrap_indent = ""; I haven't verified this without the printf but I do see the code in the assembly (gcc 8.3.0): Address range 0x6c39bd to 0x6c39f2: /usr/include/c++/8/bits/basic_string.h: 1023 clear() _GLIBCXX_NOEXCEPT 0x00000000006c39bd <-7732339>: mov 0x1d25ddc(%rip),%rdx # 0x23e97a0 <_ZL11wrap_buffer> 0x00000000006c39c4 <-7732332>: lea 0xc983db(%rip),%rcx # 0x135bda6 0x00000000006c39cb <-7732325>: mov %rax,%rdi 0x00000000006c39ce <-7732322>: movq $0x0,0x1d25dcf(%rip) # 0x23e97a8 <_ZL11wrap_buffer+8> /usr/include/c++/8/bits/char_traits.h: 287 { __c1 = __c2; } 0x00000000006c39d9 <-7732311>: movl $0x0,0x1d25d9d(%rip) # 0x23e9780 <_ZL11wrap_column> 0x00000000006c39e3 <-7732301>: movb $0x0,(%rdx) ../../gdb/utils.c: 1700 wrap_indent = ""; 0x00000000006c39e6 <-7732298>: mov %rcx,0x1d25d9b(%rip) # 0x23e9788 <_ZL11wrap_indent> 0x00000000006c39ed <-7732291>: callq 0x695080 <_Unwind_Resume@plt> End of assembler dump. Christian
diff --git a/gdb/utils.c b/gdb/utils.c index 0e09f646bf..721babee7f 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1699,14 +1699,6 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, 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. */ @@ -1823,8 +1815,6 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream, lineptr++; } } - - buffer_clearer.release (); } void