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

Pedro Alves
2017-01-12T15:15:42Z

On 01/10/2017 12:26 PM, Yao Qi wrote:
> + class gdb_disassembler_test : public gdb_disassembler
> + {
> + public:
> +
> +#ifndef DISASSEMBLER_TEST_VERBOSE
> + explicit gdb_disassembler_test (struct gdbarch *gdbarch,
> + const gdb_byte *insn)
> + : gdb_disassembler (gdbarch, ui_file_new (),
> + gdb_disassembler_test::read_memory),
> + m_insn (insn)
> + {
> + }
> +
> + ~gdb_disassembler_test ()
> + {
> + ui_file_delete ((struct ui_file *) m_di.stream);

Hmm, looks like you've made m_di be "protected" for
these uses.

But we have the public stream() method already,
so I think could be:

~gdb_disassembler_test ()
{
ui_file_delete (stream ());
}

You could then make m_di private again.

But you shouldn't really need to create a new stream for
testing. We have other places that want to print to a
a null stream. We can factor out out the null_stream creation
from gdb_insn_length into a new function:

struct ui_file *
null_stream ()
{
static struct ui_file *stream = NULL;

if (stream == NULL)
{
stream = ui_file_new ();
make_final_cleanup (do_ui_file_delete, stream);
}
return stream;
}

and then use it wherever necessary.

> + }
> +#else
> + explicit gdb_disassembler_test (struct gdbarch *gdbarch,
> + const gdb_byte *insn)
> + : gdb_disassembler (gdbarch, gdb_stdout,
> + gdb_disassembler_test::read_memory),
> + m_insn (insn)
> + {
> + }
> +
> + int
> + print_insn (CORE_ADDR memaddr)
> + {
> + fprintf_unfiltered (stream (), "%s ",
> + gdbarch_bfd_arch_info (arch ())->arch_name);
> +
> + int len = gdb_disassembler::print_insn (memaddr);
> +
> + fprintf_unfiltered (stream (), "\n");
> + return len;
> + }
> +#endif /* DISASSEMBLER_TEST_VERBOSE */
> +

I think it'll be nicer if you make DISASSEMBLER_TEST_VERBOSE always
defined, either to 0 or 1, so we can use
"if (DISASSEMBLER_TEST_VERBOSE)" instead of #ifdef.
That ensures that both paths keep compiling. The compiler
easily gets rid of the dead path when compiling with
optimizations enabled.

Putting it all together, you'd have something like:

explicit gdb_disassembler_test (struct gdbarch *gdbarch,
struct ui_file *stream,
const gdb_byte *insn)
: gdb_disassembler (gdbarch,
(DISASSEMBLER_TEST_VERBOSE
? gdb_stdout : null_stream ()),
gdb_disassembler_test::read_memory),
m_insn (insn)
{}

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;
}

Thanks,
Pedro Alves