Re: [PATCH 4/6] Disassembly unit test: disassemble one instruction

[Pedro Alves <palves@redhat.com>]

On 01/16/2017 10:02 AM, Yao Qi wrote:

> +static void
> +gdb_disassembler_print_one_insn_test (struct gdbarch *gdbarch)
> +{
> +  size_t len = 0;
> +  const gdb_byte *insn = NULL;
> +

> +      insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, (int *) &len);

That "(int *) &len" is invalid code.  It's an aliasing violation.
And even if that weren't a problem, consider what happens when
sizeof size_t != sizeof int, on big endian and little endian.

Use a temporary variable of the right type, like e.g.:

      int bplen;
      insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &bplen);
      len = bplen;

> +      break;
> +    default:
> +      {
> +	/* Test disassemble breakpoint instruction.  */
> +	CORE_ADDR pc = 0;
> +	int kind = gdbarch_breakpoint_kind_from_pc (gdbarch, &pc);
> +
> +	insn = gdbarch_sw_breakpoint_from_kind (gdbarch, kind,
> +						(int *) &len);

Ditto.


> +      len = sizeof (xstormy16_insn);
> +      break;
> +    case bfd_arch_arc:
> +      {
> +	/* PR 21003 */
> +	if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601)
> +	  return;
> +      }

Odd that this case got braces when it doesn't declare any variable,
and when other cases don't.  Also, is the fallthrough intended?
If so, add a comment otherwise we may get a warning with GCC 7.

> +    case bfd_arch_nios2:
> +    case bfd_arch_score:
> +      /* nios2 and score need to know the current instruction to select
> +	 breakpoint instruction.  Give the breakpoint instruction kind
> +	 explicitly.  */
> +      insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, (int *) &len);
> +      break;
> +    default:


> +
> +	break;
> +      }
> +    }
> +  SELF_CHECK (len > 0);
> +
> +  /* Test gdb_disassembler for a given gdbarch by reading data from a
> +     pre-allocated buffer.  If you want to see the disassembled
> +     instruction printed to gdb_stdout, set DISASSEMBLER_TEST_VERBOSE
> +     to true.  */
> +
> +  class gdb_disassembler_test : public gdb_disassembler
> +  {
> +  public:
> +
> +    const bool DISASSEMBLER_TEST_VERBOSE = false;

static.  We give macros long unique names in order to
avoid naming conflicts, but if this is no longer a macro,
the name could be shortened, to e.g., just:

 static const bool verbose = false;

> +
> +    explicit gdb_disassembler_test (struct gdbarch *gdbarch,
> +				    const gdb_byte *insn,
> +				    size_t len)
> +      : gdb_disassembler (gdbarch,
> +			  (DISASSEMBLER_TEST_VERBOSE
> +			   ? gdb_stdout : null_stream ()),
> +			  gdb_disassembler_test::read_memory),
> +	m_insn (insn), m_len (len)
> +    {
> +    }
> +
> +    int
> +    print_insn (CORE_ADDR memaddr)
> +    {
> +      if (DISASSEMBLER_TEST_VERBOSE)
> +	{
> +	  fprintf_unfiltered (stream (), "%s ",
> +			      gdbarch_bfd_arch_info (arch ())->arch_name);
> +	}
> +
> +      int len = gdb_disassembler::print_insn (memaddr);
> +
> +      if (DISASSEMBLER_TEST_VERBOSE)
> +	fprintf_unfiltered (stream (), "\n");
> +
> +      return len;
> +    }
> +
> +  private:
> +    /* A buffer contain one instruction.  */
> +    const gdb_byte *m_insn;
> +
> +    /* Length of the buffer.  */
> +    size_t m_len;
> +
> +    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);
> +
> +      /* The disassembler in opcodes may read more data than one
> +	 instruction.  */

I suggest:

      /* The opcodes disassembler may read more data than one
         instruction.  Supply infinite consecutive copies
         of the same instruction.

> +      for (unsigned int i = 0; i < len; i++)

size_t.

> +	myaddr[i] = self->m_insn[(memaddr + i) % self->m_len];

Clever.  :-)

> +
> +      return 0;
> +    }
> +  };
> +
> +  gdb_disassembler_test di (gdbarch, insn, len);
> +
> +  SELF_CHECK (di.print_insn (0) == len);
> +}
> +
> +} // namespace selftests
> +#endif /* GDB_SELF_TEST */
> +
> +/* Suppress warning from -Wmissing-prototypes.  */
> +extern initialize_file_ftype _initialize_disasm_test;
> +
> +void
> +_initialize_disasm_test (void)

The standard is to name the _initialize_foo function after
the file/module name:

 _initialize_disasm_selftests

> +
> +static void
> +tests_with_arch (void)

We longer need the "void" in C++.

> +{
> +  int failed = 0;
> +
> +  for (const auto &f : gdbarch_tests)
> +    {
> +      const char **arches = gdbarch_printable_names ();
> +      int i;
> +
> +      for (i = 0; arches[i] != NULL; i++)

Can be "for (int i ..." now.


> +/* Suppress warning from -Wmissing-prototypes.  */
> +extern initialize_file_ftype _initialize_selftests_with_arch;
> +
> +void
> +_initialize_selftests_with_arch (void)

Likewise (naming / void).

> +#ifndef SELFTEST_ARCH_H
> +#define SELFTEST_ARCH_H
> +
> +typedef void self_test_function_with_gdbarch (struct gdbarch *);
> +
> +extern void register_self_test (self_test_function_with_gdbarch *function);

IMO, overloading the "register_self_test" function is confusing.
This function and the register_self_test() function in selftest.c
are semantically different, not two ways to do the same
thing (like e.g. const char * vs std::string).

If nothing else, it makes it a bit harder to grep for / find
arch self tests (only) in the future.

I'd prefer calling this something else that indicates more clearly
what the selftest being registered is about.  That's why I had
suggested before the distinct:

  register_arch_self_test

Perhaps better would be:

  register_self_test_foreach_arch

And then self_test_function_with_gdbarch -> self_test_foreach_arch_function.

WDYT?

Thanks,
Pedro Alves