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

Pedro Alves
2017-01-12T14:35:38Z

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