Re: [PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

Shahab,

I took a look at you patch last night, and there were a couple of
things I thought should be tweaked.  I wanted to remove the trailing
addresses with no content, and ideally have the window stop scrolling
at the last valid instruction - much like the source window does with
its content.  I also saw in a test here that I couldn't scroll back to
the first instruction (test file was single asm file containing 35
1-byte nop instructions).

I initially intended to just suggest how you might do that, but in
trying to figure out what to suggest I ended up with the patch below.

This still needs some more work - I was initially testing this with an
asm file containing ~35 nop instructions, and just scrolling that up
and down, and this worked fine.

But then I took a look at:
  https://sourceware.org/bugzilla/show_bug.cgi?id=9765
and tried scrolling up and down in a helloworld binary.  For some
reason on _that_ test the scrolling seems to get "stuck" at the end,
and when I use Page-Up to scroll up I see the test get "stuck"
alternating between two addresses and not actually making progress at
scrolling up.

In summary, I think the patch below would be a good starting point,
but it needs some more work to fix the last few nits.  I'll take a
look at this tomorrow or Sunday.

[ NOTE: I've been using Pedro's patch to prevent exceptions entering
  readline in my tree too. ]

Feedback welcome,

Thanks,
Andrew

---

commit 354f0a9f7a9c0edfb862d43656c21119fe9007e2
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Sat Jan 11 01:38:28 2020 +0000

    gdb/tui: Make asm window handle reading invalid memory
    
    When scrolling the asm window, if we try to disassemble invalid memory
    this should not prevent the window displaying something sane for those
    addresses that can be disassembled.x
    
    gdb/ChangeLog:
    
            * tui/tui-disasm.c (tui_disassemble): Update header comment,
            remove unneeded parameter, add try/catch around gdb_print_insn,
            rewrite to add items to asm_lines vector.
            (tui_find_disassembly_address): Don't assume the length of
            asm_lines after calling tui_disassemble, update for changes in
            tui_disassemble API.
            (tui_disasm_window::set_contents): Likewise.
    
    Change-Id: I292f4b8d571516ca8b25d9d60c85228c98222086

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 98c691f3387..eeecaec43bf 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -81,13 +81,21 @@ 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.  Return the address of the count'th
+   instruction after pc.  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.
+
+   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 ();
@@ -96,10 +104,31 @@ tui_disassemble (struct gdbarch *gdbarch,
   /* 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)
+	{
+	  /* If pc points to an invalid address then we'll catch a
+	     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)
@@ -107,19 +136,14 @@ tui_disassemble (struct gdbarch *gdbarch,
 	  size_t new_size;
 
 	  if (term_out)
-	    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+	    new_size = len_without_escapes (tal.addr_string);
 	  else
-	    new_size = asm_lines[pos + i].addr_string.size ();
+	    new_size = tal.addr_string.size ();
 	  *addr_size = std::max (*addr_size, new_size);
-	  asm_lines[pos + i].addr_size = new_size;
+	  tal.addr_size = new_size;
 	}
 
-      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
-
-      asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
-
-      /* Reset the buffer to empty.  */
-      gdb_dis_out.clear ();
+      asm_lines.push_back (tal);
     }
   return pc;
 }
@@ -137,41 +161,54 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
   if (max_lines <= 1)
     return pc;
 
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
 
   new_low = pc;
   if (from > 0)
     {
-      tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-      new_low = asm_lines[max_lines - 1].addr;
+      tui_disassemble (gdbarch, asm_lines, pc, max_lines);
+      new_low = asm_lines.back ().addr;
     }
   else
     {
       CORE_ADDR last_addr;
-      int pos;
       struct bound_minimal_symbol msymbol;
 
       /* Find backward an address which is a symbol and for which
          disassembling from that address will fill completely the
          window.  */
-      pos = max_lines - 1;
-      do {
-         new_low -= 1 * max_lines;
-         msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
+      do
+	{
+	  /* We want to move back at least one byte.  */
+	  new_low -= 1;
+
+	  /* If there's a symbol covering this address then jump back to
+	     the address of this symbol.  */
+	  msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
+	  if (msymbol.minsym)
+	    new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
+	  else
+	    new_low += 1;
+
+	  /* Disassemble forward a few lines and see where we got to.  */
+	  tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+	  last_addr = asm_lines.back ().addr;
 
-         if (msymbol.minsym)
-            new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-         else
-            new_low += 1 * max_lines;
+	} while (last_addr >= pc && msymbol.minsym != nullptr);
 
-         tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines);
-         last_addr = asm_lines[pos].addr;
-      } while (last_addr > pc && msymbol.minsym);
+      /* If we failed to disassemble the required number of lines then the
+	 following walk forward is not going to work.  And what would it
+	 mean to try and walk forward through memory we know can't be
+	 disassembled?  Just return the original address which should
+	 indicate we can't move backward in this case.  */
+      if (asm_lines.size () < max_lines)
+	return pc;
 
       /* Scan forward disassembling one instruction at a time until
          the last visible instruction of the window matches the pc.
          We keep the disassembled instructions in the 'lines' window
          and shift it downward (increasing its addresses).  */
+      int pos = max_lines - 1;
       if (last_addr < pc)
         do
           {
@@ -181,12 +218,15 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
             if (pos >= max_lines)
               pos = 0;
 
-            next_addr = tui_disassemble (gdbarch, asm_lines,
-					 last_addr, pos, 1);
+	    std::vector<tui_asm_line> single_asm_line;
+	    next_addr = tui_disassemble (gdbarch, single_asm_line,
+					 last_addr, 1);
 
             /* If there are some problems while disassembling exit.  */
             if (next_addr <= last_addr)
               break;
+	    gdb_assert (single_asm_line.size () == 1);
+	    asm_lines[pos] = single_asm_line[0];
             last_addr = next_addr;
           } while (last_addr <= pc);
       pos++;
@@ -224,9 +264,9 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
   line_width = width - TUI_EXECINFO_SIZE - 2;
 
   /* Get temporary table that will hold all strings (addr & insn).  */
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
   size_t addr_size = 0;
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
+  tui_disassemble (gdbarch, asm_lines, pc, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -237,17 +277,29 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
     {
       tui_source_element *src = &content[i];
 
-      std::string line
-	= (asm_lines[i].addr_string
-	   + n_spaces (insn_pos - asm_lines[i].addr_size)
-	   + asm_lines[i].insn);
+      std::string line;
+      CORE_ADDR addr;
+
+      if (i < asm_lines.size ())
+	{
+	  line
+	    = (asm_lines[i].addr_string
+	       + n_spaces (insn_pos - asm_lines[i].addr_size)
+	       + asm_lines[i].insn);
+	  addr = asm_lines[i].addr;
+	}
+      else
+	{
+	  line = "";
+	  addr = 0;
+	}
 
       const char *ptr = line.c_str ();
       src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0);
 
       src->line_or_addr.loa = LOA_ADDRESS;
-      src->line_or_addr.u.addr = asm_lines[i].addr;
-      src->is_exec_point = asm_lines[i].addr == cur_pc;
+      src->line_or_addr.u.addr = addr;
+      src->is_exec_point = (addr == cur_pc && line.size () > 0);
     }
   return true;
 }