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

Pedro Alves
2010-07-20T14:52:19Z

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