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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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?