Re: [PATCH 1/3] Move the traceframe_available_memory code from memory_xfer_partial_1 down to the targets

Yao Qi
2014-03-22T11:20:50Z

On 03/21/2014 07:56 PM, Pedro Alves wrote:
> This "&remote_ops" here will be wrong in case we're connected
> with extended-remote. Looks like remote_read_bytes needs to be
> gain an ops parameter.

Right, add 'ops' argument to remote_read_bytes in the updated patch.
Patch below is pushed in.

--
Yao (é½å°§)

Subject: [PATCH 1/3] Move the traceframe_available_memory code from memory_xfer_partial_1 down to the targets

As a follow-up to

[PATCH 7/8] Adjust read_value_memory to use to_xfer_partial
https://sourceware.org/ml/gdb-patches/2014-02/msg00384.html

this patch moves traceframe_available_memory down to the target side.
After this patch, the gdb core code is cleaner, and code on handling
unavailable memory is moved to remote/tfile/ctf targets.

In details, this patch moves traceframe_available_memory code from
memory_xfer_partial_1 to remote target only, so remote target still
uses traceframe_info mechanism to check unavailable memory, and use
remote_ops to read them from read-only sections. We don't use
traceframe_info mechanism for tfile and ctf target, because it is
fast to iterate all traceframes from trace file, so the summary
information got from traceframe_info is not necessary.

This patch also moves two functions to remote.c from target.c,
because they are only used in remote.c. I'll clean them up in another
patch.

gdb:

2014-03-22 Yao Qi <yao@codesourcery.com>

* ctf.c (ctf_xfer_partial): Check the return value of
exec_read_partial_read_only, if it is not TARGET_XFER_OK,
return TARGET_XFER_UNAVAILABLE.
* tracefile-tfile.c (tfile_xfer_partial): Likewise.
* target.c (target_read_live_memory): Move it to remote.c.
(memory_xfer_live_readonly_partial): Likewise.
(memory_xfer_partial_1): Move some code to remote_read_bytes.
* remote.c (target_read_live_memory): Moved from target.c.
(memory_xfer_live_readonly_partial): Likewise.
(remote_read_bytes): Factored out from
memory_xfer_partial_1.
---
gdb/ctf.c | 16 +++++-
gdb/remote.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++-
gdb/target.c | 138 ----------------------------------------------
gdb/tracefile-tfile.c | 16 +++++-
4 files changed, 171 insertions(+), 143 deletions(-)

diff --git a/gdb/ctf.c b/gdb/ctf.c
index 95fd31f..ebd40d6 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1377,6 +1377,7 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
{
struct bt_iter_pos *pos;
int i = 0;
+ enum target_xfer_status res;

gdb_assert (ctf_iter != NULL);
/* Save the current position. */
@@ -1466,7 +1467,20 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
/* Restore the position. */
bt_iter_set_pos (bt_ctf_get_iter (ctf_iter), pos);

- return exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+ /* Requested memory is unavailable in the context of traceframes,
+ and this address falls within a read-only section, fallback
+ to reading from executable. */
+ res = exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+
+ if (res == TARGET_XFER_OK)
+ return TARGET_XFER_OK;
+ else
+ {
+ /* No use trying further, we know some memory starting
+ at MEMADDR isn't available. */
+ *xfered_len = len;
+ return TARGET_XFER_UNAVAILABLE;
+ }
}
else
{
diff --git a/gdb/remote.c b/gdb/remote.c
index e03d3bf..dcee6e1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6824,6 +6824,87 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
packet_format[0], 1);
}

+/* Read memory from the live target, even if currently inspecting a
+ traceframe. The return is the same as that of target_read. */
+
+static enum target_xfer_status
+target_read_live_memory (enum target_object object,
+ ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
+ ULONGEST *xfered_len)
+{
+ enum target_xfer_status ret;
+ struct cleanup *cleanup;
+
+ /* Switch momentarily out of tfind mode so to access live memory.
+ Note that this must not clear global state, such as the frame
+ cache, which must still remain valid for the previous traceframe.
+ We may be _building_ the frame cache at this point. */
+ cleanup = make_cleanup_restore_traceframe_number ();
+ set_traceframe_number (-1);
+
+ ret = target_xfer_partial (current_target.beneath, object, NULL,
+ myaddr, NULL, memaddr, len, xfered_len);
+
+ do_cleanups (cleanup);
+ return ret;
+}
+
+/* Using the set of read-only target sections of OPS, read live
+ read-only memory. Note that the actual reads start from the
+ top-most target again.
+
+ For interface/parameters/return description see target.h,
+ to_xfer_partial. */
+
+static enum target_xfer_status
+memory_xfer_live_readonly_partial (struct target_ops *ops,
+ enum target_object object,
+ gdb_byte *readbuf, ULONGEST memaddr,
+ ULONGEST len, ULONGEST *xfered_len)
+{
+ struct target_section *secp;
+ struct target_section_table *table;
+
+ secp = target_section_by_addr (ops, memaddr);
+ if (secp != NULL
+ && (bfd_get_section_flags (secp->the_bfd_section->owner,
+ secp->the_bfd_section)
+ & SEC_READONLY))
+ {
+ struct target_section *p;
+ ULONGEST memend = memaddr + len;
+
+ table = target_get_section_table (ops);
+
+ for (p = table->sections; p < table->sections_end; p++)
+ {
+ if (memaddr >= p->addr)
+ {
+ if (memend <= p->endaddr)
+ {
+ /* Entire transfer is within this section. */
+ return target_read_live_memory (object, memaddr,
+ readbuf, len, xfered_len);
+ }
+ else if (memaddr >= p->endaddr)
+ {
+ /* This section ends before the transfer starts. */
+ continue;
+ }
+ else
+ {
+ /* This section overlaps the transfer. Just do half. */
+ len = p->endaddr - memaddr;
+ return target_read_live_memory (object, memaddr,
+ readbuf, len, xfered_len);
+ }
+ }
+ }
+ }
+
+ return TARGET_XFER_EOF;
+}
+
/* Read memory data directly from the remote machine.
This does not use the data cache; the data cache uses this.
MEMADDR is the address in the remote memory space.
@@ -6835,8 +6916,8 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
transferred in *XFERED_LEN. */

static enum target_xfer_status
-remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
- ULONGEST *xfered_len)
+remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
+ gdb_byte *myaddr, ULONGEST len, ULONGEST *xfered_len)
{
struct remote_state *rs = get_remote_state ();
int max_buf_size; /* Max size of packet output buffer. */
@@ -6847,6 +6928,63 @@ remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
if (len == 0)
return 0;

+ if (get_traceframe_number () != -1)
+ {
+ VEC(mem_range_s) *available;
+
+ /* If we fail to get the set of available memory, then the
+ target does not support querying traceframe info, and so we
+ attempt reading from the traceframe anyway (assuming the
+ target implements the old QTro packet then). */
+ if (traceframe_available_memory (&available, memaddr, len))
+ {
+ struct cleanup *old_chain;
+
+ old_chain = make_cleanup (VEC_cleanup(mem_range_s), &available);
+
+ if (VEC_empty (mem_range_s, available)
+ || VEC_index (mem_range_s, available, 0)->start != memaddr)
+ {
+ enum target_xfer_status res;
+
+ /* Don't read into the traceframe's available
+ memory. */
+ if (!VEC_empty (mem_range_s, available))
+ {
+ LONGEST oldlen = len;
+
+ len = VEC_index (mem_range_s, available, 0)->start - memaddr;
+ gdb_assert (len <= oldlen);
+ }
+
+ do_cleanups (old_chain);
+
+ /* This goes through the topmost target again. */
+ res = memory_xfer_live_readonly_partial (ops,
+ TARGET_OBJECT_MEMORY,
+ myaddr, memaddr,
+ len, xfered_len);
+ if (res == TARGET_XFER_OK)
+ return TARGET_XFER_OK;
+ else
+ {
+ /* No use trying further, we know some memory starting
+ at MEMADDR isn't available. */
+ *xfered_len = len;
+ return TARGET_XFER_UNAVAILABLE;
+ }
+ }
+
+ /* Don't try to read more than how much is available, in
+ case the target implements the deprecated QTro packet to
+ cater for older GDBs (the target's knowledge of read-only
+ sections may be outdated by now). */
+ len = VEC_index (mem_range_s, available, 0)->length;
+
+ do_cleanups (old_chain);
+ }
+ }
+
max_buf_size = get_memory_read_packet_size ();
/* The packet buffer will be large enough for the payload;
get_memory_packet_size ensures this. */
@@ -8698,7 +8836,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
if (writebuf != NULL)
return remote_write_bytes (offset, writebuf, len, xfered_len);
else
- return remote_read_bytes (offset, readbuf, len, xfered_len);
+ return remote_read_bytes (ops, offset, readbuf, len, xfered_len);
}

/* Handle SPU memory using qxfer packets. */
diff --git a/gdb/target.c b/gdb/target.c
index 0d22297..1b48f79 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -940,87 +940,6 @@ target_section_by_addr (struct target_ops *target, CORE_ADDR addr)
return NULL;
}

-/* Read memory from the live target, even if currently inspecting a
- traceframe. The return is the same as that of target_read. */
-
-static enum target_xfer_status
-target_read_live_memory (enum target_object object,
- ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
- ULONGEST *xfered_len)
-{
- enum target_xfer_status ret;
- struct cleanup *cleanup;
-
- /* Switch momentarily out of tfind mode so to access live memory.
- Note that this must not clear global state, such as the frame
- cache, which must still remain valid for the previous traceframe.
- We may be _building_ the frame cache at this point. */
- cleanup = make_cleanup_restore_traceframe_number ();
- set_traceframe_number (-1);
-
- ret = target_xfer_partial (current_target.beneath, object, NULL,
- myaddr, NULL, memaddr, len, xfered_len);
-
- do_cleanups (cleanup);
- return ret;
-}
-
-/* Using the set of read-only target sections of OPS, read live
- read-only memory. Note that the actual reads start from the
- top-most target again.
-
- For interface/parameters/return description see target.h,
- to_xfer_partial. */
-
-static enum target_xfer_status
-memory_xfer_live_readonly_partial (struct target_ops *ops,
- enum target_object object,
- gdb_byte *readbuf, ULONGEST memaddr,
- ULONGEST len, ULONGEST *xfered_len)
-{
- struct target_section *secp;
- struct target_section_table *table;
-
- secp = target_section_by_addr (ops, memaddr);
- if (secp != NULL
- && (bfd_get_section_flags (secp->the_bfd_section->owner,
- secp->the_bfd_section)
- & SEC_READONLY))
- {
- struct target_section *p;
- ULONGEST memend = memaddr + len;
-
- table = target_get_section_table (ops);
-
- for (p = table->sections; p < table->sections_end; p++)
- {
- if (memaddr >= p->addr)
- {
- if (memend <= p->endaddr)
- {
- /* Entire transfer is within this section. */
- return target_read_live_memory (object, memaddr,
- readbuf, len, xfered_len);
- }
- else if (memaddr >= p->endaddr)
- {
- /* This section ends before the transfer starts. */
- continue;
- }
- else
- {
- /* This section overlaps the transfer. Just do half. */
- len = p->endaddr - memaddr;
- return target_read_live_memory (object, memaddr,
- readbuf, len, xfered_len);
- }
- }
- }
- }
-
- return TARGET_XFER_EOF;
-}
-
/* Read memory from more than one valid target. A core file, for
instance, could have some of memory but delegate other bits to
the target below it. So, we must manually try all targets. */
@@ -1128,63 +1047,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
}
}

- /* If reading unavailable memory in the context of traceframes, and
- this address falls within a read-only section, fallback to
- reading from live memory. */
- if (readbuf != NULL && get_traceframe_number () != -1)
- {
- VEC(mem_range_s) *available;
-
- /* If we fail to get the set of available memory, then the
- target does not support querying traceframe info, and so we
- attempt reading from the traceframe anyway (assuming the
- target implements the old QTro packet then). */
- if (traceframe_available_memory (&available, memaddr, len))
- {
- struct cleanup *old_chain;
-
- old_chain = make_cleanup (VEC_cleanup(mem_range_s), &available);
-
- if (VEC_empty (mem_range_s, available)
- || VEC_index (mem_range_s, available, 0)->start != memaddr)
- {
- /* Don't read into the traceframe's available
- memory. */
- if (!VEC_empty (mem_range_s, available))
- {
- LONGEST oldlen = len;
-
- len = VEC_index (mem_range_s, available, 0)->start - memaddr;
- gdb_assert (len <= oldlen);
- }
-
- do_cleanups (old_chain);
-
- /* This goes through the topmost target again. */
- res = memory_xfer_live_readonly_partial (ops, object,
- readbuf, memaddr,
- len, xfered_len);
- if (res == TARGET_XFER_OK)
- return TARGET_XFER_OK;
- else
- {
- /* No use trying further, we know some memory starting
- at MEMADDR isn't available. */
- *xfered_len = len;
- return TARGET_XFER_UNAVAILABLE;
- }
- }
-
- /* Don't try to read more than how much is available, in
- case the target implements the deprecated QTro packet to
- cater for older GDBs (the target's knowledge of read-only
- sections may be outdated by now). */
- len = VEC_index (mem_range_s, available, 0)->length;
-
- do_cleanups (old_chain);
- }
- }
-
/* Try GDB's internal data cache. */
region = lookup_mem_region (memaddr);
/* region->hi == 0 means there's no upper bound. */
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index cbf746d..a36596f 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -900,6 +900,7 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
if (get_traceframe_number () != -1)
{
int pos = 0;
+ enum target_xfer_status res;

/* Iterate through the traceframe's blocks, looking for
memory. */
@@ -937,7 +938,20 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
pos += (8 + 2 + mlen);
}

- return exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+ /* Requested memory is unavailable in the context of traceframes,
+ and this address falls within a read-only section, fallback
+ to reading from executable. */
+ res = exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+
+ if (res == TARGET_XFER_OK)
+ return TARGET_XFER_OK;
+ else
+ {
+ /* No use trying further, we know some memory starting
+ at MEMADDR isn't available. */
+ *xfered_len = len;
+ return TARGET_XFER_UNAVAILABLE;
+ }
}
else
{
--
1.7.7.6