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

[Gary Benson <gbenson@redhat.com>]

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.

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?

Thanks,
Gary

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

-- 
http://gbenson.net/