Re: [PATCH 2/7] Move some integer operations to common.

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

On 09/11/2015 01:16 PM, Antoine Tremblay wrote:
>
>
> On 09/11/2015 10:24 AM, Gary Benson wrote:
>> Hi Antoine,
>>
>> Please don't introduce "#ifdef GDBSERVER" conditionals into common
>> code, I spent some time removing them.  I know I didn't get them
>> all, but the remaining two are on my hit list.
>>
>
> Humm what is the issue that makes this a bad idea if I may ?
>
>
>> To work around this... do you really need to deal with endianness in
>> gdbserver?  It's running on the target so if you're pulling numbers
>> from target memory in target endianness then could you use or
>> generalize target_read_uint32 for your needs?
>>
>
> Yes unfortunately I do need the endianness since the program can be in a
> different endianness then GDBServer the code section can even be
> different than the data section...
>
> I could however :
>
>   - Remove int-utils.h from common-defs.h and move to defs.h for GDB
>   - Move the int-utils.h in server.h for GDBServer which is kinda the
> GDBServer's equivalent of defs.h with the BFD defines before the include...
>
> Does that sound ok ?
>

Actually this doesn't work since there's an int-utils.c that needs to 
include again either bfh.h or define the enum with the same #ifdef 
GSBSERVER ... it would just be moved to the .c ...

I would need to move the code to a .h only...

At this point I need to understand more the argument about the #ifndef 
GDBSERVER...?

Or any other suggestions are welcome...

>> Antoine Tremblay wrote:
>>> This patch is in preparation for sharing code between GDB and
>>> GDBServer to
>>> enable software single stepping on ARM aarch32-linux.
>>>
>>> It moves multiple functions related to extracting or storing a value
>>> based on
>>> its endianness from findvar.c in gdb to a new file called int-utils.c in
>>> common.
>>>
>>> Definitions of these functions are also moved to defs.h to
>>> common-defs.h.
>>>
>>> gdb/ChangeLog:
>>>     * Makefile.in: Add int-utils.o.
>>>     * common/common-defs.h: New functions defs from defs.h.
>>>     * common/int-utils.c: New file.
>>>     * common/int-utils.h: New file.
>>>     * defs.h:  Move functions defs to common-defs.h.
>>>     * findvar.c (extract_signed_integer): Move to int-utils.c.
>>>     (extract_unsigned_integer): Likewise.
>>>     (extract_long_unsigned_integer): Likewise.
>>>     (store_signed_integer): Likewise.
>>>     (store_unsigned_integer): Likewise.
>>> ---
>>>   gdb/Makefile.in          |   9 ++-
>>>   gdb/common/common-defs.h |   1 +
>>>   gdb/common/int-utils.c   | 199
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   gdb/common/int-utils.h   |  45 +++++++++++
>>>   gdb/defs.h               |  16 ----
>>>   gdb/findvar.c            | 176
>>> -----------------------------------------
>>>   6 files changed, 252 insertions(+), 194 deletions(-)
>>>   create mode 100644 gdb/common/int-utils.c
>>>   create mode 100644 gdb/common/int-utils.h
>>>
>>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>>> index 0d7cf97..e20c5a6 100644
>>> --- a/gdb/Makefile.in
>>> +++ b/gdb/Makefile.in
>>> @@ -854,6 +854,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c
>>> ada-valprint.c ada-tasks.c \
>>>       infcall.c \
>>>       infcmd.c inflow.c infrun.c \
>>>       inline-frame.c \
>>> +    common/int-utils.c \
>>>       interps.c \
>>>       jv-exp.y jv-lang.c jv-valprint.c jv-typeprint.c jv-varobj.c \
>>>       language.c linespec.c location.c minidebug.c \
>>> @@ -985,7 +986,7 @@ i386-linux-nat.h common/common-defs.h
>>> common/errors.h common/common-types.h \
>>>   common/common-debug.h common/cleanups.h common/gdb_setjmp.h \
>>>   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 \
>>> +common/fileio.h nat/x86-linux.h nat/x86-linux-dregs.h
>>> common/int-utils.h \
>>>   nat/linux-namespaces.h arch/arm.h common/gdb_sys_time.h
>>>
>>>   # Header files that already have srcdir in them, or which are in
>>> objdir.
>>> @@ -1084,7 +1085,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>>>       common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
>>>       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-exceptions.o btrace-common.o fileio.o int-utils.o \
>>>       $(SUBDIR_GCC_COMPILE_OBS)
>>>
>>>   TSOBS = inflow.o
>>> @@ -2265,6 +2266,10 @@ btrace-common.o: ${srcdir}/common/btrace-common.c
>>>   fileio.o: ${srcdir}/common/fileio.c
>>>       $(COMPILE) $(srcdir)/common/fileio.c
>>>       $(POSTCOMPILE)
>>> +int-utils.o: ${srcdir}/common/int-utils.c
>>> +    $(COMPILE) $(srcdir)/common/int-utils.c
>>> +    $(POSTCOMPILE)
>>> +
>>>   #
>>>   # gdb/target/ dependencies
>>>   #
>>> diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
>>> index 2be0d7d..cb79234 100644
>>> --- a/gdb/common/common-defs.h
>>> +++ b/gdb/common/common-defs.h
>>> @@ -49,6 +49,7 @@
>>>   #include "common-debug.h"
>>>   #include "cleanups.h"
>>>   #include "common-exceptions.h"
>>> +#include "int-utils.h"
>>>
>>>   #ifdef __cplusplus
>>>   # define EXTERN_C extern "C"
>>> diff --git a/gdb/common/int-utils.c b/gdb/common/int-utils.c
>>> new file mode 100644
>>> index 0000000..57d0dba
>>> --- /dev/null
>>> +++ b/gdb/common/int-utils.c
>>> @@ -0,0 +1,199 @@
>>> +/* Shared utility routines for integer endianness manipulations.
>>> +   Copyright (C) 2015 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"
>>> +
>>> +/* Basic byte-swapping routines.  All 'extract' functions return a
>>> +   host-format integer from a target-format integer at ADDR which is
>>> +   LEN bytes long.  */
>>> +
>>> +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;
>>> +}
>>> +
>>> +/* 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;
>>> +    }
>>> +    }
>>> +}
>>> diff --git a/gdb/common/int-utils.h b/gdb/common/int-utils.h
>>> new file mode 100644
>>> index 0000000..c170348
>>> --- /dev/null
>>> +++ b/gdb/common/int-utils.h
>>> @@ -0,0 +1,45 @@
>>> +/* Shared utility routines for integer endianness manipulations.
>>> +   Copyright (C) 2015 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 INT_UTILS_H
>>> +#define INT_UTILS_H 1
>>> +
>>> +#ifdef GDBSERVER
>>> +/* Allow this enum without requiring bfd in gdbserver.  */
>>> +enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE,
>>> BFD_ENDIAN_UNKNOWN };
>>> +#else
>>> +#include "bfd.h"
>>> +#endif
>>> +
>>> +extern LONGEST extract_signed_integer (const gdb_byte *, int,
>>> +                       enum bfd_endian);
>>> +
>>> +extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
>>> +                      enum bfd_endian);
>>> +
>>> +extern int extract_long_unsigned_integer (const gdb_byte *, int,
>>> +                      enum bfd_endian, LONGEST *);
>>> +
>>> +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 /* INT_UTILS_H */
>>> diff --git a/gdb/defs.h b/gdb/defs.h
>>> index 03f7e8a..e292977 100644
>>> --- a/gdb/defs.h
>>> +++ b/gdb/defs.h
>>> @@ -596,28 +596,12 @@ 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);
>>> -
>>> -extern int extract_long_unsigned_integer (const gdb_byte *, int,
>>> -                      enum bfd_endian, LONGEST *);
>>> -
>>>   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);
>>>
>>> -\f
>>>   /* From valops.c */
>>>
>>>   extern int watchdog;
>>> diff --git a/gdb/findvar.c b/gdb/findvar.c
>>> index 1c077f7..2299ca4 100644
>>> --- a/gdb/findvar.c
>>> +++ b/gdb/findvar.c
>>> @@ -46,124 +46,6 @@
>>>   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,64 +60,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.  */
>>> --
>>> 1.9.1
>>