[patch] Dummy first call to gdb_has_a_terminal()

[patch] Dummy first call to gdb_has_a_terminal()

[Balazs Kezes]

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.

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 ();
+
    /* Create a default uiout builder for the TUI.  */
    tui_out = tui_out_new (gdb_stdout);
    interp_add (interp_new (INTERP_TUI, NULL, tui_out, &procs));

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

[Balazs Kezes]

I've forgotten to add an include file to the patch, so here's the revised patch:

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 09:07:21 -0000
***************
*** 30,35 ****
--- 30,36 ----
  #include "tui/tui.h"
  #include "tui/tui-io.h"
  #include "exceptions.h"
+ #include "terminal.h"

  /* Set to 1 when the TUI mode must be activated when we first start
     gdb.  */
*************** _initialize_tui_interp (void)
*** 224,229 ****
--- 225,233 ----
      tui_command_loop,
    };

+   /* Dummy first call to save sane terminal settings. */
+   (void) gdb_has_a_terminal ();
+
    /* Create a default uiout builder for the TUI.  */
    tui_out = tui_out_new (gdb_stdout);
    interp_add (interp_new (INTERP_TUI, NULL, tui_out, &procs));

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

[Pedro Alves]

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

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

[Balazs Kezes]

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
>

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

[Balazs Kezes]

> It works for me.

Sorry scratch that, it doesn't work the other way (gdb->gdbtui).
I'll look into this when I have a little more time and not already
late to work. :)

--
Balazs

On 7/28/10, Balazs Kezes <rlblaster@gmail.com> wrote:
> 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
>>
>

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

[Balazs Kezes]

Okay, now I checked this more thoroughly. initalize_all_files() in init.c
calls tui initialization functions which mess up the terminal and call
gdb_has_a_terminal() afterwards. So it must be called before this function.

The other way (start gdbtui /bin/echo, switch to gdb, run) seems to work
after the gdb_has_a_terminal is added to the initialize_stdin_serial().

So here is my revised patch:

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
       initialize_all_files because it assumes it has been already called.


Index: inflow.c
===================================================================
RCS file: /cvs/src/src/gdb/inflow.c,v
retrieving revision 1.59
diff -c -p -r1.59 inflow.c
*** inflow.c	14 May 2010 21:25:51 -0000	1.59
--- inflow.c	28 Jul 2010 21:09:15 -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 21:09:15 -0000
*************** gdb_init (char *argv0)
*** 1613,1618 ****
--- 1613,1619 ----
    init_cmd_lists ();		/* This needs to be done first */
    initialize_targets ();	/* Setup target_terminal macros for utils.c */
    initialize_utils ();		/* Make errors and warnings possible */
+   initialize_stdin_serial ();	/* Make sure interpreters can init properly */
    initialize_all_files ();
    /* This creates the current_program_space.  Do this after all the
       _initialize_foo routines have had a chance to install their
*************** gdb_init (char *argv0)
*** 1625,1632 ****
    init_cli_cmds();
    init_main ();			/* But that omits this file!  Do it now */

-   initialize_stdin_serial ();
-
    async_init_signals ();

    /* We need a default language for parsing expressions, so simple things like
--- 1626,1631 ----

--
Balazs