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

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 21335080d31..e238355dc11 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -666,24 +666,18 @@  msym_prefer_to_msym_type (lookup_msym_prefer prefer)
   gdb_assert_not_reached ("unhandled 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 matches SECTION (which is not NULL).
-   Returns a pointer to the minimal symbol if such a symbol is found,
-   or NULL if PC is not in a suitable range.
+/* See minsyms.h.
+
    Note that we need to look through ALL the minimal symbol tables
    before deciding on the symbol that comes closest to the specified PC.
    This is because objfiles can overlap, for example objfile A has .text
    at 0x100 and .data at 0x40000 and objfile B has .text at 0x234 and
-   .data at 0x40048.
-
-   If WANT_TRAMPOLINE is set, prefer mst_solib_trampoline symbols when
-   there are text and trampoline symbols at the same address.
-   Otherwise prefer mst_text symbols.  */
+   .data at 0x40048.  */
 
 bound_minimal_symbol
 lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *section,
-				     lookup_msym_prefer prefer)
+				     lookup_msym_prefer prefer,
+				     bound_minimal_symbol *previous)
 {
   int lo;
   int hi;
@@ -693,6 +687,12 @@  lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
   struct objfile *best_objfile = NULL;
   struct bound_minimal_symbol result;
 
+  if (previous != nullptr)
+    {
+      previous->minsym = nullptr;
+      previous->objfile = nullptr;
+    }
+
   if (section == NULL)
     {
       section = find_pc_section (pc_in);
@@ -886,8 +886,23 @@  lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 		  if (best_zero_sized != -1)
 		    hi = best_zero_sized;
 		  else
-		    /* Go on to the next object file.  */
-		    continue;
+		    {
+		      /* If needed record this symbol as the closest
+			 previous symbol.  */
+		      if (previous != nullptr)
+			{
+			  if (previous->minsym == nullptr
+			      || (MSYMBOL_VALUE_RAW_ADDRESS (&msymbol[hi])
+				  > MSYMBOL_VALUE_RAW_ADDRESS
+					(previous->minsym)))
+			    {
+			      previous->minsym = &msymbol[hi];
+			      previous->objfile = objfile;
+			    }
+			}
+		      /* Go on to the next object file.  */
+		      continue;
+		    }
 		}
 
 	      /* The minimal symbol indexed by hi now is the best one in this
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
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
+   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.  */
 
 struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
-  (CORE_ADDR,
-   struct obj_section *,
-   lookup_msym_prefer prefer = lookup_msym_prefer::TEXT);
+  (CORE_ADDR pc_in,
+   struct obj_section *section,
+   lookup_msym_prefer prefer = lookup_msym_prefer::TEXT,
+   bound_minimal_symbol *previous = nullptr);
 
 /* Backward compatibility: search through the minimal symbol table 
    for a matching PC (no section given).
diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index cec2735764e..f78baab1081 100644
--- 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
+# 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
+    # 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.
+   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 ();
   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)
+	{
+	  /* 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,23 +140,45 @@  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 (std::move (tal));
     }
   return pc;
 }
 
+/* Look backward from ADDR for an address from which we can start
+   disassembling, this needs to be something we can be reasonably
+   confident will fall on an instruction boundary.  We use msymbol
+   addresses, or the start of a section.  */
+
+static CORE_ADDR
+tui_find_backward_disassembly_start_address (CORE_ADDR addr)
+{
+  struct bound_minimal_symbol msym, msym_prev;
+
+  msym = lookup_minimal_symbol_by_pc_section (addr - 1, nullptr,
+					      lookup_msym_prefer::TEXT,
+					      &msym_prev);
+  if (msym.minsym != nullptr)
+    return BMSYMBOL_VALUE_ADDRESS (msym);
+  else if (msym_prev.minsym != nullptr)
+    return BMSYMBOL_VALUE_ADDRESS (msym_prev);
+
+  /* Find the section that ADDR is in, and look for the start of the
+     section.  */
+  struct obj_section *section = find_pc_section (addr);
+  if (section != NULL)
+    return obj_section_addr (section);
+
+  return addr;
+}
+
 /* Find the disassembly address that corresponds to FROM lines above
    or below the PC.  Variable sized instructions are taken into
    account by the algorithm.  */
@@ -134,65 +189,125 @@  tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
   int max_lines;
 
   max_lines = (from > 0) ? from : - from;
-  if (max_lines <= 1)
+  if (max_lines == 0)
     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;
+      /* Always disassemble 1 extra instruction here, then if the last
+	 instruction fails to disassemble we will take the address of the
+	 previous instruction that did disassemble as the result.  */
+      tui_disassemble (gdbarch, asm_lines, pc, max_lines + 1);
+      new_low = asm_lines.back ().addr;
     }
   else
     {
+      /* In order to disassemble backwards we need to find a suitable
+	 address to start disassembling from and then work forward until we
+	 re-find the address we're currently at.  We can then figure out
+	 which address will be at the top of the TUI window after our
+	 backward scroll.  During our backward disassemble we need to be
+	 able to distinguish between the case where the last address we
+	 _can_ disassemble is ADDR, and the case where the disassembly
+	 just happens to stop at ADDR, for this reason we increase
+	 MAX_LINES by one.  */
+      max_lines++;
+
+      /* When we disassemble a series of instructions this will hold the
+	 address of the last instruction disassembled.  */
       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);
-
-         if (msymbol.minsym)
-            new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-         else
-            new_low += 1 * max_lines;
-
-         tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines);
-         last_addr = asm_lines[pos].addr;
-      } while (last_addr > pc && msymbol.minsym);
+
+      /* And this will hold the address of the next instruction that would
+	 have been disassembled.  */
+      CORE_ADDR next_addr;
+
+      /* As we search backward if we find an address that looks like a
+	 promising starting point then we record it in this structure.  If
+	 the next address we try is not a suitable starting point then we
+	 will fall back to the address held here.  */
+      gdb::optional<CORE_ADDR> possible_new_low;
+
+      /* The previous value of NEW_LOW so we know if the new value is
+	 different or not.  */
+      CORE_ADDR prev_low;
+
+      do
+	{
+	  /* Find an address from which we can start disassembling.  */
+	  prev_low = new_low;
+	  new_low = tui_find_backward_disassembly_start_address (new_low);
+
+	  /* Disassemble forward.  */
+	  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+	  last_addr = asm_lines.back ().addr;
+
+	  /* If disassembling from the current value of NEW_LOW reached PC
+	     (or went past it) then this would do as a starting point if we
+	     can't find anything better, so remember it.  */
+	  if (last_addr >= pc && new_low != prev_low
+	      && asm_lines.size () >= max_lines)
+	    possible_new_low.emplace (new_low);
+
+	  /* Continue searching until we find a value of NEW_LOW from which
+	     disassembling MAX_LINES instructions doesn't reach PC.  We
+	     know this means we can find the required number of previous
+	     instructions then.  */
+	}
+      while ((last_addr > pc
+	      || (last_addr == pc && asm_lines.size () < max_lines))
+	     && new_low != prev_low);
+
+      /* If we failed to disassemble the required number of lines then the
+	 following walk forward is not going to work, it assumes that
+	 ASM_LINES contains exactly MAX_LINES entries.  Instead we should
+	 consider falling back to a previous possible start address in
+	 POSSIBLE_NEW_LOW.  */
+      if (asm_lines.size () < max_lines)
+	{
+	  if (!possible_new_low.has_value ())
+	    return pc;
+
+	  /* Take the best possible match we have.  */
+	  new_low = *possible_new_low;
+	  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+	  last_addr = asm_lines.back ().addr;
+	  gdb_assert (asm_lines.size () >= max_lines);
+	}
 
       /* 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
           {
-            CORE_ADDR next_addr;
-
             pos++;
             if (pos >= max_lines)
               pos = 0;
 
-            next_addr = tui_disassemble (gdbarch, asm_lines,
-					 last_addr, pos, 1);
-
+	    CORE_ADDR old_next_addr = next_addr;
+	    std::vector<tui_asm_line> single_asm_line;
+	    next_addr = tui_disassemble (gdbarch, single_asm_line,
+					 next_addr, 1);
             /* If there are some problems while disassembling exit.  */
-            if (next_addr <= last_addr)
-              break;
-            last_addr = next_addr;
-          } while (last_addr <= pc);
+	    if (next_addr <= old_next_addr)
+	      return pc;
+	    gdb_assert (single_asm_line.size () == 1);
+	    asm_lines[pos] = single_asm_line[0];
+	  } while (next_addr <= pc);
       pos++;
       if (pos >= max_lines)
          pos = 0;
       new_low = asm_lines[pos].addr;
+
+      /* When scrolling backward the addresses should move backward, or at
+	 the very least stay the same if we are at the first address that
+	 can be disassembled.  */
+      gdb_assert (new_low <= pc);
     }
   return new_low;
 }
@@ -224,9 +339,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 +352,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;
 }
@@ -326,10 +453,6 @@  tui_disasm_window::do_scroll_vertical (int num_to_scroll)
       CORE_ADDR pc;
 
       pc = start_line_or_addr.u.addr;
-      if (num_to_scroll >= 0)
-	num_to_scroll++;
-      else
-	--num_to_scroll;
 
       symtab_and_line sal {};
       sal.pspace = current_program_space;