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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-10-17 1:04 p.m., Pedro Alves wrote:
> 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.

Yeah that makes sense.  This crossed my mind when I look at the issue,
but for some reason I didn't went that route.  But now that you say it,
it appears obvious that this should be the fix.

I just tried copying the inferior::terminal_state value from the old
inferior to the new inferior, and it fixes the problem.  But
actually, we should also be transferring all of the struct terminal_info
associated to the inferior, is that right?

> 
>> 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.

Ok.

Thanks,

Simon