[2/6] Refactor disassembly code

  This patch addes class gdb_disassembler, and refactor
code to use it.  The gdb_disassembler object is saved
in disassember_info.application_data.  However,
disassember_info.application_data is already used by
gdb for arm, mips spu, and scm-disasm.  In arm and mips,
.application_data is gdbarch, but we can still get gdbarch
from gdb_disassember.

The use of application_data in spu is a little bit
complicated.  It creates its own disassemble_info, and
save spu_dis_asm_data in .application_data.  This will
overwrite the pointer to gdb_disassembler, so we need
to find another place to save spu_dis_asm_data.  I
extend disassemble_info, and put "id" there.

v2:
 - Merge to print_insn functions into one, with default
   parameter value,

gdb:

2017-01-13  Pedro Alves  <palves@redhat.com>
	    Yao Qi  <yao.qi@linaro.org>

	* arm-tdep.c: Include "disasm.h".
	(gdb_print_insn_arm): Update code to get gdbarch.
	* disasm.c (dis_asm_read_memory): Change it to
	gdb_disassembler::dis_asm_read_memory.
	(dis_asm_memory_error): Likewise.
	(dis_asm_print_address): Likewise.
	(gdb_pretty_print_insn): Change it to
	gdb_disassembler::pretty_print_insn.
	(dump_insns): Add one argument gdb_disassemlber.  All
	callers updated.
	(do_mixed_source_and_assembly_deprecated): Likewise.
	(do_mixed_source_and_assembly): Likewise.
	(do_assembly_only): Likewise.
	(gdb_disassembler::gdb_disassembler): New.
	(gdb_disassembler::print_insn): New.
	* disasm.h (class gdb_disassembler): New.
	(gdb_pretty_print_insn): Remove declaration.
	(gdb_disassemble_info): Likewise.
	* guile/scm-disasm.c (class gdbscm_disassembler): New.
	(gdbscm_disasm_read_memory_worker): Update.
	(gdbscm_disasm_read_memory): Update.
	(gdbscm_disasm_memory_error): Remove.
	(gdbscm_disasm_print_address): Remove.
	(gdbscm_disassembler::gdbscm_disassembler): New.
	(gdbscm_print_insn_from_port): Update.
	* mips-tdep.c: Include disasm.h.
	(gdb_print_insn_mips): Update code to get gdbarch.
	* record-btrace.c (btrace_insn_history): Update.
	* spu-tdep.c: Include disasm.h.
	(struct spu_dis_asm_data): Remove.
	(struct spu_dis_asm_info): New.
	(spu_dis_asm_print_address): Use spu_dis_asm_info to get
	SPU id.
	(gdb_print_insn_spu): Cast disassemble_info to
	spu_dis_asm_info.
---
 gdb/arm-tdep.c         |   5 +-
 gdb/disasm.c           | 151 +++++++++++++++++++++++++++----------------------
 gdb/disasm.h           |  53 ++++++++++++-----
 gdb/guile/scm-disasm.c |  77 +++++++------------------
 gdb/mips-tdep.c        |   5 +-
 gdb/record-btrace.c    |   5 +-
 gdb/spu-tdep.c         |  20 +++----
 7 files changed, 162 insertions(+), 154 deletions(-)
  

On 01/16/2017 04:02 AM, Yao Qi wrote:
> This patch addes class gdb_disassembler, and refactor
> code to use it.  The gdb_disassembler object is saved
> in disassember_info.application_data.  However,
> disassember_info.application_data is already used by
> gdb for arm, mips spu, and scm-disasm.  In arm and mips,
> .application_data is gdbarch, but we can still get gdbarch
> from gdb_disassember.
>
> The use of application_data in spu is a little bit
> complicated.  It creates its own disassemble_info, and
> save spu_dis_asm_data in .application_data.  This will
> overwrite the pointer to gdb_disassembler, so we need
> to find another place to save spu_dis_asm_data.  I
> extend disassemble_info, and put "id" there.
>
> v2:
>  - Merge to print_insn functions into one, with default
>    parameter value,
>
> gdb:
>
> 2017-01-13  Pedro Alves  <palves@redhat.com>
> 	    Yao Qi  <yao.qi@linaro.org>
>
> 	* arm-tdep.c: Include "disasm.h".
> 	(gdb_print_insn_arm): Update code to get gdbarch.
> 	* disasm.c (dis_asm_read_memory): Change it to
> 	gdb_disassembler::dis_asm_read_memory.
> 	(dis_asm_memory_error): Likewise.
> 	(dis_asm_print_address): Likewise.
> 	(gdb_pretty_print_insn): Change it to
> 	gdb_disassembler::pretty_print_insn.
> 	(dump_insns): Add one argument gdb_disassemlber.  All
> 	callers updated.
> 	(do_mixed_source_and_assembly_deprecated): Likewise.
> 	(do_mixed_source_and_assembly): Likewise.
> 	(do_assembly_only): Likewise.
> 	(gdb_disassembler::gdb_disassembler): New.
> 	(gdb_disassembler::print_insn): New.
> 	* disasm.h (class gdb_disassembler): New.
> 	(gdb_pretty_print_insn): Remove declaration.
> 	(gdb_disassemble_info): Likewise.
> 	* guile/scm-disasm.c (class gdbscm_disassembler): New.
> 	(gdbscm_disasm_read_memory_worker): Update.
> 	(gdbscm_disasm_read_memory): Update.
> 	(gdbscm_disasm_memory_error): Remove.
> 	(gdbscm_disasm_print_address): Remove.
> 	(gdbscm_disassembler::gdbscm_disassembler): New.
> 	(gdbscm_print_insn_from_port): Update.
> 	* mips-tdep.c: Include disasm.h.
> 	(gdb_print_insn_mips): Update code to get gdbarch.
> 	* record-btrace.c (btrace_insn_history): Update.
> 	* spu-tdep.c: Include disasm.h.
> 	(struct spu_dis_asm_data): Remove.
> 	(struct spu_dis_asm_info): New.
> 	(spu_dis_asm_print_address): Use spu_dis_asm_info to get
> 	SPU id.
> 	(gdb_print_insn_spu): Cast disassemble_info to
> 	spu_dis_asm_info.
> ---
>  gdb/arm-tdep.c         |   5 +-
>  gdb/disasm.c           | 151 +++++++++++++++++++++++++++----------------------
>  gdb/disasm.h           |  53 ++++++++++++-----
>  gdb/guile/scm-disasm.c |  77 +++++++------------------
>  gdb/mips-tdep.c        |   5 +-
>  gdb/record-btrace.c    |   5 +-
>  gdb/spu-tdep.c         |  20 +++----
>  7 files changed, 162 insertions(+), 154 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 2bdfa57..0ae311f 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -27,6 +27,7 @@
>  #include "gdbcmd.h"
>  #include "gdbcore.h"
>  #include "dis-asm.h"		/* For register styles.  */
> +#include "disasm.h"

I just noticed this. Unfortunate naming of these two files.

>  #include "regcache.h"
>  #include "reggroups.h"
>  #include "doublest.h"
> @@ -7739,7 +7740,9 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch,
>  static int
>  gdb_print_insn_arm (bfd_vma memaddr, disassemble_info *info)
>  {
> -  struct gdbarch *gdbarch = (struct gdbarch *) info->application_data;
> +  gdb_disassembler *di
> +    = static_cast<gdb_disassembler *>(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 ae3a2f1..f31d8d3 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -120,28 +120,34 @@ line_has_code_p (htab_t table, struct symtab *symtab, int line)
>  }
>
>  /* Like target_read_memory, but slightly different parameters.  */

Should we update these comments to a more useful version? "slightly 
different parameters" doesn't explain much, as it is obvious they are 
slightly different. Other uses below have the same problem.

> -static int
> -dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
> -		     struct disassemble_info *info)
> +
> +int
> +gdb_disassembler::dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
> +				       unsigned int len,
> +				       struct disassemble_info *info)
>  {
>    return target_read_code (memaddr, myaddr, len);
>  }
>
>  /* Like memory_error with slightly different parameters.  */
> -static void
> -dis_asm_memory_error (int err, bfd_vma memaddr,
> -		      struct disassemble_info *info)
> +
> +void
> +gdb_disassembler::dis_asm_memory_error (int err, bfd_vma memaddr,
> +					struct disassemble_info *info)
>  {
>    memory_error (TARGET_XFER_E_IO, memaddr);
>  }
>
>  /* Like print_address with slightly different parameters.  */
> -static void
> -dis_asm_print_address (bfd_vma addr, struct disassemble_info *info)
> +
> +void
> +gdb_disassembler::dis_asm_print_address (bfd_vma addr,
> +					 struct disassemble_info *info)
>  {
> -  struct gdbarch *gdbarch = (struct gdbarch *) info->application_data;
> +  gdb_disassembler *self
> +    = static_cast<gdb_disassembler *>(info->application_data);
>
> -  print_address (gdbarch, addr, (struct ui_file *) info->stream);
> +  print_address (self->arch (), addr, self->stream ());
>  }
>
>  static int
> @@ -173,10 +179,9 @@ compare_lines (const void *mle1p, const void *mle2p)
>  /* See disasm.h.  */
>
>  int
> -gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
> -		       struct disassemble_info * di,
> -		       const struct disasm_insn *insn, int flags,
> -		       struct ui_file *stb)
> +gdb_disassembler::pretty_print_insn (struct ui_out *uiout,
> +				     const struct disasm_insn *insn,
> +				     int flags)
>  {
>    /* parts of the symbolic representation of the address */
>    int unmapped;
> @@ -187,6 +192,8 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
>    char *filename = NULL;
>    char *name = NULL;
>    CORE_ADDR pc;
> +  struct ui_file *stb = stream ();
> +  struct gdbarch *gdbarch = arch ();
>
>    ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
>    pc = insn->addr;
> @@ -254,14 +261,14 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
>        struct cleanup *cleanups =
>  	make_cleanup_ui_file_delete (opcode_stream);
>
> -      size = gdbarch_print_insn (gdbarch, pc, di);
> +      size = print_insn (pc);
>        end_pc = pc + size;
>
>        for (;pc < end_pc; ++pc)
>  	{
> -	  err = (*di->read_memory_func) (pc, &data, 1, di);
> +	  err = m_di.read_memory_func (pc, &data, 1, &m_di);
>  	  if (err != 0)
> -	    (*di->memory_error_func) (err, pc, di);
> +	    m_di.memory_error_func (err, pc, &m_di);
>  	  fprintf_filtered (opcode_stream, "%s%02x",
>  			    spacer, (unsigned) data);
>  	  spacer = " ";
> @@ -273,7 +280,7 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
>        do_cleanups (cleanups);
>      }
>    else
> -    size = gdbarch_print_insn (gdbarch, pc, di);
> +    size = print_insn (pc);
>
>    uiout->field_stream ("inst", stb);
>    ui_file_rewind (stb);
> @@ -284,10 +291,9 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
>  }
>
>  static int
> -dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> -	    struct disassemble_info * di,
> +dump_insns (struct ui_out *uiout, gdb_disassembler *di,
>  	    CORE_ADDR low, CORE_ADDR high,
> -	    int how_many, int flags, struct ui_file *stb,
> +	    int how_many, int flags,
>  	    CORE_ADDR *end_pc)
>  {
>    struct disasm_insn insn;
> @@ -300,7 +306,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>      {
>        int size;
>
> -      size = gdb_pretty_print_insn (gdbarch, uiout, di, &insn, flags, stb);
> +      size = di->pretty_print_insn (uiout, &insn, flags);
>        if (size <= 0)
>  	break;
>
> @@ -326,10 +332,10 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>
>  static void
>  do_mixed_source_and_assembly_deprecated
> -  (struct gdbarch *gdbarch, struct ui_out *uiout,
> -   struct disassemble_info *di, struct symtab *symtab,
> +  (struct ui_out *uiout,
> +   gdb_disassembler *di, struct symtab *symtab,
>     CORE_ADDR low, CORE_ADDR high,
> -   int how_many, int flags, struct ui_file *stb)
> +   int how_many, int flags)
>  {
>    int newlines = 0;
>    int nlines;
> @@ -462,9 +468,9 @@ do_mixed_source_and_assembly_deprecated
>  	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
>  	}
>
> -      num_displayed += dump_insns (gdbarch, uiout, di,
> +      num_displayed += dump_insns (uiout, di,
>  				   mle[i].start_pc, mle[i].end_pc,
> -				   how_many, flags, stb, NULL);
> +				   how_many, flags, NULL);
>
>        /* When we've reached the end of the mle array, or we've seen the last
>           assembly range for this source line, close out the list/tuple.  */
> @@ -488,11 +494,12 @@ do_mixed_source_and_assembly_deprecated
>     immediately following.  */
>
>  static void
> -do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> -			      struct disassemble_info *di,
> +do_mixed_source_and_assembly (struct gdbarch *gdbarch,
> +			      struct ui_out *uiout,
> +			      gdb_disassembler *di,
>  			      struct symtab *main_symtab,
>  			      CORE_ADDR low, CORE_ADDR high,
> -			      int how_many, int flags, struct ui_file *stb)
> +			      int how_many, int flags)
>  {
>    const struct linetable_entry *le, *first_le;
>    int i, nlines;
> @@ -711,8 +718,8 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>  	end_pc = std::min (sal.end, high);
>        else
>  	end_pc = pc + 1;
> -      num_displayed += dump_insns (gdbarch, uiout, di, pc, end_pc,
> -				   how_many, flags, stb, &end_pc);
> +      num_displayed += dump_insns (uiout, di, pc, end_pc,
> +				   how_many, flags, &end_pc);
>        pc = end_pc;
>
>        if (how_many >= 0 && num_displayed >= how_many)
> @@ -726,16 +733,16 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>  }
>
>  static void
> -do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
> -		  struct disassemble_info * di,
> +do_assembly_only (struct ui_out *uiout,
> +		  gdb_disassembler *di,
>  		  CORE_ADDR low, CORE_ADDR high,
> -		  int how_many, int flags, struct ui_file *stb)
> +		  int how_many, int flags)
>  {
>    struct cleanup *ui_out_chain;
>
>    ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
>
> -  dump_insns (gdbarch, uiout, di, low, high, how_many, flags, stb, NULL);
> +  dump_insns (uiout, di, low, high, how_many, flags, NULL);
>
>    do_cleanups (ui_out_chain);
>  }
> @@ -755,15 +762,15 @@ fprintf_disasm (void *stream, const char *format, ...)
>    return 0;
>  }
>
> -struct disassemble_info
> -gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
> +gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
> +				    struct ui_file *file,
> +				    di_read_memory_ftype read_memory_func)
> +  : m_gdbarch (gdbarch)
>  {
> -  struct disassemble_info di;
> -
> -  init_disassemble_info (&di, file, fprintf_disasm);
> -  di.flavour = bfd_target_unknown_flavour;
> -  di.memory_error_func = dis_asm_memory_error;
> -  di.print_address_func = dis_asm_print_address;
> +  init_disassemble_info (&m_di, file, fprintf_disasm);
> +  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 optomization here.  By default it would
>       access the executable file, instead of the target memory (there
> @@ -772,14 +779,29 @@ gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
>       didn't work as they relied on the access going to the target.
>       Further, it has been supperseeded by trust-read-only-sections
>       (although that should be superseeded by target_trust..._p()).  */
> -  di.read_memory_func = dis_asm_read_memory;
> -  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);
> -  di.application_data = gdbarch;
> -  disassemble_init_for_target (&di);
> -  return di;
> +  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);
> +  m_di.endian_code = gdbarch_byte_order_for_code (gdbarch);
> +  m_di.application_data = this;
> +  disassemble_init_for_target (&m_di);
> +}
> +
> +int
> +gdb_disassembler::print_insn (CORE_ADDR memaddr,
> +			      int *branch_delay_insns)
> +{
> +  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
> +
> +  if (branch_delay_insns != NULL)
> +    {
> +      if (m_di.insn_info_valid)
> +	*branch_delay_insns = m_di.branch_delay_insns;
> +      else
> +	*branch_delay_insns = 0;
> +    }
> +  return length;

length doesn't seem to have a purpose other than being returned. So just 
return gdbarch_print_insn (arch (), memaddr, &m_di)?

>  }
>
>  void
> @@ -789,7 +811,7 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>  {
>    struct ui_file *stb = mem_fileopen ();
>    struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
> -  struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
> +  gdb_disassembler di (gdbarch, stb);
>    struct symtab *symtab;
>    int nlines = -1;
>
> @@ -801,15 +823,15 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>
>    if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
>        || nlines <= 0)
> -    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
> +    do_assembly_only (uiout, &di, low, high, how_many, flags);
>
>    else if (flags & DISASSEMBLY_SOURCE)
>      do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
> -				  how_many, flags, stb);
> +				  how_many, flags);
>
>    else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
> -    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
> -					     low, high, how_many, flags, stb);
> +    do_mixed_source_and_assembly_deprecated (uiout, &di, symtab,
> +					     low, high, how_many, flags);
>
>    do_cleanups (cleanups);
>    gdb_flush (gdb_stdout);
> @@ -823,19 +845,10 @@ int
>  gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
>  		struct ui_file *stream, int *branch_delay_insns)
>  {
> -  struct disassemble_info di;
> -  int length;
>
> -  di = gdb_disassemble_info (gdbarch, stream);
> -  length = gdbarch_print_insn (gdbarch, memaddr, &di);
> -  if (branch_delay_insns)
> -    {
> -      if (di.insn_info_valid)
> -	*branch_delay_insns = di.branch_delay_insns;
> -      else
> -	*branch_delay_insns = 0;
> -    }
> -  return length;
> +  gdb_disassembler di (gdbarch, stream);
> +
> +  return di.print_insn (memaddr, branch_delay_insns);
>  }
>
>  /* Return the length in bytes of the instruction at address MEMADDR in
> diff --git a/gdb/disasm.h b/gdb/disasm.h
> index 4c6fd54..5122fa3 100644
> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -33,6 +33,46 @@ 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)
> +  {}
> +
> +  int print_insn (CORE_ADDR memaddr, int *branch_delay_insns = NULL);
> +
> +  /* Prints the instruction INSN into UIOUT and returns the length of
> +     the printed instruction in bytes.  */
> +  int pretty_print_insn (struct ui_out *uiout,
> +			 const struct disasm_insn *insn, int flags);

Can this function return negative? If not, use unsigned?

> +
> +  /* Return the gdbarch of gdb_disassembler.  */
> +  struct gdbarch *arch ()
> +  { return m_gdbarch; }
> +
> +protected:
> +  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
> +		    di_read_memory_ftype func);
> +
> +  struct ui_file *stream ()
> +  { return (struct ui_file *) m_di.stream; }
> +
> +private:
> +  struct gdbarch *m_gdbarch;
> +  struct disassemble_info m_di;

Add comments explaining what m_gdbarch and m_di are used for and/or how?

> +
> +  static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
> +				  unsigned int len,
> +				  struct disassemble_info *info);
> +  static void dis_asm_memory_error (int err, bfd_vma memaddr,
> +				    struct disassemble_info *info);
> +  static void dis_asm_print_address (bfd_vma addr,
> +				     struct disassemble_info *info);
> +};
> +
>  /* An instruction to be disassembled.  */
>
>  struct disasm_insn
> @@ -47,19 +87,6 @@ struct disasm_insn
>    unsigned int is_speculative:1;
>  };
>
> -/* Prints the instruction INSN into UIOUT and returns the length of the
> -   printed instruction in bytes.  */
> -
> -extern int gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
> -				  struct disassemble_info * di,
> -				  const struct disasm_insn *insn, int flags,
> -				  struct ui_file *stb);
> -
> -/* Return a filled in disassemble_info object for use by gdb.  */
> -
> -extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,
> -						     struct ui_file *file);
> -
>  extern void gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>  			     char *file_string, int flags, int how_many,
>  			     CORE_ADDR low, CORE_ADDR high);
> diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
> index d06c481..25cae5a 100644
> --- a/gdb/guile/scm-disasm.c
> +++ b/gdb/guile/scm-disasm.c
> @@ -37,11 +37,13 @@ static SCM address_symbol;
>  static SCM asm_symbol;
>  static SCM length_symbol;
>
> -/* Struct used to pass "application data" in disassemble_info.  */
> -
> -struct gdbscm_disasm_data
> +class gdbscm_disassembler : public gdb_disassembler
>  {
> -  struct gdbarch *gdbarch;
> +public:
> +  gdbscm_disassembler (struct gdbarch *gdbarch,
> +		       struct ui_file *stream,
> +		       SCM port, ULONGEST offset);
> +
>    SCM port;
>    /* The offset of the address of the first instruction in PORT.  */
>    ULONGEST offset;
> @@ -55,7 +57,7 @@ struct gdbscm_disasm_read_data
>    bfd_vma memaddr;
>    bfd_byte *myaddr;
>    unsigned int length;
> -  struct disassemble_info *dinfo;
> +  gdbscm_disassembler *dinfo;
>  };
>  
>  /* Subroutine of gdbscm_arch_disassemble to simplify it.
> @@ -81,13 +83,11 @@ gdbscm_disasm_read_memory_worker (void *datap)
>  {
>    struct gdbscm_disasm_read_data *data
>      = (struct gdbscm_disasm_read_data *) datap;
> -  struct disassemble_info *dinfo = data->dinfo;
> -  struct gdbscm_disasm_data *disasm_data
> -    = (struct gdbscm_disasm_data *) dinfo->application_data;
> -  SCM seekto, newpos, port = disasm_data->port;
> +  gdbscm_disassembler *dinfo = data->dinfo;
> +  SCM seekto, newpos, port = dinfo->port;
>    size_t bytes_read;
>
> -  seekto = gdbscm_scm_from_ulongest (data->memaddr - disasm_data->offset);
> +  seekto = gdbscm_scm_from_ulongest (data->memaddr - dinfo->offset);
>    newpos = scm_seek (port, seekto, scm_from_int (SEEK_SET));
>    if (!scm_is_eq (seekto, newpos))
>      return "seek error";
> @@ -108,13 +108,15 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
>  			   unsigned int length,
>  			   struct disassemble_info *dinfo)
>  {
> +  gdbscm_disassembler *self
> +    = static_cast<gdbscm_disassembler *> (dinfo->application_data);
>    struct gdbscm_disasm_read_data data;
>    const char *status;
>
>    data.memaddr = memaddr;
>    data.myaddr = myaddr;
>    data.length = length;
> -  data.dinfo = dinfo;
> +  data.dinfo = self;
>
>    status = gdbscm_with_guile (gdbscm_disasm_read_memory_worker, &data);
>
> @@ -123,30 +125,12 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
>    return status != NULL ? -1 : 0;
>  }
>
> -/* disassemble_info.memory_error_func for gdbscm_print_insn_from_port.
> -   Technically speaking, we don't need our own memory_error_func,
> -   but to not provide one would leave a subtle dependency in the code.
> -   This function exists to keep a clear boundary.  */
> -
> -static void
> -gdbscm_disasm_memory_error (int status, bfd_vma memaddr,
> -			    struct disassemble_info *info)
> -{
> -  memory_error (TARGET_XFER_E_IO, memaddr);
> -}
> -
> -/* disassemble_info.print_address_func for gdbscm_print_insn_from_port.
> -   Since we need to use our own application_data value, we need to supply
> -   this routine as well.  */
> -
> -static void
> -gdbscm_disasm_print_address (bfd_vma addr, struct disassemble_info *info)
> +gdbscm_disassembler::gdbscm_disassembler (struct gdbarch *gdbarch,
> +					  struct ui_file *stream,
> +					  SCM port_, ULONGEST offset_)
> +  : gdb_disassembler (gdbarch, stream, gdbscm_disasm_read_memory),
> +    port (port_), offset (offset_)
>  {
> -  struct gdbscm_disasm_data *data
> -    = (struct gdbscm_disasm_data *) info->application_data;
> -  struct gdbarch *gdbarch = data->gdbarch;
> -
> -  print_address (gdbarch, addr, (struct ui_file *) info->stream);
>  }
>
>  /* Subroutine of gdbscm_arch_disassemble to simplify it.
> @@ -164,30 +148,9 @@ gdbscm_print_insn_from_port (struct gdbarch *gdbarch,
>  			     SCM port, ULONGEST offset, CORE_ADDR memaddr,
>  			     struct ui_file *stream, int *branch_delay_insns)
>  {
> -  struct disassemble_info di;
> -  int length;
> -  struct gdbscm_disasm_data data;
> -
> -  di = gdb_disassemble_info (gdbarch, stream);
> -  data.gdbarch = gdbarch;
> -  data.port = port;
> -  data.offset = offset;
> -  di.application_data = &data;
> -  di.read_memory_func = gdbscm_disasm_read_memory;
> -  di.memory_error_func = gdbscm_disasm_memory_error;
> -  di.print_address_func = gdbscm_disasm_print_address;
> -
> -  length = gdbarch_print_insn (gdbarch, memaddr, &di);
> -
> -  if (branch_delay_insns)
> -    {
> -      if (di.insn_info_valid)
> -	*branch_delay_insns = di.branch_delay_insns;
> -      else
> -	*branch_delay_insns = 0;
> -    }
> +  gdbscm_disassembler di (gdbarch, stream, port, offset);
>
> -  return length;
> +  return di.print_insn (memaddr, branch_delay_insns);
>  }
>
>  /* (arch-disassemble <gdb:arch> address
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 637b34e..41cb9d8 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -44,6 +44,7 @@
>  #include "symcat.h"
>  #include "sim-regno.h"
>  #include "dis-asm.h"
> +#include "disasm.h"
>  #include "frame-unwind.h"
>  #include "frame-base.h"
>  #include "trad-frame.h"
> @@ -6982,7 +6983,9 @@ reinit_frame_cache_sfunc (char *args, int from_tty,
>  static int
>  gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info)
>  {
> -  struct gdbarch *gdbarch = (struct gdbarch *) info->application_data;
> +  gdb_disassembler *di
> +    = static_cast<gdb_disassembler *>(info->application_data);
> +  struct gdbarch *gdbarch = di->arch ();
>
>    /* FIXME: cagney/2003-06-26: Is this even necessary?  The
>       disassembler needs to be able to locally determine the ISA, and
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 6cba1d2..8896241 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -698,7 +698,6 @@ btrace_insn_history (struct ui_out *uiout,
>  {
>    struct ui_file *stb;
>    struct cleanup *cleanups, *ui_item_chain;
> -  struct disassemble_info di;
>    struct gdbarch *gdbarch;
>    struct btrace_insn_iterator it;
>    struct btrace_line_range last_lines;
> @@ -711,7 +710,7 @@ btrace_insn_history (struct ui_out *uiout,
>    gdbarch = target_gdbarch ();
>    stb = mem_fileopen ();
>    cleanups = make_cleanup_ui_file_delete (stb);
> -  di = gdb_disassemble_info (gdbarch, stb);
> +  gdb_disassembler di (gdbarch, stb);
>    last_lines = btrace_mk_line_range (NULL, 0, 0);
>
>    make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
> @@ -773,7 +772,7 @@ btrace_insn_history (struct ui_out *uiout,
>  	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
>  	    dinsn.is_speculative = 1;
>
> -	  gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb);
> +	  di.pretty_print_insn (uiout, &dinsn, flags);
>  	}
>      }
>
> diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
> index 8756256..70d7f6f 100644
> --- a/gdb/spu-tdep.c
> +++ b/gdb/spu-tdep.c
> @@ -33,6 +33,7 @@
>  #include "value.h"
>  #include "inferior.h"
>  #include "dis-asm.h"
> +#include "disasm.h"
>  #include "objfiles.h"
>  #include "language.h"
>  #include "regcache.h"
> @@ -1693,18 +1694,19 @@ spu_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>
>  /* Disassembler.  */
>
> -struct spu_dis_asm_data
> +struct spu_dis_asm_info : disassemble_info
>  {
> -  struct gdbarch *gdbarch;
>    int id;
>  };
>
>  static void
>  spu_dis_asm_print_address (bfd_vma addr, struct disassemble_info *info)
>  {
> -  struct spu_dis_asm_data *data
> -    = (struct spu_dis_asm_data *) info->application_data;
> -  print_address (data->gdbarch, SPUADDR (data->id, addr),
> +  struct spu_dis_asm_info *data = (struct spu_dis_asm_info *) info;
> +  gdb_disassembler *di
> +    = static_cast<gdb_disassembler *>(info->application_data);
> +
> +  print_address (di->arch (), SPUADDR (data->id, addr),
>  		 (struct ui_file *) info->stream);
>  }
>
> @@ -1714,12 +1716,10 @@ gdb_print_insn_spu (bfd_vma memaddr, struct disassemble_info *info)
>    /* The opcodes disassembler does 18-bit address arithmetic.  Make
>       sure the SPU ID encoded in the high bits is added back when we
>       call print_address.  */
> -  struct disassemble_info spu_info = *info;
> -  struct spu_dis_asm_data data;
> -  data.gdbarch = (struct gdbarch *) info->application_data;
> -  data.id = SPUADDR_SPU (memaddr);
> +  struct spu_dis_asm_info spu_info;
>
> -  spu_info.application_data = &data;
> +  memcpy (&spu_info, info, sizeof (*info));
> +  spu_info.id = SPUADDR_SPU (memaddr);
>    spu_info.print_address_func = spu_dis_asm_print_address;
>    return print_insn_spu (memaddr, &spu_info);
>  }
>
  

On 17-01-17 08:14:27, Luis Machado wrote:
> >
> > /* Like target_read_memory, but slightly different parameters.  */
> 
> Should we update these comments to a more useful version? "slightly
> different parameters" doesn't explain much, as it is obvious they
> are slightly different. Other uses below have the same problem.

How about this?

  /* Wrapper of target_read_code.  */

> 
> >-static int
> >-dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
> >-		     struct disassemble_info *info)
> >+
> >+int
> >+gdb_disassembler::dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
> >+				       unsigned int len,
> >+				       struct disassemble_info *info)
> > {
> >   return target_read_code (memaddr, myaddr, len);
> > }
> >
> > /* Like memory_error with slightly different parameters.  */

and how about

/* Wrapper of memory_error.  */

> >-static void
> >-dis_asm_memory_error (int err, bfd_vma memaddr,
> >-		      struct disassemble_info *info)
> >+
> >+

> >+int
> >+gdb_disassembler::print_insn (CORE_ADDR memaddr,
> >+			      int *branch_delay_insns)
> >+{
> >+  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
> >+
> >+  if (branch_delay_insns != NULL)
> >+    {
> >+      if (m_di.insn_info_valid)
> >+	*branch_delay_insns = m_di.branch_delay_insns;
> >+      else
> >+	*branch_delay_insns = 0;
> >+    }
> >+  return length;
> 
> length doesn't seem to have a purpose other than being returned. So
> just return gdbarch_print_insn (arch (), memaddr, &m_di)?
> 

branch_delay_insns needs update after gdbarch_print_insn call.

> >+
> >+  /* Prints the instruction INSN into UIOUT and returns the length of
> >+     the printed instruction in bytes.  */
> >+  int pretty_print_insn (struct ui_out *uiout,
> >+			 const struct disasm_insn *insn, int flags);
> 
> Can this function return negative? If not, use unsigned?
> 

I don't think pretty_print_insn can return negative, and we can change
it to size_t.  However, this patch is a code refactor.  I want to avoid
change like this in this patch.  I can change 'int' to 'size_t' later.

> >+
> >+  /* Return the gdbarch of gdb_disassembler.  */
> >+  struct gdbarch *arch ()
> >+  { return m_gdbarch; }
> >+
> >+protected:
> >+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
> >+		    di_read_memory_ftype func);
> >+
> >+  struct ui_file *stream ()
> >+  { return (struct ui_file *) m_di.stream; }
> >+
> >+private:
> >+  struct gdbarch *m_gdbarch;
> >+  struct disassemble_info m_di;
> 
> Add comments explaining what m_gdbarch and m_di are used for and/or how?

I don't know what kind of comments I can add for m_gdbarch.  Something
like "gdbarch of gdb_disassembler" looks useless.  We've already had
method arch(with comments) which returns m_gdbarch.

I can think of the comments to m_di

  /* Pass it to opcodes disassembler and collect some outputs
     from opcodes disassembler.  */

  struct disassemble_info m_di;

What do you think?
  

On 01/18/2017 10:33 AM, Yao Qi wrote:
> On 17-01-17 08:14:27, Luis Machado wrote:
>>>
>>> /* Like target_read_memory, but slightly different parameters.  */
>>
>> Should we update these comments to a more useful version? "slightly
>> different parameters" doesn't explain much, as it is obvious they
>> are slightly different. Other uses below have the same problem.
>
> How about this?
>
>   /* Wrapper of target_read_code.  */
>

I think that's better than the previous one.

>>> +int
>>> +gdb_disassembler::print_insn (CORE_ADDR memaddr,
>>> +			      int *branch_delay_insns)
>>> +{
>>> +  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
>>> +
>>> +  if (branch_delay_insns != NULL)
>>> +    {
>>> +      if (m_di.insn_info_valid)
>>> +	*branch_delay_insns = m_di.branch_delay_insns;
>>> +      else
>>> +	*branch_delay_insns = 0;
>>> +    }
>>> +  return length;
>>
>> length doesn't seem to have a purpose other than being returned. So
>> just return gdbarch_print_insn (arch (), memaddr, &m_di)?
>>
>
> branch_delay_insns needs update after gdbarch_print_insn call.
>

Indeed. Missed the m_di.

>>> +
>>> +  /* Prints the instruction INSN into UIOUT and returns the length of
>>> +     the printed instruction in bytes.  */
>>> +  int pretty_print_insn (struct ui_out *uiout,
>>> +			 const struct disasm_insn *insn, int flags);
>>
>> Can this function return negative? If not, use unsigned?
>>
>
> I don't think pretty_print_insn can return negative, and we can change
> it to size_t.  However, this patch is a code refactor.  I want to avoid
> change like this in this patch.  I can change 'int' to 'size_t' later.
>

Sounds good.

>>> +
>>> +  /* Return the gdbarch of gdb_disassembler.  */
>>> +  struct gdbarch *arch ()
>>> +  { return m_gdbarch; }
>>> +
>>> +protected:
>>> +  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
>>> +		    di_read_memory_ftype func);
>>> +
>>> +  struct ui_file *stream ()
>>> +  { return (struct ui_file *) m_di.stream; }
>>> +
>>> +private:
>>> +  struct gdbarch *m_gdbarch;
>>> +  struct disassemble_info m_di;
>>
>> Add comments explaining what m_gdbarch and m_di are used for and/or how?
>
> I don't know what kind of comments I can add for m_gdbarch.  Something
> like "gdbarch of gdb_disassembler" looks useless.  We've already had

I can still see value in describing it as a "Pointer to the target's 
gdbarch". But feel free not to document it if you don't think it's worth it.

The documentation is likely not for us (experienced gdb developers), but 
for people getting started on gdb's code.

> method arch(with comments) which returns m_gdbarch.
>
> I can think of the comments to m_di
>
>   /* Pass it to opcodes disassembler and collect some outputs
>      from opcodes disassembler.  */
>
>   struct disassemble_info m_di;
>
> What do you think?
>

I think something along the lines of:

/* Stores disassembler data.  */

or

/* Stores data required for disassembling instructions.  */

... is good enough, as trivial as it may seem.