* Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own
2022-01-24 19:28 [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own Simon Marchi
@ 2022-01-24 19:39 ` Vasili Burdo
2022-01-24 19:40 ` Simon Marchi
2022-01-28 22:41 ` Andrew Burgess
1 sibling, 1 reply; 9+ messages in thread
From: Vasili Burdo @ 2022-01-24 19:39 UTC (permalink / raw)
To: Simon Marchi; +Cc: Vasili Burdo via Gdb-patches, Simon Marchi
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own
2022-01-24 19:28 [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own Simon Marchi
2022-01-24 19:39 ` Vasili Burdo
@ 2022-01-28 22:41 ` Andrew Burgess
2022-01-29 1:20 ` Simon Marchi
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2022-01-28 22:41 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, Vasili Burdo, Simon Marchi
* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2022-01-24 14:28:11 -0500]:
> 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 │
This looks like a great improvement. I took a look through the
changes, I have more of one question...
>
> 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)
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 = 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread