Re: [PATCH 2/6] Refactor disassembly code

Yao Qi
2017-01-18T16:34:19Z

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?

--
Yao (é½å°§)