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

[Balazs Kezes <rlblaster at gmail dot com>]

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
>