Re: Better MI memory commands

Eli Zaretskii
2010-06-25T16:05:10Z

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Fri, 25 Jun 2010 12:32:55 +0400
>
> The attached patch makes a few changes in MI memory commands, with the
> goal of making it easy for frontend to just display memory view, even
> around the boundaries of accessible memory.

Thanks. I have a few comments about the documentation part of the
patch.

> +@item @var{count}
> +The number of bytes to read. This should be an integer literal.
^^
Two spaces here, please.

> +The commands attempts to read memory at the specified address.
^^^^^^^^^^^^
"This command"

> + When a
> +memory map is available

A cross-reference here to where memory maps are described would be
useful.

> + regions marked as unreadable in the memory
> +map will be skipped. In addition, when @value{GDBN} encouters an error
^^
Two spaces.

> +reading a memory range, it will attempt to find readable subrange at
^^^^^^^^^^^^^^^^^^
"a readable subrange"

> +the beginning and range. Essentially, this command will make
^^ ^^
Something ("end of the"?) is missing here. And two spaces between
sentences.

I'm not sure I completely understand this part:

In addition, when @value{GDBN} encouters an error reading a memory
range, it will attempt to find readable subrange [...]

What if there are two or more non-contiguous sub-ranges at the
beginning -- will GDB find and read both of them? More generally,
what happens if the specified range has several disjoint portions that
are not readable?

I'm asking because your description seems to imply that we only look
for the first readable address after start and the last readable
address before the end of the specified region. IOW, it sounds like
unreadable sub-regions in the middle of the region are not supported.

If my understanding is correct, then the following sentence

> + Essentially, this command will make
> +reasonable effort to return all readable memory content within the
> +requested range.

is at least inaccurate, if not misleading ("all readable memory within
the range").

> +The output of the command has a result record named @samp{memory},
^^^^^^^^^^^^^^^^^^^
Perhaps "is a result record"? And what is the importance of naming
this record `memory'? why is the name important here?

> +@item contents
> +The contents of the memory block, as hex-encoded string of bytes.

But your example shows this:

> + contents="01000000020000000300"@}]

This doesn't look like a ``string of bytes'' to me. Or maybe I don't
understand what you meant by that?

> +@item @var{contents}
> +The hex-encoded bytes to write. The size of this parameter determines
> +how many bytes should be written.^^^^^^^^

You probably meant "the value", not "the size".

> + Otherwise, if there's a readable subrange at the end, it will be
> + completely and returned. Any readable subranges before it (obviously,
^^
"read" is missing here.

> + The purpose of this function is to handle a read across a boundary of
> + accessible memory in a case when memory map is not available. The above
> + restrictions are fine for this case, but will give incorrect results if
> + the memory is 'patchy'. However, supporting 'patchy' memory would require
> + trying to read every single byte, and it seems unacceptable solution.
> + Explicit memory map is recommended for this case -- and
> + target_read_memory_robust will take care of reading multiple ranges then.

At least some of this comment should IMO be in the manual.

Thanks.