From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114505 invoked by alias); 31 Jan 2018 09:27:49 -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 114491 invoked by uid 89); 31 Jan 2018 09:27:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=unfinished, Unfortunately X-HELO: mail-vk0-f68.google.com Received: from mail-vk0-f68.google.com (HELO mail-vk0-f68.google.com) (209.85.213.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 31 Jan 2018 09:27:46 +0000 Received: by mail-vk0-f68.google.com with SMTP id q62so8594930vkb.11 for ; Wed, 31 Jan 2018 01:27:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=kAh5sYZ0xdCUntZR5h9X8MG+JDo8l8mMWVLRMN0Y/5w=; b=HsJ5ukSQiIQZQJd5ko4NPbrVuc8U0eFxQTh8YsuGbxSOxZpAfHXHVo7CjD6t3LJWO6 ZWupiRS0ha22WX+Nk3KykIUxdv6XrPTovLGqtZwRNMh/yreJjPoI+y1d1toWSR5RiRBC As4s8NDwMga48rygUxaNIhyGBgDLM2z9vRlpwWWzW8GYkDNektmpddsf6HQ0nmwHl57V vba001mUPJXfSPlf46gZPisNyTHZt0hVqSOF3RkAGqIl9YzqtdGyj+l0sC4H3blmf3h+ 2ptR1QAcRzVkRM21p7jSZvWBGABuHRzMdWL3jIzcbOPhm3ON1/Hg1SFREpO8zCmMLO3w 5+AA== X-Gm-Message-State: AKwxytcN58IzKXbLiIBY480veFDt+U8kmQFla0N0AswPeqcYlYvQ6CVu NQF4GYGtAOfk5kOeb56EPaRecCjyYjS8WDJcb8H8VA== X-Google-Smtp-Source: AH8x224884vv+wpDhf+i2yysdicjOlKSfjux0nADtxT7J7WSSSYgRnlTn50Er93hadtcexxpZ0ej2QWJN2/DBSJIctM= X-Received: by 10.31.58.195 with SMTP id h186mr23666619vka.169.1517390863922; Wed, 31 Jan 2018 01:27:43 -0800 (PST) MIME-Version: 1.0 Received: by 10.103.126.14 with HTTP; Wed, 31 Jan 2018 01:27:43 -0800 (PST) In-Reply-To: <447fdbde-e596-1153-85c7-251a5a767499@redhat.com> References: <1512416651-6970-1-git-send-email-palves@redhat.com> <447fdbde-e596-1153-85c7-251a5a767499@redhat.com> From: Christophe Lyon Date: Wed, 31 Jan 2018 09:27:00 -0000 Message-ID: Subject: Re: [PATCH] Per-inferior target_terminal state, fix PR gdb/13211, more To: Pedro Alves Cc: GDB Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00648.txt.bz2 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 ? Thanks, Christophe > Thanks, > Pedro Alves