Message ID | 20230411082332.25052-2-tdevries@suse.de |
---|---|
State | Committed |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 959B13857703 for <patchwork@sourceware.org>; Tue, 11 Apr 2023 08:23:58 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 959B13857703 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1681201438; bh=vUZLntWeiTa4aCde9YHVTcfT72AZYJH0JaEAh8cAmYs=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=WwZzovjHn/v49VqDfjkDkH7vmcmJbfQ3NyHKwK29dszIIk0KFobWNkcRQXGOVdoUu JHoPS9Gh/IbllQ7a5oalQ2wft8oVSbQQHx9D1v7rXhs4q57tljbXEx+VaUp3oFDVEs /jpxfQY6pJzyWG7heYtV+5WeyGt40gJY1tkuYCpo= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 052823858D38 for <gdb-patches@sourceware.org>; Tue, 11 Apr 2023 08:23:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 052823858D38 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 35B721FE02; Tue, 11 Apr 2023 08:23:24 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 1D1E1139AF; Tue, 11 Apr 2023 08:23:24 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id kHQYBvwYNWRpBAAAMHmgww (envelope-from <tdevries@suse.de>); Tue, 11 Apr 2023 08:23:24 +0000 To: gdb-patches@sourceware.org Cc: Tom Tromey <tom@tromey.com> Subject: [PATCH 2/3] [gdb/tui] Fix left margin in disassembly window Date: Tue, 11 Apr 2023 10:23:31 +0200 Message-Id: <20230411082332.25052-2-tdevries@suse.de> X-Mailer: git-send-email 2.35.3 In-Reply-To: <20230411082332.25052-1-tdevries@suse.de> References: <20230411082332.25052-1-tdevries@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Tom de Vries <tdevries@suse.de> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
[1/3,gdb/tui] Add maint set/show tui-left-margin-verbose
|
|
Commit Message
Tom de Vries
April 11, 2023, 8:23 a.m. UTC
With a hello world a.out, and maint set tui-left-margin-verbose on, we have this disassembly window: ... ┌───────────────────────────────────────────────────────────┐ │___ 0x555555555149 <main> endbr64 │ │___ 0x55555555514d <main+4> push %rbp │ │___ 0x55555555514e <main+5> mov %rsp,%rbp │ │B+> 0x555555555151 <main+8> lea 0xeac(%rip),%rax│ │___ 0x555555555158 <main+15> mov %rax,%rdi │ ... Note the space between "B+>" and 0x555555555151. The space shows that a bit of the left margin is not written, which is a problem because that location is showing a character previously written, which happens to be a space, but also may be something else, for instance a '[' as reported in PR tui/30325. The problem is caused by confusion about the meaning of: ... #define TUI_EXECINFO_SIZE 4 ... There's the meaning of defining the size of this zero-terminated char array: ... char element[TUI_EXECINFO_SIZE]; ... which is used to print the "B+>" bit, which is 3 chars wide. And there's the meaning of defining part of the size of the left margin: ... int left_margin () const { return 1 + TUI_EXECINFO_SIZE + extra_margin (); } ... where it represents 4 chars. The discrepancy between the two causes the space between "B+>" and "0x555555555151". Fix this by redefining TUI_EXECINFO_SIZE to 3, and using: ... char element[TUI_EXECINFO_SIZE + 1]; ... such that we have: ... |B+>0x555555555151 <main+8> lea 0xeac(%rip),%rax │ ... This changes the layout of the disassembly window back to what it was before commit 9e820dec13e ("Use a curses pad for source and disassembly windows"), the commit that introduced the PR30325 regression. This also changes the source window from: ... │___000005__{ | ... to: ... │___000005_{ | ... Tested on x86_64-linux. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30325 --- gdb/tui/tui-winsource.c | 7 ++++--- gdb/tui/tui-winsource.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
Comments
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> Fix this by redefining TUI_EXECINFO_SIZE to 3, and using:
Tom> ...
Tom> char element[TUI_EXECINFO_SIZE + 1];
Tom> ...
Tom> such that we have:
Tom> ...
Tom> |B+>0x555555555151 <main+8> lea 0xeac(%rip),%rax │
Tom> ...
Looks good. Thank you for finding this.
Tom
Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes: > With a hello world a.out, and maint set tui-left-margin-verbose on, we have > this disassembly window: > ... > ┌───────────────────────────────────────────────────────────┐ > │___ 0x555555555149 <main> endbr64 │ > │___ 0x55555555514d <main+4> push %rbp │ > │___ 0x55555555514e <main+5> mov %rsp,%rbp │ > │B+> 0x555555555151 <main+8> lea 0xeac(%rip),%rax│ > │___ 0x555555555158 <main+15> mov %rax,%rdi │ > ... > Note the space between "B+>" and 0x555555555151. The space shows that a bit > of the left margin is not written, which is a problem because that location is > showing a character previously written, which happens to be a space, but also > may be something else, for instance a '[' as reported in PR tui/30325. > > The problem is caused by confusion about the meaning of: > ... > #define TUI_EXECINFO_SIZE 4 > ... > > There's the meaning of defining the size of this zero-terminated char array: > ... > char element[TUI_EXECINFO_SIZE]; > ... > which is used to print the "B+>" bit, which is 3 chars wide. > > And there's the meaning of defining part of the size of the left margin: > ... > int left_margin () const > { return 1 + TUI_EXECINFO_SIZE + extra_margin (); } > ... > where it represents 4 chars. > > The discrepancy between the two causes the space between "B+>" and > "0x555555555151". > > Fix this by redefining TUI_EXECINFO_SIZE to 3, and using: > ... > char element[TUI_EXECINFO_SIZE + 1]; > ... > such that we have: > ... > |B+>0x555555555151 <main+8> lea 0xeac(%rip),%rax │ > ... > > This changes the layout of the disassembly window back to what it was before > commit 9e820dec13e ("Use a curses pad for source and disassembly windows"), > the commit that introduced the PR30325 regression. > > This also changes the source window from: > ... > │___000005__{ | > ... > to: > ... > │___000005_{ | > ... > > Tested on x86_64-linux. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30325 > --- > gdb/tui/tui-winsource.c | 7 ++++--- > gdb/tui/tui-winsource.h | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) I'm surprised there's no new test for this. I kind of assumed the new maint command (from patch #1) was to make writing the test easier... Thanks, Andrew > > diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c > index 6c69fb7a907..84f9d97c554 100644 > --- a/gdb/tui/tui-winsource.c > +++ b/gdb/tui/tui-winsource.c > @@ -666,12 +666,13 @@ tui_source_window_base::update_exec_info (bool refresh_p) > for (int i = 0; i < m_content.size (); i++) > { > struct tui_source_element *src_element = &m_content[i]; > - char element[TUI_EXECINFO_SIZE]; > + /* Add 1 for '\0'. */ > + char element[TUI_EXECINFO_SIZE + 1]; > /* Initialize all but last element. */ > char space = tui_left_margin_verbose ? '_' : ' '; > - memset (element, space, TUI_EXECINFO_SIZE - 1); > + memset (element, space, TUI_EXECINFO_SIZE); > /* Initialize last element. */ > - element[TUI_EXECINFO_SIZE - 1] = '\0'; > + element[TUI_EXECINFO_SIZE] = '\0'; > > /* Now update the exec info content based upon the state > of each line as indicated by the source content. */ > diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h > index 7370ae95d8b..a8ff94f5769 100644 > --- a/gdb/tui/tui-winsource.h > +++ b/gdb/tui/tui-winsource.h > @@ -58,7 +58,7 @@ DEF_ENUM_FLAGS_TYPE (enum tui_bp_flag, tui_bp_flags); > #define TUI_BP_HIT_POS 0 > #define TUI_BP_BREAK_POS 1 > #define TUI_EXEC_POS 2 > -#define TUI_EXECINFO_SIZE 4 > +#define TUI_EXECINFO_SIZE 3 > > /* Elements in the Source/Disassembly Window. */ > struct tui_source_element > -- > 2.35.3
On 4/12/23 23:06, Andrew Burgess wrote: > I'm surprised there's no new test for this. I kind of assumed the new > maint command (from patch #1) was to make writing the test easier... > It certainly could be used for that, yes. My primary purpose though was to make the problem easily visible. I haven't added a test because I'm still fixing issues I find with the current testsuite... I might add a test-case for this eventually, or hijack one of the existing tests. Thanks, - Tom
Tom de Vries <tdevries@suse.de> writes: > On 4/12/23 23:06, Andrew Burgess wrote: >> I'm surprised there's no new test for this. I kind of assumed the new >> maint command (from patch #1) was to make writing the test easier... >> > > It certainly could be used for that, yes. My primary purpose though was > to make the problem easily visible. > > I haven't added a test because I'm still fixing issues I find with the > current testsuite... > > I might add a test-case for this eventually, or hijack one of the > existing tests. I don't understand -- aren't you proposing this patch for inclusion now? Patches should be accompanied by a corresponding test. I really think this should have a test before it's merged. Thanks, Andrew
Andrew Burgess <aburgess@redhat.com> writes: > Tom de Vries <tdevries@suse.de> writes: > >> On 4/12/23 23:06, Andrew Burgess wrote: >>> I'm surprised there's no new test for this. I kind of assumed the new >>> maint command (from patch #1) was to make writing the test easier... >>> >> >> It certainly could be used for that, yes. My primary purpose though was >> to make the problem easily visible. >> >> I haven't added a test because I'm still fixing issues I find with the >> current testsuite... >> >> I might add a test-case for this eventually, or hijack one of the >> existing tests. > > I don't understand -- aren't you proposing this patch for inclusion now? > > Patches should be accompanied by a corresponding test. > > I really think this should have a test before it's merged. Oh, I just noticed you merged this anyway. GDB development is usually done by consensus. That would mean if someone raises a concern -- like a test is missing -- then you don't merge a patch until the concern has been discussed and addressed. I've had plenty of patches on the list that have had an approval, and then a concern raised by someone else. I don't just choose to see the approval and merge the patch regardless -- that's just disrespectful to a pretty legitimate concern I had about this patch. I know writing tests can be a real drag, but it really is critical to making GDB a solid product. Hopefully you can prioritise getting a test written for this patch. Thanks, Andrew
On 4/13/23 11:25, Andrew Burgess wrote: > Andrew Burgess <aburgess@redhat.com> writes: > >> Tom de Vries <tdevries@suse.de> writes: >> >>> On 4/12/23 23:06, Andrew Burgess wrote: >>>> I'm surprised there's no new test for this. I kind of assumed the new >>>> maint command (from patch #1) was to make writing the test easier... >>>> >>> >>> It certainly could be used for that, yes. My primary purpose though was >>> to make the problem easily visible. >>> >>> I haven't added a test because I'm still fixing issues I find with the >>> current testsuite... >>> >>> I might add a test-case for this eventually, or hijack one of the >>> existing tests. >> >> I don't understand -- aren't you proposing this patch for inclusion now? >> >> Patches should be accompanied by a corresponding test. >> >> I really think this should have a test before it's merged. > > Oh, I just noticed you merged this anyway. > > GDB development is usually done by consensus. That would mean if > someone raises a concern -- like a test is missing -- then you don't > merge a patch until the concern has been discussed and addressed. > > I've had plenty of patches on the list that have had an approval, and > then a concern raised by someone else. I don't just choose to see the > approval and merge the patch regardless -- that's just disrespectful to > a pretty legitimate concern I had about this patch. > Hi Andrew, I understand that the sequence of events can be read as such, but I went ahead and committed after seeing the approval by Tom, without seeing your reply, sorry about that. I only saw it when replying to Eli. > I know writing tests can be a real drag, but it really is critical to > making GDB a solid product. > I agree. And I don't consider it a drag, it's just that I felt a bit overwhelmed with other work (that also tries to make GDB a solid product). > Hopefully you can prioritise getting a test written for this patch. Yes, now that this ( https://sourceware.org/pipermail/gdb-patches/2023-April/198848.html ) is out of the door, I'll take a look. Thanks, - Tom
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c index 6c69fb7a907..84f9d97c554 100644 --- a/gdb/tui/tui-winsource.c +++ b/gdb/tui/tui-winsource.c @@ -666,12 +666,13 @@ tui_source_window_base::update_exec_info (bool refresh_p) for (int i = 0; i < m_content.size (); i++) { struct tui_source_element *src_element = &m_content[i]; - char element[TUI_EXECINFO_SIZE]; + /* Add 1 for '\0'. */ + char element[TUI_EXECINFO_SIZE + 1]; /* Initialize all but last element. */ char space = tui_left_margin_verbose ? '_' : ' '; - memset (element, space, TUI_EXECINFO_SIZE - 1); + memset (element, space, TUI_EXECINFO_SIZE); /* Initialize last element. */ - element[TUI_EXECINFO_SIZE - 1] = '\0'; + element[TUI_EXECINFO_SIZE] = '\0'; /* Now update the exec info content based upon the state of each line as indicated by the source content. */ diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h index 7370ae95d8b..a8ff94f5769 100644 --- a/gdb/tui/tui-winsource.h +++ b/gdb/tui/tui-winsource.h @@ -58,7 +58,7 @@ DEF_ENUM_FLAGS_TYPE (enum tui_bp_flag, tui_bp_flags); #define TUI_BP_HIT_POS 0 #define TUI_BP_BREAK_POS 1 #define TUI_EXEC_POS 2 -#define TUI_EXECINFO_SIZE 4 +#define TUI_EXECINFO_SIZE 3 /* Elements in the Source/Disassembly Window. */ struct tui_source_element