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

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> 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).

I understand exactly where you're unease comes from, and I had the same
feeling.  The problem I think is the name of these things.

So we used to have two classes 'gdb_disassembler' which is really a
wrapper around the libopcodes 'disassemble_info'.

Then we had 'gdb_pretty_print_disassembler' which had-a
gdb_disassembler.

In one of my original versions of this patch I added just
gdb_disassemble_info as a base class, then gdb_disassembler inherited
from that.

At some point the patch expanded, and the new classes took the
*_disassembler suffix rather than the *_disassemble_info suffix, which
was maybe a mistake.

So the classes you listed above, gdb_printing_disassembler and
gdb_disassembler, really are specialisations of gdb_disassemble_info,
but maybe this would all be better if I renamed all these things to
use a _disassemble_info suffix?

Thanks,
Andrew


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