[rfc] Correct semantics of target_read_partial, add target_read_whole

Daniel Jacobowitz
2006-06-22T03:24:07Z

Originally, target_read_partial was supposed to read "however much it could
manage to" and then higher level functions were supposed to handle the rest.
But every existing implementation always reads enough data in its first
call; the one remote protocol implementation did so by issuing as many
packets as necessary, which defeated the point of the original design.

This patch adjusts the remote protocol layer not to do that. It also
promotes a useful function from auxv.c to target.c:

+/* Wrappers to perform a full read of unknown size. OBJECT/ANNEX will
+ be read using OPS. The return value will be -1 if the transfer
+ fails or is not supported; 0 if the object is empty; and the length
+ of the object otherwise. If a positive value is returned, a
+ sufficiently large buffer will be allocated using xmalloc and
+ returned in *BUF_P containing the contents of the object.
+
+ This method should be used for objects sufficiently small to store
+ in a single xmalloced buffer, when no fixed bound on the object's
+ size is known in advance. Don't try to read TARGET_OBJECT_MEMORY
+ through this function. */
+
+extern LONGEST target_read_whole (struct target_ops *ops,
+ enum target_object object,
+ const char *annex, gdb_byte **buf_p);

When you use to_xfer_partial to get at memory, obviously you don't want "the
whole object". But for other objects, such as auxv vectors or XML
description files, usually you do. This provides a central interface
to handle short reads correctly, instead of letting that code circulate
(buggily in many cases) through other files.

It should have no net measurable change on GDB's behavior, and I've been
using an earlier version of this patch for several months now. I verified
that the same number of qPart:auxv:read packets are used when talking to
gdbserver; the third patch in this sequence will cut it down from two to
one.

Unless someone sees a problem with this patch, I will commit it when the
qXfer changes are ready.

--
Daniel Jacobowitz
CodeSourcery

2006-06-21 Daniel Jacobowitz <dan@codesourcery.com>

* target.c (target_read): Stop if target_read_partial returns 0
when some bytes have already been read.
(target_write): Likewise for target_write_partial.
(target_read_whole): New.
* target.h (target_read): Improve description.
(target_read_whole): New prototype.

* auxv.c (target_auxv_read): Delete.
(target_auxv_search, fprint_target_auxv): Use target_read_whole.
* auxv.h (target_auxv_read): Delete prototype.
* avr-tdep.c (avr_io_reg_read_command): Use target_read_whole.
* ia64-tdep.c (getunwind_table, get_kernel_table): Likewise.
* linux-nat.c (linux_nat_make_corefile_notes): Likewise.
* procfs.c (procfs_make_note_section): Likewise.
* remote.c (remote_xfer_partial): Don't loop here.
* sparc-tdep.c (sparc_fetch_wcookie): Use target_read.

Index: src/gdb/auxv.c
===================================================================
--- src.orig/gdb/auxv.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/auxv.c 2006-06-21 17:06:27.000000000 -0400
@@ -76,43 +76,6 @@ procfs_xfer_auxv (struct target_ops *ops
return n;
}

-/* Read all the auxv data into a contiguous xmalloc'd buffer,
- stored in *DATA. Return the size in bytes of this data.
- If zero, there is no data and *DATA is null.
- if < 0, there was an error and *DATA is null. */
-LONGEST
-target_auxv_read (struct target_ops *ops, gdb_byte **data)
-{
- size_t auxv_alloc = 512, auxv_pos = 0;
- gdb_byte *auxv = xmalloc (auxv_alloc);
- int n;
-
- while (1)
- {
- n = target_read_partial (ops, TARGET_OBJECT_AUXV,
- NULL, &auxv[auxv_pos], 0,
- auxv_alloc - auxv_pos);
- if (n <= 0)
- break;
- auxv_pos += n;
- if (auxv_pos < auxv_alloc) /* Read all there was. */
- break;
- gdb_assert (auxv_pos == auxv_alloc);
- auxv_alloc *= 2;
- auxv = xrealloc (auxv, auxv_alloc);
- }
-
- if (auxv_pos == 0)
- {
- xfree (auxv);
- *data = NULL;
- return n;
- }
-
- *data = auxv;
- return auxv_pos;
-}
-
/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
Return 0 if *READPTR is already at the end of the buffer.
Return -1 if there is insufficient buffer for a whole entry.
@@ -148,7 +111,7 @@ target_auxv_search (struct target_ops *o
{
CORE_ADDR type, val;
gdb_byte *data;
- int n = target_auxv_read (ops, &data);
+ LONGEST n = target_read_whole (ops, TARGET_OBJECT_AUXV, NULL, &data);
gdb_byte *ptr = data;
int ents = 0;

@@ -184,7 +147,7 @@ fprint_target_auxv (struct ui_file *file
{
CORE_ADDR type, val;
gdb_byte *data;
- int len = target_auxv_read (ops, &data);
+ LONGEST len = target_read_whole (ops, TARGET_OBJECT_AUXV, NULL, &data);
gdb_byte *ptr = data;
int ents = 0;

Index: src/gdb/auxv.h
===================================================================
--- src.orig/gdb/auxv.h 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/auxv.h 2006-06-21 17:06:27.000000000 -0400
@@ -31,12 +31,6 @@
struct target_ops; /* Forward declaration. */

-/* Read all the auxv data into a contiguous xmalloc'd buffer,
- stored in *DATA. Return the size in bytes of this data.
- If zero, there is no data and *DATA is null.
- if < 0, there was an error and *DATA is null. */
-extern LONGEST target_auxv_read (struct target_ops *ops, gdb_byte **data);
-
/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
Return 0 if *READPTR is already at the end of the buffer.
Return -1 if there is insufficient buffer for a whole entry.
Index: src/gdb/avr-tdep.c
===================================================================
--- src.orig/gdb/avr-tdep.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/avr-tdep.c 2006-06-21 17:06:27.000000000 -0400
@@ -1323,35 +1323,22 @@ static void
avr_io_reg_read_command (char *args, int from_tty)
{
LONGEST bufsiz = 0;
- char buf[400];
+ gdb_byte *buf;
char query[400];
char *p;
unsigned int nreg = 0;
unsigned int val;
int i, j, k, step;

- /* Just get the maximum buffer size. */
- bufsiz = target_read_partial (&current_target, TARGET_OBJECT_AVR,
- NULL, NULL, 0, 0);
- if (bufsiz < 0)
- {
- fprintf_unfiltered (gdb_stderr,
- _("ERR: info io_registers NOT supported "
- "by current target\n"));
- return;
- }
- if (bufsiz > sizeof (buf))
- bufsiz = sizeof (buf);
-
/* Find out how many io registers the target has. */
- strcpy (query, "avr.io_reg");
- target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf, 0,
- bufsiz);
+ bufsiz = target_read_whole (&current_target, TARGET_OBJECT_AVR,
+ "avr.io_reg", &buf);

- if (strncmp (buf, "", bufsiz) == 0)
+ if (bufsiz <= 0)
{
fprintf_unfiltered (gdb_stderr,
- _("info io_registers NOT supported by target\n"));
+ _("ERR: info io_registers NOT supported "
+ "by current target\n"));
return;
}

@@ -1359,9 +1346,12 @@ avr_io_reg_read_command (char *args, int
{
fprintf_unfiltered (gdb_stderr,
_("Error fetching number of io registers\n"));
+ xfree (buf);
return;
}

+ xfree (buf);
+
reinitialize_more_filter ();

printf_unfiltered (_("Target has %u io registers:\n\n"), nreg);
@@ -1377,8 +1367,8 @@ avr_io_reg_read_command (char *args, int
j = nreg - i; /* last block is less than 8 registers */

snprintf (query, sizeof (query) - 1, "avr.io_reg:%x,%x", i, j);
- target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf,
- 0, bufsiz);
+ bufsiz = target_read_whole (&current_target, TARGET_OBJECT_AVR, query,
+ &buf);

p = buf;
for (k = i; k < (i + j); k++)
@@ -1393,6 +1383,8 @@ avr_io_reg_read_command (char *args, int
break;
}
}
+
+ xfree (buf);
}
}

Index: src/gdb/ia64-tdep.c
===================================================================
--- src.orig/gdb/ia64-tdep.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/ia64-tdep.c 2006-06-21 17:06:27.000000000 -0400
@@ -2458,8 +2458,8 @@ ia64_access_mem (unw_addr_space_t as,
}

/* Call low-level function to access the kernel unwind table. */
-static int
-getunwind_table (void *buf, size_t len)
+static LONGEST
+getunwind_table (gdb_byte **buf_p)
{
LONGEST x;

@@ -2470,10 +2470,11 @@ getunwind_table (void *buf, size_t len)
we want to preserve fall back to the running kernel's table, then
we should find a way to override the corefile layer's
xfer_partial method. */
- x = target_read_partial (&current_target, TARGET_OBJECT_UNWIND_TABLE, NULL,
- buf, 0, len);

- return (int)x;
+ x = target_read_whole (&current_target, TARGET_OBJECT_UNWIND_TABLE,
+ NULL, buf_p);
+
+ return x;
}

/* Get the kernel unwind table. */
@@ -2484,14 +2485,15 @@ get_kernel_table (unw_word_t ip, unw_dyn

if (!ktab)
{
+ gdb_byte *ktab_buf;
size_t size;
- size = getunwind_table (NULL, 0);
- if ((int)size < 0)
- return -UNW_ENOINFO;
- ktab_size = size;
- ktab = xmalloc (ktab_size);
- getunwind_table (ktab, ktab_size);
-
+
+ ktab_size = getunwind_table (&ktab_buf);
+ if (ktab_size <= 0)
+ return -UNW_ENOINFO;
+ else
+ ktab = (struct ia64_table_entry *) ktab_buf;
+
for (etab = ktab; etab->start_offset; ++etab)
etab->info_offset += KERNEL_START;
}
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/linux-nat.c 2006-06-21 17:06:27.000000000 -0400
@@ -2697,7 +2697,8 @@ linux_nat_make_corefile_notes (bfd *obfd
note_data = thread_args.note_data;
}

- auxv_len = target_auxv_read (&current_target, &auxv);
+ auxv_len = target_read_whole (&current_target, TARGET_OBJECT_AUXV, NULL,
+ &auxv);
if (auxv_len > 0)
{
note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/procfs.c
===================================================================
--- src.orig/gdb/procfs.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/procfs.c 2006-06-21 17:06:27.000000000 -0400
@@ -6130,7 +6130,8 @@ procfs_make_note_section (bfd *obfd, int
note_data = thread_args.note_data;
}

- auxv_len = target_auxv_read (&current_target, &auxv);
+ auxv_len = target_read_whole (&current_target, TARGET_OBJECT_AUXV, NULL,
+ &auxv);
if (auxv_len > 0)
{
note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/sparc-tdep.c
===================================================================
--- src.orig/gdb/sparc-tdep.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/sparc-tdep.c 2006-06-21 17:06:27.000000000 -0400
@@ -158,7 +158,7 @@ sparc_fetch_wcookie (void)
gdb_byte buf[8];
int len;

- len = target_read_partial (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
+ len = target_read (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
if (len == -1)
return 0;

Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/target.c 2006-06-21 17:06:27.000000000 -0400
@@ -1373,8 +1373,9 @@ target_read (struct target_ops *ops,
(gdb_byte *) buf + xfered,
offset + xfered, len - xfered);
/* Call an observer, notifying them of the xfer progress? */
- if (xfer <= 0)
- /* Call memory_error? */
+ if (xfer == 0)
+ return xfered;
+ if (xfer < 0)
return -1;
xfered += xfer;
QUIT;
@@ -1395,8 +1396,9 @@ target_write (struct target_ops *ops,
(gdb_byte *) buf + xfered,
offset + xfered, len - xfered);
/* Call an observer, notifying them of the xfer progress? */
- if (xfer <= 0)
- /* Call memory_error? */
+ if (xfer == 0)
+ return xfered;
+ if (xfer < 0)
return -1;
xfered += xfer;
QUIT;
@@ -1404,6 +1406,45 @@ target_write (struct target_ops *ops,
return len;
}

+/* Perform a full target read of unknown size. */
+
+LONGEST
+target_read_whole (struct target_ops *ops,
+ enum target_object object,
+ const char *annex, gdb_byte **buf_p)
+{
+ size_t buf_alloc = 512, buf_pos = 0;
+ gdb_byte *buf = xmalloc (buf_alloc);
+ LONGEST n, total;
+
+ total = 0;
+ while (1)
+ {
+ n = target_read (ops, object, annex, &buf[buf_pos],
+ buf_pos, buf_alloc - buf_pos);
+ if (n < 0)
+ {
+ /* An error occurred. */
+ xfree (buf);
+ return -1;
+ }
+
+ buf_pos += n;
+ if (buf_pos < buf_alloc)
+ {
+ /* Read all there was. */
+ if (buf_pos == 0)
+ xfree (buf);
+ else
+ *buf_p = buf;
+ return buf_pos;
+ }
+
+ buf_alloc *= 2;
+ buf = xrealloc (buf, buf_alloc);
+ }
+}
+
/* Memory transfer methods. */

void
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h 2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/target.h 2006-06-21 17:06:27.000000000 -0400
@@ -246,7 +246,10 @@ extern LONGEST target_write_partial (str
const char *annex, const gdb_byte *buf,
ULONGEST offset, LONGEST len);

-/* Wrappers to perform the full transfer. */
+/* Wrappers to perform a full transfer. These functions take the
+ same arguments as the partial versions, above, but a return
+ value of a positive number less than LEN implies that no more
+ data can be read or written. */
extern LONGEST target_read (struct target_ops *ops,
enum target_object object,
const char *annex, gdb_byte *buf,
@@ -257,6 +260,22 @@ extern LONGEST target_write (struct targ
const char *annex, const gdb_byte *buf,
ULONGEST offset, LONGEST len);

+/* Wrappers to perform a full read of unknown size. OBJECT/ANNEX will
+ be read using OPS. The return value will be -1 if the transfer
+ fails or is not supported; 0 if the object is empty; and the length
+ of the object otherwise. If a positive value is returned, a
+ sufficiently large buffer will be allocated using xmalloc and
+ returned in *BUF_P containing the contents of the object.
+
+ This method should be used for objects sufficiently small to store
+ in a single xmalloced buffer, when no fixed bound on the object's
+ size is known in advance. Don't try to read TARGET_OBJECT_MEMORY
+ through this function. */
+
+extern LONGEST target_read_whole (struct target_ops *ops,
+ enum target_object object,
+ const char *annex, gdb_byte **buf_p);
+
/* Wrappers to target read/write that perform memory transfers. They
throw an error if the memory transfer fails.

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c 2006-06-21 17:07:19.000000000 -0400
+++ src/gdb/remote.c 2006-06-21 17:08:38.000000000 -0400
@@ -5128,35 +5128,23 @@ remote_xfer_partial (struct target_ops *
case TARGET_OBJECT_AUXV:
if (remote_protocol_packets[PACKET_qPart_auxv].support != PACKET_DISABLE)
{
- unsigned int total = 0;
- while (len > 0)
- {
- LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
- snprintf (rs->buf, get_remote_packet_size (),
- "qPart:auxv:read::%s,%s",
- phex_nz (offset, sizeof offset),
- phex_nz (n, sizeof n));
- i = putpkt (rs->buf);
- if (i < 0)
- return total > 0 ? total : i;
- rs->buf[0] = '\0';
- getpkt (&rs->buf, &rs->buf_size, 0);
- if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
- != PACKET_OK)
- return total > 0 ? total : -1;
- if (strcmp (rs->buf, "OK") == 0)
- break; /* Got EOF indicator. */
- /* Got some data. */
- i = hex2bin (rs->buf, readbuf, len);
- if (i > 0)
- {
- readbuf = (void *) ((char *) readbuf + i);
- offset += i;
- len -= i;
- total += i;
- }
- }
- return total;
+ LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
+ snprintf (rs->buf, get_remote_packet_size (),
+ "qPart:auxv:read::%s,%s",
+ phex_nz (offset, sizeof offset),
+ phex_nz (n, sizeof n));
+ i = putpkt (rs->buf);
+ if (i < 0)
+ return i;
+ rs->buf[0] = '\0';
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
+ != PACKET_OK)
+ return -1;
+ if (strcmp (rs->buf, "OK") == 0)
+ return 0; /* Got EOF indicator. */
+ /* Got some data. */
+ return hex2bin (rs->buf, readbuf, len);
}
return -1;