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

  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
  

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

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 ?

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

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

Antoine Tremblay wrote:
> On 09/11/2015 01:16 PM, Antoine Tremblay wrote:
> > On 09/11/2015 10:24 AM, Gary Benson wrote:
> > > 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 ?

The way the common code is built is currently kind of weird and ugly.
Even though the code is shared, there's still places you have to do
the same work twice, for example if you add a new .c or .h file to
common, for example, you have to add it to both GDB's and gdbserver's
Makefiles.  If you add stuff that needs configure checks, you have to
add those twice too.  Also we build gnulib twice.  Also, the way the
compiler is invoked means that you can accidentally #include GDB- or
gdbserver-specific headers in common code.  It's all very error-prone. 

For the future we'd like to move the common code into its own toplevel
directory with it's own Makefile, it's own ./configure, etc.  It would
be built once, to a libgdbcommon.a or something that GDB and gdbserver
would statically link.  We can't do that if gdb/{common,nat,target}
have conditional code.

Thanks,
Gary
  

On 09/14/2015 05:24 AM, Gary Benson wrote:
> Antoine Tremblay wrote:
>> On 09/11/2015 01:16 PM, Antoine Tremblay wrote:
>>> On 09/11/2015 10:24 AM, Gary Benson wrote:
>>>> 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 ?
>
> The way the common code is built is currently kind of weird and ugly.
> Even though the code is shared, there's still places you have to do
> the same work twice, for example if you add a new .c or .h file to
> common, for example, you have to add it to both GDB's and gdbserver's
> Makefiles.  If you add stuff that needs configure checks, you have to
> add those twice too.  Also we build gnulib twice.  Also, the way the
> compiler is invoked means that you can accidentally #include GDB- or
> gdbserver-specific headers in common code.  It's all very error-prone.
>
> For the future we'd like to move the common code into its own toplevel
> directory with it's own Makefile, it's own ./configure, etc.  It would
> be built once, to a libgdbcommon.a or something that GDB and gdbserver
> would statically link.  We can't do that if gdb/{common,nat,target}
> have conditional code.
>

Ok, thanks for clarifying this for me.

So if there were a libgdbcommon I would need to include bfd into it and 
make it a requirement of that lib so that both GDB and GDBServer can use it.

So I've made bfd.h a requirement of GDBServer, and when there will be a 
libgdbcommon we can have the whole lib as a requirement there.

See patch v2 in next mail...
  

Hi Antoine, Pedro,

Antoine Tremblay wrote:
> On 09/14/2015 05:24 AM, Gary Benson wrote:
> > Antoine Tremblay wrote:
> > > On 09/11/2015 01:16 PM, Antoine Tremblay wrote:
> > > > On 09/11/2015 10:24 AM, Gary Benson wrote:
> > > > > 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 ?
> > 
> > The way the common code is built is currently kind of weird and
> > ugly.  Even though the code is shared, there's still places you
> > have to do the same work twice, for example if you add a new .c or
> > .h file to common, for example, you have to add it to both GDB's
> > and gdbserver's Makefiles.  If you add stuff that needs configure
> > checks, you have to add those twice too.  Also we build gnulib
> > twice.  Also, the way the compiler is invoked means that you can
> > accidentally #include GDB- or gdbserver-specific headers in common
> > code.  It's all very error-prone.
> > 
> > For the future we'd like to move the common code into its own
> > toplevel directory with it's own Makefile, it's own ./configure,
> > etc.  It would be built once, to a libgdbcommon.a or something
> > that GDB and gdbserver would statically link.  We can't do that
> > if gdb/{common,nat,target} have conditional code.
> 
> Ok, thanks for clarifying this for me.
> 
> So if there were a libgdbcommon I would need to include bfd into it
> and make it a requirement of that lib so that both GDB and GDBServer
> can use it.
> 
> So I've made bfd.h a requirement of GDBServer, and when there will
> be a libgdbcommon we can have the whole lib as a requirement there.
> 
> See patch v2 in next mail...

I don't think this will be acceptable.  If I understand correctly,
gdbserver supports some platforms that GDB (and BFD) does not, and
this patch would prevent gdbserver being built on those platforms.
Even if I'm wrong here, I've previously found it useful to build
gdbserver alone, and I think this would break that too.

Pedro knows more about these kinds of setups, I've copied him in.

Thanks,
Gary
  

On 09/21/2015 10:10 AM, Gary Benson wrote:
> Hi Antoine, Pedro,
> 
> Antoine Tremblay wrote:

>> So I've made bfd.h a requirement of GDBServer, and when there will
>> be a libgdbcommon we can have the whole lib as a requirement there.
>>
>> See patch v2 in next mail...
> 
> I don't think this will be acceptable.  If I understand correctly,
> gdbserver supports some platforms that GDB (and BFD) does not, and
> this patch would prevent gdbserver being built on those platforms.
> Even if I'm wrong here, I've previously found it useful to build
> gdbserver alone, and I think this would break that too.
> 
> Pedro knows more about these kinds of setups, I've copied him in.
> 

(Without looking at the patch in detail),

Gary's right.  bfd.h is a generated file, generated at bfd build time.
Anton, try building only gdbserver in a clean directory,
separate from gdb, and it will fail with your patch.

Thanks,
Pedro Alves
  

On 09/21/2015 05:16 AM, Pedro Alves wrote:
> On 09/21/2015 10:10 AM, Gary Benson wrote:
>> Hi Antoine, Pedro,
>>
>> Antoine Tremblay wrote:
>
>>> So I've made bfd.h a requirement of GDBServer, and when there will
>>> be a libgdbcommon we can have the whole lib as a requirement there.
>>>
>>> See patch v2 in next mail...
>>
>> I don't think this will be acceptable.  If I understand correctly,
>> gdbserver supports some platforms that GDB (and BFD) does not, and
>> this patch would prevent gdbserver being built on those platforms.
>> Even if I'm wrong here, I've previously found it useful to build
>> gdbserver alone, and I think this would break that too.
>>
>> Pedro knows more about these kinds of setups, I've copied him in.
>>
>
> (Without looking at the patch in detail),
>
> Gary's right.  bfd.h is a generated file, generated at bfd build time.
> Anton, try building only gdbserver in a clean directory,
> separate from gdb, and it will fail with your patch.
>
Ok I was worried this would not work..

I've removed much of the endianness dependencies from my patchset but I 
still have a dependency on the bfd endiannness enum to share code with 
GDB's read_memory_unsigned_integer.

So I will do a wrapper around read_memory_unsigned_integer in GDB that 
takes an int and transfers it to the real read_memory_unsigned_integer 
as the proper enum (by implicit conversion). And use an int when 
referring to the enum in shared code.

Unless there's an objection to this method ?
  

On Mon, Sep 21, 2015 at 10:49 AM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
>
>
> On 09/21/2015 05:16 AM, Pedro Alves wrote:
>>
>> On 09/21/2015 10:10 AM, Gary Benson wrote:
>>>
>>> Hi Antoine, Pedro,
>>>
>>> Antoine Tremblay wrote:
>>
>>
>>>> So I've made bfd.h a requirement of GDBServer, and when there will
>>>> be a libgdbcommon we can have the whole lib as a requirement there.
>>>>
>>>> See patch v2 in next mail...
>>>
>>>
>>> I don't think this will be acceptable.  If I understand correctly,
>>> gdbserver supports some platforms that GDB (and BFD) does not, and
>>> this patch would prevent gdbserver being built on those platforms.
>>> Even if I'm wrong here, I've previously found it useful to build
>>> gdbserver alone, and I think this would break that too.
>>>
>>> Pedro knows more about these kinds of setups, I've copied him in.
>>>
>>
>> (Without looking at the patch in detail),
>>
>> Gary's right.  bfd.h is a generated file, generated at bfd build time.
>> Anton, try building only gdbserver in a clean directory,
>> separate from gdb, and it will fail with your patch.
>>
> Ok I was worried this would not work..
>
> I've removed much of the endianness dependencies from my patchset but I
> still have a dependency on the bfd endiannness enum to share code with GDB's
> read_memory_unsigned_integer.
>
> So I will do a wrapper around read_memory_unsigned_integer in GDB that takes
> an int and transfers it to the real read_memory_unsigned_integer as the
> proper enum (by implicit conversion). And use an int when referring to the
> enum in shared code.
>
> Unless there's an objection to this method ?

I'd rather not discard the enum.

The first question I have is: where do we want to be in the long term?
I totally support moving more and more application independent code
into application independent places.
It's really a shame that something as simple as this is getting in the way.

[Ideally, I'd also like to remove bfd dependencies wherever possible,
but that can be a bit problematic. So I'm setting aside this
possibility. E.g., using an enum without bfd in the name.]

The next question I have is: Is there anything in what we need that
needs to be in a generated header?
Can we ask the bfd folks to move things like bfd_endian to a
non-generated header?
[bfd.h can still include it]
  

On 09/22/2015 12:05 PM, Doug Evans wrote:
> On Mon, Sep 21, 2015 at 10:49 AM, Antoine Tremblay
> <antoine.tremblay@ericsson.com> wrote:
>>
>>
>> On 09/21/2015 05:16 AM, Pedro Alves wrote:
>>>
>>> On 09/21/2015 10:10 AM, Gary Benson wrote:
>>>>
>>>> Hi Antoine, Pedro,
>>>>
>>>> Antoine Tremblay wrote:
>>>
>>>
>>>>> So I've made bfd.h a requirement of GDBServer, and when there will
>>>>> be a libgdbcommon we can have the whole lib as a requirement there.
>>>>>
>>>>> See patch v2 in next mail...
>>>>
>>>>
>>>> I don't think this will be acceptable.  If I understand correctly,
>>>> gdbserver supports some platforms that GDB (and BFD) does not, and
>>>> this patch would prevent gdbserver being built on those platforms.
>>>> Even if I'm wrong here, I've previously found it useful to build
>>>> gdbserver alone, and I think this would break that too.
>>>>
>>>> Pedro knows more about these kinds of setups, I've copied him in.
>>>>
>>>
>>> (Without looking at the patch in detail),
>>>
>>> Gary's right.  bfd.h is a generated file, generated at bfd build time.
>>> Anton, try building only gdbserver in a clean directory,
>>> separate from gdb, and it will fail with your patch.
>>>
>> Ok I was worried this would not work..
>>
>> I've removed much of the endianness dependencies from my patchset but I
>> still have a dependency on the bfd endiannness enum to share code with GDB's
>> read_memory_unsigned_integer.
>>
>> So I will do a wrapper around read_memory_unsigned_integer in GDB that takes
>> an int and transfers it to the real read_memory_unsigned_integer as the
>> proper enum (by implicit conversion). And use an int when referring to the
>> enum in shared code.
>>
>> Unless there's an objection to this method ?
>
> I'd rather not discard the enum.
>
Yes me too, that's why I had these previous solutions.

> The first question I have is: where do we want to be in the long term?
> I totally support moving more and more application independent code
> into application independent places.
> It's really a shame that something as simple as this is getting in the way.
>
> [Ideally, I'd also like to remove bfd dependencies wherever possible,
> but that can be a bit problematic. So I'm setting aside this
> possibility. E.g., using an enum without bfd in the name.]
>

To me this sounds like a case by case basis, you can't move to 
application independent code without accepting to import some 
dependencies from the applications or removing those dependencies.

So it seems not easy to me to set a future direction to this kind of 
balance. But I'm still new to GDB, I'm sure others may have more ideas 
on this.

> The next question I have is: Is there anything in what we need that
> needs to be in a generated header?
> Can we ask the bfd folks to move things like bfd_endian to a
> non-generated header?
> [bfd.h can still include it]
>

Quickly looking at how bfd.h is done it seems to be possible to move 
some stuff to a static file however I wonder if the current problem 
would prove enough justification for that work.

It would also introduce a bfd version dependency in common code to check 
for this static header. And it could be quite an ugly #ifdef changing 
ints to enum in case the header is present.

One thing to consider too is that this patchset has now changed a bit 
and this enum is no longer used in GDBServer itself at all.

Since the enum is only used in GDB's context and that the common code is 
only a carrier for this context, this is not a risk type wise.

It's also not a risk for C++ conversion, as this is a pure C enum that 
won't be converted to C++ unless bfd is which I doubt may happen ?

It could also be possible to require a make headers in bfd to build 
GDBServer and possibly make that more convenient in the Makefile ?

I'm not sure it's worth it as long as GDBServer doesn't use the enum 
itself, however I'm curious if others would find that acceptable.