From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121628 invoked by alias); 30 Mar 2018 14:54:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 121616 invoked by uid 89); 30 Mar 2018 14:54:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=leaves, UD:ca, alone, 8.1 X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Mar 2018 14:54:06 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 25E571E073; Fri, 30 Mar 2018 10:54:04 -0400 (EDT) Subject: Re: [PATCH] Per-inferior target_terminal state, fix PR gdb/13211, more To: Pedro Alves , gdb-patches@sourceware.org References: <1512416651-6970-1-git-send-email-palves@redhat.com> From: Simon Marchi Message-ID: Date: Fri, 30 Mar 2018 14:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1512416651-6970-1-git-send-email-palves@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00608.txt.bz2 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