This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 7/8] Adjust read_value_memory to use to_xfer_partial
- From: Yao Qi <yao at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Sun, 23 Feb 2014 11:52:17 +0800
- Subject: Re: [PATCH 7/8] Adjust read_value_memory to use to_xfer_partial
- Authentication-results: sourceware.org; auth=none
- References: <1392185152-21220-1-git-send-email-yao at codesourcery dot com> <1392185152-21220-8-git-send-email-yao at codesourcery dot com> <53060103 dot 8060301 at redhat dot com>
On 02/20/2014 09:20 PM, Pedro Alves wrote:
> I think we'll want to move the traceframe_available_memory
> code from memory_xfer_partial_1 down to the targets too
> (at some point).
>
Yes, that'll be a step forward after this patch,
[unavailable values part 1, 05/17] move traceframe memory reading fallback to read-only sections to GDB side
https://sourceware.org/ml/gdb-patches/2011-02/msg00136.html
and we'll move "traceframe memory reading fallback to read-only
sections" from GDB core to GDB target part, such as tfile, ctf and
remote.
AFAICS, targets tfile and ctf nowadays have such fallback logic,
but target remote doesn't. We need to add such logic in remote
target. I'll look into it.
>> > - if (unavail != memaddr + length)
>> > - mark_value_bytes_unavailable (val,
>> > - embedded_offset + unavail - memaddr,
>> > - (memaddr + length) - unavail);
>> > + if (status == TARGET_XFER_EOF)
>> > + memory_error (TARGET_XFER_E_IO, memaddr + xfered);
>> > + else if (status == TARGET_XFER_E_UNAVAILABLE)
>> > + mark_value_bytes_unavailable (val, embedded_offset + xfered,
>> > + xfered_len);
>> > + else if (TARGET_XFER_STATUS_ERROR_P (status))
>> > + memory_error (status, memaddr + xfered);
> I maintain that using TARGET_XFER_STATUS_ERROR_P tends to
> be unnecessary, and actually leads to fragile code. It doesn't
> really help here. A newer status added in the future that is
> not an error will pass by here unhandled. Handle TARGET_XFER_OK
> explicitly instead, like this:
>
> if (status == TARGET_XFER_EOF)
^^^^^^^^^^^^^^^
It should be TARGET_XFER_OK
> /* nothing */;
> else if (status == TARGET_XFER_E_UNAVAILABLE)
> mark_value_bytes_unavailable (val, embedded_offset + xfered,
> xfered_len);
> else if (status == TARGET_XFER_EOF)
> memory_error (TARGET_XFER_E_IO, memaddr + xfered);
> else
> memory_error (status, memaddr + xfered);
>
> xfered += xfered_len;
> QUIT;
> }
>
> and now all future unhandled statuses fall through to
> memory_error.
OK, update patch as you suggested. I'll get rid of
TARGET_XFER_STATUS_ERROR_P in a follow-up patch.
>
> (Hmm, the _E_ in TARGET_XFER_E_UNAVAILABLE is now starting
> to look a little odd and confusing to me. E.g., it's not
> really an error in this case. I'm pondering renaming it to
> TARGET_XFER_UNAVAILABLE.)
It isn't an error. Will rename it in a follow-up patch too.
Patch below is pushed in. Note that I move hunk on
making section_table_available_memory static from patch #6
to patch #7, to fix a compilation error when patches #1~#6
are applied.
--
Yao (éå)
gdb:
2014-02-23 Yao Qi <yao@codesourcery.com>
* valops.c (read_value_memory): Rewrite it. Call
target_xfer_partial in a loop.
* exec.h (section_table_available_memory): Remove declaration.
Move comments to ...
* exec.c (section_table_available_memory): ... here. Make it static.
---
gdb/ChangeLog | 9 +++++
gdb/exec.c | 7 +++-
gdb/exec.h | 11 ------
gdb/valops.c | 96 +++++++++++++-------------------------------------------
4 files changed, 38 insertions(+), 85 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 55745af..326909f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
2014-02-23 Yao Qi <yao@codesourcery.com>
+ * valops.c (read_value_memory): Rewrite it. Call
+ target_xfer_partial in a loop.
+ * exec.h (section_table_available_memory): Remove declaration.
+ Move comments to ...
+ * exec.c (section_table_available_memory): ... here. Make it
+ static.
+
+2014-02-23 Yao Qi <yao@codesourcery.com>
+
* exec.c (section_table_read_available_memory): New function.
* exec.h (section_table_read_available_memory): Declare.
* ctf.c (ctf_xfer_partial): Call
diff --git a/gdb/exec.c b/gdb/exec.c
index 607f5ac..758e382 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -577,7 +577,12 @@ exec_read_partial_read_only (gdb_byte *readbuf, ULONGEST offset,
return TARGET_XFER_E_IO;
}
-VEC(mem_range_s) *
+/* Appends all read-only memory ranges found in the target section
+ table defined by SECTIONS and SECTIONS_END, starting at (and
+ intersected with) MEMADDR for LEN bytes. Returns the augmented
+ VEC. */
+
+static VEC(mem_range_s) *
section_table_available_memory (VEC(mem_range_s) *memory,
CORE_ADDR memaddr, ULONGEST len,
struct target_section *sections,
diff --git a/gdb/exec.h b/gdb/exec.h
index 84dc40f..4d9de90 100644
--- a/gdb/exec.h
+++ b/gdb/exec.h
@@ -55,17 +55,6 @@ extern enum target_xfer_status
exec_read_partial_read_only (gdb_byte *readbuf, ULONGEST offset,
ULONGEST len, ULONGEST *xfered_len);
-/* Appends all read-only memory ranges found in the target section
- table defined by SECTIONS and SECTIONS_END, starting at (and
- intersected with) MEMADDR for LEN bytes. Returns the augmented
- VEC. */
-
-extern VEC(mem_range_s) *
- section_table_available_memory (VEC(mem_range_s) *ranges,
- CORE_ADDR memaddr, ULONGEST len,
- struct target_section *sections,
- struct target_section *sections_end);
-
/* Read or write from mappable sections of BFD executable files.
Request to transfer up to LEN 8-bit bytes of the target sections
diff --git a/gdb/valops.c b/gdb/valops.c
index 898401d..0d726d0 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -949,81 +949,31 @@ read_value_memory (struct value *val, int embedded_offset,
int stack, CORE_ADDR memaddr,
gdb_byte *buffer, size_t length)
{
- if (length)
- {
- VEC(mem_range_s) *available_memory;
-
- if (!traceframe_available_memory (&available_memory, memaddr, length))
- {
- if (stack)
- read_stack (memaddr, buffer, length);
- else
- read_memory (memaddr, buffer, length);
- }
+ ULONGEST xfered = 0;
+
+ while (xfered < length)
+ {
+ enum target_xfer_status status;
+ ULONGEST xfered_len;
+
+ status = target_xfer_partial (current_target.beneath,
+ TARGET_OBJECT_MEMORY, NULL,
+ buffer + xfered, NULL,
+ memaddr + xfered, length - xfered,
+ &xfered_len);
+
+ if (status == TARGET_XFER_OK)
+ /* nothing */;
+ else if (status == TARGET_XFER_E_UNAVAILABLE)
+ mark_value_bytes_unavailable (val, embedded_offset + xfered,
+ xfered_len);
+ else if (status == TARGET_XFER_EOF)
+ memory_error (TARGET_XFER_E_IO, memaddr + xfered);
else
- {
- struct target_section_table *table;
- struct cleanup *old_chain;
- CORE_ADDR unavail;
- mem_range_s *r;
- int i;
-
- /* Fallback to reading from read-only sections. */
- table = target_get_section_table (&exec_ops);
- available_memory =
- section_table_available_memory (available_memory,
- memaddr, length,
- table->sections,
- table->sections_end);
-
- old_chain = make_cleanup (VEC_cleanup(mem_range_s),
- &available_memory);
-
- normalize_mem_ranges (available_memory);
+ memory_error (status, memaddr + xfered);
- /* Mark which bytes are unavailable, and read those which
- are available. */
-
- unavail = memaddr;
-
- for (i = 0;
- VEC_iterate (mem_range_s, available_memory, i, r);
- i++)
- {
- if (mem_ranges_overlap (r->start, r->length,
- memaddr, length))
- {
- CORE_ADDR lo1, hi1, lo2, hi2;
- CORE_ADDR start, end;
-
- /* Get the intersection window. */
- lo1 = memaddr;
- hi1 = memaddr + length;
- lo2 = r->start;
- hi2 = r->start + r->length;
- start = max (lo1, lo2);
- end = min (hi1, hi2);
-
- gdb_assert (end - memaddr <= length);
-
- if (start > unavail)
- mark_value_bytes_unavailable (val,
- (embedded_offset
- + unavail - memaddr),
- start - unavail);
- unavail = end;
-
- read_memory (start, buffer + start - memaddr, end - start);
- }
- }
-
- if (unavail != memaddr + length)
- mark_value_bytes_unavailable (val,
- embedded_offset + unavail - memaddr,
- (memaddr + length) - unavail);
-
- do_cleanups (old_chain);
- }
+ xfered += xfered_len;
+ QUIT;
}
}
--
1.7.7.6