[PATCH] Clear *VAL in regcache_raw_read

public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed

* [PATCH] Clear *VAL in regcache_raw_read_unsigned
@ 2016-02-09 14:54 Yao Qi
  2016-02-10 16:45 ` Yao Qi
  0 siblings, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-09 14:54 UTC (permalink / raw)
  To: gdb-patches

We have function regcache_raw_read_unsigned defined in both GDB and
GDBserver, so that it is used in common like this,

  ULONGEST value;
  status = regcache_raw_read_unsigned (regcache, regnum, &value);

'value' is correctly set in GDB side, but may not be correctly set
in GDBserver, because &value is passed in regcache_raw_read_unsigned
but collect_register may only set part of the whole variable.  In my
test, I see the top half of 'value' is garbage.  This patch fixes this
problem by clearing *VAL before calling collect_register.

Regression tests are still running.  I'll push it in if there is no
regression in tests.

gdb/gdbserver:

2016-02-09  Yao Qi  <yao.qi@linaro.org>

	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.
---
 gdb/gdbserver/regcache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 6a737ea..2af8e24 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -440,6 +440,7 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
             "%d bytes."),
           (int) sizeof (ULONGEST));

+  *val = 0;
   collect_register (regcache, regnum, val);

   return REG_VALID;
-- 
1.9.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-09 14:54 [PATCH] Clear *VAL in regcache_raw_read_unsigned Yao Qi
@ 2016-02-10 16:45 ` Yao Qi
  2016-02-10 16:52   ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-10 16:45 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

Yao Qi <qiyaoltc@gmail.com> writes:

> Regression tests are still running.  I'll push it in if there is no
> regression in tests.
>
> gdb/gdbserver:
>
> 2016-02-09  Yao Qi  <yao.qi@linaro.org>
>
> 	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.

Regression test on arm-linux is done.  I push it in to both master and
7.11 branch.  Note that this function is only used for software single
step on arm-linux, so I run tests for arm-linux target.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-10 16:45 ` Yao Qi
@ 2016-02-10 16:52   ` Pedro Alves
  2016-02-10 17:25     ` Yao Qi
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2016-02-10 16:52 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 02/10/2016 04:45 PM, Yao Qi wrote:
> Yao Qi <qiyaoltc@gmail.com> writes:
> 
>> Regression tests are still running.  I'll push it in if there is no
>> regression in tests.
>>
>> gdb/gdbserver:
>>
>> 2016-02-09  Yao Qi  <yao.qi@linaro.org>
>>
>> 	* regcache.c (regcache_raw_read_unsigned): Clear *VAL.
> 
> Regression test on arm-linux is done.  I push it in to both master and
> 7.11 branch.  Note that this function is only used for software single
> step on arm-linux, so I run tests for arm-linux target.

Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
the higher 32-bits of a 64-bit variable.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-10 16:52   ` Pedro Alves
@ 2016-02-10 17:25     ` Yao Qi
  2016-02-10 17:36       ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-10 17:25 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
> the higher 32-bits of a 64-bit variable.

What do you mean by "this"?  IIUC, "this" means
regcache_raw_read_unsigned, rather than my patch.  My patch just clears
*VAL before passing it to collect_register, so it shouldn't break anything
(I hope) on big endian.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-10 17:25     ` Yao Qi
@ 2016-02-10 17:36       ` Pedro Alves
  2016-02-11 10:12         ` Yao Qi
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2016-02-10 17:36 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 02/10/2016 05:25 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>> Isn't this broken on big endian?  AFAICS, we're reading 32-bits into
>> the higher 32-bits of a 64-bit variable.
> 
> What do you mean by "this"?  IIUC, "this" means
> regcache_raw_read_unsigned, rather than my patch.  My patch just clears
> *VAL before passing it to collect_register, so it shouldn't break anything
> (I hope) on big endian.

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.

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);

  size = register_size (regcache->tdesc, regnum);

  if (size > (int) sizeof (ULONGEST))
    error (_("That operation is not available on integers of more than"
            "%d bytes."),
          (int) sizeof (ULONGEST));

  collect_register (regcache, regnum, val);

  return REG_VALID;
}

VAL is 64-bit.  collect_register () writes directly into VAL, but it
only writes 32-bits.  On little endian, that will leave the upper halve
of VAL as garbage.  So your patch clears that.  But on big endian,
that collect_register() call writes into the upper halve of VAL, and
the result is that VAL now has the wrong value.

E.g., if the 32-bit register's value is supposed to be 0x11223344,
after the collect register call, *VAL ends up with 0x1122334400000000,
which happens to work for little endian, but not for big endian.

You should be able to trigger this on an ARM system with big endian code.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-10 17:36       ` Pedro Alves
@ 2016-02-11 10:12         ` Yao Qi
  2016-02-11 12:46           ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-11 10:12 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> 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.

>
> 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);
>
>   size = register_size (regcache->tdesc, regnum);
>
>   if (size > (int) sizeof (ULONGEST))
>     error (_("That operation is not available on integers of more than"
>             "%d bytes."),
>           (int) sizeof (ULONGEST));
>
>   collect_register (regcache, regnum, val);
>
>   return REG_VALID;
> }
>
> VAL is 64-bit.  collect_register () writes directly into VAL, but it
> only writes 32-bits.  On little endian, that will leave the upper halve
> of VAL as garbage.  So your patch clears that.  But on big endian,
> that collect_register() call writes into the upper halve of VAL, and
> the result is that VAL now has the wrong value.

On big endian, we need to clear VAL too, otherwise the bottom half of
VAL is garbage.

>
> E.g., if the 32-bit register's value is supposed to be 0x11223344,
> after the collect register call, *VAL ends up with 0x1122334400000000,
> which happens to work for little endian, but not for big endian.
>
> You should be able to trigger this on an ARM system with big endian code.

I don't have a big endian system on my hand, but my patch below, which is
copied from ppc_collect_ptrace_register, should fix the endianess problem.

-- 
Yao (齐尧)
From 330b996c191b119a6b0414c2d96c4e20f18588bf Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 11 Feb 2016 09:53:35 +0000
Subject: [PATCH] Fix regcache_raw_read_unsigned in big endian

This patch fixes function regcache_raw_read_unsigned in big endian.

gdb/gdbserver:

2016-02-11  Yao Qi  <yao.qi@linaro.org>

	* regcache.c: Include endian.h.
	(regcache_raw_read_unsigned): Handle big endian.

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);
+    }
 
   return REG_VALID;
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 10:12         ` Yao Qi
@ 2016-02-11 12:46           ` Pedro Alves
  2016-02-11 15:15             ` Yao Qi
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2016-02-11 12:46 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 2990 bytes --]

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


[-- Attachment #2: 0001-Move-enum-bfd_endian-to-a-non-generated-header.patch --]
[-- Type: text/x-patch, Size: 3578 bytes --]

From dd4a4f86240eac1262c760b6109b42242e007923 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 11 Feb 2016 11:35:04 +0000
Subject: [PATCH 1/3] Move 'enum bfd_endian' to a non-generated header

Because:

- GDB uses enum bfd_endian extensively.
- gdbserver does not build/link-with bfd.
- We'd like to share more code between gdb and gdbserver.

It'd make our lives easier if we could just use bfd_endian in
gdbserver as well.

The problem is that bfd_endian is defined in a header that only exists
if bfd is built/configured.

Thus this moves bfd_endian to a separate header, so gdbserver can
include it.

bfd/ChangeLog:
2016-02-11  Pedro Alves  <palves@redhat.com>

	* bfd-in.h: Include bfd-types.h
	* bfd-in2.h: Regenerate.
	* targets.c (enum bfd_endian): Moved to include/bfd-types.h.

include/ChangeLog:
2016-02-11  Pedro Alves  <palves@redhat.com>

	* bfd-types.h: New file.
---
 bfd/bfd-in.h        |  1 +
 bfd/bfd-in2.h       |  3 +--
 bfd/targets.c       |  2 --
 include/bfd-types.h | 26 ++++++++++++++++++++++++++
 4 files changed, 28 insertions(+), 4 deletions(-)
 create mode 100644 include/bfd-types.h

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index 5df2bab..277ca5f 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -35,6 +35,7 @@ extern "C" {
 #include "ansidecl.h"
 #include "symcat.h"
 #include <sys/stat.h>
+#include "bfd-types.h"
 
 #if defined (__STDC__) || defined (ALMOST_STDC) || defined (HAVE_STRINGIZE)
 #ifndef SABER
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index fb4858c..0d6b40f 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -42,6 +42,7 @@ extern "C" {
 #include "ansidecl.h"
 #include "symcat.h"
 #include <sys/stat.h>
+#include "bfd-types.h"
 
 #if defined (__STDC__) || defined (ALMOST_STDC) || defined (HAVE_STRINGIZE)
 #ifndef SABER
@@ -7124,8 +7125,6 @@ enum bfd_flavour
   bfd_target_sym_flavour
 };
 
-enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE, BFD_ENDIAN_UNKNOWN };
-
 /* Forward declaration.  */
 typedef struct bfd_link_info _bfd_link_info;
 
diff --git a/bfd/targets.c b/bfd/targets.c
index 50f3712..a2ee6ee 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -171,8 +171,6 @@ DESCRIPTION
 .  bfd_target_sym_flavour
 .};
 .
-.enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE, BFD_ENDIAN_UNKNOWN };
-.
 .{* Forward declaration.  *}
 .typedef struct bfd_link_info _bfd_link_info;
 .
diff --git a/include/bfd-types.h b/include/bfd-types.h
new file mode 100644
index 0000000..816fac4
--- /dev/null
+++ b/include/bfd-types.h
@@ -0,0 +1,26 @@
+/* bfd core types that do not depend on configuration.
+
+   Copyright (C) 1990-2016 Free Software Foundation, Inc.
+
+   This file is part of BFD, the Binary File Descriptor library.
+
+   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, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#ifndef BFD_TYPES_H
+#define BFD_TYPES_H
+
+enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE, BFD_ENDIAN_UNKNOWN };
+
+#endif
-- 
1.9.3


[-- Attachment #3: 0002-Move-store-extract-.-integer-routines-to-gdb-common.patch --]
[-- Type: text/x-patch, Size: 16717 bytes --]

From ca47aa93d4a13e676e771041eb7372ff2cdb1487 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 11 Feb 2016 11:35:05 +0000
Subject: [PATCH 2/3] Move store/extract ... integer routines to gdb/common/

In preparation for gdbserver using them as well.

gdb/ChangeLog:
2016-02-11  Pedro Alves  <palves@redhat.com>

	* Makefile.in (SFILES): Add common/gdb-byteswap.c.
	(HFILES_NO_SRCDIR): Add common/gdb-byteswap.h.
	(COMMON_OBS): Add common/gdb-byteswap.o.
	(gdb-byteswap.o): New rule.
	* common/common-types.h: Include bfd-types.h.
	* common/gdb-byteswap.c: New file.
	* common/gdb-byteswap.h: New file.
	* defs.h (extract_signed_integer, extract_unsigned_integer)
	(extract_long_unsigned_integer, extract_typed_address)
	(store_signed_integer, store_unsigned_integer): Moved to
	common/gdb-byteswap.h.
	* findvar.c (extract_signed_integer, extract_unsigned_integer)
	(extract_long_unsigned_integer, extract_typed_address)
	(store_signed_integer, store_unsigned_integer): Moved to
	common/gdb-byteswap.c.
---
 gdb/Makefile.in           |  11 ++-
 gdb/common/common-types.h |   4 +
 gdb/common/gdb-byteswap.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/gdb-byteswap.h |  49 +++++++++++
 gdb/defs.h                |  17 +---
 gdb/findvar.c             | 189 -------------------------------------------
 6 files changed, 265 insertions(+), 206 deletions(-)
 create mode 100644 gdb/common/gdb-byteswap.c
 create mode 100644 gdb/common/gdb-byteswap.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ec2af52..d63d8aa 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -896,6 +896,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	target/waitstatus.c common/print-utils.c common/rsp-low.c \
 	common/errors.c common/common-debug.c common/common-exceptions.c \
 	common/btrace-common.c common/fileio.c common/common-regcache.c \
+	common/gdb-byteswap.c \
 	$(SUBDIR_GCC_COMPILE_SRCS)
 
 LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -988,7 +989,9 @@ common/common-exceptions.h target/target.h common/symbol.h \
 common/common-regcache.h fbsd-tdep.h nat/linux-personality.h \
 common/fileio.h nat/x86-linux.h nat/x86-linux-dregs.h nat/amd64-linux-siginfo.h\
 nat/linux-namespaces.h arch/arm.h common/gdb_sys_time.h arch/aarch64-insn.h \
-tid-parse.h
+tid-parse.h \
+common/gdb-byteswap.h
+
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1087,7 +1090,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	format.o registry.o btrace.o record-btrace.o waitstatus.o \
 	print-utils.o rsp-low.o errors.o common-debug.o debug.o \
 	common-exceptions.o btrace-common.o fileio.o \
-	common-regcache.o \
+	common-regcache.o gdb-byteswap.o \
 	$(SUBDIR_GCC_COMPILE_OBS)
 
 TSOBS = inflow.o
@@ -2275,6 +2278,10 @@ common-regcache.o: ${srcdir}/common/common-regcache.c
 	$(COMPILE) $(srcdir)/common/common-regcache.c
 	$(POSTCOMPILE)
 
+gdb-byteswap.o: ${srcdir}/common/gdb-byteswap.c
+	$(COMPILE) $(srcdir)/common/gdb-byteswap.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common-types.h b/gdb/common/common-types.h
index efeb0db..760f477 100644
--- a/gdb/common/common-types.h
+++ b/gdb/common/common-types.h
@@ -20,6 +20,10 @@
 #ifndef COMMON_TYPES_H
 #define COMMON_TYPES_H
 
+/* This header is always available, even when BFD is not
+   configured.  */
+#include "bfd-types.h"
+
 #ifdef GDBSERVER
 
 /* * A byte from the program being debugged.  */
diff --git a/gdb/common/gdb-byteswap.c b/gdb/common/gdb-byteswap.c
new file mode 100644
index 0000000..ed2c8f5
--- /dev/null
+++ b/gdb/common/gdb-byteswap.c
@@ -0,0 +1,201 @@
+/* Basic byte-swapping routines, for GDB, the GNU debugger.
+
+   Copyright (C) 1986-2016 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 "common-defs.h"
+#include "gdb-byteswap.h"
+#include "host-defs.h"
+
+/* See gdb-byteswap.h.  */
+
+LONGEST
+extract_signed_integer (const gdb_byte *addr, int len,
+			enum bfd_endian byte_order)
+{
+  LONGEST retval;
+  const unsigned char *p;
+  const unsigned char *startaddr = addr;
+  const unsigned char *endaddr = startaddr + len;
+
+  if (len > (int) sizeof (LONGEST))
+    error (_("\
+That operation is not available on integers of more than %d bytes."),
+	   (int) sizeof (LONGEST));
+
+  /* Start at the most significant end of the integer, and work towards
+     the least significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      p = startaddr;
+      /* Do the sign extension once at the start.  */
+      retval = ((LONGEST) * p ^ 0x80) - 0x80;
+      for (++p; p < endaddr; ++p)
+	retval = (retval << 8) | *p;
+    }
+  else
+    {
+      p = endaddr - 1;
+      /* Do the sign extension once at the start.  */
+      retval = ((LONGEST) * p ^ 0x80) - 0x80;
+      for (--p; p >= startaddr; --p)
+	retval = (retval << 8) | *p;
+    }
+  return retval;
+}
+
+/* See gdb-byteswap.h.  */
+
+ULONGEST
+extract_unsigned_integer (const gdb_byte *addr, int len,
+			  enum bfd_endian byte_order)
+{
+  ULONGEST retval;
+  const unsigned char *p;
+  const unsigned char *startaddr = addr;
+  const unsigned char *endaddr = startaddr + len;
+
+  if (len > (int) sizeof (ULONGEST))
+    error (_("\
+That operation is not available on integers of more than %d bytes."),
+	   (int) sizeof (ULONGEST));
+
+  /* Start at the most significant end of the integer, and work towards
+     the least significant.  */
+  retval = 0;
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = startaddr; p < endaddr; ++p)
+	retval = (retval << 8) | *p;
+    }
+  else
+    {
+      for (p = endaddr - 1; p >= startaddr; --p)
+	retval = (retval << 8) | *p;
+    }
+  return retval;
+}
+
+/* See gdb_byteswap.h.  */
+
+int
+extract_long_unsigned_integer (const gdb_byte *addr, int orig_len,
+			       enum bfd_endian byte_order, LONGEST *pval)
+{
+  const gdb_byte *p;
+  const gdb_byte *first_addr;
+  int len;
+
+  len = orig_len;
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = addr;
+	   len > (int) sizeof (LONGEST) && p < addr + orig_len;
+	   p++)
+	{
+	  if (*p == 0)
+	    len--;
+	  else
+	    break;
+	}
+      first_addr = p;
+    }
+  else
+    {
+      first_addr = addr;
+      for (p = addr + orig_len - 1;
+	   len > (int) sizeof (LONGEST) && p >= addr;
+	   p--)
+	{
+	  if (*p == 0)
+	    len--;
+	  else
+	    break;
+	}
+    }
+
+  if (len <= (int) sizeof (LONGEST))
+    {
+      *pval = (LONGEST) extract_unsigned_integer (first_addr,
+						  sizeof (LONGEST),
+						  byte_order);
+      return 1;
+    }
+
+  return 0;
+}
+
+
+/* See gdb-byteswap.h.  */
+
+void
+store_signed_integer (gdb_byte *addr, int len,
+		      enum bfd_endian byte_order, LONGEST val)
+{
+  gdb_byte *p;
+  gdb_byte *startaddr = addr;
+  gdb_byte *endaddr = startaddr + len;
+
+  /* Start at the least significant end of the integer, and work towards
+     the most significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = endaddr - 1; p >= startaddr; --p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+  else
+    {
+      for (p = startaddr; p < endaddr; ++p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+}
+
+/* See gdb-byteswap.h.  */
+
+void
+store_unsigned_integer (gdb_byte *addr, int len,
+			enum bfd_endian byte_order, ULONGEST val)
+{
+  unsigned char *p;
+  unsigned char *startaddr = (unsigned char *) addr;
+  unsigned char *endaddr = startaddr + len;
+
+  /* Start at the least significant end of the integer, and work towards
+     the most significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = endaddr - 1; p >= startaddr; --p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+  else
+    {
+      for (p = startaddr; p < endaddr; ++p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+}
diff --git a/gdb/common/gdb-byteswap.h b/gdb/common/gdb-byteswap.h
new file mode 100644
index 0000000..9cf1d22
--- /dev/null
+++ b/gdb/common/gdb-byteswap.h
@@ -0,0 +1,49 @@
+/* Basic byte-swapping routines, for GDB, the GNU debugger.
+
+   Copyright (C) 2009-2016 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/>.  */
+
+#ifndef GDB_BYTESWAP_H
+#define GDB_BYTESWAP_H 1
+
+/* All 'extract' functions return a host-format integer from a
+   target-format integer at ADDR which is LEN bytes long.  */
+
+extern LONGEST extract_signed_integer (const gdb_byte *, int,
+				       enum bfd_endian);
+
+extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
+					  enum bfd_endian);
+
+/* Sometimes a long long unsigned integer can be extracted as a
+   LONGEST value.  This is done so that we can print these values
+   better.  If this integer can be converted to a LONGEST, this
+   function returns 1 and sets *PVAL.  Otherwise it returns 0.  */
+
+extern int extract_long_unsigned_integer (const gdb_byte *, int,
+					  enum bfd_endian, LONGEST *);
+
+/* All 'store' functions accept a host-format integer and store a
+   target-format integer at ADDR which is LEN bytes long.  */
+
+extern void store_signed_integer (gdb_byte *, int,
+				  enum bfd_endian, LONGEST);
+
+extern void store_unsigned_integer (gdb_byte *, int,
+				    enum bfd_endian, ULONGEST);
+
+#endif
diff --git a/gdb/defs.h b/gdb/defs.h
index f6ffeac..1e7fd10 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -594,26 +594,13 @@ extern double atof (const char *);	/* X3.159-1989  4.10.1.1 */
 
 enum { MAX_REGISTER_SIZE = 64 };
 
-/* In findvar.c.  */
-
-extern LONGEST extract_signed_integer (const gdb_byte *, int,
-				       enum bfd_endian);
-
-extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
-					  enum bfd_endian);
+#include "gdb-byteswap.h"
 
-extern int extract_long_unsigned_integer (const gdb_byte *, int,
-					  enum bfd_endian, LONGEST *);
+/* In findvar.c.  */
 
 extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
 					struct type *type);
 
-extern void store_signed_integer (gdb_byte *, int,
-				  enum bfd_endian, LONGEST);
-
-extern void store_unsigned_integer (gdb_byte *, int,
-				    enum bfd_endian, ULONGEST);
-
 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);
 
diff --git a/gdb/findvar.c b/gdb/findvar.c
index a39d897..70b9249 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -34,136 +34,6 @@
 #include "language.h"
 #include "dwarf2loc.h"
 
-/* Basic byte-swapping routines.  All 'extract' functions return a
-   host-format integer from a target-format integer at ADDR which is
-   LEN bytes long.  */
-
-#if TARGET_CHAR_BIT != 8 || HOST_CHAR_BIT != 8
-  /* 8 bit characters are a pretty safe assumption these days, so we
-     assume it throughout all these swapping routines.  If we had to deal with
-     9 bit characters, we would need to make len be in bits and would have
-     to re-write these routines...  */
-you lose
-#endif
-
-LONGEST
-extract_signed_integer (const gdb_byte *addr, int len,
-			enum bfd_endian byte_order)
-{
-  LONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (LONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (LONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      p = startaddr;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (++p; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      p = endaddr - 1;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (--p; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}
-
-ULONGEST
-extract_unsigned_integer (const gdb_byte *addr, int len,
-			  enum bfd_endian byte_order)
-{
-  ULONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (ULONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (ULONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  retval = 0;
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}
-
-/* Sometimes a long long unsigned integer can be extracted as a
-   LONGEST value.  This is done so that we can print these values
-   better.  If this integer can be converted to a LONGEST, this
-   function returns 1 and sets *PVAL.  Otherwise it returns 0.  */
-
-int
-extract_long_unsigned_integer (const gdb_byte *addr, int orig_len,
-			       enum bfd_endian byte_order, LONGEST *pval)
-{
-  const gdb_byte *p;
-  const gdb_byte *first_addr;
-  int len;
-
-  len = orig_len;
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = addr;
-	   len > (int) sizeof (LONGEST) && p < addr + orig_len;
-	   p++)
-	{
-	  if (*p == 0)
-	    len--;
-	  else
-	    break;
-	}
-      first_addr = p;
-    }
-  else
-    {
-      first_addr = addr;
-      for (p = addr + orig_len - 1;
-	   len > (int) sizeof (LONGEST) && p >= addr;
-	   p--)
-	{
-	  if (*p == 0)
-	    len--;
-	  else
-	    break;
-	}
-    }
-
-  if (len <= (int) sizeof (LONGEST))
-    {
-      *pval = (LONGEST) extract_unsigned_integer (first_addr,
-						  sizeof (LONGEST),
-						  byte_order);
-      return 1;
-    }
-
-  return 0;
-}
-
-
 /* Treat the bytes at BUF as a pointer of type TYPE, and return the
    address it represents.  */
 CORE_ADDR
@@ -178,65 +48,6 @@ extract_typed_address (const gdb_byte *buf, struct type *type)
   return gdbarch_pointer_to_address (get_type_arch (type), type, buf);
 }
 
-/* All 'store' functions accept a host-format integer and store a
-   target-format integer at ADDR which is LEN bytes long.  */
-
-void
-store_signed_integer (gdb_byte *addr, int len,
-		      enum bfd_endian byte_order, LONGEST val)
-{
-  gdb_byte *p;
-  gdb_byte *startaddr = addr;
-  gdb_byte *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
-void
-store_unsigned_integer (gdb_byte *addr, int len,
-			enum bfd_endian byte_order, ULONGEST val)
-{
-  unsigned char *p;
-  unsigned char *startaddr = (unsigned char *) addr;
-  unsigned char *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
 /* Store the address ADDR as a pointer of type TYPE at BUF, in target
    form.  */
 void
-- 
1.9.3


[-- Attachment #4: 0003-Fix-gdbserver-s-regcache_raw_read_unsigned-on-big-en.patch --]
[-- Type: text/x-patch, Size: 3963 bytes --]

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(-)

diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 1e874e3..06a6f1b 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -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
 
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 2af8e24..f875b10 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 12:46           ` Pedro Alves
@ 2016-02-11 15:15             ` Yao Qi
  2016-02-11 15:51               ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-11 15:15 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches

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.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 15:15             ` Yao Qi
@ 2016-02-11 15:51               ` Pedro Alves
  2016-02-11 16:32                 ` Antoine Tremblay
  2016-02-11 17:00                 ` Yao Qi
  0 siblings, 2 replies; 13+ messages in thread
From: Pedro Alves @ 2016-02-11 15:51 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 15:51               ` Pedro Alves
@ 2016-02-11 16:32                 ` Antoine Tremblay
  2016-02-11 17:00                 ` Yao Qi
  1 sibling, 0 replies; 13+ messages in thread
From: Antoine Tremblay @ 2016-02-11 16:32 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 15:51               ` Pedro Alves
  2016-02-11 16:32                 ` Antoine Tremblay
@ 2016-02-11 17:00                 ` Yao Qi
  2016-02-11 17:24                   ` Pedro Alves
  1 sibling, 1 reply; 13+ messages in thread
From: Yao Qi @ 2016-02-11 17:00 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 4590 bytes --]

Hi Pedro,

On 11/02/16 15:51, Pedro Alves wrote:
> 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?
>

No, I am not against code sharing between GDB and GDBserver in general,
but sharing extract_unsigned_integer is not necessary for GDBserver.

>> >
>>> >>
>>> >>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:

IMO, it was a design issue to have endianness in get-next-pcs stuff,
because endianess is only used by GDB, so we shouldn't bring endianess
into get-next-pcs at all.  I pointed it out during the review
https://sourceware.org/ml/gdb-patches/2015-12/msg00040.html but I
didn't insist on this, because I didn't want this block the whole patch
set and I can fix this issue later.

In fact, I've already had a patch to remove byte_order and
byte_oder_for_code from get-next-pcs, but have no change to post it out
due to the recent bug fixing for 7.11 release.

>
> 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,
>
>

All of them can be removed by the patch below,

-- 
Yao (é½å°§)

[-- Attachment #2: 0001-Remove-fields-byte_order-and-byte_order_for_code-fro.patch --]
[-- Type: text/x-patch, Size: 21058 bytes --]

From e1ea49a05e595a35973ed390d4ae9f6e8f1ed226 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Wed, 13 Jan 2016 14:49:56 +0000
Subject: [PATCH] Remove fields byte_order and byte_order_for_code from struct
 arm_get_next_pcs

We have fields byte_order and byte_order_for_code in
"struct arm_get_next_pcs", but we don't use them at all in GDBserver.
This patch adds a sub-class of arm_get_next_pcs for GDB and move fields
byte_order and byte_order_for_code there.  In order to differentiate
reading from data and instruction (due to the different endianess of
data and instruction), field read_mem_uint is split to two,
read_code_uint and read_data_uint.

gdb:

2016-01-13  Yao Qi  <yao.qi@linaro.org>

	* arch/arm-get-next-pcs.c (arm_get_next_pcs_ctor): Remove
	arguments byte_order and byte_order_for_code.  Remove code
	setting self->byte_order and self->byte_order_for_code.
	Callers updated.
	(thumb_deal_with_atomic_sequence_raw): Remove local variable
	'byte_order_for_code'.  Replace function pointer
	read_mem_uint with read_code_uint and read_data_unint.
	(arm_deal_with_atomic_sequence_raw): Likewise.
	(thumb_get_next_pcs_raw): Likewise.
	(arm_get_next_pcs_raw): Likewise.
	* arch/arm-get-next-pcs.h (struct arm_get_next_pcs_ops)
	<read_mem_uint>: Remove.
	<read_code_uint>, <read_data_uint>: New fields.
	(struct arm_get_next_pcs) <byte_order>: New field.
	<byte_order_for_code>: New field.
	(arm_get_next_pcs_ctor): Update declaration.
	* arm-linux-tdep.c (arm_linux_get_next_pcs_ops): Update
	initialization.
	(arm_linux_software_single_step): Use struct arm_gdb_get_next_pcs
	and set byte_order and byte_order_for_code.
	* arm-tdep.c (arm_get_next_pcs_ops): Update
	initialization.
	(arm_get_next_pcs_read_memory_unsigned_integer): Remove.
	(arm_get_next_pcs_read_code_unsigned_integer): New function.
	(arm_get_next_pcs_read_data_unsigned_integer): New function.
	(arm_software_single_step): Use struct arm_gdb_get_next_pcs and
	set byte_order and byte_order_for_code.
	* arm-tdep.h (struct get_next_pcs): Remove declaration.
	(struct arm_get_next_pcs, struct gdb_get_next_pcs): Likewise.
	Include arch/arm-get-next-pcs.h.
	(arm_get_next_pcs_read_memory_unsigned_integer): Remove declaration.
	(struct arm_gdb_get_next_pcs): New.
	(arm_get_next_pcs_read_code_unsigned_integer): Declare.
	(arm_get_next_pcs_read_data_unsigned_integer): Likewise.

gdb/gdbserver:

2016-01-13  Yao Qi  <yao.qi@linaro.org>

	* linux-arm-low.c (get_next_pcs_read_memory_unsigned_integer):
	Update declaration.
	(get_next_pcs_ops): Update initialization.
	(get_next_pcs_read_memory_unsigned_integer): Remove argument byte_order.
	Add argument self.
---
 gdb/arch/arm-get-next-pcs.c   | 57 +++++++++++++++++++------------------------
 gdb/arch/arm-get-next-pcs.h   | 11 +++------
 gdb/arm-linux-tdep.c          | 13 +++++-----
 gdb/arm-tdep.c                | 34 +++++++++++++++++---------
 gdb/arm-tdep.h                | 24 +++++++++++++-----
 gdb/gdbserver/linux-arm-low.c | 21 +++++++---------
 6 files changed, 85 insertions(+), 75 deletions(-)

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index e840147..be640b6 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -28,14 +28,10 @@
 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)
 {
   self->ops = ops;
-  self->byte_order = byte_order;
-  self->byte_order_for_code = byte_order_for_code;
   self->has_thumb2_breakpoint = has_thumb2_breakpoint;
   self->regcache = regcache;
 }
@@ -48,7 +44,6 @@ arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
 static VEC (CORE_ADDR) *
 thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
 {
-  int byte_order_for_code = self->byte_order_for_code;
   CORE_ADDR breaks[2] = {-1, -1};
   CORE_ADDR pc = regcache_read_pc (self->regcache);
   CORE_ADDR loc = pc;
@@ -67,13 +62,13 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
     return NULL;
 
   /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction.  */
-  insn1 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
+  insn1 = self->ops->read_code_uint (self, loc, 2);
 
   loc += 2;
   if (thumb_insn_size (insn1) != 4)
     return NULL;
 
-  insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
+  insn2 = self->ops->read_code_uint (self, loc, 2);
 
   loc += 2;
   if (!((insn1 & 0xfff0) == 0xe850
@@ -84,7 +79,7 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
      instructions.  */
   for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
     {
-      insn1 = self->ops->read_mem_uint (loc, 2,byte_order_for_code);
+      insn1 = self->ops->read_code_uint (self, loc, 2);
       loc += 2;
 
       if (thumb_insn_size (insn1) != 4)
@@ -111,7 +106,7 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
 	}
       else
 	{
-	  insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
+	  insn2 = self->ops->read_code_uint (self, loc, 2);
 
 	  loc += 2;
 
@@ -185,7 +180,6 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
 static VEC (CORE_ADDR) *
 arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
 {
-  int byte_order_for_code = self->byte_order_for_code;
   CORE_ADDR breaks[2] = {-1, -1};
   CORE_ADDR pc = regcache_read_pc (self->regcache);
   CORE_ADDR loc = pc;
@@ -199,7 +193,7 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
   /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction.
      Note that we do not currently support conditionally executed atomic
      instructions.  */
-  insn = self->ops->read_mem_uint (loc, 4, byte_order_for_code);
+  insn = self->ops->read_code_uint (self, loc, 4);
 
   loc += 4;
   if ((insn & 0xff9000f0) != 0xe1900090)
@@ -209,7 +203,7 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
      instructions.  */
   for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
     {
-      insn = self->ops->read_mem_uint (loc, 4, byte_order_for_code);
+      insn = self->ops->read_code_uint (self, loc, 4);
 
       loc += 4;
 
@@ -263,8 +257,6 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
 static VEC (CORE_ADDR) *
 thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 {
-  int byte_order = self->byte_order;
-  int byte_order_for_code = self->byte_order_for_code;
   CORE_ADDR pc = regcache_read_pc (self->regcache);
   unsigned long pc_val = ((unsigned long) pc) + 4;	/* PC after prefetch */
   unsigned short inst1;
@@ -277,7 +269,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
   nextpc = MAKE_THUMB_ADDR (nextpc);
   pc_val = MAKE_THUMB_ADDR (pc_val);
 
-  inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
+  inst1 = self->ops->read_code_uint (self, pc, 2);
 
   /* Thumb-2 conditional execution support.  There are eight bits in
      the CPSR which describe conditional execution state.  Once
@@ -310,7 +302,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
 	  while (itstate != 0 && ! condition_true (itstate >> 4, status))
 	    {
-	      inst1 = self->ops->read_mem_uint (pc, 2,byte_order_for_code);
+	      inst1 = self->ops->read_code_uint (self, pc, 2);
 	      pc += thumb_insn_size (inst1);
 	      itstate = thumb_advance_itstate (itstate);
 	    }
@@ -329,7 +321,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
 	      while (itstate != 0 && ! condition_true (itstate >> 4, status))
 		{
-		  inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
+		  inst1 = self->ops->read_code_uint (self, pc, 2);
 
 		  pc += thumb_insn_size (inst1);
 		  itstate = thumb_advance_itstate (itstate);
@@ -372,7 +364,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		 the instruction after the IT block.  */
 	      do
 		{
-		  inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
+		  inst1 = self->ops->read_code_uint (self, pc, 2);
 		  pc += thumb_insn_size (inst1);
 		  itstate = thumb_advance_itstate (itstate);
 		}
@@ -410,7 +402,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
          all of the other registers.  */
       offset = bitcount (bits (inst1, 0, 7)) * INT_REGISTER_SIZE;
       sp = regcache_raw_get_unsigned (regcache, ARM_SP_REGNUM);
-      nextpc = self->ops->read_mem_uint (sp + offset, 4, byte_order);
+      nextpc = self->ops->read_data_uint (self, sp + offset, 4);
     }
   else if ((inst1 & 0xf000) == 0xd000)	/* conditional branch */
     {
@@ -429,7 +421,8 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
   else if (thumb_insn_size (inst1) == 4) /* 32-bit instruction */
     {
       unsigned short inst2;
-      inst2 = self->ops->read_mem_uint (pc + 2, 2, byte_order_for_code);
+
+      inst2 = self->ops->read_code_uint (self, pc + 2, 2);
 
       /* Default to the next instruction.  */
       nextpc = pc + 4;
@@ -519,7 +512,8 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	  if (load_pc)
 	    {
 	      CORE_ADDR addr = regcache_raw_get_unsigned (regcache, rn);
-	      nextpc = self->ops->read_mem_uint	(addr + offset, 4, byte_order);
+
+	      nextpc = self->ops->read_data_uint (self, addr + offset, 4);
 	    }
 	}
       else if ((inst1 & 0xffef) == 0xea4f && (inst2 & 0xfff0) == 0x0f00)
@@ -566,8 +560,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	    load_pc = 0;
 
 	  if (load_pc)
-	    nextpc
-	      = self->ops->read_mem_uint (base, 4, byte_order);
+	    nextpc = self->ops->read_data_uint (self, base, 4);
 	}
       else if ((inst1 & 0xfff0) == 0xe8d0 && (inst2 & 0xfff0) == 0xf000)
 	{
@@ -581,7 +574,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	    table = regcache_raw_get_unsigned (regcache, tbl_reg);
 
 	  offset = regcache_raw_get_unsigned (regcache, bits (inst2, 0, 3));
-	  length = 2 * self->ops->read_mem_uint (table + offset, 1, byte_order);
+	  length = 2 * self->ops->read_data_uint (self, table + offset, 1);
 	  nextpc = pc_val + length;
 	}
       else if ((inst1 & 0xfff0) == 0xe8d0 && (inst2 & 0xfff0) == 0xf010)
@@ -596,7 +589,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	    table = regcache_raw_get_unsigned (regcache, tbl_reg);
 
 	  offset = 2 * regcache_raw_get_unsigned (regcache, bits (inst2, 0, 3));
-	  length = 2 * self->ops->read_mem_uint (table + offset, 2, byte_order);
+	  length = 2 * self->ops->read_data_uint (self, table + offset, 2);
 	  nextpc = pc_val + length;
 	}
     }
@@ -644,8 +637,6 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 static VEC (CORE_ADDR) *
 arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
 {
-  int byte_order = self->byte_order;
-  int byte_order_for_code = self->byte_order_for_code;
   unsigned long pc_val;
   unsigned long this_instr = 0;
   unsigned long status;
@@ -655,7 +646,7 @@ arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
   VEC (CORE_ADDR) *next_pcs = NULL;
 
   pc_val = (unsigned long) pc;
-  this_instr = self->ops->read_mem_uint (pc, 4, byte_order_for_code);
+  this_instr = self->ops->read_code_uint (self, pc, 4);
 
   status = regcache_raw_get_unsigned (regcache, ARM_PS_REGNUM);
   nextpc = (CORE_ADDR) (pc_val + 4);	/* Default case */
@@ -838,8 +829,9 @@ arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
 			base -= offset;
 		    }
 		  nextpc
-		    = (CORE_ADDR) self->ops->read_mem_uint ((CORE_ADDR) base,
-							    4, byte_order);
+		    = (CORE_ADDR) self->ops->read_data_uint (self,
+							     (CORE_ADDR) base,
+							     4);
 		}
 	    }
 	  break;
@@ -870,8 +862,9 @@ arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		    offset = -4;
 
 		  rn_val_offset = rn_val + offset;
-		  nextpc = (CORE_ADDR) self->ops->read_mem_uint (rn_val_offset,
-								 4, byte_order);
+		  nextpc = (CORE_ADDR) self->ops->read_data_uint (self,
+								  rn_val_offset,
+								 4);
 		}
 	    }
 	  break;
diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
index 7cb0858..daa674b 100644
--- a/gdb/arch/arm-get-next-pcs.h
+++ b/gdb/arch/arm-get-next-pcs.h
@@ -26,7 +26,10 @@ struct arm_get_next_pcs;
 /* get_next_pcs operations.  */
 struct arm_get_next_pcs_ops
 {
-  ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order);
+  ULONGEST (*read_code_uint) (struct arm_get_next_pcs *self,
+			      CORE_ADDR memaddr, int len);
+  ULONGEST (*read_data_uint) (struct arm_get_next_pcs *self,
+			      CORE_ADDR memaddr, int len);
   CORE_ADDR (*syscall_next_pc) (struct arm_get_next_pcs *self, CORE_ADDR pc);
   CORE_ADDR (*addr_bits_remove) (struct arm_get_next_pcs *self, CORE_ADDR val);
   int (*is_thumb) (struct arm_get_next_pcs *self);
@@ -37,10 +40,6 @@ struct arm_get_next_pcs
 {
   /* Operations implementations.  */
   struct arm_get_next_pcs_ops *ops;
-  /* Byte order for data.  */
-  int byte_order;
-  /* Byte order for code.  */
-  int byte_order_for_code;
   /* Whether the target has 32-bit thumb-2 breakpoint defined or
      not.  */
   int has_thumb2_breakpoint;
@@ -51,8 +50,6 @@ struct arm_get_next_pcs
 /* Initialize arm_get_next_pcs.  */
 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);
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 3421f3b..f5a9a53 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -271,7 +271,8 @@ static CORE_ADDR
 
 /* Operation function pointers for get_next_pcs.  */
 static struct arm_get_next_pcs_ops arm_linux_get_next_pcs_ops = {
-  arm_get_next_pcs_read_memory_unsigned_integer,
+  arm_get_next_pcs_read_code_unsigned_integer,
+  arm_get_next_pcs_read_data_unsigned_integer,
   arm_linux_get_next_pcs_syscall_next_pc,
   arm_get_next_pcs_addr_bits_remove,
   arm_get_next_pcs_is_thumb
@@ -929,7 +930,7 @@ arm_linux_software_single_step (struct frame_info *frame)
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct address_space *aspace = get_regcache_aspace (regcache);
-  struct arm_get_next_pcs next_pcs_ctx;
+  struct arm_gdb_get_next_pcs next_pcs_ctx;
   CORE_ADDR pc;
   int i;
   VEC (CORE_ADDR) *next_pcs = NULL;
@@ -940,14 +941,14 @@ arm_linux_software_single_step (struct frame_info *frame)
   if (target_can_do_single_step () == 1)
     return 0;
 
-  arm_get_next_pcs_ctor (&next_pcs_ctx,
+  arm_get_next_pcs_ctor ((struct arm_get_next_pcs *) &next_pcs_ctx,
 			 &arm_linux_get_next_pcs_ops,
-			 gdbarch_byte_order (gdbarch),
-			 gdbarch_byte_order_for_code (gdbarch),
 			 1,
 			 regcache);
+  next_pcs_ctx.byte_order = gdbarch_byte_order (gdbarch);
+  next_pcs_ctx.byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
 
-  next_pcs = arm_get_next_pcs (&next_pcs_ctx);
+  next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs_ctx);
 
   for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
     {
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 6ac05f0..1b4c4c4 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -244,7 +244,8 @@ static CORE_ADDR
 
 /* get_next_pcs operations.  */
 static struct arm_get_next_pcs_ops arm_get_next_pcs_ops = {
-  arm_get_next_pcs_read_memory_unsigned_integer,
+  arm_get_next_pcs_read_code_unsigned_integer,
+  arm_get_next_pcs_read_data_unsigned_integer,
   arm_get_next_pcs_syscall_next_pc,
   arm_get_next_pcs_addr_bits_remove,
   arm_get_next_pcs_is_thumb
@@ -6125,15 +6126,24 @@ thumb2_copy_block_xfer (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2,
   return 0;
 }
 
-/* Wrapper over read_memory_unsigned_integer for use in arm_get_next_pcs.
- This is used to avoid a dependency on BFD's bfd_endian enum.  */
+/* Wrapper over read_memory_unsigned_integer for use in arm_get_next_pcs.  */
 
 ULONGEST
-arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, int len,
-					       int byte_order)
+arm_get_next_pcs_read_code_unsigned_integer (struct arm_get_next_pcs *self,
+					     CORE_ADDR memaddr, int len)
 {
-  return read_memory_unsigned_integer (memaddr, len,
-				       (enum bfd_endian) byte_order);
+  struct arm_gdb_get_next_pcs *gdb = (struct arm_gdb_get_next_pcs *) self;
+
+  return read_memory_unsigned_integer (memaddr, len, gdb->byte_order_for_code);
+}
+
+ULONGEST
+arm_get_next_pcs_read_data_unsigned_integer (struct arm_get_next_pcs *self,
+					     CORE_ADDR memaddr, int len)
+{
+  struct arm_gdb_get_next_pcs *gdb = (struct arm_gdb_get_next_pcs *) self;
+
+  return read_memory_unsigned_integer (memaddr, len, gdb->byte_order);
 }
 
 /* Wrapper over gdbarch_addr_bits_remove for use in arm_get_next_pcs.  */
@@ -6173,20 +6183,20 @@ arm_software_single_step (struct frame_info *frame)
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct address_space *aspace = get_regcache_aspace (regcache);
-  struct arm_get_next_pcs next_pcs_ctx;
+  struct arm_gdb_get_next_pcs next_pcs_ctx;
   CORE_ADDR pc;
   int i;
   VEC (CORE_ADDR) *next_pcs = NULL;
   struct cleanup *old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), &next_pcs);
 
-  arm_get_next_pcs_ctor (&next_pcs_ctx,
+  arm_get_next_pcs_ctor ((struct arm_get_next_pcs *) &next_pcs_ctx,
 			 &arm_get_next_pcs_ops,
-			 gdbarch_byte_order (gdbarch),
-			 gdbarch_byte_order_for_code (gdbarch),
 			 0,
 			 regcache);
+  next_pcs_ctx.byte_order = gdbarch_byte_order (gdbarch);
+  next_pcs_ctx.byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
 
-  next_pcs = arm_get_next_pcs (&next_pcs_ctx);
+  next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs_ctx);
 
   for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
     arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 1306cbb..41adffd 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -23,11 +23,9 @@
 struct gdbarch;
 struct regset;
 struct address_space;
-struct get_next_pcs;
-struct arm_get_next_pcs;
-struct gdb_get_next_pcs;
 
 #include "arch/arm.h"
+#include "arch/arm-get-next-pcs.h"
 
 /* Say how long FP registers are.  Used for documentation purposes and
    code readability in this header.  IEEE extended doubles are 80
@@ -250,9 +248,23 @@ extern void
 
 CORE_ADDR arm_skip_stub (struct frame_info *, CORE_ADDR);
 
-ULONGEST arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
-							int len,
-							int byte_order);
+struct arm_gdb_get_next_pcs
+{
+  struct arm_get_next_pcs base;
+
+  enum bfd_endian byte_order;
+  enum bfd_endian byte_order_for_code;
+};
+
+ULONGEST
+  arm_get_next_pcs_read_code_unsigned_integer (struct arm_get_next_pcs *self,
+					       CORE_ADDR memaddr,
+					       int len);
+
+ULONGEST
+  arm_get_next_pcs_read_data_unsigned_integer (struct arm_get_next_pcs *self,
+					       CORE_ADDR memaddr,
+					       int len);
 
 CORE_ADDR arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
 					     CORE_ADDR val);
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 0f62706..8ca7e6c 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -140,9 +140,10 @@ static int arm_regmap[] = {
 };
 
 /* Forward declarations needed for get_next_pcs ops.  */
-static ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
-							   int len,
-							   int byte_order);
+static ULONGEST
+  get_next_pcs_read_memory_unsigned_integer (struct arm_get_next_pcs *self,
+					     CORE_ADDR memaddr,
+					     int len);
 
 static CORE_ADDR get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
 						CORE_ADDR val);
@@ -155,6 +156,7 @@ static int get_next_pcs_is_thumb (struct arm_get_next_pcs *self);
 /* get_next_pcs operations.  */
 static struct arm_get_next_pcs_ops get_next_pcs_ops = {
   get_next_pcs_read_memory_unsigned_integer,
+  get_next_pcs_read_memory_unsigned_integer,
   get_next_pcs_syscall_next_pc,
   get_next_pcs_addr_bits_remove,
   get_next_pcs_is_thumb
@@ -252,13 +254,11 @@ get_next_pcs_is_thumb (struct arm_get_next_pcs *self)
   return arm_is_thumb_mode ();
 }
 
-/* Read memory from the inferiror.
-   BYTE_ORDER is ignored and there to keep compatiblity with GDB's
-   read_memory_unsigned_integer. */
+/* Read memory from the inferior.  */
 static ULONGEST
-get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
-					   int len,
-					   int byte_order)
+get_next_pcs_read_memory_unsigned_integer (struct arm_get_next_pcs *self,
+					   CORE_ADDR memaddr,
+					   int len)
 {
   ULONGEST res;
 
@@ -930,9 +930,6 @@ arm_gdbserver_get_next_pcs (struct regcache *regcache)
 
   arm_get_next_pcs_ctor (&next_pcs_ctx,
 			 &get_next_pcs_ops,
-			 /* Byte order is ignored assumed as host.  */
-			 0,
-			 0,
 			 1,
 			 regcache);
 
-- 
1.9.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 17:00                 ` Yao Qi
@ 2016-02-11 17:24                   ` Pedro Alves
  2016-02-12 11:15                     ` Yao Qi
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2016-02-11 17:24 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 02/11/2016 04:59 PM, Yao Qi wrote:
> Hi Pedro,
> 
> On 11/02/16 15:51, Pedro Alves wrote:
>> 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?
>>
> 
> No, I am not against code sharing between GDB and GDBserver in general,
> but sharing extract_unsigned_integer is not necessary for GDBserver.

It's not like like we'd be bringing in some big complicated piece of
code.  The routines in question are quite small.  It's about the
same code as the other solution with the switch.  Compare,

gdb's:

ULONGEST
extract_unsigned_integer (const gdb_byte *addr, int len,
                         enum bfd_endian byte_order)
{
  ULONGEST retval;
  const unsigned char *p;
  const unsigned char *startaddr = addr;
  const unsigned char *endaddr = startaddr + len;

  if (len > (int) sizeof (ULONGEST))
    error (_("\
That operation is not available on integers of more than %d bytes."),
          (int) sizeof (ULONGEST));

  /* Start at the most significant end of the integer, and work towards
     the least significant.  */
  retval = 0;
  if (byte_order == BFD_ENDIAN_BIG)
    {
      for (p = startaddr; p < endaddr; ++p)
       retval = (retval << 8) | *p;
    }
  else
    {
      for (p = endaddr - 1; p >= startaddr; --p)
       retval = (retval << 8) | *p;
    }
  return retval;
}

vs

gdbserver's, using a switch:

ULONGEST
extract_unsigned_integer (const gdb_byte *addr, int len)
{
  uint8_t u8;
  uint16_t u16;
  uint32_t u32;
  uint64_t u64;

  if (len > (int) sizeof (ULONGEST))
    error (_("That operation is not available on integers of more than"
            "%d bytes."),
          (int) sizeof (ULONGEST));

  switch (len)
  {
    case 1:
      memcpy (&u8, addr, len);
      return u8;
   case 2:
     memcpy (&u16, addr, len);
     return u16;
   case 4:
     memcpy (&u32, addr, len);
     return u32;
   case 8:
     memcpy (&u64, addr, len);
     return u64;
  }
}

I just don't see what the worry is.  OTOH, sharing the routines
would pave to way to share more, and have one less diverging detail
to worry about, avoiding proliferation of "bridging" interfaces like
regcache_raw_read_unsigned or the need to hook in more entry points
to get_next_pcs and whatever else we put in shared code areas.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Clear *VAL in regcache_raw_read_unsigned
  2016-02-11 17:24                   ` Pedro Alves
@ 2016-02-12 11:15                     ` Yao Qi
  0 siblings, 0 replies; 13+ messages in thread
From: Yao Qi @ 2016-02-12 11:15 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> I just don't see what the worry is.  OTOH, sharing the routines
> would pave to way to share more, and have one less diverging detail
> to worry about, avoiding proliferation of "bridging" interfaces like
> regcache_raw_read_unsigned or the need to hook in more entry points
> to get_next_pcs and whatever else we put in shared code areas.

I don't worry about the code sharing.  I am OK with your patches moving
bfd_endian to a separate header and fix the issue in
regcache_raw_read_unsigned with extract_unsigned_integer.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-02-12 11:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-09 14:54 [PATCH] Clear *VAL in regcache_raw_read_unsigned Yao Qi
2016-02-10 16:45 ` Yao Qi
2016-02-10 16:52   ` Pedro Alves
2016-02-10 17:25     ` Yao Qi
2016-02-10 17:36       ` Pedro Alves
2016-02-11 10:12         ` Yao Qi
2016-02-11 12:46           ` Pedro Alves
2016-02-11 15:15             ` Yao Qi
2016-02-11 15:51               ` Pedro Alves
2016-02-11 16:32                 ` Antoine Tremblay
2016-02-11 17:00                 ` Yao Qi
2016-02-11 17:24                   ` Pedro Alves
2016-02-12 11:15                     ` Yao Qi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).