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
@@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
extern void store_typed_address (gdb_byte *buf, struct type *type,
CORE_ADDR addr);
-
+extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
+ const gdb_byte *source, int source_size,
+ bool is_signed, enum bfd_endian byte_order);
+
/* From valops.c */
extern int watchdog;
@@ -33,6 +33,7 @@
#include "objfiles.h"
#include "language.h"
#include "dwarf2loc.h"
+#include "selftest.h"
/* Basic byte-swapping routines. All 'extract' functions return a
host-format integer from a target-format integer at ADDR which is
@@ -249,7 +250,46 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
}
+/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
+ bytes. If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
+ significant bytes. If SOURCE_SIZE is less than DEST_SIZE then either sign
+ or zero extended according to IS_SIGNED. Values are stored in memory with
+ endianess BYTE_ORDER. */
+void
+copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
+ int source_size, bool is_signed,
+ enum bfd_endian byte_order)
+{
+ signed int size_diff = dest_size - source_size;
+
+ /* Copy across everything from SOURCE that can fit into DEST. */
+
+ if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
+ memcpy (dest + size_diff, source, source_size);
+ else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
+ memcpy (dest, source - size_diff, dest_size);
+ else
+ memcpy (dest, source, std::min (source_size, dest_size));
+
+ /* Fill the remaining space in DEST by either zero extending or sign
+ extending. */
+
+ if (size_diff > 0)
+ {
+ gdb_byte extension = 0;
+ if (is_signed
+ && ((byte_order != BFD_ENDIAN_BIG && source[source_size - 1] & 0x80)
+ || (byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)))
+ extension = 0xff;
+
+ /* Extend into MSBs of SOURCE. */
+ if (byte_order == BFD_ENDIAN_BIG)
+ memset (dest, extension, size_diff);
+ else
+ memset (dest + source_size, extension, size_diff);
+ }
+}
/* Return a `value' with the contents of (virtual or cooked) register
REGNUM as found in the specified FRAME. The register's type is
@@ -1005,3 +1045,78 @@ address_from_register (int regnum, struct frame_info *frame)
return result;
}
+#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;
+ enum bfd_endian byte_order = i ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
+
+ store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
+ copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, false,
+ byte_order);
+ SELF_CHECK (dest_val == extract_unsigned_integer (destbuf, dest_size,
+ byte_order));
+ }
+}
+
+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;
+ 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_cuint_test (0x12345678, 8, 0x12345678, 4);
+ do_cuint_test (0x345678, 8, 0x12345678, 3);
+ do_cuint_test (0x5678, 2, 0x12345678, 3);
+ do_cuint_test (0xdeadbeef, 8, 0xdeadbeef, 4);
+ do_cuint_test (0xadbeef, 8, 0xdeadbeef, 3);
+ do_cuint_test (0xbeef, 2, 0xdeadbeef, 3);
+ do_cint_test (0x12345678, 8, 0x12345678, 4);
+ do_cint_test (0x345678, 8, 0x12345678, 3);
+ do_cint_test (0x5678, 2, 0x12345678, 3);
+ do_cint_test (0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
+ do_cint_test (0xffffffffffadbeef, 8, 0xdeadbeef, 3);
+ do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);
+}
+
+} // namespace findvar_test
+} // namespace selftests
+
+#endif
+
+void
+_initialize_findvar (void)
+{
+#if GDB_SELF_TEST
+ register_self_test (selftests::findvar_tests::copy_integer_to_size_test);
+#endif
+}
@@ -47,57 +47,24 @@
34th is a dummy for padding. */
#define MIPS_FBSD_NUM_FPREGS 34
-/* Supply a single register. If the source register size matches the
- size the regcache expects, this can use regcache_raw_supply(). If
- they are different, this copies the source register into a buffer
- that can be passed to regcache_raw_supply(). */
+/* Supply a single register. The register size might not match, so use
+ regcache->raw_supply_integer (). */
static void
mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
size_t len)
{
- struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
- if (register_size (gdbarch, regnum) == len)
- regcache_raw_supply (regcache, regnum, addr);
- else
- {
- enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
- gdb_byte buf[MAX_REGISTER_SIZE];
- LONGEST val;
-
- val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
- store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
- val);
- regcache_raw_supply (regcache, regnum, buf);
- }
+ regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
}
-/* Collect a single register. If the destination register size
- matches the size the regcache expects, this can use
- regcache_raw_supply(). If they are different, this fetches the
- register via regcache_raw_supply() into a buffer and then copies it
- into the final destination. */
+/* Collect a single register. The register size might not match, so use
+ regcache->raw_collect_integer (). */
static void
mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
size_t len)
{
- struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
- if (register_size (gdbarch, regnum) == len)
- regcache_raw_collect (regcache, regnum, addr);
- else
- {
- enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
- gdb_byte buf[MAX_REGISTER_SIZE];
- LONGEST val;
-
- regcache_raw_collect (regcache, regnum, buf);
- val = extract_signed_integer (buf, register_size (gdbarch, regnum),
- byte_order);
- store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
- }
+ regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len, true);
}
/* Supply the floating-point registers stored in FPREGS to REGCACHE.
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
static void
supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
{
- struct gdbarch *gdbarch = get_regcache_arch (regcache);
- enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
- gdb_byte buf[MAX_REGISTER_SIZE];
- store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
- extract_signed_integer ((const gdb_byte *) addr, 4,
- byte_order));
- regcache_raw_supply (regcache, regnum, buf);
+ regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
}
/* Unpack an elf_gregset_t into GDB's register cache. */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
mips64_elf_gregset_t *gregsetp, int regno)
{
struct gdbarch *gdbarch = get_regcache_arch (regcache);
- enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
int regaddr, regi;
mips64_elf_greg_t *regp = *gregsetp;
void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,
if (regaddr != -1)
{
- gdb_byte buf[MAX_REGISTER_SIZE];
- LONGEST val;
-
- regcache_raw_collect (regcache, regno, buf);
- val = extract_signed_integer (buf, register_size (gdbarch, regno),
- byte_order);
dst = regp + regaddr;
- store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+ regcache->raw_collect_integer (regno, (gdb_byte *) dst, 8, true);
}
}
@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
}
else if (regno == mips_regnum (gdbarch)->fp_control_status)
{
- gdb_byte buf[MAX_REGISTER_SIZE];
- LONGEST val;
-
- regcache_raw_collect (regcache, regno, buf);
- val = extract_signed_integer (buf, register_size (gdbarch, regno),
- byte_order);
to = (gdb_byte *) (*fpregsetp + 32);
- store_signed_integer (to, 4, byte_order, val);
+ regcache->raw_collect_integer (regno, to, 4, true);
}
else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
{
- gdb_byte buf[MAX_REGISTER_SIZE];
- LONGEST val;
-
- regcache_raw_collect (regcache, regno, buf);
- val = extract_signed_integer (buf, register_size (gdbarch, regno),
- byte_order);
to = (gdb_byte *) (*fpregsetp + 32) + 4;
- store_signed_integer (to, 4, byte_order, val);
+ regcache->raw_collect_integer (regno, to, 4, true);
}
else if (regno == -1)
{
@@ -294,8 +294,14 @@ public:
void raw_collect (int regnum, void *buf) const;
+ void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+ bool is_signed) const;
+
void raw_supply (int regnum, const void *buf);
+ void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+ bool is_signed);
+
void raw_supply_zeroed (int regnum);
void raw_copy (int regnum, struct regcache *src_regcache);
@@ -1189,6 +1189,31 @@ regcache::raw_supply (int regnum, const void *buf)
}
}
+/* Supply register REGNUM to REGCACHE. Value to supply is an integer stored at
+ address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. If
+ the register size is greater than ADDR_LEN, then the integer will be sign or
+ zero extended. If the register size is smaller than the integer, then the
+ most significant bytes of the integer will be truncated. */
+
+void
+regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+ bool is_signed)
+{
+ enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+ gdb_byte *regbuf;
+ size_t regsize;
+
+ gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+ gdb_assert (!m_readonly_p);
+
+ regbuf = register_buffer (regnum);
+ regsize = m_descr->sizeof_register[regnum];
+
+ copy_integer_to_size (regbuf, regsize, addr, addr_len, is_signed,
+ byte_order);
+ m_register_status[regnum] = REG_VALID;
+}
+
/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same
as calling raw_supply with NULL (which will set the state to
unavailable). */
@@ -1232,6 +1257,29 @@ regcache::raw_collect (int regnum, void *buf) const
memcpy (buf, regbuf, size);
}
+/* Collect register REGNUM from REGCACHE. Store collected value as an integer
+ at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.
+ If ADDR_LEN is greater than the register size, then the integer will be sign
+ or zero extended. If ADDR_LEN is smaller than the register size, then the
+ most significant bytes of the integer will be truncated. */
+
+void
+regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+ bool is_signed) const
+{
+ enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+ const gdb_byte *regbuf;
+ size_t regsize;
+
+ gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+ regbuf = register_buffer (regnum);
+ regsize = m_descr->sizeof_register[regnum];
+
+ copy_integer_to_size (addr, addr_len, regbuf, regsize, is_signed,
+ byte_order);
+}
+
void
regcache::raw_copy (int regnum, struct regcache *src_regcache)
{