Re: [patch] Dummy first call to gdb_has_a_terminal()

Balazs Kezes
2010-07-28T07:23:24Z

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 <pedro@codesourcery.com> 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
>