[PATCH] GDB: Treat memory exception from tui_disassemble() gracefully

[Andrew Burgess]

* Shahab Vahedi <shahab.vahedi@gmail.com> [2020-01-08 16:25:50 +0100]:

> From: Shahab Vahedi <shahab@synopsys.com>
> 
> In TUI mode, when the assembly layout reaches the end of a binary,
> GDB wants to disassemle the addresses beyond the last valid ones.
> This results in a "MEMORY_ERROR" exception to be thrown when
> tui_disasm_window::set_contents() invokes tui_disassemble(). When
> that happens set_contents() bails out prematurely without filling
> the "content" for the valid addresses. This eventually leads to
> no assembly lines or termination of GDB when you scroll down to
> the last lines of the program.
> 
> With this change, tui_disassemble() is surrounded with try/catch.
> If a MEMORY_ERROR exception is caught, it is ignored.
> 
> The issue has been discussed at length in bug 25345:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=25345
> 
> gdb/ChangeLog:
> 2020-01-08  Shahab Vahedi  <shahab@synopsys.com>
>         * tui/tui-disasm.c (tui_disasm_window::set_contents):
>         Handle MEMORY_ERROR exception gracefully.

Thanks for taking a look at this.

I have some initial thoughts, maybe these are worth investigating
before we merge this fix:

 - I wonder if the try/catch would be better inside tui_disassemble in
   some way.  I don't know the exact implications of this, but
   tui_disassemble is called from ::set_contents and (through a couple
   of layers) ::do_scroll_vertical, surely we'd want both of these
   cases to not throw exceptions just because we hit unreadable
   memory?

 - While I write the above I realise this might be the reason of the
   failure you mention seeing, even with this patch, at the bug report
   above.

 - I also notice that there's some nasty over highlighting with this
   patch, all of the blank lines are, for some reason, marked as being
   "the line we're currently executing on", which is clearly wrong.  I
   didn't dig into why this is, but we should probably try to fix
   this, or at least understand what the issue is here.

This was discussed on IRC and it was pointed out that this bug also
exists:
  https://sourceware.org/bugzilla/show_bug.cgi?id=9765

If/when this fix is merged you should be able to mark that resolved
too!

Thanks,
Andrew




> ---
>  gdb/tui/tui-disasm.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index 98c691f3387..7faaa45f039 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -226,7 +226,18 @@ 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 (max_lines);
>    size_t addr_size = 0;
> -  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
> +  try
> +    {
> +      tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
> +    }
> +  catch (const gdb_exception &except)
> +    {
> +      /* In cases where max_lines is asking tui_disassemble() to fetch
> +	 too much, like when PC goes past the valid address range, a
> +	 MEMORY_ERROR is thrown, but it is alright.  */
> +      if (except.error != MEMORY_ERROR)
> +	  throw;
> +    }
>  
>    /* Align instructions to the same column.  */
>    insn_pos = (1 + (addr_size / tab_len)) * tab_len;
> -- 
> 2.24.1
>