Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own

[Vasili Burdo via Gdb-patches <gdb-patches@sourceware.org>]

Hi, Simon

Everything is OK.
Thanks for help.


пн, 24 янв. 2022 г., 22:28 Simon Marchi <simon.marchi@polymtl.ca>:

> From: Vasili Burdo <vasili.burdo@gmail.com>
>
> 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:
>
> │    0x7ffff4037b27
> <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1390>
>    movb   $0x1,-0xa│
> │    0x7ffff4037b2e
> <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1397>
>    lea    -0x40(%rb│
> │    0x7ffff4037b32
> <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1401>
>    mov    %rax,%rdx│
> │    0x7ffff4037b35
> <_Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class+1404>
>    shr    $0x3,%rdx│
>
> 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:
>
> │
> _Z22read_src_fs_parametersPK8bt_valuePS1_S2_P16ctf_fs_componentP17bt_self_componentP23bt_self_component_class:
>                                         │
> │    0x7ffff4037b27 <+1390>  movb   $0x1,-0xa1(%rbp)
>
>        │
> │    0x7ffff4037b2e <+1397>  lea    -0x40(%rbx),%rax
>
>        │
> │    0x7ffff4037b32 <+1401>  mov    %rax,%rdx
>
>       │
>
> 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 <simon.marchi@efficios.com>
> ---
>  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 <main>"
> +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>"
> +    "$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 <main>"
> +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>"
> +    " 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 <main>.*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 "<main>"
> +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 the
> +    # second line.  If the last instruction of a function goes out of the
> 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}] != \
> -               [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=$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 within
>     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<tui_asm_line> &asm_lines,
>                  CORE_ADDR pc, int count,
> -                size_t *addr_size = nullptr)
> +                size_t *addr_size = nullptr,
> +                bool for_ui = false)
>  {
>    bool term_out = 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 = 0; i < count; ++i)
> +  while (asm_lines.size () < count)
>      {
>        tui_asm_line tal;
>        CORE_ADDR orig_pc = pc;
> @@ -135,8 +141,54 @@ tui_disassemble (struct gdbarch *gdbarch,
>
>        /* 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 ());
> +      if (for_ui)
> +        {
> +         /* If we're rendering for UI...  */
> +          std::string name, filename;
> +          int unmapped = 0, offset = 0, line = 0;
> +
> +          if (build_address_symbolic (gdbarch, orig_pc, asm_demangle,
> +                false, &name, &offset, &filename, &line, &unmapped) == 0)
> +            {
> +              if (offset == 0 || asm_lines.empty ())
> +                {
> +                 /* "offset == 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 = -1;
> +                  tal_header.addr_string = std::move (gdb_dis_out.string
> ());
> +                  tal_header.addr_size = 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 = 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 = std::move (gdb_dis_out.string ());
> +        }
>        gdb_dis_out.clear ();
>
>        if (addr_size != 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<tui_asm_line> asm_lines;
>    size_t addr_size = 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 = (1 + (addr_size / tab_len)) * tab_len;
>
> base-commit: 2f279a64a27bca5be4393e84cbb579e18ef4a4ad
> --
> 2.34.1
>
>