[9/9] Add unit test to gdbarch methods register_to_value and value_to_register

  This patch adds one unit test for gdbarch methods register_to_value and
value_to_register.  The test pass different combinations of {regnu, type}
to gdbarch_register_to_value and gdbarch_value_to_register.  In order
to do the test, add a new function create_new_frame to create a fake
frame.  It can be improved after we converted frame_info to class.

This test is useful with ASAN.  Suppose I incorrectly modified
the size of buffer as below,

@@ -1228,7 +1228,7 @@ ia64_register_to_value (struct frame_info *frame, int regnum,
                        int *optimizedp, int *unavailablep)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  gdb_byte in[MAX_REGISTER_SIZE];
+  gdb_byte in[1];

   /* Convert to TYPE.  */
   if (!get_frame_register_bytes (frame, regnum, 0,

build GDB with "-fsanitize=address" and run unittest.exp, asan can detect
such error,
  

On 2017-04-13 03:26, Yao Qi wrote:
> This patch adds one unit test for gdbarch methods register_to_value and
> value_to_register.  The test pass different combinations of {regnu, 
> type}
> to gdbarch_register_to_value and gdbarch_value_to_register.  In order
> to do the test, add a new function create_new_frame to create a fake
> frame.  It can be improved after we converted frame_info to class.
> 
> This test is useful with ASAN.  Suppose I incorrectly modified
> the size of buffer as below,
> 
> @@ -1228,7 +1228,7 @@ ia64_register_to_value (struct frame_info
> *frame, int regnum,
>                         int *optimizedp, int *unavailablep)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> -  gdb_byte in[MAX_REGISTER_SIZE];
> +  gdb_byte in[1];
> 
>    /* Convert to TYPE.  */
>    if (!get_frame_register_bytes (frame, regnum, 0,
> 
> build GDB with "-fsanitize=address" and run unittest.exp, asan can 
> detect
> such error,
> 
> =================================================================
> ==6003==ERROR: AddressSanitizer: stack-buffer-overflow on address
> 0x7fff60a36520 at pc 0xbaa15d bp 0x7fff60a36250 sp 0x7fff60a36248
> WRITE of size 16 at 0x7fff60a36520 thread T0
>     #0 0xbaa15c in frame_register_unwind(frame_info*, int, int*, int*,
> lval_type*, unsigned long*, int*, unsigned char*) gdb/frame.c:1119
>     #1 0xbaa43b in frame_register(frame_info*, int, int*, int*,
> lval_type*, unsigned long*, int*, unsigned char*) gdb/frame.c:1147
>     #2 0xbab998 in get_frame_register_bytes(frame_info*, int, unsigned
> long, int, unsigned char*, int*, int*) gdb/frame.c:1424
>     #3 0x6ff39e in ia64_register_to_value gdb/ia64-tdep.c:1236
>     #4 0xbc9276 in gdbarch_register_to_value(gdbarch*, frame_info*,
> int, type*, unsigned char*, int*, int*) gdb/gdbarch.c:2565
>     #5 0xbd8e6b in register_to_value_test 
> gdb/git/gdb/gdbarch-selftests.c:83
>     #6 0xd4e628 in tests_with_arch gdb/selftest-arch.c:75
>     #7 0xd4d02c in run_self_tests() gdb/selftest.c:48
>     #8 0xc8cc3d in maintenance_selftest gdb/maint.c:949
> 
> gdb:
> 
> 2017-04-11  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdbarch-selftests.c: New file.

Some unit tests are in the unittests directory.  Do we have a rule for 
what goes there versus in gdb/ directly?

> 	* frame.c [GDB_SELF_TESTS] (create_new_frame): New function.
> 	* frame.h [GDB_SELF_TESTS] (create_new_frame): Declare.
> ---
>  gdb/Makefile.in         |   2 +
>  gdb/findvar.c           |   1 -
>  gdb/frame.c             |  18 +++++++
>  gdb/frame.h             |   4 ++
>  gdb/gdbarch-selftests.c | 123 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 147 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/gdbarch-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 23e4bed..a926d9a 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1087,6 +1087,7 @@ SFILES = \
>  	gdb_obstack.c \
>  	gdb_usleep.c \
>  	gdbarch.c \
> +	gdbarch-selftests.c \
>  	gdbtypes.c \
>  	gnu-v2-abi.c \
>  	gnu-v3-abi.c \
> @@ -1697,6 +1698,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>  	gdb_usleep.o \
>  	gdb_vecs.o \
>  	gdbarch.o \
> +	gdbarch-selftests.o \
>  	gdbtypes.o \
>  	gnu-v2-abi.o \
>  	gnu-v3-abi.o \
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index ed4d5c1..84f4f47 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -1004,4 +1004,3 @@ address_from_register (int regnum, struct
> frame_info *frame)
> 
>    return result;
>  }
> -

Spurious change.

> diff --git a/gdb/frame.c b/gdb/frame.c
> index d4fc456..0dc8726 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1704,6 +1704,24 @@ select_frame (struct frame_info *fi)
>      }
>  }
> 
> +#if GDB_SELF_TEST
> +struct frame_info *
> +create_new_frame (struct gdbarch *gdbarch)
> +{
> +  struct frame_info *this_frame = XCNEW (struct frame_info);
> +  struct regcache *regcache = regcache_xmalloc (gdbarch, NULL);
> +
> +  sentinel_frame = create_sentinel_frame (NULL, regcache);
> +  sentinel_frame->prev = this_frame;
> +  sentinel_frame->prev_p = 1;;
> +  this_frame->prev_arch.p = 1;
> +  this_frame->prev_arch.arch = gdbarch;
> +  this_frame->next = sentinel_frame;
> +
> +  return this_frame;
> +}
> +#endif
> +
>  /* Create an arbitrary (i.e. address specified by user) or innermost 
> frame.
>     Always returns a non-NULL value.  */
> 
> diff --git a/gdb/frame.h b/gdb/frame.h
> index 1d0644f..78727b2 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -833,6 +833,10 @@ extern struct frame_info
> *deprecated_safe_get_selected_frame (void);
> 
>  extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR 
> pc);
> 
> +#if GDB_SELF_TEST
> +extern struct frame_info *create_new_frame (struct gdbarch *gdbarch);

The name create_new_frame suggests that it could be used to create a 
frame used in the standard GDB context.  Maybe create_test_frame would 
be more appropriate?

Even if this function is only used in tests, a comment explaining what 
it does would be nice, especially since it does more than creating a new 
frame (it sets up some global state).

> +#endif
> +
>  /* Return true if the frame unwinder for frame FI is UNWINDER; false
>     otherwise.  */
> 
> diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
> new file mode 100644
> index 0000000..df084ce
> --- /dev/null
> +++ b/gdb/gdbarch-selftests.c
> @@ -0,0 +1,123 @@
> +/* Self tests for gdbarch for GDB, the GNU debugger.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#if GDB_SELF_TEST
> +#include "selftest.h"
> +#include "selftest-arch.h"
> +#include "inferior.h"
> +
> +namespace selftests {
> +
> +/* Test gdbarch methods register_to_value and value_to_register.  */
> +
> +static void
> +register_to_value_test (struct gdbarch *gdbarch)
> +{
> +  const struct builtin_type *builtin = builtin_type (gdbarch);
> +  struct type *types[] =
> +    {
> +      builtin->builtin_void, builtin->builtin_char,
> +      builtin->builtin_short, builtin->builtin_int,
> +      builtin->builtin_long, builtin->builtin_signed_char,

Can you put all of them on their own line?

> +      builtin->builtin_unsigned_short,
> +      builtin->builtin_unsigned_int,
> +      builtin->builtin_unsigned_long,
> +      builtin->builtin_float,
> +      builtin->builtin_double,
> +      builtin->builtin_long_double,
> +      builtin->builtin_complex,
> +      builtin->builtin_double_complex,
> +      builtin->builtin_string,
> +      builtin->builtin_bool,
> +      builtin->builtin_long_long,
> +      builtin->builtin_unsigned_long_long,
> +      builtin->builtin_int8,
> +      builtin->builtin_uint8,
> +      builtin->builtin_int16,
> +      builtin->builtin_uint16,
> +      builtin->builtin_int32,
> +      builtin->builtin_uint32,
> +      builtin->builtin_int64,
> +      builtin->builtin_uint64,
> +      builtin->builtin_int128,
> +      builtin->builtin_uint128,
> +      builtin->builtin_char16,
> +      builtin->builtin_char32,
> +    };
> +
> +  current_inferior()->gdbarch = gdbarch;
> +
> +  struct frame_info *frame = create_new_frame (gdbarch);

If we need to reuse this in other tests, it

> +  const int num_regs = (gdbarch_num_regs (gdbarch)
> +			+ gdbarch_num_pseudo_regs (gdbarch));
> +
> +  /* Test gdbarch methods register_to_value and value_to_register with
> +     different combinations of register numbers and types.  */
> +  for (const auto &type : types)
> +    {
> +      for (auto regnum = 0; regnum < num_regs; regnum++)
> +	{
> +	  if (gdbarch_convert_register_p (gdbarch, regnum, type))
> +	    {
> +	      std::vector<gdb_byte> buf (TYPE_LENGTH (type));
> +	      int optim, unavail, ok;
> +
> +	      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
> +					      buf.data (), &optim, &unavail);
> +
> +	      SELF_CHECK (ok);
> +	      SELF_CHECK (!optim);
> +	      SELF_CHECK (!unavail);
> +
> +	      bool saw_no_process_error = false;
> +	      TRY
> +		{
> +		  gdbarch_value_to_register (gdbarch, frame, regnum, type,
> +					     buf.data ());
> +		}
> +	      CATCH (ex, RETURN_MASK_ERROR)
> +		{
> +		  if (strcmp (ex.message,
> +			      "You can't do that without a process to debug.")
> +		      == 0)
> +		    saw_no_process_error = true;
> +		}
> +
> +	      /* GDB wants to write registers to target, so it is expected
> +		 to see no process exception.  */
> +	      SELF_CHECK (saw_no_process_error);

When you subclass regcache to have a "mock" regcache, do you think we'll 
be able to test the success case of this?

> +	    }
> +	}
> +    }
> +}
> +
> +} // namespace selftests
> +#endif /* GDB_SELF_TEST */
> +
> +/* Suppress warning from -Wmissing-prototypes.  */
> +extern initialize_file_ftype _initialize_gdbarch_selftests;

I don't think we still need this anymore.

> +
> +void
> +_initialize_gdbarch_selftests (void)
> +{
> +#if GDB_SELF_TEST
> +  register_self_test_foreach_arch (selftests::register_to_value_test);
> +#endif
> +}

Thanks,

Simon
  

Simon Marchi <simon.marchi@polymtl.ca> writes:

>> 2017-04-11  Yao Qi  <yao.qi@linaro.org>
>>
>> 	* gdbarch-selftests.c: New file.
>
> Some unit tests are in the unittests directory.  Do we have a rule for
> what goes there versus in gdb/ directly?
>

My understanding is that unit tests to code shared between GDB and
GDBserver go to gdb/unittests, and GDB code unit tests stay in gdb/.

>>
>> +#if GDB_SELF_TEST
>> +extern struct frame_info *create_new_frame (struct gdbarch *gdbarch);
>
> The name create_new_frame suggests that it could be used to create a
> frame used in the standard GDB context.  Maybe create_test_frame would
> be more appropriate?

OK.

>
> Even if this function is only used in tests, a comment explaining what
> it does would be nice, especially since it does more than creating a
> new frame (it sets up some global state).

I'll add comments, but to be clear, I don't expect this function being
used somewhere else.  It is added only for this specific test.

>> +
>> +/* Test gdbarch methods register_to_value and value_to_register.  */
>> +
>> +static void
>> +register_to_value_test (struct gdbarch *gdbarch)
>> +{
>> +  const struct builtin_type *builtin = builtin_type (gdbarch);
>> +  struct type *types[] =
>> +    {
>> +      builtin->builtin_void, builtin->builtin_char,
>> +      builtin->builtin_short, builtin->builtin_int,
>> +      builtin->builtin_long, builtin->builtin_signed_char,
>
> Can you put all of them on their own line?
>

Sure.

>> +      builtin->builtin_unsigned_short,
>> +      builtin->builtin_unsigned_int,
>> +      builtin->builtin_unsigned_long,
>> +      builtin->builtin_float,
>> +      builtin->builtin_double,
>> +      builtin->builtin_long_double,
>> +      builtin->builtin_complex,
>> +      builtin->builtin_double_complex,
>> +      builtin->builtin_string,
>> +      builtin->builtin_bool,
>> +      builtin->builtin_long_long,
>> +      builtin->builtin_unsigned_long_long,
>> +      builtin->builtin_int8,
>> +      builtin->builtin_uint8,
>> +      builtin->builtin_int16,
>> +      builtin->builtin_uint16,
>> +      builtin->builtin_int32,
>> +      builtin->builtin_uint32,
>> +      builtin->builtin_int64,
>> +      builtin->builtin_uint64,
>> +      builtin->builtin_int128,
>> +      builtin->builtin_uint128,
>> +      builtin->builtin_char16,
>> +      builtin->builtin_char32,
>> +    };
>> +
>> +  current_inferior()->gdbarch = gdbarch;
>> +
>> +  struct frame_info *frame = create_new_frame (gdbarch);
>
> If we need to reuse this in other tests, it
>

It is an incomplete sentence.

>> +  const int num_regs = (gdbarch_num_regs (gdbarch)
>> +			+ gdbarch_num_pseudo_regs (gdbarch));
>> +
>> +  /* Test gdbarch methods register_to_value and value_to_register with
>> +     different combinations of register numbers and types.  */
>> +  for (const auto &type : types)
>> +    {
>> +      for (auto regnum = 0; regnum < num_regs; regnum++)
>> +	{
>> +	  if (gdbarch_convert_register_p (gdbarch, regnum, type))
>> +	    {
>> +	      std::vector<gdb_byte> buf (TYPE_LENGTH (type));
>> +	      int optim, unavail, ok;
>> +
>> +	      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
>> +					      buf.data (), &optim, &unavail);
>> +
>> +	      SELF_CHECK (ok);
>> +	      SELF_CHECK (!optim);
>> +	      SELF_CHECK (!unavail);
>> +
>> +	      bool saw_no_process_error = false;
>> +	      TRY
>> +		{
>> +		  gdbarch_value_to_register (gdbarch, frame, regnum, type,
>> +					     buf.data ());
>> +		}
>> +	      CATCH (ex, RETURN_MASK_ERROR)
>> +		{
>> +		  if (strcmp (ex.message,
>> +			      "You can't do that without a process to debug.")
>> +		      == 0)
>> +		    saw_no_process_error = true;
>> +		}
>> +
>> +	      /* GDB wants to write registers to target, so it is expected
>> +		 to see no process exception.  */
>> +	      SELF_CHECK (saw_no_process_error);
>
> When you subclass regcache to have a "mock" regcache, do you think
> we'll be able to test the success case of this?
>

We can add two protected methods "target_store_registers" and
"target_prepare_to_store" in class regcache.

void
regcache::target_prepare_to_store ()
{
  target_prepare_to_store (this);
}

In regcache subclass for this test, we can overwrite them to do nothing,
as if the target registers store is successful.

Alternatively, we can pass an object of target when constructing
regcache.  In normal code, we pass current_target, in unit tests, we
pass a "mock" target.  Taking multi-target support into account,
regcache can retrieve the target object from inferior or address space
(different inferiors or address spaces may be associated with different
targets.)

>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +} // namespace selftests
>> +#endif /* GDB_SELF_TEST */
>> +
>> +/* Suppress warning from -Wmissing-prototypes.  */
>> +extern initialize_file_ftype _initialize_gdbarch_selftests;
>
> I don't think we still need this anymore.

I'll remove it.
  

On 2017-04-19 06:26, Yao Qi wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
>>> +  current_inferior()->gdbarch = gdbarch;
>>> +
>>> +  struct frame_info *frame = create_new_frame (gdbarch);
>> 
>> If we need to reuse this in other tests, it
>> 
> 
> It is an incomplete sentence.

Ah, I think I was going to say:  we might need this in other tests, so 
it could be in a function "setup_test_environment", or something like 
that.  That function would reset all global state, prepare a fake 
inferior, frame, etc.

>>> +  const int num_regs = (gdbarch_num_regs (gdbarch)
>>> +			+ gdbarch_num_pseudo_regs (gdbarch));
>>> +
>>> +  /* Test gdbarch methods register_to_value and value_to_register 
>>> with
>>> +     different combinations of register numbers and types.  */
>>> +  for (const auto &type : types)
>>> +    {
>>> +      for (auto regnum = 0; regnum < num_regs; regnum++)
>>> +	{
>>> +	  if (gdbarch_convert_register_p (gdbarch, regnum, type))
>>> +	    {
>>> +	      std::vector<gdb_byte> buf (TYPE_LENGTH (type));
>>> +	      int optim, unavail, ok;
>>> +
>>> +	      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
>>> +					      buf.data (), &optim, &unavail);
>>> +
>>> +	      SELF_CHECK (ok);
>>> +	      SELF_CHECK (!optim);
>>> +	      SELF_CHECK (!unavail);
>>> +
>>> +	      bool saw_no_process_error = false;
>>> +	      TRY
>>> +		{
>>> +		  gdbarch_value_to_register (gdbarch, frame, regnum, type,
>>> +					     buf.data ());
>>> +		}
>>> +	      CATCH (ex, RETURN_MASK_ERROR)
>>> +		{
>>> +		  if (strcmp (ex.message,
>>> +			      "You can't do that without a process to debug.")
>>> +		      == 0)
>>> +		    saw_no_process_error = true;
>>> +		}
>>> +
>>> +	      /* GDB wants to write registers to target, so it is expected
>>> +		 to see no process exception.  */
>>> +	      SELF_CHECK (saw_no_process_error);
>> 
>> When you subclass regcache to have a "mock" regcache, do you think
>> we'll be able to test the success case of this?
>> 
> 
> We can add two protected methods "target_store_registers" and
> "target_prepare_to_store" in class regcache.
> 
> void
> regcache::target_prepare_to_store ()
> {
>   target_prepare_to_store (this);
> }
> 
> In regcache subclass for this test, we can overwrite them to do 
> nothing,
> as if the target registers store is successful.
> 
> Alternatively, we can pass an object of target when constructing
> regcache.  In normal code, we pass current_target, in unit tests, we
> pass a "mock" target.  Taking multi-target support into account,
> regcache can retrieve the target object from inferior or address space
> (different inferiors or address spaces may be associated with different
> targets.)

Ok, that sounds good!

Simon