Re: [PATCH 2/6] Refactor disassembly code

[Luis Machado <lgustavo@codesourcery.com>]

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.