From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id oiAwNP8fBGApfAAAWB0awg (envelope-from ) for ; Sun, 17 Jan 2021 06:31:11 -0500 Received: by simark.ca (Postfix, from userid 112) id C8F921EF80; Sun, 17 Jan 2021 06:31:11 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.4 required=5.0 tests=DKIM_SIGNED,MAILING_LIST_MULTI, RDNS_NONE,T_DKIM_INVALID,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D8D311E940 for ; Sun, 17 Jan 2021 06:31:10 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 34CCD389042E; Sun, 17 Jan 2021 11:31:10 +0000 (GMT) Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id A25D2386EC55 for ; Sun, 17 Jan 2021 11:31:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A25D2386EC55 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42c.google.com with SMTP id a12so13703556wrv.8 for ; Sun, 17 Jan 2021 03:31:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=JG7WnSjYPkOQ5XnfLrdyiD/PuWCq24x8QR3zMwBKyM4=; b=D7hBd9VkjvPLjaAFnh3ElCL7fQkx8O+9cOu8goRsq1n4UMXaJ0hVO3vX1/ixUuMN/5 wJr3mF+GhSYWloCDeYlaBZgZK1zyKfdEn6dIZ/YBCeJBsvrheZxWSaA9wP0Iw8AI01u1 z2NOVzNigy1rfjkbiibC3azLcJnxu+y7T5YF1ANpthm/VQK7U+x7Bd4zGwVMivdNueoE 4OQsNMIjGyJN1PDEbR4Zlkv5YeP0cB+YsSZhXW5a8zro8/8ocBajS6C+n6/jmhP0btMY yn8XLAMtwvcZPiXCkTjOkkaMXFV0dY2SodS4ii7Rpm9X3SMX38JsClST7XyR8gf9WRAN eFdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JG7WnSjYPkOQ5XnfLrdyiD/PuWCq24x8QR3zMwBKyM4=; b=gKktBdIt4Fom7umckvtGf2MaLtvLLVEGQPiFKxdVeOGkd3XotiP/T7QK7qy042aRl/ uDyhhg65vvKMOGD+8TVTyoiIOmYW02UM7GZoXAQNSKd++Cq7Gvmz3Gh58Xy+On7Yabro Gwwnfm/Ex4TG3ozrX1R1nChsS7qeDRke2xG/2R2RalTUgRMO24bFkXHw+vbTEOKKtxAg Tw15nUi9OjcpGjf8vPDGnSSuexz0RzGO67CI5xn/5CWdKe4TNZvttvyaqiQRe2rE0tc2 h2/DSWGBvVzz2u0KS3BzW2eHjMsCZcWmhwTK7lOUPVSV59Dsfkpd/BHxWrrYm2YUwFoB h6rg== X-Gm-Message-State: AOAM533BBdnrVhK3L7XegAVRW1XExXVMkeZFLpdrkyiNrqrKtiMPxGgV XxQT947uQqSCMX8QVgaLUix15WYjHMPcVw== X-Google-Smtp-Source: ABdhPJwlNk2bujlZ4n8GzYEaQlusufNxFDO0wc62Jl8VMfVth6yfx6nDA+w933cPFWt7+XNjSEX0iA== X-Received: by 2002:a5d:4f0e:: with SMTP id c14mr21338336wru.84.1610883064545; Sun, 17 Jan 2021 03:31:04 -0800 (PST) Received: from localhost (host86-166-129-230.range86-166.btcentralplus.com. [86.166.129.230]) by smtp.gmail.com with ESMTPSA id z15sm23460605wrv.67.2021.01.17.03.31.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 17 Jan 2021 03:31:03 -0800 (PST) Date: Sun, 17 Jan 2021 11:31:02 +0000 From: Andrew Burgess To: Hannes Domani Subject: Re: [PATCH 2/4] Return true in TuiWindow.is_valid only if TUI is enabled Message-ID: <20210117113102.GM265215@embecosm.com> References: <20201229170227.821-1-ssbssa@yahoo.de> <20201229170227.821-2-ssbssa@yahoo.de> <20210115164811.GK265215@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210115164811.GK265215@embecosm.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 11:30:06 up 39 days, 16:14, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Tom Tromey , gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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 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)> : 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) : 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) -/* 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 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 . */ + +#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 . + +# 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 . + +# 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. */