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

[Simon Marchi <simon.marchi@polymtl.ca>]

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.