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

Simon Marchi
2015-04-15T19:48:03Z

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
<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: " <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