From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id mBjUKkVx9GFeEAAAWB0awg (envelope-from ) for ; Fri, 28 Jan 2022 17:42:13 -0500 Received: by simark.ca (Postfix, from userid 112) id AD0A31F0E7; Fri, 28 Jan 2022 17:42:13 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C0FA41EDF0 for ; Fri, 28 Jan 2022 17:42:12 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DF5903864821 for ; Fri, 28 Jan 2022 22:42:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DF5903864821 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1643409731; bh=DZkCKcUakXa049/HgYweIZSSXmqIMHjVwmJXmG0Vhj4=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=X29qNDRzwwFAFd1iRZD/jrrcBuzmVFHMtxUzG5tbZwXg61hoJcgY8+Wy8VGvc9fCM vG95g4pe3XklfbQ4D66Rur6IHV0q9xNbd+hOnuqGI+jGdQXp/vzIRimPmqCCubD8Wm KkQrOeSPk2XQBMOFbRYCpwQWMKHjztiZK68BGphc= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 8C699385840C for ; Fri, 28 Jan 2022 22:41:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8C699385840C Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-646-Wn6-ZczKMdm6R_7A7LjH0A-1; Fri, 28 Jan 2022 17:41:44 -0500 X-MC-Unique: Wn6-ZczKMdm6R_7A7LjH0A-1 Received: by mail-wm1-f71.google.com with SMTP id q125-20020a1c4383000000b003503e10c027so1789591wma.2 for ; Fri, 28 Jan 2022 14:41:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=yCn10DThZCpfUGgaeFS+bvkBH/uaGXL8ktZ9mkhtip0=; b=npFBbygaU9dv1FZcOuPpnDGB4qabRM0oAJ9gTGlywheUmzZB6uOAA+SUt7ePurCAFe yCD4XTPbp2r27jxqFPMHFwCOwhhDfTzBNGaNn711ETpCxcAbotZLD6dWI0P5zXyiHRoM 3Evthwda4ET1IQKwjtqnbyZSHmv1+IyI35QltEX3TbAsADShYQxl6Q/jKFn/srFHUe+E eCvB6Ef3i58KoTQQrEmHXqzDkW9QjkY6+nZvq0Ys9/OgkT1rbLMjvLUlOiRQ3xYW2Uyh 4npXRw0cTJffDw0Ykw/EfqNfQbyPAPZcKeeH5m6pv/2UM96WQ/ceQ7v8FvFrbPVcQBLc wOLA== X-Gm-Message-State: AOAM532LlOS06LqIM5QNjsMFXAGMjS42VfnvnU+mTk36RUSsMwiPFqHK oxHU+JrCcA/4GcgCsM6MYGBl9SCb6MBIvSZaRccFkV/m3lBE8yTcC7x7LRoYd3aQ1RiQZW7FB2R GuPHqVTDcEhHse7jxEYxYPw== X-Received: by 2002:a5d:5907:: with SMTP id v7mr8875512wrd.307.1643409702934; Fri, 28 Jan 2022 14:41:42 -0800 (PST) X-Google-Smtp-Source: ABdhPJzCpeFKzYN/MpfEQUwHxkKIN9BUhyo88zSkTS+wewEw8WXKqXg4APo6G99429yu6mYDbcT5bA== X-Received: by 2002:a5d:5907:: with SMTP id v7mr8875494wrd.307.1643409702534; Fri, 28 Jan 2022 14:41:42 -0800 (PST) Received: from localhost (host86-140-92-93.range86-140.btcentralplus.com. [86.140.92.93]) by smtp.gmail.com with ESMTPSA id k12sm5640751wrd.98.2022.01.28.14.41.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Jan 2022 14:41:42 -0800 (PST) Date: Fri, 28 Jan 2022 22:41:40 +0000 To: Simon Marchi Subject: Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own Message-ID: <20220128224140.GF425591@redhat.com> References: <20220124192811.1530670-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 In-Reply-To: <20220124192811.1530670-1-simon.marchi@polymtl.ca> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 22:31:18 up 4 days, 13:18, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Andrew Burgess via Gdb-patches Reply-To: Andrew Burgess Cc: Vasili Burdo , Simon Marchi , gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" * Simon Marchi via Gdb-patches [2022-01-24 14:= 28:11 -0500]: > From: Vasili Burdo >=20 > Hi Vasili, >=20 > Here's a new version of your patch [1], I did the following changes: >=20 > - Updated gdb.ui tests. > - Renamed tal2 to tal_header. > - Omitted copying tal to tal2 (tal_header), since we overwrite all the > fields anyway. > - General formatting. > - Added commit message (see below) >=20 > Since the patch is still in your name, please let me know if that is OK > with you. >=20 > [1] https://sourceware.org/pipermail/gdb-patches/2022-January/185399.html >=20 > ~~~ >=20 > When debbugging C++ programs, symbols tend to be quite long. This makes > the TUI's disassembly view difficult to use, as each line includes the > function's symbol name, which can leave little or no room for the > instructions. For example: >=20 > =E2=94=82 0x7ffff4037b27 <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_= P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1390> = movb $0x1,-0xa=E2=94=82 > =E2=94=82 0x7ffff4037b2e <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_= P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1397> = lea -0x40(%rb=E2=94=82 > =E2=94=82 0x7ffff4037b32 <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_= P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1401> = mov %rax,%rdx=E2=94=82 > =E2=94=82 0x7ffff4037b35 <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_= P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1404> = shr $0x3,%rdx=E2=94=82 >=20 > To address this, stop including the symbol name with the offset on each > line. Instead, put it on a line of its own just before the address is > points to. Because it would be possible for the current symbol name to > be outside the window, and therefore it would be difficult for the user > to know what function they are currently stepping into, make it so the > first symbol name always "sticks" to the top of the window. >=20 > The instructions from the example above would now look like this: >=20 > =E2=94=82 _Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_compon= entP17bt_self_componentP23bt_self_component_class: = =E2=94=82 > =E2=94=82 0x7ffff4037b27 <+1390> movb $0x1,-0xa1(%rbp) = = =E2=94=82 > =E2=94=82 0x7ffff4037b2e <+1397> lea -0x40(%rbx),%rax = = =E2=94=82 > =E2=94=82 0x7ffff4037b32 <+1401> mov %rax,%rdx = = =E2=94=82 This looks like a great improvement. I took a look through the changes, I have more of one question... >=20 > Update the gdb.tui tests to match the new output. The changes are all > trivial except the scrolling test in gdb.tui/tui-layout-asm.exp. The > new behavior means that sometimes, when scrolling down, two lines will > disappear at once instead of a single line, when the last instruction > mapped to a symbol goes out of view. Update the test to account for > that. >=20 > Change-Id: I9cf46c094058b9382087b54d212f22265b1fc324 > Co-Authored-By: Simon Marchi > --- > gdb/testsuite/gdb.tui/basic.exp | 4 +- > gdb/testsuite/gdb.tui/list.exp | 2 +- > gdb/testsuite/gdb.tui/new-layout.exp | 4 +- > .../gdb.tui/tui-layout-asm-short-prog.exp | 5 +- > gdb/testsuite/gdb.tui/tui-layout-asm.exp | 18 +++--- > gdb/tui/tui-disasm.c | 62 +++++++++++++++++-- > 6 files changed, 76 insertions(+), 19 deletions(-) >=20 > diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basi= c.exp > index 0e9f2e3eab28..82fec97319a1 100644 > --- a/gdb/testsuite/gdb.tui/basic.exp > +++ b/gdb/testsuite/gdb.tui/basic.exp > @@ -96,13 +96,13 @@ if {[Term::wait_for $regexp] \ > Term::check_box "source box" 0 0 80 15 > =20 > Term::command "layout asm" > -Term::check_contents "asm window shows main" "$hex
" > +Term::check_contents "asm window shows main" " main:" > =20 > Term::check_box "asm box" 0 0 80 15 > =20 > Term::command "layout split" > Term::check_contents "split layout contents" \ > - "$main_line *$main_re.*$hex
" > + "$main_line *$main_re.* main:" > =20 > Term::check_box "source box in split layout" 0 0 80 7 > Term::check_box "asm box in split layout" 0 6 80 9 > diff --git a/gdb/testsuite/gdb.tui/list.exp b/gdb/testsuite/gdb.tui/list.= exp > index d153052cfd77..00854097a2d3 100644 > --- a/gdb/testsuite/gdb.tui/list.exp > +++ b/gdb/testsuite/gdb.tui/list.exp > @@ -32,7 +32,7 @@ if {![Term::enter_tui]} { > Term::check_contents "initial source listing" "21 *return 0" > =20 > Term::command "layout asm" > -Term::check_contents "asm window shows main" "$hex
" > +Term::check_contents "asm window shows main" " main:" > =20 > Term::command "list -q main" > Term::check_contents "list -q main" "21 *return 0" > diff --git a/gdb/testsuite/gdb.tui/new-layout.exp b/gdb/testsuite/gdb.tui= /new-layout.exp > index 2b5e07db612c..f64b11c6ea44 100644 > --- a/gdb/testsuite/gdb.tui/new-layout.exp > +++ b/gdb/testsuite/gdb.tui/new-layout.exp > @@ -81,13 +81,13 @@ gdb_assert {![string match "No Source Available" $tex= t]} \ > =20 > Term::command "layout example" > Term::check_contents "example layout shows assembly" \ > - "$hex
" > + " main:" > =20 > Term::command "layout h" > Term::check_box "left window box" 0 0 40 15 > Term::check_box "right window box" 39 0 41 15 > Term::check_contents "horizontal display" \ > - "$hex
.*21.*return 0" > + " main:.*21.*return 0" > =20 > Term::command "winheight src - 5" > Term::check_box "left window box after shrink" 0 0 40 10 > diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp b/gdb/te= stsuite/gdb.tui/tui-layout-asm-short-prog.exp > index a6be799113df..6f43b4cac73c 100644 > --- a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp > +++ b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp > @@ -34,7 +34,7 @@ if {![Term::prepare_for_tui]} { > =20 > # This puts us into TUI mode, and should display the ASM window. > Term::command_no_prompt_prefix "layout asm" > -Term::check_box_contents "check asm box contents" 0 0 80 15 "<_start>" > +Term::check_box_contents "check asm box contents" 0 0 80 15 " _start:" > =20 > # Record the first line of output, we'll need this later. > set first_line [Term::get_line 1] > @@ -44,7 +44,8 @@ set first_line [Term::get_line 1] > Term::command "+ 13" > Term::check_box_contents "check asm box contents again" 0 0 80 15 \ > [multi_line \ > -=09 "^ *$hex\[^\r\n\]+" \ > +=09 " *_start: *" \ > +=09 " *$hex\[^\r\n\]+" \ > =09 "\\s+"] > =20 > # Now scroll backward again, we should return to the start of the > diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb= .tui/tui-layout-asm.exp > index a76e637b4c93..6eda03d922f4 100644 > --- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp > +++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp > @@ -40,7 +40,7 @@ proc count_whitespace { string } { > =20 > # This puts us into TUI mode, and should display the ASM window. > Term::command_no_prompt_prefix "layout asm" > -Term::check_box_contents "check asm box contents" 0 0 ${tui_asm_window_w= idth} 15 "
" > +Term::check_box_contents "check asm box contents" 0 0 ${tui_asm_window_w= idth} 15 " main:" > =20 > # Scroll the ASM window down using the down arrow key. In an ideal > # world we'd like to use PageDown here, but currently our terminal > @@ -48,10 +48,13 @@ Term::check_box_contents "check asm box contents" 0 0= ${tui_asm_window_width} 15 > 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] > + # Grab the third line, this is about to become either the first or t= he > + # second line. If the last instruction of a function goes out of th= e window > + # the scroll will make two lines disappear, the line with the symbol= name and > + # the line with the instruction. > + set line [Term::get_line 3] > =20 > - # Except, if the second line is blank then we are at the end of > + # Except, if the third 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 won't change, so the call to Term::wait_for > @@ -70,11 +73,12 @@ while (1) { > # Ignore whitespace mismatches. > regsub -all {\s+} $re_line {\s+} re_line > if {[Term::wait_for $re_line] \ > -=09 && [regexp $re_line [Term::get_line 1]]} { > +=09 && ([regexp $re_line [Term::get_line 1]] > +=09=09|| [regexp $re_line [Term::get_line 2]])} { > =09# We scrolled successfully. > } else { > =09if {[count_whitespace ${line}] !=3D \ > -=09=09[count_whitespace [Term::get_line 1]]} { > +=09=09[count_whitespace [Term::get_line 2]]} { > =09 # GDB's TUI assembler display will widen columns based on > =09 # the longest item that appears in a column on any line. > =09 # As we have just scrolled, and so revealed a new line, it > @@ -93,7 +97,7 @@ while (1) { > =09 verbose -log " details." > =09} > =20 > -=09fail "$testname (scroll failed)" > +=09fail "$testname (scroll failed, down_count=3D$down_count)" > =09Term::dump_screen > =09break > } > diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c > index f40d4e2e9f1c..027231e466b1 100644 > --- a/gdb/tui/tui-disasm.c > +++ b/gdb/tui/tui-disasm.c > @@ -27,6 +27,7 @@ > #include "value.h" > #include "source.h" > #include "disasm.h" > +#include "valprint.h" > #include "tui/tui.h" > #include "tui/tui-command.h" > #include "tui/tui-data.h" > @@ -93,6 +94,10 @@ len_without_escapes (const std::string &str) > field on each item in ASM_LINES, otherwise the addr_size fields withi= n > ASM_LINES are undefined. > =20 > + FOR_UI boolean flag shows the purpose this function is called for. T= RUE > + means we're going to show ASM_LINES on screen, FALSE means ASM_LINES = are > + used to figure out current scroll position or any other purpose. > + > It is worth noting that ASM_LINES might not have COUNT entries when t= his > function returns. If the disassembly is truncated for some other > reason, for example, we hit invalid memory, then ASM_LINES can have > @@ -101,7 +106,8 @@ static CORE_ADDR > tui_disassemble (struct gdbarch *gdbarch, > =09=09 std::vector &asm_lines, > =09=09 CORE_ADDR pc, int count, > -=09=09 size_t *addr_size =3D nullptr) > +=09=09 size_t *addr_size =3D nullptr, > +=09=09 bool for_ui =3D false) This default `false` concerned my initially. The only times we call tui_disassemble is either because we want to redraw the screen contents, or we want to know what _would_ be drawn to the screen if/when we do the redraw. Only, now, we draw the screen differently for these two cases, so, my thinking goes, surely there's going to be some edge case where we ask, what address would be on the screen if .... and we'll get the wrong answer back. I played with this for a while, but couldn't get anything obvious to break - I suspect that if there are bugs, they are going to be super subtle, which addresses appear on the screen doesn't change much, usually just one instruction different I think, so maybe it doesn't matter. And given I couldn't spot anything, maybe I'm over thinking this, and there is no problem... I guess my question is, did you already consider this already? Is there a reason why having two strategies is known to be OK? FWIW, I'm happy to have this go in, worst case, we find there is an issue, and we just switch to always do the disassembly the new way, it's not going to be a difficult fix - just thought I'd ask. Thanks, Andrew > { > bool term_out =3D source_styling && gdb_stdout->can_emit_style_escape = (); > string_file gdb_dis_out (term_out); > @@ -110,7 +116,7 @@ tui_disassemble (struct gdbarch *gdbarch, > asm_lines.clear (); > =20 > /* Now construct each line. */ > - for (int i =3D 0; i < count; ++i) > + while (asm_lines.size () < count) > { > tui_asm_line tal; > CORE_ADDR orig_pc =3D pc; > @@ -135,8 +141,54 @@ tui_disassemble (struct gdbarch *gdbarch, > =20 > /* And capture the address the instruction is at. */ > tal.addr =3D orig_pc; > - print_address (gdbarch, orig_pc, &gdb_dis_out); > - tal.addr_string =3D std::move (gdb_dis_out.string ()); > + if (for_ui) > + { > +=09 /* If we're rendering for UI... */ > + std::string name, filename; > + int unmapped =3D 0, offset =3D 0, line =3D 0; > + > + if (build_address_symbolic (gdbarch, orig_pc, asm_demangle, > + false, &name, &offset, &filename, &line, &unmapped) =3D= =3D 0) > + { > + if (offset =3D=3D 0 || asm_lines.empty ()) > + { > +=09=09 /* "offset =3D=3D 0" means function start, > + "asm_lines.empty ()" means top line of output. > + For these cases, insert "function header" on top of > + "real" disassembly line. */ > + fputs_styled (name.c_str (), function_name_style.style= (), > + &gdb_dis_out); > + fputs_filtered (":", &gdb_dis_out); > + > + tui_asm_line tal_header; > + tal_header.addr =3D -1; > + tal_header.addr_string =3D std::move (gdb_dis_out.stri= ng ()); > + tal_header.addr_size =3D 0; > + tal_header.insn.clear (); > + asm_lines.push_back (std::move (tal_header)); > + > + gdb_dis_out.clear (); > + } > + > + fputs_styled (paddress (gdbarch, orig_pc), > + address_style.style (), &gdb_dis_out); > + fputs_filtered (" <", &gdb_dis_out); > + if (unmapped) > + fputs_filtered ("*", &gdb_dis_out); > + fprintf_styled (&gdb_dis_out, address_style.style (), > + "%+d", offset); > + if (unmapped) > + fputs_filtered ("*", &gdb_dis_out); > + fputs_filtered (">", &gdb_dis_out); > + tal.addr_string =3D std::move (gdb_dis_out.string ()); > + } > + } > + else > + { /* If we're rendering for some other purpose than UI... > + Fall back to legacy disassembly line style. */ > + print_address (gdbarch, orig_pc, &gdb_dis_out); > + tal.addr_string =3D std::move (gdb_dis_out.string ()); > + } > gdb_dis_out.clear (); > =20 > if (addr_size !=3D nullptr) > @@ -342,7 +394,7 @@ tui_disasm_window::set_contents (struct gdbarch *arch= , > /* Get temporary table that will hold all strings (addr & insn). */ > std::vector asm_lines; > size_t addr_size =3D 0; > - tui_disassemble (m_gdbarch, asm_lines, pc, max_lines, &addr_size); > + tui_disassemble (m_gdbarch, asm_lines, pc, max_lines, &addr_size, true= ); > =20 > /* Align instructions to the same column. */ > insn_pos =3D (1 + (addr_size / tab_len)) * tab_len; >=20 > base-commit: 2f279a64a27bca5be4393e84cbb579e18ef4a4ad > --=20 > 2.34.1 >=20