Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior

[Patrick Palka <patrick@parcs.ath.cx>]

On Sat, Nov 22, 2014 at 3:39 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> Currently when we start an inferior we have the inferior inherit our
> terminal state.  Under TUI, our terminal is highly modified by ncurses
> and readline.  So when starting an inferior under TUI, the inferior will
> have a highly modified terminal state which will interfere with standard
> I/O. For example,
>
> $ gdb gdb
> (gdb) break main
> (gdb) run
> (gdb) print puts ("a\nb")
> a
> b
> $1 = 4
> (gdb) [enter TUI mode]
> (gdb) run
> (gdb) [exit TUI mode]
> (gdb) print puts ("a\nb")
> a
>  b
>   $2 = 4
> (gdb) print puts ("a\r\nb\r")
> a
> b
> $3 = 6
>
> As you can see, when we start the inferior under the regular interface,
> puts() prints the text properly.  But when we start the inferior under
> TUI, puts() does not print the text properly.  This is because when we
> start the inferior under TUI it inherits our current terminal state
> which has been modified by ncurses to, among other things, require an
> explicit \r\n to print a new line. As a result the inferior performs
> standard I/O in an unexpected way.
>
> Because of this discrepancy, it doesn't seem like a good idea to have
> the inferior inherit our _current_ terminal state for it may have been
> modified by readline and/or ncurses.  Instead, we should have the
> inferior inherit a pristine snapshot of our terminal state taken before
> readline or ncurses have had a chance to alter it.  This enables the
> inferior to run in a more accurate way, more closely mimicking its
> behavior had the program run standalone.  And it fixes the above
> mentioned issue.
>
> I wonder, does this change make sense?  What do others think?
>
> Tested on x86_64-unknown-linux-gnu.
>
>         * terminal.h (set_initial_inferior_ttystate): Declare.
>         * inflow.c (initial_inferior_ttystate): New static variable.
>         (set_initial_inferior_ttystate): New setter.
>         (child_terminal_init_with_pgrp): Copy initial_inferior_ttystate
>         instead of our current terminal state.
>         * top.c (gdb_init): Call set_initial_inferior_ttystate.
> ---
>  gdb/inflow.c   | 13 ++++++++++++-
>  gdb/terminal.h |  2 ++
>  gdb/top.c      |  4 ++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index 8902174..7b432ad 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -79,6 +79,10 @@ struct terminal_info
>     unimportant.  */
>  static struct terminal_info our_terminal_info;
>
> +/* The initial tty state given to each new inferior.  It is a snapshot of our
> +   own tty state taken during initialization of GDB.  */
> +static serial_ttystate initial_inferior_ttystate;
> +
>  static struct terminal_info *get_inflow_inferior_data (struct inferior *);
>
>  #ifdef PROCESS_GROUP_TYPE
> @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty,
>      fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value);
>  }
>
> +/* Set the initial tty state that is to be inherited by new inferiors.  */
> +void
> +set_initial_inferior_ttystate (void)
> +{
> +  initial_inferior_ttystate = serial_get_tty_state (stdin_serial);
> +}
> +
>  /* Does GDB have a terminal (on stdin)?  */
>  int
>  gdb_has_a_terminal (void)
> @@ -227,7 +238,7 @@ child_terminal_init_with_pgrp (int pgrp)
>      {
>        xfree (tinfo->ttystate);
>        tinfo->ttystate = serial_copy_tty_state (stdin_serial,
> -                                              our_terminal_info.ttystate);
> +                                              initial_inferior_ttystate);
>
>        /* Make sure that next time we call terminal_inferior (which will be
>           before the program runs, as it needs to be), we install the new
> diff --git a/gdb/terminal.h b/gdb/terminal.h
> index 433aa7d..186bce2 100644
> --- a/gdb/terminal.h
> +++ b/gdb/terminal.h
> @@ -103,6 +103,8 @@ extern int gdb_has_a_terminal (void);
>
>  extern void gdb_save_tty_state (void);
>
> +extern void set_initial_inferior_ttystate (void);
> +
>  /* Set the process group of the caller to its own pid, or do nothing
>     if we lack job control.  */
>  extern int gdb_setpgid (void);
> diff --git a/gdb/top.c b/gdb/top.c
> index 83d858a..c4b5c2c 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1886,6 +1886,10 @@ gdb_init (char *argv0)
>
>    initialize_stdin_serial ();
>
> +  /* Take a snapshot of our tty state before readline/ncurses have had a chance
> +     to alter it.  */
> +  set_initial_inferior_ttystate ();
> +
>    async_init_signals ();
>
>    /* We need a default language for parsing expressions, so simple
> --
> 2.2.0.rc1.23.gf570943
>

Ping.