[PATCHv4 4/5] gdb: refactor the non-printing disassemblers

[Andrew Burgess <aburgess@redhat.com>]

This commit started from an observation I made while working on some
other disassembler patches, that is, that the function
gdb_buffered_insn_length, is broken ... sort of.

I noticed that the gdb_buffered_insn_length function doesn't set up
the application data field if the disassemble_info structure.

Further, I noticed that some architectures, for example, ARM, require
that the application_data field be set, see gdb_print_insn_arm in
arm-tdep.c.

And so, if we ever use gdb_buffered_insn_length for ARM, then GDB will
likely crash.  Which is why I said only "sort of" broken.  Right now
we don't use gdb_buffered_insn_length with ARM, so maybe it isn't
broken yet?

Anyway to prove to myself that there was a problem here I extended the
disassembler self tests in disasm-selftests.c to include a test of
gdb_buffered_insn_length.  As I run the test for all architectures, I
do indeed see GDB crash for ARM.

To fix this we need gdb_buffered_insn_length to create a disassembler
that inherits from gdb_disassemble_info, but we also need this new
disassembler to not print anything.

And so, I introduce a new gdb_non_printing_disassembler class, this is
a disassembler that doesn't print anything to the output stream.

I then observed that both ARC and S12Z also create non-printing
disassemblers, but these are slightly different.  While the
disassembler in gdb_non_printing_disassembler reads the instruction
from a buffer, the ARC and S12Z disassemblers read from target memory
using target_read_code.

And so, I further split gdb_non_printing_disassembler into two
sub-classes, gdb_non_printing_memory_disassembler and
gdb_non_printing_buffer_disassembler.

The new selftests now pass, but otherwise, there should be no user
visible changes after this commit.
---
 gdb/arc-linux-tdep.c   | 15 +++----
 gdb/arc-tdep.c         | 29 +++-----------
 gdb/arc-tdep.h         |  5 ---
 gdb/disasm-selftests.c | 70 ++++++++++++++++++++++++++-------
 gdb/disasm.c           | 88 ++++++++++++++++++------------------------
 gdb/disasm.h           | 56 ++++++++++++++++++++++++---
 gdb/s12z-tdep.c        | 26 +------------
 7 files changed, 158 insertions(+), 131 deletions(-)

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 13595f2e8e9..04ca38f1355 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -356,7 +356,7 @@ arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch,
    */
 
 static std::vector<CORE_ADDR>
-handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+handle_atomic_sequence (arc_instruction insn, disassemble_info *di)
 {
   const int atomic_seq_len = 24;    /* Instruction sequence length.  */
   std::vector<CORE_ADDR> next_pcs;
@@ -374,7 +374,7 @@ handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
   for (int insn_count = 0; insn_count < atomic_seq_len; ++insn_count)
     {
       arc_insn_decode (arc_insn_get_linear_next_pc (insn),
-		       &di, arc_delayed_print_insn, &insn);
+		       di, arc_delayed_print_insn, &insn);
 
       if (insn.insn_class == BRCC)
         {
@@ -412,15 +412,15 @@ arc_linux_software_single_step (struct regcache *regcache)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   arc_gdbarch_tdep *tdep = (arc_gdbarch_tdep *) gdbarch_tdep (gdbarch);
-  struct disassemble_info di = arc_disassemble_info (gdbarch);
+  struct gdb_non_printing_memory_disassembler dis (gdbarch);
 
   /* Read current instruction.  */
   struct arc_instruction curr_insn;
-  arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
-		   &curr_insn);
+  arc_insn_decode (regcache_read_pc (regcache), dis.disasm_info (),
+		   arc_delayed_print_insn, &curr_insn);
 
   if (curr_insn.insn_class == LLOCK)
-    return handle_atomic_sequence (curr_insn, di);
+    return handle_atomic_sequence (curr_insn, dis.disasm_info ());
 
   CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
   std::vector<CORE_ADDR> next_pcs;
@@ -431,7 +431,8 @@ arc_linux_software_single_step (struct regcache *regcache)
   if (curr_insn.has_delay_slot)
     {
       struct arc_instruction next_insn;
-      arc_insn_decode (next_pc, &di, arc_delayed_print_insn, &next_insn);
+      arc_insn_decode (next_pc, dis.disasm_info (), arc_delayed_print_insn,
+		       &next_insn);
       next_pcs.push_back (arc_insn_get_linear_next_pc (next_insn));
     }
   else
diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 98bd1c4bc0a..75fd3077ca7 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -1306,24 +1306,6 @@ arc_is_in_prologue (struct gdbarch *gdbarch, const struct arc_instruction &insn,
   return false;
 }
 
-/* See arc-tdep.h.  */
-
-struct disassemble_info
-arc_disassemble_info (struct gdbarch *gdbarch)
-{
-  struct disassemble_info di;
-  init_disassemble_info_for_no_printing (&di);
-  di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di.endian = gdbarch_byte_order (gdbarch);
-  di.read_memory_func = [](bfd_vma memaddr, gdb_byte *myaddr,
-			   unsigned int len, struct disassemble_info *info)
-    {
-      return target_read_code (memaddr, myaddr, len);
-    };
-  return di;
-}
-
 /* Analyze the prologue and update the corresponding frame cache for the frame
    unwinder for unwinding frames that doesn't have debug info.  In such
    situation GDB attempts to parse instructions in the prologue to understand
@@ -1394,9 +1376,10 @@ arc_analyze_prologue (struct gdbarch *gdbarch, const CORE_ADDR entrypoint,
   while (current_prologue_end < limit_pc)
     {
       struct arc_instruction insn;
-      struct disassemble_info di = arc_disassemble_info (gdbarch);
-      arc_insn_decode (current_prologue_end, &di, arc_delayed_print_insn,
-		       &insn);
+
+      struct gdb_non_printing_memory_disassembler dis (gdbarch);
+      arc_insn_decode (current_prologue_end, dis.disasm_info (),
+		       arc_delayed_print_insn, &insn);
 
       if (arc_debug)
 	arc_insn_dump (insn);
@@ -2460,8 +2443,8 @@ dump_arc_instruction_command (const char *args, int from_tty)
 
   CORE_ADDR address = value_as_address (val);
   struct arc_instruction insn;
-  struct disassemble_info di = arc_disassemble_info (target_gdbarch ());
-  arc_insn_decode (address, &di, arc_delayed_print_insn, &insn);
+  struct gdb_non_printing_memory_disassembler dis (target_gdbarch ());
+  arc_insn_decode (address, dis.disasm_info (), arc_delayed_print_insn, &insn);
   arc_insn_dump (insn);
 }
 
diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
index ceca003204f..53e5d8476fc 100644
--- a/gdb/arc-tdep.h
+++ b/gdb/arc-tdep.h
@@ -186,11 +186,6 @@ arc_arch_is_em (const struct bfd_arch_info* arch)
    can't be set to an actual NULL value - that would cause a crash.  */
 int arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info);
 
-/* Return properly initialized disassemble_info for ARC disassembler - it will
-   not print disassembled instructions to stderr.  */
-
-struct disassemble_info arc_disassemble_info (struct gdbarch *gdbarch);
-
 /* Get branch/jump target address for the INSN.  Note that this function
    returns branch target and doesn't evaluate if this branch is taken or not.
    For the indirect jumps value depends in register state, hence can change.
diff --git a/gdb/disasm-selftests.c b/gdb/disasm-selftests.c
index 928d26f7018..07586f04abd 100644
--- a/gdb/disasm-selftests.c
+++ b/gdb/disasm-selftests.c
@@ -25,13 +25,19 @@
 
 namespace selftests {
 
-/* Test disassembly of one instruction.  */
+/* Return a pointer to a buffer containing an instruction that can be
+   disassembled for architecture GDBARCH.  *LEN will be set to the length
+   of the returned buffer.
 
-static void
-print_one_insn_test (struct gdbarch *gdbarch)
+   If there's no known instruction to disassemble for GDBARCH (because we
+   haven't figured on out, not because no instructions exist) then nullptr
+   is returned, and *LEN is set to 0.  */
+
+static const gdb_byte *
+get_test_insn (struct gdbarch *gdbarch, size_t *len)
 {
-  size_t len = 0;
-  const gdb_byte *insn = NULL;
+  *len = 0;
+  const gdb_byte *insn = nullptr;
 
   switch (gdbarch_bfd_arch_info (gdbarch)->arch)
     {
@@ -40,34 +46,34 @@ print_one_insn_test (struct gdbarch *gdbarch)
       static const gdb_byte bfin_insn[] = {0x17, 0xe1, 0xff, 0xff};
 
       insn = bfin_insn;
-      len = sizeof (bfin_insn);
+      *len = sizeof (bfin_insn);
       break;
     case bfd_arch_arm:
       /* mov     r0, #0 */
       static const gdb_byte arm_insn[] = {0x0, 0x0, 0xa0, 0xe3};
 
       insn = arm_insn;
-      len = sizeof (arm_insn);
+      *len = sizeof (arm_insn);
       break;
     case bfd_arch_ia64:
     case bfd_arch_mep:
     case bfd_arch_mips:
     case bfd_arch_tic6x:
     case bfd_arch_xtensa:
-      return;
+      return insn;
     case bfd_arch_s390:
       /* nopr %r7 */
       static const gdb_byte s390_insn[] = {0x07, 0x07};
 
       insn = s390_insn;
-      len = sizeof (s390_insn);
+      *len = sizeof (s390_insn);
       break;
     case bfd_arch_xstormy16:
       /* nop */
       static const gdb_byte xstormy16_insn[] = {0x0, 0x0};
 
       insn = xstormy16_insn;
-      len = sizeof (xstormy16_insn);
+      *len = sizeof (xstormy16_insn);
       break;
     case bfd_arch_nios2:
     case bfd_arch_score:
@@ -78,13 +84,13 @@ print_one_insn_test (struct gdbarch *gdbarch)
       {
 	int bplen;
 	insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &bplen);
-	len = bplen;
+	*len = bplen;
       }
       break;
     case bfd_arch_arc:
       /* PR 21003 */
       if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601)
-	return;
+	return insn;
       goto generic_case;
     case bfd_arch_i386:
       {
@@ -93,7 +99,7 @@ print_one_insn_test (struct gdbarch *gdbarch)
 	   opcodes rejects an attempt to disassemble for an arch with
 	   a 64-bit address size when bfd_vma is 32-bit.  */
 	if (info->bits_per_address > sizeof (bfd_vma) * CHAR_BIT)
-	  return;
+	  return insn;
       }
       /* fall through */
     default:
@@ -105,12 +111,26 @@ print_one_insn_test (struct gdbarch *gdbarch)
 	int bplen;
 
 	insn = gdbarch_sw_breakpoint_from_kind (gdbarch, kind, &bplen);
-	len = bplen;
+	*len = bplen;
 
 	break;
       }
     }
-  SELF_CHECK (len > 0);
+  SELF_CHECK (*len > 0);
+
+  return insn;
+}
+
+/* Test disassembly of one instruction.  */
+
+static void
+print_one_insn_test (struct gdbarch *gdbarch)
+{
+  size_t len;
+  const gdb_byte *insn = get_test_insn (gdbarch, &len);
+
+  if (insn == nullptr)
+    return;
 
   /* Test gdb_disassembler for a given gdbarch by reading data from a
      pre-allocated buffer.  If you want to see the disassembled
@@ -175,6 +195,24 @@ print_one_insn_test (struct gdbarch *gdbarch)
   SELF_CHECK (di.print_insn (0) == len);
 }
 
+/* Test the gdb_buffered_insn_length function.  */
+
+static void
+buffered_insn_length_test (struct gdbarch *gdbarch)
+{
+  size_t buf_len;
+  const gdb_byte *insn = get_test_insn (gdbarch, &buf_len);
+
+  if (insn == nullptr)
+    return;
+
+  CORE_ADDR insn_address = 0;
+  int calculated_len = gdb_buffered_insn_length (gdbarch, insn, buf_len,
+						 insn_address);
+
+  SELF_CHECK (calculated_len == buf_len);
+}
+
 /* Test disassembly on memory error.  */
 
 static void
@@ -235,4 +273,6 @@ _initialize_disasm_selftests ()
 					 selftests::print_one_insn_test);
   selftests::register_test_foreach_arch ("memory_error",
 					 selftests::memory_error_test);
+  selftests::register_test_foreach_arch ("buffered_insn_length",
+					 selftests::buffered_insn_length_test);
 }
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 16e3c39b702..1dfa141b10b 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -996,66 +996,56 @@ gdb_insn_length (struct gdbarch *gdbarch, CORE_ADDR addr)
   return gdb_print_insn (gdbarch, addr, &null_stream, NULL);
 }
 
-/* An fprintf-function for use by the disassembler when we know we don't
-   want to print anything.  Always returns success.  */
+/* See disasm.h.  */
 
-static int ATTRIBUTE_PRINTF (2, 3)
-gdb_disasm_null_printf (void *stream, const char *format, ...)
+int
+gdb_non_printing_disassembler::null_fprintf_func (void *stream,
+						  const char *format, ...)
 {
   return 0;
 }
 
-/* An fprintf-function for use by the disassembler when we know we don't
-   want to print anything, and the disassembler is using style.  Always
-   returns success.  */
+/* See disasm.h.  */
 
-static int ATTRIBUTE_PRINTF (3, 4)
-gdb_disasm_null_styled_printf (void *stream,
-			       enum disassembler_style style,
-			       const char *format, ...)
+int
+gdb_non_printing_disassembler::null_fprintf_styled_func
+  (void *stream, enum disassembler_style style, const char *format, ...)
 {
   return 0;
 }
 
 /* See disasm.h.  */
 
-void
-init_disassemble_info_for_no_printing (struct disassemble_info *dinfo)
+int
+gdb_non_printing_memory_disassembler::dis_asm_read_memory
+  (bfd_vma memaddr, bfd_byte *myaddr, unsigned int length,
+   struct disassemble_info *dinfo)
 {
-  init_disassemble_info (dinfo, nullptr, gdb_disasm_null_printf,
-			 gdb_disasm_null_styled_printf);
+  return target_read_code (memaddr, myaddr, length);
 }
 
-/* Initialize a struct disassemble_info for gdb_buffered_insn_length.
-   Upon return, *DISASSEMBLER_OPTIONS_HOLDER owns the string pointed
-   to by DI.DISASSEMBLER_OPTIONS.  */
+/* A non-printing disassemble_info management class.  The disassemble_info
+   setup by this class will not print anything to the output stream (there
+   is no output stream), and the instruction to be disassembled will be
+   read from a buffer passed to the constructor.  */
 
-static void
-gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch,
-				   struct disassemble_info *di,
-				   const gdb_byte *insn, int max_len,
-				   CORE_ADDR addr,
-				   std::string *disassembler_options_holder)
+struct gdb_non_printing_buffer_disassembler
+  : public gdb_non_printing_disassembler
 {
-  init_disassemble_info_for_no_printing (di);
-
-  /* init_disassemble_info installs buffer_read_memory, etc.
-     so we don't need to do that here.
-     The cast is necessary until disassemble_info is const-ified.  */
-  di->buffer = (gdb_byte *) insn;
-  di->buffer_length = max_len;
-  di->buffer_vma = addr;
-
-  di->arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di->mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di->endian = gdbarch_byte_order (gdbarch);
-  di->endian_code = gdbarch_byte_order_for_code (gdbarch);
-
-  *disassembler_options_holder = get_all_disassembler_options (gdbarch);
-  if (!disassembler_options_holder->empty ())
-    di->disassembler_options = disassembler_options_holder->c_str ();
-  disassemble_init_for_target (di);
-}
+  /* Constructor.  GDBARCH is the architecture to disassemble for, BUFFER
+     contains the instruction to disassemble, and INSN_ADDRESS is the
+     address (in target memory) of the instruction to disassemble.  */
+  gdb_non_printing_buffer_disassembler (struct gdbarch *gdbarch,
+					gdb::array_view<const gdb_byte> buffer,
+					CORE_ADDR insn_address)
+    : gdb_non_printing_disassembler (gdbarch, nullptr)
+  {
+    /* The cast is necessary until disassemble_info is const-ified.  */
+    m_di.buffer = (gdb_byte *) buffer.data ();
+    m_di.buffer_length = buffer.size ();
+    m_di.buffer_vma = insn_address;
+  }
+};
 
 /* Return the length in bytes of INSN.  MAX_LEN is the size of the
    buffer containing INSN.  */
@@ -1064,14 +1054,10 @@ int
 gdb_buffered_insn_length (struct gdbarch *gdbarch,
 			  const gdb_byte *insn, int max_len, CORE_ADDR addr)
 {
-  struct disassemble_info di;
-  std::string disassembler_options_holder;
-
-  gdb_buffered_insn_length_init_dis (gdbarch, &di, insn, max_len, addr,
-				     &disassembler_options_holder);
-
-  int result = gdb_print_insn_1 (gdbarch, addr, &di);
-  disassemble_free_target (&di);
+  gdb::array_view<const gdb_byte> buffer
+    = gdb::make_array_view (insn, max_len);
+  gdb_non_printing_buffer_disassembler dis (gdbarch, buffer, addr);
+  int result = gdb_print_insn_1 (gdbarch, addr, dis.disasm_info ());
   return result;
 }
 
diff --git a/gdb/disasm.h b/gdb/disasm.h
index b3e40e2981e..6c1d7673b01 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -136,6 +136,56 @@ struct gdb_printing_disassembler : public gdb_disassemble_info
     ATTRIBUTE_PRINTF(3,4);
 };
 
+/* A basic disassembler that doesn't actually print anything.  */
+
+struct gdb_non_printing_disassembler : public gdb_disassemble_info
+{
+  gdb_non_printing_disassembler (struct gdbarch *gdbarch,
+				 read_memory_ftype read_memory_func)
+    : gdb_disassemble_info (gdbarch, nullptr /* stream */,
+			    read_memory_func,
+			    nullptr /* memory_error_func */,
+			    nullptr /* print_address_func */,
+			    null_fprintf_func,
+			    null_fprintf_styled_func)
+  { /* Nothing.  */ }
+
+private:
+
+  /* Callback used as the disassemble_info's fprintf_func callback, this
+     doesn't write anything to STREAM, but just returns 0.  */
+  static int null_fprintf_func (void *stream, const char *format, ...)
+    ATTRIBUTE_PRINTF(2,3);
+
+  /* Callback used as the disassemble_info's fprintf_styled_func callback,
+     , this doesn't write anything to STREAM, but just returns 0.  */
+  static int null_fprintf_styled_func (void *stream,
+				       enum disassembler_style style,
+				       const char *format, ...)
+    ATTRIBUTE_PRINTF(3,4);
+};
+
+/* A non-printing disassemble_info management class.  The disassemble_info
+   setup by this class will not print anything to the output stream (there
+   is no output stream), and the instruction to be disassembled will be
+   read from target memory.  */
+
+struct gdb_non_printing_memory_disassembler
+  : public gdb_non_printing_disassembler
+{
+  /* Constructor.  GDBARCH is the architecture to disassemble for.  */
+  gdb_non_printing_memory_disassembler (struct gdbarch *gdbarch)
+    :gdb_non_printing_disassembler (gdbarch, dis_asm_read_memory)
+  { /* Nothing.  */ }
+
+private:
+
+  /* Implements the read_memory_func disassemble_info callback.  */
+  static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
+				  unsigned int len,
+				  struct disassemble_info *info);
+};
+
 /* A dissassembler class that provides 'print_insn', a method for
    disassembling a single instruction to the output stream.  */
 
@@ -278,10 +328,4 @@ extern char *get_disassembler_options (struct gdbarch *gdbarch);
 
 extern void set_disassembler_options (const char *options);
 
-/* Setup DINFO with its output function and output stream setup so that
-   nothing is printed while disassembling.  */
-
-extern void init_disassemble_info_for_no_printing
-  (struct disassemble_info *dinfo);
-
 #endif
diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index 5394c1bbf5e..4e33faaea9a 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -141,27 +141,6 @@ s12z_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int num)
 
 /* Support functions for frame handling.  */
 
-
-/* Return a disassemble_info initialized for s12z disassembly, however,
-   the disassembler will not actually print anything.  */
-
-static struct disassemble_info
-s12z_disassemble_info (struct gdbarch *gdbarch)
-{
-  struct disassemble_info di;
-  init_disassemble_info_for_no_printing (&di);
-  di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di.endian = gdbarch_byte_order (gdbarch);
-  di.read_memory_func = [](bfd_vma memaddr, gdb_byte *myaddr,
-			   unsigned int len, struct disassemble_info *info)
-    {
-      return target_read_code (memaddr, myaddr, len);
-    };
-  return di;
-}
-
-
 /* A struct (based on mem_read_abstraction_base) to read memory
    through the disassemble_info API.  */
 struct mem_read_abstraction
@@ -332,15 +311,14 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
   int frame_size = 0;
   int saved_frame_size = 0;
 
-  struct disassemble_info di = s12z_disassemble_info (gdbarch);
-
+  struct gdb_non_printing_memory_disassembler dis (gdbarch);
 
   struct mem_read_abstraction mra;
   mra.base.read = (int (*)(mem_read_abstraction_base*,
 			   int, size_t, bfd_byte*)) abstract_read_memory;
   mra.base.advance = advance ;
   mra.base.posn = posn;
-  mra.info = &di;
+  mra.info = dis.disasm_info ();
 
   while (this_pc > addr)
     {
-- 
2.25.4