Re: [PATCH v3 02/24] New gdbarch memory tagging hooks

Luis Machado via Gdb-patches
2020-12-28T15:44:22Z

On 12/25/20 1:40 AM, Simon Marchi wrote:
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
>> We need some new gdbarch hooks to help us manipulate memory tags without having
>> to have GDB calls the target methods directly.
>
> calls -> call?
>

Fixed.

>>
>> This patch adds the following hooks:
>>
>> gdbarch_memtag_to_string
>> --
>> Returns a printable string corresponding to the tag.
>>
>> gdbarch_tagged_address_p
>> --
>> Checks if a particular address is protected with memory tagging.
>>
>> gdbarch_memtag_mismatch_p
>> --
>> Checks if there is a mismatch between the logical tag of a pointer and the
>> allocation tag.
>
> It would seem more natural to me to have gdbarch_memtag_matches_p
> instead,which would just be the opposite gdbarch_memtag_mismatch_p.
> If I want to know if a tag matches, I have to do:
>
> if (!gdbarch_memtag_mismatch_p (...))
>
> and that's like a double negative, "if the memtag does not not match".
>

Mismatches are what we're looking for, so I named it based on that. But
I see your point. I've renamed it to gdbarch_memtag_matches_p

>> gdbarch_set_memtags:
>> --
>> Sets either the allocation tag or the logical tag for a particular value.
>>
>> gdbarch_get_memtag:
>> --
>> Gets either the allocation tag or the logical tag for a particular value.
>>
>> gdbarch_granule_size
>> --
>> Sets the memory tag granule size, which represents the number of bytes a
>> particular allocation tag covers. For example, this is 16 bytes for
>> AArch64's MTE.
>
> I'd suggest having "memtag" in the name of this function, to make it clear
> it's memtag-related. Like "gdbarch_memtag_granule_size".

I think I only messed up the text, because the code is already as suggested.

>
>>
>> I've used struct value as opposed to straight CORE_ADDR so other architectures
>> can use the infrastructure without having to rely on fixed types.
>
> What do you mean by "having to rely on fixed types"?
>

Instead of assuming the tagged address will fit in a CORE_ADDR, for
example, we use a "struct value *" to cope with architectures that have
addresses that are larger than a CORE_ADDR or that have completely
different types.

CHERI/Morello is one such example. The pointers will be capability types
of 16 bytes, with a tag stored as metadata. So this patch makes it
possible to support those architectures in the future.

Using "struct value *" also prevents generic code from having to know
what specific types/sizes we're dealing with.

Does that make it clear?

> Otherwise, this LGTM.
>
> Simon
>

Thanks for the review.