From patchwork Wed Jan 7 21:43:58 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Palka X-Patchwork-Id: 4555 Received: (qmail 8021 invoked by alias); 7 Jan 2015 21:44:16 -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 8009 invoked by uid 89); 7 Jan 2015 21:44:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-qg0-f49.google.com Received: from mail-qg0-f49.google.com (HELO mail-qg0-f49.google.com) (209.85.192.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 07 Jan 2015 21:44:13 +0000 Received: by mail-qg0-f49.google.com with SMTP id f51so1497150qge.22 for ; Wed, 07 Jan 2015 13:44:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version:content-type; bh=3d8VaUUBBMs3+7ggYMt6mY3uWsdn4aIKE6CFUSEPnXM=; b=ju3gXvMzNSGangRrIQvYTVevHkbujEoAOKd0Dj0K4uwcZY+sMuHNu+CweHLVnwvu/7 pF/FYdgvJt/00e+c5n1Qyg+ASIF5SYAkspZrsJmTp0XUNYTzsSbbxzrPrxLeyVRAmyqg hRqYjW9P76vKP2RoTRUv/RVfj3JkckVJC8JD1urmgjxRutGFwTht/bICAnJadyBVTM4p P0V5wDVY8hZyPEpFyJh/Jt0h3mwsnXXO4NMxO3TK11aCLTuSS+CC0yxKgc+4HapwN78u Tw50D6XqytBVUZCjM4Fbyo26amkXrJ5Gy7lwFxm1Gj/X79glEUmbpAdjCuyrjzE0yw6P KcLw== X-Gm-Message-State: ALoCoQk9hpYeHuHt0BanK4aKWi4j7t20CMkiv9QbMm/LUyE9LxTyM5VZLzdBdDBSdr2ZHI/eEb/g X-Received: by 10.224.14.66 with SMTP id f2mr8606997qaa.81.1420667051464; Wed, 07 Jan 2015 13:44:11 -0800 (PST) Received: from [192.168.1.130] (ool-4353ac94.dyn.optonline.net. [67.83.172.148]) by mx.google.com with ESMTPSA id 77sm2233367qgx.43.2015.01.07.13.44.10 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 07 Jan 2015 13:44:10 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Wed, 7 Jan 2015 16:43:58 -0500 (EST) To: Pedro Alves cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior In-Reply-To: <54AD3716.7070209@redhat.com> Message-ID: References: <1416688748-20448-1-git-send-email-patrick@parcs.ath.cx> <54AD3716.7070209@redhat.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 On Wed, 7 Jan 2015, Pedro Alves wrote: > On 11/22/2014 08:39 PM, Patrick Palka wrote: >> Currently when we start an inferior we have the inferior inherit our >> terminal state. Under TUI, our terminal is highly modified by ncurses >> and readline. So when starting an inferior under TUI, the inferior will >> have a highly modified terminal state which will interfere with standard >> I/O. For example, >> >> $ gdb gdb >> (gdb) break main >> (gdb) run >> (gdb) print puts ("a\nb") >> a >> b >> $1 = 4 >> (gdb) [enter TUI mode] >> (gdb) run >> (gdb) [exit TUI mode] >> (gdb) print puts ("a\nb") >> a >> b >> $2 = 4 >> (gdb) print puts ("a\r\nb\r") >> a >> b >> $3 = 6 >> >> As you can see, when we start the inferior under the regular interface, >> puts() prints the text properly. But when we start the inferior under >> TUI, puts() does not print the text properly. This is because when we >> start the inferior under TUI it inherits our current terminal state >> which has been modified by ncurses to, among other things, require an >> explicit \r\n to print a new line. As a result the inferior performs >> standard I/O in an unexpected way. >> >> Because of this discrepancy, it doesn't seem like a good idea to have >> the inferior inherit our _current_ terminal state for it may have been >> modified by readline and/or ncurses. Instead, we should have the >> inferior inherit a pristine snapshot of our terminal state taken before >> readline or ncurses have had a chance to alter it. This enables the >> inferior to run in a more accurate way, more closely mimicking its >> behavior had the program run standalone. And it fixes the above >> mentioned issue. >> >> I wonder, does this change make sense? What do others think? > > Thanks. This makes sense to me. > >> >> +/* The initial tty state given to each new inferior. It is a snapshot of our >> + own tty state taken during initialization of GDB. */ >> +static serial_ttystate initial_inferior_ttystate; >> + >> static struct terminal_info *get_inflow_inferior_data (struct inferior *); >> >> #ifdef PROCESS_GROUP_TYPE >> @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, >> fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); >> } >> >> +/* Set the initial tty state that is to be inherited by new inferiors. */ >> +void >> +set_initial_inferior_ttystate (void) >> +{ >> + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); >> +} > > "initial inferior" reminded me of the initial inferior that > GDB always creates. E.g., in main.c: > > /* Now that gdb_init has created the initial inferior, we're in > position to set args for that inferior. */ > > > Could you rename the function and the variable? Like: > > set_initial_gdb_ttystate (void) > { > initial_gdb_ttystate = serial_get_tty_state (stdin_serial); > } > > ... > > /* Snapshot of our own tty state taken during initialization > of GDB. This is used as tty state given to each new inferior. */ I noticed that I forgot to accordingly update the comment for initial_gdb_ttystate as you suggested. I have committed this trivial patch under the "obvious" rule: -- >8 -- Subject: [PATCH] Trivially tweak the comment documenting initial_gdb_ttystate gdb/ChangeLog: * inflow.c (initial_gdb_ttystate): Tweak comment. --- gdb/ChangeLog | 4 ++++ gdb/inflow.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e9b0377..b188988 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2015-01-07 Patrick Palka + + * inflow.c (initial_gdb_ttystate): Tweak comment. + 2015-01-07 Joel Brobecker * inflow.c (set_initial_gdb_ttystate): Add empty line after diff --git a/gdb/inflow.c b/gdb/inflow.c index 4c81a68..1456fd8 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -79,8 +79,8 @@ struct terminal_info unimportant. */ static struct terminal_info our_terminal_info; -/* The initial tty state given to each new inferior. It is a snapshot of our - own tty state taken during initialization of GDB. */ +/* Snapshot of our own tty state taken during initialization of GDB. + This is used as the initial tty state given to each new inferior. */ static serial_ttystate initial_gdb_ttystate; static struct terminal_info *get_inflow_inferior_data (struct inferior *);