From patchwork Mon Jan 13 20:46:27 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 37361 Received: (qmail 45985 invoked by alias); 13 Jan 2020 20:46:42 -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 45899 invoked by uid 89); 13 Jan 2020 20:46:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.4 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=scroll, randomly, Capture, Search X-HELO: mail-wr1-f42.google.com Received: from mail-wr1-f42.google.com (HELO mail-wr1-f42.google.com) (209.85.221.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 13 Jan 2020 20:46:38 +0000 Received: by mail-wr1-f42.google.com with SMTP id d16so10046872wre.10 for ; Mon, 13 Jan 2020 12:46:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :in-reply-to:references; bh=FC8sKcyxd1xAiHwsXBknVkf0ff77sgJ9LU+04cD53c4=; b=OUYm6q9sfAYEx8tKVBFydQwgXEr0kXsn0+PK1dRHNwMWBdROYkNKxkCP/+7sAtDmLX DZ2SuDZ3Az3CGQTXQBUxiWb7Zx4Tkin9rj5eENuoIa3CP8RfB+gUrTwMDTJXHHGq6Z/s OayOoHbAsdlmy5mWAbvSnIHMVOIKXNF6GyWoLzCLJkAOI+A7Tjd3j3rU8GUL19lIzxpK E/nSDh4id4O6ZAoMKxa7ZtqWJv3lWxeg7QeGC3lw8yqGJJItcXsyk7zOYVmNN5N2pIqG FTmO2WCNZlg2r9wrq/Gv1Nx2xJZg3Aw5wYUHH//XBFU9LjNFCMoTj9mFvoosEyY7mZBt UZSA== Return-Path: Received: from localhost (cust242-dsl59.idnet.net. [212.69.59.242]) by smtp.gmail.com with ESMTPSA id n8sm16495185wrx.42.2020.01.13.12.46.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 13 Jan 2020 12:46:34 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Shahab Vahedi , Pedro Alves , Tom Tromey , Andrew Burgess Subject: [PATCH 2/2] gdb/tui: asm window handles invalid memory and scrolls better Date: Mon, 13 Jan 2020 20:46:27 +0000 Message-Id: In-Reply-To: References: In-Reply-To: References: <20200110143056.GD3815@gmail.com> X-IsSubscribed: yes This started as a patch to enable the asm window to handle attempts to disassemble invalid memory, but it ended up expanding into a significant rewrite of how the asm window handles scrolling. These two things ended up being tied together as it was impossible to correctly test scrolling into invalid memory when the asm window would randomly behave weirdly while scrolling. Things that should work nicely now, scroll to the bottom or top of the listing with PageUp, PageDown, Up Arrow, Down Arrow and we should be able to scroll past small areas of memory that don't have symbols associated with them now. Adding tests for this scrolling was a little bit of a problem. First I would have liked to add tests for PageUp / PageDown, but the tuiterm library we use doesn't support these commands. Next, I would have liked to test scrolling to the start or end of the assembler listing and then trying to scroll even more, however, this is a problem because in a well behaving GDB a scroll at the start/end has no effect. What we need to do is: - Move to start of assembler listing, - Send scroll up command, - Wait for all curses output, - Ensure the assembler listing is unchanged, we're still at the start of the listing. The problem is that there is no curses output, so how long do we wait at step 3? The same problem exists for scrolling to the bottom of the assembler listing. However, when scrolling down you can at least see the end coming, so I added a test for this case, however, this feels like an area of code that is massively undertested. 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_symbol_backward): New function. (tui_find_disassembly_address): Updated throughout. (tui_disasm_window::set_contents): Update for changes to tui_disassemble. (tui_disasm_window::do_scroll_vertical): No need to adjust the number of lines to scroll. gdb/testsuite/ChangeLog: * gdb.tui/tui-layout-asm.exp: Add scrolling test for asm window. Change-Id: I2114f6cca5cd89fb8ef5dfdacd5f751248c461e0 --- gdb/ChangeLog | 12 ++ gdb/testsuite/ChangeLog | 4 + gdb/testsuite/gdb.tui/tui-layout-asm.exp | 41 ++++++ gdb/tui/tui-disasm.c | 216 +++++++++++++++++++++++-------- 4 files changed, 217 insertions(+), 56 deletions(-) diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp index cec2735764e..f78baab1081 100644 --- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp +++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp @@ -32,3 +32,44 @@ if {![Term::prepare_for_tui]} { # This puts us into TUI mode, and should display the ASM window. Term::command "layout asm" Term::check_box_contents "check asm box contents" 0 0 80 15 "
" + +# Scroll the ASM window down using the down arrow key. In an ideal +# world I'd like to use PageDown here, but currently our terminal +# library doesn't support such advanced things. +set testname "scroll to end of assembler" +set down_count 0 +while (1) { + # Grab the second line, this is about to become the first line. + set line [Term::get_line 2] + + # Except, if the second line is blank then we are at the end of + # the available asm output. Pressing down again _shouldn't_ + # change the output, however, if GDB is working, and we press down + # then the screen wont change, so the call to Term::wait_for below + # will just timeout. So for now we avoid testing the edge case. + if {[regexp -- "^\\| +\\|$" $line]} { + # Second line is blank, we're at the end of the assembler. + pass $testname + break + } + + # Send the down key to GDB. + send_gdb "\033\[B" + incr down_count + if {[Term::wait_for [string_to_regexp $line]] \ + && [Term::get_line 1] == $line} { + # We scrolled successfully. + } else { + fail "$testname (scroll failed)" + Term::dump_screen + break + } + + if { $down_count > 250 } { + # Maybe we should accept this as a pass in case a target + # really does have loads of assembler to scroll through. + fail "$testname (too much assembler)" + Term::dump_screen + break + } +} diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c index 98c691f3387..c3d66bf348b 100644 --- a/gdb/tui/tui-disasm.c +++ b/gdb/tui/tui-disasm.c @@ -81,25 +81,58 @@ 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 (which will be emptied of any previous + contents). 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 (); string_file gdb_dis_out (term_out); + /* Must start with an empty list. */ + asm_lines.clear (); + /* 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,21 +140,36 @@ 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.push_back (tal); + } + return pc; +} - asm_lines[pos + i].insn = std::move (gdb_dis_out.string ()); +/* Look backward from ADDR for a bound_minimal_symbol. If no symbol can be + found then return a null bound_minimal_symbol. This is used when + disassembling backwards. */ - /* Reset the buffer to empty. */ - gdb_dis_out.clear (); +static struct bound_minimal_symbol +tui_find_symbol_backward (CORE_ADDR addr) +{ + struct bound_minimal_symbol msym; + + for (int offset = 1; offset <= 1024; offset *= 2) + { + CORE_ADDR tmp = addr - offset; + msym = lookup_minimal_symbol_by_pc_section (tmp, 0); + if (msym.minsym) + return msym; } - return pc; + + return { nullptr, nullptr }; } /* Find the disassembly address that corresponds to FROM lines above @@ -134,65 +182,113 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from) int max_lines; max_lines = (from > 0) ? from : - from; - if (max_lines <= 1) + if (max_lines == 0) 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; + /* Always disassemble 1 extra instruction here, then if the last + instruction fails to disassemble we will take the address of the + previous instruction that did disassemble as the result. */ + tui_disassemble (gdbarch, asm_lines, pc, max_lines + 1); + new_low = asm_lines.back ().addr; } else { CORE_ADDR last_addr; - int pos; struct bound_minimal_symbol msymbol; + max_lines++; + /* 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); - - if (msymbol.minsym) - new_low = BMSYMBOL_VALUE_ADDRESS (msymbol); - else - new_low += 1 * max_lines; - - tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines); - last_addr = asm_lines[pos].addr; - } while (last_addr > pc && msymbol.minsym); + disassembling from that address will give us MAX_LINES of + disassembly. The variable NEXT_ADDR will be set to the next + address to disassemble after the MAX_LINES of disassembly. */ + CORE_ADDR next_addr; + + /* As we search backward if we find an address that looks promising + then we record it in this structure. If the next address we try + is not suitable then we fall back to the previous good address. + Otherwise, if the next address is also good it gets recorded here + instead, and then we try the next address. */ + struct + { + bool found = false; + CORE_ADDR new_low; + } possible_new_low; + + do + { + /* Search backward for a symbol, this allows us to find a good + spot to start disassembling from which should, we hope, be + the start of an instruction. */ + msymbol = tui_find_symbol_backward (new_low); + if (msymbol.minsym != nullptr) + new_low = BMSYMBOL_VALUE_ADDRESS (msymbol); + + /* Disassemble forward a few lines and see where we got to. */ + next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines); + last_addr = asm_lines.back ().addr; + if (last_addr > pc && msymbol.minsym != nullptr + && asm_lines.size () >= max_lines) + { + /* This will do if we can't find anything better. */ + possible_new_low.found = true; + possible_new_low.new_low = new_low; + } + } while (last_addr > pc && msymbol.minsym != nullptr); + + /* If we failed to disassemble the required number of lines then the + following walk forward is not going to work, it assumes that + ASM_LINES contains exactly MAX_LINES entries. Instead we should + consider falling back to a previous possible start address in + POSSIBLE_NEW_LOW. */ + if (asm_lines.size () < max_lines) + { + if (!possible_new_low.found) + return pc; + + /* Take the best possible match we have. */ + new_low = possible_new_low.new_low; + next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines); + last_addr = asm_lines.back ().addr; + gdb_assert (asm_lines.size () >= max_lines); + } /* 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 { - CORE_ADDR next_addr; - pos++; if (pos >= max_lines) pos = 0; - next_addr = tui_disassemble (gdbarch, asm_lines, - last_addr, pos, 1); - + CORE_ADDR old_next_addr = next_addr; + std::vector single_asm_line; + next_addr = tui_disassemble (gdbarch, single_asm_line, + next_addr, 1); /* If there are some problems while disassembling exit. */ - if (next_addr <= last_addr) - break; - last_addr = next_addr; - } while (last_addr <= pc); + if (next_addr <= old_next_addr) + return pc; + gdb_assert (single_asm_line.size () == 1); + asm_lines[pos] = single_asm_line[0]; + } while (next_addr <= pc); pos++; if (pos >= max_lines) pos = 0; new_low = asm_lines[pos].addr; + + /* When scrolling backward the addresses should move backward, or at + the very least stay the same if we are at the first address that + can be disassembled. */ + gdb_assert (new_low <= pc); } return new_low; } @@ -224,9 +320,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 +333,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; } @@ -326,10 +434,6 @@ tui_disasm_window::do_scroll_vertical (int num_to_scroll) CORE_ADDR pc; pc = start_line_or_addr.u.addr; - if (num_to_scroll >= 0) - num_to_scroll++; - else - --num_to_scroll; symtab_and_line sal {}; sal.pspace = current_program_space;