Message ID | 20200108152550.112120-1-shahab.vahedi@gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 28638 invoked by alias); 8 Jan 2020 15:26:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28619 invoked by uid 89); 8 Jan 2020 15:26:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Received:a19 X-HELO: mail-lf1-f67.google.com Received: from mail-lf1-f67.google.com (HELO mail-lf1-f67.google.com) (209.85.167.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 08 Jan 2020 15:25:58 +0000 Received: by mail-lf1-f67.google.com with SMTP id f15so2720444lfl.13 for <gdb-patches@sourceware.org>; Wed, 08 Jan 2020 07:25:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=dD5V4Xc0HHQGCqGWJV3o2iZyjr57puSqsilulL6bp4U=; b=VEPm/6aR4ie+vz6h5svBPUNVUwhZyxJDzBLpsdw4zts+s+CSuAHs6qCzqISK4rS2On Ob9N9bz7Ks4ITXRy5E/T+hsK3xWAj8Hv3EqdrjAcUuaONr9tneqKsWCg9jWXbpSoyKXH NxawbbginueWhhpEDaLO+eRorvbX8eUOw0NnNa6E+OBiYK2dWKrNSEKKu/LnL82zxs2o 1qY3czjxykDYWAfK6pRfW9/QsU8BqxFc0+7U25fOlkYp2CnsHrNAEMWoOHdR97MYlWl+ JVDB36XM10g+Vz0ufSUJDvNHVaHCLHKShc82l05VIPZ/hjwPEjyRcM7UMKaG4H+GxK90 bOYw== Return-Path: <shahab.vahedi@gmail.com> Received: from archie.localdomain ([2a03:1b20:6:f011::2d]) by smtp.gmail.com with ESMTPSA id h24sm1427610ljl.80.2020.01.08.07.25.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Jan 2020 07:25:55 -0800 (PST) From: Shahab Vahedi <shahab.vahedi@gmail.com> To: gdb-patches@sourceware.org Cc: Shahab Vahedi <shahab@synopsys.com>, Pedro Alves <palves@redhat.com>, Andrew Burgess <andrew.burgess@embecosm.com>, Claudiu Zissulescu <claziss@synopsys.com>, Francois Bedard <fbedard@synopsys.com> Subject: [PATCH] GDB: Treat memory exception from tui_disassemble() gracefully Date: Wed, 8 Jan 2020 16:25:50 +0100 Message-Id: <20200108152550.112120-1-shahab.vahedi@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Shahab Vahedi
Jan. 8, 2020, 3:25 p.m. UTC
From: Shahab Vahedi <shahab@synopsys.com>
In TUI mode, when the assembly layout reaches the end of a binary,
GDB wants to disassemle the addresses beyond the last valid ones.
This results in a "MEMORY_ERROR" exception to be thrown when
tui_disasm_window::set_contents() invokes tui_disassemble(). When
that happens set_contents() bails out prematurely without filling
the "content" for the valid addresses. This eventually leads to
no assembly lines or termination of GDB when you scroll down to
the last lines of the program.
With this change, tui_disassemble() is surrounded with try/catch.
If a MEMORY_ERROR exception is caught, it is ignored.
The issue has been discussed at length in bug 25345:
https://sourceware.org/bugzilla/show_bug.cgi?id=25345
gdb/ChangeLog:
2020-01-08 Shahab Vahedi <shahab@synopsys.com>
* tui/tui-disasm.c (tui_disasm_window::set_contents):
Handle MEMORY_ERROR exception gracefully.
---
gdb/tui/tui-disasm.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
Comments
* Shahab Vahedi <shahab.vahedi@gmail.com> [2020-01-08 16:25:50 +0100]: > From: Shahab Vahedi <shahab@synopsys.com> > > In TUI mode, when the assembly layout reaches the end of a binary, > GDB wants to disassemle the addresses beyond the last valid ones. > This results in a "MEMORY_ERROR" exception to be thrown when > tui_disasm_window::set_contents() invokes tui_disassemble(). When > that happens set_contents() bails out prematurely without filling > the "content" for the valid addresses. This eventually leads to > no assembly lines or termination of GDB when you scroll down to > the last lines of the program. > > With this change, tui_disassemble() is surrounded with try/catch. > If a MEMORY_ERROR exception is caught, it is ignored. > > The issue has been discussed at length in bug 25345: > https://sourceware.org/bugzilla/show_bug.cgi?id=25345 > > gdb/ChangeLog: > 2020-01-08 Shahab Vahedi <shahab@synopsys.com> > * tui/tui-disasm.c (tui_disasm_window::set_contents): > Handle MEMORY_ERROR exception gracefully. Thanks for taking a look at this. I have some initial thoughts, maybe these are worth investigating before we merge this fix: - I wonder if the try/catch would be better inside tui_disassemble in some way. I don't know the exact implications of this, but tui_disassemble is called from ::set_contents and (through a couple of layers) ::do_scroll_vertical, surely we'd want both of these cases to not throw exceptions just because we hit unreadable memory? - While I write the above I realise this might be the reason of the failure you mention seeing, even with this patch, at the bug report above. - I also notice that there's some nasty over highlighting with this patch, all of the blank lines are, for some reason, marked as being "the line we're currently executing on", which is clearly wrong. I didn't dig into why this is, but we should probably try to fix this, or at least understand what the issue is here. This was discussed on IRC and it was pointed out that this bug also exists: https://sourceware.org/bugzilla/show_bug.cgi?id=9765 If/when this fix is merged you should be able to mark that resolved too! Thanks, Andrew > --- > gdb/tui/tui-disasm.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c > index 98c691f3387..7faaa45f039 100644 > --- a/gdb/tui/tui-disasm.c > +++ b/gdb/tui/tui-disasm.c > @@ -226,7 +226,18 @@ tui_disasm_window::set_contents (struct gdbarch *arch, > /* Get temporary table that will hold all strings (addr & insn). */ > std::vector<tui_asm_line> asm_lines (max_lines); > size_t addr_size = 0; > - tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size); > + try > + { > + tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size); > + } > + catch (const gdb_exception &except) > + { > + /* In cases where max_lines is asking tui_disassemble() to fetch > + too much, like when PC goes past the valid address range, a > + MEMORY_ERROR is thrown, but it is alright. */ > + if (except.error != MEMORY_ERROR) > + throw; > + } > > /* Align instructions to the same column. */ > insn_pos = (1 + (addr_size / tab_len)) * tab_len; > -- > 2.24.1 >
Hi Andrew, After a few iterations, I came up with a solution that is listed at the end of this mail. In my opinion, it works the best. You can see it in action here: https://sourceware.org/bugzilla/attachment.cgi?id=12178 It does not highlight the empty lines anymore or crash when it goes one window beyond the last instructions. Moreover, going to the top and right is fine too. The only downside is that last invalid address (the address just after the _last valid_ address) is repeated. If I try to remove it, the highlight issue kicks in. If you agree this is good enough, I will submit a V2 patch. diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c index 98c691f3387..3dfe70bc480 100644 --- a/gdb/tui/tui-disasm.c +++ b/gdb/tui/tui-disasm.c @@ -114,7 +114,18 @@ tui_disassemble (struct gdbarch *gdbarch, asm_lines[pos + i].addr_size = new_size; } - pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL); + try + { + pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL); + } + catch (const gdb_exception &except) + { + /* In cases where max_lines is asking tui_disassemble() to fetch + too much, like when PC goes past the valid address range, a + MEMORY_ERROR is thrown, but it is alright. */ + if (except.error != MEMORY_ERROR) + throw; + } asm_lines[pos + i].insn = std::move (gdb_dis_out.string ()); -- Shahab
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c index 98c691f3387..7faaa45f039 100644 --- a/gdb/tui/tui-disasm.c +++ b/gdb/tui/tui-disasm.c @@ -226,7 +226,18 @@ tui_disasm_window::set_contents (struct gdbarch *arch, /* Get temporary table that will hold all strings (addr & insn). */ std::vector<tui_asm_line> asm_lines (max_lines); size_t addr_size = 0; - tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size); + try + { + tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size); + } + catch (const gdb_exception &except) + { + /* In cases where max_lines is asking tui_disassemble() to fetch + too much, like when PC goes past the valid address range, a + MEMORY_ERROR is thrown, but it is alright. */ + if (except.error != MEMORY_ERROR) + throw; + } /* Align instructions to the same column. */ insn_pos = (1 + (addr_size / tab_len)) * tab_len;