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

[Pedro Alves <pedro at codesourcery dot com>]

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