This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/6] Refactor disassembly code
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 (齐尧)