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

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

[Pedro Alves]

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);
+      if (s == NULL)
+	{
+	  error (_("Cannot enable the TUI: error opening terminal [TERM=%s]"),
+		 gdb_getenv_term ());
+	}
+      w = stdscr;
+
+      /* Check required terminal capabilities.  */
+      cap = tigetstr ("cup");
+      if (cap == NULL || cap == (char *) -1 || *cap == '\0')
+	{
+	  endwin ();
+	  delscreen (s);
+	  error (_("Cannot enable the TUI: "
+		   "terminal doesn't support cursor addressing [TERM=%s]"),
+		 gdb_getenv_term ());
+	}
 
-      w = initscr ();
-  
       cbreak ();
       noecho ();
       /* timeout (1); */
diff --git a/gdb/tui/tui.h b/gdb/tui/tui.h
index 9afadb2..b1dddc0 100644
--- a/gdb/tui/tui.h
+++ b/gdb/tui/tui.h
@@ -64,10 +64,6 @@ extern int tui_get_command_dimension (unsigned int *width,
    key shortcut.  */
 extern void tui_initialize_readline (void);
 
-/* True if enabling the TUI is allowed.  Example, if the top level
-   interpreter is MI, enabling curses will certainly lose.  */
-extern int tui_allowed_p (void);
-
 /* Enter in the tui mode (curses).  */
 extern void tui_enable (void);
 
-- 
1.9.3

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

[Simon Marchi]

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?

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

[Pedro Alves]

Hi Simon,

On 11/03/2014 09:37 PM, Simon Marchi wrote:

> 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?

Sounds like an obvious fix then.  If you could submit/push a patch, I'd
appreciate it.  Just be sure to include the info above in the commit
log.

Thanks,
Pedro Alves