Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Pedro Alves
2020-01-22T17:48:34Z

Hi!

I think this should be filed under PR tui/9765 too?

I didn't try to grok the code very deeply. I did try it out, and
saw that it works as described. I'm happy with that.
Thanks for doing all this.

Tiny nits below, mostly comments issues I noticed while
skimming the patch.

> index 98735e75e33..a8d50a463ba 100644
> --- a/gdb/minsyms.h
> +++ b/gdb/minsyms.h
> @@ -240,7 +240,9 @@ enum class lookup_msym_prefer
>
> /* Search through the minimal symbol table for each objfile and find
> the symbol whose address is the largest address that is still less
> - than or equal to PC, and which matches SECTION.
> + than or equal to PC_IN, and which matches SECTION. A matching symbol
> + must either by zero sized and have address PC_IN, or PC_IN must fall

"BE zero sized", I think.

> + within the range of addresses covered by the matching symbol.
>
> If SECTION is NULL, this uses the result of find_pc_section
> instead.
> @@ -249,12 +251,17 @@ enum class lookup_msym_prefer
> found, or NULL if PC is not in a suitable range.
>
> See definition of lookup_msym_prefer for description of PREFER. By
> - default mst_text symbols are preferred. */
> + default mst_text symbols are preferred.
> +
> + If the PREVIOUS pointer is non-NULL, and no matching symbol is found,
> + then the contents will be set to reference the closest symbol before
> + PC_IN. */
>

> --- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> +++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> @@ -32,3 +32,44 @@ if {![Term::prepare_for_tui]} {
> # This puts us into TUI mode, and should display the ASM window.
> Term::command "layout asm"
> Term::check_box_contents "check asm box contents" 0 0 80 15 " "
> +
> +# Scroll the ASM window down using the down arrow key. In an ideal
> +# world I'd like to use PageDown here, but currently our terminal

Please avoid "I" in comments.

For example, you could write "In an ideal world we'd use".

> +# library doesn't support such advanced things.
> +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]
> +
> + # Except, if the second 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 wont change, so the call to Term::wait_for below

wont -> won't

> + # will just timeout. So for now we avoid testing the edge case.
> + if {[regexp -- "^\\| +\\|$" $line]} {
> + # Second line is blank, we're at the end of the assembler.
> + pass $testname
> + break
> + }
> +
> + # Send the down key to GDB.
> + send_gdb "\033\[B"
> + incr down_count
> + if {[Term::wait_for [string_to_regexp $line]] \
> + && [Term::get_line 1] == $line} {
> + # We scrolled successfully.
> + } else {
> + fail "$testname (scroll failed)"
> + Term::dump_screen
> + break
> + }
> +
> + if { $down_count > 250 } {
> + # Maybe we should accept this as a pass in case a target
> + # really does have loads of assembler to scroll through.
> + fail "$testname (too much assembler)"
> + Term::dump_screen
> + break
> + }
> +}
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index 98c691f3387..5b606dcf696 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -81,25 +81,58 @@ len_without_escapes (const std::string &str)
> return len;
> }
>
> -/* Function to set the disassembly window's content.
> - Disassemble count lines starting at pc.
> - Return address of the count'th instruction after pc. */
> +/* Function to disassemble up to COUNT instructions starting from address
> + PC into the ASM_LINES vector (which will be emptied of any previous
> + contents). Return the address of the count'th instruction after pc.

Uppercase COUNT in "COUNT'th" ?

( I'm not sure I'm able to pronounce that. :-) )

> + When ADDR_SIZE is non-null then place the maximum size of an address and
> + label into the value pointed to by ADDR_SIZE, and set the addr_size
> + field on each item in ASM_LINES, otherwise the addr_size fields within
> + asm_lines are undefined.

Uppercase last ASM_LINES ?

> +
> + 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
> + fewer entries than requested. */
> static CORE_ADDR
> tui_disassemble (struct gdbarch *gdbarch,
> std::vector<tui_asm_line> &asm_lines,
> - CORE_ADDR pc, int pos, int count,
> + CORE_ADDR pc, int count,
> size_t *addr_size = nullptr)
> {
> bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
> string_file gdb_dis_out (term_out);
>
> + /* Must start with an empty list. */
> + asm_lines.clear ();
> +
> /* Now construct each line. */
> for (int i = 0; i < count; ++i)
> {
> - print_address (gdbarch, pc, &gdb_dis_out);
> - asm_lines[pos + i].addr = pc;
> - asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ());
> + tui_asm_line tal;
> + CORE_ADDR orig_pc = pc;
>
> + try
> + {
> + pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
> + }
> + catch (const gdb_exception &except)

This can be gdb_exception_error.

> + {
> + /* If pc points to an invalid address then we'll catch a

Uppercase PC.

> + MEMORY_ERROR here, this should stop the disassembly, but
> + otherwise is fine. */
> + if (except.error != MEMORY_ERROR)
> + throw;
> + return pc;
> + }
> +
> + /* Capture the disassembled instruction. */
> + tal.insn = std::move (gdb_dis_out.string ());
> + gdb_dis_out.clear ();
> +
> + /* 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 ());
> gdb_dis_out.clear ();
>
> if (addr_size != nullptr)

Thanks,
Pedro Alves