Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves <palves@redhat.com>]

On 05/26/2017 09:54 AM, Alan Hayward wrote:

> 2017-05-25  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* defs.h (copy_integer_to_size): New declaration.
> 	* findvar.c (copy_integer_to_size): New function.
> 	(do_cuint_test): New selftest function.
> 	(do_cint_test): New selftest function.
> 	(copy_integer_to_size_test): Likewise.
> 	(_initialize_findvar): Likewise.
> 	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
> 	(mips_fbsd_collect_reg): Use raw_collect_integer.
> 	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
> 	(mips64_fill_gregset): Use raw_collect_integer
> 	(mips64_fill_fpregset): Use raw_supply_integer.
> 	* regcache.c (regcache::raw_supply_integer): New function.
> 	(regcache::raw_collect_integer): Likewise.
> 	* regcache.h: (regcache::raw_supply_integer): New declaration.
>         (regcache::raw_collect_integer): Likewise.

Last line looks like needs s/spaces/tab/.

> +#if GDB_SELF_TEST
> +namespace selftests {
> +namespace findvar_tests {
> +
> +/* Functions to test copy_integer_to_size.  Store SOURCE_VAL with size
> +   SOURCE_SIZE to a buffer, making sure no sign extending happens at this
> +   stage.  Copy buffer to a new buffer using copy_integer_to_size.  Extract
> +   copied value and compare to DEST_VAL.  Do all of this for both LITTLE and
> +   BIG target endians.  */
> +
> +static void
> +do_cuint_test (long dest_val, int dest_size, ULONGEST src_val, int src_size)
> +{
> +  for (int i = 0; i < 2 ; i++)
> +    {
> +      ULONGEST srcbuf_val = 0, destbuf_val = 0;
> +      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
> +      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;

Nit, I'd prefer writing instead:

      gdb_byte srcbuf[sizeof (ULONGEST)] = {};
      gdb_byte destbuf[sizeof (ULONGEST)] = {};

I.e., get rid of the local ULONGEST variables.  It took me a second
to realize that below you're writing to these variables instead of
to "dest_val" directly, which made me first wonder about
strict aliasing issues.

(But see further below.)

> +static void
> +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size)
> +{
> +  for (int i = 0; i < 2 ; i++)
> +    {
> +      LONGEST srcbuf_val = 0, destbuf_val = 0;
> +      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
> +      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;

Ditto.

> +      enum bfd_endian byte_order = i ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
> +
> +      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
> +      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true,
> +			    byte_order);
> +      SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size,
> +						      byte_order));
> +    }
> +}
> +
> +static void
> +copy_integer_to_size_test ()
> +{

...

> +  do_cint_test (0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
> +  do_cint_test (0xffffffffffadbeef, 8, 0xdeadbeef, 3);
> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);

Note that:

> +static void
> +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size)

"long" can be 32-bit, depending on host.  It's 32-bit on 64-bit Windows.

So you're passing 0xffffffffffffbeef, which gets truncated
to 0xffffbeef, but then sign extended here again:

      SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size,
						      byte_order));

and it'll work out (by chance).

I think it would be better to always pass an unsigned ULONGEST
as dest_val:

 static void
 do_cint_test (ULONGEST dest_val, int dest_size, LONGEST src_val, int src_size)

likewise for do_cuint_test, of course.

Makes me notice that you didn't add any unsigned test for
src/dest size > 4, which would fail on 64-bit Windows, for the
same reason (32-bit long), like:

 do_cuint_test (0xff12345678, 8, 0xff12345678, 6);


I notice you have no tests that exercise src_size == dest_size,
for multiple sizes.  It'd be good to add them.


Another issue with the test is that by relying on
extract_(un)signed_integer, the test can't notice if the
zero / sign extension in copy_integer_bytes filled the right
number of bytes:

> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);

I.e., above, the last test _must_ not write more than
two bytes to the destination, so those leading
0xFFs before "beef" are artificial.

Passing ULONGEST as destination value avoids that problem, making
you write that last test as:

   do_cint_test (0x000000000000beef, 2, 0xbeef, 3);


That brings us to the next problem, that you can't tell
whether zero extension writes the zeros in the first place.

I.e., here:

 do_cuint_test (0x12345678, 8, 0x12345678, 4);

Given do_cuint_test does this:

+      ULONGEST srcbuf_val = 0, destbuf_val = 0;

You can't tell whether copy_integer_to_size really
filled in bytes 4-8 with zero.

You can get that back by making the do_cint_test/do_int_test
functions memset the temporary buffers with some byte
that the callers never pass down as part of any src/dest value.
Like, say reserve 0xaa for that:

      gdb_byte srcbuf[sizeof (ULONGEST)];
      gdb_byte destbuf[sizeof (ULONGEST)];

      memset (srcbuf, 0xaa, sizeof (srcbuf));
      memset (destbuf, 0xaa, sizeof (srcbuf));

(with suitable comments)

So in sum:

#1 - ULONGEST as destination value, both signed and unsigned tests.
#2 - Add unsigned tests with size >4
#3 - Add tests src_size == dest_size
#4 - Use gdb_byte arrays for temporary buffers, and memset them with 0xaa.
#5 - Adjust tests to pass.
#6 - Add comments where appropriate.

Thanks,
Pedro Alves