New ARI regression Fri Oct 23 01:57:01 UTC 2009

New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Pierre Muller]

Is there a valid reason to use LITTLE_ENDIAN rather than
BFD_LITTLE_ENDIAN as required by the ARI rule:

LITTLE ENDIAN	3	Do not use LITTLE_ENDIAN, instead use
BFD_ENDIAN_LITTLE
See:
http://sourceware.org/gdb/current/ari/

Pierre Muller
as ARI "maintainer"


> > gdb/record.c:1958: regression: LITTLE_ENDIAN: Do not use LITTLE_ENDIAN,
instead use BFD_ENDIAN_LITTLE
> gdb/record.c:1958:  return (BYTE_ORDER == LITTLE_ENDIAN)
> > gdb/record.c:1966: regression: LITTLE_ENDIAN: Do not use LITTLE_ENDIAN,
instead use BFD_ENDIAN_LITTLE
> gdb/record.c:1966:  return (BYTE_ORDER == LITTLE_ENDIAN)
> > gdb/record.c:1974: regression: LITTLE_ENDIAN: Do not use LITTLE_ENDIAN,
instead use BFD_ENDIAN_LITTLE
> gdb/record.c:1974:  return (BYTE_ORDER == LITTLE_ENDIAN)

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Michael Snyder]

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

Pierre Muller wrote:
> Is there a valid reason to use LITTLE_ENDIAN rather than
> BFD_LITTLE_ENDIAN as required by the ARI rule:
> 
> LITTLE ENDIAN	3	Do not use LITTLE_ENDIAN, instead use
> BFD_ENDIAN_LITTLE
> See:
> http://sourceware.org/gdb/current/ari/
> 
> Pierre Muller
> as ARI "maintainer"


Thanks Pierre.  Checked in as obvious.

So let it be written -- so let it be done.


[-- Attachment #2: little.txt --]
[-- Type: text/plain, Size: 1169 bytes --]

2009-10-23  Michael Snyder  <msnyder@vmware.com>

	* record.c (netorder64): Use BFD_ENDIAN_LITTLE not LITTLE_ENDIAN.
	(netorder32): Ditto.
	(netorder16): Ditto.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.30
diff -u -p -r1.30 record.c
--- record.c	22 Oct 2009 19:36:06 -0000	1.30
+++ record.c	23 Oct 2009 16:05:47 -0000
@@ -1955,7 +1955,7 @@ bfdcore_read (bfd *obfd, asection *osec,
 static inline uint64_t
 netorder64 (uint64_t fromfile)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
+  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
     ? bswap_64 (fromfile) 
     : fromfile;
 }
@@ -1963,7 +1963,7 @@ netorder64 (uint64_t fromfile)
 static inline uint32_t
 netorder32 (uint32_t fromfile)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
+  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
     ? bswap_32 (fromfile) 
     : fromfile;
 }
@@ -1971,7 +1971,7 @@ netorder32 (uint32_t fromfile)
 static inline uint16_t
 netorder16 (uint16_t fromfile)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
+  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
     ? bswap_16 (fromfile) 
     : fromfile;
 }

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Pedro Alves]

On Friday 23 October 2009 17:00:20, Michael Snyder wrote:
> Pierre Muller wrote:
> > Is there a valid reason to use LITTLE_ENDIAN rather than
> > BFD_LITTLE_ENDIAN as required by the ARI rule:
> > 
> > LITTLE ENDIAN 3       Do not use LITTLE_ENDIAN, instead use
> > BFD_ENDIAN_LITTLE
> > See:
> > http://sourceware.org/gdb/current/ari/
> > 
> > Pierre Muller
> > as ARI "maintainer"
> 
> 
> Thanks Pierre.  Checked in as obvious.

No, this is wrong.  BYTE_ORDER isn't available on all hosts, and
even if it did, its values surely don't match enum bfd_endian...

-- 
Pedro Alves

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Michael Snyder]

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

Pedro Alves wrote:
> On Friday 23 October 2009 17:00:20, Michael Snyder wrote:
>> Pierre Muller wrote:
>>> Is there a valid reason to use LITTLE_ENDIAN rather than
>>> BFD_LITTLE_ENDIAN as required by the ARI rule:
>>>
>>> LITTLE ENDIAN 3       Do not use LITTLE_ENDIAN, instead use
>>> BFD_ENDIAN_LITTLE
>>> See:
>>> http://sourceware.org/gdb/current/ari/
>>>
>>> Pierre Muller
>>> as ARI "maintainer"
>>
>> Thanks Pierre.  Checked in as obvious.
> 
> No, this is wrong.  BYTE_ORDER isn't available on all hosts, and
> even if it did, its values surely don't match enum bfd_endian...

Dang.  How about this?

The byte order that's saved in the file is never actually used.
We just need to make sure that it's a consistent order, and that
hopefully we can read the execution log into gdb on a different
host.  So target byte order should be all that matters.


[-- Attachment #2: nobyteorder.txt --]
[-- Type: text/plain, Size: 6424 bytes --]

2009-10-23  Michael Snyder  <msnyder@vmware.com>

	* record.c (netorder64): Add gdbarch argument.  Use for byte order.
	(netorder32): Ditto.
	(netorder16): Ditto.
	(record_restore): Get gdbarch and pass to netorder.
	(cmd_record_save): Ditto.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.32
diff -u -p -r1.32 record.c
--- record.c	23 Oct 2009 14:35:30 -0000	1.32
+++ record.c	23 Oct 2009 17:17:21 -0000
@@ -31,7 +31,6 @@
 #include "elf-bfd.h"
 #include "gcore.h"
 
-#include <byteswap.h>
 #include <signal.h>
 
 /* This module implements "target record", also known as "process
@@ -59,7 +58,7 @@
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
 
-#define RECORD_FILE_MAGIC	netorder32(0x20091016)
+#define RECORD_FILE_MAGIC	netorder32(gdbarch, 0x20091016)
 
 /* These are the core structs of the process record functionality.
 
@@ -1916,7 +1915,7 @@ info_record_command (char *args, int fro
        n bytes: memory value (n == memory length).
 
    Version 2
-     4 bytes: magic number netorder32(0x20091016).
+     4 bytes: magic number netorder32(gdbarch, 0x20091016).
        NOTE: be sure to change whenever this file format changes!
 
    Records:
@@ -1953,27 +1952,33 @@ bfdcore_read (bfd *obfd, asection *osec,
 }
 
 static inline uint64_t
-netorder64 (uint64_t fromfile)
+netorder64 (struct gdbarch *gdbarch, uint64_t input)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
-    ? bswap_64 (fromfile) 
-    : fromfile;
+  uint64_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  gdbarch_byte_order (gdbarch), input);
+  return ret;
 }
 
 static inline uint32_t
-netorder32 (uint32_t fromfile)
+netorder32 (struct gdbarch *gdbarch, uint32_t input)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
-    ? bswap_32 (fromfile) 
-    : fromfile;
+  uint32_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  gdbarch_byte_order (gdbarch), input);
+  return ret;
 }
 
 static inline uint16_t
-netorder16 (uint16_t fromfile)
+netorder16 (struct gdbarch *gdbarch, uint16_t input)
 {
-  return (BYTE_ORDER == LITTLE_ENDIAN) 
-    ? bswap_16 (fromfile) 
-    : fromfile;
+  uint16_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  gdbarch_byte_order (gdbarch), input);
+  return ret;
 }
 
 /* Restore the execution log from a core_bfd file.  */
@@ -1987,6 +1992,7 @@ record_restore (void)
   uint32_t osec_size;
   int bfd_offset = 0;
   struct regcache *regcache;
+  struct gdbarch *gdbarch;
 
   /* We restore the execution log from the open core bfd,
      if there is one.  */
@@ -2010,6 +2016,9 @@ record_restore (void)
   if (record_debug)
     printf_filtered ("%s", bfd_section_name (core_bfd, osec));
 
+  regcache = get_current_regcache ();
+  gdbarch = get_regcache_arch (regcache);
+
   /* Check the magic code.  */
   bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
   if (magic != RECORD_FILE_MAGIC)
@@ -2018,7 +2027,7 @@ record_restore (void)
   if (record_debug)
     printf_filtered ("\
   Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
-		     phex_nz (netorder32 (magic), 4));
+		     phex_nz (netorder32 (gdbarch, magic), 4));
 
   /* Restore the entries in recfd into record_arch_list_head and
      record_arch_list_tail.  */
@@ -2026,7 +2035,6 @@ record_restore (void)
   record_arch_list_tail = NULL;
   record_insn_num = 0;
   old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
-  regcache = get_current_regcache ();
 
   while (1)
     {
@@ -2046,7 +2054,7 @@ record_restore (void)
           /* Get register number to regnum.  */
           bfdcore_read (core_bfd, osec, &regnum,
 			sizeof (regnum), &bfd_offset);
-	  regnum = netorder32 (regnum);
+	  regnum = netorder32 (gdbarch, regnum);
 
           rec = record_reg_alloc (regcache, regnum);
 
@@ -2066,12 +2074,12 @@ record_restore (void)
           /* Get len.  */
           bfdcore_read (core_bfd, osec, &len, 
 			sizeof (len), &bfd_offset);
-	  len = netorder32 (len);
+	  len = netorder32 (gdbarch, len);
 
           /* Get addr.  */
           bfdcore_read (core_bfd, osec, &addr,
 			sizeof (addr), &bfd_offset);
-	  addr = netorder64 (addr);
+	  addr = netorder64 (gdbarch, addr);
 
           rec = record_mem_alloc (addr, len);
 
@@ -2096,13 +2104,13 @@ record_restore (void)
 	  /* Get signal value.  */
 	  bfdcore_read (core_bfd, osec, &signal, 
 			sizeof (signal), &bfd_offset);
-	  signal = netorder32 (signal);
+	  signal = netorder32 (gdbarch, signal);
 	  rec->u.end.sigval = signal;
 
 	  /* Get insn count.  */
 	  bfdcore_read (core_bfd, osec, &count, 
 			sizeof (count), &bfd_offset);
-	  count = netorder32 (count);
+	  count = netorder32 (gdbarch, count);
 	  rec->u.end.insn_num = count;
 	  record_insn_count = count + 1;
           if (record_debug)
@@ -2314,7 +2322,7 @@ cmd_record_save (char *args, int from_tt
 				 record_list->u.reg.len);
 
               /* Write regnum.  */
-              regnum = netorder32 (record_list->u.reg.num);
+              regnum = netorder32 (gdbarch, record_list->u.reg.num);
               bfdcore_write (obfd, osec, &regnum,
 			     sizeof (regnum), &bfd_offset);
 
@@ -2334,11 +2342,11 @@ cmd_record_save (char *args, int from_tt
 				 record_list->u.mem.len);
 
 	      /* Write memlen.  */
-	      len = netorder32 (record_list->u.mem.len);
+	      len = netorder32 (gdbarch, record_list->u.mem.len);
 	      bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
 
 	      /* Write memaddr.  */
-	      addr = netorder64 (record_list->u.mem.addr);
+	      addr = netorder64 (gdbarch, record_list->u.mem.addr);
 	      bfdcore_write (obfd, osec, &addr, 
 			     sizeof (addr), &bfd_offset);
 
@@ -2354,12 +2362,12 @@ cmd_record_save (char *args, int from_tt
 				   (unsigned long) sizeof (signal),
 				   (unsigned long) sizeof (count));
 		/* Write signal value.  */
-		signal = netorder32 (record_list->u.end.sigval);
+		signal = netorder32 (gdbarch, record_list->u.end.sigval);
 		bfdcore_write (obfd, osec, &signal,
 			       sizeof (signal), &bfd_offset);
 
 		/* Write insn count.  */
-		count = netorder32 (record_list->u.end.insn_num);
+		count = netorder32 (gdbarch, record_list->u.end.insn_num);
 		bfdcore_write (obfd, osec, &count,
 			       sizeof (count), &bfd_offset);
                 break;

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Pedro Alves]

On Friday 23 October 2009 18:12:29, Michael Snyder wrote:
> 2009-10-23  Michael Snyder  <msnyder@vmware.com>
> 
>         * record.c (netorder64): Add gdbarch argument.  Use for byte order.
>         (netorder32): Ditto.
>         (netorder16): Ditto.
>         (record_restore): Get gdbarch and pass to netorder.
>         (cmd_record_save): Ditto.
> 
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 record.c
> --- record.c    23 Oct 2009 14:35:30 -0000      1.32
> +++ record.c    23 Oct 2009 17:17:21 -0000
> @@ -31,7 +31,6 @@
>  #include "elf-bfd.h"
>  #include "gcore.h"
>  
> -#include <byteswap.h>
>  #include <signal.h>
>  
>  /* This module implements "target record", also known as "process
> @@ -59,7 +58,7 @@
>  #define RECORD_IS_REPLAY \
>       (record_list->next || execution_direction == EXEC_REVERSE)
>  
> -#define RECORD_FILE_MAGIC      netorder32(0x20091016)
> +#define RECORD_FILE_MAGIC      netorder32(gdbarch, 0x20091016)

I'd still make the magic target order independent.  As simple as:

static unsigned char record_file_magic[] = { 0x20, 0x09, 0x10, 0x16 };

and then use straight memcmp to compare this:

	read 4 bytes of header into buf
        
        if (memcmp (buf, record_file_magic, 4) == 0)
          success!!

Or simply store_unsigned_integer/extract_unsigned_integer
with one of BFD_ENDIAN_LITTLE|BIG hardcoded...

I'd also add a simple version field after the magic; a simple
incrementing integer, so that you don't have to keep
inventing a new magic for every bump.

>  static inline uint64_t
> -netorder64 (uint64_t fromfile)
> +netorder64 (struct gdbarch *gdbarch, uint64_t input)
>  {
> -  return (BYTE_ORDER == LITTLE_ENDIAN) 
> -    ? bswap_64 (fromfile) 
> -    : fromfile;
> +  uint64_t ret;
> +
> +  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
> +                         gdbarch_byte_order (gdbarch), input);
> +  return ret;
>  }

These functions aren't always "network order" now, and are
hence misnamed.  Rename or eliminate them by inlining they
bodies at the call sites.  Alternatively, if you wanted to
keep using "netorder", you could just simply always
hardcode BFD_ENDIAN_BIG, or pull htonl from gnulib.

-- 
Pedro Alves

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Michael Snyder]

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

Pedro Alves wrote:

> These functions aren't always "network order" now, and are
> hence misnamed.  Rename or eliminate them by inlining they
> bodies at the call sites.  Alternatively, if you wanted to
> keep using "netorder", you could just simply always
> hardcode BFD_ENDIAN_BIG, or pull htonl from gnulib.
> 

Yeah.  Sorry for all the back-and-forth.  I like the last idea.
How's this patch?




[-- Attachment #2: bigend.txt --]
[-- Type: text/plain, Size: 1775 bytes --]

2009-10-23  Michael Snyder  <msnyder@vmware.com>

	* record.c (top level): Don't include byteswap.h.
	(netorder64): Use store_unsigned_integer instead of bswap_64.
	(netorder32): Use store_unsigned_integer instead of bswap_32.
	(netorder16): Use store_unsigned_integer instead of bswap_16.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.34
diff -u -p -r1.34 record.c
--- record.c	23 Oct 2009 17:12:25 -0000	1.34
+++ record.c	23 Oct 2009 22:33:56 -0000
@@ -31,7 +31,6 @@
 #include "elf-bfd.h"
 #include "gcore.h"
 
-#include <byteswap.h>
 #include <signal.h>
 
 /* This module implements "target record", also known as "process
@@ -1956,27 +1955,33 @@ bfdcore_read (bfd *obfd, asection *osec,
 }
 
 static inline uint64_t
-netorder64 (uint64_t fromfile)
+netorder64 (uint64_t input)
 {
-  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
-    ? bswap_64 (fromfile) 
-    : fromfile;
+  uint64_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  BFD_ENDIAN_BIG, input);
+  return ret;
 }
 
 static inline uint32_t
-netorder32 (uint32_t fromfile)
+netorder32 (uint32_t input)
 {
-  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
-    ? bswap_32 (fromfile) 
-    : fromfile;
+  uint32_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  BFD_ENDIAN_BIG, input);
+  return ret;
 }
 
 static inline uint16_t
-netorder16 (uint16_t fromfile)
+netorder16 (uint16_t input)
 {
-  return (BYTE_ORDER == BFD_ENDIAN_LITTLE) 
-    ? bswap_16 (fromfile) 
-    : fromfile;
+  uint16_t ret;
+
+  store_unsigned_integer ((gdb_byte *) &ret, sizeof (ret), 
+			  BFD_ENDIAN_BIG, input);
+  return ret;
 }
 
 /* Restore the execution log from a core_bfd file.  */

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Pedro Alves]

> 2009-10-23  Michael Snyder  <msnyder@vmware.com>
> 
>         * record.c (top level): Don't include byteswap.h.
>         (netorder64): Use store_unsigned_integer instead of bswap_64.
>         (netorder32): Use store_unsigned_integer instead of bswap_32.
>         (netorder16): Use store_unsigned_integer instead of bswap_16.

Ok.  Thanks.

-- 
Pedro Alves

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Michael Snyder]

Pedro Alves wrote:
>> 2009-10-23  Michael Snyder  <msnyder@vmware.com>
>>
>>         * record.c (top level): Don't include byteswap.h.
>>         (netorder64): Use store_unsigned_integer instead of bswap_64.
>>         (netorder32): Use store_unsigned_integer instead of bswap_32.
>>         (netorder16): Use store_unsigned_integer instead of bswap_16.
> 
> Ok.  Thanks.
> 

Good -- committed.
Thanks for all the help.

Re: New ARI regression Fri Oct 23 01:57:01 UTC 2009

[Hui Zhu]

Thanks for your work.

Hui

On Sat, Oct 24, 2009 at 07:35, Michael Snyder <msnyder@vmware.com> wrote:
> Pedro Alves wrote:
>>>
>>> 2009-10-23  Michael Snyder  <msnyder@vmware.com>
>>>
>>>        * record.c (top level): Don't include byteswap.h.
>>>        (netorder64): Use store_unsigned_integer instead of bswap_64.
>>>        (netorder32): Use store_unsigned_integer instead of bswap_32.
>>>        (netorder16): Use store_unsigned_integer instead of bswap_16.
>>
>> Ok.  Thanks.
>>
>
> Good -- committed.
> Thanks for all the help.
>
>