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

  This patch adds one unit test, which disassemble one instruction for
every gdbarch if available.  The test needs one valid instruction of
each gdbarch, and most of them are got from breakpoint instruction.
For the rest gdbarch whose breakpoint instruction isn't a valid
instruction, I copy one instruction from the gas/testsuite/gas/
directory.

I get the valid instruction of most gdbarch except ia64, mep, mips,
tic6x, and xtensa.  People familiar with these arch should be easy
to extend the test.

In order to achieve "do the unit test for every gdbarch", I extend
selftest.[c,h], so that we can register a function pointer, which
has one argument gdbarch.  selftest.c will iterate over all gdbarches
to call the registered function pointer.

gdb:

2017-01-06  Yao Qi  <yao.qi@linaro.org>

	* disasm.c [GDB_SELF_TEST]: Include selftest.h.
	(selftests::gdb_disassembler_print_one_insn_test): New function.
	(_initialize_disasm): New function.
	* selftest.c: Include "arch-utils.h".
	(gdbarch_tests): New vector.
	(register_self_test): New function.
	(run_self_tests): Iterate each gdbarch and call functions in
	gdbarch_tests.
	* selftest.h (self_test_function_with_gdbarch): New typedef.
	(register_self_test): Declare.
---
 gdb/disasm.c   | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/selftest.c |  55 +++++++++++++++++++++
 gdb/selftest.h |   3 ++
 3 files changed, 206 insertions(+)
  

On 2017-01-10 07:26, Yao Qi wrote:
> @@ -290,6 +294,139 @@ gdb_disassembler::pretty_print_insn (struct 
> ui_out *uiout,
>    return size;
>  }
> 
> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> +/* Test disassembly one instruction.  */

I'd say either

   /* Test disassembly of one instruction.  */

or

   /* Test disassembling one instruction.  */

> +
> +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;
> +    case bfd_arch_arm:
> +      /* mov     r0, #0 */
> +      insn = (const gdb_byte[]) {0x0, 0x0, 0xa0, 0xe3};
> +      len = 4;
> +      break;
> +    case bfd_arch_ia64:
> +    case bfd_arch_mep:
> +    case bfd_arch_mips:
> +    case bfd_arch_tic6x:
> +    case bfd_arch_xtensa:
> +      return;
> +    case bfd_arch_s390:
> +      /* nopr %r7 */
> +      insn = (const gdb_byte[]) {0x07, 0x07};
> +      len = 2;
> +      break;
> +    case bfd_arch_xstormy16:
> +      /* nop */
> +      insn = (const gdb_byte[]) {0x0, 0x0};
> +      len = 2;
> +      break;
> +    case bfd_arch_arc:
> +      {
> +	/* PR 21003 */
> +	if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601)
> +	  return;
> +      }
> +    case bfd_arch_nios2:
> +    case bfd_arch_score:
> +      insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &len);
> +      break;
> +    case bfd_arch_sh:
> +      insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 2, &len);
> +      break;

Is there a reason why these two can't fall in the default case?  If so, 
maybe add a comment explaining why?

> +  private:
> +    const gdb_byte *m_insn;
> +
> +    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);
> +
> +      memcpy (myaddr, self->m_insn, len);

Would this break if the disassembler decided to do a memory read at 
memaddr != 0?  I suppose it doesn't happen in practice now since the 
test passes, but it might some day, like if we make a test that 
disassembles more than one instruction.

I'd suggest either putting some kind of assert here that memaddr == 0, 
or consider memaddr in the copy, ideally with some bounds checking.
  

I'd much prefer if the core of the unit testing framework doesn't learn
about different random subsystems.  Consider what we'd do if we
wanted to reuse selftest.c in gdbserver.  I think we will at some point.

How about we move all this gdbarch stuff elsewhere, like
gdb/arch-utils.c or a new gdb/arch-selftests.c?

You'd then have,

- call the new function typedef "arch_self_test_function"

- in arch-selftests.c:

static std::vector<arch_self_test_function *> gdbarch_tests;

void
register_arch_self_test (arch_self_test_function *function)
{
  gdbarch_tests.push_back (function);
}

namespace selftests {

arch_tests ()
{
  for (const auto &f : gdbarch_tests)
    {
      const char **arches = gdbarch_printable_names ();
      int i;
   [...]
}

} /* namespace selftests */

void
_initialize_arch_selftests (void)
{
#if GDB_SELF_TEST
  register_self_test (selftests::arch_tests);
#endif
}

Thanks,
Pedro Alves
  

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
  

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
  

On 17-01-12 15:15:34, Pedro Alves wrote:
> 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.
> 

I did write code that way, but I changed it because the destroy of
stream is done in cleanup.  I want the test case depends on other
part of GDB as less as possible.  I hope this test case can be run
even without cleanup stuff.  It is sort of RAII (stream is regarded
as resource).
  

On 01/12/2017 03:34 PM, Yao Qi wrote:
> On 17-01-12 15:15:34, Pedro Alves wrote:
>> 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.
>>
> 
> I did write code that way, but I changed it because the destroy of
> stream is done in cleanup.  I want the test case depends on other
> part of GDB as less as possible.  I hope this test case can be run
> even without cleanup stuff.  It is sort of RAII (stream is regarded
> as resource).

Note that's a _final_ cleanup.  I.e., it only runs when GDB exits.
There's no need for gdb_disassembler_test to destroy the stream
with that.

And in my palves/cxx-eliminate-cleanups branch, where I've
C++-fied the whole ui_file hierarchy, the cleanup completely
disappears.

Thanks,
Pedro Alves
  

On 01/12/2017 03:15 PM, Pedro Alves wrote:
> Putting it all together, you'd have something like:
> 
>     explicit gdb_disassembler_test (struct gdbarch *gdbarch,
> 			            struct ui_file *stream,

                                    ^^^^^^^^^^^^^^^^^^^^^^

This stream parameter here shouldn't exist, of course.  It's just
that my Thunderbird hasn't learned to flag coding problems yet.  :-)

> 				    const gdb_byte *insn)
>       : gdb_disassembler (gdbarch, 
>                           (DISASSEMBLER_TEST_VERBOSE
>                            ? gdb_stdout : null_stream ()),
> 			  gdb_disassembler_test::read_memory),
> 	m_insn (insn)
>     {}
  

On 17-01-12 13:06:26, Pedro Alves wrote:
> I'd much prefer if the core of the unit testing framework doesn't learn
> about different random subsystems.  Consider what we'd do if we
> wanted to reuse selftest.c in gdbserver.  I think we will at some point.

Can we consider using some general c++ unit test frameworks rather than
selftest.c?  I don't see any issues if we can merge the unit test
results into dejagnu test result gdb.sum.

It should be straightforward to convert some c++ unit test result
to the dejagnu style, and use dg-extract-results.sh to merge them
into a single gdb.sum.

David proposed using gtest in gcc unit test, and the major objection
/concern is we may have two different format of test results.
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00765.html but the
concern can be addressed as I stated above.

> 
> How about we move all this gdbarch stuff elsewhere, like
> gdb/arch-utils.c or a new gdb/arch-selftests.c?
> 

OK, I'd like to name it as gdb/selftests-arch.c, because
arch-selftests.c looks like "a unit test case of arch".  However,
I want this file serves as "a runner to run one test case for every
gdbarch".
  

On 01/12/2017 05:03 PM, Yao Qi wrote:
> On 17-01-12 13:06:26, Pedro Alves wrote:
>> I'd much prefer if the core of the unit testing framework doesn't learn
>> about different random subsystems.  Consider what we'd do if we
>> wanted to reuse selftest.c in gdbserver.  I think we will at some point.
> 
> Can we consider using some general c++ unit test frameworks rather than
> selftest.c?  I don't see any issues if we can merge the unit test
> results into dejagnu test result gdb.sum.

We can consider anything, of course.  But I don't know whether
that's give us any significant advantage.  I never evaluated any myself.
Do we miss something important with the current framework?  
Would it be used as an as external dependency (required? optional?), or
would we import it into the codebase?  How would it
affect the way we write tests?  And the way we write the code that ends
up tested?  Would it force something on the codebase that would be
undesirable?  Etc., etc.

> It should be straightforward to convert some c++ unit test result
> to the dejagnu style, and use dg-extract-results.sh to merge them
> into a single gdb.sum.
> 
> David proposed using gtest in gcc unit test, and the major objection
> /concern is we may have two different format of test results.
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00765.html but the
> concern can be addressed as I stated above.
> 
>>
>> How about we move all this gdbarch stuff elsewhere, like
>> gdb/arch-utils.c or a new gdb/arch-selftests.c?
>>
> 
> OK, I'd like to name it as gdb/selftests-arch.c, because
> arch-selftests.c looks like "a unit test case of arch".  However,
> I want this file serves as "a runner to run one test case for every
> gdbarch".

Makes sense.

Thanks,
Pedro Alves
  

On Thu, Jan 12, 2017 at 5:43 PM, Pedro Alves <palves@redhat.com> wrote:
> On 01/12/2017 05:03 PM, Yao Qi wrote:
>> On 17-01-12 13:06:26, Pedro Alves wrote:
>>> I'd much prefer if the core of the unit testing framework doesn't learn
>>> about different random subsystems.  Consider what we'd do if we
>>> wanted to reuse selftest.c in gdbserver.  I think we will at some point.
>>
>> Can we consider using some general c++ unit test frameworks rather than
>> selftest.c?  I don't see any issues if we can merge the unit test
>> results into dejagnu test result gdb.sum.
>
> We can consider anything, of course.  But I don't know whether
> that's give us any significant advantage.  I never evaluated any myself.
> Do we miss something important with the current framework?

Nothing so far.  I need an assert or check that an exception should be
thrown in some statement in patch #7, like EXPECT_THROW in gtest.

All current unit/self tests are started by a command "maint selftests".
Probably, we can't unit test event loop or command handling.

> Would it be used as an as external dependency (required? optional?), or
> would we import it into the codebase?  How would it
> affect the way we write tests?  And the way we write the code that ends
> up tested?  Would it force something on the codebase that would be
> undesirable?  Etc., etc.
>

We only have one assertion SELF_CHECK, but we need more, like EXPECT_EQ,
EXPECT_TRUE, etc.  They can be easily added to selftest.h.  I don't
think of the answers of these questions either, but we should think of them
at this stage, because we don't have many unit test cases yet, it is
easy to change to any framework.  It is difficult to change when we have
thousands of unit tests on top of selftest.{c,h} framework.  so, if we
want to use
something as a framework, be careful :)

I'll try some c++ unit test frameworks in gdb, and see what I can get.