From patchwork Mon Nov 6 17:05:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 24119 Received: (qmail 56133 invoked by alias); 6 Nov 2017 17:05:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 56123 invoked by uid 89); 6 Nov 2017 17:05:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Particularly X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 Nov 2017 17:05:38 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3E7F1E313E for ; Mon, 6 Nov 2017 17:05:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3E7F1E313E Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id A1543190DA for ; Mon, 6 Nov 2017 17:05:36 +0000 (UTC) Subject: [pushed v2] Simplify child_terminal_inferior To: GDB Patches References: <1509653822-10820-1-git-send-email-palves@redhat.com> From: Pedro Alves Message-ID: <2471e5db-4a4d-7992-1992-aca17c63268b@redhat.com> Date: Mon, 6 Nov 2017 17:05:35 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: 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 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 * 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 + * 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 + * 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); } -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.