[PATCH v2 5/7] target: consider addressable unit size when reading/writing memory

[PATCH v2 5/7] target: consider addressable unit size when reading/writing memory

[Simon Marchi]

If we are reading/writing from a memory object, the length represents
the number of "addresses" to read/write, so the addressable unit size
needs to be taken into account when allocating memory on gdb's side.

gdb/ChangeLog:

	* target.c (target_read): Consider addressable unit size when
	reading from a memory object.
	(read_memory_robust): Same.
	(read_whatever_is_readable): Same.
	(target_write_with_progress): Consider addressable unit size
	when writing to a memory object.
	* target.h (target_read): Update documentation.
	(target_write): Add documentation.
---
 gdb/target.c | 35 ++++++++++++++++++++++++++++-------
 gdb/target.h | 33 ++++++++++++++++++++++++++-------
 2 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index bd9a0eb..437b34e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1589,6 +1589,15 @@ target_read (struct target_ops *ops,
 	     ULONGEST offset, LONGEST len)
 {
   LONGEST xfered_total = 0;
+  int unit_size = 1;
+
+  /* If we are reading from a memory object, find the length of an addressable
+     unit for that architecture.  */
+  if (object == TARGET_OBJECT_MEMORY
+      || object == TARGET_OBJECT_STACK_MEMORY
+      || object == TARGET_OBJECT_CODE_MEMORY
+      || object == TARGET_OBJECT_RAW_MEMORY)
+    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch());
 
   while (xfered_total < len)
     {
@@ -1596,7 +1605,7 @@ target_read (struct target_ops *ops,
       enum target_xfer_status status;
 
       status = target_read_partial (ops, object, annex,
-				    buf + xfered_total,
+				    buf + xfered_total * unit_size,
 				    offset + xfered_total, len - xfered_total,
 				    &xfered_partial);
 
@@ -1639,6 +1648,7 @@ target_read (struct target_ops *ops,
 static void
 read_whatever_is_readable (struct target_ops *ops,
 			   const ULONGEST begin, const ULONGEST end,
+			   int unit_size,
 			   VEC(memory_read_result_s) **result)
 {
   gdb_byte *buf = xmalloc (end - begin);
@@ -1705,7 +1715,7 @@ read_whatever_is_readable (struct target_ops *ops,
 	}
 
       xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-			  buf + (first_half_begin - begin),
+			  buf + (first_half_begin - begin) * unit_size,
 			  first_half_begin,
 			  first_half_end - first_half_begin);
 
@@ -1741,8 +1751,9 @@ read_whatever_is_readable (struct target_ops *ops,
       /* The [current_end, end) range has been read.  */
       LONGEST region_len = end - current_end;
 
-      r.data = xmalloc (region_len);
-      memcpy (r.data, buf + current_end - begin, region_len);
+      r.data = xmalloc (region_len * unit_size);
+      memcpy (r.data, buf + (current_end - begin) * unit_size,
+	      region_len * unit_size);
       r.begin = current_end;
       r.end = end;
       xfree (buf);
@@ -1769,6 +1780,7 @@ read_memory_robust (struct target_ops *ops,
 		    const ULONGEST offset, const LONGEST len)
 {
   VEC(memory_read_result_s) *result = 0;
+  int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());
 
   LONGEST xfered_total = 0;
   while (xfered_total < len)
@@ -1794,7 +1806,7 @@ read_memory_robust (struct target_ops *ops,
       else
 	{
 	  LONGEST to_read = min (len - xfered_total, region_len);
-	  gdb_byte *buffer = (gdb_byte *)xmalloc (to_read);
+	  gdb_byte *buffer = (gdb_byte *) xmalloc (to_read * unit_size);
 
 	  LONGEST xfered_partial =
 	      target_read (ops, TARGET_OBJECT_MEMORY, NULL,
@@ -1806,7 +1818,7 @@ read_memory_robust (struct target_ops *ops,
 	      /* Got an error reading full chunk.  See if maybe we can read
 		 some subrange.  */
 	      xfree (buffer);
-	      read_whatever_is_readable (ops, offset + xfered_total,
+	      read_whatever_is_readable (ops, offset + xfered_total, unit_size,
 					 offset + xfered_total + to_read, &result);
 	      xfered_total += to_read;
 	    }
@@ -1836,6 +1848,15 @@ target_write_with_progress (struct target_ops *ops,
 			    void (*progress) (ULONGEST, void *), void *baton)
 {
   LONGEST xfered_total = 0;
+  int unit_size = 1;
+
+  /* If we are writing to a memory object, find the length of an addressable
+     unit for that architecture.  */
+  if (object == TARGET_OBJECT_MEMORY
+      || object == TARGET_OBJECT_STACK_MEMORY
+      || object == TARGET_OBJECT_CODE_MEMORY
+      || object == TARGET_OBJECT_RAW_MEMORY)
+    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());
 
   /* Give the progress callback a chance to set up.  */
   if (progress)
@@ -1847,7 +1868,7 @@ target_write_with_progress (struct target_ops *ops,
       enum target_xfer_status status;
 
       status = target_write_partial (ops, object, annex,
-				     (gdb_byte *) buf + xfered_total,
+				     buf + xfered_total * unit_size,
 				     offset + xfered_total, len - xfered_total,
 				     &xfered_partial);
 
diff --git a/gdb/target.h b/gdb/target.h
index ad50f07..6f9ff65 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -259,13 +259,17 @@ typedef enum target_xfer_status
 			     ULONGEST len,
 			     ULONGEST *xfered_len);
 
-/* Request that OPS transfer up to LEN 8-bit bytes of the target's
-   OBJECT.  The OFFSET, for a seekable object, specifies the
-   starting point.  The ANNEX can be used to provide additional
-   data-specific information to the target.
-
-   Return the number of bytes actually transfered, or a negative error
-   code (an 'enum target_xfer_error' value) if the transfer is not
+/* Request that OPS transfer up to LEN addressable units of the target's
+   OBJECT.  When reading from a memory object, the size of an addressable unit
+   is architecture dependent and can be found using
+   gdbarch_addressable_memory_unit_size.  Otherwise, an addressable unit is 1
+   byte long.  BUF should point to a buffer large enough to hold the read data,
+   taking into account the addressable unit size.  The OFFSET, for a seekable
+   object, specifies the starting point.  The ANNEX can be used to provide
+   additional data-specific information to the target.
+
+   Return the number of addressable units actually transferred, or a negative
+   error code (an 'enum target_xfer_error' value) if the transfer is not
    supported or otherwise fails.  Return of a positive value less than
    LEN indicates that no further transfer is possible.  Unlike the raw
    to_xfer_partial interface, callers of these functions do not need
@@ -294,6 +298,21 @@ extern VEC(memory_read_result_s)* read_memory_robust (struct target_ops *ops,
 						      const ULONGEST offset,
 						      const LONGEST len);
 
+/* Request that OPS transfer up to LEN addressable units from BUF to the
+   target's OBJECT.  When writing to a memory object, the addressable unit
+   size is architecture dependent and can be found using
+   gdbarch_addressable_memory_unit_size.  Otherwise, an addressable unit is 1
+   byte long.  The OFFSET, for a seekable object, specifies the starting point.
+   The ANNEX can be used to provide additional data-specific information to
+   the target.
+
+   Return the number of addressable units actually transferred, or a negative
+   error code (an 'enum target_xfer_status' value) if the transfer is not
+   supported or otherwise fails.  Return of a positive value less than
+   LEN indicates that no further transfer is possible.  Unlike the raw
+   to_xfer_partial interface, callers of these functions do not need to
+   retry partial transfers.  */
+
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,
 			     const char *annex, const gdb_byte *buf,
-- 
2.1.4

[PATCH v2 1/7] Various cleanups in target read/write code

[Simon Marchi]

This contains various cleanups in the target memory read and write code.
They are not directly related to the non-8-bits changes, but they
clarify things a bit down the line.

gdb/ChangeLog:

	* target.c (target_read): Rename variables and use
	TARGET_XFER_E_IO.
	(target_read_with_progress): Same.
	(read_memory_robust): Constify parameters and rename
	variables.
	(read_whatever_is_readable): Constify parameters,
	rename variables, adjust formatting.
	* target.h (read_memory_robust): Constify parameters.
---
 gdb/target.c | 90 +++++++++++++++++++++++++++++++-----------------------------
 gdb/target.h |  6 ++--
 2 files changed, 49 insertions(+), 47 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 306c21d..fcf7090 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1592,28 +1592,28 @@ target_read (struct target_ops *ops,
 	     const char *annex, gdb_byte *buf,
 	     ULONGEST offset, LONGEST len)
 {
-  LONGEST xfered = 0;
+  LONGEST xfered_total = 0;
 
-  while (xfered < len)
+  while (xfered_total < len)
     {
-      ULONGEST xfered_len;
+      ULONGEST xfered_partial;
       enum target_xfer_status status;
 
       status = target_read_partial (ops, object, annex,
-				    (gdb_byte *) buf + xfered,
-				    offset + xfered, len - xfered,
-				    &xfered_len);
+				    buf + xfered_total,
+				    offset + xfered_total, len - xfered_total,
+				    &xfered_partial);
 
       /* Call an observer, notifying them of the xfer progress?  */
       if (status == TARGET_XFER_EOF)
-	return xfered;
+	return xfered_total;
       else if (status == TARGET_XFER_OK)
 	{
-	  xfered += xfered_len;
+	  xfered_total += xfered_partial;
 	  QUIT;
 	}
       else
-	return -1;
+	return TARGET_XFER_E_IO;
 
     }
   return len;
@@ -1642,7 +1642,7 @@ target_read (struct target_ops *ops,
 
 static void
 read_whatever_is_readable (struct target_ops *ops,
-			   ULONGEST begin, ULONGEST end,
+			   const ULONGEST begin, const ULONGEST end,
 			   VEC(memory_read_result_s) **result)
 {
   gdb_byte *buf = xmalloc (end - begin);
@@ -1669,7 +1669,7 @@ read_whatever_is_readable (struct target_ops *ops,
       ++current_begin;
     }
   else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
-				buf + (end-begin) - 1, end - 1, 1,
+				buf + (end - begin) - 1, end - 1, 1,
 				&xfered_len) == TARGET_XFER_OK)
     {
       forward = 0;
@@ -1691,7 +1691,7 @@ read_whatever_is_readable (struct target_ops *ops,
       ULONGEST first_half_begin, first_half_end;
       ULONGEST second_half_begin, second_half_end;
       LONGEST xfer;
-      ULONGEST middle = current_begin + (current_end - current_begin)/2;
+      ULONGEST middle = current_begin + (current_end - current_begin) / 2;
 
       if (forward)
 	{
@@ -1723,7 +1723,7 @@ read_whatever_is_readable (struct target_ops *ops,
       else
 	{
 	  /* This half is not readable.  Because we've tried one byte, we
-	     know some part of this half if actually redable.  Go to the next
+	     know some part of this half if actually readable.  Go to the next
 	     iteration to divide again and try to read.
 
 	     We don't handle the other half, because this function only tries
@@ -1743,10 +1743,10 @@ read_whatever_is_readable (struct target_ops *ops,
   else
     {
       /* The [current_end, end) range has been read.  */
-      LONGEST rlen = end - current_end;
+      LONGEST region_len = end - current_end;
 
-      r.data = xmalloc (rlen);
-      memcpy (r.data, buf + current_end - begin, rlen);
+      r.data = xmalloc (region_len);
+      memcpy (r.data, buf + current_end - begin, region_len);
       r.begin = current_end;
       r.end = end;
       xfree (buf);
@@ -1769,57 +1769,59 @@ free_memory_read_result_vector (void *x)
 }
 
 VEC(memory_read_result_s) *
-read_memory_robust (struct target_ops *ops, ULONGEST offset, LONGEST len)
+read_memory_robust (struct target_ops *ops,
+		    const ULONGEST offset, const LONGEST len)
 {
   VEC(memory_read_result_s) *result = 0;
 
-  LONGEST xfered = 0;
-  while (xfered < len)
+  LONGEST xfered_total = 0;
+  while (xfered_total < len)
     {
-      struct mem_region *region = lookup_mem_region (offset + xfered);
-      LONGEST rlen;
+      struct mem_region *region = lookup_mem_region (offset + xfered_total);
+      LONGEST region_len;
 
       /* If there is no explicit region, a fake one should be created.  */
       gdb_assert (region);
 
       if (region->hi == 0)
-	rlen = len - xfered;
+	region_len = len - xfered_total;
       else
-	rlen = region->hi - offset;
+	region_len = region->hi - offset;
 
       if (region->attrib.mode == MEM_NONE || region->attrib.mode == MEM_WO)
 	{
 	  /* Cannot read this region.  Note that we can end up here only
 	     if the region is explicitly marked inaccessible, or
 	     'inaccessible-by-default' is in effect.  */
-	  xfered += rlen;
+	  xfered_total += region_len;
 	}
       else
 	{
-	  LONGEST to_read = min (len - xfered, rlen);
+	  LONGEST to_read = min (len - xfered_total, region_len);
 	  gdb_byte *buffer = (gdb_byte *)xmalloc (to_read);
 
-	  LONGEST xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-				      (gdb_byte *) buffer,
-				      offset + xfered, to_read);
+	  LONGEST xfered_partial =
+	      target_read (ops, TARGET_OBJECT_MEMORY, NULL,
+			   (gdb_byte *) buffer,
+			   offset + xfered_total, to_read);
 	  /* Call an observer, notifying them of the xfer progress?  */
-	  if (xfer <= 0)
+	  if (xfered_partial <= 0)
 	    {
 	      /* Got an error reading full chunk.  See if maybe we can read
 		 some subrange.  */
 	      xfree (buffer);
-	      read_whatever_is_readable (ops, offset + xfered,
-					 offset + xfered + to_read, &result);
-	      xfered += to_read;
+	      read_whatever_is_readable (ops, offset + xfered_total,
+					 offset + xfered_total + to_read, &result);
+	      xfered_total += to_read;
 	    }
 	  else
 	    {
 	      struct memory_read_result r;
 	      r.data = buffer;
-	      r.begin = offset + xfered;
-	      r.end = r.begin + xfer;
+	      r.begin = offset + xfered_total;
+	      r.end = r.begin + xfered_partial;
 	      VEC_safe_push (memory_read_result_s, result, &r);
-	      xfered += xfer;
+	      xfered_total += xfered_partial;
 	    }
 	  QUIT;
 	}
@@ -1837,29 +1839,29 @@ target_write_with_progress (struct target_ops *ops,
 			    ULONGEST offset, LONGEST len,
 			    void (*progress) (ULONGEST, void *), void *baton)
 {
-  LONGEST xfered = 0;
+  LONGEST xfered_total = 0;
 
   /* Give the progress callback a chance to set up.  */
   if (progress)
     (*progress) (0, baton);
 
-  while (xfered < len)
+  while (xfered_total < len)
     {
-      ULONGEST xfered_len;
+      ULONGEST xfered_partial;
       enum target_xfer_status status;
 
       status = target_write_partial (ops, object, annex,
-				     (gdb_byte *) buf + xfered,
-				     offset + xfered, len - xfered,
-				     &xfered_len);
+				     (gdb_byte *) buf + xfered_total,
+				     offset + xfered_total, len - xfered_total,
+				     &xfered_partial);
 
       if (status != TARGET_XFER_OK)
-	return status == TARGET_XFER_EOF ? xfered : -1;
+	return status == TARGET_XFER_EOF ? xfered_total : TARGET_XFER_E_IO;
 
       if (progress)
-	(*progress) (xfered_len, baton);
+	(*progress) (xfered_partial, baton);
 
-      xfered += xfered_len;
+      xfered_total += xfered_partial;
       QUIT;
     }
   return len;
diff --git a/gdb/target.h b/gdb/target.h
index f57e431..ad50f07 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -291,9 +291,9 @@ DEF_VEC_O(memory_read_result_s);
 extern void free_memory_read_result_vector (void *);
 
 extern VEC(memory_read_result_s)* read_memory_robust (struct target_ops *ops,
-						      ULONGEST offset,
-						      LONGEST len);
-  
+						      const ULONGEST offset,
+						      const LONGEST len);
+
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,
 			     const char *annex, const gdb_byte *buf,
-- 
2.1.4

[PATCH v2 0/7] Support reading/writing memory on architectures with non 8-bits addressable memory

[Simon Marchi]

Here is the second version of this patch. The biggest change is to use
"addressable memory unit" for the amount of data associated to an
address in memory, and "byte" for an 8-bits chunk.

I added a section in the manual to describe what an addressable memory
unit is, and the difference between that and a byte in the context of
gdb.

Below is the original message, with the vocabulary adjusted:

At Ericsson, we work with an architecture which uses memory with
addressable units of 16-bits instead of the common 8-bits. There are a
few examples in the DSP world of processors that work with 16, 24 or
32-bits addressable memory units. Over the past year or so, we have
adapted gdb to work with our architecture and we now feel it's the right
time to start contributing some of the changes back. On gdb's side, it
should be a step towards supporting a new range of chips. On our side,
it will help us stay closer to mainline.

On such a system, memory is addressable in atomic chunks of 16-bits. On
a "normal" system, you have 8 bits of data associated with each memory
address:

  Address    Data
  ---------------
  0x1000     0xaa
  0x1001     0xbb
  0x1002     0xcc
  0x1003     0xdd

whereas on a system with 16-bits addressable units, you have 16-bits of
data per address:

  Address    Data
  ---------------
  0x1000   0xaaaa
  0x1001   0xbbbb
  0x1002   0xcccc
  0x1003   0xdddd

To support these systems, GDB must be modified to consider the addressable
unit size when reading/writing memory. This is what this first patch
series is about.

Also, on these systems, sizeof(char) == 1 == 16 bits. There is therefore
many places related to types and values handling that need to be
modified. This will be the subject of subsequent patch series.

I did a quick research of publicly available chips which use non-8-bits
addressable memory units.

 - TI C54x family (16-bits memory)
   http://www.ti.com/lsds/ti/dsp/c5000_dsp/c54x/products.page

   It seems like work had been done to port an ancient GDB for the
   earlier C4x family a long time ago, but I can't find the source
   anywhere. They must have done changes similar to ours in order to
   support the 16-bits memory. If you know where to find that source,
   I'd be really interested to see it!
   http://www.elec.canterbury.ac.nz/c4x/

 - Atmel ATMega family, the flash program space has 16-bits units
   http://www.atmel.com/products/microcontrollers/avr/megaavr.aspx

 - Some DSPs targetting audio have 16-bits or 24-bits memory units.
   http://www.analog.com/media/en/technical-documentation/data-sheets/ADSP-21990.pdf

 - Another one by Freescale
   http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=DSP56321


Terminology
-----------

Talking about this stuff becomes quickly confusing without a consistent
terminology. Here is what I try to stick to in my messages,
documentation and code:

 - `byte` when referring to a chunk of 8 bits
 - `addressable memory unit` when referring to one chunk of the
   addressable size of the memory


API proposition
---------------

The current APIs (MI and RSP) need to be clarified in order to be usable
with 16-bits (and other) systems. In the current state, it is not clear
if a length should be expressed in bytes or addressable memory units.
Here are the changes we suggest. They are based on what we have been
using for some time, so we are confident it works quite well. This should
not affect any existing system using 8-bits memory (IOW, everything
supported right now), since an addressable memory unit and a byte are the
same thing for them.

For MI, the parameters representing lengths and offsets in memory in 

  -data-read-memory-bytes
  -data-write-memory-bytes

should be in addressable memory units. This makes the API easier to use,
because it's impossible to request an invalid amount of units (e.g.
trying to read 3 bytes on a 16-bits addressable system). Also, it feels
more natural this way, because the client expresses the length as the
number of memory addresses it wants to read. Here is an example MI
session with an hypothetic system that uses 16-bits addressable memory.

  -data-read-memory-bytes -o 4096 0 4
  ^done,memory=[{begin="0x1000",offset="0x0000",end="0x1004",contents="aaaabbbbccccdddd"}]

  -data-write-memory-bytes 4096 eeeeffff 3
  ^done

  -data-read-memory-bytes -o 4096 0 4
  ^done,memory=[{begin="0x1000",offset="0x0000",end="0x1004",contents="eeeeffffeeeedddd"}]

  -data-write-memory-bytes 4096 ee
  ^error,msg="Hex-encoded 'ee' must represent an integral number of addressable memory units."

The error case in the last command is similar to the error thrown when
trying to write an odd number of hex digits. The number of given hex
digits must represent an integral number of bytes of the memory you are
writing to.

For RSP's m, M and X packets, the "length" parameters are used to
correctly encode or decode the packet at a low level, where we don't
want to have to deal with different target byte sizes. Also, for a
network protocol, it would make sense to use a fixed-sized unit.
Therefore, it would be easier if those lengths were always in bytes.
Here is an example that corresponds to the previous MI example.

   -> $m1000,8#??
   <- aaaabbbbccccdddd

   -> $M1000,6:eeeeffffeeee#??
   <- OK

   -> $m1000,8#??
   <- eeeeffffeeeedddd

If there are any other RSP packets or MI commands that need such
clarification, it will be on a case-by-case basis, whatever makes more
sense for each particular one.


Simon Marchi (7):
  Various cleanups in target read/write code
  Cleanup some docs about memory write
  Clarify doc about memory read/write and non-8-bits addressable memory
    unit sizes
  gdbarch: add addressable_memory_unit_size method
  target: consider addressable unit size when reading/writing memory
  remote: consider addressable unit size when reading/writing memory
  MI: consider addressable unit size when reading/writing memory

 gdb/arch-utils.c       |   9 +++
 gdb/arch-utils.h       |   1 +
 gdb/common/rsp-low.c   |  68 +++++++++++++------
 gdb/common/rsp-low.h   |  18 ++---
 gdb/corefile.c         |   4 +-
 gdb/doc/gdb.texinfo    |  56 ++++++++++------
 gdb/doc/python.texi    |   5 +-
 gdb/gdbarch.c          |  23 +++++++
 gdb/gdbarch.h          |   7 ++
 gdb/gdbarch.sh         |   5 ++
 gdb/gdbcore.h          |   6 +-
 gdb/gdbserver/server.c |   4 +-
 gdb/mi/mi-main.c       |  56 +++++++++-------
 gdb/remote.c           | 174 +++++++++++++++++++++++++++----------------------
 gdb/target.c           | 121 +++++++++++++++++++---------------
 gdb/target.h           |  39 ++++++++---
 gdb/target/target.h    |  10 +--
 17 files changed, 381 insertions(+), 225 deletions(-)

-- 
2.1.4

[PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Simon Marchi]

New in v2:

 * Change wording: use byte for 8-bits chunks and addressable memory unit
   for the unit of data associated to a single address.
 * Introduce definition of addressable memory unit in the Memory
   section.

This patch modifies the manual to clarify the MI, RSP and Python APIs in
regard to reading/writing memory on architectures with addressable
memory unit that are not 8 bits.

Care is taken to use the word "addressable memory unit" or "memory unit"
when referring to one piece of the smallest addressable size on the
current architecture and the word "byte" when referring to an 8-bits
data piece.

For MI, -data-{read,write}-memory are not modified, since they are
deprecated.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Data Manipulation): Clarify usage of
	bytes and memory units for -data-{read,write}-memory-bytes.
	* python.texi (Inferiors In Python): Same for read_memory and
	write_memory.
---
 gdb/doc/gdb.texinfo | 56 +++++++++++++++++++++++++++++++++++------------------
 gdb/doc/python.texi |  5 +++--
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6255c14..dd5f941 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -9006,6 +9006,18 @@ If the @code{x} command has a repeat count, the address and contents saved
 are from the last memory unit printed; this is not the same as the last
 address printed if several units were printed on the last line of output.
 
+@anchor{addressable memory unit}
+@cindex addressable memory unit
+Most targets have an addressable memory unit size of 8 bits.  This means
+that to each memory address are associated 8 bits of data.  Some
+targets, however, have other addressable memory unit sizes.
+Within @value{GDBN} and this document, the term
+@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
+when explicitely referring to a chunk of data of that size.  The word
+@dfn{byte} is used to refer to a chunk of data of 8 bits, regardless of
+the addressable memory unit size of the target.  For most systems,
+addressable memory unit is a synonym of byte.
+
 @cindex remote memory comparison
 @cindex target memory comparison
 @cindex verify remote memory image
@@ -29473,6 +29485,9 @@ can be used to instantiate this class for a varobj:
 This section describes the @sc{gdb/mi} commands that manipulate data:
 examine memory and registers, evaluate expressions, etc.
 
+For details about what an addressable memory unit is,
+@pxref{addressable memory unit}.
+
 @c REMOVED FROM THE INTERFACE.
 @c @subheading -data-assign
 @c Change the value of a program variable. Plenty of side effects.
@@ -29997,7 +30012,7 @@ next-page="0x000013c0",prev-page="0x00001380",memory=[
 @subsubheading Synopsis
 
 @smallexample
- -data-read-memory-bytes [ -o @var{byte-offset} ]
+ -data-read-memory-bytes [ -o @var{offset} ]
    @var{address} @var{count}
 @end smallexample
 
@@ -30006,18 +30021,19 @@ where:
 
 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-read.  Complex expressions containing embedded white space should be
+An expression specifying the address of the first addressable memory unit
+to be read.  Complex expressions containing embedded white space should be
 quoted using the C convention.
 
 @item @var{count}
-The number of bytes to read.  This should be an integer literal.
+The number of addressable memory units to read.  This should be an integer
+literal.
 
-@item @var{byte-offset}
-The offsets in bytes relative to @var{address} at which to start
-reading.  This should be an integer literal.  This option is provided
-so that a frontend is not required to first evaluate address and then
-perform address arithmetics itself.
+@item @var{offset}
+The offset relative to @var{address} at which to start reading.  This
+should be an integer literal.  This option is provided so that a frontend
+is not required to first evaluate address and then perform address
+arithmetics itself.
 
 @end table
 
@@ -30028,10 +30044,10 @@ Attributes}.  Second, @value{GDBN} will attempt to read the remaining
 regions.  For each one, if reading full region results in an errors,
 @value{GDBN} will try to read a subset of the region.
 
-In general, every single byte in the region may be readable or not,
-and the only way to read every readable byte is to try a read at
+In general, every single memory unit in the region may be readable or not,
+and the only way to read every readable unit is to try a read at
 every address, which is not practical.   Therefore, @value{GDBN} will
-attempt to read all accessible bytes at either beginning or the end
+attempt to read all accessible memory units at either beginning or the end
 of the region, using a binary division scheme.  This heuristic works
 well for reading accross a memory map boundary.  Note that if a region
 has a readable range that is neither at the beginning or the end,
@@ -30091,17 +30107,19 @@ where:
 
 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-written.  Complex expressions containing embedded white space should be
-quoted using the C convention.
+An expression specifying the address of the first addressable memory unit
+to be written.  Complex expressions containing embedded white space should
+be quoted using the C convention.
 
 @item @var{contents}
-The hex-encoded bytes to write.
+The hex-encoded data to write.  It is an error if @var{contents} does
+not represent an integral number of addressable memory units.
 
 @item @var{count}
-Optional argument indicating the number of bytes to be written.  If @var{count} 
-is greater than @var{contents}' length, @value{GDBN} will repeatedly 
-write @var{contents} until it fills @var{count} bytes.
+Optional argument indicating the number of addressable memory units to be
+written.  If @var{count} is greater than @var{contents}' length,
+@value{GDBN} will repeatedly write @var{contents} until it fills
+@var{count} memory units.
 
 @end table
 
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 098d718..c9d3f13 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2718,7 +2718,7 @@ return an empty tuple.
 
 @findex Inferior.read_memory
 @defun Inferior.read_memory (address, length)
-Read @var{length} bytes of memory from the inferior, starting at
+Read @var{length} addressable memory units from the inferior, starting at
 @var{address}.  Returns a buffer object, which behaves much like an array
 or a string.  It can be modified and given to the
 @code{Inferior.write_memory} function.  In @code{Python} 3, the return
@@ -2731,7 +2731,8 @@ Write the contents of @var{buffer} to the inferior, starting at
 @var{address}.  The @var{buffer} parameter must be a Python object
 which supports the buffer protocol, i.e., a string, an array or the
 object returned from @code{Inferior.read_memory}.  If given, @var{length}
-determines the number of bytes from @var{buffer} to be written.
+determines the number of addressable memory units from @var{buffer} to be
+written.
 @end defun
 
 @findex gdb.search_memory
-- 
2.1.4

[PATCH v2 2/7] Cleanup some docs about memory write

[Simon Marchi]

Some docs seemed outdated. In the case of target_write_memory, the docs
in target.c and target/target.h diverged a bit, so I tried to find a
reasonnable in-between version.

gdb/ChangeLog:

	* corefile.c (write_memory): Update doc.
	* gdbcore.h (write_memory): Same.
	* target.c (target_write_memory): Same.
	* target/target.h (target_write_memory): Same.
---
 gdb/corefile.c      |  4 ++--
 gdb/gdbcore.h       |  6 ++----
 gdb/target.c        |  6 +-----
 gdb/target/target.h | 10 +++++-----
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..83b0e80 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
   return extract_typed_address (buf, type);
 }
 
-/* Same as target_write_memory, but report an error if can't
-   write.  */
+/* See gdbcore.h. */
+
 void
 write_memory (CORE_ADDR memaddr, 
 	      const bfd_byte *myaddr, ssize_t len)
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 63a75f0..1106db8 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -101,10 +101,8 @@ extern void read_memory_string (CORE_ADDR, char *, int);
 
 CORE_ADDR read_memory_typed_address (CORE_ADDR addr, struct type *type);
 
-/* This takes a char *, not void *.  This is probably right, because
-   passing in an int * or whatever is wrong with respect to
-   byteswapping, alignment, different sizes for host vs. target types,
-   etc.  */
+/* Same as target_write_memory, but report an error if can't
+   write.  */
 
 extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 			  ssize_t len);
diff --git a/gdb/target.c b/gdb/target.c
index fcf7090..bd9a0eb 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1464,11 +1464,7 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
     return TARGET_XFER_E_IO;
 }
 
-/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
-   Returns either 0 for success or TARGET_XFER_E_IO if any
-   error occurs.  If an error occurs, no guarantee is made about how
-   much data got written.  Callers that can deal with partial writes
-   should call target_write.  */
+/* See target/target.h.  */
 
 int
 target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 05ac758..e0b7554 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -49,12 +49,12 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 
 /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
-   Return zero for success, nonzero if any error occurs.  This
+   Return zero for success or TARGET_XFER_E_IO if any error occurs.  This
    function must be provided by the client.  Implementations of this
-   function may define and use their own error codes, but functions
-   in the common, nat and target directories must treat the return
-   code as opaque.  No guarantee is made about the contents of the
-   data at MEMADDR if any error occurs.  */
+   function may define and use their own error codes, but functions in
+   the common, nat and target directories must treat the return code as
+   opaque.  No guarantee is made about how much data got written if any
+   error occurs.  */
 
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);
-- 
2.1.4

[PATCH v2 7/7] MI: consider addressable unit size when reading/writing memory

[Simon Marchi]

As a user of the target memory read/write interface, the MI code must
adjust its memory allocations to take into account the addressable memory
unitsize of the target.

gdb/ChangeLog:

	mi/mi-main.c (mi_cmd_data_read_memory_bytes): Consider byte
	size.
	(mi_cmd_data_write_memory_bytes): Same.
---
 gdb/mi/mi-main.c | 56 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 2733e80..ddfc9d9 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1595,6 +1595,7 @@ mi_cmd_data_read_memory_bytes (char *command, char **argv, int argc)
   int ix;
   VEC(memory_read_result_s) *result;
   long offset = 0;
+  int unit_size = gdbarch_addressable_memory_unit_size (gdbarch);
   int oind = 0;
   char *oarg;
   enum opt
@@ -1650,10 +1651,11 @@ mi_cmd_data_read_memory_bytes (char *command, char **argv, int argc)
 			      - addr);
       ui_out_field_core_addr (uiout, "end", gdbarch, read_result->end);
 
-      data = xmalloc ((read_result->end - read_result->begin) * 2 + 1);
+      data = xmalloc (
+	  (read_result->end - read_result->begin) * 2 * unit_size + 1);
 
       for (i = 0, p = data;
-	   i < (read_result->end - read_result->begin);
+	   i < ((read_result->end - read_result->begin) * unit_size);
 	   ++i, p += 2)
 	{
 	  sprintf (p, "%02x", read_result->data[i]);
@@ -1762,29 +1764,36 @@ mi_cmd_data_write_memory_bytes (char *command, char **argv, int argc)
   char *cdata;
   gdb_byte *data;
   gdb_byte *databuf;
-  size_t len, i, steps, remainder;
-  long int count, j;
+  size_t len_hex, len_bytes, len_units, i, steps, remaining_units;
+  long int count_units;
   struct cleanup *back_to;
+  int unit_size;
 
   if (argc != 2 && argc != 3)
     error (_("Usage: ADDR DATA [COUNT]."));
 
   addr = parse_and_eval_address (argv[0]);
   cdata = argv[1];
-  if (strlen (cdata) % 2)
-    error (_("Hex-encoded '%s' must have an even number of characters."),
+  len_hex = strlen (cdata);
+  unit_size = gdbarch_addressable_memory_unit_size (get_current_arch ());
+
+  if (len_hex % (unit_size * 2) != 0)
+    error (_("Hex-encoded '%s' must represent an integral number of "
+	     "addressable memory units."),
 	   cdata);
 
-  len = strlen (cdata)/2;
+  len_bytes = len_hex / 2;
+  len_units = len_bytes / unit_size;
+
   if (argc == 3)
-    count = strtoul (argv[2], NULL, 10);
+    count_units = strtoul (argv[2], NULL, 10);
   else
-    count = len;
+    count_units = len_units;
 
-  databuf = xmalloc (len * sizeof (gdb_byte));
+  databuf = xmalloc (len_bytes * sizeof (gdb_byte));
   back_to = make_cleanup (xfree, databuf);
 
-  for (i = 0; i < len; ++i)
+  for (i = 0; i < len_bytes; ++i)
     {
       int x;
       if (sscanf (cdata + i * 2, "%02x", &x) != 1)
@@ -1792,29 +1801,32 @@ mi_cmd_data_write_memory_bytes (char *command, char **argv, int argc)
       databuf[i] = (gdb_byte) x;
     }
 
-  if (len < count)
+  if (len_units < count_units)
     {
-      /* Pattern is made of less bytes than count:
+      /* Pattern is made of less units than count:
          repeat pattern to fill memory.  */
-      data = xmalloc (count);
+      data = xmalloc (count_units * unit_size);
       make_cleanup (xfree, data);
 
-      steps = count / len;
-      remainder = count % len;
-      for (j = 0; j < steps; j++)
-        memcpy (data + j * len, databuf, len);
+      /* Number of times the pattern is entirely repeated.  */
+      steps = count_units / len_units;
+      /* Number of remaining addressable memory units.  */
+      remaining_units = count_units % len_units;
+      for (i = 0; i < steps; i++)
+        memcpy (data + i * len_bytes, databuf, len_bytes);
 
-      if (remainder > 0)
-        memcpy (data + steps * len, databuf, remainder);
+      if (remaining_units > 0)
+        memcpy (data + steps * len_bytes, databuf,
+		remaining_units * unit_size);
     }
   else
     {
       /* Pattern is longer than or equal to count:
-         just copy count bytes.  */
+         just copy count addressable memory units.  */
       data = databuf;
     }
 
-  write_memory_with_notification (addr, data, count);
+  write_memory_with_notification (addr, data, count_units);
 
   do_cleanups (back_to);
 }
-- 
2.1.4

[PATCH v2 6/7] remote: consider addressable unit size when reading/writing memory

[Simon Marchi]

Adapt code in remote.c to take into account addressable unit size when
reading/writing memory.

A few variables are renamed and suffixed with _bytes or _units. This
way, it's more obvious if there is any place where we add or compare
values of different kinds (which would be a mistake).

gdb/ChangeLog:

	* common/rsp-low.c (needs_escaping): New.
	(remote_escape_output): Add unit_size parameter. Refactor to
	support multi-byte addressable units.  Rename parameters.
	* common/rsp-low.h (remote_escape_output): Add unit_size
	parameter and rename others. Update doc.
	* remote.c (align_for_efficient_write): New.
	(remote_write_bytes_aux): Add unit_size parameter and use it.
	Rename some variables.  Update doc.
	(remote_xfer_partial): Get unit size and use it.
	(remote_read_bytes_1): Add unit_size parameter and use it.
	Rename some variables.
	(remote_write_bytes): Same.
	(remote_xfer_live_readonly_partial): Same.
	(remote_read_bytes): Same.
	(remote_flash_write): Update call to remote_write_bytes_aux.
	(remote_write_qxfer): Update call to remote_escape_output.
	(remote_search_memory): Same.
	(remote_hostio_pwrite): Same.

gdb/gdbserver/ChangeLog:

	* server.c (write_qxfer_response): Update call to
	remote_escape_output.
---
 gdb/common/rsp-low.c   |  68 +++++++++++++------
 gdb/common/rsp-low.h   |  18 ++---
 gdb/gdbserver/server.c |   4 +-
 gdb/remote.c           | 174 +++++++++++++++++++++++++++----------------------
 4 files changed, 154 insertions(+), 110 deletions(-)

diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index d3d3d65..ba2ff0b 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -146,38 +146,64 @@ bin2hex (const gdb_byte *bin, char *hex, int count)
   return i;
 }
 
+static int needs_escaping (gdb_byte b)
+{
+  return b == '$' || b == '#' || b == '}' || b == '*';
+}
+
 /* See rsp-low.h.  */
 
 int
-remote_escape_output (const gdb_byte *buffer, int len,
-		      gdb_byte *out_buf, int *out_len,
-		      int out_maxlen)
+remote_escape_output (const gdb_byte *buffer, int len_units, int unit_size,
+		      gdb_byte *out_buf, int *out_len_units,
+		      int out_maxlen_bytes)
 {
-  int input_index, output_index;
-
-  output_index = 0;
-  for (input_index = 0; input_index < len; input_index++)
+  int input_unit_index, output_byte_index = 0, byte_index_in_unit;
+  int number_escape_bytes_needed;
+
+  /* Try to copy integral addressable memory units until
+     (1) we run out of space or
+     (2) we copied all of them.  */
+  for (input_unit_index = 0;
+       input_unit_index < len_units;
+       input_unit_index++)
     {
-      gdb_byte b = buffer[input_index];
-
-      if (b == '$' || b == '#' || b == '}' || b == '*')
+      /* Find out how many escape bytes we need for this unit.  */
+      number_escape_bytes_needed = 0;
+      for (byte_index_in_unit = 0;
+	   byte_index_in_unit < unit_size;
+	   byte_index_in_unit++)
 	{
-	  /* These must be escaped.  */
-	  if (output_index + 2 > out_maxlen)
-	    break;
-	  out_buf[output_index++] = '}';
-	  out_buf[output_index++] = b ^ 0x20;
+	  int idx = input_unit_index * unit_size + byte_index_in_unit;
+	  gdb_byte b = buffer[idx];
+	  if (needs_escaping (b))
+	    number_escape_bytes_needed++;
 	}
-      else
+
+      /* Check if we have room to fit this escaped unit.  */
+      if (output_byte_index + unit_size + number_escape_bytes_needed >
+	    out_maxlen_bytes)
+	  break;
+
+      /* Copy the unit byte per byte, adding escapes.  */
+      for (byte_index_in_unit = 0;
+	   byte_index_in_unit < unit_size;
+	   byte_index_in_unit++)
 	{
-	  if (output_index + 1 > out_maxlen)
-	    break;
-	  out_buf[output_index++] = b;
+	  int idx = input_unit_index * unit_size + byte_index_in_unit;
+	  gdb_byte b = buffer[idx];
+	  if (needs_escaping(b))
+	    {
+	      out_buf[output_byte_index++] = '}';
+	      out_buf[output_byte_index++] = b ^ 0x20;
+	    }
+	  else
+	    out_buf[output_byte_index++] = b;
 	}
     }
 
-  *out_len = input_index;
-  return output_index;
+  *out_len_units = input_unit_index;
+  return output_byte_index;
 }
 
 /* See rsp-low.h.  */
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index d62f67e..f45d0ae 100644
--- a/gdb/common/rsp-low.h
+++ b/gdb/common/rsp-low.h
@@ -59,17 +59,17 @@ extern int hex2bin (const char *hex, gdb_byte *bin, int count);
 
 extern int bin2hex (const gdb_byte *bin, char *hex, int count);
 
-/* Convert BUFFER, binary data at least LEN bytes long, into escaped
-   binary data in OUT_BUF.  Set *OUT_LEN to the length of the data
-   encoded in OUT_BUF, and return the number of bytes in OUT_BUF
-   (which may be more than *OUT_LEN due to escape characters).  The
-   total number of bytes in the output buffer will be at most
-   OUT_MAXLEN.  This function properly escapes '*', and so is suitable
+/* Convert BUFFER, binary data at least LEN_UNITS addressable memory units
+   long, into escaped binary data in OUT_BUF.  Only copy memory units that fit
+   completely in OUT_BUF.  Set *OUT_LEN_UNITS to the number of units from
+   BUFFER successfully encoded in OUT_BUF, and return the number of bytes used
+   in OUT_BUF.  The total number of bytes in the output buffer will be at most
+   OUT_MAXLEN_BYTES.  This function properly escapes '*', and so is suitable
    for the server side as well as the client.  */
 
-extern int remote_escape_output (const gdb_byte *buffer, int len,
-				 gdb_byte *out_buf, int *out_len,
-				 int out_maxlen);
+extern int remote_escape_output (const gdb_byte *buffer, int len_units,
+				 int unit_size, gdb_byte *out_buf,
+				 int *out_len_units, int out_maxlen_bytes);
 
 /* Convert BUFFER, escaped data LEN bytes long, into binary data
    in OUT_BUF.  Return the number of bytes written to OUT_BUF.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3408ef7..367379e 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -383,8 +383,8 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
   else
     buf[0] = 'l';
 
-  return remote_escape_output (data, len, (unsigned char *) buf + 1, &out_len,
-			       PBUFSIZ - 2) + 1;
+  return remote_escape_output (data, len, 1, (unsigned char *) buf + 1,
+			       &out_len, PBUFSIZ - 2) + 1;
 }
 
 /* Handle btrace enabling in BTS format.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index dcd24c4..4a1f72d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6639,51 +6639,61 @@ check_binary_download (CORE_ADDR addr)
     }
 }
 
+/* Helper function to resize the payload in order to try to get a good
+   alignment.  We try to write an amount of data such that the next write will
+   start on an address aligned on REMOTE_ALIGN_WRITES.  */
+
+static int
+align_for_efficient_write (int todo, CORE_ADDR memaddr)
+{
+  return ((memaddr + todo) & ~(REMOTE_ALIGN_WRITES - 1)) - memaddr;
+}
+
 /* Write memory data directly to the remote machine.
    This does not inform the data cache; the data cache uses this.
    HEADER is the starting part of the packet.
    MEMADDR is the address in the remote memory space.
    MYADDR is the address of the buffer in our space.
-   LEN is the number of bytes.
+   LEN_UNITS is the number of addressable units.
+   UNIT_SIZE is the length in bytes of an addressable unit.
    PACKET_FORMAT should be either 'X' or 'M', and indicates if we
    should send data as binary ('X'), or hex-encoded ('M').
 
    The function creates packet of the form
        <HEADER><ADDRESS>,<LENGTH>:<DATA>
 
-   where encoding of <DATA> is termined by PACKET_FORMAT.
+   where encoding of <DATA> is terminated by PACKET_FORMAT.
 
    If USE_LENGTH is 0, then the <LENGTH> field and the preceding comma
    are omitted.
 
    Return the transferred status, error or OK (an
-   'enum target_xfer_status' value).  Save the number of bytes
-   transferred in *XFERED_LEN.  Only transfer a single packet.  */
+   'enum target_xfer_status' value).  Save the number of addressable units
+   transferred in *XFERED_LEN_UNITS.  Only transfer a single packet.  */
 
 static enum target_xfer_status
 remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
-			const gdb_byte *myaddr, ULONGEST len,
-			ULONGEST *xfered_len, char packet_format,
-			int use_length)
+			const gdb_byte *myaddr, ULONGEST len_units,
+			int unit_size, ULONGEST *xfered_len_units,
+			char packet_format, int use_length)
 {
   struct remote_state *rs = get_remote_state ();
   char *p;
   char *plen = NULL;
   int plenlen = 0;
-  int todo;
-  int nr_bytes;
-  int payload_size;
-  int payload_length;
-  int header_length;
+  int todo_units;
+  int units_written;
+  int payload_capacity_bytes;
+  int payload_length_bytes;
 
   if (packet_format != 'X' && packet_format != 'M')
     internal_error (__FILE__, __LINE__,
 		    _("remote_write_bytes_aux: bad packet format"));
 
-  if (len == 0)
+  if (len_units == 0)
     return TARGET_XFER_EOF;
 
-  payload_size = get_memory_write_packet_size ();
+  payload_capacity_bytes = get_memory_write_packet_size ();
 
   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */
@@ -6692,13 +6702,12 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   /* Compute the size of the actual payload by subtracting out the
      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */
 
-  payload_size -= strlen ("$,:#NN");
+  payload_capacity_bytes -= strlen ("$,:#NN");
   if (!use_length)
     /* The comma won't be used.  */
-    payload_size += 1;
-  header_length = strlen (header);
-  payload_size -= header_length;
-  payload_size -= hexnumlen (memaddr);
+    payload_capacity_bytes += 1;
+  payload_capacity_bytes -= strlen (header);
+  payload_capacity_bytes -= hexnumlen (memaddr);
 
   /* Construct the packet excluding the data: "<header><memaddr>,<len>:".  */
 
@@ -6709,28 +6718,30 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   if (packet_format == 'X')
     {
       /* Best guess at number of bytes that will fit.  */
-      todo = min (len, payload_size);
+      todo_units = min (len_units, payload_capacity_bytes / unit_size);
       if (use_length)
-	payload_size -= hexnumlen (todo);
-      todo = min (todo, payload_size);
+	/* The size in the packet is expressed in bytes, not addressable
+	   units.  */
+	payload_capacity_bytes -= hexnumlen (todo_units * unit_size);
+      todo_units = min (todo_units, payload_capacity_bytes / unit_size);
     }
   else
     {
-      /* Num bytes that will fit.  */
-      todo = min (len, payload_size / 2);
+      /* Number of bytes that will fit.  */
+      todo_units = min (len_units, (payload_capacity_bytes / unit_size) / 2);
       if (use_length)
-	payload_size -= hexnumlen (todo);
-      todo = min (todo, payload_size / 2);
+	payload_capacity_bytes -= hexnumlen (todo_units * unit_size);
+      todo_units = min (todo_units, (payload_capacity_bytes / unit_size) / 2);
     }
 
-  if (todo <= 0)
+  if (todo_units <= 0)
     internal_error (__FILE__, __LINE__,
 		    _("minimum packet size too small to write data"));
 
   /* If we already need another packet, then try to align the end
      of this packet to a useful boundary.  */
-  if (todo > 2 * REMOTE_ALIGN_WRITES && todo < len)
-    todo = ((memaddr + todo) & ~(REMOTE_ALIGN_WRITES - 1)) - memaddr;
+  if (todo_units > 2 * REMOTE_ALIGN_WRITES && todo_units < len_units)
+    todo_units = align_for_efficient_write (todo_units, memaddr);
 
   /* Append "<memaddr>".  */
   memaddr = remote_address_masked (memaddr);
@@ -6741,10 +6752,10 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Append ",".  */
       *p++ = ',';
 
-      /* Append <len>.  Retain the location/size of <len>.  It may need to
-	 be adjusted once the packet body has been created.  */
+      /* Append <len_units>.  Retain the location/size of the length.  It may
+         need to be adjusted once the packet body has been created.  */
       plen = p;
-      plenlen = hexnumstr (p, (ULONGEST) todo);
+      plenlen = hexnumstr (p, (ULONGEST) todo_units * unit_size);
       p += plenlen;
     }
 
@@ -6758,32 +6769,35 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Binary mode.  Send target system values byte by byte, in
 	 increasing byte addresses.  Only escape certain critical
 	 characters.  */
-      payload_length = remote_escape_output (myaddr, todo, (gdb_byte *) p,
-					     &nr_bytes, payload_size);
+      payload_length_bytes =
+	  remote_escape_output (myaddr, todo_units, unit_size, (gdb_byte *) p,
+				&units_written, payload_capacity_bytes);
 
-      /* If not all TODO bytes fit, then we'll need another packet.  Make
+      /* If not all TODO units fit, then we'll need another packet.  Make
 	 a second try to keep the end of the packet aligned.  Don't do
 	 this if the packet is tiny.  */
-      if (nr_bytes < todo && nr_bytes > 2 * REMOTE_ALIGN_WRITES)
+      if (units_written < todo_units && units_written > 2 * REMOTE_ALIGN_WRITES)
 	{
-	  int new_nr_bytes;
-
-	  new_nr_bytes = (((memaddr + nr_bytes) & ~(REMOTE_ALIGN_WRITES - 1))
-			  - memaddr);
-	  if (new_nr_bytes != nr_bytes)
-	    payload_length = remote_escape_output (myaddr, new_nr_bytes,
-						   (gdb_byte *) p, &nr_bytes,
-						   payload_size);
+	  int new_todo_units;
+
+	  new_todo_units = align_for_efficient_write (units_written, memaddr);
+
+	  if (new_todo_units != units_written)
+	    payload_length_bytes =
+		remote_escape_output (myaddr, new_todo_units, unit_size,
+				      (gdb_byte *) p, &units_written,
+				      payload_capacity_bytes);
 	}
 
-      p += payload_length;
-      if (use_length && nr_bytes < todo)
+      p += payload_length_bytes;
+      if (use_length && units_written < todo_units)
 	{
 	  /* Escape chars have filled up the buffer prematurely,
-	     and we have actually sent fewer bytes than planned.
+	     and we have actually sent fewer units than planned.
 	     Fix-up the length field of the packet.  Use the same
 	     number of characters as before.  */
-	  plen += hexnumnstr (plen, (ULONGEST) nr_bytes, plenlen);
+	  plen += hexnumnstr (plen, (ULONGEST) units_written * unit_size,
+			      plenlen);
 	  *plen = ':';  /* overwrite \0 from hexnumnstr() */
 	}
     }
@@ -6792,8 +6806,8 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Normal mode: Send target system values byte by byte, in
 	 increasing byte addresses.  Each byte is encoded as a two hex
 	 value.  */
-      nr_bytes = bin2hex (myaddr, p, todo);
-      p += 2 * nr_bytes;
+      p += 2 * bin2hex (myaddr, p, todo_units * unit_size);
+      units_written = todo_units;
     }
 
   putpkt_binary (rs->buf, (int) (p - rs->buf));
@@ -6802,9 +6816,9 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   if (rs->buf[0] == 'E')
     return TARGET_XFER_E_IO;
 
-  /* Return NR_BYTES, not TODO, in case escape chars caused us to send
-     fewer bytes than we'd planned.  */
-  *xfered_len = (ULONGEST) nr_bytes;
+  /* Return UNITS_WRITTEN, not TODO_UNITS, in case escape chars caused us to
+     send fewer units than we'd planned.  */
+  *xfered_len_units = (ULONGEST) units_written;
   return TARGET_XFER_OK;
 }
 
@@ -6820,7 +6834,7 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
 
 static enum target_xfer_status
 remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
-		    ULONGEST *xfered_len)
+		    int unit_size, ULONGEST *xfered_len)
 {
   char *packet_format = 0;
 
@@ -6843,7 +6857,7 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
     }
 
   return remote_write_bytes_aux (packet_format,
-				 memaddr, myaddr, len, xfered_len,
+				 memaddr, myaddr, len, unit_size, xfered_len,
 				 packet_format[0], 1);
 }
 
@@ -6858,21 +6872,21 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
    transferred in *XFERED_LEN.  */
 
 static enum target_xfer_status
-remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
-		     ULONGEST *xfered_len)
+remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len_units,
+		     int unit_size, ULONGEST *xfered_len_units)
 {
   struct remote_state *rs = get_remote_state ();
-  int max_buf_size;		/* Max size of packet output buffer.  */
+  int buf_size_bytes;		/* Max size of packet output buffer.  */
   char *p;
-  int todo;
-  int i;
+  int todo_units;
+  int decoded_bytes;
 
-  max_buf_size = get_memory_read_packet_size ();
+  buf_size_bytes = get_memory_read_packet_size ();
   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */
 
-  /* Number if bytes that will fit.  */
-  todo = min (len, max_buf_size / 2);
+  /* Number of bytes that will fit.  */
+  todo_units = min (len_units, (buf_size_bytes / unit_size) / 2);
 
   /* Construct "m"<memaddr>","<len>".  */
   memaddr = remote_address_masked (memaddr);
@@ -6880,7 +6894,7 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
   *p++ = 'm';
   p += hexnumstr (p, (ULONGEST) memaddr);
   *p++ = ',';
-  p += hexnumstr (p, (ULONGEST) todo);
+  p += hexnumstr (p, (ULONGEST) todo_units * unit_size);
   *p = '\0';
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -6891,9 +6905,9 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
   /* Reply describes memory byte by byte, each byte encoded as two hex
      characters.  */
   p = rs->buf;
-  i = hex2bin (p, myaddr, todo);
+  decoded_bytes = hex2bin (p, myaddr, todo_units * unit_size);
   /* Return what we have.  Let higher layers handle partial reads.  */
-  *xfered_len = (ULONGEST) i;
+  *xfered_len_units = (ULONGEST) (decoded_bytes / unit_size);
   return TARGET_XFER_OK;
 }
 
@@ -6906,7 +6920,7 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
 static enum target_xfer_status
 remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 				   ULONGEST memaddr, ULONGEST len,
-				   ULONGEST *xfered_len)
+				   int unit_size, ULONGEST *xfered_len)
 {
   struct target_section *secp;
   struct target_section_table *table;
@@ -6929,7 +6943,7 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 	      if (memend <= p->endaddr)
 		{
 		  /* Entire transfer is within this section.  */
-		  return remote_read_bytes_1 (memaddr, readbuf, len,
+		  return remote_read_bytes_1 (memaddr, readbuf, len, unit_size,
 					      xfered_len);
 		}
 	      else if (memaddr >= p->endaddr)
@@ -6941,7 +6955,7 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 		{
 		  /* This section overlaps the transfer.  Just do half.  */
 		  len = p->endaddr - memaddr;
-		  return remote_read_bytes_1 (memaddr, readbuf, len,
+		  return remote_read_bytes_1 (memaddr, readbuf, len, unit_size,
 					      xfered_len);
 		}
 	    }
@@ -6957,7 +6971,8 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 
 static enum target_xfer_status
 remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
-		   gdb_byte *myaddr, ULONGEST len, ULONGEST *xfered_len)
+		   gdb_byte *myaddr, ULONGEST len, int unit_size,
+		   ULONGEST *xfered_len)
 {
   if (len == 0)
     return TARGET_XFER_EOF;
@@ -6995,7 +7010,7 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
 
 	      /* This goes through the topmost target again.  */
 	      res = remote_xfer_live_readonly_partial (ops, myaddr, memaddr,
-						       len, xfered_len);
+						       len, unit_size, xfered_len);
 	      if (res == TARGET_XFER_OK)
 		return TARGET_XFER_OK;
 	      else
@@ -7017,7 +7032,7 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
 	}
     }
 
-  return remote_read_bytes_1 (memaddr, myaddr, len, xfered_len);
+  return remote_read_bytes_1 (memaddr, myaddr, len, unit_size, xfered_len);
 }
 
 \f
@@ -7103,7 +7118,7 @@ remote_flash_write (struct target_ops *ops, ULONGEST address,
 					  &saved_remote_timeout);
 
   remote_timeout = remote_flash_timeout;
-  ret = remote_write_bytes_aux ("vFlashWrite:", address, data, length,
+  ret = remote_write_bytes_aux ("vFlashWrite:", address, data, length, 1,
 				xfered_len,'X', 0);
   do_cleanups (back_to);
 
@@ -8775,7 +8790,7 @@ remote_write_qxfer (struct target_ops *ops, const char *object_name,
 
   /* Escape as much data as fits into rs->buf.  */
   buf_len = remote_escape_output 
-    (writebuf, len, (gdb_byte *) rs->buf + i, &max_size, max_size);
+    (writebuf, len, 1, (gdb_byte *) rs->buf + i, &max_size, max_size);
 
   if (putpkt_binary (rs->buf, i + buf_len) < 0
       || getpkt_sane (&rs->buf, &rs->buf_size, 0) < 0
@@ -8886,6 +8901,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
   int i;
   char *p2;
   char query_type;
+  int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());
 
   set_remote_traceframe ();
   set_general_thread (inferior_ptid);
@@ -8902,9 +8918,11 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
 	return TARGET_XFER_EOF;
 
       if (writebuf != NULL)
-	return remote_write_bytes (offset, writebuf, len, xfered_len);
+	return remote_write_bytes (offset, writebuf, len, unit_size,
+				   xfered_len);
       else
-	return remote_read_bytes (ops, offset, readbuf, len, xfered_len);
+	return remote_read_bytes (ops, offset, readbuf, len, unit_size,
+				  xfered_len);
     }
 
   /* Handle SPU memory using qxfer packets.  */
@@ -9137,7 +9155,7 @@ remote_search_memory (struct target_ops* ops,
 
   /* Escape as much data as fits into rs->buf.  */
   escaped_pattern_len =
-    remote_escape_output (pattern, pattern_len, (gdb_byte *) rs->buf + i,
+    remote_escape_output (pattern, pattern_len, 1, (gdb_byte *) rs->buf + i,
 			  &used_pattern_len, max_size);
 
   /* Bail if the pattern is too large.  */
@@ -9922,7 +9940,7 @@ remote_hostio_pwrite (struct target_ops *self,
   remote_buffer_add_int (&p, &left, offset);
   remote_buffer_add_string (&p, &left, ",");
 
-  p += remote_escape_output (write_buf, len, (gdb_byte *) p, &out_len,
+  p += remote_escape_output (write_buf, len, 1, (gdb_byte *) p, &out_len,
 			     get_remote_packet_size () - (p - rs->buf));
 
   return remote_hostio_send_command (p - rs->buf, PACKET_vFile_pwrite,
-- 
2.1.4

[PATCH v2 4/7] gdbarch: add addressable_memory_unit_size method

[Simon Marchi]

Add a new gdbarch method to get the length of an addressable memory unit
for a given architecture. The default implementation returns 1.

gdb/ChangeLog:

	* arch-utils.h (default_addressable_memory_unit_size): New.
	* arch-utils.c (default_addressable_memory_unit_size): New.
	* gdbarch.sh (addressable_memory_unit_size): New.
	* gdbarch.h: Re-generated.
	* gdbarch.c: Re-generated.
---
 gdb/arch-utils.c |  9 +++++++++
 gdb/arch-utils.h |  1 +
 gdb/gdbarch.c    | 23 +++++++++++++++++++++++
 gdb/gdbarch.h    |  7 +++++++
 gdb/gdbarch.sh   |  5 +++++
 5 files changed, 45 insertions(+)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index e1c8ab0..3bb182e 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -882,6 +882,15 @@ default_gnu_triplet_regexp (struct gdbarch *gdbarch)
   return gdbarch_bfd_arch_info (gdbarch)->arch_name;
 }
 
+/* Default method for gdbarch_addressable_memory_unit_size.  */
+
+int
+default_addressable_memory_unit_size (struct gdbarch *gdbarch)
+{
+  return 1;
+}
+
 /* -Wmissing-prototypes */
 extern initialize_file_ftype _initialize_gdbarch_utils;
 
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index ed3bec9..b2d20c6 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -202,5 +202,6 @@ extern void default_skip_permanent_breakpoint (struct regcache *regcache);
 extern CORE_ADDR default_infcall_mmap (CORE_ADDR size, unsigned prot);
 extern char *default_gcc_target_options (struct gdbarch *gdbarch);
 extern const char *default_gnu_triplet_regexp (struct gdbarch *gdbarch);
+extern int default_addressable_memory_unit_size (struct gdbarch *gdbarch);
 
 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 97874c9..1cdc2df 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -328,6 +328,7 @@ struct gdbarch
   gdbarch_infcall_mmap_ftype *infcall_mmap;
   gdbarch_gcc_target_options_ftype *gcc_target_options;
   gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp;
+  gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -428,6 +429,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->infcall_mmap = default_infcall_mmap;
   gdbarch->gcc_target_options = default_gcc_target_options;
   gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
+  gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   /* gdbarch_alloc() */
 
   return gdbarch;
@@ -660,6 +662,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of infcall_mmap, invalid_p == 0 */
   /* Skip verify of gcc_target_options, invalid_p == 0 */
   /* Skip verify of gnu_triplet_regexp, invalid_p == 0 */
+  /* Skip verify of addressable_memory_unit_size, invalid_p == 0 */
   buf = ui_file_xstrdup (log, &length);
   make_cleanup (xfree, buf);
   if (length > 0)
@@ -711,6 +714,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: address_to_pointer = <%s>\n",
                       host_address_to_string (gdbarch->address_to_pointer));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: addressable_memory_unit_size = <%s>\n",
+                      host_address_to_string (gdbarch->addressable_memory_unit_size));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_adjust_breakpoint_address_p() = %d\n",
                       gdbarch_adjust_breakpoint_address_p (gdbarch));
   fprintf_unfiltered (file,
@@ -4707,6 +4713,23 @@ set_gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch,
   gdbarch->gnu_triplet_regexp = gnu_triplet_regexp;
 }
 
+int
+gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->addressable_memory_unit_size != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_addressable_memory_unit_size called\n");
+  return gdbarch->addressable_memory_unit_size (gdbarch);
+}
+
+void
+set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch,
+                                          gdbarch_addressable_memory_unit_size_ftype addressable_memory_unit_size)
+{
+  gdbarch->addressable_memory_unit_size = addressable_memory_unit_size;
+}
+
 
 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index c94c19c..79197a4 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1459,6 +1459,13 @@ typedef const char * (gdbarch_gnu_triplet_regexp_ftype) (struct gdbarch *gdbarch
 extern const char * gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch);
 extern void set_gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch, gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp);
 
+/* Return the size in bytes of an addressable memory unit on this architecture.
+   This corresponds to the number of bytes associated to each address in memory. */
+
+typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarch);
+extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
+extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
+
 /* Definition for an unknown syscall, used basically in error-cases.  */
 #define UNKNOWN_SYSCALL (-1)
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 0f303a4..43b6adc 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1109,6 +1109,11 @@ m:char *:gcc_target_options:void:::default_gcc_target_options::0
 # returns the BFD architecture name, which is correct in nearly every
 # case.
 m:const char *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0
+
+# Return the size in bytes of an addressable memory unit on this architecture.
+# This corresponds to the number of bytes associated to each address in memory.
+m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit_size::0
+
 EOF
 }
 
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 15 Apr 2015 15:47:34 -0400
> 
> New in v2:
> 
>  * Change wording: use byte for 8-bits chunks and addressable memory unit
>    for the unit of data associated to a single address.
>  * Introduce definition of addressable memory unit in the Memory
>    section.
> 
> This patch modifies the manual to clarify the MI, RSP and Python APIs in
> regard to reading/writing memory on architectures with addressable
> memory unit that are not 8 bits.
> 
> Care is taken to use the word "addressable memory unit" or "memory unit"
> when referring to one piece of the smallest addressable size on the
> current architecture and the word "byte" when referring to an 8-bits
> data piece.

This patch is fine with me, thanks.

Re: [PATCH v2 0/7] Support reading/writing memory on architectures with non 8-bits addressable memory

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> 
> For RSP's m, M and X packets, the "length" parameters are used to
> correctly encode or decode the packet at a low level, where we don't
> want to have to deal with different target byte sizes. Also, for a
> network protocol, it would make sense to use a fixed-sized unit.
> Therefore, it would be easier if those lengths were always in bytes.
> Here is an example that corresponds to the previous MI example.

This confuses me and gives me lots of pause.  My immediate reaction
is, "well, that's odd. what's different compared to MI here?".  I'm
not imagining what exactly ends up being easier?

But anyway, I guess it's a small detail.  I'll review ignoring
that.

> 
>    -> $m1000,8#??
>    <- aaaabbbbccccdddd
> 
>    -> $M1000,6:eeeeffffeeee#??
>    <- OK
> 
>    -> $m1000,8#??
>    <- eeeeffffeeeedddd
> 
> If there are any other RSP packets or MI commands that need such
> clarification, it will be on a case-by-case basis, whatever makes more
> sense for each particular one.

Off hand, I thought of qCRC and qSearch:memory.  The latter is
more interesting:

- Would you allow searching for an 1 8-bit byte pattern?

- So what length would you use for that one?  Host byte
  or addressable units?

Thanks,
Pedro Alves

Re: [PATCH v2 2/7] Cleanup some docs about memory write

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> Some docs seemed outdated. In the case of target_write_memory, the docs
> in target.c and target/target.h diverged a bit, so I tried to find a
> reasonnable in-between version.

"reasonable"

> 
> gdb/ChangeLog:
> 
> 	* corefile.c (write_memory): Update doc.
> 	* gdbcore.h (write_memory): Same.
> 	* target.c (target_write_memory): Same.
> 	* target/target.h (target_write_memory): Same.
> ---
>  gdb/corefile.c      |  4 ++--
>  gdb/gdbcore.h       |  6 ++----
>  gdb/target.c        |  6 +-----
>  gdb/target/target.h | 10 +++++-----
>  4 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index a042e6d..83b0e80 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
>    return extract_typed_address (buf, type);
>  }
>  
> -/* Same as target_write_memory, but report an error if can't
> -   write.  */
> +/* See gdbcore.h. */

Double space after period.

> diff --git a/gdb/target.c b/gdb/target.c
> index fcf7090..bd9a0eb 100644
>  int
>  target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)

> diff --git a/gdb/target/target.h b/gdb/target/target.h
> index 05ac758..e0b7554 100644
> --- a/gdb/target/target.h
> +++ b/gdb/target/target.h
> @@ -49,12 +49,12 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
>  extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
>  
>  /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
> -   Return zero for success, nonzero if any error occurs.  This
> +   Return zero for success or TARGET_XFER_E_IO if any error occurs.  This
>     function must be provided by the client.  Implementations of this
> -   function may define and use their own error codes, but functions
> -   in the common, nat and target directories must treat the return
> -   code as opaque.  No guarantee is made about the contents of the
> -   data at MEMADDR if any error occurs.  */
> +   function may define and use their own error codes, but functions in
> +   the common, nat and target directories must treat the return code as
> +   opaque.  No guarantee is made about how much data got written if any
> +   error occurs.  */
>  
>  extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  				ssize_t len);
> 

This one's not right.  TARGET_XFER_E_IO is a GDB-specific thing, while
this declaration is meant for both gdb and gdbserver.  It's what
"This function must be provided by the client" refers to.  It doesn't
make sense to say "TARGET_XFER_E_IO" one error and then explain that
the return code is opaque.

So probably it'd be better to leave this comment:

> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1464,11 +1464,7 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>      return TARGET_XFER_E_IO;
>  }
>
> -/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
> -   Returns either 0 for success or TARGET_XFER_E_IO if any
> -   error occurs.  If an error occurs, no guarantee is made about how
> -   much data got written.  Callers that can deal with partial writes
> -   should call target_write.  */
> +/* See target/target.h.  */

... but extend it by starting by saying that this is GDB's implementation
of the function as declared in target/target.h.

Thanks,
Pedro Alves

Re: [PATCH v2 1/7] Various cleanups in target read/write code

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> This contains various cleanups in the target memory read and write code.
> They are not directly related to the non-8-bits changes, but they
> clarify things a bit down the line.
> 
> gdb/ChangeLog:
> 
> 	* target.c (target_read): Rename variables and use
> 	TARGET_XFER_E_IO.
> 	(target_read_with_progress): Same.
> 	(read_memory_robust): Constify parameters and rename
> 	variables.
> 	(read_whatever_is_readable): Constify parameters,
> 	rename variables, adjust formatting.
> 	* target.h (read_memory_robust): Constify parameters.

OK.

Thanks,
Pedro Alves

Re: [PATCH v2 4/7] gdbarch: add addressable_memory_unit_size method

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> Add a new gdbarch method to get the length of an addressable memory unit
> for a given architecture. The default implementation returns 1.
> 

> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1109,6 +1109,11 @@ m:char *:gcc_target_options:void:::default_gcc_target_options::0
>  # returns the BFD architecture name, which is correct in nearly every
>  # case.
>  m:const char *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0
> +
> +# Return the size in bytes of an addressable memory unit on this architecture.
> +# This corresponds to the number of bytes associated to each address in memory.
> +m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit_size::0

This is the central place everyone should look at first to understand
non 8-bit bytes targets.  For extra clarity, could you please
add "8-bit"s in there, say:

 "the size in 8-bit bytes" (...) to the number of 8-bit bytes associated to
 each address in memory."

Looks good to me with that change.

Thanks,
Pedro Alves

Re: [PATCH v2 5/7] target: consider addressable unit size when reading/writing memory

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> If we are reading/writing from a memory object, the length represents
> the number of "addresses" to read/write, so the addressable unit size
> needs to be taken into account when allocating memory on gdb's side.
> 
> gdb/ChangeLog:
> 
> 	* target.c (target_read): Consider addressable unit size when
> 	reading from a memory object.
> 	(read_memory_robust): Same.
> 	(read_whatever_is_readable): Same.
> 	(target_write_with_progress): Consider addressable unit size
> 	when writing to a memory object.
> 	* target.h (target_read): Update documentation.
> 	(target_write): Add documentation.

> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1589,6 +1589,15 @@ target_read (struct target_ops *ops,
>  	     ULONGEST offset, LONGEST len)
>  {
>    LONGEST xfered_total = 0;
> +  int unit_size = 1;
> +
> +  /* If we are reading from a memory object, find the length of an addressable
> +     unit for that architecture.  */
> +  if (object == TARGET_OBJECT_MEMORY
> +      || object == TARGET_OBJECT_STACK_MEMORY
> +      || object == TARGET_OBJECT_CODE_MEMORY
> +      || object == TARGET_OBJECT_RAW_MEMORY)
> +    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch());

Missing space before parens.  Otherwise looks OK.

Thanks,
Pedro Alves

Re: [PATCH v2 6/7] remote: consider addressable unit size when reading/writing memory

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> Adapt code in remote.c to take into account addressable unit size when
> reading/writing memory.
> 
> A few variables are renamed and suffixed with _bytes or _units. This
> way, it's more obvious if there is any place where we add or compare
> values of different kinds (which would be a mistake).

You should put here some of the info you have on the series intro.
Even better in comments in the code even.

E.g., this example here:

> For RSP's m, M and X packets, the "length" parameters are used to
> correctly encode or decode the packet at a low level, where we don't
> want to have to deal with different target byte sizes. Also, for a
> network protocol, it would make sense to use a fixed-sized unit.
> Therefore, it would be easier if those lengths were always in bytes.
> Here is an example that corresponds to the previous MI example.
>
>    -> $m1000,8#??
>    <- aaaabbbbccccdddd
>
>    -> $M1000,6:eeeeffffeeee#??
>    <- OK
>
>    -> $m1000,8#??
>    <- eeeeffffeeeedddd
>



> --- a/gdb/common/rsp-low.c
> +++ b/gdb/common/rsp-low.c
> @@ -146,38 +146,64 @@ bin2hex (const gdb_byte *bin, char *hex, int count)
>    return i;
>  }
>  
> +static int needs_escaping (gdb_byte b)

Line break before 'needs_escaping'.  Missing intro comment.

> +{
> +  return b == '$' || b == '#' || b == '}' || b == '*';
> +}
> +


> -	  if (output_index + 1 > out_maxlen)
> -	    break;
> -	  out_buf[output_index++] = b;
> +	  int idx = input_unit_index * unit_size + byte_index_in_unit;
> +	  gdb_byte b = buffer[idx];
> +	  if (needs_escaping(b))

Space before parens.

> +	    {

Otherwise looks good.

Thanks,
Pedro Alves

Re: [PATCH v2 7/7] MI: consider addressable unit size when reading/writing memory

[Pedro Alves]

On 04/15/2015 08:47 PM, Simon Marchi wrote:
> As a user of the target memory read/write interface, the MI code must
> adjust its memory allocations to take into account the addressable memory
> unitsize of the target.

Looks OK to me.

Thanks,
Pedro Alves

Re: [PATCH v2 0/7] Support reading/writing memory on architectures with non 8-bits addressable memory

[Simon Marchi]

On 15-05-21 01:44 PM, Pedro Alves wrote:
> This confuses me and gives me lots of pause.  My immediate reaction
> is, "well, that's odd. what's different compared to MI here?".  I'm
> not imagining what exactly ends up being easier?
>
> But anyway, I guess it's a small detail.  I'll review ignoring
> that.

I was trying to reply and explain the logic behind my choice, but the
more I developed, the more I felt it was not really founded.

My reasoning about the difference was the following:

"""
In RSP, the size is there for GDB to tell the target "here is how much data
I am handing you". Its function is to describe the data found in the packet.
It's more there as a matter convenience, saying how much data follows, than
representing an amount of target memory units.

In MI, the size is not describing the amount of data you are handing GDB. It
represents a size in the context of the memory of the target.
"""

However, the paragraph about RSP doesn't make much sense. We are the ones
defining the protocol, so the field can mean whatever we want. If we want it
to be in target addressable memory units, then there is nothing that prevents
that. It just needs to be well documented. Also, I realize that writing memory
would be the only place where we would be talking about the target's memory in
bytes, so it's not so really consistent.

About the "it is easier" part, I initially found that the common part of gdbserver
would be less encumbered with code related to this if those sizes were in bytes.
Namely,

 * decode_M_packet
 * decode_X_packet
 * write_inferior_memory
 * read_inferior_memory
 * code related to this in process_serial_event

could just operate on bytes and work without knowledge of the addressable memory unit.
However, upon closer inspection, it seems like other functions will need to be aware
of it anyway, such as check_mem_read and check_mem_write. These two functions do
computations on addresses, so they need to have the length of the read/write in
addressable units and not bytes. If we do it for these two functions, then it's
not much more work to do it cleanly for the other functions as well.

Here is a draft of how the changes would look like in gdbserver when using addressable
memory units. It's really not that bad I think.

https://github.com/simark/binutils-gdb/commit/2ecb2f054a288053e3726e92fb6126dd4c782a15

So in the end, it might be more consistent to use addressable memory units everywhere
in the RSP, and not more complicated to implement. Of course, that's only for things
related to the target memory, things that fetch an XML file would still be in bytes.

What is your opinion on this?

>>
>>    -> $m1000,8#??
>>    <- aaaabbbbccccdddd
>>
>>    -> $M1000,6:eeeeffffeeee#??
>>    <- OK
>>
>>    -> $m1000,8#??
>>    <- eeeeffffeeeedddd
>>
>> If there are any other RSP packets or MI commands that need such
>> clarification, it will be on a case-by-case basis, whatever makes more
>> sense for each particular one.
> 
> Off hand, I thought of qCRC and qSearch:memory.  The latter is
> more interesting:
> 
> - Would you allow searching for an 1 8-bit byte pattern?

Hmm I don't know. To be safe I'd say no. If we do, it means we need to
search with a granularity of a byte. What if you search for the pattern
0x2345 in this memory:

0x100 0123
0x101 4567
0x102 89ab
0x103 cdef

Should there be a match that spans halves of two addresses? Unless we only
search with a byte granularity in the special case where the pattern is
one byte long? But then what about 3-bytes patterns?

I think it's a lot of corner cases for not much value. I think it could be
enhanced later to support it if somebody needs it.

> - So what length would you use for that one?  Host byte
>   or addressable units?

Length here would be in addressable units.

> Thanks,
> Pedro Alves
> 

Thanks,

Simon

Re: [PATCH v2 0/7] Support reading/writing memory on architectures with non 8-bits addressable memory

[Simon Marchi]

On 15-06-11 05:06 PM, Simon Marchi wrote:
> On 15-05-21 01:44 PM, Pedro Alves wrote:
>> This confuses me and gives me lots of pause.  My immediate reaction
>> is, "well, that's odd. what's different compared to MI here?".  I'm
>> not imagining what exactly ends up being easier?
>>
>> But anyway, I guess it's a small detail.  I'll review ignoring
>> that.
> 
> I was trying to reply and explain the logic behind my choice, but the
> more I developed, the more I felt it was not really founded.
> 
> My reasoning about the difference was the following:
> 
> """
> In RSP, the size is there for GDB to tell the target "here is how much data
> I am handing you". Its function is to describe the data found in the packet.
> It's more there as a matter convenience, saying how much data follows, than
> representing an amount of target memory units.
> 
> In MI, the size is not describing the amount of data you are handing GDB. It
> represents a size in the context of the memory of the target.
> """
> 
> However, the paragraph about RSP doesn't make much sense. We are the ones
> defining the protocol, so the field can mean whatever we want. If we want it
> to be in target addressable memory units, then there is nothing that prevents
> that. It just needs to be well documented. Also, I realize that writing memory
> would be the only place where we would be talking about the target's memory in
> bytes, so it's not so really consistent.
> 
> About the "it is easier" part, I initially found that the common part of gdbserver
> would be less encumbered with code related to this if those sizes were in bytes.
> Namely,
> 
>  * decode_M_packet
>  * decode_X_packet
>  * write_inferior_memory
>  * read_inferior_memory
>  * code related to this in process_serial_event
> 
> could just operate on bytes and work without knowledge of the addressable memory unit.
> However, upon closer inspection, it seems like other functions will need to be aware
> of it anyway, such as check_mem_read and check_mem_write. These two functions do
> computations on addresses, so they need to have the length of the read/write in
> addressable units and not bytes. If we do it for these two functions, then it's
> not much more work to do it cleanly for the other functions as well.
> 
> Here is a draft of how the changes would look like in gdbserver when using addressable
> memory units. It's really not that bad I think.
> 
> https://github.com/simark/binutils-gdb/commit/2ecb2f054a288053e3726e92fb6126dd4c782a15
> 
> So in the end, it might be more consistent to use addressable memory units everywhere
> in the RSP, and not more complicated to implement. Of course, that's only for things
> related to the target memory, things that fetch an XML file would still be in bytes.
> 
> What is your opinion on this?
> 
>>>
>>>    -> $m1000,8#??
>>>    <- aaaabbbbccccdddd
>>>
>>>    -> $M1000,6:eeeeffffeeee#??
>>>    <- OK
>>>
>>>    -> $m1000,8#??
>>>    <- eeeeffffeeeedddd
>>>
>>> If there are any other RSP packets or MI commands that need such
>>> clarification, it will be on a case-by-case basis, whatever makes more
>>> sense for each particular one.
>>
>> Off hand, I thought of qCRC and qSearch:memory.  The latter is
>> more interesting:
>>
>> - Would you allow searching for an 1 8-bit byte pattern?
> 
> Hmm I don't know. To be safe I'd say no. If we do, it means we need to
> search with a granularity of a byte. What if you search for the pattern
> 0x2345 in this memory:
> 
> 0x100 0123
> 0x101 4567
> 0x102 89ab
> 0x103 cdef
> 
> Should there be a match that spans halves of two addresses? Unless we only
> search with a byte granularity in the special case where the pattern is
> one byte long? But then what about 3-bytes patterns?
> 
> I think it's a lot of corner cases for not much value. I think it could be
> enhanced later to support it if somebody needs it.
> 
>> - So what length would you use for that one?  Host byte
>>   or addressable units?
> 
> Length here would be in addressable units.
> 
>> Thanks,
>> Pedro Alves
>>
> 
> Thanks,
> 
> Simon

Oh, and I acknowledged your comments in the individual patches, they are all
clear, thanks a lot.

Simon

Re: [PATCH v2 0/7] Support reading/writing memory on architectures with non 8-bits addressable memory

[Pedro Alves]

On 06/11/2015 10:06 PM, Simon Marchi wrote:

> Here is a draft of how the changes would look like in gdbserver when using addressable
> memory units. It's really not that bad I think.
> 
> https://github.com/simark/binutils-gdb/commit/2ecb2f054a288053e3726e92fb6126dd4c782a15
> 
> So in the end, it might be more consistent to use addressable memory units everywhere
> in the RSP, and not more complicated to implement. Of course, that's only for things
> related to the target memory, things that fetch an XML file would still be in bytes.
> 
> What is your opinion on this?
> 

I agree.

>>>
>>>    -> $m1000,8#??
>>>    <- aaaabbbbccccdddd
>>>
>>>    -> $M1000,6:eeeeffffeeee#??
>>>    <- OK
>>>
>>>    -> $m1000,8#??
>>>    <- eeeeffffeeeedddd
>>>
>>> If there are any other RSP packets or MI commands that need such
>>> clarification, it will be on a case-by-case basis, whatever makes more
>>> sense for each particular one.
>>
>> Off hand, I thought of qCRC and qSearch:memory.  The latter is
>> more interesting:
>>
>> - Would you allow searching for an 1 8-bit byte pattern?
> 
> Hmm I don't know. To be safe I'd say no. If we do, it means we need to
> search with a granularity of a byte. What if you search for the pattern
> 0x2345 in this memory:
> 
> 0x100 0123
> 0x101 4567
> 0x102 89ab
> 0x103 cdef
> 
> Should there be a match that spans halves of two addresses? Unless we only
> search with a byte granularity in the special case where the pattern is
> one byte long? But then what about 3-bytes patterns?
> 
> I think it's a lot of corner cases for not much value. I think it could be
> enhanced later to support it if somebody needs it.

I agree.

(it seems good/desirable to me to have all memory-related packets
likewise treat memory range lengths the same)

> 
>> - So what length would you use for that one?  Host byte
>>   or addressable units?
> 
> Length here would be in addressable units.
> 

Agreed.

Thanks,
Pedro Alves

Re: [PATCH v2 1/7] Various cleanups in target read/write code

[Simon Marchi]

On 15-05-21 01:44 PM, Pedro Alves wrote:
> On 04/15/2015 08:47 PM, Simon Marchi wrote:
>> This contains various cleanups in the target memory read and write code.
>> They are not directly related to the non-8-bits changes, but they
>> clarify things a bit down the line.
>>
>> gdb/ChangeLog:
>>
>> 	* target.c (target_read): Rename variables and use
>> 	TARGET_XFER_E_IO.
>> 	(target_read_with_progress): Same.
>> 	(read_memory_robust): Constify parameters and rename
>> 	variables.
>> 	(read_whatever_is_readable): Constify parameters,
>> 	rename variables, adjust formatting.
>> 	* target.h (read_memory_robust): Constify parameters.
> 
> OK.
> 
> Thanks,
> Pedro Alves
> 

I pushed this one since it's independent from the rest of the serie.

Thanks,

Simon

Re: [PATCH v2 2/7] Cleanup some docs about memory write

[Simon Marchi]

On 15-05-21 01:45 PM, Pedro Alves wrote:
> On 04/15/2015 08:47 PM, Simon Marchi wrote:
>> Some docs seemed outdated. In the case of target_write_memory, the docs
>> in target.c and target/target.h diverged a bit, so I tried to find a
>> reasonnable in-between version.
> 
> "reasonable"
> 
>>
>> gdb/ChangeLog:
>>
>> 	* corefile.c (write_memory): Update doc.
>> 	* gdbcore.h (write_memory): Same.
>> 	* target.c (target_write_memory): Same.
>> 	* target/target.h (target_write_memory): Same.
>> ---
>>  gdb/corefile.c      |  4 ++--
>>  gdb/gdbcore.h       |  6 ++----
>>  gdb/target.c        |  6 +-----
>>  gdb/target/target.h | 10 +++++-----
>>  4 files changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/corefile.c b/gdb/corefile.c
>> index a042e6d..83b0e80 100644
>> --- a/gdb/corefile.c
>> +++ b/gdb/corefile.c
>> @@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
>>    return extract_typed_address (buf, type);
>>  }
>>  
>> -/* Same as target_write_memory, but report an error if can't
>> -   write.  */
>> +/* See gdbcore.h. */
> 
> Double space after period.

Done.

>> diff --git a/gdb/target.c b/gdb/target.c
>> index fcf7090..bd9a0eb 100644
>>  int
>>  target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
> 
>> diff --git a/gdb/target/target.h b/gdb/target/target.h
>> index 05ac758..e0b7554 100644
>> --- a/gdb/target/target.h
>> +++ b/gdb/target/target.h
>> @@ -49,12 +49,12 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
>>  extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
>>  
>>  /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
>> -   Return zero for success, nonzero if any error occurs.  This
>> +   Return zero for success or TARGET_XFER_E_IO if any error occurs.  This
>>     function must be provided by the client.  Implementations of this
>> -   function may define and use their own error codes, but functions
>> -   in the common, nat and target directories must treat the return
>> -   code as opaque.  No guarantee is made about the contents of the
>> -   data at MEMADDR if any error occurs.  */
>> +   function may define and use their own error codes, but functions in
>> +   the common, nat and target directories must treat the return code as
>> +   opaque.  No guarantee is made about how much data got written if any
>> +   error occurs.  */
>>  
>>  extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>>  				ssize_t len);
>>
> 
> This one's not right.  TARGET_XFER_E_IO is a GDB-specific thing, while
> this declaration is meant for both gdb and gdbserver.  It's what
> "This function must be provided by the client" refers to.  It doesn't
> make sense to say "TARGET_XFER_E_IO" one error and then explain that
> the return code is opaque.
> 
> So probably it'd be better to leave this comment:
> 
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -1464,11 +1464,7 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>>      return TARGET_XFER_E_IO;
>>  }
>>
>> -/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
>> -   Returns either 0 for success or TARGET_XFER_E_IO if any
>> -   error occurs.  If an error occurs, no guarantee is made about how
>> -   much data got written.  Callers that can deal with partial writes
>> -   should call target_write.  */
>> +/* See target/target.h.  */
> 
> ... but extend it by starting by saying that this is GDB's implementation
> of the function as declared in target/target.h.

Actually it seems obvious now. I'll drop this part from the patch. If we want
to clarify the comments we should do it for all the functions of the interface
(in a separate patch).

> Thanks,
> Pedro Alves

Well then, all that remains would be to fix that stale comment. Is it ok?


From c9b4d03d6f7ffd84215473456d61ca1bb0c553ac Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 12 Jun 2015 14:53:45 -0400
Subject: [PATCH] Cleanup write_memory doc

This doc about write_memory seems outdated.

gdb/ChangeLog:

	* corefile.c (write_memory): Update doc.
	* gdbcore.h (write_memory): Same.
---
 gdb/corefile.c | 4 ++--
 gdb/gdbcore.h  | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..5246f71 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
   return extract_typed_address (buf, type);
 }

-/* Same as target_write_memory, but report an error if can't
-   write.  */
+/* See gdbcore.h.  */
+
 void
 write_memory (CORE_ADDR memaddr,
 	      const bfd_byte *myaddr, ssize_t len)
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index a437c8a..0c08b37 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -101,10 +101,8 @@ extern void read_memory_string (CORE_ADDR, char *, int);

 CORE_ADDR read_memory_typed_address (CORE_ADDR addr, struct type *type);

-/* This takes a char *, not void *.  This is probably right, because
-   passing in an int * or whatever is wrong with respect to
-   byteswapping, alignment, different sizes for host vs. target types,
-   etc.  */
+/* Same as target_write_memory, but report an error if can't
+   write.  */

 extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 			  ssize_t len);
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Simon Marchi]

On 15-04-16 10:48 AM, Eli Zaretskii wrote:
> This patch is fine with me, thanks.

Hi Eli,

I added some bits in the Remote Protocol section following the discussion
with Pedro, could you review them? The existing ones are the same.

Thanks!

Simon


From e12f34e5cbfde7861c0f5b4219c31dd27f36ec12 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 26 Mar 2015 15:04:40 -0400
Subject: [PATCH] Clarify doc about memory read/write and non-8-bits
 addressable memory unit sizes

New in v3:

 * Change RSP documentation as well. The m, M and X packets now use
 lengths in addressable memory units.

New in v2:

 * Change wording: use byte for 8-bits chunks and addressable memory unit
   for the unit of data associated to a single address.
 * Introduce definition of addressable memory unit in the Memory
   section.

This patch modifies the manual to clarify the MI, RSP and Python APIs in
regard to reading/writing memory on architectures with addressable
memory unit that are not 8 bits.

Care is taken to use the word "addressable memory unit" or "memory unit"
when referring to one piece of the smallest addressable size on the
current architecture and the word "byte" when referring to an 8-bits
data piece.

For MI, -data-{read,write}-memory are not modified, since they are
deprecated.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Data Manipulation): Clarify usage of
	bytes and memory units for -data-{read,write}-memory-bytes.
	(Packets): Same for m, M and X packets.
	* python.texi (Inferiors In Python): Same for read_memory and
	write_memory.
---
 gdb/doc/gdb.texinfo | 74 ++++++++++++++++++++++++++++++++++-------------------
 gdb/doc/python.texi |  5 ++--
 2 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 65c9d4f..dc8a6c9 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -9009,6 +9009,18 @@ If the @code{x} command has a repeat count, the address and contents saved
 are from the last memory unit printed; this is not the same as the last
 address printed if several units were printed on the last line of output.

+@anchor{addressable memory unit}
+@cindex addressable memory unit
+Most targets have an addressable memory unit size of 8 bits.  This means
+that to each memory address are associated 8 bits of data.  Some
+targets, however, have other addressable memory unit sizes.
+Within @value{GDBN} and this document, the term
+@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
+when explicitely referring to a chunk of data of that size.  The word
+@dfn{byte} is used to refer to a chunk of data of 8 bits, regardless of
+the addressable memory unit size of the target.  For most systems,
+addressable memory unit is a synonym of byte.
+
 @cindex remote memory comparison
 @cindex target memory comparison
 @cindex verify remote memory image
@@ -29572,6 +29584,9 @@ can be used to instantiate this class for a varobj:
 This section describes the @sc{gdb/mi} commands that manipulate data:
 examine memory and registers, evaluate expressions, etc.

+For details about what an addressable memory unit is,
+@pxref{addressable memory unit}.
+
 @c REMOVED FROM THE INTERFACE.
 @c @subheading -data-assign
 @c Change the value of a program variable. Plenty of side effects.
@@ -30096,7 +30111,7 @@ next-page="0x000013c0",prev-page="0x00001380",memory=[
 @subsubheading Synopsis

 @smallexample
- -data-read-memory-bytes [ -o @var{byte-offset} ]
+ -data-read-memory-bytes [ -o @var{offset} ]
    @var{address} @var{count}
 @end smallexample

@@ -30105,18 +30120,19 @@ where:

 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-read.  Complex expressions containing embedded white space should be
+An expression specifying the address of the first addressable memory unit
+to be read.  Complex expressions containing embedded white space should be
 quoted using the C convention.

 @item @var{count}
-The number of bytes to read.  This should be an integer literal.
+The number of addressable memory units to read.  This should be an integer
+literal.

-@item @var{byte-offset}
-The offsets in bytes relative to @var{address} at which to start
-reading.  This should be an integer literal.  This option is provided
-so that a frontend is not required to first evaluate address and then
-perform address arithmetics itself.
+@item @var{offset}
+The offset relative to @var{address} at which to start reading.  This
+should be an integer literal.  This option is provided so that a frontend
+is not required to first evaluate address and then perform address
+arithmetics itself.

 @end table

@@ -30127,10 +30143,10 @@ Attributes}.  Second, @value{GDBN} will attempt to read the remaining
 regions.  For each one, if reading full region results in an errors,
 @value{GDBN} will try to read a subset of the region.

-In general, every single byte in the region may be readable or not,
-and the only way to read every readable byte is to try a read at
+In general, every single memory unit in the region may be readable or not,
+and the only way to read every readable unit is to try a read at
 every address, which is not practical.   Therefore, @value{GDBN} will
-attempt to read all accessible bytes at either beginning or the end
+attempt to read all accessible memory units at either beginning or the end
 of the region, using a binary division scheme.  This heuristic works
 well for reading accross a memory map boundary.  Note that if a region
 has a readable range that is neither at the beginning or the end,
@@ -30190,17 +30206,19 @@ where:

 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-written.  Complex expressions containing embedded white space should be
-quoted using the C convention.
+An expression specifying the address of the first addressable memory unit
+to be written.  Complex expressions containing embedded white space should
+be quoted using the C convention.

 @item @var{contents}
-The hex-encoded bytes to write.
+The hex-encoded data to write.  It is an error if @var{contents} does
+not represent an integral number of addressable memory units.

 @item @var{count}
-Optional argument indicating the number of bytes to be written.  If @var{count}
-is greater than @var{contents}' length, @value{GDBN} will repeatedly
-write @var{contents} until it fills @var{count} bytes.
+Optional argument indicating the number of addressable memory units to be
+written.  If @var{count} is greater than @var{contents}' length,
+@value{GDBN} will repeatedly write @var{contents} until it fills
+@var{count} memory units.

 @end table

@@ -34835,8 +34853,9 @@ probes the target state as if a new connection was opened

 @item m @var{addr},@var{length}
 @cindex @samp{m} packet
-Read @var{length} bytes of memory starting at address @var{addr}.
-Note that @var{addr} may not be aligned to any particular boundary.
+Read @var{length} addressable memory units starting at address @var{addr}
+(@pxref{addressable memory unit}). Note that @var{addr} may not be aligned to
+any particular boundary.

 The stub need not use any particular size or alignment when gathering
 data from memory for the response; even if @var{addr} is word-aligned
@@ -34850,8 +34869,8 @@ suitable for accessing memory-mapped I/O devices.
 Reply:
 @table @samp
 @item @var{XX@dots{}}
-Memory contents; each byte is transmitted as a two-digit hexadecimal
-number.  The reply may contain fewer bytes than requested if the
+Memory contents; each byte is transmitted as a two-digit hexadecimal number.
+The reply may contain fewer addressable memory units than requested if the
 server was able to read only part of the region of memory.
 @item E @var{NN}
 @var{NN} is errno
@@ -34859,9 +34878,9 @@ server was able to read only part of the region of memory.

 @item M @var{addr},@var{length}:@var{XX@dots{}}
 @cindex @samp{M} packet
-Write @var{length} bytes of memory starting at address @var{addr}.
-The data is given by @var{XX@dots{}}; each byte is transmitted as a two-digit
-hexadecimal number.
+Write @var{length} addressable memory units starting at address @var{addr}
+(@pxref{addressable memory unit}). The data is given by @var{XX@dots{}}; each
+byte is transmitted as a two-digit hexadecimal number.

 Reply:
 @table @samp
@@ -35175,7 +35194,8 @@ for success (@pxref{Stop Reply Packets})
 @anchor{X packet}
 @cindex @samp{X} packet
 Write data to memory, where the data is transmitted in binary.
-Memory is specified by its address @var{addr} and number of bytes @var{length};
+Memory is specified by its address @var{addr} and number of addressable memory
+units @var{length} (@pxref{addressable memory unit});
 @samp{@var{XX}@dots{}} is binary data (@pxref{Binary Data}).

 Reply:
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 57ec22e..a2df254 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2749,7 +2749,7 @@ return an empty tuple.

 @findex Inferior.read_memory
 @defun Inferior.read_memory (address, length)
-Read @var{length} bytes of memory from the inferior, starting at
+Read @var{length} addressable memory units from the inferior, starting at
 @var{address}.  Returns a buffer object, which behaves much like an array
 or a string.  It can be modified and given to the
 @code{Inferior.write_memory} function.  In @code{Python} 3, the return
@@ -2762,7 +2762,8 @@ Write the contents of @var{buffer} to the inferior, starting at
 @var{address}.  The @var{buffer} parameter must be a Python object
 which supports the buffer protocol, i.e., a string, an array or the
 object returned from @code{Inferior.read_memory}.  If given, @var{length}
-determines the number of bytes from @var{buffer} to be written.
+determines the number of addressable memory units from @var{buffer} to be
+written.
 @end defun

 @findex gdb.search_memory
-- 
2.1.4

Re: [PATCH v2 4/7] gdbarch: add addressable_memory_unit_size method

[Simon Marchi]

> This is the central place everyone should look at first to understand
> non 8-bit bytes targets.  For extra clarity, could you please
> add "8-bit"s in there, say:
> 
>  "the size in 8-bit bytes" (...) to the number of 8-bit bytes associated to
>  each address in memory."
> 
> Looks good to me with that change.
> 
> Thanks,
> Pedro Alves

Thanks, pushed with the change:


From 3374165f51fa3cc3ce1b1bf8c72293464da9d511 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 12 Jun 2015 16:51:51 -0400
Subject: [PATCH] gdbarch: add addressable_memory_unit_size method

Add a new gdbarch method to get the length of an addressable memory unit
for a given architecture. The default implementation returns 1.

gdb/ChangeLog:

	* arch-utils.h (default_addressable_memory_unit_size): New.
	* arch-utils.c (default_addressable_memory_unit_size): New.
	* gdbarch.sh (addressable_memory_unit_size): New.
	* gdbarch.h: Re-generate.
	* gdbarch.c: Re-generate.
---
 gdb/ChangeLog    |  8 ++++++++
 gdb/arch-utils.c |  9 +++++++++
 gdb/arch-utils.h |  1 +
 gdb/gdbarch.c    | 23 +++++++++++++++++++++++
 gdb/gdbarch.h    |  8 ++++++++
 gdb/gdbarch.sh   |  6 ++++++
 6 files changed, 55 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bde6f67..1b28992 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2015-06-12  Simon Marchi  <simon.marchi@ericsson.com>

+	* arch-utils.h (default_addressable_memory_unit_size): New.
+	* arch-utils.c (default_addressable_memory_unit_size): New.
+	* gdbarch.sh (addressable_memory_unit_size): New.
+	* gdbarch.h: Re-generate.
+	* gdbarch.c: Re-generate.
+
+2015-06-12  Simon Marchi  <simon.marchi@ericsson.com>
+
 	* target.c (target_read): Rename variables and use
 	TARGET_XFER_E_IO.
 	(target_read_with_progress): Same.
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 7f07e21..e9c622d 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -888,6 +888,15 @@ default_gnu_triplet_regexp (struct gdbarch *gdbarch)
   return gdbarch_bfd_arch_info (gdbarch)->arch_name;
 }

+/* Default method for gdbarch_addressable_memory_unit_size.  By default, a memory byte has
+   a size of 1 octet.  */
+
+int
+default_addressable_memory_unit_size (struct gdbarch *gdbarch)
+{
+  return 1;
+}
+
 /* -Wmissing-prototypes */
 extern initialize_file_ftype _initialize_gdbarch_utils;

diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 9cb28e4..27f4787 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -203,5 +203,6 @@ extern CORE_ADDR default_infcall_mmap (CORE_ADDR size, unsigned prot);
 extern void default_infcall_munmap (CORE_ADDR addr, CORE_ADDR size);
 extern char *default_gcc_target_options (struct gdbarch *gdbarch);
 extern const char *default_gnu_triplet_regexp (struct gdbarch *gdbarch);
+extern int default_addressable_memory_unit_size (struct gdbarch *gdbarch);

 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index cd82487..c289334 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -329,6 +329,7 @@ struct gdbarch
   gdbarch_infcall_munmap_ftype *infcall_munmap;
   gdbarch_gcc_target_options_ftype *gcc_target_options;
   gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp;
+  gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size;
 };

 /* Create a new ``struct gdbarch'' based on information provided by
@@ -430,6 +431,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->infcall_munmap = default_infcall_munmap;
   gdbarch->gcc_target_options = default_gcc_target_options;
   gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
+  gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   /* gdbarch_alloc() */

   return gdbarch;
@@ -663,6 +665,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of infcall_munmap, invalid_p == 0 */
   /* Skip verify of gcc_target_options, invalid_p == 0 */
   /* Skip verify of gnu_triplet_regexp, invalid_p == 0 */
+  /* Skip verify of addressable_memory_unit_size, invalid_p == 0 */
   buf = ui_file_xstrdup (log, &length);
   make_cleanup (xfree, buf);
   if (length > 0)
@@ -714,6 +717,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: address_to_pointer = <%s>\n",
                       host_address_to_string (gdbarch->address_to_pointer));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: addressable_memory_unit_size = <%s>\n",
+                      host_address_to_string (gdbarch->addressable_memory_unit_size));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_adjust_breakpoint_address_p() = %d\n",
                       gdbarch_adjust_breakpoint_address_p (gdbarch));
   fprintf_unfiltered (file,
@@ -4730,6 +4736,23 @@ set_gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch,
   gdbarch->gnu_triplet_regexp = gnu_triplet_regexp;
 }

+int
+gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->addressable_memory_unit_size != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_addressable_memory_unit_size called\n");
+  return gdbarch->addressable_memory_unit_size (gdbarch);
+}
+
+void
+set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch,
+                                          gdbarch_addressable_memory_unit_size_ftype addressable_memory_unit_size)
+{
+  gdbarch->addressable_memory_unit_size = addressable_memory_unit_size;
+}
+

 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index d2ccd2a..7d6a0cf 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1466,6 +1466,14 @@ typedef const char * (gdbarch_gnu_triplet_regexp_ftype) (struct gdbarch *gdbarch
 extern const char * gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch);
 extern void set_gdbarch_gnu_triplet_regexp (struct gdbarch *gdbarch, gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp);

+/* Return the size in 8-bit bytes of an addressable memory unit on this
+   architecture.  This corresponds to the number of 8-bit bytes associated to
+   each address in memory. */
+
+typedef int (gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarch);
+extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
+extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch, gdbarch_addressable_memory_unit_size_ftype *addressable_memory_unit_size);
+
 /* Definition for an unknown syscall, used basically in error-cases.  */
 #define UNKNOWN_SYSCALL (-1)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index a3c903e..6c5d684 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1113,6 +1113,12 @@ m:char *:gcc_target_options:void:::default_gcc_target_options::0
 # returns the BFD architecture name, which is correct in nearly every
 # case.
 m:const char *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0
+
+# Return the size in 8-bit bytes of an addressable memory unit on this
+# architecture.  This corresponds to the number of 8-bit bytes associated to
+# each address in memory.
+m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit_size::0
+
 EOF
 }

-- 
2.1.4

Re: [PATCH v2 5/7] target: consider addressable unit size when reading/writing memory

[Simon Marchi]

On 15-05-21 01:46 PM, Pedro Alves wrote:
> On 04/15/2015 08:47 PM, Simon Marchi wrote:
>> If we are reading/writing from a memory object, the length represents
>> the number of "addresses" to read/write, so the addressable unit size
>> needs to be taken into account when allocating memory on gdb's side.
>>
>> gdb/ChangeLog:
>>
>> 	* target.c (target_read): Consider addressable unit size when
>> 	reading from a memory object.
>> 	(read_memory_robust): Same.
>> 	(read_whatever_is_readable): Same.
>> 	(target_write_with_progress): Consider addressable unit size
>> 	when writing to a memory object.
>> 	* target.h (target_read): Update documentation.
>> 	(target_write): Add documentation.
> 
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -1589,6 +1589,15 @@ target_read (struct target_ops *ops,
>>  	     ULONGEST offset, LONGEST len)
>>  {
>>    LONGEST xfered_total = 0;
>> +  int unit_size = 1;
>> +
>> +  /* If we are reading from a memory object, find the length of an addressable
>> +     unit for that architecture.  */
>> +  if (object == TARGET_OBJECT_MEMORY
>> +      || object == TARGET_OBJECT_STACK_MEMORY
>> +      || object == TARGET_OBJECT_CODE_MEMORY
>> +      || object == TARGET_OBJECT_RAW_MEMORY)
>> +    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch());
> 
> Missing space before parens.  Otherwise looks OK.
> 
> Thanks,
> Pedro Alves

Thanks, pushed with the missing space added. Since we need to post what we
push, here's the same patch with the extra space :).


From d309493c38fcef624f6f85aee4aa37f4f9e3e62a Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 12 Jun 2015 17:02:44 -0400
Subject: [PATCH] target: consider addressable unit size when reading/writing
 memory

If we are reading/writing from a memory object, the length represents
the number of "addresses" to read/write, so the addressable unit size
needs to be taken into account when allocating memory on gdb's side.

gdb/ChangeLog:

	* target.c (target_read): Consider addressable unit size when
	reading from a memory object.
	(read_memory_robust): Same.
	(read_whatever_is_readable): Same.
	(target_write_with_progress): Consider addressable unit size
	when writing to a memory object.
	* target.h (target_read): Update documentation.
	(target_write): Add documentation.
---
 gdb/ChangeLog | 11 +++++++++++
 gdb/target.c  | 35 ++++++++++++++++++++++++++++-------
 gdb/target.h  | 33 ++++++++++++++++++++++++++-------
 3 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1b28992..bc30a8c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,16 @@
 2015-06-12  Simon Marchi  <simon.marchi@ericsson.com>

+	* target.c (target_read): Consider addressable unit size when
+	reading from a memory object.
+	(read_memory_robust): Same.
+	(read_whatever_is_readable): Same.
+	(target_write_with_progress): Consider addressable unit size
+	when writing to a memory object.
+	* target.h (target_read): Update documentation.
+	(target_write): Add documentation.
+
+2015-06-12  Simon Marchi  <simon.marchi@ericsson.com>
+
 	* arch-utils.h (default_addressable_memory_unit_size): New.
 	* arch-utils.c (default_addressable_memory_unit_size): New.
 	* gdbarch.sh (addressable_memory_unit_size): New.
diff --git a/gdb/target.c b/gdb/target.c
index dd2393a..4e2d005 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1593,6 +1593,15 @@ target_read (struct target_ops *ops,
 	     ULONGEST offset, LONGEST len)
 {
   LONGEST xfered_total = 0;
+  int unit_size = 1;
+
+  /* If we are reading from a memory object, find the length of an addressable
+     unit for that architecture.  */
+  if (object == TARGET_OBJECT_MEMORY
+      || object == TARGET_OBJECT_STACK_MEMORY
+      || object == TARGET_OBJECT_CODE_MEMORY
+      || object == TARGET_OBJECT_RAW_MEMORY)
+    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());

   while (xfered_total < len)
     {
@@ -1600,7 +1609,7 @@ target_read (struct target_ops *ops,
       enum target_xfer_status status;

       status = target_read_partial (ops, object, annex,
-				    buf + xfered_total,
+				    buf + xfered_total * unit_size,
 				    offset + xfered_total, len - xfered_total,
 				    &xfered_partial);

@@ -1643,6 +1652,7 @@ target_read (struct target_ops *ops,
 static void
 read_whatever_is_readable (struct target_ops *ops,
 			   const ULONGEST begin, const ULONGEST end,
+			   int unit_size,
 			   VEC(memory_read_result_s) **result)
 {
   gdb_byte *buf = xmalloc (end - begin);
@@ -1709,7 +1719,7 @@ read_whatever_is_readable (struct target_ops *ops,
 	}

       xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-			  buf + (first_half_begin - begin),
+			  buf + (first_half_begin - begin) * unit_size,
 			  first_half_begin,
 			  first_half_end - first_half_begin);

@@ -1745,8 +1755,9 @@ read_whatever_is_readable (struct target_ops *ops,
       /* The [current_end, end) range has been read.  */
       LONGEST region_len = end - current_end;

-      r.data = xmalloc (region_len);
-      memcpy (r.data, buf + current_end - begin, region_len);
+      r.data = xmalloc (region_len * unit_size);
+      memcpy (r.data, buf + (current_end - begin) * unit_size,
+	      region_len * unit_size);
       r.begin = current_end;
       r.end = end;
       xfree (buf);
@@ -1773,6 +1784,7 @@ read_memory_robust (struct target_ops *ops,
 		    const ULONGEST offset, const LONGEST len)
 {
   VEC(memory_read_result_s) *result = 0;
+  int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());

   LONGEST xfered_total = 0;
   while (xfered_total < len)
@@ -1798,7 +1810,7 @@ read_memory_robust (struct target_ops *ops,
       else
 	{
 	  LONGEST to_read = min (len - xfered_total, region_len);
-	  gdb_byte *buffer = (gdb_byte *)xmalloc (to_read);
+	  gdb_byte *buffer = (gdb_byte *) xmalloc (to_read * unit_size);

 	  LONGEST xfered_partial =
 	      target_read (ops, TARGET_OBJECT_MEMORY, NULL,
@@ -1810,7 +1822,7 @@ read_memory_robust (struct target_ops *ops,
 	      /* Got an error reading full chunk.  See if maybe we can read
 		 some subrange.  */
 	      xfree (buffer);
-	      read_whatever_is_readable (ops, offset + xfered_total,
+	      read_whatever_is_readable (ops, offset + xfered_total, unit_size,
 					 offset + xfered_total + to_read, &result);
 	      xfered_total += to_read;
 	    }
@@ -1840,6 +1852,15 @@ target_write_with_progress (struct target_ops *ops,
 			    void (*progress) (ULONGEST, void *), void *baton)
 {
   LONGEST xfered_total = 0;
+  int unit_size = 1;
+
+  /* If we are writing to a memory object, find the length of an addressable
+     unit for that architecture.  */
+  if (object == TARGET_OBJECT_MEMORY
+      || object == TARGET_OBJECT_STACK_MEMORY
+      || object == TARGET_OBJECT_CODE_MEMORY
+      || object == TARGET_OBJECT_RAW_MEMORY)
+    unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());

   /* Give the progress callback a chance to set up.  */
   if (progress)
@@ -1851,7 +1872,7 @@ target_write_with_progress (struct target_ops *ops,
       enum target_xfer_status status;

       status = target_write_partial (ops, object, annex,
-				     (gdb_byte *) buf + xfered_total,
+				     buf + xfered_total * unit_size,
 				     offset + xfered_total, len - xfered_total,
 				     &xfered_partial);

diff --git a/gdb/target.h b/gdb/target.h
index c7046be..32234f7 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -265,13 +265,17 @@ typedef enum target_xfer_status
 			     ULONGEST len,
 			     ULONGEST *xfered_len);

-/* Request that OPS transfer up to LEN 8-bit bytes of the target's
-   OBJECT.  The OFFSET, for a seekable object, specifies the
-   starting point.  The ANNEX can be used to provide additional
-   data-specific information to the target.
-
-   Return the number of bytes actually transfered, or a negative error
-   code (an 'enum target_xfer_error' value) if the transfer is not
+/* Request that OPS transfer up to LEN addressable units of the target's
+   OBJECT.  When reading from a memory object, the size of an addressable unit
+   is architecture dependent and can be found using
+   gdbarch_addressable_memory_unit_size.  Otherwise, an addressable unit is 1
+   byte long.  BUF should point to a buffer large enough to hold the read data,
+   taking into account the addressable unit size.  The OFFSET, for a seekable
+   object, specifies the starting point.  The ANNEX can be used to provide
+   additional data-specific information to the target.
+
+   Return the number of addressable units actually transferred, or a negative
+   error code (an 'enum target_xfer_error' value) if the transfer is not
    supported or otherwise fails.  Return of a positive value less than
    LEN indicates that no further transfer is possible.  Unlike the raw
    to_xfer_partial interface, callers of these functions do not need
@@ -300,6 +304,21 @@ extern VEC(memory_read_result_s)* read_memory_robust (struct target_ops *ops,
 						      const ULONGEST offset,
 						      const LONGEST len);

+/* Request that OPS transfer up to LEN addressable units from BUF to the
+   target's OBJECT.  When writing to a memory object, the addressable unit
+   size is architecture dependent and can be found using
+   gdbarch_addressable_memory_unit_size.  Otherwise, an addressable unit is 1
+   byte long.  The OFFSET, for a seekable object, specifies the starting point.
+   The ANNEX can be used to provide additional data-specific information to
+   the target.
+
+   Return the number of addressable units actually transferred, or a negative
+   error code (an 'enum target_xfer_status' value) if the transfer is not
+   supported or otherwise fails.  Return of a positive value less than
+   LEN indicates that no further transfer is possible.  Unlike the raw
+   to_xfer_partial interface, callers of these functions do not need to
+   retry partial transfers.  */
+
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,
 			     const char *annex, const gdb_byte *buf,
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Eli Zaretskii]

> Date: Fri, 12 Jun 2015 16:28:05 -0400
> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 15-04-16 10:48 AM, Eli Zaretskii wrote:
> > This patch is fine with me, thanks.
> 
> Hi Eli,
> 
> I added some bits in the Remote Protocol section following the discussion
> with Pedro, could you review them?

Below.

> +@anchor{addressable memory unit}
> +@cindex addressable memory unit
> +Most targets have an addressable memory unit size of 8 bits.  This means
> +that to each memory address are associated 8 bits of data.  Some
> +targets, however, have other addressable memory unit sizes.
> +Within @value{GDBN} and this document, the term
> +@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
> +when explicitely referring to a chunk of data of that size.  The word
        ^^^^^^^^^^^
"explicitly"

> +Read @var{length} addressable memory units starting at address @var{addr}
> +(@pxref{addressable memory unit}). Note that @var{addr} may not be aligned to
> +any particular boundary.         ^^

Two spaces between sentences, please.

> +Write @var{length} addressable memory units starting at address @var{addr}
> +(@pxref{addressable memory unit}). The data is given by @var{XX@dots{}}; each
                                    ^^
Likewise.

Thanks.

Re: [PATCH v2 2/7] Cleanup some docs about memory write

[Pedro Alves]

On 06/12/2015 08:17 PM, Simon Marchi wrote:

> Actually it seems obvious now. I'll drop this part from the patch. If we want
> to clarify the comments we should do it for all the functions of the interface
> (in a separate patch).
> 
> Well then, all that remains would be to fix that stale comment. Is it ok?

OK.

Thanks,
Pedro Alves

Re: [PATCH v2 2/7] Cleanup some docs about memory write

[Simon Marchi]

On 15-06-15 05:57 AM, Pedro Alves wrote:
> On 06/12/2015 08:17 PM, Simon Marchi wrote:
> 
>> Actually it seems obvious now. I'll drop this part from the patch. If we want
>> to clarify the comments we should do it for all the functions of the interface
>> (in a separate patch).
>>
>> Well then, all that remains would be to fix that stale comment. Is it ok?
> 
> OK.
> 
> Thanks,
> Pedro Alves


Thanks, pushed as this:

From cb6f16cf4f7a12f9aadddc0451d47f0511729c8d Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 15 Jun 2015 13:34:47 -0400
Subject: [PATCH] Cleanup write_memory doc

This doc about write_memory seems outdated.

gdb/ChangeLog:

	* corefile.c (write_memory): Update doc.
	* gdbcore.h (write_memory): Same.
---
 gdb/ChangeLog  | 5 +++++
 gdb/corefile.c | 4 ++--
 gdb/gdbcore.h  | 6 ++----
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index db7fb63..a63d894 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-15  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* corefile.c (write_memory): Update doc.
+	* gdbcore.h (write_memory): Same.
+
 2015-06-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

 	* linux-tdep.c (enum filterflags): Make it from anonymous enum.
diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..5246f71 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
   return extract_typed_address (buf, type);
 }

-/* Same as target_write_memory, but report an error if can't
-   write.  */
+/* See gdbcore.h.  */
+
 void
 write_memory (CORE_ADDR memaddr,
 	      const bfd_byte *myaddr, ssize_t len)
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index a437c8a..0c08b37 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -101,10 +101,8 @@ extern void read_memory_string (CORE_ADDR, char *, int);

 CORE_ADDR read_memory_typed_address (CORE_ADDR addr, struct type *type);

-/* This takes a char *, not void *.  This is probably right, because
-   passing in an int * or whatever is wrong with respect to
-   byteswapping, alignment, different sizes for host vs. target types,
-   etc.  */
+/* Same as target_write_memory, but report an error if can't
+   write.  */

 extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 			  ssize_t len);
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Simon Marchi]

>> +@anchor{addressable memory unit}
>> +@cindex addressable memory unit
>> +Most targets have an addressable memory unit size of 8 bits.  This means
>> +that to each memory address are associated 8 bits of data.  Some
>> +targets, however, have other addressable memory unit sizes.
>> +Within @value{GDBN} and this document, the term
>> +@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
>> +when explicitely referring to a chunk of data of that size.  The word
>         ^^^^^^^^^^^
> "explicitly"
> 
>> +Read @var{length} addressable memory units starting at address @var{addr}
>> +(@pxref{addressable memory unit}). Note that @var{addr} may not be aligned to
>> +any particular boundary.         ^^
> 
> Two spaces between sentences, please.
> 
>> +Write @var{length} addressable memory units starting at address @var{addr}
>> +(@pxref{addressable memory unit}). The data is given by @var{XX@dots{}}; each
>                                     ^^
> Likewise.
> 
> Thanks.

Thanks, here is the same patch with those fixed.


From 4dc59dfa1b65831fa5958ae7b7f16fe592287d41 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 26 Mar 2015 15:04:40 -0400
Subject: [PATCH] Clarify doc about memory read/write and non-8-bits
 addressable memory unit sizes

New in v3:

 * Change RSP documentation as well. The m, M and X packets now use
 lengths in addressable memory units.

New in v2:

 * Change wording: use byte for 8-bits chunks and addressable memory unit
   for the unit of data associated to a single address.
 * Introduce definition of addressable memory unit in the Memory
   section.

This patch modifies the manual to clarify the MI, RSP and Python APIs in
regard to reading/writing memory on architectures with addressable
memory unit that are not 8 bits.

Care is taken to use the word "addressable memory unit" or "memory unit"
when referring to one piece of the smallest addressable size on the
current architecture and the word "byte" when referring to an 8-bits
data piece.

For MI, -data-{read,write}-memory are not modified, since they are
deprecated.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Data Manipulation): Clarify usage of
	bytes and memory units for -data-{read,write}-memory-bytes.
	(Packets): Same for m, M and X packets.
	* python.texi (Inferiors In Python): Same for read_memory and
	write_memory.
---
 gdb/doc/gdb.texinfo | 74 ++++++++++++++++++++++++++++++++++-------------------
 gdb/doc/python.texi |  5 ++--
 2 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 81e3181..952844e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -9009,6 +9009,18 @@ If the @code{x} command has a repeat count, the address and contents saved
 are from the last memory unit printed; this is not the same as the last
 address printed if several units were printed on the last line of output.

+@anchor{addressable memory unit}
+@cindex addressable memory unit
+Most targets have an addressable memory unit size of 8 bits.  This means
+that to each memory address are associated 8 bits of data.  Some
+targets, however, have other addressable memory unit sizes.
+Within @value{GDBN} and this document, the term
+@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
+when explicitly referring to a chunk of data of that size.  The word
+@dfn{byte} is used to refer to a chunk of data of 8 bits, regardless of
+the addressable memory unit size of the target.  For most systems,
+addressable memory unit is a synonym of byte.
+
 @cindex remote memory comparison
 @cindex target memory comparison
 @cindex verify remote memory image
@@ -29579,6 +29591,9 @@ can be used to instantiate this class for a varobj:
 This section describes the @sc{gdb/mi} commands that manipulate data:
 examine memory and registers, evaluate expressions, etc.

+For details about what an addressable memory unit is,
+@pxref{addressable memory unit}.
+
 @c REMOVED FROM THE INTERFACE.
 @c @subheading -data-assign
 @c Change the value of a program variable. Plenty of side effects.
@@ -30103,7 +30118,7 @@ next-page="0x000013c0",prev-page="0x00001380",memory=[
 @subsubheading Synopsis

 @smallexample
- -data-read-memory-bytes [ -o @var{byte-offset} ]
+ -data-read-memory-bytes [ -o @var{offset} ]
    @var{address} @var{count}
 @end smallexample

@@ -30112,18 +30127,19 @@ where:

 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-read.  Complex expressions containing embedded white space should be
+An expression specifying the address of the first addressable memory unit
+to be read.  Complex expressions containing embedded white space should be
 quoted using the C convention.

 @item @var{count}
-The number of bytes to read.  This should be an integer literal.
+The number of addressable memory units to read.  This should be an integer
+literal.

-@item @var{byte-offset}
-The offsets in bytes relative to @var{address} at which to start
-reading.  This should be an integer literal.  This option is provided
-so that a frontend is not required to first evaluate address and then
-perform address arithmetics itself.
+@item @var{offset}
+The offset relative to @var{address} at which to start reading.  This
+should be an integer literal.  This option is provided so that a frontend
+is not required to first evaluate address and then perform address
+arithmetics itself.

 @end table

@@ -30134,10 +30150,10 @@ Attributes}.  Second, @value{GDBN} will attempt to read the remaining
 regions.  For each one, if reading full region results in an errors,
 @value{GDBN} will try to read a subset of the region.

-In general, every single byte in the region may be readable or not,
-and the only way to read every readable byte is to try a read at
+In general, every single memory unit in the region may be readable or not,
+and the only way to read every readable unit is to try a read at
 every address, which is not practical.   Therefore, @value{GDBN} will
-attempt to read all accessible bytes at either beginning or the end
+attempt to read all accessible memory units at either beginning or the end
 of the region, using a binary division scheme.  This heuristic works
 well for reading accross a memory map boundary.  Note that if a region
 has a readable range that is neither at the beginning or the end,
@@ -30197,17 +30213,19 @@ where:

 @table @samp
 @item @var{address}
-An expression specifying the address of the first memory word to be
-written.  Complex expressions containing embedded white space should be
-quoted using the C convention.
+An expression specifying the address of the first addressable memory unit
+to be written.  Complex expressions containing embedded white space should
+be quoted using the C convention.

 @item @var{contents}
-The hex-encoded bytes to write.
+The hex-encoded data to write.  It is an error if @var{contents} does
+not represent an integral number of addressable memory units.

 @item @var{count}
-Optional argument indicating the number of bytes to be written.  If @var{count}
-is greater than @var{contents}' length, @value{GDBN} will repeatedly
-write @var{contents} until it fills @var{count} bytes.
+Optional argument indicating the number of addressable memory units to be
+written.  If @var{count} is greater than @var{contents}' length,
+@value{GDBN} will repeatedly write @var{contents} until it fills
+@var{count} memory units.

 @end table

@@ -34842,8 +34860,9 @@ probes the target state as if a new connection was opened

 @item m @var{addr},@var{length}
 @cindex @samp{m} packet
-Read @var{length} bytes of memory starting at address @var{addr}.
-Note that @var{addr} may not be aligned to any particular boundary.
+Read @var{length} addressable memory units starting at address @var{addr}
+(@pxref{addressable memory unit}).  Note that @var{addr} may not be aligned to
+any particular boundary.

 The stub need not use any particular size or alignment when gathering
 data from memory for the response; even if @var{addr} is word-aligned
@@ -34857,8 +34876,8 @@ suitable for accessing memory-mapped I/O devices.
 Reply:
 @table @samp
 @item @var{XX@dots{}}
-Memory contents; each byte is transmitted as a two-digit hexadecimal
-number.  The reply may contain fewer bytes than requested if the
+Memory contents; each byte is transmitted as a two-digit hexadecimal number.
+The reply may contain fewer addressable memory units than requested if the
 server was able to read only part of the region of memory.
 @item E @var{NN}
 @var{NN} is errno
@@ -34866,9 +34885,9 @@ server was able to read only part of the region of memory.

 @item M @var{addr},@var{length}:@var{XX@dots{}}
 @cindex @samp{M} packet
-Write @var{length} bytes of memory starting at address @var{addr}.
-The data is given by @var{XX@dots{}}; each byte is transmitted as a two-digit
-hexadecimal number.
+Write @var{length} addressable memory units starting at address @var{addr}
+(@pxref{addressable memory unit}).  The data is given by @var{XX@dots{}}; each
+byte is transmitted as a two-digit hexadecimal number.

 Reply:
 @table @samp
@@ -35182,7 +35201,8 @@ for success (@pxref{Stop Reply Packets})
 @anchor{X packet}
 @cindex @samp{X} packet
 Write data to memory, where the data is transmitted in binary.
-Memory is specified by its address @var{addr} and number of bytes @var{length};
+Memory is specified by its address @var{addr} and number of addressable memory
+units @var{length} (@pxref{addressable memory unit});
 @samp{@var{XX}@dots{}} is binary data (@pxref{Binary Data}).

 Reply:
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 57ec22e..a2df254 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2749,7 +2749,7 @@ return an empty tuple.

 @findex Inferior.read_memory
 @defun Inferior.read_memory (address, length)
-Read @var{length} bytes of memory from the inferior, starting at
+Read @var{length} addressable memory units from the inferior, starting at
 @var{address}.  Returns a buffer object, which behaves much like an array
 or a string.  It can be modified and given to the
 @code{Inferior.write_memory} function.  In @code{Python} 3, the return
@@ -2762,7 +2762,8 @@ Write the contents of @var{buffer} to the inferior, starting at
 @var{address}.  The @var{buffer} parameter must be a Python object
 which supports the buffer protocol, i.e., a string, an array or the
 object returned from @code{Inferior.read_memory}.  If given, @var{length}
-determines the number of bytes from @var{buffer} to be written.
+determines the number of addressable memory units from @var{buffer} to be
+written.
 @end defun

 @findex gdb.search_memory
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Eli Zaretskii]

> Date: Mon, 15 Jun 2015 13:40:15 -0400
> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: <gdb-patches@sourceware.org>
> 
> >> +@anchor{addressable memory unit}
> >> +@cindex addressable memory unit
> >> +Most targets have an addressable memory unit size of 8 bits.  This means
> >> +that to each memory address are associated 8 bits of data.  Some
> >> +targets, however, have other addressable memory unit sizes.
> >> +Within @value{GDBN} and this document, the term
> >> +@dfn{addressable memory unit} (or @dfn{memory unit} for short) is used
> >> +when explicitely referring to a chunk of data of that size.  The word
> >         ^^^^^^^^^^^
> > "explicitly"
> > 
> >> +Read @var{length} addressable memory units starting at address @var{addr}
> >> +(@pxref{addressable memory unit}). Note that @var{addr} may not be aligned to
> >> +any particular boundary.         ^^
> > 
> > Two spaces between sentences, please.
> > 
> >> +Write @var{length} addressable memory units starting at address @var{addr}
> >> +(@pxref{addressable memory unit}). The data is given by @var{XX@dots{}}; each
> >                                     ^^
> > Likewise.
> > 
> > Thanks.
> 
> Thanks, here is the same patch with those fixed.

OK.

Re: [PATCH v2 6/7] remote: consider addressable unit size when reading/writing memory

[Simon Marchi]

On 15-05-21 01:48 PM, Pedro Alves wrote:
> On 04/15/2015 08:47 PM, Simon Marchi wrote:
>> Adapt code in remote.c to take into account addressable unit size when
>> reading/writing memory.
>>
>> A few variables are renamed and suffixed with _bytes or _units. This
>> way, it's more obvious if there is any place where we add or compare
>> values of different kinds (which would be a mistake).
> 
> You should put here some of the info you have on the series intro.
> Even better in comments in the code even.
> 
> E.g., this example here:
> 
>> For RSP's m, M and X packets, the "length" parameters are used to
>> correctly encode or decode the packet at a low level, where we don't
>> want to have to deal with different target byte sizes. Also, for a
>> network protocol, it would make sense to use a fixed-sized unit.
>> Therefore, it would be easier if those lengths were always in bytes.
>> Here is an example that corresponds to the previous MI example.
>>
>>    -> $m1000,8#??
>>    <- aaaabbbbccccdddd
>>
>>    -> $M1000,6:eeeeffffeeee#??
>>    <- OK
>>
>>    -> $m1000,8#??
>>    <- eeeeffffeeeedddd
>>

Ok, I put the example in the comment of remote_write_bytes_aux and added
a reference to it in remote_read_bytes_1. Do you have a more suitable place
in mind where to put it?

I hadn't updated remote_read_bytes_1's comment originally, so I did it now.

>> --- a/gdb/common/rsp-low.c
>> +++ b/gdb/common/rsp-low.c
>> @@ -146,38 +146,64 @@ bin2hex (const gdb_byte *bin, char *hex, int count)
>>    return i;
>>  }
>>  
>> +static int needs_escaping (gdb_byte b)
> 
> Line break before 'needs_escaping'.  Missing intro comment.

Done.

>> +{
>> +  return b == '$' || b == '#' || b == '}' || b == '*';
>> +}
>> +
> 
> 
>> -	  if (output_index + 1 > out_maxlen)
>> -	    break;
>> -	  out_buf[output_index++] = b;
>> +	  int idx = input_unit_index * unit_size + byte_index_in_unit;
>> +	  gdb_byte b = buffer[idx];
>> +	  if (needs_escaping(b))
> 
> Space before parens.

Done.

>> +	    {
> 
> Otherwise looks good.
> 
> Thanks,
> Pedro Alves

Thanks for the review. Here's what's new:

* needs_escaping: Added newline and comment.
* remote_escape_output: Add missing space.
* remote_write_bytes_aux: Send length in addressable units, update comment with example gdb/stub exchange.
* remote_read_bytes_1: Same, and update function comment (add UNIT_SIZE).


From 3c3a0d8d88dcd21da79a2d53fbb228f0d97b5906 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 27 Mar 2015 13:33:34 -0400
Subject: [PATCH] remote: consider addressable unit size when reading/writing
 memory

Adapt code in remote.c to take into account addressable unit size when
reading/writing memory.

A few variables are renamed and suffixed with _bytes or _units. This
way, it's more obvious if there is any place where we add or compare
values of different kinds (which would be a mistake).

gdb/ChangeLog:

	* common/rsp-low.c (needs_escaping): New.
	(remote_escape_output): Add unit_size parameter. Refactor to
	support multi-byte addressable units.  Rename parameters.
	* common/rsp-low.h (remote_escape_output): Add unit_size
	parameter and rename others. Update doc.
	* remote.c (align_for_efficient_write): New.
	(remote_write_bytes_aux): Add unit_size parameter and use it.
	Rename some variables.  Update doc.
	(remote_xfer_partial): Get unit size and use it.
	(remote_read_bytes_1): Add unit_size parameter and use it.
	Rename some variables. Update doc.
	(remote_write_bytes): Same.
	(remote_xfer_live_readonly_partial): Same.
	(remote_read_bytes): Same.
	(remote_flash_write): Update call to remote_write_bytes_aux.
	(remote_write_qxfer): Update call to remote_escape_output.
	(remote_search_memory): Same.
	(remote_hostio_pwrite): Same.

gdb/gdbserver/ChangeLog:

	* server.c (write_qxfer_response): Update call to
	remote_escape_output.
---
 gdb/common/rsp-low.c   |  71 ++++++++++++------
 gdb/common/rsp-low.h   |  18 ++---
 gdb/gdbserver/server.c |   4 +-
 gdb/remote.c           | 193 +++++++++++++++++++++++++++++--------------------
 4 files changed, 174 insertions(+), 112 deletions(-)

diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index d3d3d65..178c698 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -146,38 +146,67 @@ bin2hex (const gdb_byte *bin, char *hex, int count)
   return i;
 }

+/* Return whether byte B needs escaping when sent as part of binary data.  */
+
+static int
+needs_escaping (gdb_byte b)
+{
+  return b == '$' || b == '#' || b == '}' || b == '*';
+}
+
 /* See rsp-low.h.  */

 int
-remote_escape_output (const gdb_byte *buffer, int len,
-		      gdb_byte *out_buf, int *out_len,
-		      int out_maxlen)
+remote_escape_output (const gdb_byte *buffer, int len_units, int unit_size,
+		      gdb_byte *out_buf, int *out_len_units,
+		      int out_maxlen_bytes)
 {
-  int input_index, output_index;
-
-  output_index = 0;
-  for (input_index = 0; input_index < len; input_index++)
+  int input_unit_index, output_byte_index = 0, byte_index_in_unit;
+  int number_escape_bytes_needed;
+
+  /* Try to copy integral addressable memory units until
+     (1) we run out of space or
+     (2) we copied all of them.  */
+  for (input_unit_index = 0;
+       input_unit_index < len_units;
+       input_unit_index++)
     {
-      gdb_byte b = buffer[input_index];
-
-      if (b == '$' || b == '#' || b == '}' || b == '*')
+      /* Find out how many escape bytes we need for this unit.  */
+      number_escape_bytes_needed = 0;
+      for (byte_index_in_unit = 0;
+	   byte_index_in_unit < unit_size;
+	   byte_index_in_unit++)
 	{
-	  /* These must be escaped.  */
-	  if (output_index + 2 > out_maxlen)
-	    break;
-	  out_buf[output_index++] = '}';
-	  out_buf[output_index++] = b ^ 0x20;
+	  int idx = input_unit_index * unit_size + byte_index_in_unit;
+	  gdb_byte b = buffer[idx];
+	  if (needs_escaping (b))
+	    number_escape_bytes_needed++;
 	}
-      else
+
+      /* Check if we have room to fit this escaped unit.  */
+      if (output_byte_index + unit_size + number_escape_bytes_needed >
+	    out_maxlen_bytes)
+	  break;
+
+      /* Copy the unit byte per byte, adding escapes.  */
+      for (byte_index_in_unit = 0;
+	   byte_index_in_unit < unit_size;
+	   byte_index_in_unit++)
 	{
-	  if (output_index + 1 > out_maxlen)
-	    break;
-	  out_buf[output_index++] = b;
+	  int idx = input_unit_index * unit_size + byte_index_in_unit;
+	  gdb_byte b = buffer[idx];
+	  if (needs_escaping (b))
+	    {
+	      out_buf[output_byte_index++] = '}';
+	      out_buf[output_byte_index++] = b ^ 0x20;
+	    }
+	  else
+	    out_buf[output_byte_index++] = b;
 	}
     }

-  *out_len = input_index;
-  return output_index;
+  *out_len_units = input_unit_index;
+  return output_byte_index;
 }

 /* See rsp-low.h.  */
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index d62f67e..f45d0ae 100644
--- a/gdb/common/rsp-low.h
+++ b/gdb/common/rsp-low.h
@@ -59,17 +59,17 @@ extern int hex2bin (const char *hex, gdb_byte *bin, int count);

 extern int bin2hex (const gdb_byte *bin, char *hex, int count);

-/* Convert BUFFER, binary data at least LEN bytes long, into escaped
-   binary data in OUT_BUF.  Set *OUT_LEN to the length of the data
-   encoded in OUT_BUF, and return the number of bytes in OUT_BUF
-   (which may be more than *OUT_LEN due to escape characters).  The
-   total number of bytes in the output buffer will be at most
-   OUT_MAXLEN.  This function properly escapes '*', and so is suitable
+/* Convert BUFFER, binary data at least LEN_UNITS addressable memory units
+   long, into escaped binary data in OUT_BUF.  Only copy memory units that fit
+   completely in OUT_BUF.  Set *OUT_LEN_UNITS to the number of units from
+   BUFFER successfully encoded in OUT_BUF, and return the number of bytes used
+   in OUT_BUF.  The total number of bytes in the output buffer will be at most
+   OUT_MAXLEN_BYTES.  This function properly escapes '*', and so is suitable
    for the server side as well as the client.  */

-extern int remote_escape_output (const gdb_byte *buffer, int len,
-				 gdb_byte *out_buf, int *out_len,
-				 int out_maxlen);
+extern int remote_escape_output (const gdb_byte *buffer, int len_units,
+				 int unit_size, gdb_byte *out_buf,
+				 int *out_len_units, int out_maxlen_bytes);

 /* Convert BUFFER, escaped data LEN bytes long, into binary data
    in OUT_BUF.  Return the number of bytes written to OUT_BUF.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 01b9c96..c9effc2 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -387,8 +387,8 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
   else
     buf[0] = 'l';

-  return remote_escape_output (data, len, (unsigned char *) buf + 1, &out_len,
-			       PBUFSIZ - 2) + 1;
+  return remote_escape_output (data, len, 1, (unsigned char *) buf + 1,
+			       &out_len, PBUFSIZ - 2) + 1;
 }

 /* Handle btrace enabling in BTS format.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index 6dd85fb..f15d75e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6921,51 +6921,74 @@ check_binary_download (CORE_ADDR addr)
     }
 }

+/* Helper function to resize the payload in order to try to get a good
+   alignment.  We try to write an amount of data such that the next write will
+   start on an address aligned on REMOTE_ALIGN_WRITES.  */
+
+static int
+align_for_efficient_write (int todo, CORE_ADDR memaddr)
+{
+  return ((memaddr + todo) & ~(REMOTE_ALIGN_WRITES - 1)) - memaddr;
+}
+
 /* Write memory data directly to the remote machine.
    This does not inform the data cache; the data cache uses this.
    HEADER is the starting part of the packet.
    MEMADDR is the address in the remote memory space.
    MYADDR is the address of the buffer in our space.
-   LEN is the number of bytes.
+   LEN_UNITS is the number of addressable units to write.
+   UNIT_SIZE is the length in bytes of an addressable unit.
    PACKET_FORMAT should be either 'X' or 'M', and indicates if we
    should send data as binary ('X'), or hex-encoded ('M').

    The function creates packet of the form
        <HEADER><ADDRESS>,<LENGTH>:<DATA>

-   where encoding of <DATA> is termined by PACKET_FORMAT.
+   where encoding of <DATA> is terminated by PACKET_FORMAT.

    If USE_LENGTH is 0, then the <LENGTH> field and the preceding comma
    are omitted.

    Return the transferred status, error or OK (an
-   'enum target_xfer_status' value).  Save the number of bytes
-   transferred in *XFERED_LEN.  Only transfer a single packet.  */
+   'enum target_xfer_status' value).  Save the number of addressable units
+   transferred in *XFERED_LEN_UNITS.  Only transfer a single packet.
+
+   On a platform with an addressable memory size of 2 bytes (UNIT_SIZE == 2), an
+   exchange between gdb and the stub could look like (?? in place of the
+   checksum):
+
+   -> $m1000,4#??
+   <- aaaabbbbccccdddd
+
+   -> $M1000,3:eeeeffffeeee#??
+   <- OK
+
+   -> $m1000,4#??
+   <- eeeeffffeeeedddd  */

 static enum target_xfer_status
 remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
-			const gdb_byte *myaddr, ULONGEST len,
-			ULONGEST *xfered_len, char packet_format,
-			int use_length)
+			const gdb_byte *myaddr, ULONGEST len_units,
+			int unit_size, ULONGEST *xfered_len_units,
+			char packet_format, int use_length)
 {
   struct remote_state *rs = get_remote_state ();
   char *p;
   char *plen = NULL;
   int plenlen = 0;
-  int todo;
-  int nr_bytes;
-  int payload_size;
-  int payload_length;
-  int header_length;
+  int todo_units;
+  int units_written;
+  int payload_capacity_bytes;
+  int payload_length_bytes;

   if (packet_format != 'X' && packet_format != 'M')
     internal_error (__FILE__, __LINE__,
 		    _("remote_write_bytes_aux: bad packet format"));

-  if (len == 0)
+  if (len_units == 0)
     return TARGET_XFER_EOF;

-  payload_size = get_memory_write_packet_size ();
+  payload_capacity_bytes = get_memory_write_packet_size ();

   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */
@@ -6974,13 +6997,12 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   /* Compute the size of the actual payload by subtracting out the
      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */

-  payload_size -= strlen ("$,:#NN");
+  payload_capacity_bytes -= strlen ("$,:#NN");
   if (!use_length)
     /* The comma won't be used.  */
-    payload_size += 1;
-  header_length = strlen (header);
-  payload_size -= header_length;
-  payload_size -= hexnumlen (memaddr);
+    payload_capacity_bytes += 1;
+  payload_capacity_bytes -= strlen (header);
+  payload_capacity_bytes -= hexnumlen (memaddr);

   /* Construct the packet excluding the data: "<header><memaddr>,<len>:".  */

@@ -6991,28 +7013,28 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   if (packet_format == 'X')
     {
       /* Best guess at number of bytes that will fit.  */
-      todo = min (len, payload_size);
+      todo_units = min (len_units, payload_capacity_bytes / unit_size);
       if (use_length)
-	payload_size -= hexnumlen (todo);
-      todo = min (todo, payload_size);
+	payload_capacity_bytes -= hexnumlen (todo_units);
+      todo_units = min (todo_units, payload_capacity_bytes / unit_size);
     }
   else
     {
-      /* Num bytes that will fit.  */
-      todo = min (len, payload_size / 2);
+      /* Number of bytes that will fit.  */
+      todo_units = min (len_units, (payload_capacity_bytes / unit_size) / 2);
       if (use_length)
-	payload_size -= hexnumlen (todo);
-      todo = min (todo, payload_size / 2);
+	payload_capacity_bytes -= hexnumlen (todo_units);
+      todo_units = min (todo_units, (payload_capacity_bytes / unit_size) / 2);
     }

-  if (todo <= 0)
+  if (todo_units <= 0)
     internal_error (__FILE__, __LINE__,
 		    _("minimum packet size too small to write data"));

   /* If we already need another packet, then try to align the end
      of this packet to a useful boundary.  */
-  if (todo > 2 * REMOTE_ALIGN_WRITES && todo < len)
-    todo = ((memaddr + todo) & ~(REMOTE_ALIGN_WRITES - 1)) - memaddr;
+  if (todo_units > 2 * REMOTE_ALIGN_WRITES && todo_units < len_units)
+    todo_units = align_for_efficient_write (todo_units, memaddr);

   /* Append "<memaddr>".  */
   memaddr = remote_address_masked (memaddr);
@@ -7023,10 +7045,10 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Append ",".  */
       *p++ = ',';

-      /* Append <len>.  Retain the location/size of <len>.  It may need to
-	 be adjusted once the packet body has been created.  */
+      /* Append the length and retain its location and size.  It may need to be
+         adjusted once the packet body has been created.  */
       plen = p;
-      plenlen = hexnumstr (p, (ULONGEST) todo);
+      plenlen = hexnumstr (p, (ULONGEST) todo_units);
       p += plenlen;
     }

@@ -7040,32 +7062,35 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Binary mode.  Send target system values byte by byte, in
 	 increasing byte addresses.  Only escape certain critical
 	 characters.  */
-      payload_length = remote_escape_output (myaddr, todo, (gdb_byte *) p,
-					     &nr_bytes, payload_size);
+      payload_length_bytes =
+	  remote_escape_output (myaddr, todo_units, unit_size, (gdb_byte *) p,
+				&units_written, payload_capacity_bytes);

-      /* If not all TODO bytes fit, then we'll need another packet.  Make
+      /* If not all TODO units fit, then we'll need another packet.  Make
 	 a second try to keep the end of the packet aligned.  Don't do
 	 this if the packet is tiny.  */
-      if (nr_bytes < todo && nr_bytes > 2 * REMOTE_ALIGN_WRITES)
+      if (units_written < todo_units && units_written > 2 * REMOTE_ALIGN_WRITES)
 	{
-	  int new_nr_bytes;
-
-	  new_nr_bytes = (((memaddr + nr_bytes) & ~(REMOTE_ALIGN_WRITES - 1))
-			  - memaddr);
-	  if (new_nr_bytes != nr_bytes)
-	    payload_length = remote_escape_output (myaddr, new_nr_bytes,
-						   (gdb_byte *) p, &nr_bytes,
-						   payload_size);
+	  int new_todo_units;
+
+	  new_todo_units = align_for_efficient_write (units_written, memaddr);
+
+	  if (new_todo_units != units_written)
+	    payload_length_bytes =
+		remote_escape_output (myaddr, new_todo_units, unit_size,
+				      (gdb_byte *) p, &units_written,
+				      payload_capacity_bytes);
 	}

-      p += payload_length;
-      if (use_length && nr_bytes < todo)
+      p += payload_length_bytes;
+      if (use_length && units_written < todo_units)
 	{
 	  /* Escape chars have filled up the buffer prematurely,
-	     and we have actually sent fewer bytes than planned.
+	     and we have actually sent fewer units than planned.
 	     Fix-up the length field of the packet.  Use the same
 	     number of characters as before.  */
-	  plen += hexnumnstr (plen, (ULONGEST) nr_bytes, plenlen);
+	  plen += hexnumnstr (plen, (ULONGEST) units_written,
+			      plenlen);
 	  *plen = ':';  /* overwrite \0 from hexnumnstr() */
 	}
     }
@@ -7074,8 +7099,8 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
       /* Normal mode: Send target system values byte by byte, in
 	 increasing byte addresses.  Each byte is encoded as a two hex
 	 value.  */
-      nr_bytes = bin2hex (myaddr, p, todo);
-      p += 2 * nr_bytes;
+      p += 2 * bin2hex (myaddr, p, todo_units * unit_size);
+      units_written = todo_units;
     }

   putpkt_binary (rs->buf, (int) (p - rs->buf));
@@ -7084,9 +7109,9 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,
   if (rs->buf[0] == 'E')
     return TARGET_XFER_E_IO;

-  /* Return NR_BYTES, not TODO, in case escape chars caused us to send
-     fewer bytes than we'd planned.  */
-  *xfered_len = (ULONGEST) nr_bytes;
+  /* Return UNITS_WRITTEN, not TODO_UNITS, in case escape chars caused us to
+     send fewer units than we'd planned.  */
+  *xfered_len_units = (ULONGEST) units_written;
   return TARGET_XFER_OK;
 }

@@ -7102,7 +7127,7 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,

 static enum target_xfer_status
 remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
-		    ULONGEST *xfered_len)
+		    int unit_size, ULONGEST *xfered_len)
 {
   char *packet_format = 0;

@@ -7125,7 +7150,7 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
     }

   return remote_write_bytes_aux (packet_format,
-				 memaddr, myaddr, len, xfered_len,
+				 memaddr, myaddr, len, unit_size, xfered_len,
 				 packet_format[0], 1);
 }

@@ -7133,28 +7158,32 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
    This does not use the data cache; the data cache uses this.
    MEMADDR is the address in the remote memory space.
    MYADDR is the address of the buffer in our space.
-   LEN is the number of bytes.
+   LEN_UNITS is the number of addressable memory units to read..
+   UNIT_SIZE is the length in bytes of an addressable unit.

    Return the transferred status, error or OK (an
    'enum target_xfer_status' value).  Save the number of bytes
-   transferred in *XFERED_LEN.  */
+   transferred in *XFERED_LEN_UNITS.
+
+   See the comment of remote_write_bytes_aux for an example of
+   memory read/write exchange between gdb and the stub.  */

 static enum target_xfer_status
-remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
-		     ULONGEST *xfered_len)
+remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len_units,
+		     int unit_size, ULONGEST *xfered_len_units)
 {
   struct remote_state *rs = get_remote_state ();
-  int max_buf_size;		/* Max size of packet output buffer.  */
+  int buf_size_bytes;		/* Max size of packet output buffer.  */
   char *p;
-  int todo;
-  int i;
+  int todo_units;
+  int decoded_bytes;

-  max_buf_size = get_memory_read_packet_size ();
+  buf_size_bytes = get_memory_read_packet_size ();
   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */

-  /* Number if bytes that will fit.  */
-  todo = min (len, max_buf_size / 2);
+  /* Number of units that will fit.  */
+  todo_units = min (len_units, (buf_size_bytes / unit_size) / 2);

   /* Construct "m"<memaddr>","<len>".  */
   memaddr = remote_address_masked (memaddr);
@@ -7162,7 +7191,7 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
   *p++ = 'm';
   p += hexnumstr (p, (ULONGEST) memaddr);
   *p++ = ',';
-  p += hexnumstr (p, (ULONGEST) todo);
+  p += hexnumstr (p, (ULONGEST) todo_units);
   *p = '\0';
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -7173,9 +7202,9 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
   /* Reply describes memory byte by byte, each byte encoded as two hex
      characters.  */
   p = rs->buf;
-  i = hex2bin (p, myaddr, todo);
+  decoded_bytes = hex2bin (p, myaddr, todo_units * unit_size);
   /* Return what we have.  Let higher layers handle partial reads.  */
-  *xfered_len = (ULONGEST) i;
+  *xfered_len_units = (ULONGEST) (decoded_bytes / unit_size);
   return TARGET_XFER_OK;
 }

@@ -7188,7 +7217,7 @@ remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
 static enum target_xfer_status
 remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 				   ULONGEST memaddr, ULONGEST len,
-				   ULONGEST *xfered_len)
+				   int unit_size, ULONGEST *xfered_len)
 {
   struct target_section *secp;
   struct target_section_table *table;
@@ -7211,7 +7240,7 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 	      if (memend <= p->endaddr)
 		{
 		  /* Entire transfer is within this section.  */
-		  return remote_read_bytes_1 (memaddr, readbuf, len,
+		  return remote_read_bytes_1 (memaddr, readbuf, len, unit_size,
 					      xfered_len);
 		}
 	      else if (memaddr >= p->endaddr)
@@ -7223,7 +7252,7 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,
 		{
 		  /* This section overlaps the transfer.  Just do half.  */
 		  len = p->endaddr - memaddr;
-		  return remote_read_bytes_1 (memaddr, readbuf, len,
+		  return remote_read_bytes_1 (memaddr, readbuf, len, unit_size,
 					      xfered_len);
 		}
 	    }
@@ -7239,7 +7268,8 @@ remote_xfer_live_readonly_partial (struct target_ops *ops, gdb_byte *readbuf,

 static enum target_xfer_status
 remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
-		   gdb_byte *myaddr, ULONGEST len, ULONGEST *xfered_len)
+		   gdb_byte *myaddr, ULONGEST len, int unit_size,
+		   ULONGEST *xfered_len)
 {
   if (len == 0)
     return TARGET_XFER_EOF;
@@ -7277,7 +7307,7 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,

 	      /* This goes through the topmost target again.  */
 	      res = remote_xfer_live_readonly_partial (ops, myaddr, memaddr,
-						       len, xfered_len);
+						       len, unit_size, xfered_len);
 	      if (res == TARGET_XFER_OK)
 		return TARGET_XFER_OK;
 	      else
@@ -7299,7 +7329,7 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
 	}
     }

-  return remote_read_bytes_1 (memaddr, myaddr, len, xfered_len);
+  return remote_read_bytes_1 (memaddr, myaddr, len, unit_size, xfered_len);
 }

 \f
@@ -7385,7 +7415,7 @@ remote_flash_write (struct target_ops *ops, ULONGEST address,
 					  &saved_remote_timeout);

   remote_timeout = remote_flash_timeout;
-  ret = remote_write_bytes_aux ("vFlashWrite:", address, data, length,
+  ret = remote_write_bytes_aux ("vFlashWrite:", address, data, length, 1,
 				xfered_len,'X', 0);
   do_cleanups (back_to);

@@ -9125,7 +9155,7 @@ remote_write_qxfer (struct target_ops *ops, const char *object_name,

   /* Escape as much data as fits into rs->buf.  */
   buf_len = remote_escape_output
-    (writebuf, len, (gdb_byte *) rs->buf + i, &max_size, max_size);
+    (writebuf, len, 1, (gdb_byte *) rs->buf + i, &max_size, max_size);

   if (putpkt_binary (rs->buf, i + buf_len) < 0
       || getpkt_sane (&rs->buf, &rs->buf_size, 0) < 0
@@ -9236,6 +9266,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
   int i;
   char *p2;
   char query_type;
+  int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());

   set_remote_traceframe ();
   set_general_thread (inferior_ptid);
@@ -9252,9 +9283,11 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
 	return TARGET_XFER_EOF;

       if (writebuf != NULL)
-	return remote_write_bytes (offset, writebuf, len, xfered_len);
+	return remote_write_bytes (offset, writebuf, len, unit_size,
+				   xfered_len);
       else
-	return remote_read_bytes (ops, offset, readbuf, len, xfered_len);
+	return remote_read_bytes (ops, offset, readbuf, len, unit_size,
+				  xfered_len);
     }

   /* Handle SPU memory using qxfer packets.  */
@@ -9492,7 +9525,7 @@ remote_search_memory (struct target_ops* ops,

   /* Escape as much data as fits into rs->buf.  */
   escaped_pattern_len =
-    remote_escape_output (pattern, pattern_len, (gdb_byte *) rs->buf + i,
+    remote_escape_output (pattern, pattern_len, 1, (gdb_byte *) rs->buf + i,
 			  &used_pattern_len, max_size);

   /* Bail if the pattern is too large.  */
@@ -10307,7 +10340,7 @@ remote_hostio_pwrite (struct target_ops *self,
   remote_buffer_add_int (&p, &left, offset);
   remote_buffer_add_string (&p, &left, ",");

-  p += remote_escape_output (write_buf, len, (gdb_byte *) p, &out_len,
+  p += remote_escape_output (write_buf, len, 1, (gdb_byte *) p, &out_len,
 			     get_remote_packet_size () - (p - rs->buf));

   return remote_hostio_send_command (p - rs->buf, PACKET_vFile_pwrite,
-- 
2.1.4

Re: [PATCH v2 3/7] Clarify doc about memory read/write and non-8-bits addressable memory unit sizes

[Simon Marchi]

>>
>> Thanks, here is the same patch with those fixed.
> 
> OK.
> 

Pushed, thanks.

Re: [PATCH v2 7/7] MI: consider addressable unit size when reading/writing memory

[Simon Marchi]

On 15-05-21 01:52 PM, Pedro Alves wrote:
> On 04/15/2015 08:47 PM, Simon Marchi wrote:
>> As a user of the target memory read/write interface, the MI code must
>> adjust its memory allocations to take into account the addressable memory
>> unitsize of the target.
> 
> Looks OK to me.
> 
> Thanks,
> Pedro Alves

Thanks, pushed.

Re: [PATCH v2 6/7] remote: consider addressable unit size when reading/writing memory

[Pedro Alves]

On 06/15/2015 08:28 PM, Simon Marchi wrote:

>>>    -> $m1000,8#??
>>>    <- aaaabbbbccccdddd
>>>
>>>    -> $M1000,6:eeeeffffeeee#??
>>>    <- OK
>>>
>>>    -> $m1000,8#??
>>>    <- eeeeffffeeeedddd
>>>
> 
> Ok, I put the example in the comment of remote_write_bytes_aux and added
> a reference to it in remote_read_bytes_1. Do you have a more suitable place
> in mind where to put it?

Nope, that sounds good.

> Thanks for the review. Here's what's new:
> 
> * needs_escaping: Added newline and comment.
> * remote_escape_output: Add missing space.
> * remote_write_bytes_aux: Send length in addressable units, update comment with example gdb/stub exchange.
> * remote_read_bytes_1: Same, and update function comment (add UNIT_SIZE).

Looks good to me.

Thanks,
Pedro Alves

Re: [PATCH v2 6/7] remote: consider addressable unit size when reading/writing memory

[Simon Marchi]

On 15-06-17 07:55 AM, Pedro Alves wrote:
> On 06/15/2015 08:28 PM, Simon Marchi wrote:
> 
>>>>    -> $m1000,8#??
>>>>    <- aaaabbbbccccdddd
>>>>
>>>>    -> $M1000,6:eeeeffffeeee#??
>>>>    <- OK
>>>>
>>>>    -> $m1000,8#??
>>>>    <- eeeeffffeeeedddd
>>>>
>>
>> Ok, I put the example in the comment of remote_write_bytes_aux and added
>> a reference to it in remote_read_bytes_1. Do you have a more suitable place
>> in mind where to put it?
> 
> Nope, that sounds good.
> 
>> Thanks for the review. Here's what's new:
>>
>> * needs_escaping: Added newline and comment.
>> * remote_escape_output: Add missing space.
>> * remote_write_bytes_aux: Send length in addressable units, update comment with example gdb/stub exchange.
>> * remote_read_bytes_1: Same, and update function comment (add UNIT_SIZE).
> 
> Looks good to me.
> 
> Thanks,
> Pedro Alves

Thanks, pushed.