Re: [PATCHv4 1/5] gdb: add new base class to gdb_disassembler

[Simon Marchi <simark@simark.ca>]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> 
> 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 {};

I can't explain it very well, but it seems a little strange to make the
other classes inherit from gdb_disassemble_info.  From the name (and the
code), I understand that gdb_disassemble_info purely holds the data
necessary for the disassemblers to do their job.  So this is not really
an IS-A relationship, but more a HAS-A.  So I would think composition
would be more natural (i.e. gdb_printing_disassembler would have a field
of type gdb_disassemble_info).

But with the callbacks we pass to struct disassemble_info (such as
fprintf_func and fprintf_styled_func), what is data and what is behavior
gets a little blurry.

It doesn't matter much in the end, I'm fine with what you have.  And it
can always be refactored later.

> +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;
> +  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;

Are the nullptr checks needed?  Are these fields initially nullptr, or
they have some other value?


> +struct gdb_disassemble_info
> +{
> +  DISABLE_COPY_AND_ASSIGN (gdb_disassemble_info);
>  
> -  /* Return the gdbarch of gdb_disassembler.  */
> +  /* Return the gdbarch we are disassembing for.  */

disassembing -> disassembling

Simon