From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92248 invoked by alias); 31 Jan 2018 12:32:38 -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 92217 invoked by uid 89); 31 Jan 2018 12:32:37 -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,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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; Wed, 31 Jan 2018 12:32:30 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 954CB7EA85; Wed, 31 Jan 2018 12:32:29 +0000 (UTC) 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 D2F1A60BE3; Wed, 31 Jan 2018 12:32:28 +0000 (UTC) Subject: Re: [PATCH] Per-inferior target_terminal state, fix PR gdb/13211, more To: Christophe Lyon References: <1512416651-6970-1-git-send-email-palves@redhat.com> <447fdbde-e596-1153-85c7-251a5a767499@redhat.com> Cc: GDB Patches From: Pedro Alves Message-ID: <34130916-a57c-48be-60a7-c87ac2f02137@redhat.com> Date: Wed, 31 Jan 2018 12:32:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-01/txt/msg00652.txt.bz2 On 01/31/2018 09:27 AM, Christophe Lyon wrote: > On 30 January 2018 at 16:37, Pedro Alves 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