gdb: return true in TuiWindow.is_valid only if TUI is enabled

If the user implements a TUI window in Python, and this window
responds to GDB events and then redraws its window contents then there
is currently an edge case which can lead to problems.

The Python API documentation suggests that calling methods like erase
or write on a TUI window (from Python code) will raise an exception if
the window is not valid.

And the description for is_valid says:

  This method returns True when this window is valid. When the user
  changes the TUI layout, windows no longer visible in the new layout
  will be destroyed. At this point, the gdb.TuiWindow will no longer
  be valid, and methods (and attributes) other than is_valid will
  throw an exception.

From this I, as a user, would expect that if I did 'tui disable' to
switch back to CLI mode, then the window would no longer be valid.
However, this is not the case.

When the TUI is disabled the windows in the TUI are not deleted, they
are simply hidden.  As such, currently, the is_valid method continues
to return true.

This means that if the users Python code does something like:

  def event_handler (e):
    global tui_window_object
    if tui_window_object->is_valid ():
      tui_window_object->erase ()
      tui_window_object->write ("Hello World")
  gdb.events.stop.connect (event_handler)

Then when a stop event arrives GDB will try to draw the TUI window,
even when the TUI is disabled.

This exposes two bugs.  First, is_valid should be returning false in
this case, second, if the user forgot to add the is_valid call, then I
believe the erase and write calls should be throwing an
exception (when the TUI is disabled).

The solution to both of these issues is I think bound together, as it
depends on having a working 'is_valid' check.

There's a rogue assert added into tui-layout.c as part of this
commit.  While working on this commit I managed to break GDB such that
TUI_CMD_WIN was nullptr, this was causing GDB to abort.  I'm leaving
the assert in as it might help people catch issues in the future.

This patch is inspired by the work done here:

  https://sourceware.org/pipermail/gdb-patches/2020-December/174338.html

gdb/ChangeLog:

	* python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
	function.
	(REQUIRE_WINDOW): Call is_valid member function.
	(REQUIRE_WINDOW_FOR_SETTER): New define.
	(gdbpy_tui_is_valid): Call is_valid member function.
	(gdbpy_tui_set_title): Call REQUIRE_WINDOW_FOR_SETTER instead.
	* tui/tui-data.h (struct tui_win_info) <is_visible>: Check
	tui_active too.
	* tui/tui-layout.c (tui_apply_current_layout): Add an assert.
	* tui/tui.c (tui_enable): Move setting of tui_active earlier in
	the function.

gdb/doc/ChangeLog:

	* python.texinfo (TUI Windows In Python): Extend description of
	TuiWindow.is_valid.

gdb/testsuite/ChangeLog:

	* gdb.python/tui-window-disabled.c: New file.
	* gdb.python/tui-window-disabled.exp: New file.
	* gdb.python/tui-window-disabled.py: New file.

11 files changed, 394 insertions, 15 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b7b4909..912ca6b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,19 @@
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
+	function.
+	(REQUIRE_WINDOW): Call is_valid member function.
+	(REQUIRE_WINDOW_FOR_SETTER): New define.
+	(gdbpy_tui_is_valid): Call is_valid member function.
+	(gdbpy_tui_set_title): Call REQUIRE_WINDOW_FOR_SETTER instead.
+	* tui/tui-data.h (struct tui_win_info) <is_visible>: Check
+	tui_active too.
+	* tui/tui-layout.c (tui_apply_current_layout): Add an assert.
+	* tui/tui.c (tui_enable): Move setting of tui_active earlier in
+	the function.
+
+2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* python/py-tui.c (gdbpy_tui_set_title): Check that the new value
 	for the title is not nullptr.
 
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 7b21966..bc84fdc 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* python.texinfo (TUI Windows In Python): Extend description of
+	TuiWindow.is_valid.
+
 2021-02-02  Lancelot SIX  <lsix@lancelotsix.com>
 
 	* gdb.texinfo (Inferiors Connections and Programs): Document the
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 1a5209a..ba3d2f9 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5848,6 +5848,11 @@ user changes the TUI layout, windows no longer visible in the new
 layout will be destroyed.  At this point, the @code{gdb.TuiWindow}
 will no longer be valid, and methods (and attributes) other than
 @code{is_valid} will throw an exception.
+
+When the TUI is disabled using @code{tui disable} (@pxref{TUI
+Commands,,tui disable}) the window is hidden rather than destroyed,
+but @code{is_valid} will still return @code{False} and other methods
+(and attributes) will still throw an exception.
 @end defun
 
 @defvar TuiWindow.width
diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 73b73f3..72e9c0d 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -47,6 +47,9 @@ struct gdbpy_tui_window
 
   /* The TUI window, or nullptr if the window has been deleted.  */
   tui_py_window *window;
+
+  /* Return true if this object is valid.  */
+  bool is_valid () const;
 };
 
 extern PyTypeObject gdbpy_tui_window_object_type
@@ -137,6 +140,14 @@ private:
   gdbpy_ref<gdbpy_tui_window> m_wrapper;
 };
 
+/* See gdbpy_tui_window declaration above.  */
+
+bool
+gdbpy_tui_window::is_valid () const
+{
+  return window != nullptr && tui_active;
+}
+
 tui_py_window::~tui_py_window ()
 {
   gdbpy_enter enter_py (get_current_arch (), current_language);
@@ -344,11 +355,23 @@ gdbpy_register_tui_window (PyObject *self, PyObject *args, PyObject *kw)
 
 #define REQUIRE_WINDOW(Window)					\
     do {							\
-      if ((Window)->window == nullptr)				\
+      if (!(Window)->is_valid ())				\
 	return PyErr_Format (PyExc_RuntimeError,		\
 			     _("TUI window is invalid."));	\
     } while (0)
 
+/* Require that "Window" be a valid window.  */
+
+#define REQUIRE_WINDOW_FOR_SETTER(Window)			\
+    do {							\
+      if (!(Window)->is_valid ())				\
+	{							\
+	  PyErr_Format (PyExc_RuntimeError,			\
+			_("TUI window is invalid."));		\
+	  return -1;						\
+	}							\
+    } while (0)
+
 /* Python function which checks the validity of a TUI window
    object.  */
 static PyObject *
@@ -356,7 +379,7 @@ gdbpy_tui_is_valid (PyObject *self, PyObject *args)
 {
   gdbpy_tui_window *win = (gdbpy_tui_window *) self;
 
-  if (win->window != nullptr)
+  if (win->is_valid ())
     Py_RETURN_TRUE;
   Py_RETURN_FALSE;
 }
@@ -428,11 +451,7 @@ gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
 {
   gdbpy_tui_window *win = (gdbpy_tui_window *) self;
 
-  if (win->window == nullptr)
-    {
-      PyErr_Format (PyExc_RuntimeError, _("TUI window is invalid."));
-      return -1;
-    }
+  REQUIRE_WINDOW_FOR_SETTER (win);
 
   if (newvalue == nullptr)
     {
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index faee2c4..3e622ad 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,11 @@
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gdb.python/tui-window-disabled.c: New file.
+	* gdb.python/tui-window-disabled.exp: New file.
+	* gdb.python/tui-window-disabled.py: New file.
+
+2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.python/tui-window.exp: Add new tests.
 	* gdb.python/tui-window.py (TestWindow) <__init__>: Store
 	TestWindow object into global the_window.
diff --git a/gdb/testsuite/gdb.python/tui-window-disabled.c b/gdb/testsuite/gdb.python/tui-window-disabled.c
new file mode 100644
index 0000000..898c536
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.c
@@ -0,0 +1,43 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
+
+#include "../lib/attributes.h"
+
+volatile int val;
+
+void __attribute__((noinline)) ATTRIBUTE_NOCLONE
+func (int i)
+{
+  val = i;
+}
+
+int
+main ()
+{
+  func (0);
+  func (1);
+  func (2);
+  func (3);
+  func (4);
+  func (5);
+  func (6);
+  func (7);
+  func (8);
+  func (9);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/tui-window-disabled.exp b/gdb/testsuite/gdb.python/tui-window-disabled.exp
new file mode 100644
index 0000000..af1fa0c
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.exp
@@ -0,0 +1,189 @@
+# Copyright (C) 2021 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Create a TUI window in Python that responds to GDB event.  Each
+# event will trigger the TUI window to redraw itself.
+#
+# This test is checking how GDB behaves if the user first displays a
+# Python based tui window, and then does 'tui disable'.  At one point
+# it was possible that GDB would try to redraw the tui window even
+# though the tui should be disabled.
+
+load_lib gdb-python.exp
+tuiterm_env
+
+standard_testfile
+
+if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
+    return -1
+}
+
+# Copy the Python script to where the tests are being run.
+set remote_python_file [gdb_remote_download host \
+			    ${srcdir}/${subdir}/${testfile}.py]
+
+proc clean_restart_and_setup { prefix } {
+    global testfile
+    global remote_python_file
+
+    with_test_prefix $prefix {
+
+	Term::clean_restart 24 80 $testfile
+
+	# Skip all tests if Python scripting is not enabled.
+	if { [skip_python_tests] } { return 0 }
+
+	# Now source the python script.
+	gdb_test_no_output "source ${remote_python_file}" \
+	    "source ${testfile}.py"
+
+	# Create a new layout making use of our new event window.
+	gdb_test_no_output "tui new-layout test events 1 cmd 1"
+
+	# Disable source code highlighting.
+	gdb_test_no_output "set style sources off"
+
+	if {![runto_main]} {
+	    perror "test suppressed"
+	    return
+	}
+    }
+
+    return 1
+}
+
+# Run the test.  CLEANUP_PROPERLY is either true or false.  This is
+# used to set a flag in the Python code which controls whether the
+# Python TUI window cleans up properly or not.
+#
+# When the Python window does not cleanup properly then it retains a
+# cyclic reference to itself, this means that it is still possible for
+# the object to try and redraw itself even when the tui is disabled.
+proc run_test { cleanup_properly } {
+
+    if { ![clean_restart_and_setup "initial restart"] } {
+	unsupported "couldn't restart GDB"
+	return
+    }
+
+    if { $cleanup_properly } {
+	gdb_test_no_output "python cleanup_properly = True"
+    } else {
+	gdb_test_no_output "python cleanup_properly = False"
+    }
+
+    if {![Term::enter_tui]} {
+	unsupported "TUI not supported"
+	return
+    }
+
+    Term::command "layout test"
+
+    # Confirm that the events box is initially empty, then perform two
+    # actions that will add two events to the window.
+    Term::check_box_contents "no events yet" 0 0 80 16 ""
+    Term::command "next"
+    Term::check_box_contents "single stop event" 0 0 80 16 "stop"
+    Term::command "next"
+    Term::check_box_contents "two stop events" 0 0 80 16 \
+	"stop\[^\n\]+\nstop"
+
+    # Now disable the tui, we should end up back at a standard GDB prompt.
+    Term::command "tui disable"
+
+    # Do something just so we know that the CLI is working.
+    gdb_test "print 1 + 1 + 1" " = 3"
+
+    # Now perform an action that would trigger an event.  At one point
+    # there was a bug where the TUI window might try to redraw itself.
+    # This is why we use GDB_TEST_MULTIPLE here, so we can spot the tui
+    # window title and know that things have gone wrong.
+    gdb_test_multiple "next" "next at cli" {
+	-re -wrap "func \\(3\\);" {
+	    pass $gdb_test_name
+	}
+
+	-re "This Is The Event Window" {
+	    fail $gdb_test_name
+	}
+    }
+
+    # Do something just so we know that the CLI is still working.  This
+    # also serves to drain the expect buffer if the above test failed.
+    gdb_test "print 2 + 2 + 2" " = 6"
+
+    # Now tell the Python code not to check the window is valid before
+    # calling rerended.  The result is the Python code will try to draw to
+    # the screen.  This should throw a Python exception.
+    gdb_test_no_output "python perform_valid_check = False"
+    set exception_pattern "\r\nPython Exception\[^\n\r\]+TUI window is invalid\[^\n\r\]+"
+    gdb_test_multiple "next" "next at cli, with an exception" {
+	-re -wrap "func \\(4\\);${exception_pattern}" {
+	    pass $gdb_test_name
+	}
+
+	-re "This Is The Event Window" {
+	    fail $gdb_test_name
+	}
+    }
+
+    # Do something just so we know that the CLI is still working.  This
+    # also serves to drain the expect buffer if the above test failed.
+    gdb_test "print 3 + 3 + 3" " = 9"
+
+    # Set 'update_title' to True.  The Python script will now try to set
+    # the window title when an event occurs (instead of trying to redraw
+    # the window). As the window is still not displayed this will again
+    # through an exception.
+    gdb_test_no_output "python update_title = True"
+    gdb_test_multiple "next" "next at cli, with an exception for setting the title" {
+	-re -wrap "func \\(5\\);${exception_pattern}" {
+	    pass $gdb_test_name
+	}
+
+	-re "This Is The Event Window" {
+	    fail $gdb_test_name
+	}
+    }
+
+    # We need to perform a restart here as the TUI library we use for
+    # testing doesn't seem to handle output in the command window
+    # correctly, and gets really upset as we approach the bottom of
+    # the screen.
+    #
+    # Restart GDB, enable tui mode, select the new layout.  Then
+    # disable tui and re-enable again.
+    if { ![clean_restart_and_setup "second restart"] } {
+	unsupported "couldn't restart GDB"
+	return
+    }
+
+    with_test_prefix "enter tui again" {
+	if {![Term::enter_tui]} {
+	    unsupported "TUI not supported"
+	    return
+	}
+    }
+
+    Term::command "layout test"
+    Term::command "tui disable"
+    send_gdb "tui enable\n"
+    Term::check_box "check for python window" 0 0 80 16
+}
+
+# Run the tests in both cleanup modes.
+foreach_with_prefix cleanup_properly { True False } {
+    run_test $cleanup_properly
+}
diff --git a/gdb/testsuite/gdb.python/tui-window-disabled.py b/gdb/testsuite/gdb.python/tui-window-disabled.py
new file mode 100644
index 0000000..0b3c076
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.py
@@ -0,0 +1,89 @@
+# Copyright (C) 2021 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# A TUI window implemented in Python that responds to, and displays,
+# stop and exit events.
+
+import gdb
+
+# When an event arrives we ask the window to redraw itself.  We should
+# only do this if the window is valid.  When this flag is true we
+# perform the is_valid check.  When this flag is false
+perform_valid_check = True
+update_title = False
+cleanup_properly = False
+
+# A global place into which we can write the window title.
+titles_at_the_close = {}
+
+class EventWindow:
+    def __init__ (self, win):
+        self._win = win
+        self._count = 0
+        win.title = "This Is The Event Window"
+        self._stop_listener = lambda e : self._event ('stop', e)
+        gdb.events.stop.connect (self._stop_listener)
+        self._exit_listener = lambda e : self._event ('exit', e)
+        gdb.events.exited.connect (self._exit_listener)
+        self._events = []
+
+        # Ensure we can erase and write to the window from the
+        # constructor, the window should be valid by this point.
+        self._win.erase ()
+        self._win.write ("Hello world...")
+
+    def close (self):
+        global cleanup_properly
+        global titles_at_the_close
+
+        # Ensure that window properties can be read within the close method.
+        titles_at_the_close[self._win.title] = dict (width=self._win.width,
+                                                     height=self._win.height)
+
+        # The following calls are pretty pointless, but this ensures
+        # that we can erase and write to a window from the close
+        # method, the last moment a window should be valid.
+        self._win.erase ()
+        self._win.write ("Goodbye cruel world...")
+
+        if cleanup_properly:
+            # Disconnect the listeners and delete the lambda functions.
+            # This removes cyclic references to SELF, and so alows SELF to
+            # be deleted.
+            gdb.events.stop.disconnect (self._stop_listener)
+            gdb.events.exited.disconnect (self._exit_listener)
+            self._stop_listener = None
+            self._exit_listener = None
+
+    def _event (self, type, event):
+        global perform_valid_check
+        global update_title
+
+        self._count += 1
+        self._events.insert (0, type)
+        if not perform_valid_check or self._win.is_valid ():
+            if update_title:
+                self._win.title = "This Is The Event Window (" + str (self._count) + ")"
+            else:
+                self.render ()
+
+    def render (self):
+        self._win.erase ()
+        w = self._win.width
+        h = self._win.height
+        for i in range (min (h, len (self._events))):
+            self._win.write (self._events[i] + "\n")
+
+gdb.register_window_type("events", EventWindow)
diff --git a/gdb/tui/tui-data.h b/gdb/tui/tui-data.h
index c92c8f9..b4d788d 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -96,7 +96,7 @@ public:
   /* Return true if this window is visible.  */
   bool is_visible () const
   {
-    return handle != nullptr;
+    return handle != nullptr && tui_active;
   }
 
   /* Return true if this window can accept the focus.  */
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index f08d62d..bf9c3ff 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -86,6 +86,7 @@ tui_apply_current_layout ()
       tui_win_list[win_type] = nullptr;
 
   /* This should always be made visible by a layout.  */
+  gdb_assert (TUI_CMD_WIN != nullptr);
   gdb_assert (TUI_CMD_WIN->is_visible ());
 
   /* Get the new list of currently visible windows.  */
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index ce8dab3..af92b2a 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -420,6 +420,12 @@ tui_enable (void)
 	}
 #endif
 
+      /* We must mark the tui sub-system active before trying to setup the
+	 current layout as tui windows defined by an extension language
+	 rely on this flag being true in order to know that the window
+	 they are creating is currently valid.  */
+      tui_active = true;
+
       cbreak ();
       noecho ();
       /* timeout (1); */
@@ -439,19 +445,21 @@ tui_enable (void)
     }
   else
     {
-     /* Save the current gdb setting of the terminal.
-	Curses will restore this state when endwin() is called.  */
-     def_shell_mode ();
-     clearok (stdscr, TRUE);
-   }
+      /* Save the current gdb setting of the terminal.
+	 Curses will restore this state when endwin() is called.  */
+      def_shell_mode ();
+      clearok (stdscr, TRUE);
+
+      tui_active = true;
+    }
+
+  gdb_assert (tui_active);
 
   if (tui_update_variables ())
     tui_rehighlight_all ();
 
   tui_setup_io (1);
 
-  tui_active = true;
-
   /* Resize windows before anything might display/refresh a
      window.  */
   if (tui_win_resized ())