* [PATCH] Simplify child_terminal_inferior
@ 2017-11-02 20:17 Pedro Alves
2017-11-06 16:55 ` [pushed] Test attaching to a process that isn't a process group leader (Re: [PATCH] Simplify child_terminal_inferior) Pedro Alves
0 siblings, 1 reply; 3+ messages in thread
From: Pedro Alves @ 2017-11-02 20:17 UTC (permalink / raw)
To: gdb-patches
The comment about Lynx in child_terminal_init reads a bit odd, since
it's not exactly clear what "This" in "This is for Lynx" is referring
to. Looking back in history makes it clearer. When the comment was
originally added, in commit 91ecc8efa9b9, back in 1994, the code
looked like this:
~~~
#ifdef PROCESS_GROUP_TYPE
#ifdef PIDGET
/* This is for Lynx, and should be cleaned up by having Lynx be
a separate debugging target with a version of
target_terminal_init_inferior which passes in the process
group to a generic routine which does all the work (and the
non-threaded child_terminal_init_inferior can just pass in
inferior_pid to the same routine). */
inferior_process_group = PIDGET (inferior_pid);
#else
inferior_process_group = inferior_pid;
#endif
#endif
~~~
So this looked like it was about when GDB was growing support for
multi-threading, and inferior_pid was still a single int for most
ports.
Eventually we got ptid_t, so the comment isn't really useful today.
Particularly more so since we no longer support Lynx as a GDB host.
The only caller left of child_terminal_init_with_pgrp is gnu-nat.c
(the Hurd), and that target uses fork-child, so when we reach
gnu_terminal_init the current inferior must already have the PID set,
and the child must be a process group leader.
I added the assertion to child_terminal_init just because it didn't
feel right to only have a comment stating the assumption. And then
because DJGPP's header declares setpgid but not getpgid (uh...), I had
to add the autoconf check for getpgid. (Note that go32-nat.c
overrides all the terminal-related entry points...)
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* configure.ac: Add getpgid to the checked functions.
* gnu-nat.c (gnu_terminal_init): Delete.
(gnu_target): Don't install gnu_terminal_init.
* inflow.c (child_terminal_init_with_pgrp): Deleted, mergee with ...
(child_terminal_init): ... this function. Assert that inferior is
a process group leader.
---
gdb/config.in | 3 +++
gdb/configure | 2 +-
gdb/configure.ac | 2 +-
gdb/gnu-nat.c | 7 -------
gdb/inflow.c | 24 +++++++-----------------
5 files changed, 12 insertions(+), 26 deletions(-)
diff --git a/gdb/config.in b/gdb/config.in
index 3f8ee38..cbe44a7 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -192,6 +192,9 @@
/* Define to 1 if you have the `getpagesize' function. */
#undef HAVE_GETPAGESIZE
+/* Define to 1 if you have the `getpgid' function. */
+#undef HAVE_GETPGID
+
/* Define to 1 if you have the `getrlimit' function. */
#undef HAVE_GETRLIMIT
diff --git a/gdb/configure b/gdb/configure
index c638652..55e3a30 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -12678,7 +12678,7 @@ fi
for ac_func in getauxval getrusage getuid getgid \
pipe poll pread pread64 pwrite resize_term \
- sbrk setpgid setpgrp setsid \
+ sbrk getpgid setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
setrlimit getrlimit posix_madvise waitpid \
diff --git a/gdb/configure.ac b/gdb/configure.ac
index b909217..39d2485 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1358,7 +1358,7 @@ AC_FUNC_MMAP
AC_FUNC_VFORK
AC_CHECK_FUNCS([getauxval getrusage getuid getgid \
pipe poll pread pread64 pwrite resize_term \
- sbrk setpgid setpgrp setsid \
+ sbrk getpgid setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
setrlimit getrlimit posix_madvise waitpid \
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 7cb6e4a..2ae2031 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2279,12 +2279,6 @@ gnu_detach (struct target_ops *ops, const char *args, int from_tty)
inf_child_maybe_unpush_target (ops);
}
\f
-static void
-gnu_terminal_init (struct target_ops *self)
-{
- gdb_assert (gnu_current_inf);
- child_terminal_init_with_pgrp (gnu_current_inf->pid);
-}
static void
gnu_stop (struct target_ops *self, ptid_t ptid)
@@ -2693,7 +2687,6 @@ gnu_target (void)
t->to_wait = gnu_wait;
t->to_xfer_partial = gnu_xfer_partial;
t->to_find_memory_regions = gnu_find_memory_regions;
- t->to_terminal_init = gnu_terminal_init;
t->to_kill = gnu_kill_inferior;
t->to_create_inferior = gnu_create_inferior;
t->to_mourn_inferior = gnu_mourn_inferior;
diff --git a/gdb/inflow.c b/gdb/inflow.c
index b3f14e4..594717e 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -163,7 +163,7 @@ gdb_has_a_terminal (void)
before we actually run the inferior. */
void
-child_terminal_init_with_pgrp (int pgrp)
+child_terminal_init (struct target_ops *self)
{
struct inferior *inf = current_inferior ();
struct terminal_info *tinfo = get_inflow_inferior_data (inf);
@@ -171,8 +171,12 @@ child_terminal_init_with_pgrp (int pgrp)
#ifdef HAVE_TERMIOS_H
/* Store the process group even without a terminal as it is used not
only to reset the tty foreground process group, but also to
- interrupt the inferior. */
- tinfo->process_group = pgrp;
+ interrupt the inferior. We assume the child is a process group
+ leader at this point. */
+ tinfo->process_group = inf->pid;
+#ifdef HAVE_GETPGID
+ gdb_assert (getpgid (inf->pid) == inf->pid);
+#endif
#endif
if (gdb_has_a_terminal ())
@@ -202,20 +206,6 @@ gdb_save_tty_state (void)
}
}
-void
-child_terminal_init (struct target_ops *self)
-{
-#ifdef HAVE_TERMIOS_H
- /* This is for Lynx, and should be cleaned up by having Lynx be a
- separate debugging target with a version of target_terminal::init
- which passes in the process group to a generic routine which does
- all the work (and the non-threaded child_terminal_init can just
- pass in inferior_ptid to the same routine). */
- /* We assume INFERIOR_PID is also the child's process group. */
- child_terminal_init_with_pgrp (ptid_get_pid (inferior_ptid));
-#endif /* HAVE_TERMIOS_H */
-}
-
/* RAII class used to ignore SIGTTOU in a scope. */
class scoped_ignore_sigttou
--
2.5.5
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pushed] Test attaching to a process that isn't a process group leader (Re: [PATCH] Simplify child_terminal_inferior)
2017-11-02 20:17 [PATCH] Simplify child_terminal_inferior Pedro Alves
@ 2017-11-06 16:55 ` Pedro Alves
2017-11-06 17:05 ` [pushed v2] Simplify child_terminal_inferior Pedro Alves
0 siblings, 1 reply; 3+ messages in thread
From: Pedro Alves @ 2017-11-06 16:55 UTC (permalink / raw)
To: GDB Patches
On 11/02/2017 08:17 PM, Pedro Alves wrote:
> I added the assertion to child_terminal_init just because it didn't
> feel right to only have a comment stating the assumption. And then
> because DJGPP's header declares setpgid but not getpgid (uh...), I had
> to add the autoconf check for getpgid. (Note that go32-nat.c
> overrides all the terminal-related entry points...)
Whoops, I somehow forgot/missed that target_terminal::init() is
called when attaching too, not just when spawning a new child.
So that assertion would trigger if you tried to attach to a
fork child that hadn't make itself a process group leader...
So I've pushed a new testcase that would fail with
the assertion in place...
From 46f67f80ddfeea11d4a8134b347c74581faff8f6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 6 Nov 2017 15:36:47 +0000
Subject: [PATCH] Test attaching to a process that isn't a process group leader
The patch at
<https://sourceware.org/ml/gdb-patches/2017-11/msg00039.html> was
proposing to add an assertion to child_terminal_init that turns out
would fail if you tried to attach to a process that isn't a process
group leader.
Since the testsuite failed to catch the problem, this commit adds a
new testcase that would catch it, like:
(gdb) attach 12415
Attaching to program: build/gdb/testsuite/outputs/gdb.base/attach-non-pgrp-leader/attach-non-pgrp-leader, process 12415
src/gdb/inflow.c:180: internal-error: void child_terminal_init(target_ops*): Assertion `getpgid (inf->pid) == inf->pid' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.base/attach-non-pgrp-leader.exp: child: attach to child (GDB internal error)
gdb/testsuite/ChangeLog:
2017-11-06 Pedro Alves <palves@redhat.com>
* gdb.base/attach-non-pgrp-leader.c: New.
* gdb.base/attach-non-pgrp-leader.exp: New.
---
gdb/testsuite/ChangeLog | 5 ++
gdb/testsuite/gdb.base/attach-non-pgrp-leader.c | 45 ++++++++++++++
gdb/testsuite/gdb.base/attach-non-pgrp-leader.exp | 76 +++++++++++++++++++++++
3 files changed, 126 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/attach-non-pgrp-leader.c
create mode 100644 gdb/testsuite/gdb.base/attach-non-pgrp-leader.exp
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index fce9deb..371e98b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
2017-11-06 Pedro Alves <palves@redhat.com>
+ * gdb.base/attach-non-pgrp-leader.c: New.
+ * gdb.base/attach-non-pgrp-leader.exp: New.
+
+2017-11-06 Pedro Alves <palves@redhat.com>
+
* configure.ac: No longer check for termio.h and sgtty.h.
* configure: Regenerate.
* remote-utils.c: Include termios.h instead of gdb_termios.h.
diff --git a/gdb/testsuite/gdb.base/attach-non-pgrp-leader.c b/gdb/testsuite/gdb.base/attach-non-pgrp-leader.c
new file mode 100644
index 0000000..acfc284
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-non-pgrp-leader.c
@@ -0,0 +1,45 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2017 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdlib.h>
+#include <unistd.h>
+
+/* GDB reads this to figure out the child's PID. */
+pid_t child_pid;
+
+void
+marker (void)
+{
+}
+
+int
+main ()
+{
+ alarm (60);
+
+ child_pid = fork ();
+ if (child_pid == -1)
+ return 1;
+
+ while (1)
+ {
+ marker ();
+ usleep (1000);
+ }
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/attach-non-pgrp-leader.exp b/gdb/testsuite/gdb.base/attach-non-pgrp-leader.exp
new file mode 100644
index 0000000..ed1823b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-non-pgrp-leader.exp
@@ -0,0 +1,76 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+# Check that we can attach to processes that are not process group
+# leaders. We test that by attaching to a fork child that doesn't
+# call any of setpgrp/setpgid/setsid to move itself to a new process
+# group.
+
+if {![can_spawn_for_attach]} {
+ return 0
+}
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile} $srcfile {debug}] == -1 } {
+ return -1
+}
+
+proc do_test {} {
+ global binfile
+
+ set test_spawn_id [spawn_wait_for_attach $binfile]
+ set parent_pid [spawn_id_get_pid $test_spawn_id]
+
+ # Attach to the parent, run it to a known point, extract the
+ # child's PID, and detach.
+ with_test_prefix "parent" {
+ clean_restart ${binfile}
+
+ gdb_test "attach $parent_pid" \
+ "Attaching to program.*, process $parent_pid.*" \
+ "attach"
+
+ gdb_breakpoint "marker"
+ gdb_continue_to_breakpoint "marker"
+
+ set child_pid [get_integer_valueof "child_pid" -1]
+ if {$child_pid == -1} {
+ return
+ }
+
+ gdb_test "detach" \
+ "Detaching from program: .*process $parent_pid"
+ }
+
+ # Start over, and attach to the child this time.
+ with_test_prefix "child" {
+ clean_restart $binfile
+
+ gdb_test "attach $child_pid" \
+ "Attaching to program.*, process $child_pid.*" \
+ "attach"
+
+ gdb_breakpoint "marker"
+ gdb_continue_to_breakpoint "marker"
+
+ gdb_test "detach" \
+ "Detaching from program: .*process $child_pid"
+ }
+
+ kill_wait_spawned_process $test_spawn_id
+}
+
+do_test
--
2.5.5
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pushed v2] Simplify child_terminal_inferior
2017-11-06 16:55 ` [pushed] Test attaching to a process that isn't a process group leader (Re: [PATCH] Simplify child_terminal_inferior) Pedro Alves
@ 2017-11-06 17:05 ` Pedro Alves
0 siblings, 0 replies; 3+ messages in thread
From: Pedro Alves @ 2017-11-06 17:05 UTC (permalink / raw)
To: GDB Patches
On 11/06/2017 04:55 PM, Pedro Alves wrote:
> On 11/02/2017 08:17 PM, Pedro Alves wrote:
>
>> I added the assertion to child_terminal_init just because it didn't
>> feel right to only have a comment stating the assumption. And then
>> because DJGPP's header declares setpgid but not getpgid (uh...), I had
>> to add the autoconf check for getpgid. (Note that go32-nat.c
>> overrides all the terminal-related entry points...)
>
> Whoops, I somehow forgot/missed that target_terminal::init() is
> called when attaching too, not just when spawning a new child.
> So that assertion would trigger if you tried to attach to a
> fork child that hadn't make itself a process group leader...
>
> So I've pushed a new testcase that would fail with
> the assertion in place...
>
... and here's the patch that I pushed, without the assertion.
From 556e5da51349d00307bc05b7a6a813f71ac58664 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 6 Nov 2017 15:36:47 +0000
Subject: [PATCH] Simplify child_terminal_inferior
The comment about Lynx in child_terminal_init reads a bit odd, since
it's not exactly clear what "This" in "This is for Lynx" is referring
to. Looking back in history makes it clearer. When the comment was
originally added, in commit 91ecc8efa9b9, back in 1994, the code
looked like this:
~~~
#ifdef PROCESS_GROUP_TYPE
#ifdef PIDGET
/* This is for Lynx, and should be cleaned up by having Lynx be
a separate debugging target with a version of
target_terminal_init_inferior which passes in the process
group to a generic routine which does all the work (and the
non-threaded child_terminal_init_inferior can just pass in
inferior_pid to the same routine). */
inferior_process_group = PIDGET (inferior_pid);
#else
inferior_process_group = inferior_pid;
#endif
#endif
~~~
So this looked like it was about when GDB was growing support for
multi-threading, and inferior_pid was still a single int for most
ports.
Eventually we got ptid_t, so the comment isn't really useful today.
Particularly more so since we no longer support Lynx as a GDB host.
The only caller left of child_terminal_init_with_pgrp is gnu-nat.c
(the Hurd), and that target uses fork-child, so when we reach
gnu_terminal_init after spawning a new child, the current inferior
must already have the PID set, and the child must be a process group
leader.
We can't add a 'getpgid(inf->pid) == inf->pid' assertion to
child_terminal_init though (like a previous version of this patch was
doing [1]), because child_terminal_init is also reached after
attaching to a process. If we did, the new
gdb.base/attach-non-pgrp-leader.exp test would fail, with:
(gdb) attach 12415
Attaching to program: build/gdb/testsuite/outputs/gdb.base/attach-non-pgrp-leader/attach-non-pgrp-leader, process 12415
src/gdb/inflow.c:180: internal-error: void child_terminal_init(target_ops*): Assertion `getpgid (inf->pid) == inf->pid' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.base/attach-non-pgrp-leader.exp: child: attach to child (GDB internal error)
I'm not making GDB save the pgid for attached processes with getpgid
for now, because the saved process group affects other things which
I'm leaving for following patches, like e.g., the "interrupt" command.
[1] - https://sourceware.org/ml/gdb-patches/2017-11/msg00039.html
gdb/ChangeLog:
2017-11-06 Pedro Alves <palves@redhat.com>
* gnu-nat.c (gnu_terminal_init): Delete.
(gnu_target): Don't install gnu_terminal_init.
* inflow.c (child_terminal_init_with_pgrp): Delete, merged with ...
(child_terminal_init): ... this function.
---
gdb/ChangeLog | 7 +++++++
gdb/gnu-nat.c | 7 -------
gdb/inflow.c | 22 +++++-----------------
3 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7a90ea5..ac5b318 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
2017-11-06 Pedro Alves <palves@redhat.com>
+ * gnu-nat.c (gnu_terminal_init): Delete.
+ (gnu_target): Don't install gnu_terminal_init.
+ * inflow.c (child_terminal_init_with_pgrp): Delete, merged with ...
+ (child_terminal_init): ... this function.
+
+2017-11-06 Pedro Alves <palves@redhat.com>
+
* common/common.m4 (GDB_AC_COMMON): No longer check termio.h nor
sgtty.h.
* config.in, configure: Regenerate.
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 7cb6e4a..2ae2031 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2279,12 +2279,6 @@ gnu_detach (struct target_ops *ops, const char *args, int from_tty)
inf_child_maybe_unpush_target (ops);
}
\f
-static void
-gnu_terminal_init (struct target_ops *self)
-{
- gdb_assert (gnu_current_inf);
- child_terminal_init_with_pgrp (gnu_current_inf->pid);
-}
static void
gnu_stop (struct target_ops *self, ptid_t ptid)
@@ -2693,7 +2687,6 @@ gnu_target (void)
t->to_wait = gnu_wait;
t->to_xfer_partial = gnu_xfer_partial;
t->to_find_memory_regions = gnu_find_memory_regions;
- t->to_terminal_init = gnu_terminal_init;
t->to_kill = gnu_kill_inferior;
t->to_create_inferior = gnu_create_inferior;
t->to_mourn_inferior = gnu_mourn_inferior;
diff --git a/gdb/inflow.c b/gdb/inflow.c
index d46d693..2fba0fa 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -165,7 +165,7 @@ gdb_has_a_terminal (void)
before we actually run the inferior. */
void
-child_terminal_init_with_pgrp (int pgrp)
+child_terminal_init (struct target_ops *self)
{
struct inferior *inf = current_inferior ();
struct terminal_info *tinfo = get_inflow_inferior_data (inf);
@@ -173,8 +173,10 @@ child_terminal_init_with_pgrp (int pgrp)
#ifdef HAVE_TERMIOS_H
/* Store the process group even without a terminal as it is used not
only to reset the tty foreground process group, but also to
- interrupt the inferior. */
- tinfo->process_group = pgrp;
+ interrupt the inferior. A child we spawn should be a process
+ group leader (PGID==PID) at this point, though that may not be
+ true if we're attaching to an existing process. */
+ tinfo->process_group = inf->pid;
#endif
if (gdb_has_a_terminal ())
@@ -204,20 +206,6 @@ gdb_save_tty_state (void)
}
}
-void
-child_terminal_init (struct target_ops *self)
-{
-#ifdef HAVE_TERMIOS_H
- /* This is for Lynx, and should be cleaned up by having Lynx be a
- separate debugging target with a version of target_terminal::init
- which passes in the process group to a generic routine which does
- all the work (and the non-threaded child_terminal_init can just
- pass in inferior_ptid to the same routine). */
- /* We assume INFERIOR_PID is also the child's process group. */
- child_terminal_init_with_pgrp (ptid_get_pid (inferior_ptid));
-#endif /* HAVE_TERMIOS_H */
-}
-
/* Put the inferior's terminal settings into effect.
This is preparation for starting or resuming the inferior.
--
2.5.5
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-06 17:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-02 20:17 [PATCH] Simplify child_terminal_inferior Pedro Alves
2017-11-06 16:55 ` [pushed] Test attaching to a process that isn't a process group leader (Re: [PATCH] Simplify child_terminal_inferior) Pedro Alves
2017-11-06 17:05 ` [pushed v2] Simplify child_terminal_inferior Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).