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

Pedro Alves
2018-01-31T12:32:38Z

On 01/31/2018 09:27 AM, Christophe Lyon wrote:
> On 30 January 2018 at 16:37, Pedro Alves <palves@redhat.com> wrote:
>> On 12/04/2017 07: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.
>>
>> I chickened out of putting this in 8.1, but I pushed it to
>> master now, along with the prerequisite patch.
>>
>
> Hi,
>
> This caused GDB to fail to build for arm-none-eabi targets
> ../../../gdb/remote-sim.c: In function 'void init_gdbsim_ops()':
> ../../../gdb/remote-sim.c:1342:27: error: invalid conversion from
> 'void (*)(target_ops*, ptid_t)' to 'void (*)(target_ops*)'
> [-fpermissive]
> gdbsim_ops.to_interrupt = gdbsim_interrupt;
> ^
> make[2]: *** [remote-sim.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> ../../../gdb/cli/cli-cmds.c: In function 'void complete_command(const
> char*, int)':
> ../../../gdb/cli/cli-cmds.c:304:48: warning: 'word' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> get_max_completions_reached_message ());
> ^
> ../../../gdb/cli/cli-cmds.c:277:71: warning: 'tracker' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> = tracker->build_completion_result (word, word - arg, strlen (arg));
>
> (building in an Ubuntu Trusty container)
>
> Can you fix it ?

Sorry about that. Don't know how I missed this to_interrupt
implementation. And windows-nat.c too. I'll take a look.

Thanks,
Pedro Alves