From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com [IPv6:2a00:1450:4864:20::32c]) by sourceware.org (Postfix) with ESMTPS id 8D7A03857C7B for ; Mon, 8 Mar 2021 09:32:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8D7A03857C7B 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-wm1-x32c.google.com with SMTP id m20-20020a7bcb940000b029010cab7e5a9fso3355465wmi.3 for ; Mon, 08 Mar 2021 01:32:09 -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=1WzzZsBWvHtW2ekBUy9Vpw1ZZ01BD9V1N/EIgW2aTxg=; b=CKOeWTlOEkVfcvDLxf28eU+XWvmN8tQirtnaNvCjC7gY1/Hy7Qx5ayzrs/VGB7VZDU D9vXDmgNndyywCI0opOUu+vvN4FyJe0Yh6R+2KKEAMPPsI+RdLjHPwSwiFEauFt8oTz1 VCqTGeIDijXyBHXG/nN6dRkAgxB6L7/U0SEYl4Oek9ih9O6/y1EdcHs7sNexXzL8dFc3 tNIaTx6EKqsieQi9dIEn9WkRkWNk9gj+doR0IOckD6szlXEUvNwjF8+reiYqKRrCG//9 QX42OG2SMmpSKsMwCRqX4sIIa07ZNVEEFSwGDRtRNya4kFx9+avX0mhh37+0RgSOcA2f v7lQ== 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=1WzzZsBWvHtW2ekBUy9Vpw1ZZ01BD9V1N/EIgW2aTxg=; b=F0e0X+s/b2v5O0N7jVmpfqjLBIc0ZebAdWoedwJPStKIlaFzJUXNd3gPMSs9k4jQWU WeTG8hzTwCZsxZk6SYPSBxUPRkhSuPYYb/F520ahDyrXbAvJmIsE7zAtdTHJR03kvYDw irrfJTO8bSKEJow2+24aQw5vW9DBYXnLIVkH5i/qbNkeqp/whJjq6un1kgd4OWsB+j1X s5wtnJrcP6vHov9+ie+XkamDOlrCSu4mezgFj1wCEhAL2/NVuH6GyRIk5DOcdeDVoMiW lXv6fRxrfldppHWCnt+CmIbOBkwokGICfdMlD2a1636TVwcBMZwBXrwLDdhni7Fbd3fm JZhA== X-Gm-Message-State: AOAM531PxX32FfDfqDic2icSPbwl0tdJEGCTFQXreuU1j1BYgpxWjSbw TLemjYF+a3LV3OzoU9oDBfmWZA== X-Google-Smtp-Source: ABdhPJx7cGq8GbPqWD2U5098G//sLOINTMG0Gq0d7K73sMw1AUD9WVY4+W0qHX74Ej+Hxo31MqlyWw== X-Received: by 2002:a1c:68c5:: with SMTP id d188mr20522388wmc.119.1615195928398; Mon, 08 Mar 2021 01:32:08 -0800 (PST) Received: from localhost (host86-134-238-232.range86-134.btcentralplus.com. [86.134.238.232]) by smtp.gmail.com with ESMTPSA id h62sm18140768wmf.37.2021.03.08.01.32.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Mar 2021 01:32:07 -0800 (PST) Date: Mon, 8 Mar 2021 09:32:06 +0000 From: Andrew Burgess To: Hannes Domani Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 02/22] Create/disable/enable/delete breakpoints in TUI with mouse Message-ID: <20210308093206.GI1720904@embecosm.com> References: <20210306173417.21528-1-ssbssa@yahoo.de> <20210306173417.21528-3-ssbssa@yahoo.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210306173417.21528-3-ssbssa@yahoo.de> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 09:10:56 up 89 days, 13:55, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Mon, 08 Mar 2021 09:32:11 -0000 * Hannes Domani via Gdb-patches [2021-03-06 18:33:57 +0100]: > Target area is the first 3 columns of the TUI source window, where the > breakpoint status is shown. > > On a left click, this either creates a new breakpoint, or if one already > exists, inverts the enabled state of it. > And a middle click deletes the breakpoint. Thanks for working on this feature, this looks really exciting. The same breakpoint markers are used for the source and assembler windows, both of which inherit from the tui_source_window_base class. It would be nice if this breakpoint/mouse toggling support was supported for both of these windows, possibly by moving the click detection up a level in the class hierarchy? The documentation for the TUI is not very extensive, but the source/assembler windows, and their breakpoint markers are discussed in the 'TUI Overview' section. I think this change should be documented there, as well as mentioned in the NEWS file - this is certainly a note worthy new feature. Finally, I think we should consider testing. I don't know if it is possible to send ctrl sequences to the console to emulate mouse clicks, but if not then I would suggest this: add a new command 'maintenance tui click X Y BUTTON', which injects a button click into GDB, X/Y being the click coordinate within the whole GDB terminal, and BUTTON being the button number. Using this it should be possible to test the functionality in this patch. I have a few style nits I've outlined below too. > --- > gdb/tui/tui-source.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ > gdb/tui/tui-source.h | 2 ++ > 2 files changed, 73 insertions(+) > > diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c > index 69f51ceb6f1..c70485f9612 100644 > --- a/gdb/tui/tui-source.c > +++ b/gdb/tui/tui-source.c > @@ -29,6 +29,7 @@ > #include "objfiles.h" > #include "filenames.h" > #include "source-cache.h" > +#include "linespec.h" > > #include "tui/tui.h" > #include "tui/tui-data.h" > @@ -237,3 +238,73 @@ tui_source_window::show_line_number (int offset) const > xsnprintf (text, sizeof (text), "%*d ", m_digits - 1, lineno); > waddstr (handle.get (), text); > } > + > +void > +tui_source_window::click (int mouse_x, int mouse_y, int mouse_button) > +{ This function needs a header comment. This will be something like: /* See ..... */ to point to the header file where the function is declared. > + if ((mouse_button == 1 || mouse_button == 2) && mouse_x <= 2) > + { > + int line = m_start_line_or_addr.u.line_no + mouse_y; > + gdb::unique_xmalloc_ptr bp_str; > + bp_str.reset (xstrprintf ("%s:%d", m_fullname.get (), line)); > + > + struct linespec_sals lsal; > + lsal.canonical = NULL; GDB prefers nullptr to NULL. However, you could probably add a new patch to give `canonical` a default value of `nullptr` in the linespec.h file, this might be nicer than having to initialise it here. The NULL -> nullptr thing applies through out this patch. > + try > + { > + lsal.sals = decode_line_with_current_source (bp_str.get (), 0); > + } > + catch (const gdb_exception_error &e) > + { > + } > + > + if (lsal.sals.size () == 1) > + { > + symtab_and_line &sal = lsal.sals[0]; > + > + breakpoint *bp; > + bp = iterate_over_breakpoints ([&] (breakpoint *b) -> bool > + { > + struct bp_location *loc; > + > + for (loc = b->loc; loc != NULL; loc = loc->next) > + { > + if (loc->symtab != NULL > + && loc->line_number == sal.line > + && filename_cmp (m_fullname.get (), > + symtab_to_fullname (loc->symtab)) == 0) > + return true; > + } > + > + return false; > + }); > + > + if (mouse_button == 2) > + { > + if (bp) In GDB we always explicitly compare to nullptr, i.e. 'if (bp != nullptr)`. This applies below too. > + delete_breakpoint (bp); > + } > + else if (mouse_button == 1 && bp) > + { > + if (bp->enable_state == bp_disabled) > + enable_breakpoint (bp); > + else > + disable_breakpoint (bp); > + } > + else if (mouse_button == 1) > + { > + const char *loc_str = bp_str.get (); > + event_location_up location = string_to_event_location_basic > + (&loc_str, current_language, > + symbol_name_match_type::WILD); > + > + reinitialize_more_filter (); > + > + create_breakpoint (m_gdbarch, location.get (), NULL, -1, > + NULL, 0, 0, bp_breakpoint, 0, > + AUTO_BOOLEAN_FALSE, > + &bkpt_breakpoint_ops, 0, 1, 0, 0); > + } > + } > + } > +} > diff --git a/gdb/tui/tui-source.h b/gdb/tui/tui-source.h > index 4c69b87e8e3..7a395d3eb9e 100644 > --- a/gdb/tui/tui-source.h > +++ b/gdb/tui/tui-source.h > @@ -56,6 +56,8 @@ struct tui_source_window : public tui_source_window_base > void display_start_addr (struct gdbarch **gdbarch_p, > CORE_ADDR *addr_p) override; > > + void click (int mouse_x, int mouse_y, int mouse_button) override; This is probably where the click function should be documented to explain what this override does. Thanks, Andrew > + > protected: > > void do_scroll_vertical (int num_to_scroll) override; > -- > 2.30.1 >