From patchwork Sat Jan 11 01:59:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 37300 Received: (qmail 55799 invoked by alias); 11 Jan 2020 02:00:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55757 invoked by uid 89); 11 Jan 2020 02:00:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.1 spammy=sk:single_, helloworld X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 11 Jan 2020 01:59:59 +0000 Received: by mail-wm1-f65.google.com with SMTP id w5so5558590wmi.1 for ; Fri, 10 Jan 2020 17:59:59 -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:user-agent; bh=csQiFCICySO2A743Rqp1f2sBrCkKexZXU+uzSRDoiK0=; b=AEpx7sAaQ3WBEmnPTTrCzI4gshr3AvxL3Rt0QBaivPgBU/q6HHY8yllowPqq0HIf9+ lYMrlieqCr6ot+FfhBtaxuRRgMr0+WkUibWllqcX1ACAB+kMl8bCgYMmscHUsmdfD4mm CingHPHQbuYJ/zauScGJuIfrzrm3eLeMWDy7sxOmzVu2VnuGswGPzoJBZIKC6Aq85SWM ZNyFyyIz9h6mgZZguFrKAPW3sOsRjzREeaeiqywtn1L8gfxOnLTLXbN7qK4074EtobX2 fnaQ0KoPzoEhLVj1rpQpiICfGnHjLCYlW4hN0RUP9Nif5aTt98TjglXg7ZQqXhfbWpEU tL0A== Return-Path: Received: from localhost (host86-186-80-236.range86-186.btcentralplus.com. [86.186.80.236]) by smtp.gmail.com with ESMTPSA id l6sm4659796wmf.21.2020.01.10.17.59.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 10 Jan 2020 17:59:56 -0800 (PST) Date: Sat, 11 Jan 2020 01:59:55 +0000 From: Andrew Burgess To: Shahab Vahedi Cc: gdb-patches@sourceware.org, Shahab Vahedi , Pedro Alves , Tom Tromey , Claudiu Zissulescu , Francois Bedard Subject: Re: [PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file Message-ID: <20200111015955.GA3865@embecosm.com> References: <20200110115728.13940-1-shahab.vahedi@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200110115728.13940-1-shahab.vahedi@gmail.com> X-Fortune: A woman was in love with fourteen soldiers. It was clearly platoonic. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes Shahab, I took a look at you patch last night, and there were a couple of things I thought should be tweaked. I wanted to remove the trailing addresses with no content, and ideally have the window stop scrolling at the last valid instruction - much like the source window does with its content. I also saw in a test here that I couldn't scroll back to the first instruction (test file was single asm file containing 35 1-byte nop instructions). I initially intended to just suggest how you might do that, but in trying to figure out what to suggest I ended up with the patch below. This still needs some more work - I was initially testing this with an asm file containing ~35 nop instructions, and just scrolling that up and down, and this worked fine. But then I took a look at: https://sourceware.org/bugzilla/show_bug.cgi?id=9765 and tried scrolling up and down in a helloworld binary. For some reason on _that_ test the scrolling seems to get "stuck" at the end, and when I use Page-Up to scroll up I see the test get "stuck" alternating between two addresses and not actually making progress at scrolling up. In summary, I think the patch below would be a good starting point, but it needs some more work to fix the last few nits. I'll take a look at this tomorrow or Sunday. [ NOTE: I've been using Pedro's patch to prevent exceptions entering readline in my tree too. ] Feedback welcome, Thanks, Andrew --- commit 354f0a9f7a9c0edfb862d43656c21119fe9007e2 Author: Andrew Burgess Date: Sat Jan 11 01:38:28 2020 +0000 gdb/tui: Make asm window handle reading invalid memory When scrolling the asm window, if we try to disassemble invalid memory this should not prevent the window displaying something sane for those addresses that can be disassembled.x gdb/ChangeLog: * tui/tui-disasm.c (tui_disassemble): Update header comment, remove unneeded parameter, add try/catch around gdb_print_insn, rewrite to add items to asm_lines vector. (tui_find_disassembly_address): Don't assume the length of asm_lines after calling tui_disassemble, update for changes in tui_disassemble API. (tui_disasm_window::set_contents): Likewise. Change-Id: I292f4b8d571516ca8b25d9d60c85228c98222086 diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c index 98c691f3387..eeecaec43bf 100644 --- a/gdb/tui/tui-disasm.c +++ b/gdb/tui/tui-disasm.c @@ -81,13 +81,21 @@ len_without_escapes (const std::string &str) return len; } -/* Function to set the disassembly window's content. - Disassemble count lines starting at pc. - Return address of the count'th instruction after pc. */ +/* Function to disassemble up to COUNT instructions starting from address + PC into the ASM_LINES vector. Return the address of the count'th + instruction after pc. When ADDR_SIZE is non-null then place the + maximum size of an address and label into the value pointed to by + ADDR_SIZE, and set the addr_size field on each item in ASM_LINES, + otherwise the addr_size fields within asm_lines are undefined. + + It is worth noting that ASM_LINES might not have COUNT entries when + this function returns. If the disassembly is truncated for some other + reason, for example, we hit invalid memory, then ASM_LINES can have + fewer entries than requested. */ static CORE_ADDR tui_disassemble (struct gdbarch *gdbarch, std::vector &asm_lines, - CORE_ADDR pc, int pos, int count, + CORE_ADDR pc, int count, size_t *addr_size = nullptr) { bool term_out = source_styling && gdb_stdout->can_emit_style_escape (); @@ -96,10 +104,31 @@ tui_disassemble (struct gdbarch *gdbarch, /* Now construct each line. */ for (int i = 0; i < count; ++i) { - print_address (gdbarch, pc, &gdb_dis_out); - asm_lines[pos + i].addr = pc; - asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ()); + tui_asm_line tal; + CORE_ADDR orig_pc = pc; + try + { + pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL); + } + catch (const gdb_exception &except) + { + /* If pc points to an invalid address then we'll catch a + MEMORY_ERROR here, this should stop the disassembly, but + otherwise is fine. */ + if (except.error != MEMORY_ERROR) + throw; + return pc; + } + + /* Capture the disassembled instruction. */ + tal.insn = std::move (gdb_dis_out.string ()); + gdb_dis_out.clear (); + + /* And capture the address the instruction is at. */ + tal.addr = orig_pc; + print_address (gdbarch, orig_pc, &gdb_dis_out); + tal.addr_string = std::move (gdb_dis_out.string ()); gdb_dis_out.clear (); if (addr_size != nullptr) @@ -107,19 +136,14 @@ tui_disassemble (struct gdbarch *gdbarch, size_t new_size; if (term_out) - new_size = len_without_escapes (asm_lines[pos + i].addr_string); + new_size = len_without_escapes (tal.addr_string); else - new_size = asm_lines[pos + i].addr_string.size (); + new_size = tal.addr_string.size (); *addr_size = std::max (*addr_size, new_size); - asm_lines[pos + i].addr_size = new_size; + tal.addr_size = new_size; } - pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL); - - asm_lines[pos + i].insn = std::move (gdb_dis_out.string ()); - - /* Reset the buffer to empty. */ - gdb_dis_out.clear (); + asm_lines.push_back (tal); } return pc; } @@ -137,41 +161,54 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from) if (max_lines <= 1) return pc; - std::vector asm_lines (max_lines); + std::vector asm_lines; new_low = pc; if (from > 0) { - tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines); - new_low = asm_lines[max_lines - 1].addr; + tui_disassemble (gdbarch, asm_lines, pc, max_lines); + new_low = asm_lines.back ().addr; } else { CORE_ADDR last_addr; - int pos; struct bound_minimal_symbol msymbol; /* Find backward an address which is a symbol and for which disassembling from that address will fill completely the window. */ - pos = max_lines - 1; - do { - new_low -= 1 * max_lines; - msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0); + do + { + /* We want to move back at least one byte. */ + new_low -= 1; + + /* If there's a symbol covering this address then jump back to + the address of this symbol. */ + msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0); + if (msymbol.minsym) + new_low = BMSYMBOL_VALUE_ADDRESS (msymbol); + else + new_low += 1; + + /* Disassemble forward a few lines and see where we got to. */ + tui_disassemble (gdbarch, asm_lines, new_low, max_lines); + last_addr = asm_lines.back ().addr; - if (msymbol.minsym) - new_low = BMSYMBOL_VALUE_ADDRESS (msymbol); - else - new_low += 1 * max_lines; + } while (last_addr >= pc && msymbol.minsym != nullptr); - tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines); - last_addr = asm_lines[pos].addr; - } while (last_addr > pc && msymbol.minsym); + /* If we failed to disassemble the required number of lines then the + following walk forward is not going to work. And what would it + mean to try and walk forward through memory we know can't be + disassembled? Just return the original address which should + indicate we can't move backward in this case. */ + if (asm_lines.size () < max_lines) + return pc; /* Scan forward disassembling one instruction at a time until the last visible instruction of the window matches the pc. We keep the disassembled instructions in the 'lines' window and shift it downward (increasing its addresses). */ + int pos = max_lines - 1; if (last_addr < pc) do { @@ -181,12 +218,15 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from) if (pos >= max_lines) pos = 0; - next_addr = tui_disassemble (gdbarch, asm_lines, - last_addr, pos, 1); + std::vector single_asm_line; + next_addr = tui_disassemble (gdbarch, single_asm_line, + last_addr, 1); /* If there are some problems while disassembling exit. */ if (next_addr <= last_addr) break; + gdb_assert (single_asm_line.size () == 1); + asm_lines[pos] = single_asm_line[0]; last_addr = next_addr; } while (last_addr <= pc); pos++; @@ -224,9 +264,9 @@ tui_disasm_window::set_contents (struct gdbarch *arch, line_width = width - TUI_EXECINFO_SIZE - 2; /* Get temporary table that will hold all strings (addr & insn). */ - std::vector asm_lines (max_lines); + std::vector asm_lines; size_t addr_size = 0; - tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size); + tui_disassemble (gdbarch, asm_lines, pc, max_lines, &addr_size); /* Align instructions to the same column. */ insn_pos = (1 + (addr_size / tab_len)) * tab_len; @@ -237,17 +277,29 @@ tui_disasm_window::set_contents (struct gdbarch *arch, { tui_source_element *src = &content[i]; - std::string line - = (asm_lines[i].addr_string - + n_spaces (insn_pos - asm_lines[i].addr_size) - + asm_lines[i].insn); + std::string line; + CORE_ADDR addr; + + if (i < asm_lines.size ()) + { + line + = (asm_lines[i].addr_string + + n_spaces (insn_pos - asm_lines[i].addr_size) + + asm_lines[i].insn); + addr = asm_lines[i].addr; + } + else + { + line = ""; + addr = 0; + } const char *ptr = line.c_str (); src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0); src->line_or_addr.loa = LOA_ADDRESS; - src->line_or_addr.u.addr = asm_lines[i].addr; - src->is_exec_point = asm_lines[i].addr == cur_pc; + src->line_or_addr.u.addr = addr; + src->is_exec_point = (addr == cur_pc && line.size () > 0); } return true; }