Re: [PATCH 3/8] Disassembly unit test: disassemble one instruction

Simon Marchi
2017-01-11T21:15:18Z

On 2017-01-10 07:26, Yao Qi wrote:
> @@ -290,6 +294,139 @@ gdb_disassembler::pretty_print_insn (struct
> ui_out *uiout,
> return size;
> }
>
> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> +/* Test disassembly one instruction. */

I'd say either

/* Test disassembly of one instruction. */

or

/* Test disassembling one instruction. */

> +
> +static void
> +gdb_disassembler_print_one_insn_test (struct gdbarch *gdbarch)
> +{
> + int len = -1;
> + const gdb_byte *insn = NULL;
> +
> + switch (gdbarch_bfd_arch_info (gdbarch)->arch)
> + {
> + case bfd_arch_bfin:
> + /* M3.L = 0xe117 */
> + insn = (const gdb_byte[]) {0x17, 0xe1, 0xff, 0xff};
> + len = 4;
> + break;
> + case bfd_arch_arm:
> + /* mov r0, #0 */
> + insn = (const gdb_byte[]) {0x0, 0x0, 0xa0, 0xe3};
> + len = 4;
> + break;
> + case bfd_arch_ia64:
> + case bfd_arch_mep:
> + case bfd_arch_mips:
> + case bfd_arch_tic6x:
> + case bfd_arch_xtensa:
> + return;
> + case bfd_arch_s390:
> + /* nopr %r7 */
> + insn = (const gdb_byte[]) {0x07, 0x07};
> + len = 2;
> + break;
> + case bfd_arch_xstormy16:
> + /* nop */
> + insn = (const gdb_byte[]) {0x0, 0x0};
> + len = 2;
> + break;
> + case bfd_arch_arc:
> + {
> + /* PR 21003 */
> + if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601)
> + return;
> + }
> + case bfd_arch_nios2:
> + case bfd_arch_score:
> + insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &len);
> + break;
> + case bfd_arch_sh:
> + insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 2, &len);
> + break;

Is there a reason why these two can't fall in the default case? If so,
maybe add a comment explaining why?

> + private:
> + const gdb_byte *m_insn;
> +
> + static int read_memory (bfd_vma memaddr, gdb_byte *myaddr,
> + unsigned int len, struct disassemble_info *info)
> + {
> + gdb_disassembler_test *self
> + = static_cast<gdb_disassembler_test *>(info->application_data);
> +
> + memcpy (myaddr, self->m_insn, len);

Would this break if the disassembler decided to do a memory read at
memaddr != 0? I suppose it doesn't happen in practice now since the
test passes, but it might some day, like if we make a test that
disassembles more than one instruction.

I'd suggest either putting some kind of assert here that memaddr == 0,
or consider memaddr in the copy, ideally with some bounds checking.