From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id DYN6EnwA72FfWAAAWB0awg (envelope-from ) for ; Mon, 24 Jan 2022 14:39:40 -0500 Received: by simark.ca (Postfix, from userid 112) id 3C1BD1F3B6; Mon, 24 Jan 2022 14:39:40 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.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 10DD51EE18 for ; Mon, 24 Jan 2022 14:39:39 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 608EA3858013 for ; Mon, 24 Jan 2022 19:39:38 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 608EA3858013 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1643053178; bh=AoSEOkMbNFSGS6zNHG9lLXE0xNHNfVZ9iUB5qE/H1qg=; h=References:In-Reply-To:Date:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=nqr66HNkiJiicUVmHw41N14pKvY+Yakf2KoPvibM5BOj5cWoAQWTKkZSdqonmxOFd zKXBf3IJ+wSdroao2M+ZgS1fTQXCCk52zmaJAgPq7Tw26c9SUOdWNmGtUcd6qHcQdr Qh+ewrdmJr0i3iZJvccic3LMmligDEby/vaGvniE= Received: from mail-ua1-x934.google.com (mail-ua1-x934.google.com [IPv6:2607:f8b0:4864:20::934]) by sourceware.org (Postfix) with ESMTPS id 005C43858D3C for ; Mon, 24 Jan 2022 19:39:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 005C43858D3C Received: by mail-ua1-x934.google.com with SMTP id w21so33037188uan.7 for ; Mon, 24 Jan 2022 11:39:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VLiR4TR2QRKixMhmpHPmhrfjX6LxcUjWT0EJXdcqn+c=; b=GaJxo2BVJlBtS/qGCCTu47zBRL25S5RQ6Ui3r360ZD7ReKrooMAqTvgjAonP9jy1hY kvOvjM2oiiclqS+1pS9iHyDWqaSaQ6TDbvc33uwAth1w+P5NUUqz9lMOawPKr5CliprF jpKtL5YLFlUsawkUPwyo9ZN/ulmgngPfEq/KiLz3k6proKgxG1bdUhrQWq6rQ05e+3mP e47P23GrmhueAk8CVddZhkxYHu3FmOBAWqhyVhJEoC9E+aT03nd+DCqjlIkMH9JlRS5l Ch/0YvEK9VaAx8TSUa1/bLcQHBHq6c2rddML9XRQPZEEtQ4Z6qsbzGGIEJKaOPWCtWvz fjCw== X-Gm-Message-State: AOAM531VMFSZoNcS6mqM4HLNkOFxongJi39MrI+I2tdhkVjxv3c8NmAw FNjf5FVnj+qSa8szRZLA2xJZPs3CHnvBpOxvxmZ/fUJG X-Google-Smtp-Source: ABdhPJxj9LBkzmqAhg9xR/pqXGKjCoxcMcnIS11zMKkUG9WBCSPfxlMFGNywF1Qe0EFSimDdQC8B64ZXR45lj+KCrDs= X-Received: by 2002:a67:d18a:: with SMTP id w10mr1904525vsi.83.1643053157514; Mon, 24 Jan 2022 11:39:17 -0800 (PST) MIME-Version: 1.0 References: <20220124192811.1530670-1-simon.marchi@polymtl.ca> In-Reply-To: <20220124192811.1530670-1-simon.marchi@polymtl.ca> Date: Mon, 24 Jan 2022 22:39:04 +0300 Message-ID: Subject: Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own To: Simon Marchi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Vasili Burdo via Gdb-patches Reply-To: Vasili Burdo Cc: Simon Marchi , Vasili Burdo via Gdb-patches Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi, Simon Everything is OK. Thanks for help. =D0=BF=D0=BD, 24 =D1=8F=D0=BD=D0=B2. 2022 =D0=B3., 22:28 Simon Marchi : > From: Vasili Burdo > > Hi Vasili, > > Here's a new version of your patch [1], I did the following changes: > > - 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) > > Since the patch is still in your name, please let me know if that is OK > with you. > > [1] https://sourceware.org/pipermail/gdb-patches/2022-January/185399.html > > ~~~ > > 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: > > =E2=94=82 0x7ffff4037b27 > <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_sel= f_componentP23bt_self_component_class+1390> > movb $0x1,-0xa=E2=94=82 > =E2=94=82 0x7ffff4037b2e > <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_sel= f_componentP23bt_self_component_class+1397> > lea -0x40(%rb=E2=94=82 > =E2=94=82 0x7ffff4037b32 > <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_sel= f_componentP23bt_self_component_class+1401> > mov %rax,%rdx=E2=94=82 > =E2=94=82 0x7ffff4037b35 > <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_sel= f_componentP23bt_self_component_class+1404> > shr $0x3,%rdx=E2=94=82 > > 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. > > The instructions from the example above would now look like this: > > =E2=94=82 > _Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_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 > > 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. > > 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(-) > > diff --git a/gdb/testsuite/gdb.tui/basic.exp > b/gdb/testsuite/gdb.tui/basic.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 > > Term::command "layout asm" > -Term::check_contents "asm window shows main" "$hex
" > +Term::check_contents "asm window shows main" " main:" > > Term::check_box "asm box" 0 0 80 15 > > Term::command "layout split" > Term::check_contents "split layout contents" \ > - "$main_line *$main_re.*$hex
" > + "$main_line *$main_re.* main:" > > 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" > > Term::command "layout asm" > -Term::check_contents "asm window shows main" "$hex
" > +Term::check_contents "asm window shows main" " main:" > > 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" > $text]} \ > > Term::command "layout example" > Term::check_contents "example layout shows assembly" \ > - "$hex
" > + " main:" > > 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" > > 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/testsuite/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]} { > > # 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:" > > # 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 \ > - "^ *$hex\[^\r\n\]+" \ > + " *_start: *" \ > + " *$hex\[^\r\n\]+" \ > "\\s+"] > > # 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 } { > > # 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_width} 15 "
" > +Term::check_box_contents "check asm box contents" 0 0 > ${tui_asm_window_width} 15 " main:" > > # 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] > > - # 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] \ > - && [regexp $re_line [Term::get_line 1]]} { > + && ([regexp $re_line [Term::get_line 1]] > + || [regexp $re_line [Term::get_line 2]])} { > # We scrolled successfully. > } else { > if {[count_whitespace ${line}] !=3D \ > - [count_whitespace [Term::get_line 1]]} { > + [count_whitespace [Term::get_line 2]]} { > # GDB's TUI assembler display will widen columns based on > # the longest item that appears in a column on any line. > # As we have just scrolled, and so revealed a new line, it > @@ -93,7 +97,7 @@ while (1) { > verbose -log " details." > } > > - fail "$testname (scroll failed)" > + fail "$testname (scroll failed, down_count=3D$down_count)" > Term::dump_screen > break > } > 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. > > + FOR_UI boolean flag shows the purpose this function is called for. > TRUE > + 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 > this > 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, > std::vector &asm_lines, > CORE_ADDR pc, int count, > - size_t *addr_size =3D nullptr) > + size_t *addr_size =3D nullptr, > + bool for_ui =3D false) > { > 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 (); > > /* 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, > > /* 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) > + { > + /* 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 ()) > + { > + /* "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 (); > > 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= ); > > /* Align instructions to the same column. */ > insn_pos =3D (1 + (addr_size / tab_len)) * tab_len; > > base-commit: 2f279a64a27bca5be4393e84cbb579e18ef4a4ad > -- > 2.34.1 > >