Re: Better MI memory commands

Daniel Jacobowitz
2010-07-07T16:30:07Z

I have no further comments on the documentation, but Eli had several.

On Fri, Jun 25, 2010 at 12:32:55PM +0400, Vladimir Prus wrote:
> - The current output of -data-read-memory is totally insane. This patch
> introduces -data-read-memory-raw that clearly reports what regions were
> read, and shows the data as hex string. I've also added -data-write-memory-raw
> which, contrary to -data-write-memory can actually write *memory*, as opposed to
> a single byte.

I think that "raw" is not very expressive - it is too much like "new"
in that you don't know later what is newer than what. The main change
to -data-read-memory is to return a string of byte data, instead of
the word-oriented table. So how about -data-read-bytes?

> + if (argc != 2)
> + error ("Usage: ADDR LENGTH.");

Missing [-o BYTE-OFFSET] in the message.

> + /* Loop invariant is that the [current_begin, current_end) was previously
> + found to be not readable as a whole.
> +
> + Note loop condition -- if the range has 1 byte, we can't divide the range
> + so there's no point trying further. */
> + while (current_end - current_begin > 1)
> + {
> + ULONGEST first_half_begin, first_half_end;
> + ULONGEST second_half_begin, second_half_end;
> + LONGEST xfer;
> +
> + ULONGEST middle = current_begin + (current_end - current_begin)/2;
> + if (forward)
> + {
> + first_half_begin = current_begin;
> + first_half_end = middle;
> + second_half_begin = middle;
> + second_half_end = current_end;
> + }
> + else
> + {
> + first_half_begin = middle;
> + first_half_end = current_end;
> + second_half_begin = current_begin;
> + second_half_end = middle;
> + }
> +
> + xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
> + buf + (first_half_begin - begin),
> + first_half_begin,
> + first_half_end - first_half_begin);
> +
> + if (xfer == first_half_end - first_half_begin)
> + {
> + /* This half reads up fine. So, the error must be in the other half. */
> + current_begin = second_half_begin;
> + current_end = second_half_end;
> + }
> + else
> + {
> + current_begin = first_half_begin;
> + current_end = first_half_end;
> + }
> + }

If we take the first branch of this if statement, then we've read the
"fine" half of the search space into BUF. We search the "bad" half
for the error. If we take the second branch of the if statement, then
we narrow the search space to the first half. But we didn't read the
second half. Won't we return uninitialized data in the buffer?

I think I see now. The assumption is a single valid/invalid
transition in the entire range. So we assume that in the else block,
the whole second half won't be readable. Right? If so, this is OK,
but I'd appreciate a comment to that effect.

> - if (xfer == 0)
> - return xfered;
> - if (xfer < 0)
> - {
> - remaining = half;
> - }
> - else
> - {
> - /* We have successfully read the first half. So, the
> - error must be in the second half. Adjust start and
> - remaining to point at the second half. */
> - xfered += xfer;
> - start += xfer;
> - remaining -= xfer;
> - }
> - half = remaining/2;
> + xfree (buffer);
> + xfered += to_read;
> }

Why do we skip to_read bytes if we succeed at reading zero bytes?
For that matter, what does a return value of zero mean? It seems like
this would mean the same as -1.

> -
> - return xfered;
> + else if (xfer < 0)
> + {
> + /* Got an error reading full chunk. See if maybe we can read
> + some subrange. */
> + xfree (buffer);
> + read_whatever_is_readable (ops, offset + xfered, offset + xfered + to_read, &result);
> + xfered += to_read;
> + }
> + else
> + {
> + struct memory_read_result r;
> + r.data = buffer;
> + r.begin = offset + xfered;
> + r.end = r.begin + xfer;
> + VEC_safe_push (memory_read_result_s, result, &r);
> + xfered += xfer;
> + }

A single consecutive block of memory may appear as more than one
memory_read_result (and thus more than one MI record). Is this what
we want? If so, please mention it in the documentation.

> diff --git a/gdb/target.h b/gdb/target.h
> index 870a1eb..66e1166 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h

> +extern VEC(memory_read_result_s)*
> +read_memory_robust (struct target_ops *ops, ULONGEST offset, LONGEST len);

Formatting: function name doesn't go in column 1 for the prototype.

--
Daniel Jacobowitz