Re: [PATCH,v2] [aarch64] Fix removal of non-address bits for PAuth

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Luis Machado <luis.machado@arm.com> writes:

> On 8/19/22 00:47, Thiago Jung Bauermann wrote:
>> Hello Luis,
>> The patch looks great to me, FWIW.
>> I have just a few small comments.
>> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> While at it, make GDB and GDBServer share some more code for aarch64
>> Nice!
>> 
>>> +/* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>>> +   non address bits from a pointer value.  */
>>> +
>>> +static CORE_ADDR
>>> +aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
>>> +{
>>> +  aarch64_gdbarch_tdep *tdep
>>> +    = (aarch64_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>>> +
>>> +  /* By default, we assume TBI and discard the top 8 bits plus the VA range
>>> +     select bit (55).  */
>>> +  CORE_ADDR mask = (((CORE_ADDR) 1) << (64 - 9)) - 1;
>>> +  mask = ~mask;
>> One suggestion is to put the above in gdb/arch/aarch64.h as:
>> /* By default, we assume TBI and discard the top 8 bits plus the VA range
>>     select bit (55).  */
>> #define AARCH64_TOP_BITS_MASK (~((((CORE_ADDR) 1) << (64 - 9)) - 1))
>> and then use it here and in gdbserver's version of this function so that
>> there's a little bit more code shared between them.
>> 
>
> That makes sense, but I wonder if it will fit gdb/arch/aarch64.h. The VA bit is
> a generic ISA thing, but the TBI is OS-specific I think.

Right, an OS may or may not support TBI.

> Maybe a gdb/arch/aarch64-pauth.h header that gets shared between gdb and gdbserver
> then?

Sounds good to me.

>>> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
>>> index fc10e8600ba..c6836b63c50 100644
>>> --- a/gdb/gdbarch-components.py
>>> +++ b/gdb/gdbarch-components.py
>>> @@ -1116,15 +1116,21 @@ possible it should be in TARGET_READ_PC instead).
>>>       invalid=False,
>>>   )
>>>   -Value(
>>> +Method(
>>>       comment="""
>>> -On some machines, not all bits of an address word are significant.
>>> -For example, on AArch64, the top bits of an address known as the "tag"
>>> -are ignored by the kernel, the hardware, etc. and can be regarded as
>>> -additional data associated with the address.
>>> +On some architectures, not all bits of a pointer are significant.
>>> +On AArch64, for example, the top bits of a pointer may carry a "tag", which
>>> +can be ignored by the kernel and the hardware. The "tag" can be regarded as
>>> +additional data associated with the pointer, but it is not part of the address.
>>> +
>>> +Given a pointer for the architecture, this hook removes all the
>>> +non-significant bits and sign-extends things as needed. It is used mostly
>>> +for data pointers, as opposed to code pointers.
>> Is the last sentence just FYI or does it imply some consequence for the
>> GDB user or even GDB itself?
>> 
>
> More a FYI really. This was documented somewhere in the code, and I wanted to
> bring it to the manual. I'll clarify it.

Thanks. The reason I asked is that if I were trying to use
gdbarch_remove_non_address_bits, I didn't know whether there was
something I should be aware of, or account for.

-- 
Thiago