This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368)
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