From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25699 invoked by alias); 28 Jul 2010 07:23:24 -0000 Received: (qmail 25664 invoked by uid 22791); 28 Jul 2010 07:23:21 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,TW_DB X-Spam-Check-By: sourceware.org Received: from mail-bw0-f41.google.com (HELO mail-bw0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 28 Jul 2010 07:23:13 +0000 Received: by bwz9 with SMTP id 9so4560849bwz.0 for ; Wed, 28 Jul 2010 00:23:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.204.128.130 with SMTP id k2mr5381219bks.159.1280301789348; Wed, 28 Jul 2010 00:23:09 -0700 (PDT) Received: by 10.204.18.144 with HTTP; Wed, 28 Jul 2010 00:23:09 -0700 (PDT) In-Reply-To: <201007201552.03274.pedro@codesourcery.com> References: <201007201552.03274.pedro@codesourcery.com> Date: Wed, 28 Jul 2010 07:23:00 -0000 Message-ID: Subject: Re: [patch] Dummy first call to gdb_has_a_terminal() From: Balazs Kezes To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 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 X-SW-Source: 2010-07/txt/msg00467.txt.bz2 Here is the new patch based on your observations. It works for me. -- Balazs Changelog: * inflow.c (initialize_stdin_serial): Added gdb_has_a_terminal to actually save all the tty settings. * top.c (gdb_init): Move initialize_stdin_serial before init_main because deep down it has some dependencies on initialize_stdin_serial. diff -c -p -r1.59 inflow.c *** inflow.c 14 May 2010 21:25:51 -0000 1.59 --- inflow.c 28 Jul 2010 07:04:43 -0000 *************** void *** 843,848 **** --- 843,849 ---- initialize_stdin_serial (void) { stdin_serial = serial_fdopen (0); + (void) gdb_has_a_terminal (); } void Index: top.c =================================================================== RCS file: /cvs/src/src/gdb/top.c,v retrieving revision 1.181.2.1 diff -c -p -r1.181.2.1 top.c *** top.c 27 Jul 2010 19:13:11 -0000 1.181.2.1 --- top.c 28 Jul 2010 07:04:44 -0000 *************** gdb_init (char *argv0) *** 1623,1631 **** initialize_inferiors (); initialize_current_architecture (); init_cli_cmds(); - init_main (); /* But that omits this file! Do it now */ - initialize_stdin_serial (); async_init_signals (); --- 1623,1630 ---- initialize_inferiors (); initialize_current_architecture (); init_cli_cmds(); initialize_stdin_serial (); + init_main (); /* But that omits this file! Do it now */ async_init_signals (); On 7/20/10, Pedro Alves wrote: > On Sunday 18 July 2010 01:30:40, Balazs Kezes wrote: >> Hi, >> >> I've noticed a weird behaviour with TUI at home and at work. It appears >> when I >> switch to the other mode (to command line from gdbtui or to tui from gdb). >> It >> does fix itself when I manage to switch back, but sometimes after >> executing a >> command it messes up readline so I can't switch back easily. >> >> For example: >> >> gdbtui /bin/echo >> ^x^a >> run >> >> The input is messed up now on my computer. >> >> I think bug gdb/9294 is exactly this. >> >> I've tracked it down to gdb_has_a_terminal(). Deep in tui_enable() this >> function this called. At this time the terminal has a messed up state (for >> example echo is disabled) which is fine. But it turns out this is the >> first >> call to this function and therefore it saves the current terminal settings >> which will be used to restore the terminal before displaying the prompt >> after >> executing a command. Even though it works for the current mode, it doesn't >> for the other. A neutral mode (the state when gdb starts up) seems to work >> for >> both modes. >> >> The fix is to have a dummy first call somewhere where the terminal is >> still in >> sane state. > > Thanks for investigating this. This bug annoys me too. > >> >> Cheers, >> Balazs >> >> Index: tui-interp.c >> =================================================================== >> RCS file: /cvs/src/src/gdb/tui/tui-interp.c,v >> retrieving revision 1.27 >> diff -c -p -r1.27 tui-interp.c >> *** tui-interp.c 17 May 2010 22:21:43 -0000 1.27 >> --- tui-interp.c 18 Jul 2010 00:29:25 -0000 >> *************** _initialize_tui_interp (void) >> *** 224,229 **** >> --- 224,232 ---- >> tui_command_loop, >> }; >> >> + /* Dummy first call to save sane terminal settings. */ >> + (void) gdb_has_a_terminal (); >> + > > Unfortunately, this is not a good place for this call. See the > comment in inflow.c: > > /* Does GDB have a terminal (on stdin)? */ > int > gdb_has_a_terminal (void) > { > switch (gdb_has_a_terminal_flag) > { > case yes: > return 1; > case no: > return 0; > case have_not_checked: > /* Get all the current tty settings (including whether we have a > tty at all!). Can't do this in _initialize_inflow because > serial_fdopen() won't work until the serial_ops_list is > initialized. */ > > Two issues here: > > First, you can't rely on the order the _initialize_XXX > functions are called. In fact, on my gdb build, all the > serial_add_interface calls have already happened by the time > _initialize_inflow is called (meaning serial_ops_list is already > fully initialized), so per-chance, moving your call to > _initialize_inflow _would_ work for me. But there's no garantee it > will keep working, and neither does adding the call > in _initialize_tui_interp. > > Second, your new call happens before stdin_serial has been > initialized by initialize_stdin_serial, called from gdb_init: > > /* Get all the current tty settings (including whether we have a > tty at all!). We can't do this in _initialize_inflow because > serial_fdopen() won't work until the serial_ops_list is > initialized, but we don't want to do it lazily either, so > that we can guarantee stdin_serial is opened if there is > a terminal. */ > void > initialize_stdin_serial (void) > { > stdin_serial = serial_fdopen (0); > } > > That means that gdb_has_a_terminal would always return false > with your patch, given the (stdin_serial != NULL) check it > has: > > /* Does GDB have a terminal (on stdin)? */ > int > gdb_has_a_terminal (void) > { > switch (gdb_has_a_terminal_flag) > { > case yes: > return 1; > case no: > return 0; > case have_not_checked: > ... > gdb_has_a_terminal_flag = no; > if (stdin_serial != NULL) > { > our_terminal_info.ttystate = serial_get_tty_state (stdin_serial); > > if (our_terminal_info.ttystate != NULL) > { > gdb_has_a_terminal_flag = yes; > > > It would seem better to add the new call in initialize_stdin_serial. > In fact the "(including whether we have a tty at all!)" comment > makes it sound like that was also the intent. I don't know the > history of this function. > > Does that work? We may need to tweak the orders in gdb_init as well, > not sure. > > -- > Pedro Alves >