Re: [PATCH] Per-inferior target_terminal state, fix PR gdb/13211, more

[Simon Marchi <simark@simark.ca>]

On 2017-12-04 02:44 PM, Pedro Alves wrote:
> (This applies on top of:
> 
>   [PATCH] linux-nat: Eliminate custom target_terminal_{inferior,ours}, stop using set_sigint_trap
>   https://sourceware.org/ml/gdb-patches/2017-11/msg00368.html
> )
> 
> In my multi-target branch I ran into problems with GDB's terminal
> handling that exist in master as well, with multi-inferior debugging.
> 
> This patch adds a testcase for said problems
> (gdb.multi/multi-term-settings.exp), fixes the problems, fixes PR
> gdb/13211 as well (and adds a testcase for that too,
> gdb.base/interrupt-daemon.exp).
> 
> The basis of the problem I ran into is the following.  Consider a
> scenario where you have:
> 
>  - inferior 1 - started with "attach", process is running on some
>    other terminal.
> 
>  - inferior 2 - started with "run", process is sharing gdb's terminal.
> 
> In this scenario, when you stop/resume both inferiors, you want GDB to
> save/restore the terminal settings of inferior 2, the one that is
> sharing GDB's terminal.  I.e., you want inferior 2 to "own" the
> terminal (in target_terminal::is_ours/target_terminal::is_inferior
> sense).
> 
> Unfortunately, that's not what you get currently.  Because GDB doesn't
> know whether an attached inferior is actually sharing GDB's terminal,
> it tries to save/restore its settings anyway, ignoring errors.  In
> this case, this is pointless, because inferior 1 is running on a
> different terminal, but GDB doesn't know better.
> 
> And then, because it is only possible to have the terminal settings of
> a single inferior be in effect at a time, or make one inferior/pgrp be
> the terminal's foreground pgrp (aka, only one inferior can "own" the
> terminal, ignoring fork children here), if GDB happens to try to
> restore the terminal settings of inferior 1 first, then GDB never
> restores the terminal settings of inferior 2.
> 
> This patch fixes that and a few things more along the way:
> 
>  - Moves enum target_terminal::terminal_state out of the
>    target_terminal class (it's currently private) and makes it a
>    scoped enum so that it can be easily used elsewhere.
> 
>  - Replaces the inflow.c:terminal_is_ours boolean with a
>    target_terminal_state variable.  This allows distinguishing is_ours
>    and is_ours_for_output states.  This allows finally making
>    child_terminal_ours_1 do something with its "output_only"
>    parameter.
> 
>  - Makes each inferior have its own copy of the
>    is_ours/is_ours_for_output/is_inferior state.
> 
>  - Adds a way for GDB to tell whether the inferior is sharing GDB's
>    terminal.  Works best on Linux and Solaris; the fallback works just
>    as well as currently.
> 
>  - With that, we can remove the inf->attach_flag tests from
>    child_terminal_inferior/child_terminal_ours.
> 
>  - Currently target_ops.to_ours is responsible for both saving the
>    current inferior's terminal state, and restoring gdb's state.
>    Because each inferior has its own terminal state (possibly handled
>    by different targets in a multi-target world, even), we need to
>    split the inferior-saving part from the gdb-restoring part.  The
>    patch adds a new target_ops.to_save_inferior target method for
>    that.
> 
>  - Adds a new target_terminal::save_inferior() function, so that
>    sequences like:
> 
>      scoped_restore_terminal_state save_state;
>      target_terminal::ours_for_output ();
> 
>    ... restore back inferiors that were
>    target_terminal_state::is_inferior before back to is_inferior, and
>    leaves inferiors that were is_ours alone.
> 
>  - Along the way, this adds a default implementation of
>    target_pass_ctrlc to inflow.c (for inf-child.c), that handles
>    passing the Ctrl-C to a process running on GDB's terminal or to
>    some other process otherwise.
> 
>  - Similarly, adds a new target default implementation of
>    target_interrupt, for the "interrupt" command.  The current
>    implementation of this hook in inf-ptrace.c kills the whole process
>    group, but that's incorrect/undesirable because we may not be
>    attached to all processes in the process group.  And also, it's
>    incorrect because inferior_process_group() doesn't really return
>    the inferior's real process group id if the inferior is not a
>    process group leader...  This is the cause of PR gdb/13211 [1],
>    which this patch fixes.  While at it, that target method's "ptid"
>    parameter is eliminated, because it's not really used.
> 
>  - A new test is included that exercises and fixes PR gdb/13211, and
>    also fixes a GDB issue reported on stackoverflow that I ran into
>    while working on this [2].  The problem is similar to PR gdb/13211,
>    except that it also triggers with Ctrl-C.  When debugging a daemon
>    (i.e., a process that disconnects from the controlling terminal and
>    is not a process group leader, then Ctrl-C doesn't work, you just
>    can't interrupt the inferior at all, resulting in a hung debug
>    session.  The problem is that since the inferior is no longer
>    associated with gdb's session / controlling terminal, then trying
>    to put the inferior in the foreground fails.  And so Ctrl-C never
>    reaches the inferior directly.  pass_signal is only used when the
>    inferior is attached, but that is not the case here.  This is fixed
>    by the new child_pass_ctrlc.  Without the fix, the new
>    interrupt-daemon.exp testcase fails with timeout waiting for a
>    SIGINT that never arrives.
> 
> [1] PR gdb/13211 - Async / Process group and interrupt not working
> https://sourceware.org/bugzilla/show_bug.cgi?id=13211
> 
> [2] GDB not reacting Ctrl-C when after fork() and setsid()
> https://stackoverflow.com/questions/46101292/gdb-not-reacting-ctrl-c-when-after-fork-and-setsid
> 
> Note this patch does _not_ fix:
> 
>  - PR gdb/14559 - The 'interrupt' command does not work if sigwait is in use
>    https://sourceware.org/bugzilla/show_bug.cgi?id=14559
> 
>  - PR gdb/9425 - When using "sigwait" GDB doesn't trap SIGINT. Ctrl+C terminates program when should break gdb.
>    https://sourceware.org/bugzilla/show_bug.cgi?id=9425
> 
> The only way to fix that that I know of (without changing the kernel)
> is to make GDB put inferiors in a separate session (create a
> pseudo-tty master/slave pair, make the inferior run with the slave as
> its terminal, and have gdb pump output/input on the master end).  I
> have a follow up series that builds on top of this one that does that,
> but that's too invasive for 8.1, I think.  While this one fixes a
> couple bugs already, and I think having it in 8.1 would simplify
> backports for (a future) 8.1.1.

Hi Pedro,

I found a regression caused by this commit, I filed a bug here:

https://sourceware.org/bugzilla/show_bug.cgi?id=23020

I'll take a look, but I'm always confused by the terminal target/ours switching
flow, so I don't have high hopes of finding a good fix.

Simon