[binutils-gdb] gdb: add new base class to gdb_disassembler

[Andrew Burgess <aburgess@sourceware.org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f0c2e3e020d350b410e1bbe4ed636f2ea228d555

commit f0c2e3e020d350b410e1bbe4ed636f2ea228d555
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Oct 27 10:07:56 2021 +0100

    gdb: add new base class to gdb_disassembler
    
    The motivation for this change is an upcoming Python disassembler API
    that I would like to add.  As part of that change I need to create a
    new disassembler like class that contains a disassemble_info and a
    gdbarch.  The management of these two objects is identical to how we
    manage these objects within gdb_disassembler, so it might be tempting
    for my new class to inherit from gdb_disassembler.
    
    The problem however, is that gdb_disassembler has a tight connection
    between its constructor, and its print_insn method.  In the
    constructor the ui_file* that is passed in is replaced with a member
    variable string_file*, and then in print_insn, the contents of the
    member variable string_file are printed to the original ui_file*.
    
    What this means is that the gdb_disassembler class has a tight
    coupling between its constructor and print_insn; the class just isn't
    intended to be used in a situation where print_insn is not going to be
    called, which is how my (upcoming) sub-class would need to operate.
    
    My solution then, is to separate out the management of the
    disassemble_info and gdbarch into a new gdb_disassemble_info class,
    and make this class a parent of gdb_disassembler.
    
    In arm-tdep.c and mips-tdep.c, where we used to cast the
    disassemble_info->application_data to a gdb_disassembler, we can now
    cast to a gdb_disassemble_info as we only need to access the gdbarch
    information.
    
    Now, my new Python disassembler sub-class will still want to print
    things to an output stream, and so we will want access to the
    dis_asm_fprintf functionality for printing.
    
    However, rather than move this printing code into the
    gdb_disassemble_info base class, I have added yet another level of
    hierarchy, a gdb_printing_disassembler, thus the class structure is
    now:
    
      struct gdb_disassemble_info {};
      struct gdb_printing_disassembler : public gdb_disassemble_info {};
      struct gdb_disassembler : public gdb_printing_disassembler {};
    
    In a later commit my new Python disassembler will inherit from
    gdb_printing_disassembler.
    
    The reason for adding the additional layer to the class hierarchy is
    that in yet another commit I intend to rewrite the function
    gdb_buffered_insn_length, and to do this I will be creating yet more
    disassembler like classes, however, these will not print anything,
    thus I will add a gdb_non_printing_disassembler class that also
    inherits from gdb_disassemble_info.  Knowing that that change is
    coming, I've gone with the above class hierarchy now.
    
    There should be no user visible changes after this commit.

Diff:
---
 gdb/arm-tdep.c  |   4 +-
 gdb/disasm.c    |  58 +++++++++++++++--------
 gdb/disasm.h    | 140 ++++++++++++++++++++++++++++++++++++++++++++------------
 gdb/mips-tdep.c |   4 +-
 4 files changed, 154 insertions(+), 52 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 456649afdaa..fe62617d4bf 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8290,8 +8290,8 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch,
 static int
 gdb_print_insn_arm (bfd_vma memaddr, disassemble_info *info)
 {
-  gdb_disassembler *di
-    = static_cast<gdb_disassembler *>(info->application_data);
+  gdb_disassemble_info *di
+    = static_cast<gdb_disassemble_info *> (info->application_data);
   struct gdbarch *gdbarch = di->arch ();
 
   if (arm_pc_is_thumb (gdbarch, memaddr))
diff --git a/gdb/disasm.c b/gdb/disasm.c
index f2df5ef7bc5..6ac84388cc3 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -166,7 +166,8 @@ gdb_disassembler::dis_asm_print_address (bfd_vma addr,
 /* Format disassembler output to STREAM.  */
 
 int
-gdb_disassembler::dis_asm_fprintf (void *stream, const char *format, ...)
+gdb_printing_disassembler::fprintf_func (void *stream,
+					 const char *format, ...)
 {
   va_list args;
 
@@ -180,9 +181,9 @@ gdb_disassembler::dis_asm_fprintf (void *stream, const char *format, ...)
 /* See disasm.h.  */
 
 int
-gdb_disassembler::dis_asm_styled_fprintf (void *stream,
-					  enum disassembler_style style,
-					  const char *format, ...)
+gdb_printing_disassembler::fprintf_styled_func (void *stream,
+						enum disassembler_style style,
+						const char *format, ...)
 {
   va_list args;
 
@@ -797,26 +798,41 @@ get_all_disassembler_options (struct gdbarch *gdbarch)
 
 gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
 				    struct ui_file *file,
-				    di_read_memory_ftype read_memory_func)
-  : m_gdbarch (gdbarch),
+				    read_memory_ftype func)
+  : gdb_printing_disassembler (gdbarch, &m_buffer, func,
+			       dis_asm_memory_error, dis_asm_print_address),
     m_buffer (!use_ext_lang_colorization_p && disassembler_styling
 	      && file->can_emit_style_escape ()),
     m_dest (file)
+{ /* Nothing.  */ }
+
+/* See disasm.h.  */
+
+gdb_disassemble_info::gdb_disassemble_info
+  (struct gdbarch *gdbarch, struct ui_file *stream,
+   read_memory_ftype read_memory_func, memory_error_ftype memory_error_func,
+   print_address_ftype print_address_func, fprintf_ftype fprintf_func,
+   fprintf_styled_ftype fprintf_styled_func)
+    : m_gdbarch (gdbarch)
 {
-  init_disassemble_info (&m_di, &m_buffer, dis_asm_fprintf,
-			 dis_asm_styled_fprintf);
+  gdb_assert (fprintf_func != nullptr);
+  gdb_assert (fprintf_styled_func != nullptr);
+  init_disassemble_info (&m_di, stream, fprintf_func,
+			 fprintf_styled_func);
   m_di.flavour = bfd_target_unknown_flavour;
-  m_di.memory_error_func = dis_asm_memory_error;
-  m_di.print_address_func = dis_asm_print_address;
-  /* NOTE: cagney/2003-04-28: The original code, from the old Insight
-     disassembler had a local optimization here.  By default it would
-     access the executable file, instead of the target memory (there
-     was a growing list of exceptions though).  Unfortunately, the
-     heuristic was flawed.  Commands like "disassemble &variable"
-     didn't work as they relied on the access going to the target.
-     Further, it has been superseeded by trust-read-only-sections
-     (although that should be superseeded by target_trust..._p()).  */
-  m_di.read_memory_func = read_memory_func;
+
+  /* The memory_error_func, print_address_func, and read_memory_func are
+     all initialized to a default (non-nullptr) value by the call to
+     init_disassemble_info above.  If the user is overriding these fields
+     (by passing non-nullptr values) then do that now, otherwise, leave
+     these fields as the defaults.  */
+  if (memory_error_func != nullptr)
+    m_di.memory_error_func = memory_error_func;
+  if (print_address_func != nullptr)
+    m_di.print_address_func = print_address_func;
+  if (read_memory_func != nullptr)
+    m_di.read_memory_func = read_memory_func;
+
   m_di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
   m_di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
   m_di.endian = gdbarch_byte_order (gdbarch);
@@ -828,7 +844,9 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
   disassemble_init_for_target (&m_di);
 }
 
-gdb_disassembler::~gdb_disassembler ()
+/* See disasm.h.  */
+
+gdb_disassemble_info::~gdb_disassemble_info ()
 {
   disassemble_free_target (&m_di);
 }
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 7efab7db46c..f31ca92b038 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -26,43 +26,137 @@ struct gdbarch;
 struct ui_out;
 struct ui_file;
 
-class gdb_disassembler
-{
-  using di_read_memory_ftype = decltype (disassemble_info::read_memory_func);
-
-public:
-  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file)
-    : gdb_disassembler (gdbarch, file, dis_asm_read_memory)
-  {}
+/* A wrapper around a disassemble_info and a gdbarch.  This is the core
+   set of data that all disassembler sub-classes will need.  This class
+   doesn't actually implement the disassembling process, that is something
+   that sub-classes will do, with each sub-class doing things slightly
+   differently.
 
-  ~gdb_disassembler ();
+   The constructor of this class is protected, you should not create
+   instances of this class directly, instead create an instance of an
+   appropriate sub-class.  */
 
-  DISABLE_COPY_AND_ASSIGN (gdb_disassembler);
-
-  int print_insn (CORE_ADDR memaddr, int *branch_delay_insns = NULL);
+struct gdb_disassemble_info
+{
+  DISABLE_COPY_AND_ASSIGN (gdb_disassemble_info);
 
-  /* Return the gdbarch of gdb_disassembler.  */
+  /* Return the gdbarch we are disassembling for.  */
   struct gdbarch *arch ()
   { return m_gdbarch; }
 
+  /* Return a pointer to the disassemble_info, this will be needed for
+     passing into the libopcodes disassembler.  */
+  struct disassemble_info *disasm_info ()
+  { return &m_di; }
+
 protected:
-  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
-		    di_read_memory_ftype func);
 
+  /* Types for the function callbacks within m_di.  */
+  using read_memory_ftype = decltype (disassemble_info::read_memory_func);
+  using memory_error_ftype = decltype (disassemble_info::memory_error_func);
+  using print_address_ftype = decltype (disassemble_info::print_address_func);
+  using fprintf_ftype = decltype (disassemble_info::fprintf_func);
+  using fprintf_styled_ftype = decltype (disassemble_info::fprintf_styled_func);
+
+  /* Constructor, many fields in m_di are initialized from GDBARCH.  STREAM
+     is where the output of the disassembler will be written too, the
+     remaining arguments are function callbacks that are written into
+     m_di.  Of these function callbacks FPRINTF_FUNC and
+     FPRINTF_STYLED_FUNC must not be nullptr.  If READ_MEMORY_FUNC,
+     MEMORY_ERROR_FUNC, or PRINT_ADDRESS_FUNC are nullptr, then that field
+     within m_di is left with its default value (see the libopcodes
+     function init_disassemble_info for the defaults).  */
+  gdb_disassemble_info (struct gdbarch *gdbarch,
+			struct ui_file *stream,
+			read_memory_ftype read_memory_func,
+			memory_error_ftype memory_error_func,
+			print_address_ftype print_address_func,
+			fprintf_ftype fprintf_func,
+			fprintf_styled_ftype fprintf_styled_func);
+
+  /* Destructor.  */
+  virtual ~gdb_disassemble_info ();
+
+  /* The stream that disassembler output is being written too.  */
   struct ui_file *stream ()
   { return (struct ui_file *) m_di.stream; }
 
-private:
-  struct gdbarch *m_gdbarch;
-
   /* Stores data required for disassembling instructions in
      opcodes.  */
   struct disassemble_info m_di;
 
+private:
+  /* The architecture we are disassembling for.  */
+  struct gdbarch *m_gdbarch;
+
   /* If we own the string in `m_di.disassembler_options', we do so
      using this field.  */
   std::string m_disassembler_options_holder;
+};
+
+/* A wrapper around gdb_disassemble_info.  This class adds default
+   print functions that are supplied to the disassemble_info within the
+   parent class.  These default print functions write to the stream, which
+   is also contained in the parent class.
+
+   As with the parent class, the constructor for this class is protected,
+   you should not create instances of this class, but create an
+   appropriate sub-class instead.  */
 
+struct gdb_printing_disassembler : public gdb_disassemble_info
+{
+  DISABLE_COPY_AND_ASSIGN (gdb_printing_disassembler);
+
+protected:
+
+  /* Constructor.  All the arguments are just passed to the parent class.
+     We also add the two print functions to the arguments passed to the
+     parent.  See gdb_disassemble_info for a description of how the
+     arguments are handled.  */
+  gdb_printing_disassembler (struct gdbarch *gdbarch,
+			     struct ui_file *stream,
+			     read_memory_ftype read_memory_func,
+			     memory_error_ftype memory_error_func,
+			     print_address_ftype print_address_func)
+    : gdb_disassemble_info (gdbarch, stream, read_memory_func,
+			    memory_error_func, print_address_func,
+			    fprintf_func, fprintf_styled_func)
+  { /* Nothing.  */ }
+
+  /* Callback used as the disassemble_info's fprintf_func callback, this
+     writes to STREAM, which will be m_di.stream.  */
+  static int fprintf_func (void *stream, const char *format, ...)
+    ATTRIBUTE_PRINTF(2,3);
+
+  /* Callback used as the disassemble_info's fprintf_styled_func callback,
+     this writes to STREAM, which will be m_di.stream.  */
+  static int fprintf_styled_func (void *stream,
+				  enum disassembler_style style,
+				  const char *format, ...)
+    ATTRIBUTE_PRINTF(3,4);
+};
+
+/* A dissassembler class that provides 'print_insn', a method for
+   disassembling a single instruction to the output stream.  */
+
+struct gdb_disassembler : public gdb_printing_disassembler
+{
+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file)
+    : gdb_disassembler (gdbarch, file, dis_asm_read_memory)
+  { /* Nothing.  */ }
+
+  DISABLE_COPY_AND_ASSIGN (gdb_disassembler);
+
+  /* Disassemble a single instruction at MEMADDR to the ui_file* that was
+     passed to the constructor.  If a memory error occurs while
+     disassembling this instruction then an error will be thrown.  */
+  int print_insn (CORE_ADDR memaddr, int *branch_delay_insns = NULL);
+
+protected:
+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
+		    read_memory_ftype func);
+
+private:
   /* This member variable is given a value by calling dis_asm_memory_error.
      If after calling into the libopcodes disassembler we get back a
      negative value (which indicates an error), then, if this variable has
@@ -95,16 +189,6 @@ private:
      (currently just to addresses and symbols) as it goes.  */
   static bool use_ext_lang_colorization_p;
 
-  static int dis_asm_fprintf (void *stream, const char *format, ...)
-    ATTRIBUTE_PRINTF(2,3);
-
-  /* Print formatted message to STREAM, the content can be styled based on
-     STYLE if desired.  */
-  static int dis_asm_styled_fprintf (void *stream,
-				     enum disassembler_style style,
-				     const char *format, ...)
-    ATTRIBUTE_PRINTF(3,4);
-
   static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
 				  unsigned int len,
 				  struct disassemble_info *info);
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 805c5beba59..65aa86dd98d 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -7021,8 +7021,8 @@ reinit_frame_cache_sfunc (const char *args, int from_tty,
 static int
 gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info)
 {
-  gdb_disassembler *di
-    = static_cast<gdb_disassembler *>(info->application_data);
+  gdb_disassemble_info *di
+    = static_cast<gdb_disassemble_info *> (info->application_data);
   struct gdbarch *gdbarch = di->arch ();
 
   /* FIXME: cagney/2003-06-26: Is this even necessary?  The