Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368)

[Pedro Alves <palves at redhat dot com>]

On 10/16/2018 04:38 AM, Simon Marchi wrote:
> Here's a summary of PR 23368:
> 
>   #include <unistd.h>
>   int main (void)
>   {
>     char *exec_args[] = { "/bin/ls", NULL };
>     execve (exec_args[0], exec_args, NULL);
>   }
> 
> $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
> ...
> [1]  + 13146 suspended (tty output)  gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
> $
> 
> Here's what happens: when the inferior execs with "follow-exec-mode
> new", we first "mourn" it before creating the new one.  This ends up
> calling inflow_inferior_exit, which sets the per-inferior terminal state
> to "is_ours":
> 
>   inf->terminal_state = target_terminal_state::is_ours;
> 
> At this point, the inferior's terminal_state is is_ours, while the
> "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the
> terminal).
> 
> Later, we continue processing the exec inferior event and decide we want
> to stop (because of the "catch exec") and call target_terminal::ours to
> make sure we own the terminal.  However, we don't actually go to the
> target backend to change the settings, because the core thinks that no
> inferior owns the terminal (inf->terminal_state is
> target_terminal_state::is_ours, as checked in
> target_terminal_is_ours_kind, for both inferiors). 
                                ^^^^^^^^^^^^^^^^^^

I think the "for both inferiors" part is what's dubious here.

The process lives on in the new inferior, but we lost its
terminal settings!  Seems to me that they should be migrated
from the old inferior to the new one.  And then the problem
sorts itself out, because then the new inferior will have
target_terminal_state::is_inferior state.

> When something in
> readline tries to mess with the terminal settings, it generates a
> SIGTTOU.
> 
> This patch manages to fix this particular case by calling
> target_terminal::ours() in inflow_inferior_exit.  This makes so that
> inflow actually changes the terminal settings so that GDB owns it, which
> avoids the SIGTTOU later.
> 
> The buildbot doesn't complain, but I am not sure this is the most
> bestest way to fix this, maybe it's just papering over the actual
> problem or introduces some latent bug.  In particular, I am not sure if
> this is correct in case we have multiple inferiors sharing the same
> terminal.  But I thought I would still submit this patch to get the ball
> rolling.

Yeah, I don't think this is right in the multi-inferior (or multi-target)
case.  It may happen to just work because we always stop everything
when an inferior exits.  In the exec case, if you don't catch the exec,
then we won't stop; likely it ends up working because we end up calling
target_terminal::inferior() on resume.  Still, I think this is
a hack that would likely cause trouble down the road.

> 
> gdb/ChangeLog:
> 
> 	PR gdb/23368
> 	* inflow.c (inflow_inferior_exit): Update doc.  Call
> 	target_terminal::ours.
> ---
>  gdb/inflow.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index e355f4aa9fc5..95e195167ef7 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -653,18 +653,15 @@ get_inflow_inferior_data (struct inferior *inf)
>    return info;
>  }
>  
> -/* This is a "inferior_exit" observer.  Releases the TERMINAL_INFO member
> -   of the inferior structure.  This field is private to inflow.c, and
> -   its type is opaque to the rest of GDB.  PID is the target pid of
> -   the inferior that is about to be removed from the inferior
> -   list.  */
> +/* This is a "inferior_exit" observer.  Releases the terminal info associated
> +   to INF.  */
>  
>  static void
>  inflow_inferior_exit (struct inferior *inf)
>  {
>    struct terminal_info *info;
>  
> -  inf->terminal_state = target_terminal_state::is_ours;
> +  target_terminal::ours ();
>  
>    info = (struct terminal_info *) inferior_data (inf, inflow_inferior_data);
>    if (info != NULL)

Thanks,
Pedro Alves