diff options
author | Pedro Alves <palves@redhat.com> | 2018-01-30 14:23:51 +0000 |
---|---|---|
committer | Pedro Alves <palves@redhat.com> | 2018-01-30 14:55:18 +0000 |
commit | e671cd59d74cec9f53e110ce887128d1eeadb7f2 (patch) | |
tree | c2e6a7cd82327a763f8648770e9326a02fbc71ef /gdb/target | |
parent | 9c3a5d9319648db16b30a91253ad02d41d242cef (diff) | |
download | binutils-gdb-e671cd59d74cec9f53e110ce887128d1eeadb7f2.tar.gz |
Per-inferior target_terminal state, fix PR gdb/13211, more
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).
gdb/ChangeLog:
2018-01-30 Pedro Alves <palves@redhat.com>
PR gdb/13211
* config.in, configure: Regenerate.
* configure.ac: Check for getpgid.
* go32-nat.c (go32_pass_ctrlc): New.
(go32_target): Install it.
* inf-child.c (inf_child_target): Install
child_terminal_save_inferior, child_pass_ctrlc and
child_interrupt.
* inf-ptrace.c (inf_ptrace_interrupt): Delete.
(inf_ptrace_target): No longer install it.
* infcmd.c (interrupt_target_1): Adjust.
* inferior.h (child_terminal_save_inferior, child_pass_ctrlc)
(child_interrupt): Declare.
(inferior::terminal_state): New.
* inflow.c (struct terminal_info): Update comments.
(inferior_process_group): Delete.
(terminal_is_ours): Delete.
(gdb_tty_state): New.
(child_terminal_init): Adjust.
(is_gdb_terminal, sharing_input_terminal_1)
(sharing_input_terminal): New functions.
(child_terminal_inferior): Adjust. Use sharing_input_terminal.
Set the process's actual process group in the foreground if
possible. Handle is_ours_for_output/is_ours distinction. Don't
mark terminal as the inferior's if not sharing GDB's terminal.
Don't check attach_flag.
(child_terminal_ours_for_output, child_terminal_ours): Adjust to
pass down a target_terminal_state.
(child_terminal_save_inferior): New, factored out from ...
(child_terminal_ours_1): ... this. Handle
target_terminal_state::is_ours_for_output.
(child_interrupt, child_pass_ctrlc): New.
(inflow_inferior_exit): Clear the inferior's terminal_state.
(copy_terminal_info): Copy the inferior's terminal state.
(_initialize_inflow): Remove reference to terminal_is_ours.
* inflow.h (inferior_process_group): Delete.
* nto-procfs.c (nto_handle_sigint, procfs_interrupt): Adjust.
* procfs.c (procfs_target): Don't install procfs_interrupt.
(procfs_interrupt): Delete.
* remote.c (remote_serial_quit_handler): Adjust.
(remote_interrupt): Remove ptid parameter. Adjust.
* target-delegates.c: Regenerate.
* target.c: Include "terminal.h".
(target_terminal::terminal_state): Rename to ...
(target_terminal::m_terminal_state): ... this.
(target_terminal::init): Adjust.
(target_terminal::inferior): Adjust to per-inferior
terminal_state.
(target_terminal::restore_inferior, target_terminal_is_ours_kind): New.
(target_terminal::ours, target_terminal::ours_for_output): Use
target_terminal_is_ours_kind.
(target_interrupt): Remove ptid parameter. Adjust.
(default_target_pass_ctrlc): Adjust.
* target.h (target_ops::to_terminal_save_inferior): New field.
(target_ops::to_interrupt): Remove ptid_t parameter.
(target_interrupt): Remove ptid_t parameter. Update comment.
(target_pass_ctrlc): Update comment.
* target/target.h (target_terminal_state): New scoped enum,
factored out of ...
(target_terminal::terminal_state): ... here.
(target_terminal::inferior): Update comments.
(target_terminal::restore_inferior): New.
(target_terminal::is_inferior, target_terminal::is_ours)
(target_terminal::is_ours_for_output): Adjust.
(target_terminal::scoped_restore_terminal_state): Adjust to
rename, and call restore_inferior() instead of inferior().
(target_terminal::scoped_restore_terminal_state::m_state): Change
type.
(target_terminal::terminal_state): Rename to ...
(target_terminal::m_terminal_state): ... this and change type.
gdb/gdbserver/ChangeLog:
2018-01-30 Pedro Alves <palves@redhat.com>
PR gdb/13211
* target.c (target_terminal::terminal_state): Rename to ...
(target_terminal::m_terminal_state): ... this.
gdb/testsuite/ChangeLog:
2018-01-30 Pedro Alves <palves@redhat.com>
PR gdb/13211
* gdb.base/interrupt-daemon.c: New.
* gdb.base/interrupt-daemon.exp: New.
* gdb.multi/multi-term-settings.c: New.
* gdb.multi/multi-term-settings.exp: New.
Diffstat (limited to 'gdb/target')
-rw-r--r-- | gdb/target/target.h | 67 |
1 files changed, 38 insertions, 29 deletions
diff --git a/gdb/target/target.h b/gdb/target/target.h index 5571e16b34f..879e7308ab7 100644 --- a/gdb/target/target.h +++ b/gdb/target/target.h @@ -95,6 +95,21 @@ extern void target_mourn_inferior (ptid_t ptid); extern int target_supports_multi_process (void); +/* Possible terminal states. */ + +enum class target_terminal_state + { + /* The inferior's terminal settings are in effect. */ + is_inferior = 0, + + /* Some of our terminal settings are in effect, enough to get + proper output. */ + is_ours_for_output = 1, + + /* Our terminal settings are in effect, for output and input. */ + is_ours = 2 + }; + /* Represents the state of the target terminal. */ class target_terminal { @@ -108,9 +123,9 @@ public: before we actually run the inferior. */ static void init (); - /* Put the inferior's terminal settings into effect. This is - preparation for starting or resuming the inferior. This is a no-op - unless called with the main UI as current UI. */ + /* Put the current inferior's terminal settings into effect. This + is preparation for starting or resuming the inferior. This is a + no-op unless called with the main UI as current UI. */ static void inferior (); /* Put our terminal settings into effect. First record the inferior's @@ -125,40 +140,34 @@ public: UI as current UI. */ static void ours_for_output (); + /* Restore terminal settings of inferiors that are in + is_ours_for_output state back to "inferior". Used when we need + to temporarily switch to is_ours_for_output state. */ + static void restore_inferior (); + /* Returns true if the terminal settings of the inferior are in effect. */ static bool is_inferior () { - return terminal_state == terminal_is_inferior; + return m_terminal_state == target_terminal_state::is_inferior; } /* Returns true if our terminal settings are in effect. */ static bool is_ours () { - return terminal_state == terminal_is_ours; + return m_terminal_state == target_terminal_state::is_ours; + } + + /* Returns true if our terminal settings are in effect. */ + static bool is_ours_for_output () + { + return m_terminal_state == target_terminal_state::is_ours_for_output; } /* Print useful information about our terminal status, if such a thing exists. */ static void info (const char *arg, int from_tty); -private: - - /* Possible terminal states. */ - - enum terminal_state - { - /* The inferior's terminal settings are in effect. */ - terminal_is_inferior = 0, - - /* Some of our terminal settings are in effect, enough to get - proper output. */ - terminal_is_ours_for_output = 1, - - /* Our terminal settings are in effect, for output and input. */ - terminal_is_ours = 2 - }; - public: /* A class that restores the state of the terminal to the current @@ -168,7 +177,7 @@ public: public: scoped_restore_terminal_state () - : m_state (terminal_state) + : m_state (m_terminal_state) { } @@ -176,14 +185,14 @@ public: { switch (m_state) { - case terminal_is_ours: + case target_terminal_state::is_ours: ours (); break; - case terminal_is_ours_for_output: + case target_terminal_state::is_ours_for_output: ours_for_output (); break; - case terminal_is_inferior: - inferior (); + case target_terminal_state::is_inferior: + restore_inferior (); break; } } @@ -192,12 +201,12 @@ public: private: - target_terminal::terminal_state m_state; + target_terminal_state m_state; }; private: - static terminal_state terminal_state; + static target_terminal_state m_terminal_state; }; #endif /* TARGET_COMMON_H */ |