From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id eyj/NFPHAWAhRQAAWB0awg (envelope-from ) for ; Fri, 15 Jan 2021 11:48:19 -0500 Received: by simark.ca (Postfix, from userid 112) id CC1FE1EF80; Fri, 15 Jan 2021 11:48:19 -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 E45011E945 for ; Fri, 15 Jan 2021 11:48:18 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4A6D439874DF; Fri, 15 Jan 2021 16:48:18 +0000 (GMT) Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id 17BFF3985479 for ; Fri, 15 Jan 2021 16:48:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 17BFF3985479 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-x431.google.com with SMTP id d26so9926148wrb.12 for ; Fri, 15 Jan 2021 08:48:14 -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=f1EerUugeKRvUhOMFfR7Y864PzebEktCL82hH7s2/ts=; b=LCuklGR3kQMXUo8BG5zz6nQM0AsxOaJb0Wl6n7M0btYylOve2fBUtv92SJ6cAb5V+x c52nXcSEFEYpdxUb75pKBaSr+WUenNP+Hl9ga2vu4dS/CgFII9KPxke0F0I7g8UWI6I1 LufngTUnaOY67fsrr8gFWm5npMzPcEBYawEzWijARniddw5YAU4zcIgvKTTKtjx7oQH0 rziMJDTnVLjtTQA4lb+7WsOornyvaR5J+nfPUnPa5ueUVv1+3fXopC9S5bHU5+wLAMi0 SIIuKxF2eMHqE503CdkGV4JfFSs/R+iuh0VISz+9poAsL81O6g7DSnp/sqH2DGAaWpMH Y5vQ== 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=f1EerUugeKRvUhOMFfR7Y864PzebEktCL82hH7s2/ts=; b=CV+4I79VWohOh1ZQ0Of5+paF5oHEk3q4ONoqrlnKCZG0bYzqWL63SrWWFvIcnKXy4s bTkKPurHmMCmvkDnmlc3bkDqZ2kFqoSqzKzMPcGWBc7gYJIYiEp6q5Gtgp71wfJ++3hw iJlgzZezYyVxolJYBPJ5ZM6VP0yLGMlKDZJ+i8hV8y57zPZ1yvbUYqhnJalLxgUYckLQ J1jnWFL1J85vh5HrTAVOs40/LGkYvzTEz9lgUMjJD5LfESsQ/24LePdsrfOsCK4iCHOG 26M4f7QgtIcxd37QDMDI6EgbeQSL9mwddvRMimZjNn19egALUufwMXJqu2IAHfRruOeJ 3kug== X-Gm-Message-State: AOAM531Pm5U2FJEelCyFx1HCeAqJI9HJUgGANkG7UKpYYNUMGjfypbX7 lsMKHLOif/7MxVZxAD3tCYwBgA== X-Google-Smtp-Source: ABdhPJweMZ/RUr4mqF9S3WgFh5gVW1teaWCtUdEXjL6ruD/uGPH7TZF5vFZd3V1XhWBI19/8L8LzvQ== X-Received: by 2002:a5d:4905:: with SMTP id x5mr13998583wrq.75.1610729293223; Fri, 15 Jan 2021 08:48:13 -0800 (PST) Received: from localhost (host86-166-129-230.range86-166.btcentralplus.com. [86.166.129.230]) by smtp.gmail.com with ESMTPSA id a25sm12916335wmb.25.2021.01.15.08.48.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Jan 2021 08:48:12 -0800 (PST) Date: Fri, 15 Jan 2021 16:48:11 +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: <20210115164811.GK265215@embecosm.com> References: <20201229170227.821-1-ssbssa@yahoo.de> <20201229170227.821-2-ssbssa@yahoo.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201229170227.821-2-ssbssa@yahoo.de> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 16:23:43 up 37 days, 21:08, 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" 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 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)> : New member function. (class tui_py_window) : Likewise. (REQUIRE_WINDOW): Call is_valid member function. (gdbpy_tui_is_valid): Likewise. * tui/tui-data.h (struct tui_win_info) : 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 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 . */ + +#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 . + +# 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 . + +# 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