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

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

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.