From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108551 invoked by alias); 22 Jan 2020 17:48:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 108276 invoked by uid 89); 22 Jan 2020 17:48:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=scroll, tui_disassemble, asm_lines, Grab X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Jan 2020 17:48:05 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579715283; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=t3VvhCNe9n5iK+SNwfdPiQZlCklmD6LAYq0KDS5lPHw=; b=jFQc30s6zmQz8r946acgPYPgZvtwq4C/78XqW+bZqvFPsrCC+xthSuyKXaTWt0OCi/GcXW FKl8MzWgaZ+XsvlfS/ZlEcH+4fLIsrKnqNSg7zR+xpJgMG0yhQcqaCPy29+c2lVaw/wThH BN91AKObjtyHN4vDXIf+sZr+Pbx0Okg= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-392-OjbYE_LQNku5lOxU0jXVLQ-1; Wed, 22 Jan 2020 12:48:01 -0500 Received: by mail-wr1-f69.google.com with SMTP id f10so242951wro.14 for ; Wed, 22 Jan 2020 09:48:01 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id l19sm4784182wmj.12.2020.01.22.09.47.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 22 Jan 2020 09:47:59 -0800 (PST) Subject: Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better To: Andrew Burgess , gdb-patches@sourceware.org References: Cc: Shahab Vahedi , Tom Tromey From: Pedro Alves Message-ID: <7dfe280b-38fa-406f-5cd1-436c286abee6@redhat.com> Date: Wed, 22 Jan 2020 19:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-01/txt/msg00685.txt.bz2 Hi! I think this should be filed under PR tui/9765 too? I didn't try to grok the code very deeply. I did try it out, and saw that it works as described. I'm happy with that. Thanks for doing all this. Tiny nits below, mostly comments issues I noticed while skimming the patch. > index 98735e75e33..a8d50a463ba 100644 > --- a/gdb/minsyms.h > +++ b/gdb/minsyms.h > @@ -240,7 +240,9 @@ enum class lookup_msym_prefer > > /* Search through the minimal symbol table for each objfile and find > the symbol whose address is the largest address that is still less > - than or equal to PC, and which matches SECTION. > + than or equal to PC_IN, and which matches SECTION. A matching symbol > + must either by zero sized and have address PC_IN, or PC_IN must fall "BE zero sized", I think. > + within the range of addresses covered by the matching symbol. > > If SECTION is NULL, this uses the result of find_pc_section > instead. > @@ -249,12 +251,17 @@ enum class lookup_msym_prefer > found, or NULL if PC is not in a suitable range. > > See definition of lookup_msym_prefer for description of PREFER. By > - default mst_text symbols are preferred. */ > + default mst_text symbols are preferred. > + > + If the PREVIOUS pointer is non-NULL, and no matching symbol is found, > + then the contents will be set to reference the closest symbol before > + PC_IN. */ > > --- 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 Please avoid "I" in comments. For example, you could write "In an ideal world we'd use". > +# 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 wont -> won't > + # 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..5b606dcf696 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. Uppercase COUNT in "COUNT'th" ? ( I'm not sure I'm able to pronounce that. :-) ) > + 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. Uppercase last ASM_LINES ? > + > + 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) This can be gdb_exception_error. > + { > + /* If pc points to an invalid address then we'll catch a Uppercase PC. > + 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) Thanks, Pedro Alves