From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29916 invoked by alias); 12 Jan 2015 18:39:13 -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 29904 invoked by uid 89); 12 Jan 2015 18:39:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-ob0-f181.google.com Received: from mail-ob0-f181.google.com (HELO mail-ob0-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 12 Jan 2015 18:39:10 +0000 Received: by mail-ob0-f181.google.com with SMTP id gq1so23667972obb.12 for ; Mon, 12 Jan 2015 10:39:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=AIRB4NsNj6+xt7afrmX7XFQrs9IsvLcwbwIzhMAcBOA=; b=R0GOhRU7IBEmlt10YcENvzRg8XauVZue6wP2lhX8HbVuG4xfXvTUdMsAPa0QGfVUT7 stUR/nKgWpvab66OPfp8bIdL6rgrPKxomlhfazd3eOFE64livf+7adyPVvIwYpnAG7dO Iii2DAIvO4RPmIez8ZCszn7WrFFd+sNbKdHXqLivIL4tT2JdZDX8Gsih3bZevkLZQFNR aqkgl3oALWr8DWmUr5bMGj5INm/eJc6hpZU51Zh7ngMUx7ne29EVnneIYKPuwSoL5DHj Pkcve1D9rai9IcYBAWOdnFPm7cpmzdFIAGkEtQxYCgZtmys8nHYBpAT8yTaL9q7o71fr BslA== X-Gm-Message-State: ALoCoQl2hoB5PTDn/LsvXwQT9UFj7/Q4Nmw4Hwpq9YW/hYsjP41u5+D8q5hTdKPsHL5zsDt3IZ5c MIME-Version: 1.0 X-Received: by 10.202.59.136 with SMTP id i130mr17026994oia.114.1421087948119; Mon, 12 Jan 2015 10:39:08 -0800 (PST) Received: by 10.182.222.98 with HTTP; Mon, 12 Jan 2015 10:39:08 -0800 (PST) In-Reply-To: <1421080814-10269-1-git-send-email-palves@redhat.com> References: <1421080814-10269-1-git-send-email-palves@redhat.com> Date: Mon, 12 Jan 2015 18:39:00 -0000 Message-ID: Subject: Re: [PATCH] [7.8 regression] PR cli/17828: -batch -ex r breaks terminal From: Doug Evans To: Pedro Alves Cc: gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-01/txt/msg00313.txt.bz2 On Mon, Jan 12, 2015 at 8:40 AM, Pedro Alves wrote: > Commit d3d4baed (PR python/17372 - Python hangs when displaying > help()) had the side effect of causing 'gdb -batch' to leave the > terminal in the wrong state if the program was run. E.g,. > > $ echo 'main(){*(int*)0=0;}' | gcc -x c -; ./gdb/gdb -batch -ex r ./a.out > Program received signal SIGSEGV, Segmentation fault. > 0x00000000004004ff in main () > $ > > If you start typing the next command, seemingly nothing happens - GDB > left the terminal with echo disabled. > > The issue is that that "r" ends up in fetch_inferior_event, which > calls reinstall_readline_callback_handler_cleanup, which causes > readline to prep the terminal (raw, echo disabled). But "-batch" > causes GDB to exit before the top level event loop is first started, > and then nothing de-preps the terminal. > > The reinstall_readline_callback_handler_cleanup function's intro > comment mentions: > > "Need to do this as we go back to the event loop, ready to process > further input." > > but the implementation forgets the case of when the interpreter is > sync, which indicates we won't return to the event loop yet, or as in > the case of -batch, we have not started it yet. > > The fix is to not install the readline callback in that case. > > For the test, in this case, checking that command echo still works is > sufficient. Comparing stty output before/after running GDB is even > better. Because stty may not be available, the test tries both ways. > In any case, since expect's spawn (what we use to start gdb) creates a > new pseudo tty, another expect spawn or tcl exec after GDB exits would > not see the wrong terminal settings. So instead, the test spawns a > shell and runs stty and GDB in it. > > Tested on x86_64 Fedora 20. > > gdb/ > 2015-01-12 Pedro Alves > > * infrun.c (reinstall_readline_callback_handler_cleanup): Don't > reinstall is the interpreter is sync. > > gdb/testsuite/ > 2015-01-12 Pedro Alves > > * testsuite/gdb.base/batch-preserve-term-settings.c: New file. > * testsuite/gdb.base/batch-preserve-term-settings.exp: New file. > --- > gdb/infrun.c | 2 +- > .../gdb.base/batch-preserve-term-settings.c | 22 +++ > .../gdb.base/batch-preserve-term-settings.exp | 186 +++++++++++++++++++++ > 3 files changed, 209 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.base/batch-preserve-term-settings.c > create mode 100644 gdb/testsuite/gdb.base/batch-preserve-term-settings.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 105862a..4c9979f 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3178,7 +3178,7 @@ wait_for_inferior (void) > static void > reinstall_readline_callback_handler_cleanup (void *arg) > { > - if (async_command_editing_p && !sync_execution) > + if (interpreter_async && async_command_editing_p && !sync_execution) > gdb_rl_callback_handler_reinstall (); > } > IWBN if the subtleties going on here were explained in the code. Maybe something like /* Don't call gdb_rl_callback_handler_reinstall if !interpreter_async: It will prep the terminal, readline-style (raw, noecho), which is not what we want if we're not using readline (e.g., --batch). */ I'm sure that can be improved on, but the point is this stuff is obviously subtle and readers should at least be given a bit of assistance (I'm all for more full explanations residing in the commit log). Another way to go, and while this is a separate thread, this seems like a good time to (re)start it, would be to try to encapsulate this a bit more. I'm not saying to do this now, obviously; we want to get a fix in for the release. But long term, it seems like there should be a layer between infrun and "readline", and it's this layer's job to DTRT instead of infrun checking no less than three global state variables to decide to install the "real" readline callback handler. I say "real" because gdb has another readline: gdb_readline2, another reason for more encapsulation.