Re: [PATCH v2 07/24] Documentation for memory tagging remote packets

Luis Machado via Gdb-patches
2020-10-23T14:07:56Z

Hi Eli,

On 10/23/20 3:25 AM, Eli Zaretskii wrote:
>> Date: Thu, 22 Oct 2020 16:59:57 -0300
>> From: Luis Machado via Gdb-patches <gdb-patches@sourceware.org>
>> Cc: david.spickett@linaro.org
>>
>> gdb/doc/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * gdb.texinfo (General Query Packets): Document qMemTags and
>> QMemTags.
>> Document the "memory-tagging" feature.
>
> Since the last sentence belongs to the same node as the one before it,
> please don't start it on a new line, but after the previous sentence
> ends (with 2 spaces between them).

Thanks for clarifying that. Fixed now.

>
>> +@item qMemTags:@var{start address},@var{length}:@var{type}
>> +@cindex fetch memory tags
>> +@cindex @samp{qMemTags} packet
>> +Fetch memory tags of type @var{type} from the address range
>> +@r{[}@var{start address}, @var{start address} + @var{length}@r{)}. The target
>
> The expression in @r{...} should be wrapped with @w{..}, so that it
> doesn't get split between two lines.
>

Fixed.

>> +@var{type} is the type of tag, a signed integer, the request wants to fetch.
>
> This is ambiguous: does "the request wants to fetch" refer to the tag
> or to the "signed integer" part? Suggest to move the "a signed
> integer" part to the end of the sentence.

Yeah. It reads a bit funny. How about the following?

@var{type} is the type of tag the request wants to fetch. The type is a
signed integer.

>
>> +tags found in the request memory range.
> ^^^^^^^
> "requested"
>

Oops. Fixed.

>> +Store memory tags of type @var{type} to the address range
>> +@r{[}@var{start address}, @var{start address} + @var{length}@r{)}. The target
>
> Same comment here about @r{..}.
>

Fixed.

>> +If the number of memory tags, @var{nt}, is greater than or equal to the
>> +number of memory tag granules, @var{ng}, only @var{ng} tags will be
>> +stored.
>
> It is not clear here how NT and NG are related to the parameters of
> the packet. Can you add something that explains the relation?
>

How about the following?

NT is the number of memory tags contained in @var{tag bytes}. Only
target-specific code can determine this value. For example, AArch64's
tags are stored 1 per byte.

NG is the number of memory tag granules in the memory range
@w{@r{[}@var{start address}, @var{start address} + @var{length}@r{)}}.
Only target-specific can determine this value. For example, AArch64 has
a tag granule size of 16 bytes. That is, it has one memory for every 16
bytes of memory.

>> +@var{type} is the type of tag, a signed integer, the request wants to store.
>
> Same comment as above regarding ambiguity of the sentence.
>

Fixed in the same way as above.

>> +@item memory-tagging
>> +The remote stub supports and implements the required memory tagging
>> +functionality and understands the @samp{qMemTags} and @samp{QMemTags} packets.
>
> This is far enough from the description of these packets to warrant a
> cross-reference to that description. Please add a cross-reference.
>
> Thanks.
>

I did...

"The remote stub supports and implements the required memory tagging
functionality and understands the @samp{qMemTags} (@pxref{qMemTags}) and
@samp{QMemTags} (@pxref{QMemTags}) packets."

And I've added a couple anchors to the packets.

Is that what you had in mind?