Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Thiago, Luis

On 4/3/24 11:04 AM, Luis Machado wrote:
> On 3/30/24 03:08, Thiago Jung Bauermann wrote:
>>
>> Just one more suggestion that occurred to me later:
>>
>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>
>>> For supporting the new packet, a new target hook is introduced,
>>> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
>>> gdbarch hook in the upper layers (printcmd.c).
>>
>> "check_memtag_addr" is a bit vague: what does it check? This confused me
>> a bit when I was reading the code with these patches
>> applied. Alternatives I can think of are "is_addr_tagged", or
>> "tagged_address_p".
> 
> That last bit seems more in line with the gdb terminology. A target took "tagged_address_p" or "target_is_address_tagged" should be a bit more clear.
> 
> Plus we have precedent in the target hooks is_async_p, can_async_p, always_non_stop_p etc.
> 
>>
>> I'm not too fond of the "_p" suffix, but it has the advantage of being
>> consistent with the existing gdbarch hook so it may be preferable in
>> this case.

So, I really thought of something like the is_address_tagged at first,
without the _p suffix because I thought this suffix would be present
only in gdbarch hooks, not in the target hooks, but that seems not right,
which is still a bit confusing to me tbh, because I have this in mind:

   /* MTE-specific settings and hooks.  */
   if (tdep->has_mte ())
     {
       /* Register a hook for checking if an address is tagged or not.  */
       set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);

which I understand that functions with _p would be the functions to be
set as the hooks for the gdbarch hooks only, using the set_gdbarch_*
"API" functions. That, afaics, doesn't happen for the target hooks, like
is_async_p, which is never explicitly set to any hook using a set_* function
(so I'm also not fond of the _p in the is_async_p case too).
  
Anyways, I'm going for "target_is_address_tagged", which I think is the
clearest option and we all seem to agree on it :) Thanks!


Cheers,
Gustavo