Re: [PATCH] Fix "attach" command vs user input race [Re: Regression for attach from stdin [Re: [pushed] Re: [PATCH v6 0/2] enable target-async by default]]

[Doug Evans <dje@google.com>]

Pedro Alves writes:
 > gdb/
 > 2014-07-04  Pedro Alves  <palves@redhat.com>
 > 
 > 	* infcmd.c (attach_command_post_wait): Don't call
 > 	target_terminal_inferior here.
 > 	(attach_command): Call it here instead.
 > [...]
 > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
 > index c4bb401..76ab12b 100644
 > --- a/gdb/infcmd.c
 > +++ b/gdb/infcmd.c
 > @@ -2381,9 +2381,6 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
 > 
 >    post_create_inferior (&current_target, from_tty);
 > 
 > -  /* Install inferior's terminal modes.  */
 > -  target_terminal_inferior ();
 > -
 >    if (async_exec)
 >      {
 >        /* The user requested an `attach&', so be sure to leave threads
 > @@ -2495,9 +2492,11 @@ attach_command (char *args, int from_tty)
 >       shouldn't refer to attach_target again.  */
 >    attach_target = NULL;
 > 
 > -  /* Set up the "saved terminal modes" of the inferior
 > -     based on what modes we are starting it with.  */
 > +  /* Set up the "saved terminal modes" of the inferior based on what
 > +     modes we are starting it with, and remove stdin from the event
 > +     loop.  */
 >    target_terminal_init ();
 > +  target_terminal_inferior ();
 > 
 >    /* Set up execution context to know that we should return from
 >       wait_for_inferior as soon as the target reports a stop.  */

Our nomenclature here is problematic. I always do a double take when
I see attach and target_terminal_foo mentioned in the same sentence.
Bleah.

For the case at hand, and at least until we have something more
readable, can I ask for a slight change here?

target_terminal_inferior can do a lot more than just "remove stdin
from the event loop", thus as a reader I'm left being unsure
if there aren't still more bugs here.

Plus, while I can see how to map the comment to the code in the patch,

"Set up the "saved terminal modes" of the inferior based on what
modes we are starting it with"
-->
  target_terminal_init ();

and

"remove stdin from the event loop"
-->
  target_terminal_inferior ();

If I look at the result after the patch,
combining the two steps into one sentence doesn't help clarify
things given that target_terminal_inferior can do more than just
"remove stdin from the event loop".

E.g., this is a marginal improvement:

  /* Set up the "saved terminal modes" of the inferior based on what
     modes we are starting it with.  */
  target_terminal_init ();
  /* And remove stdin from the event loop.  */
  target_terminal_inferior ();

But I'm hoping for more text in the second comment to explain things
better.