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

[Pedro Alves <palves at redhat dot com>]

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