Clear *VAL in regcache_raw_read_unsigned
Commit Message
On 02/11/2016 10:12 AM, Yao Qi wrote:> Pedro Alves <palves@redhat.com> writes:
>
>> The issue you noticed exposed that regcache_raw_read_unsigned function
>> is broken for memcpy'ing a 32-bit value into a 64-bit variable, and I think
>> your patch papered over the problem for little endian, only.
>
> regcache_raw_read_unsigned has two orthogonal issues, one is VAL isn't
> cleared, and the other one is the endianness issue. My "Clear *VAL"
> patch fixes the former, and I didn't realize the latter then.
I guess you could see it that way, though the way I imagine fixing this
handles both issues as one.
> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 2af8e24..d69ed5b 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -21,6 +21,8 @@
> #include "gdbthread.h"
> #include "tdesc.h"
> #include "rsp-low.h"
> +#include <endian.h>
> +
> #ifndef IN_PROCESS_AGENT
>
> struct regcache *
> @@ -441,7 +443,27 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
> (int) sizeof (ULONGEST));
>
> *val = 0;
> - collect_register (regcache, regnum, val);
> +
> + if (__BYTE_ORDER == __LITTLE_ENDIAN)
> + {
> + /* Little-endian values always sit at the left end of the buffer. */
> + collect_register (regcache, regnum, val);
> + }
> + else if (__BYTE_ORDER == __BIG_ENDIAN)
> + {
> + /* Big-endian values sit at the right end of the buffer. In case of
> + registers whose sizes are smaller than sizeof (long), we must use a
> + padding to access them correctly. */
> + int size = register_size (regcache->tdesc, regnum);
> +
> + if (size < sizeof (ULONGEST))
> + {
> + collect_register (regcache, regnum,
> + (char *) val + sizeof (ULONGEST) - size);
> + }
> + else
> + collect_register (regcache, regnum, val);
> + }
I was thinking we'd just use properly sized types, and the let the
compiler do the zero extension:
uint8_t u8;
uint16_t u16;
uint32_t u32;
uint64_t u64;
switch (size)
{
case 1:
collect_register (regcache, regnum, &u8);
*val = u8;
break;
case 2:
collect_register (regcache, regnum, &u16);
*val = u16;
break;
case 4:
collect_register (regcache, regnum, &u32);
*val = u32;
break;
case 8:
collect_register (regcache, regnum, &u64);
*val = u64;
break;
}
This should work in either endianess, and the '*val = 0' is no longer
necessary either.
Or maybe better, just byte the bullet and make gdbserver use
extract_unsigned_integer, like gdb.
The problem with that is that gdbserver can't currently use 'enum bfd_endian',
which IIRC, was already an issue in the get-next-pcs stuff. I've attached a
patch series that handles that by moving bfd_endian to a separate header.
I've pushed it to the users/palves/gdbserver-extract-unsigned-integer branch
as well.
If you agree with this, I'll run the bfd_endian patch by the binutils folks.
Thanks,
Pedro Alves
Comments
Pedro Alves <palves@redhat.com> writes:
> uint8_t u8;
> uint16_t u16;
> uint32_t u32;
> uint64_t u64;
>
> switch (size)
> {
> case 1:
> collect_register (regcache, regnum, &u8);
> *val = u8;
> break;
> case 2:
> collect_register (regcache, regnum, &u16);
> *val = u16;
> break;
> case 4:
> collect_register (regcache, regnum, &u32);
> *val = u32;
> break;
> case 8:
> collect_register (regcache, regnum, &u64);
> *val = u64;
> break;
> }
>
> This should work in either endianess, and the '*val = 0' is no longer
> necessary either.
I think this is better than 'make gdbserver use extract_unsigned_integer',
which looks overkill to me.
>
> Or maybe better, just byte the bullet and make gdbserver use
> extract_unsigned_integer, like gdb.
>
> The problem with that is that gdbserver can't currently use 'enum bfd_endian',
> which IIRC, was already an issue in the get-next-pcs stuff. I've attached a
get-next-pcs stuff needs endianness in GDB side. In GDBserver,
endianness is not needed.
> patch series that handles that by moving bfd_endian to a separate header.
> I've pushed it to the users/palves/gdbserver-extract-unsigned-integer branch
> as well.
>
> If you agree with this, I'll run the bfd_endian patch by the binutils folks.
Since the endianness of GDBserver is always identical to the endianness
of the program, I am not sure sharing extract_unsigned_integer between
GDB and GDBserver is necessary.
On 02/11/2016 03:15 PM, Yao Qi wrote:
> I think this is better than 'make gdbserver use extract_unsigned_integer',
> which looks overkill to me.
I think it's only going to bite us back in the future.
From the perspective of potentially making it easier to share more
code between gdb and gdbserver, I'd prefer that. Would you object it?
>
>>
>> Or maybe better, just byte the bullet and make gdbserver use
>> extract_unsigned_integer, like gdb.
>>
>> The problem with that is that gdbserver can't currently use 'enum bfd_endian',
>> which IIRC, was already an issue in the get-next-pcs stuff. I've attached a
>
> get-next-pcs stuff needs endianness in GDB side. In GDBserver,
> endianness is not needed.
The get-next-pcs stuff does have endianness bits, but it works
around the lack of 'enum bfd_endian' by hacking it through an int instead:
void
arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
struct arm_get_next_pcs_ops *ops,
int byte_order,
int byte_order_for_code,
int has_thumb2_breakpoint,
struct regcache *regcache)
{
$ grep byte_order *
arm-get-next-pcs.c: int byte_order,
arm-get-next-pcs.c: int byte_order_for_code,
arm-get-next-pcs.c: self->byte_order = byte_order;
arm-get-next-pcs.c: self->byte_order_for_code = byte_order_for_code;
arm-get-next-pcs.c: int byte_order_for_code = self->byte_order_for_code;
arm-get-next-pcs.c: insn1 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
arm-get-next-pcs.c: insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
arm-get-next-pcs.c: insn1 = self->ops->read_mem_uint (loc, 2,byte_order_for_code);
arm-get-next-pcs.c: insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
arm-get-next-pcs.c: int byte_order_for_code = self->byte_order_for_code;
arm-get-next-pcs.c: insn = self->ops->read_mem_uint (loc, 4, byte_order_for_code);
arm-get-next-pcs.c: insn = self->ops->read_mem_uint (loc, 4, byte_order_for_code);
arm-get-next-pcs.c: int byte_order = self->byte_order;
arm-get-next-pcs.c: int byte_order_for_code = self->byte_order_for_code;
arm-get-next-pcs.c: inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
arm-get-next-pcs.c: inst1 = self->ops->read_mem_uint (pc, 2,byte_order_for_code);
arm-get-next-pcs.c: inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
arm-get-next-pcs.c: inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
arm-get-next-pcs.c: nextpc = self->ops->read_mem_uint (sp + offset, 4, byte_order);
arm-get-next-pcs.c: inst2 = self->ops->read_mem_uint (pc + 2, 2, byte_order_for_code);
arm-get-next-pcs.c: nextpc = self->ops->read_mem_uint (addr + offset, 4, byte_order);
arm-get-next-pcs.c: = self->ops->read_mem_uint (base, 4, byte_order);
arm-get-next-pcs.c: length = 2 * self->ops->read_mem_uint (table + offset, 1, byte_order);
arm-get-next-pcs.c: length = 2 * self->ops->read_mem_uint (table + offset, 2, byte_order);
arm-get-next-pcs.c: int byte_order = self->byte_order;
arm-get-next-pcs.c: int byte_order_for_code = self->byte_order_for_code;
arm-get-next-pcs.c: this_instr = self->ops->read_mem_uint (pc, 4, byte_order_for_code);
arm-get-next-pcs.c: 4, byte_order);
arm-get-next-pcs.c: 4, byte_order);
arm-get-next-pcs.h: ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order);
arm-get-next-pcs.h: int byte_order;
arm-get-next-pcs.h: int byte_order_for_code;
arm-get-next-pcs.h: int byte_order,
arm-get-next-pcs.h: int byte_order_for_code,
>
>> patch series that handles that by moving bfd_endian to a separate header.
>> I've pushed it to the users/palves/gdbserver-extract-unsigned-integer branch
>> as well.
>>
>> If you agree with this, I'll run the bfd_endian patch by the binutils folks.
>
> Since the endianness of GDBserver is always identical to the endianness
> of the program, I am not sure sharing extract_unsigned_integer between
> GDB and GDBserver is necessary.
Thanks,
Pedro Alves
Pedro Alves writes:
> On 02/11/2016 03:15 PM, Yao Qi wrote:
>
>> I think this is better than 'make gdbserver use extract_unsigned_integer',
>> which looks overkill to me.
>
> I think it's only going to bite us back in the future.
>
> From the perspective of potentially making it easier to share more
> code between gdb and gdbserver, I'd prefer that. Would you object it?
>
For what it's worth, I would also like extract_unsigned_integer to be in
common and have the bfd_enums moved if possible.
>> get-next-pcs stuff needs endianness in GDB side. In GDBserver,
>> endianness is not needed.
Note that I dropped BE8 support from my single step patches, but should
we ever want to reintroduce something like that endianness would be
needed in GDBServer.
>
> The get-next-pcs stuff does have endianness bits, but it works
> around the lack of 'enum bfd_endian' by hacking it through an int instead:
>
That hack is indeed unfortunate.
>>
>>> patch series that handles that by moving bfd_endian to a separate header.
>>> I've pushed it to the users/palves/gdbserver-extract-unsigned-integer branch
>>> as well.
Thank you!
Regards,
Antoine
From 04956cf9b84b53728e20f0dae1f561ab26714453 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 11 Feb 2016 11:44:35 +0000
Subject: [PATCH 3/3] Fix gdbserver's regcache_raw_read_unsigned on big endian
hosts
The regcache_raw_read_unsigned function is memcpy'ing a 32-bit value
directly into a 64-bit variable, which doesn't work on big endian
targets.
Fix this by memcpy'ing to a buffer, and then using
extract_unsigned_integer, just like gdb's version.
gdb/gdbserver/ChangeLog:
2016-02-11 Pedro Alves <palves@redhat.com>
* Makefile.in (SFILES): Add $(srcdir)/common/gdb-byteswap.c.
(gdb-byteswap.o): New rule.
* regcache.c: Include "gdb-byteswap.h".
(host_bfd_endian): New function.
(regcache_raw_read_unsigned): Use extract_unsigned_integer and
host_bfd_endian.
---
gdb/gdbserver/Makefile.in | 7 ++++++-
gdb/gdbserver/regcache.c | 31 ++++++++++++++++++++++++-------
2 files changed, 30 insertions(+), 8 deletions(-)
@@ -185,7 +185,8 @@ SFILES= $(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
$(srcdir)/common/btrace-common.c \
$(srcdir)/common/fileio.c $(srcdir)/nat/linux-namespaces.c \
$(srcdir)/arch/arm.c $(srcdir)/common/common-regcache.c \
- $(srcdir)/arch/arm-linux.c $(srcdir)/arch/arm-get-next-pcs.c
+ $(srcdir)/arch/arm-linux.c $(srcdir)/arch/arm-get-next-pcs.c \
+ $(srcdir)/common/gdb-byteswap.c
DEPFILES = @GDBSERVER_DEPFILES@
@@ -200,6 +201,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
tdesc.o print-utils.o rsp-low.o errors.o common-debug.o cleanups.o \
common-exceptions.o symbol.o btrace-common.o fileio.o common-regcache.o \
+ gdb-byteswap.o \
$(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
GDBREPLAY_OBS = gdbreplay.o version.o
GDBSERVER_LIBS = @GDBSERVER_LIBS@
@@ -590,6 +592,9 @@ fileio.o: ../common/fileio.c
common-regcache.o: ../common/common-regcache.c
$(COMPILE) $<
$(POSTCOMPILE)
+gdb-byteswap.o: ../common/gdb-byteswap.c
+ $(COMPILE) $<
+ $(POSTCOMPILE)
# Arch object files rules form ../arch
@@ -21,6 +21,9 @@
#include "gdbthread.h"
#include "tdesc.h"
#include "rsp-low.h"
+#include "bfd-types.h"
+#include "gdb-byteswap.h"
+
#ifndef IN_PROCESS_AGENT
struct regcache *
@@ -424,14 +427,25 @@ collect_register (struct regcache *regcache, int n, void *buf)
register_size (regcache->tdesc, n));
}
+#ifndef IN_PROCESS_AGENT
+
+/* Return host endianness as an enum bfd_endian. */
+
+static enum bfd_endian
+host_bfd_endian (void)
+{
+ return (__BYTE_ORDER == __LITTLE_ENDIAN
+ ? BFD_ENDIAN_LITTLE
+ : BFD_ENDIAN_BIG);
+}
+
enum register_status
regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
ULONGEST *val)
{
int size;
-
- gdb_assert (regcache != NULL);
- gdb_assert (regnum >= 0 && regnum < regcache->tdesc->num_registers);
+ gdb_byte *buf;
+ enum bfd_endian byteorder;
size = register_size (regcache->tdesc, regnum);
@@ -440,14 +454,17 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
"%d bytes."),
(int) sizeof (ULONGEST));
- *val = 0;
- collect_register (regcache, regnum, val);
+ buf = (gdb_byte *) alloca (size);
+ collect_register (regcache, regnum, buf);
+
+ /* Assume the inferior's byte order is the same as gdbserver's (the
+ host). */
+ byteorder = host_bfd_endian ();
+ *val = extract_unsigned_integer (buf, size, byteorder);
return REG_VALID;
}
-#ifndef IN_PROCESS_AGENT
-
void
collect_register_as_string (struct regcache *regcache, int n, char *buf)
{
--
1.9.3