* [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own
@ 2022-01-24 19:28 Simon Marchi
2022-01-24 19:39 ` Vasili Burdo
2022-01-28 22:41 ` Andrew Burgess
0 siblings, 2 replies; 9+ messages in thread
From: Simon Marchi @ 2022-01-24 19:28 UTC (permalink / raw)
To: gdb-patches; +Cc: Vasili Burdo, Simon Marchi
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-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:39 ` Vasili Burdo
@ 2022-01-24 19:40 ` Simon Marchi
0 siblings, 0 replies; 9+ messages in thread
From: Simon Marchi @ 2022-01-24 19:40 UTC (permalink / raw)
To: Vasili Burdo, Simon Marchi; +Cc: Vasili Burdo via Gdb-patches, Tom Tromey
On 2022-01-24 14:39, Vasili Burdo wrote:
> Hi, Simon
>
> Everything is OK.
> Thanks for help.
Thanks, I'll leave Tom some time in case he has anything to say,
otherwise I'll push the patch in a week or so. Thanks for the
contribution.
Simon
^ 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
* Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own
2022-01-28 22:41 ` Andrew Burgess
@ 2022-01-29 1:20 ` Simon Marchi
2022-01-29 8:04 ` Vasili Burdo
2022-01-30 2:17 ` Simon Marchi
0 siblings, 2 replies; 9+ messages in thread
From: Simon Marchi @ 2022-01-29 1:20 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, Vasili Burdo, Simon Marchi
> 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?
No, I haven't considered this, it is a good question. I really don't
know the TUI code well (if at all), so my thinking was that if the TUI
experts say it's ok, it's because it's ok :). But indeed, it would be
good to understand exactly what happens here.
I'll git a little bit. Vasili, if you happen to know why we have these
two behaviors (for_ui and !for_ui), feel free to answer.
Simon
^ 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-29 1:20 ` Simon Marchi
@ 2022-01-29 8:04 ` Vasili Burdo
2022-01-30 2:17 ` Simon Marchi
1 sibling, 0 replies; 9+ messages in thread
From: Vasili Burdo @ 2022-01-29 8:04 UTC (permalink / raw)
To: Simon Marchi; +Cc: Andrew Burgess, Vasili Burdo via Gdb-patches, Simon Marchi
Hi,
Vasili, if you happen to know why we have these
> two behaviors (for_ui and !for_ui), feel free to answer.
tui_disassembly() is used for 2 purposes: (1) on-screen text - ui, and (2)
for setting scroll position.
As I said before, I tried to use new-style disassembly for (2) and subtle
scrolling problems we have now became just worse.
The scrolling code is quite complex and hard to understand, so I decided to
use new style disassembly for UI only while keeping old style for anything
else. That's the reason for 'for-ui' flag.
^ 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-29 1:20 ` Simon Marchi
2022-01-29 8:04 ` Vasili Burdo
@ 2022-01-30 2:17 ` Simon Marchi
2022-02-02 13:40 ` Andrew Burgess
1 sibling, 1 reply; 9+ messages in thread
From: Simon Marchi @ 2022-01-30 2:17 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Vasili Burdo, Simon Marchi, gdb-patches
On 2022-01-28 20:20, Simon Marchi via Gdb-patches wrote:
>> 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?
>
> No, I haven't considered this, it is a good question. I really don't
> know the TUI code well (if at all), so my thinking was that if the TUI
> experts say it's ok, it's because it's ok :). But indeed, it would be
> good to understand exactly what happens here.
>
> I'll git a little bit. Vasili, if you happen to know why we have these
> two behaviors (for_ui and !for_ui), feel free to answer.
Ok, so it in fact "breaks" page up / down, in the sense that the number
of lines scrolled up and down is not right. This makes sense, as the
other uses of tui_disassemble are to help with scrolling. It is used to
answer the question: assuming that we are currently showing disassembly
starting at this PC, what would be the starting PC if going up/down N
lines?
Without this patch, let's say that my asm windows shows:
│ 0x110c <__do_global_dtors_aux+76> nopl 0x0(%rax) │
│ 0x1110 <frame_dummy> endbr64 │
│ 0x1114 <frame_dummy+4> jmp 0x1080 <register_tm_clones> │
│ 0x1119 <main> push %rbp │
│ 0x111a <main+1> mov %rsp,%rbp │
│ 0x111d <main+4> movq $0x0,-0x8(%rbp) │
│ 0x1125 <main+12> mov -0x8(%rbp),%rax │
│ 0x1129 <main+16> mov (%rax),%eax │
│ 0x112b <main+18> pop %rbp │
│ 0x112c <main+19> ret │
Doing page down, the goal (at least my understanding of it) is that the
last shown line becomes the first one. The result is:
│ 0x112c <main+19> ret │
│ 0x112d nopl (%rax) │
│ 0x1130 <__libc_csu_init> endbr64 │
│ 0x1134 <__libc_csu_init+4> push %r15 │
│ 0x1136 <__libc_csu_init+6> lea 0x2ceb(%rip),%r15 # 0x3e28 │
│ 0x113d <__libc_csu_init+13> push %r14 │
│ 0x113f <__libc_csu_init+15> mov %rdx,%r14 │
│ 0x1142 <__libc_csu_init+18> push %r13 │
│ 0x1144 <__libc_csu_init+20> mov %rsi,%r13 │
│ 0x1147 <__libc_csu_init+23> push %r12 │
With this patch applied, with the same starting position (0x110c being
the first instruction shown), I have:
│ __do_global_dtors_aux: │
│ 0x110c <+76> nopl 0x0(%rax) │
│ frame_dummy: │
│ 0x1110 <+0> endbr64 │
│ 0x1114 <+4> jmp 0x1080 <register_tm_clones> │
│ main: │
│ 0x1119 <+0> push %rbp │
│ 0x111a <+1> mov %rsp,%rbp │
│ 0x111d <+4> movq $0x0,-0x8(%rbp) │
│ 0x1125 <+12> mov -0x8(%rbp),%rax │
After page down:
│ main: │
│ 0x112c <+19> ret │
│ nopl (%rax) │
│ __libc_csu_init: │
│ 0x1130 <+0> endbr64 │
│ 0x1134 <+4> push %r15 │
│ 0x1136 <+6> lea 0x2ceb(%rip),%r15 # 0x3e28 │
│ 0x113d <+13> push %r14 │
│ 0x113f <+15> mov %rdx,%r14 │
│ 0x1142 <+18> push %r13 │
Since 0x1125 is the last instruction shown before page down, it should
be the first one after page down. But it is 0x11c2 - the same as we
would have gotten before the patch. It's clear now: since the code to
compute where we land after a page down passes for_ui == false, that
computation is done using the lines of the old layout, so the new
starting address is the same, 0x112c. But since the layout shown in the
UI is different, that means we completely miss instructions 0x1129 and
0x112b.
So it is now obvious that the for_ui flag is wrong, we need to do the
scrolling computation considering the new layout.
Simon
^ 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-30 2:17 ` Simon Marchi
@ 2022-02-02 13:40 ` Andrew Burgess
2022-02-02 14:33 ` Simon Marchi
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2022-02-02 13:40 UTC (permalink / raw)
To: Simon Marchi; +Cc: Vasili Burdo, Simon Marchi, gdb-patches
* Simon Marchi <simon.marchi@polymtl.ca> [2022-01-29 21:17:43 -0500]:
> On 2022-01-28 20:20, Simon Marchi via Gdb-patches wrote:
> >> 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?
> >
> > No, I haven't considered this, it is a good question. I really don't
> > know the TUI code well (if at all), so my thinking was that if the TUI
> > experts say it's ok, it's because it's ok :). But indeed, it would be
> > good to understand exactly what happens here.
> >
> > I'll git a little bit. Vasili, if you happen to know why we have these
> > two behaviors (for_ui and !for_ui), feel free to answer.
>
> Ok, so it in fact "breaks" page up / down, in the sense that the number
> of lines scrolled up and down is not right. This makes sense, as the
> other uses of tui_disassemble are to help with scrolling. It is used to
> answer the question: assuming that we are currently showing disassembly
> starting at this PC, what would be the starting PC if going up/down N
> lines?
>
> Without this patch, let's say that my asm windows shows:
>
> │ 0x110c <__do_global_dtors_aux+76> nopl 0x0(%rax) │
> │ 0x1110 <frame_dummy> endbr64 │
> │ 0x1114 <frame_dummy+4> jmp 0x1080 <register_tm_clones> │
> │ 0x1119 <main> push %rbp │
> │ 0x111a <main+1> mov %rsp,%rbp │
> │ 0x111d <main+4> movq $0x0,-0x8(%rbp) │
> │ 0x1125 <main+12> mov -0x8(%rbp),%rax │
> │ 0x1129 <main+16> mov (%rax),%eax │
> │ 0x112b <main+18> pop %rbp │
> │ 0x112c <main+19> ret │
>
> Doing page down, the goal (at least my understanding of it) is that the
> last shown line becomes the first one. The result is:
>
> │ 0x112c <main+19> ret │
> │ 0x112d nopl (%rax) │
> │ 0x1130 <__libc_csu_init> endbr64 │
> │ 0x1134 <__libc_csu_init+4> push %r15 │
> │ 0x1136 <__libc_csu_init+6> lea 0x2ceb(%rip),%r15 # 0x3e28 │
> │ 0x113d <__libc_csu_init+13> push %r14 │
> │ 0x113f <__libc_csu_init+15> mov %rdx,%r14 │
> │ 0x1142 <__libc_csu_init+18> push %r13 │
> │ 0x1144 <__libc_csu_init+20> mov %rsi,%r13 │
> │ 0x1147 <__libc_csu_init+23> push %r12 │
>
> With this patch applied, with the same starting position (0x110c being
> the first instruction shown), I have:
>
> │ __do_global_dtors_aux: │
> │ 0x110c <+76> nopl 0x0(%rax) │
> │ frame_dummy: │
> │ 0x1110 <+0> endbr64 │
> │ 0x1114 <+4> jmp 0x1080 <register_tm_clones> │
> │ main: │
> │ 0x1119 <+0> push %rbp │
> │ 0x111a <+1> mov %rsp,%rbp │
> │ 0x111d <+4> movq $0x0,-0x8(%rbp) │
> │ 0x1125 <+12> mov -0x8(%rbp),%rax │
>
> After page down:
>
> │ main: │
> │ 0x112c <+19> ret │
> │ nopl (%rax) │
> │ __libc_csu_init: │
> │ 0x1130 <+0> endbr64 │
> │ 0x1134 <+4> push %r15 │
> │ 0x1136 <+6> lea 0x2ceb(%rip),%r15 # 0x3e28 │
> │ 0x113d <+13> push %r14 │
> │ 0x113f <+15> mov %rdx,%r14 │
> │ 0x1142 <+18> push %r13 │
>
> Since 0x1125 is the last instruction shown before page down, it should
> be the first one after page down. But it is 0x11c2 - the same as we
> would have gotten before the patch. It's clear now: since the code to
> compute where we land after a page down passes for_ui == false, that
> computation is done using the lines of the old layout, so the new
> starting address is the same, 0x112c. But since the layout shown in the
> UI is different, that means we completely miss instructions 0x1129 and
> 0x112b.
>
> So it is now obvious that the for_ui flag is wrong, we need to do the
> scrolling computation considering the new layout.
Thanks for looking into this. I hadn't considered the page up/down
case when I was looking for issues.
I guess you'll post another version without the for_ui flag?
Vasili, if you can find reproducers for the scrolling problems you are
seeing then we should probably look into those separately.
Thanks,
Andrew
^ 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-02-02 13:40 ` Andrew Burgess
@ 2022-02-02 14:33 ` Simon Marchi
0 siblings, 0 replies; 9+ messages in thread
From: Simon Marchi @ 2022-02-02 14:33 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi; +Cc: Vasili Burdo, gdb-patches
> Thanks for looking into this. I hadn't considered the page up/down
> case when I was looking for issues.
>
> I guess you'll post another version without the for_ui flag?
>
> Vasili, if you can find reproducers for the scrolling problems you are
> seeing then we should probably look into those separately.
Yes, I am looking into making a new version that handles better page
up/down. The thing is that we have to adjust
tui_find_disassembly_address to work with the fact that tui_disassemble
can sometimes produce 1 or 2 lines per disassembled address, this is not
trivial.
Simon
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-02 14:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2022-01-29 1:20 ` Simon Marchi
2022-01-29 8:04 ` Vasili Burdo
2022-01-30 2:17 ` Simon Marchi
2022-02-02 13:40 ` Andrew Burgess
2022-02-02 14:33 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).