[PATCH 1/4] Fix wrong method name

[PATCH 1/4] Fix wrong method name

[Hannes Domani]

The objects returned by FrameDecorator.frame_args need to implement a
method named symbol, not argument.

gdb/doc/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* python.texi (Frame Decorator API): Fix method name.
---
 gdb/doc/python.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 11cc877125..51f6d1aa05 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2045,7 +2045,7 @@ empty iterable, or @code{None} means frame arguments will not be
 printed for this frame.  This iterable must contain objects that
 implement two methods, described here.
 
-This object must implement a @code{argument} method which takes a
+This object must implement a @code{symbol} method which takes a
 single @code{self} parameter and must return a @code{gdb.Symbol}
 (@pxref{Symbols In Python}), or a Python string.  The object must also
 implement a @code{value} method which takes a single @code{self}
-- 
2.29.2

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

[Hannes Domani]

There seems to be no other way to determine if TUI is enabled, which is a
problem in case the TUI is disabled, and you redraw the window contents
based on some registered event.
Then the TUI is redrawn, even though the TUI stays disabled, and the prompt
is anywhere on the screen.

gdb/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* python/py-tui.c (gdbpy_tui_is_valid): Check tui_active as well.
---
 gdb/python/py-tui.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 87065eaf60..9c35778bcb 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -356,7 +356,7 @@ gdbpy_tui_is_valid (PyObject *self, PyObject *args)
 {
   gdbpy_tui_window *win = (gdbpy_tui_window *) self;
 
-  if (win->window != nullptr)
+  if (tui_active && win->window != nullptr)
     Py_RETURN_TRUE;
   Py_RETURN_FALSE;
 }
-- 
2.29.2

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

[Simon Marchi]

On 2020-12-29 12:02 p.m., Hannes Domani via Gdb-patches wrote:
> There seems to be no other way to determine if TUI is enabled, which is a
> problem in case the TUI is disabled, and you redraw the window contents
> based on some registered event.
> Then the TUI is redrawn, even though the TUI stays disabled, and the prompt
> is anywhere on the screen.
> 
> gdb/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* python/py-tui.c (gdbpy_tui_is_valid): Check tui_active as well.
> ---
>  gdb/python/py-tui.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
> index 87065eaf60..9c35778bcb 100644
> --- a/gdb/python/py-tui.c
> +++ b/gdb/python/py-tui.c
> @@ -356,7 +356,7 @@ gdbpy_tui_is_valid (PyObject *self, PyObject *args)
>  {
>    gdbpy_tui_window *win = (gdbpy_tui_window *) self;
>  
> -  if (win->window != nullptr)
> +  if (tui_active && win->window != nullptr)
>      Py_RETURN_TRUE;
>    Py_RETURN_FALSE;
>  }
> 

I think that makes sense, although I'd like Tom to confirm.

Simon

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

[Andrew Burgess]

Hannes,

Thanks for looking into this issue.

I started working on a test for this issue, and as I did I discovered
more than just the original bug.

I'd like to propose the patch below which fixes the original issue you
found, as well as some of the additional issues.

Let me know what you think.

Thanks,
Andrew

---

From bdc61716725587696ffac9c1f2b1d793332be31e Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Fri, 15 Jan 2021 10:31:19 +0000
Subject: [PATCH] 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 (struct gdbpy_tui_window)> <is_valid>: New
	member function.
	(class tui_py_window) <is_valid>: Likewise.
	(REQUIRE_WINDOW): Call is_valid member function.
	(gdbpy_tui_is_valid): Likewise.
	* 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.
---
 gdb/ChangeLog                                 |  13 +++
 gdb/doc/ChangeLog                             |   5 +
 gdb/doc/python.texi                           |   5 +
 gdb/python/py-tui.c                           |  21 +++-
 gdb/testsuite/ChangeLog                       |   6 +
 .../gdb.python/tui-window-disabled.c          |  43 +++++++
 .../gdb.python/tui-window-disabled.exp        | 109 ++++++++++++++++++
 .../gdb.python/tui-window-disabled.py         |  56 +++++++++
 gdb/tui/tui-data.h                            |   2 +-
 gdb/tui/tui-layout.c                          |   1 +
 gdb/tui/tui.c                                 |   7 +-
 11 files changed, 263 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/tui-window-disabled.c
 create mode 100644 gdb/testsuite/gdb.python/tui-window-disabled.exp
 create mode 100644 gdb/testsuite/gdb.python/tui-window-disabled.py

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 0f776f54768..4beccc18488 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5844,6 +5844,11 @@
 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 6e9a1462ec5..5d2ea316876 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
@@ -96,6 +99,12 @@ class tui_py_window : public tui_win_info
       }
   }
 
+  /* A TUI window is valid only when it is visible.  */
+  bool is_valid ()
+  {
+    return is_visible ();
+  }
+
   /* Erase and re-box the window.  */
   void erase ()
   {
@@ -137,6 +146,14 @@ class tui_py_window : public tui_win_info
   gdbpy_ref<gdbpy_tui_window> m_wrapper;
 };
 
+/* See gdbpy_tui_window declaration above.  */
+
+bool
+gdbpy_tui_window::is_valid () const
+{
+  return window != nullptr && window->is_valid ();
+}
+
 tui_py_window::~tui_py_window ()
 {
   gdbpy_enter enter_py (get_current_arch (), current_language);
@@ -344,7 +361,7 @@ 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)
@@ -356,7 +373,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;
 }
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..620eca66043
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.exp
@@ -0,0 +1,109 @@
+# 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 checks that if the user has done 'tui disable' then the
+# is_valid method on the TUI window will return false, this allows the
+# user to correctly skip redrawing the window.
+
+load_lib gdb-python.exp
+tuiterm_env
+
+standard_testfile
+
+if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
+    return -1
+}
+
+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 {![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"
+
+# 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\\);\r\nPython Exception\[^\n\r\]+TUI window is invalid\[^\n\r\]+" {
+	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"
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..50771141991
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-disabled.py
@@ -0,0 +1,56 @@
+# 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
+
+class EventWindow:
+    def __init__ (self, win):
+        self._win = win
+        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):
+        # 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
+
+        self._events.insert (0, type)
+        if not perform_valid_check or self._win.is_valid ():
+            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..fe4b19fe90f 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -96,7 +96,7 @@ struct tui_win_info
   /* 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 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 3a2229db16c..2c2612f3146 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 ())
-- 
2.25.4

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

[Hannes Domani]

 Am Freitag, 15. Januar 2021, 17:58:16 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> Hannes,
>
> Thanks for looking into this issue.
>
> I started working on a test for this issue, and as I did I discovered
> more than just the original bug.
>
> I'd like to propose the patch below which fixes the original issue you
> found, as well as some of the additional issues.
>
> Let me know what you think.

Yes, this is better.

I can't run any TUI testsuite examples since I'm using pdcurses, so I'm
grateful that you also covered that.


Hannes

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

[Andrew Burgess]

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.  */

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

[Hannes Domani]

 Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

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

This version crashes for me when I disable and then re-enable TUI, because
tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
tui_update_gdb_sizes call at the end of tui_enable dereferences TUI_CMD_WIN.

Also, and I think your previous patch behaves the same, but I can no longer use
the TuiWindows variables width/height/title inside my Window.close function,
because at this point tui_active==false, so the assertion inside is_valid
fails.
This can be worked around by saving the values in some Window class members,
but it is more inconvenient.


Hannes

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

[Andrew Burgess]

* Hannes Domani <ssbssa@yahoo.de> [2021-01-17 13:19:41 +0000]:

>  Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> 
> > After further though I think there's an even better solution for this
> > issue.  See the patch below.
> 
> This version crashes for me when I disable and then re-enable TUI, because
> tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
> tui_update_gdb_sizes call at the end of tui_enable dereferences
> TUI_CMD_WIN.

That's annoying.  I would have sworn I'd tested that.  Guess not.
I'll take a look at this later today.  I guess something that's
currently conditional will need to become unconditional so that the
windows are recreated when the tui is enabled.

> 
> Also, and I think your previous patch behaves the same, but I can no longer use
> the TuiWindows variables width/height/title inside my Window.close function,
> because at this point tui_active==false, so the assertion inside is_valid
> fails.

OK, I think this one should be easy enough to fix.

Out of interest, what do you do with the width/height/title in the
close method?  Given the window is about to vanish I'd have thought
all you'd be doing at this point is internal object clean up?

> This can be worked around by saving the values in some Window class members,
> but it is more inconvenient.

I think my fix will mean you don't need to do this.

Thanks,
Andrew

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

[Hannes Domani]

 Am Montag, 18. Januar 2021, 11:31:00 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> * Hannes Domani <ssbssa@yahoo.de> [2021-01-17 13:19:41 +0000]:
>
> >  Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> >
> > > After further though I think there's an even better solution for this
> > > issue.  See the patch below.
> >
> > This version crashes for me when I disable and then re-enable TUI, because
> > tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
> > tui_update_gdb_sizes call at the end of tui_enable dereferences
> > TUI_CMD_WIN.
>
> That's annoying.  I would have sworn I'd tested that.  Guess not.
> I'll take a look at this later today.  I guess something that's
> currently conditional will need to become unconditional so that the
> windows are recreated when the tui is enabled.
>
> >
> > Also, and I think your previous patch behaves the same, but I can no longer use
> > the TuiWindows variables width/height/title inside my Window.close function,
> > because at this point tui_active==false, so the assertion inside is_valid
> > fails.
>
> OK, I think this one should be easy enough to fix.
>
> Out of interest, what do you do with the width/height/title in the
> close method?  Given the window is about to vanish I'd have thought
> all you'd be doing at this point is internal object clean up?

I have all my TUI windows in a dictionary, with the window title as key.
And on Window.close I remove the window from the dictionary.


> > This can be worked around by saving the values in some Window class members,
> > but it is more inconvenient.
>
> I think my fix will mean you don't need to do this.

Thanks.


Hannes

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

[Andrew Burgess]

* Hannes Domani <ssbssa@yahoo.de> [2021-01-17 13:19:41 +0000]:

>  Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> 
> > After further though I think there's an even better solution for this
> > issue.  See the patch below.
> 
> This version crashes for me when I disable and then re-enable TUI, because
> tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
> tui_update_gdb_sizes call at the end of tui_enable dereferences TUI_CMD_WIN.
> 
> Also, and I think your previous patch behaves the same, but I can no longer use
> the TuiWindows variables width/height/title inside my Window.close function,
> because at this point tui_active==false, so the assertion inside is_valid
> fails.
> This can be worked around by saving the values in some Window class members,
> but it is more inconvenient.

Hannes,

Would you be able to give the version below a try please and let me
know if you are still seeing any problems.

This is much more like the first patch I posted.

I honestly think the second approach is better, but it throws up so
many other issues that would need to be resolved around the whole
disable/enable transition, that I think going with the first approach
is just going to be simpler.

Anyway, I think this covers all the situations you've described so
far, but let me know if you still see any issues.

Thanks,
Andrew

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

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2021-01-23 17:54:42 +0000]:

> * Hannes Domani <ssbssa@yahoo.de> [2021-01-17 13:19:41 +0000]:
> 
> >  Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> > 
> > > After further though I think there's an even better solution for this
> > > issue.  See the patch below.
> > 
> > This version crashes for me when I disable and then re-enable TUI, because
> > tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
> > tui_update_gdb_sizes call at the end of tui_enable dereferences TUI_CMD_WIN.
> > 
> > Also, and I think your previous patch behaves the same, but I can no longer use
> > the TuiWindows variables width/height/title inside my Window.close function,
> > because at this point tui_active==false, so the assertion inside is_valid
> > fails.
> > This can be worked around by saving the values in some Window class members,
> > but it is more inconvenient.
> 
> Hannes,
> 
> Would you be able to give the version below a try please and let me
> know if you are still seeing any problems.
> 
> This is much more like the first patch I posted.
> 
> I honestly think the second approach is better, but it throws up so
> many other issues that would need to be resolved around the whole
> disable/enable transition, that I think going with the first approach
> is just going to be simpler.
> 
> Anyway, I think this covers all the situations you've described so
> far, but let me know if you still see any issues.
> 
> Thanks,
> Andrew

Might help if I included the patch :)

---

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

    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): Remove duplicate error checking, 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.

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 35568594f58..eed4d7d49e1 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5848,6 +5848,11 @@
 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 6e9a1462ec5..5a35f7b3ec2 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 @@ class tui_py_window : public tui_win_info
   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,17 +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;
-    }
-
-  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..af1fa0cde63
--- /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 00000000000..0b3c076f7e9
--- /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 21dc2eff571..fe4b19fe90f 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -96,7 +96,7 @@ struct tui_win_info
   /* 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 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..af92b2a8042 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 ())

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

[Hannes Domani]

 Am Samstag, 23. Januar 2021, 18:55:56 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> * Andrew Burgess <andrew.burgess@embecosm.com> [2021-01-23 17:54:42 +0000]:
>
> > * Hannes Domani <ssbssa@yahoo.de> [2021-01-17 13:19:41 +0000]:
> >
> > >  Am Sonntag, 17. Januar 2021, 12:31:05 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> > >
> > > > After further though I think there's an even better solution for this
> > > > issue.  See the patch below.
> > >
> > > This version crashes for me when I disable and then re-enable TUI, because
> > > tui_windows and tui_win_list are empty, not filled again in tui_enable, and the
> > > tui_update_gdb_sizes call at the end of tui_enable dereferences TUI_CMD_WIN.
> > >
> > > Also, and I think your previous patch behaves the same, but I can no longer use
> > > the TuiWindows variables width/height/title inside my Window.close function,
> > > because at this point tui_active==false, so the assertion inside is_valid
> > > fails.
> > > This can be worked around by saving the values in some Window class members,
> > > but it is more inconvenient.
> >
> > Hannes,
> >
> > Would you be able to give the version below a try please and let me
> > know if you are still seeing any problems.
> >
> > This is much more like the first patch I posted.
> >
> > I honestly think the second approach is better, but it throws up so
> > many other issues that would need to be resolved around the whole
> > disable/enable transition, that I think going with the first approach
> > is just going to be simpler.
> >
> > Anyway, I think this covers all the situations you've described so
> > far, but let me know if you still see any issues.
> >
> > Thanks,
> > Andrew
>
> Might help if I included the patch :)

Yes, with this I have seen no problems so far.


Hannes

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew>     gdb/ChangeLog:
    
Andrew>             * python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
Andrew>             function.
Andrew>             (REQUIRE_WINDOW): Call is_valid member function.
Andrew>             (REQUIRE_WINDOW_FOR_SETTER): New define.
Andrew>             (gdbpy_tui_is_valid): Call is_valid member function.
Andrew>             (gdbpy_tui_set_title): Remove duplicate error checking, call
Andrew>             REQUIRE_WINDOW_FOR_SETTER instead.
Andrew>             * tui/tui-data.h (struct tui_win_info) <is_visible>: Check
Andrew>             tui_active too.
Andrew>             * tui/tui-layout.c (tui_apply_current_layout): Add an assert.
Andrew>             * tui/tui.c (tui_enable): Move setting of tui_active earlier in
Andrew>             the function.
    
Hi.  Thanks for the patch.

I really thought I had covered this scenario somehow, but I guess not!
Oops, sorry about that.  Maybe I was thinking that tui disable would
destory the windows.  Or maybe I planned to do that and then never did.
I don't recall any more.

Andrew> -  if (win->window == nullptr)
Andrew> -    {
Andrew> -      PyErr_Format (PyExc_RuntimeError, _("TUI window is invalid."));
Andrew> -      return -1;
Andrew> -    }
Andrew> -
Andrew> -  if (win->window == nullptr)
Andrew> -    {
Andrew> -      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
Andrew> -      return -1;
Andrew> -    }

This second 'if' has the same condition.  It's actually a latent bug, as
the code should read:

    if (newvalue == nullptr)

... and then the "if" should remain, rather than being deleted by this
patch.

I think the patch is ok with this tweak.

Tom

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

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-02-06 12:38:56 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew>     gdb/ChangeLog:
>     
> Andrew>             * python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
> Andrew>             function.
> Andrew>             (REQUIRE_WINDOW): Call is_valid member function.
> Andrew>             (REQUIRE_WINDOW_FOR_SETTER): New define.
> Andrew>             (gdbpy_tui_is_valid): Call is_valid member function.
> Andrew>             (gdbpy_tui_set_title): Remove duplicate error checking, call
> Andrew>             REQUIRE_WINDOW_FOR_SETTER instead.
> Andrew>             * tui/tui-data.h (struct tui_win_info) <is_visible>: Check
> Andrew>             tui_active too.
> Andrew>             * tui/tui-layout.c (tui_apply_current_layout): Add an assert.
> Andrew>             * tui/tui.c (tui_enable): Move setting of tui_active earlier in
> Andrew>             the function.
>     
> Hi.  Thanks for the patch.
> 
> I really thought I had covered this scenario somehow, but I guess not!
> Oops, sorry about that.  Maybe I was thinking that tui disable would
> destory the windows.  Or maybe I planned to do that and then never did.
> I don't recall any more.
> 
> Andrew> -  if (win->window == nullptr)
> Andrew> -    {
> Andrew> -      PyErr_Format (PyExc_RuntimeError, _("TUI window is invalid."));
> Andrew> -      return -1;
> Andrew> -    }
> Andrew> -
> Andrew> -  if (win->window == nullptr)
> Andrew> -    {
> Andrew> -      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
> Andrew> -      return -1;
> Andrew> -    }
> 
> This second 'if' has the same condition.  It's actually a latent bug, as
> the code should read:
> 
>     if (newvalue == nullptr)
> 
> ... and then the "if" should remain, rather than being deleted by this
> patch.
> 
> I think the patch is ok with this tweak.

Thanks for pointing this out.

I span this fix out into its own commit and added a test for it.
Below is what I pushed.

Thanks,
Andrew

---

commit e0c23e11da18b615c382888da8e978f16428e81b
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Feb 8 11:44:51 2021 +0000

    gdb/python: don't allow the user to delete window title attributes
    
    There's a bug in the python tui API.  If the user tries to delete the
    window title attribute then this will trigger undefined behaviour in
    GDB due to a missing nullptr check.
    
    gdb/ChangeLog:
    
            * python/py-tui.c (gdbpy_tui_set_title): Check that the new value
            for the title is not nullptr.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.python/tui-window.exp: Add new tests.
            * gdb.python/tui-window.py (TestWindow) <__init__>: Store
            TestWindow object into global the_window.
            <remote_title>: New method.
            (delete_window_title): New function.

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 6e9a1462ec5..73b73f33d4a 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -434,7 +434,7 @@ gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
       return -1;
     }
 
-  if (win->window == nullptr)
+  if (newvalue == nullptr)
     {
       PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
       return -1;
diff --git a/gdb/testsuite/gdb.python/tui-window.exp b/gdb/testsuite/gdb.python/tui-window.exp
index 13e14be0654..8d86afb1449 100644
--- a/gdb/testsuite/gdb.python/tui-window.exp
+++ b/gdb/testsuite/gdb.python/tui-window.exp
@@ -47,6 +47,12 @@ Term::check_contents "test title" \
     "This Is The Title"
 Term::check_contents "Window display" "Test: 0"
 
+Term::command "python delete_window_title ()"
+Term::check_contents "error message after trying to delete title" \
+    "TypeError: Cannot delete \"title\" attribute\\."
+Term::check_contents "title is unchanged" \
+    "This Is The Title"
+
 Term::resize 51 51
 # Remember that a resize request actually does two resizes...
 Term::check_contents "Window was updated" "Test: 2"
diff --git a/gdb/testsuite/gdb.python/tui-window.py b/gdb/testsuite/gdb.python/tui-window.py
index 88a1b06f12c..3bea78846ae 100644
--- a/gdb/testsuite/gdb.python/tui-window.py
+++ b/gdb/testsuite/gdb.python/tui-window.py
@@ -22,7 +22,7 @@ the_window = None
 class TestWindow:
     def __init__(self, win):
         global the_window
-        the_window = win
+        the_window = self
         self.count = 0
         self.win = win
         win.title = "This Is The Title"
@@ -34,8 +34,16 @@ class TestWindow:
         self.win.write("Test: " + str(self.count) + " " + str(w) + "x" + str(h))
         self.count = self.count + 1
 
+    # Tries to delete the title attribute.  GDB will throw an error.
+    def remove_title(self):
+        del self.win.title
+
 gdb.register_window_type("test", TestWindow)
 
+# Call REMOVE_TITLE on the global window object.
+def delete_window_title ():
+    the_window.remove_title ()
+
 # A TUI window "constructor" that always fails.
 def failwin(win):
     raise RuntimeError("Whoops")

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I span this fix out into its own commit and added a test for it.
Andrew> Below is what I pushed.

Thank you for doing this.

Tom

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

[Eli Zaretskii]

> Date: Fri, 15 Jan 2021 16:48:11 +0000
> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org
> 
> gdb/ChangeLog:
> 
> 	* python/py-tui.c (struct gdbpy_tui_window)> <is_valid>: New
> 	member function.
> 	(class tui_py_window) <is_valid>: Likewise.
> 	(REQUIRE_WINDOW): Call is_valid member function.
> 	(gdbpy_tui_is_valid): Likewise.
> 	* 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.

OK for the documentation part, thanks.

[PATCH 3/4] Add optional styled argument to gdb.execute

[Hannes Domani]

This makes it possible to use the colored output of commands, e.g. for
a custom TuiWindow.

gdb/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* cli/cli-script.c (execute_control_commands_to_string): Use
	styled argument.
	* cli/cli-script.h (execute_control_commands_to_string): Add
	styled argument.
	* python/python.c (execute_gdb_command): Parse "styled" argument.

gdb/doc/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* python.texi (Basic Python): Document "styled" argument.
---
 gdb/cli/cli-script.c |  4 ++--
 gdb/cli/cli-script.h |  2 +-
 gdb/doc/python.texi  |  6 +++++-
 gdb/python/python.c  | 24 ++++++++++++++++++------
 4 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index c9c4a713de..273f925cbc 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -422,13 +422,13 @@ execute_control_commands (struct command_line *cmdlines, int from_tty)
 
 std::string
 execute_control_commands_to_string (struct command_line *commands,
-				    int from_tty)
+				    int from_tty, bool styled)
 {
   /* GDB_STDOUT should be better already restored during these
      restoration callbacks.  */
   set_batch_flag_and_restore_page_info save_page_info;
 
-  string_file str_file;
+  string_file str_file (styled);
 
   {
     current_uiout->redirect (&str_file);
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 6ad6e61fb4..2ca9f13d4f 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -135,7 +135,7 @@ extern void execute_control_commands (struct command_line *cmdlines,
    will be temporarily set to true.  */
 
 extern std::string execute_control_commands_to_string
-    (struct command_line *commands, int from_tty);
+    (struct command_line *commands, int from_tty, bool styled = false);
 
 /* Exported to gdb/breakpoint.c */
 
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 51f6d1aa05..c97866e9fc 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -217,7 +217,7 @@ A string containing the python directory (@pxref{Python}).
 @end defvar
 
 @findex gdb.execute
-@defun gdb.execute (command @r{[}, from_tty @r{[}, to_string@r{]]})
+@defun gdb.execute (command @r{[}, from_tty @r{[}, to_string @r{[}, styled@r{]]]})
 Evaluate @var{command}, a string, as a @value{GDBN} CLI command.
 If a GDB exception happens while @var{command} runs, it is
 translated as described in @ref{Exception Handling,,Exception Handling}.
@@ -234,6 +234,10 @@ returned as a string.  The default is @code{False}, in which case the
 return value is @code{None}.  If @var{to_string} is @code{True}, the
 @value{GDBN} virtual terminal will be temporarily set to unlimited width
 and height, and its pagination will be disabled; @pxref{Screen Size}.
+
+If both @var{to_string} and @var{styled} are @code{True}, then the returned
+string also contains the ANSI terminal escape styling sequences used for
+colored output.
 @end defun
 
 @findex gdb.breakpoints
diff --git a/gdb/python/python.c b/gdb/python/python.c
index bf3abdbbbe..eb9f8fb4fe 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -574,13 +574,15 @@ static PyObject *
 execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 {
   const char *arg;
-  PyObject *from_tty_obj = NULL, *to_string_obj = NULL;
-  int from_tty, to_string;
-  static const char *keywords[] = { "command", "from_tty", "to_string", NULL };
+  PyObject *from_tty_obj = NULL, *to_string_obj = NULL, *styled_obj = NULL;
+  int from_tty, to_string, styled;
+  static const char *keywords[] = { "command", "from_tty", "to_string",
+				    "styled", NULL };
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!", keywords, &arg,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!O!", keywords, &arg,
 					&PyBool_Type, &from_tty_obj,
-					&PyBool_Type, &to_string_obj))
+					&PyBool_Type, &to_string_obj,
+					&PyBool_Type, &styled_obj))
     return NULL;
 
   from_tty = 0;
@@ -601,6 +603,15 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
       to_string = cmp;
     }
 
+  styled = 0;
+  if (styled_obj)
+    {
+      int cmp = PyObject_IsTrue (styled_obj);
+      if (cmp < 0)
+	return NULL;
+      styled = cmp;
+    }
+
   std::string to_string_res;
 
   scoped_restore preventer = prevent_dont_repeat ();
@@ -638,7 +649,8 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 
 	if (to_string)
 	  to_string_res = execute_control_commands_to_string (lines.get (),
-							      from_tty);
+							      from_tty,
+							      styled);
 	else
 	  execute_control_commands (lines.get (), from_tty);
       }
-- 
2.29.2

Re: [PATCH 3/4] Add optional styled argument to gdb.execute

[Eli Zaretskii]

> Date: Tue, 29 Dec 2020 18:02:26 +0100
> From: Hannes Domani via Gdb-patches <gdb-patches@sourceware.org>
> 
> This makes it possible to use the colored output of commands, e.g. for
> a custom TuiWindow.
> 
> gdb/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* cli/cli-script.c (execute_control_commands_to_string): Use
> 	styled argument.
> 	* cli/cli-script.h (execute_control_commands_to_string): Add
> 	styled argument.
> 	* python/python.c (execute_gdb_command): Parse "styled" argument.
> 
> gdb/doc/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* python.texi (Basic Python): Document "styled" argument.

OK for the documentation part.  Thanks.

Re: [PATCH 3/4] Add optional styled argument to gdb.execute

[Simon Marchi]

On 2020-12-29 12:02 p.m., Hannes Domani via Gdb-patches wrote:
> This makes it possible to use the colored output of commands, e.g. for
> a custom TuiWindow.
> 
> gdb/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* cli/cli-script.c (execute_control_commands_to_string): Use
> 	styled argument.
> 	* cli/cli-script.h (execute_control_commands_to_string): Add
> 	styled argument.
> 	* python/python.c (execute_gdb_command): Parse "styled" argument.
> 
> gdb/doc/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* python.texi (Basic Python): Document "styled" argument.
> ---
>  gdb/cli/cli-script.c |  4 ++--
>  gdb/cli/cli-script.h |  2 +-
>  gdb/doc/python.texi  |  6 +++++-
>  gdb/python/python.c  | 24 ++++++++++++++++++------
>  4 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index c9c4a713de..273f925cbc 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -422,13 +422,13 @@ execute_control_commands (struct command_line *cmdlines, int from_tty)
>  
>  std::string
>  execute_control_commands_to_string (struct command_line *commands,
> -				    int from_tty)
> +				    int from_tty, bool styled)
>  {
>    /* GDB_STDOUT should be better already restored during these
>       restoration callbacks.  */
>    set_batch_flag_and_restore_page_info save_page_info;
>  
> -  string_file str_file;
> +  string_file str_file (styled);
>  
>    {
>      current_uiout->redirect (&str_file);
> diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
> index 6ad6e61fb4..2ca9f13d4f 100644
> --- a/gdb/cli/cli-script.h
> +++ b/gdb/cli/cli-script.h
> @@ -135,7 +135,7 @@ extern void execute_control_commands (struct command_line *cmdlines,
>     will be temporarily set to true.  */
>  
>  extern std::string execute_control_commands_to_string
> -    (struct command_line *commands, int from_tty);
> +    (struct command_line *commands, int from_tty, bool styled = false);
>  
>  /* Exported to gdb/breakpoint.c */
>  
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 51f6d1aa05..c97866e9fc 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -217,7 +217,7 @@ A string containing the python directory (@pxref{Python}).
>  @end defvar
>  
>  @findex gdb.execute
> -@defun gdb.execute (command @r{[}, from_tty @r{[}, to_string@r{]]})
> +@defun gdb.execute (command @r{[}, from_tty @r{[}, to_string @r{[}, styled@r{]]]})
>  Evaluate @var{command}, a string, as a @value{GDBN} CLI command.
>  If a GDB exception happens while @var{command} runs, it is
>  translated as described in @ref{Exception Handling,,Exception Handling}.
> @@ -234,6 +234,10 @@ returned as a string.  The default is @code{False}, in which case the
>  return value is @code{None}.  If @var{to_string} is @code{True}, the
>  @value{GDBN} virtual terminal will be temporarily set to unlimited width
>  and height, and its pagination will be disabled; @pxref{Screen Size}.
> +
> +If both @var{to_string} and @var{styled} are @code{True}, then the returned
> +string also contains the ANSI terminal escape styling sequences used for
> +colored output.
>  @end defun
>  
>  @findex gdb.breakpoints
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index bf3abdbbbe..eb9f8fb4fe 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -574,13 +574,15 @@ static PyObject *
>  execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>  {
>    const char *arg;
> -  PyObject *from_tty_obj = NULL, *to_string_obj = NULL;
> -  int from_tty, to_string;
> -  static const char *keywords[] = { "command", "from_tty", "to_string", NULL };
> +  PyObject *from_tty_obj = NULL, *to_string_obj = NULL, *styled_obj = NULL;
> +  int from_tty, to_string, styled;
> +  static const char *keywords[] = { "command", "from_tty", "to_string",
> +				    "styled", NULL };
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!", keywords, &arg,
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!O!", keywords, &arg,
>  					&PyBool_Type, &from_tty_obj,
> -					&PyBool_Type, &to_string_obj))
> +					&PyBool_Type, &to_string_obj,
> +					&PyBool_Type, &styled_obj))
>      return NULL;
>  
>    from_tty = 0;
> @@ -601,6 +603,15 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>        to_string = cmp;
>      }
>  
> +  styled = 0;
> +  if (styled_obj)
> +    {
> +      int cmp = PyObject_IsTrue (styled_obj);
> +      if (cmp < 0)
> +	return NULL;
> +      styled = cmp;
> +    }
> +
>    std::string to_string_res;
>  
>    scoped_restore preventer = prevent_dont_repeat ();
> @@ -638,7 +649,8 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>  	if (to_string)
>  	  to_string_res = execute_control_commands_to_string (lines.get (),
> -							      from_tty);
> +							      from_tty,
> +							      styled);
>  	else
>  	  execute_control_commands (lines.get (), from_tty);
>        }
> 

I think that also makes sense, but I'd like if Tom could give
it a quick look too.

Simon

[PATCH 4/4] Fix raw-frame-arguments in combination with frame-filters

[Hannes Domani]

Currently, if frame-filters are active, raw-values is used instead of
raw-frame-arguments to decide if a pretty-printer should be invoked for
frame arguments in a backtrace.

This adds the PRINT_RAW_FRAME_ARGUMENTS flag to frame_filter_flag which is
then used in the frame-filter to override the raw flag in enumerate_args.

gdb/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* extension.h (enum frame_filter_flag): Add
	PRINT_RAW_FRAME_ARGUMENTS.
	* python/py-framefilter.c (enumerate_args): Override raw flag.
	raw_frame_args argument.
	(py_mi_print_variables): Forward raw flag.
	(py_print_args): Forward raw_frame_args flag.
	(py_print_frame): Handle PRINT_RAW_FRAME_ARGUMENTS.
	* stack.c (backtrace_command_1): Set PRINT_RAW_FRAME_ARGUMENTS.

gdb/testsuite/ChangeLog:

2020-12-29  Hannes Domani  <ssbssa@yahoo.de>

	* gdb.python/py-frame-args.exp: Add bt raw-frame-arguments tests.
	* gdb.python/py-frame-args.py: Add basic frame-filter.
---
 gdb/extension.h                            |  4 ++++
 gdb/python/py-framefilter.c                | 14 ++++++++++----
 gdb/stack.c                                |  2 ++
 gdb/testsuite/gdb.python/py-frame-args.exp | 22 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-frame-args.py  | 13 +++++++++++++
 5 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/gdb/extension.h b/gdb/extension.h
index c840dbc704..f3ec61b163 100644
--- a/gdb/extension.h
+++ b/gdb/extension.h
@@ -103,6 +103,10 @@ enum frame_filter_flag
 
     /* Set this flag if elided frames should not be printed.  */
     PRINT_HIDE = 1 << 5,
+
+    /* Set this flag if pretty printers for frame arguments should not
+       be invoked.  */
+    PRINT_RAW_FRAME_ARGUMENTS = 1 << 6,
   };
 
 DEF_ENUM_FLAGS_TYPE (enum frame_filter_flag, frame_filter_flags);
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 944a0eee50..c61bfa4c2e 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -422,12 +422,14 @@ static enum ext_lang_bt_status
 enumerate_args (PyObject *iter,
 		struct ui_out *out,
 		enum ext_lang_frame_args args_type,
+		bool raw_frame_args,
 		int print_args_field,
 		struct frame_info *frame)
 {
   struct value_print_options opts;
 
   get_user_print_options (&opts);
+  opts.raw = raw_frame_args;
 
   if (args_type == CLI_SCALAR_VALUES)
     {
@@ -655,8 +657,8 @@ py_mi_print_variables (PyObject *filter, struct ui_out *out,
   ui_out_emit_list list_emitter (out, "variables");
 
   if (args_iter != Py_None
-      && (enumerate_args (args_iter.get (), out, args_type, 1, frame)
-	  == EXT_LANG_BT_ERROR))
+      && (enumerate_args (args_iter.get (), out, args_type, opts->raw, 1,
+			  frame) == EXT_LANG_BT_ERROR))
     return EXT_LANG_BT_ERROR;
 
   if (locals_iter != Py_None
@@ -701,6 +703,7 @@ static enum ext_lang_bt_status
 py_print_args (PyObject *filter,
 	       struct ui_out *out,
 	       enum ext_lang_frame_args args_type,
+	       bool raw_frame_args,
 	       struct frame_info *frame)
 {
   gdbpy_ref<> args_iter (get_py_iter_from_func (filter, "frame_args"));
@@ -726,7 +729,8 @@ py_print_args (PyObject *filter,
 	}
     }
   else if (args_iter != Py_None
-	   && (enumerate_args (args_iter.get (), out, args_type, 0, frame)
+	   && (enumerate_args (args_iter.get (), out, args_type,
+			       raw_frame_args, 0, frame)
 	       == EXT_LANG_BT_ERROR))
     return EXT_LANG_BT_ERROR;
 
@@ -957,7 +961,9 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
      wrong.  */
   if (print_args && (location_print || out->is_mi_like_p ()))
     {
-      if (py_print_args (filter, out, args_type, frame) == EXT_LANG_BT_ERROR)
+      bool raw_frame_args = (flags & PRINT_RAW_FRAME_ARGUMENTS) != 0;
+      if (py_print_args (filter, out, args_type, raw_frame_args, frame)
+	  == EXT_LANG_BT_ERROR)
 	return EXT_LANG_BT_ERROR;
     }
 
diff --git a/gdb/stack.c b/gdb/stack.c
index 943b3db087..07abdea8c1 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2028,6 +2028,8 @@ backtrace_command_1 (const frame_print_options &fp_opts,
     flags |= PRINT_LOCALS;
   if (bt_opts.hide)
     flags |= PRINT_HIDE;
+  if (fp_opts.print_raw_frame_arguments)
+    flags |= PRINT_RAW_FRAME_ARGUMENTS;
 
   if (!bt_opts.no_filters)
     {
diff --git a/gdb/testsuite/gdb.python/py-frame-args.exp b/gdb/testsuite/gdb.python/py-frame-args.exp
index fd9c1f4342..7c621e1302 100644
--- a/gdb/testsuite/gdb.python/py-frame-args.exp
+++ b/gdb/testsuite/gdb.python/py-frame-args.exp
@@ -34,6 +34,28 @@ gdb_test_no_output "source ${remote_python_file}" "load python file"
 gdb_breakpoint [gdb_get_line_number "break-here"]
 gdb_continue_to_breakpoint "break-here" ".* break-here .*"
 
+# Test raw-frame-arguments on backtrace with and without frame-filter
+foreach filtered [list "enable" "disable"] {
+    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
+
+    gdb_test "bt 1" \
+	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
+	"bt frame-filter=$filtered,pretty"
+
+    gdb_test "bt -raw-frame-arguments on 1" \
+	".*foo \\(x=42, ss=\[.\]{3}\\).*" \
+	"bt frame-filter=$filtered,raw"
+
+    # "set print raw-values" should not affect frame arguments
+    gdb_test_no_output "set print raw-values on" \
+	"raw-values-on,frame-filter=$filtered"
+    gdb_test "bt 1" \
+	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
+	"bt frame-filter=$filtered,pretty,raw-values"
+    gdb_test_no_output "set print raw-values off" \
+	"raw-values-off,frame-filter=$filtered"
+}
+
 # Test all combinations with raw off.
 
 gdb_test_no_output "set print raw-frame-arguments off"
diff --git a/gdb/testsuite/gdb.python/py-frame-args.py b/gdb/testsuite/gdb.python/py-frame-args.py
index c2908829c5..aed2070d94 100644
--- a/gdb/testsuite/gdb.python/py-frame-args.py
+++ b/gdb/testsuite/gdb.python/py-frame-args.py
@@ -73,3 +73,16 @@ pretty_printers_dict = {}
 
 register_pretty_printers ()
 gdb.pretty_printers.append (lookup_function)
+
+
+class BasicFrameFilter (object):
+    def __init__ (self):
+        self.name = "BasicFrameFilter"
+        self.priority = 100
+        self.enabled = True
+        gdb.frame_filters[self.name] = self
+
+    def filter (self, frame_iter):
+        return frame_iter
+
+BasicFrameFilter ()
-- 
2.29.2

Re: [PATCH 4/4] Fix raw-frame-arguments in combination with frame-filters

[Simon Marchi]

On 2020-12-29 12:02 p.m., Hannes Domani via Gdb-patches wrote:
> Currently, if frame-filters are active, raw-values is used instead of
> raw-frame-arguments to decide if a pretty-printer should be invoked for
> frame arguments in a backtrace.
> 
> This adds the PRINT_RAW_FRAME_ARGUMENTS flag to frame_filter_flag which is
> then used in the frame-filter to override the raw flag in enumerate_args.

I'm not sure I understand the problem, but I have a small comment for the
test case:

> diff --git a/gdb/testsuite/gdb.python/py-frame-args.exp b/gdb/testsuite/gdb.python/py-frame-args.exp
> index fd9c1f4342..7c621e1302 100644
> --- a/gdb/testsuite/gdb.python/py-frame-args.exp
> +++ b/gdb/testsuite/gdb.python/py-frame-args.exp
> @@ -34,6 +34,28 @@ gdb_test_no_output "source ${remote_python_file}" "load python file"
>  gdb_breakpoint [gdb_get_line_number "break-here"]
>  gdb_continue_to_breakpoint "break-here" ".* break-here .*"
>  
> +# Test raw-frame-arguments on backtrace with and without frame-filter
> +foreach filtered [list "enable" "disable"] {
> +    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
> +
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty"
> +
> +    gdb_test "bt -raw-frame-arguments on 1" \
> +	".*foo \\(x=42, ss=\[.\]{3}\\).*" \
> +	"bt frame-filter=$filtered,raw"
> +
> +    # "set print raw-values" should not affect frame arguments
> +    gdb_test_no_output "set print raw-values on" \
> +	"raw-values-on,frame-filter=$filtered"
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty,raw-values"
> +    gdb_test_no_output "set print raw-values off" \
> +	"raw-values-off,frame-filter=$filtered"
> +}

You can use foreach_with_prefix, that will ensure test names are
unique without having to include frame-filter=$filtered in each
test name.  Something like this:

# Test raw-frame-arguments on backtrace with and without frame-filter
foreach_with_prefix filtered {enable disable} {
    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"

    gdb_test "bt 1" \
	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
	"bt pretty"

    gdb_test "bt -raw-frame-arguments on 1" \
	".*foo \\(x=42, ss=\[.\]{3}\\).*" \
	"bt raw"

    # "set print raw-values" should not affect frame arguments
    gdb_test_no_output "set print raw-values on"
    gdb_test "bt 1" \
	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
	"bt pretty,raw-values"
    gdb_test_no_output "set print raw-values off"
}

Simon

Re: [PATCH 4/4] Fix raw-frame-arguments in combination with frame-filters

[Hannes Domani]

 Am Donnerstag, 31. Dezember 2020, 05:54:03 MEZ hat Simon Marchi <simon.marchi@polymtl.ca> Folgendes geschrieben:

> On 2020-12-29 12:02 p.m., Hannes Domani via Gdb-patches wrote:
> > Currently, if frame-filters are active, raw-values is used instead of
> > raw-frame-arguments to decide if a pretty-printer should be invoked for
> > frame arguments in a backtrace.
> >
> > This adds the PRINT_RAW_FRAME_ARGUMENTS flag to frame_filter_flag which is
> > then used in the frame-filter to override the raw flag in enumerate_args.
>
> I'm not sure I understand the problem, but I have a small comment for the
> test case:
>
> > diff --git a/gdb/testsuite/gdb.python/py-frame-args.exp b/gdb/testsuite/gdb.python/py-frame-args.exp
> > index fd9c1f4342..7c621e1302 100644
> > --- a/gdb/testsuite/gdb.python/py-frame-args.exp
> > +++ b/gdb/testsuite/gdb.python/py-frame-args.exp
> > @@ -34,6 +34,28 @@ gdb_test_no_output "source ${remote_python_file}" "load python file"
> >  gdb_breakpoint [gdb_get_line_number "break-here"]
> >  gdb_continue_to_breakpoint "break-here" ".* break-here .*"
> >
> > +# Test raw-frame-arguments on backtrace with and without frame-filter
> > +foreach filtered [list "enable" "disable"] {
> > +    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
> > +
> > +    gdb_test "bt 1" \
> > +    ".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> > +    "bt frame-filter=$filtered,pretty"
> > +
> > +    gdb_test "bt -raw-frame-arguments on 1" \
> > +    ".*foo \\(x=42, ss=\[.\]{3}\\).*" \
> > +    "bt frame-filter=$filtered,raw"
> > +
> > +    # "set print raw-values" should not affect frame arguments
> > +    gdb_test_no_output "set print raw-values on" \
> > +    "raw-values-on,frame-filter=$filtered"
> > +    gdb_test "bt 1" \
> > +    ".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> > +    "bt frame-filter=$filtered,pretty,raw-values"
> > +    gdb_test_no_output "set print raw-values off" \
> > +    "raw-values-off,frame-filter=$filtered"
> > +}
>
> You can use foreach_with_prefix, that will ensure test names are
> unique without having to include frame-filter=$filtered in each
> test name.  Something like this:
>
> # Test raw-frame-arguments on backtrace with and without frame-filter
> foreach_with_prefix filtered {enable disable} {
>     gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
>
>     gdb_test "bt 1" \
>     ".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
>     "bt pretty"
>
>     gdb_test "bt -raw-frame-arguments on 1" \
>     ".*foo \\(x=42, ss=\[.\]{3}\\).*" \
>     "bt raw"
>
>     # "set print raw-values" should not affect frame arguments
>     gdb_test_no_output "set print raw-values on"
>     gdb_test "bt 1" \
>     ".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
>     "bt pretty,raw-values"
>
>     gdb_test_no_output "set print raw-values off"
> }

I've changed it locally like this, thanks.


Hannes

Re: [PATCH 4/4] Fix raw-frame-arguments in combination with frame-filters

[Joel Brobecker]

Hi Hannes,

On Tue, Dec 29, 2020 at 06:02:27PM +0100, Hannes Domani via Gdb-patches wrote:
> Currently, if frame-filters are active, raw-values is used instead of
> raw-frame-arguments to decide if a pretty-printer should be invoked for
> frame arguments in a backtrace.
> 
> This adds the PRINT_RAW_FRAME_ARGUMENTS flag to frame_filter_flag which is
> then used in the frame-filter to override the raw flag in enumerate_args.
> 
> gdb/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* extension.h (enum frame_filter_flag): Add
> 	PRINT_RAW_FRAME_ARGUMENTS.
> 	* python/py-framefilter.c (enumerate_args): Override raw flag.
> 	raw_frame_args argument.
> 	(py_mi_print_variables): Forward raw flag.
> 	(py_print_args): Forward raw_frame_args flag.
> 	(py_print_frame): Handle PRINT_RAW_FRAME_ARGUMENTS.
> 	* stack.c (backtrace_command_1): Set PRINT_RAW_FRAME_ARGUMENTS.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* gdb.python/py-frame-args.exp: Add bt raw-frame-arguments tests.
> 	* gdb.python/py-frame-args.py: Add basic frame-filter.

I'm sure someone more versed in the Python extension API would
immediately understand what you mean. Unfortunately, we seeem to be
in short supply of this at the moment, as highlighted by the lack
of meaninful review. Simon seems to have tried to look at the patch,
and so did I, and one barrier to the review is the fact that we need
to understand what problem exactly you are trying to resolve. I think
an example, with behavior behavior, why this is wrong, and behavior
after, would go a long way towards lowering that barrier...

In fact, speaking more generally, I think that commit messages that
do not make any assumption as to the level of expertise of the
person looking at the patch are always desirable, as they are more
inclusive (being accessible to a larger population).

If you can provide that, I will try to review your patch next weekend
if someone else hasn't done so already.

> ---
>  gdb/extension.h                            |  4 ++++
>  gdb/python/py-framefilter.c                | 14 ++++++++++----
>  gdb/stack.c                                |  2 ++
>  gdb/testsuite/gdb.python/py-frame-args.exp | 22 ++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-frame-args.py  | 13 +++++++++++++
>  5 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/extension.h b/gdb/extension.h
> index c840dbc704..f3ec61b163 100644
> --- a/gdb/extension.h
> +++ b/gdb/extension.h
> @@ -103,6 +103,10 @@ enum frame_filter_flag
>  
>      /* Set this flag if elided frames should not be printed.  */
>      PRINT_HIDE = 1 << 5,
> +
> +    /* Set this flag if pretty printers for frame arguments should not
> +       be invoked.  */
> +    PRINT_RAW_FRAME_ARGUMENTS = 1 << 6,
>    };
>  
>  DEF_ENUM_FLAGS_TYPE (enum frame_filter_flag, frame_filter_flags);
> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
> index 944a0eee50..c61bfa4c2e 100644
> --- a/gdb/python/py-framefilter.c
> +++ b/gdb/python/py-framefilter.c
> @@ -422,12 +422,14 @@ static enum ext_lang_bt_status
>  enumerate_args (PyObject *iter,
>  		struct ui_out *out,
>  		enum ext_lang_frame_args args_type,
> +		bool raw_frame_args,
>  		int print_args_field,
>  		struct frame_info *frame)
>  {
>    struct value_print_options opts;
>  
>    get_user_print_options (&opts);
> +  opts.raw = raw_frame_args;
>  
>    if (args_type == CLI_SCALAR_VALUES)
>      {
> @@ -655,8 +657,8 @@ py_mi_print_variables (PyObject *filter, struct ui_out *out,
>    ui_out_emit_list list_emitter (out, "variables");
>  
>    if (args_iter != Py_None
> -      && (enumerate_args (args_iter.get (), out, args_type, 1, frame)
> -	  == EXT_LANG_BT_ERROR))
> +      && (enumerate_args (args_iter.get (), out, args_type, opts->raw, 1,
> +			  frame) == EXT_LANG_BT_ERROR))
>      return EXT_LANG_BT_ERROR;
>  
>    if (locals_iter != Py_None
> @@ -701,6 +703,7 @@ static enum ext_lang_bt_status
>  py_print_args (PyObject *filter,
>  	       struct ui_out *out,
>  	       enum ext_lang_frame_args args_type,
> +	       bool raw_frame_args,
>  	       struct frame_info *frame)
>  {
>    gdbpy_ref<> args_iter (get_py_iter_from_func (filter, "frame_args"));
> @@ -726,7 +729,8 @@ py_print_args (PyObject *filter,
>  	}
>      }
>    else if (args_iter != Py_None
> -	   && (enumerate_args (args_iter.get (), out, args_type, 0, frame)
> +	   && (enumerate_args (args_iter.get (), out, args_type,
> +			       raw_frame_args, 0, frame)
>  	       == EXT_LANG_BT_ERROR))
>      return EXT_LANG_BT_ERROR;
>  
> @@ -957,7 +961,9 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>       wrong.  */
>    if (print_args && (location_print || out->is_mi_like_p ()))
>      {
> -      if (py_print_args (filter, out, args_type, frame) == EXT_LANG_BT_ERROR)
> +      bool raw_frame_args = (flags & PRINT_RAW_FRAME_ARGUMENTS) != 0;
> +      if (py_print_args (filter, out, args_type, raw_frame_args, frame)
> +	  == EXT_LANG_BT_ERROR)
>  	return EXT_LANG_BT_ERROR;
>      }
>  
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 943b3db087..07abdea8c1 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2028,6 +2028,8 @@ backtrace_command_1 (const frame_print_options &fp_opts,
>      flags |= PRINT_LOCALS;
>    if (bt_opts.hide)
>      flags |= PRINT_HIDE;
> +  if (fp_opts.print_raw_frame_arguments)
> +    flags |= PRINT_RAW_FRAME_ARGUMENTS;
>  
>    if (!bt_opts.no_filters)
>      {
> diff --git a/gdb/testsuite/gdb.python/py-frame-args.exp b/gdb/testsuite/gdb.python/py-frame-args.exp
> index fd9c1f4342..7c621e1302 100644
> --- a/gdb/testsuite/gdb.python/py-frame-args.exp
> +++ b/gdb/testsuite/gdb.python/py-frame-args.exp
> @@ -34,6 +34,28 @@ gdb_test_no_output "source ${remote_python_file}" "load python file"
>  gdb_breakpoint [gdb_get_line_number "break-here"]
>  gdb_continue_to_breakpoint "break-here" ".* break-here .*"
>  
> +# Test raw-frame-arguments on backtrace with and without frame-filter
> +foreach filtered [list "enable" "disable"] {
> +    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
> +
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty"
> +
> +    gdb_test "bt -raw-frame-arguments on 1" \
> +	".*foo \\(x=42, ss=\[.\]{3}\\).*" \
> +	"bt frame-filter=$filtered,raw"
> +
> +    # "set print raw-values" should not affect frame arguments
> +    gdb_test_no_output "set print raw-values on" \
> +	"raw-values-on,frame-filter=$filtered"
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty,raw-values"
> +    gdb_test_no_output "set print raw-values off" \
> +	"raw-values-off,frame-filter=$filtered"
> +}
> +
>  # Test all combinations with raw off.
>  
>  gdb_test_no_output "set print raw-frame-arguments off"
> diff --git a/gdb/testsuite/gdb.python/py-frame-args.py b/gdb/testsuite/gdb.python/py-frame-args.py
> index c2908829c5..aed2070d94 100644
> --- a/gdb/testsuite/gdb.python/py-frame-args.py
> +++ b/gdb/testsuite/gdb.python/py-frame-args.py
> @@ -73,3 +73,16 @@ pretty_printers_dict = {}
>  
>  register_pretty_printers ()
>  gdb.pretty_printers.append (lookup_function)
> +
> +
> +class BasicFrameFilter (object):
> +    def __init__ (self):
> +        self.name = "BasicFrameFilter"
> +        self.priority = 100
> +        self.enabled = True
> +        gdb.frame_filters[self.name] = self
> +
> +    def filter (self, frame_iter):
> +        return frame_iter
> +
> +BasicFrameFilter ()
> -- 
> 2.29.2

-- 
Joel

Re: [PATCH 1/4] Fix wrong method name

[Eli Zaretskii]

> Date: Tue, 29 Dec 2020 18:02:24 +0100
> From: Hannes Domani via Gdb-patches <gdb-patches@sourceware.org>
> 
> The objects returned by FrameDecorator.frame_args need to implement a
> method named symbol, not argument.
> 
> gdb/doc/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* python.texi (Frame Decorator API): Fix method name.

OK (this is an obvious fix, for which you don't need approval).

Thanks.

Re: [PATCH 1/4] Fix wrong method name

[Hannes Domani]

 Am Dienstag, 29. Dezember 2020, 18:18:55 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:

> > Date: Tue, 29 Dec 2020 18:02:24 +0100
>
> > From: Hannes Domani via Gdb-patches <gdb-patches@sourceware.org>
> >
> > The objects returned by FrameDecorator.frame_args need to implement a
> > method named symbol, not argument.
> >
> > gdb/doc/ChangeLog:
> >
> > 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     * python.texi (Frame Decorator API): Fix method name.
>
>
> OK (this is an obvious fix, for which you don't need approval).

Pushed, thanks.


Hannes