Re: [PATCH 2/4] Return true in TuiWindow.is_valid only if TUI is enabled

[Andrew Burgess <andrew.burgess@embecosm.com>]

After further though I think there's an even better solution for this
issue.  See the patch below.

Thanks,
Andrew

---

commit d810f10aeb428e1d83148906381b8a5e837a0719
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Jan 15 10:31:19 2021 +0000

    gdb: return true in TuiWindow.is_valid even when tui is disabled
    
    This commit is inspired by the patch proposed here:
    
      https://sourceware.org/pipermail/gdb-patches/2020-December/174338.html
    
    And is a replacement for the patch proposed here:
    
      https://sourceware.org/pipermail/gdb-patches/2021-January/175122.html
    
    A recap of the problems addressed by this patch:
    
    If the user implements a TUI window in Python, and this window
    responds to GDB events and redraws its window contents, then there is
    currently a case where GDB will incorrectly try to draw the tui window
    to the screen when it shouldn't, causing the screen to become
    corrupted.
    
    The Python API documentation says 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.
    
    My interpretation of "destroys" in this paragraph is that the
    TuiWindow object will be deleted, and indeed this is true.  If I
    create a TuiWindow and include it in a layout then the TuiWindow is
    constructed.  When I switch to a tui layout that doesn't include my
    new TuiWindow then the object is deleted and the destructor is called.
    
    However, the edge case occurs when 'tui disable' is used.  In this
    case the window objects are NOT deleted.  Instead the cursors display
    is temporarily hidden and the CLI restored.  As the TuiWindow has not
    been destroyed the is_valid method still returns true.
    
    This means that if the user writes something like this:
    
      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)
    
    When an event arrives, even when 'tui disable' is in effect, the
    is_valid call returns True and we end up calling erase and write.
    Internally these methods repeat the is_valid check (which is still
    True), and so they write to the screen, corrupting the CLI display.
    
    In the first two attempts to fix this issue the approach taken was to
    try and work around the current behaviour of 'tui disable', the
    is_valid check was extended to check both conditions, has the window
    been destroyed, OR, is the tui disabled.
    
    However, the more I considered this, the more I was dissatisfied with
    this approach.
    
    This new patch proposes to bring 'tui disable' in line with any other
    change to the layout, windows that are no longer displayed (in this
    case all of them) are deleted.
    
    Now the existing is_valid check is sufficient.  A TuiWindow is either
    displayed, or destroyed.  I even add an assert that if the tui is
    disabled then the window will be destroyed.
    
    I did worry briefly that deleting all windows on 'tui disable' was
    wasteful, but that didn't worry me for long.  I figure a tui user is
    likely to switch layouts far more often than they enable/disable tui
    mode, and if deleting unused windows is acceptable as we switch
    layouts, then deleting the windows on disable should be fine too.
    
    I've added a test case that captures the original situation, a call to
    is_valid when 'tui disable' is in effect now returns False.  The test
    also checks that calling erase/write will throw an exception.
    
    There's also a new 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.
    
    gdb/ChangeLog:
    
            * python/py-tui.c (struct 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): Remove duplicate error check, call
            REQUIRE_WINDOW_FOR_SETTER.
            * tui/tui-data.h (struct tui_win_info) <is_visible>: Add an
            assert.
            * 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.
            (tui_disable): Delete all windows.
    
    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.

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 6e9a1462ec5..e97acab4043 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -47,6 +47,18 @@ 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.  The WINDOW pointer is set back
+     to nullptr whenever the tui window is deleted, and the tui window is
+     always deleted when it is no being displayed, so this check is really
+     just a check that the WINDOW pointer is not nullptr.  */
+  bool is_valid () const
+  {
+    /* This assert ensures that either the tui is active, or this the tui
+       window for this object has been deleted.  */
+    gdb_assert (tui_active || window == nullptr);
+    return window != nullptr;
+  }
 };
 
 extern PyTypeObject gdbpy_tui_window_object_type
@@ -340,15 +352,29 @@ gdbpy_register_tui_window (PyObject *self, PyObject *args, PyObject *kw)
 
 \f
 
-/* Require that "Window" be a valid window.  */
+/* Require that "Window" be a valid window.  Used from functions that
+   return a PyObject*.  */
 
 #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.  Used from functions that
+   return an integer.  */
+
+#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 +382,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,17 +454,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;
-    }
-
-  if (win->window == nullptr)
-    {
-      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
-      return -1;
-    }
+  REQUIRE_WINDOW_FOR_SETTER (win);
 
   gdb::unique_xmalloc_ptr<char> value
     = python_string_to_host_string (newvalue);
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 00000000000..898c5361ca3
--- /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 00000000000..d55bf2947c0
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.exp
@@ -0,0 +1,152 @@
+# 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
+}
+
+# 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 } {
+    global testfile srcdir subdir
+
+    Term::clean_restart 24 80 $testfile
+
+    # Skip all tests if Python scripting is not enabled.
+    if { [skip_python_tests] } { continue }
+
+    # Copy the Python script over and source it into GDB.
+    set remote_python_file [gdb_remote_download host \
+				${srcdir}/${subdir}/${testfile}.py]
+    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 { $cleanup_properly } {
+	gdb_test_no_output "python cleanup_properly = True"
+    } else {
+	gdb_test_no_output "python cleanup_properly = False"
+    }
+
+    if {![runto_main]} {
+	perror "test suppressed"
+	return
+    }
+
+    if {![Term::enter_tui]} {
+	unsupported "TUI not supported"
+    }
+
+    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"
+
+    set exception_pattern ""
+    if { ! $cleanup_properly } {
+	set exception_pattern "\r\nPython Exception\[^\n\r\]+TUI window is invalid\[^\n\r\]+"
+    }
+
+    # 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"
+    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
+	}
+    }
+}
+
+# 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 00000000000..1fcd2f7f04b
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.py
@@ -0,0 +1,67 @@
+# 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
+
+perform_valid_check = True
+update_title = False
+cleanup_properly = False
+
+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 = []
+
+    def close (self):
+        global cleanup_properly
+
+        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 21dc2eff571..b24cfc3ec54 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -96,6 +96,7 @@ struct tui_win_info
   /* Return true if this window is visible.  */
   bool is_visible () const
   {
+    gdb_assert (tui_active || handle == nullptr);
     return handle != nullptr;
   }
 
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index d6e299b00f1..0a545769afb 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -94,6 +94,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 ());
 
   /* Now delete any window that was not re-applied.  */
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index ce8dab3c642..d3736ae9607 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -360,6 +360,11 @@ tui_enable (void)
   if (tui_active)
     return;
 
+  /* We must mark the tui sub-system active before trying to initialise or
+     refresh things below as this flag is checked when deciding if a window
+     is visible or not.  */
+  tui_active = true;
+
   /* To avoid to initialize curses when gdb starts, there is a deferred
      curses initialization.  This initialization is made only once
      and the first time the curses mode is entered.  */
@@ -450,8 +455,6 @@ tui_enable (void)
 
   tui_setup_io (1);
 
-  tui_active = true;
-
   /* Resize windows before anything might display/refresh a
      window.  */
   if (tui_win_resized ())
@@ -512,6 +515,19 @@ tui_disable (void)
 
   tui_active = false;
   tui_update_gdb_sizes ();
+
+  /* Delete all the windows now we're leaving tui mode.  */
+  tui_win_info *locator = tui_locator_win_info_ptr ();
+  for (tui_win_info *win_info : tui_windows)
+    {
+      if (win_info != locator)
+	delete win_info;
+    }
+  tui_windows.clear ();
+
+  /* Update the global list of builtin windows types.  */
+  for (int win_type = SRC_WIN; (win_type < MAX_MAJOR_WINDOWS); win_type++)
+    tui_win_list[win_type] = nullptr;
 }
 
 /* Command wrapper for enabling tui mode.  */