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

[Pedro Alves <palves@redhat.com>]

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 "<main>"
> +
> +# 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