Re: [pushed] PR tui/16138, PR tui/17519, and misc failures to initialize the terminal

[Simon Marchi <simon dot marchi at ericsson dot com>]

Hi Pedro,

(see below)

On 2014-10-29 10:36 AM, Pedro Alves wrote:
> PR tui/16138 is about failure to initialize curses resulting in GDB
> exiting instead of throwing an error.  E.g.:
> 
>  $ TERM=foo gdb
>  (gdb) layout asm
>  Error opening terminal: foo.
>  $
> 
> The problem is that we're calling initscr to initialize the screen.
> As mentioned in
> http://pubs.opengroup.org/onlinepubs/7908799/xcurses/initscr.html:
> 
>  If errors occur, initscr() writes an appropriate error message to
>  standard error and exits.
>                     ^^^^^
> 
> Instead, we should use newterm:
> 
>  "A program that needs an indication of error conditions, so it can
>  continue to run in a line-oriented mode if the terminal cannot support
>  a screen-oriented program, would also use this function."
> 
> After the patch:
> 
>  $ TERM=foo gdb -q -nx
>  (gdb) layout asm
>  Cannot enable the TUI: error opening terminal [TERM=foo]
>  (gdb)
> 
> And then PR tui/17519 is about GDB not validating whether the terminal
> has the necessary capabilities when enabling the TUI.  If one tries to
> enable the TUI with TERM=dumb (and e.g., from a shell within emacs),
> GDB ends up with a clear screen, the cursor is placed at the
> bottom/right corner of the screen, there's no prompt, typing shows no
> echo, and there's no indication of what's going on.  c-x,a gets you
> out of the TUI, but it's completely non-obvious.
> 
> After the patch, we get:
> 
>  $ TERM=dumb gdb -q -nx
>  (gdb) layout asm
>  Cannot enable the TUI: terminal doesn't support cursor addressing [TERM=dumb]
>  (gdb)
> 
> While at it, I've moved all the tui_allowed_p validation to
> tui_enable, and expanded the error messages.  Previously we'd get:
> 
>  $ gdb -q -nx -i=mi
>  (gdb)
>  layout asm
>  &"layout asm\n"
>  &"TUI mode not allowed\n"
>  ^error,msg="TUI mode not allowed"
> 
> and:
> 
>  $ gdb -q -nx -ex "layout asm" > foo
>  TUI mode not allowed
> 
> While now we get:
> 
>  $ gdb -q -nx -i=mi
>  (gdb)
>  layout asm
>  &"layout asm\n"
>  &"Cannot enable the TUI when the interpreter is 'mi'\n"
>  ^error,msg="Cannot enable the TUI when the interpreter is 'mi'"
>  (gdb)
> 
> and:
> 
>  $ gdb -q -nx -ex "layout asm" > foo
>  Cannot enable the TUI when output is not a terminal
> 
> Tested on x86_64 Fedora 20.
> 
> gdb/
> 2014-10-29  Pedro Alves  <palves@redhat.com>
> 
> 	PR tui/16138
> 	PR tui/17519
> 	* tui/tui-interp.c (tui_is_toplevel): Delete global.
> 	(tui_allowed_p): Delete function.
> 	* tui/tui.c: Include "interps.h".
> 	(tui_enable): Don't use tui_allowed_p.  Error out here with
> 	detailed error messages if the TUI is the top level interpreter,
> 	or if output is not a terminal.  Use newterm instead of initscr,
> 	and error out if initializing the terminal fails.  Also error out if
> 	the terminal doesn't support cursor addressing.
> 	* tui/tui.h (tui_allowed_p): Delete declaration.
> ---
>  gdb/tui/tui-interp.c | 17 -----------------
>  gdb/tui/tui.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  gdb/tui/tui.h        |  4 ----
>  3 files changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index 24cb5a3..b377db2 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -51,9 +51,6 @@ tui_exit (void)
>    tui_disable ();
>  }
>  
> -/* True if TUI is the top-level interpreter.  */
> -static int tui_is_toplevel = 0;
> -
>  /* Observers for several run control events.  If the interpreter is
>     quiet (i.e., another interpreter is being run with
>     interpreter-exec), print nothing.  */
> @@ -126,8 +123,6 @@ tui_on_command_error (void)
>  static void *
>  tui_init (struct interp *self, int top_level)
>  {
> -  tui_is_toplevel = top_level;
> -
>    /* Install exit handler to leave the screen in a good shape.  */
>    atexit (tui_exit);
>  
> @@ -150,18 +145,6 @@ tui_init (struct interp *self, int top_level)
>    return NULL;
>  }
>  
> -/* True if enabling the TUI is allowed.  Example, if the top level
> -   interpreter is MI, enabling curses will certainly lose.  */
> -
> -int
> -tui_allowed_p (void)
> -{
> -  /* Only if TUI is the top level interpreter.  Also don't try to
> -     setup curses (and print funny control characters) if we're not
> -     outputting to a terminal.  */
> -  return tui_is_toplevel && ui_file_isatty (gdb_stdout);
> -}
> -
>  static int
>  tui_resume (void *data)
>  {
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index a02c855..ca66ccd 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -48,6 +48,7 @@
>  #include <setjmp.h>
>  
>  #include "gdb_curses.h"
> +#include "interps.h"
>  
>  /* This redefines CTRL if it is not already defined, so it must come
>     after terminal state releated include files like <term.h> and
> @@ -361,6 +362,20 @@ tui_initialize_readline (void)
>    rl_bind_key_in_map ('s', tui_rl_next_keymap, tui_ctlx_keymap);
>  }
>  
> +/* Return the TERM variable from the environment, or "<unset>"
> +   if not set.  */
> +
> +static const char *
> +gdb_getenv_term (void)
> +{
> +  const char *term;
> +
> +  term = getenv ("TERM");
> +  if (term != NULL)
> +    return term;
> +  return "<unset>";
> +}
> +
>  /* Enter in the tui mode (curses).
>     When in normal mode, it installs the tui hooks in gdb, redirects
>     the gdb output, configures the readline to work in tui mode.
> @@ -368,8 +383,7 @@ tui_initialize_readline (void)
>  void
>  tui_enable (void)
>  {
> -  if (!tui_allowed_p ())
> -    error (_("TUI mode not allowed"));
> +  struct interp *interp;
>  
>    if (tui_active)
>      return;
> @@ -380,9 +394,40 @@ tui_enable (void)
>    if (tui_finish_init)
>      {
>        WINDOW *w;
> +      SCREEN *s;
> +      const char *cap;
> +      const char *interp;
> +
> +      /* If the top level interpreter is not the console/tui (e.g.,
> +	 MI), enabling curses will certainly lose.  */
> +      interp = interp_name (top_level_interpreter ());
> +      if (strcmp (interp, INTERP_TUI) != 0)
> +	error (_("Cannot enable the TUI when the interpreter is '%s'"), interp);
> +
> +      /* Don't try to setup curses (and print funny control
> +	 characters) if we're not outputting to a terminal.  */
> +      if (!ui_file_isatty (gdb_stdout))
> +	error (_("Cannot enable the TUI when output is not a terminal"));
> +
> +      s = newterm (NULL, NULL, NULL);

I just noticed this causes a segfault on SLED11, which is somewhat old. "zypper info libncurses5" gives version "5.6-90.55".

Old ncurses' newterm doesn't like it when we pass NULL for ofp and ifp (the last two parameters). Passing NULL to name (the first parameter) is ok though.

I just checked the difference in the source, and new ncurses' newterm has this:

FILE *_ofp = ofp ? ofp : stdout;
FILE *_ifp = ifp ? ifp : stdin;

while the old one does not, which explains the difference in behavior. I tried to change the line for:

s = newterm (NULL, stdout, stdin);

and everything seems to work as usual.

Do you want me to submit a patch, or are you going to take care of it yourself?