[PATCH 6/7] remote: Consider byte size when reading/writing memory

Simon Marchi
2015-04-08T19:56:40Z

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

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

gdb/ChangeLog:

* common/rsp-low.c (needs_escaping): New.
(remote_escape_output): Add byte_size parameter and refactor to
use it. Rename parameters.
* common/rsp-low.h (remote_escape_output): Add byte_size
parameter and rename others. Update doc.
* remote.c (align_for_efficient_write): New.
(remote_write_bytes_aux): Add byte_size parameter and use it.
Minor refactoring and variable renaming.
(remote_xfer_partial): Get byte size and use it.
(remote_read_bytes_1): Add byte_size parameter and use it.
(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 | 16 +++---
gdb/gdbserver/server.c | 4 +-
gdb/remote.c | 153 +++++++++++++++++++++++++++----------------------
4 files changed, 145 insertions(+), 99 deletions(-)

diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index d3d3d65..6e21208 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;
}

+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_bytes, int byte_size,
+ gdb_byte *out_buf, int *out_len_bytes,
+ int out_maxlen_octets)
{
- int input_index, output_index;
-
- output_index = 0;
- for (input_index = 0; input_index < len; input_index++)
+ int input_byte_index, output_octet_index, octet_index_in_byte;
+
+ /* The number of octets we'll need to escape in that byte. */
+ int number_escape_octets_needed;
+
+ /* Try to copy integral target memory bytes until
+ (1) we run out of space or
+ (2) we copied all of them. */
+ output_octet_index = 0;
+ for (input_byte_index = 0;
+ input_byte_index < len_bytes;
+ input_byte_index++)
{
- gdb_byte b = buffer[input_index];
-
- if (b == '$' || b == '#' || b == '}' || b == '*')
+ /* Find out how many escape octets we need for this byte. */
+ number_escape_octets_needed = 0;
+ for (octet_index_in_byte = 0;
+ octet_index_in_byte < byte_size;
+ octet_index_in_byte++)
{
- /* These must be escaped. */
- if (output_index + 2 > out_maxlen)
- break;
- out_buf[output_index++] = '}';
- out_buf[output_index++] = b ^ 0x20;
+ int idx = input_byte_index * byte_size + octet_index_in_byte;
+ gdb_byte b = buffer[idx];
+ if (needs_escaping (b))
+ number_escape_octets_needed++;
}
- else
+
+ /* Check if we have room to fit this escaped byte. */
+ if (output_octet_index + byte_size + number_escape_octets_needed >
+ out_maxlen_octets)
+ break;
+
+ /* Copy the byte octet per octet, adding escapes. */
+ for (octet_index_in_byte = 0;
+ octet_index_in_byte < byte_size;
+ octet_index_in_byte++)
{
- if (output_index + 1 > out_maxlen)
- break;
- out_buf[output_index++] = b;
+ int idx = input_byte_index * byte_size + octet_index_in_byte;
+ gdb_byte b = buffer[idx];
+ if (needs_escaping(b))
+ {
+ out_buf[output_octet_index++] = '}';
+ out_buf[output_octet_index++] = b ^ 0x20;
+ }
+ else
+ out_buf[output_octet_index++] = b;
}
}

- *out_len = input_index;
- return output_index;
+ *out_len_bytes = input_byte_index;
+ return output_octet_index;
}

/* See rsp-low.h. */
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index d62f67e..c20e88c 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
+/* Convert BUFFER, binary data at least LEN_BYTES bytes long, into escaped
+ binary data in OUT_BUF. Only copy target memory bytes that fit completely
+ in OUT_BUF. Set *OUT_LEN_BYTES to the number of bytes from BUFFER
+ successfully encoded in OUT_BUF, and return the number of octets used in
+ OUT_BUF. The total number of octets in the output buffer will be at most
OUT_MAXLEN. 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_bytes,
+ int byte_size, gdb_byte *out_buf,
+ int *out_len_bytes, int out_maxlen_octets);

/* 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..d9b4350 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6639,6 +6639,16 @@ 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_bytes, CORE_ADDR memaddr)
+{
+ return ((memaddr + todo_bytes) & ~(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.
@@ -6651,7 +6661,7 @@ check_binary_download (CORE_ADDR addr)
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.
@@ -6662,28 +6672,27 @@ check_binary_download (CORE_ADDR addr)

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,
+ const gdb_byte *myaddr, ULONGEST len_bytes,
+ int byte_size, ULONGEST *xfered_len_bytes, 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_bytes;
+ int bytes_written;
+ int payload_capacity_octets;
+ int payload_length_octets;

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

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

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

/* The packet buffer will be large enough for the payload;
get_memory_packet_size ensures this. */
@@ -6692,13 +6701,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_octets -= 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_octets += 1;
+ payload_capacity_octets -= strlen (header);
+ payload_capacity_octets -= hexnumlen (memaddr);

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

@@ -6709,28 +6717,29 @@ 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_bytes = min (len_bytes, payload_capacity_octets / byte_size);
if (use_length)
- payload_size -= hexnumlen (todo);
- todo = min (todo, payload_size);
+ /* The size in the packet is expressed in octets, not target bytes. */
+ payload_capacity_octets -= hexnumlen (todo_bytes * byte_size);
+ todo_bytes = min (todo_bytes, payload_capacity_octets / byte_size);
}
else
{
- /* Num bytes that will fit. */
- todo = min (len, payload_size / 2);
+ /* Number of bytes that will fit. */
+ todo_bytes = min (len_bytes, (payload_capacity_octets / byte_size) / 2);
if (use_length)
- payload_size -= hexnumlen (todo);
- todo = min (todo, payload_size / 2);
+ payload_capacity_octets -= hexnumlen (todo_bytes * byte_size);
+ todo_bytes = min (todo_bytes, (payload_capacity_octets / byte_size) / 2);
}

- if (todo <= 0)
+ if (todo_bytes <= 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_bytes > 2 * REMOTE_ALIGN_WRITES && todo_bytes < len_bytes)
+ todo_bytes = align_for_efficient_write (todo_bytes, memaddr);

/* Append "<memaddr>". */
memaddr = remote_address_masked (memaddr);
@@ -6741,10 +6750,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_bytes>. 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_bytes * byte_size);
p += plenlen;
}

@@ -6758,42 +6767,45 @@ 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_octets =
+ remote_escape_output (myaddr, todo_bytes, byte_size, (gdb_byte *) p,
+ &bytes_written, payload_capacity_octets);

/* If not all TODO bytes 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 (bytes_written < todo_bytes && bytes_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_bytes;
+
+ new_todo_bytes = align_for_efficient_write (bytes_written, memaddr);
+
+ if (new_todo_bytes != bytes_written)
+ payload_length_octets =
+ remote_escape_output (myaddr, new_todo_bytes, byte_size,
+ (gdb_byte *) p, &bytes_written,
+ payload_capacity_octets);
}

- p += payload_length;
- if (use_length && nr_bytes < todo)
+ p += payload_length_octets;
+ if (use_length && bytes_written < todo_bytes)
{
/* Escape chars have filled up the buffer prematurely,
and we have actually sent fewer bytes 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) bytes_written * byte_size,
+ plenlen);
*plen = ':'; /* overwrite \0 from hexnumnstr() */
}
}
else
{
/* Normal mode: Send target system values byte by byte, in
- increasing byte addresses. Each byte is encoded as a two hex
+ increasing byte addresses. Each octet is encoded as a two hex
value. */
- nr_bytes = bin2hex (myaddr, p, todo);
- p += 2 * nr_bytes;
+ p += 2 * bin2hex (myaddr, p, todo_bytes * byte_size);
+ bytes_written = todo_bytes;
}

putpkt_binary (rs->buf, (int) (p - rs->buf));
@@ -6804,7 +6816,7 @@ remote_write_bytes_aux (const char *header, CORE_ADDR memaddr,

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

@@ -6820,7 +6832,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 byte_size, ULONGEST *xfered_len)
{
char *packet_format = 0;

@@ -6843,7 +6855,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, byte_size, xfered_len,
packet_format[0], 1);
}

@@ -6859,20 +6871,21 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,

static enum target_xfer_status
remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
- ULONGEST *xfered_len)
+ int byte_size, ULONGEST *xfered_len)
{
struct remote_state *rs = get_remote_state ();
int max_buf_size; /* Max size of packet output buffer. */
char *p;
- int todo;
- int i;
+ int todo_bytes;
+ int decoded_octets;
+ int len_octets = len * byte_size;

max_buf_size = 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_bytes = min (len, (max_buf_size / byte_size) / 2);

/* Construct "m"<memaddr>","<len>". */
memaddr = remote_address_masked (memaddr);
@@ -6880,7 +6893,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_bytes * byte_size);
*p = '\0';
putpkt (rs->buf);
getpkt (&rs->buf, &rs->buf_size, 0);
@@ -6891,9 +6904,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_octets = hex2bin (p, myaddr, todo_bytes * byte_size);
/* Return what we have. Let higher layers handle partial reads. */
- *xfered_len = (ULONGEST) i;
+ *xfered_len = (ULONGEST) (decoded_octets / byte_size);
return TARGET_XFER_OK;
}

@@ -6906,7 +6919,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 byte_size, ULONGEST *xfered_len)
{
struct target_section *secp;
struct target_section_table *table;
@@ -6929,7 +6942,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, byte_size,
xfered_len);
}
else if (memaddr >= p->endaddr)
@@ -6941,7 +6954,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, byte_size,
xfered_len);
}
}
@@ -6957,7 +6970,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 byte_size,
+ ULONGEST *xfered_len)
{
if (len == 0)
return TARGET_XFER_EOF;
@@ -6995,7 +7009,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, byte_size, xfered_len);
if (res == TARGET_XFER_OK)
return TARGET_XFER_OK;
else
@@ -7017,7 +7031,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, byte_size, xfered_len);
}

\f
@@ -7103,7 +7117,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 +8789,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 +8900,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
int i;
char *p2;
char query_type;
+ int byte_size = gdbarch_memory_byte_size (target_gdbarch ());

set_remote_traceframe ();
set_general_thread (inferior_ptid);
@@ -8902,9 +8917,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, byte_size,
+ xfered_len);
else
- return remote_read_bytes (ops, offset, readbuf, len, xfered_len);
+ return remote_read_bytes (ops, offset, readbuf, len, byte_size,
+ xfered_len);
}

/* Handle SPU memory using qxfer packets. */
@@ -9137,7 +9154,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 +9939,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