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

Pedro Alves
2017-01-20T00:04:03Z

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