Patchwork [v2,3/3] gdb: Remove unused buffer_clearer variable.

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.

---

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