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

[Pedro Alves <palves@redhat.com>]

On 01/10/2017 12:26 PM, Yao Qi wrote:
> +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;

This is construct is problematic.

Unfortunately, compound literals aren't valid C++.  They're
valid in C, but not in C++.  G++ accepts them as an extension.

Maybe all C++ compilers we care about support them too, so that's
not the real problem I'm pointing at.

The problem is that compound literal above may have automatic
storage duration, and thus die at the end of the switch scope.

From http://en.cppreference.com/w/c/language/compound_literal

 "The unnamed object to which the compound literal evaluates has static
 storage duration if the compound literal occurs at file scope and automatic
 storage duration if the compound literal occurs at block scope (in which
 case the object's lifetime ends at the end of the enclosing block)."

I didn't check the C standard, but I assume that's correct.

At <https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html>,
we see:

"As an optimization, G++ sometimes gives array compound literals
 longer lifetimes: when the array either appears outside a function
 or has a const-qualified type. If foo and its initializer had elements
 of type char *const rather than char *, or if foo were a global
 variable, the array would have static storage duration. But it is probably
 safest just to avoid the use of array compound literals in C++ code. "

Given all the warnings, I'd think it best to avoid the construct.
We can just name the arrays:

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 */
      static const gdb_byte bfin_insn[] = { 0x17, 0xe1, 0xff, 0xff };
      insn = bfin_insn;
      len = 4;
      break;
    case bfd_arch_bfin:
      /* mov     r0, #0 */
      static const gdb_byte arm_insn[] = { 0x0, 0x0, 0xa0, 0xe3 };
      insn = arm_insn;
      len = 4;


Or maybe make the compiler do the "sizeof" for us,
maybe eliminating copy/paste mistakes:

struct test_insn
 {
   template<size_t SIZE>
   explicit test_insn (const gdb_byte (&insn_)[Len])
    : insn (insn_), len_ (Len)
   {}
   gdb_byte *insn;
   int len;
 };

static void
gdb_disassembler_print_one_insn_test (struct gdbarch *gdbarch)
{
  test_insn insn = { NULL, -1 };

  switch (gdbarch_bfd_arch_info (gdbarch)->arch)
    {
    case bfd_arch_bfin:
      /* M3.L = 0xe117 */
      static const gdb_byte bfin_insn[] = { 0x17, 0xe1, 0xff, 0xff };
      insn = test_insn (bfin_insn);
      break;
    case bfd_arch_bfin:
      /* mov     r0, #0 */
      static const gdb_byte arm_insn[] = { 0x0, 0x0, 0xa0, 0xe3 };
      insn = test_insn (arm_insn);
  [....]


Maybe split the insns out of the switch:

/* M3.L = 0xe117 */
static const gdb_byte bfin_test_insn[] = { 0x17, 0xe1, 0xff, 0xff };
/* mov     r0, #0 */
static const gdb_byte arm_test_insn[] = { 0x0, 0x0, 0xa0, 0xe3 };

static void
gdb_disassembler_print_one_insn_test (struct gdbarch *gdbarch)
{
  test_insn insn = { NULL, -1 };

  switch (gdbarch_bfd_arch_info (gdbarch)->arch)
    {
    case bfd_arch_bfin:
      insn = test_insn (bfin_test_insn);
      break;
    case bfd_arch_bfin:
      insn = test_insn (arm_test_insn);
      break;
  [....]


Just some ideas.

Thanks,
Pedro Alves