This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

-- 
Yao (齐尧)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]