Re: [PATCH v5 20/25] New memory-tag commands

Simon Marchi via Gdb-patches
2021-03-23T21:46:43Z

On 2021-02-08 1:59 p.m., Luis Machado wrote:> On 2/5/21 1:49 AM, Simon Marchi wrote:
>>> +/* Parse ARGS and extract ADDR and TAG.
>>> + ARGS should have format <expression> <tag bytes>. */
>>> +
>>> +static void
>>> +parse_with_logical_tag_input (const char *args, struct value **val,
>>> + gdb::byte_vector &tags,
>>> + value_print_options *print_opts)
>>> +{
>>> + /* Fetch the address. */
>>> + std::string address_string = extract_string_maybe_quoted (&args);
>>> +
>>> + /* Parse the address into a value. */
>>> + *val = process_print_command_args (address_string.c_str (), print_opts,
>>> + true);
>>> +
>>> + /* Fetch the tag bytes. */
>>> + std::string tag_string = extract_string_maybe_quoted (&args);
>>> +
>>> + /* Validate the input. */
>>> + if (address_string.empty () || tag_string.empty ())
>>> + error (_("Missing arguments."));
>>> +
>>> + if (tag_string.length () != 2)
>>> + error (_("Error parsing tags argument. The tag should be 2 digits."));
>>
>> Can't an hypothetical architecture use more than one byte for a tag?
>>
>
> It could. But it is unlikely, given architectures use spare bits of the virtual address.
>
> We could solve this by having another gdbarch constant that tells us the size of a tag in a particular architecture. Then we check for size * 2 characters. Or check for more than a couple characters, and let the architecture device if the tag value is sane or not.
>
> What do you think?
>
>>> void _initialize_printcmd ();
>>> void
>>> _initialize_printcmd ()
>>> @@ -2982,4 +3263,63 @@ Construct a GDB command and then evaluate it.\n\
>>> Usage: eval \"format string\", ARG1, ARG2, ARG3, ..., ARGN\n\
>>> Convert the arguments to a string as \"printf\" would, but then\n\
>>> treat this string as a command line, and evaluate it."));
>>> +
>>> + /* Memory tagging commands. */
>>> + add_prefix_cmd ("memory-tag", class_vars, memory_tag_command, _("\
>>> +Generic command for printing and manipulating memory tag properties."),
>>> + &memory_tag_list, "memory-tag ", 0, &cmdlist);
>>> + add_cmd ("print-logical-tag", class_vars,
>>> + memory_tag_print_logical_tag_command,
>>> + ("Print the logical tag for an address.\n\
>>> +Usage: memory-tag print-logical-tag .\n\
>>> + is an expression that evaluates to a pointer or memory address.\n\
>>> +GDB will print the logical tag associated with . The tag\n\
>>
>> Instead of saying "GDB will print the...", use the infinite "Print the
>> ...". I would also swap the two sentences to say what the command says
>> first, before describing the argument. Something like:
>
> I don't have a preference, but the 'x' command, for example, does the opposite. I used it as reference.
>
> --
>
> c = add_com ("x", class_vars, x_command, _("\
> Examine memory: x/FMT ADDRESS.\n\
> ADDRESS is an expression for the memory address to examine.\n\
> FMT is a repeat count followed by a format letter and a size letter.\n\
> Format letters are o(octal), x(hex), d(decimal), u(unsigned decimal),\n\
> t(binary), f(float), a(address), i(instruction), c(char), s(string)\n\
> and z(hex, zero padded on the left).\n\
> Size letters are b(byte), h(halfword), w(word), g(giant, 8 bytes).\n\
> The specified number of objects of the specified size are printed\n\
> according to the format. If a negative number is specified, memory is\n\
> examined backward from the address.\n\n\
> Defaults for format and size letters are those previously used.\n\
> Default count is 1. Default address is following last thing printed\n\
> with this command or \"print\"."));
>
> --
>
> Besides...
>
>>
>> Print the logical tag associated with a pointer. POINTER is an
>> expression that evaluates to a pointer.
>
> ...this is almost exactly the first line of the command documentation.
>
> Looking at 'x', it feels to me that the second portion of the documentation describes what GDB will acomplish in a more verbose manner, since the command's output is already described in the first line.

Indeed, I realized later that the very first line was already the
one-sentence description of what the command does (see my own reply to
this message).

> Is there a standard for these command documentation bits somewhere?

Not sure it's spelled out anywhere. But IMO the first sentence should
always be a very high level explanation of what the command does, so you
can get a quick idea if this is what you're looking for without having
to read a whole paragraph. Then I like when things (like description of
each argument) are nicely separated in paragraphs instead of one big
paragraph, so it's easy to scan and jump to what you are looking for.

>> understand it is that logical tags are encoded into pointers, whereas
>> allocation tags are associated to memory addresses. So I think it would
>> make more sense to use "pointer" for things related to logical tags and
>> "address" for things related to allocation tags.
>
> That's the correct understanding. Though in GDB, sometimes things get a bit fuzzy, and I may have exposed some of GDB's inner interpretation here.
>
> For example, when you pass in a constant to a command. Is that a pointer or an address? Maybe an address that gets converted to a typed pointer?
>
> For simplicity, I think we can go for pointers when talking about memory tags and memory when talking about allocation tags.

I suppose you meant "... and address when talking about allocation
tags".

What you have in v6 LGTM.

Simon