Re: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.

[Luis Machado <luis.machado@arm.com>]

On 6/14/24 10:38, Schimpe, Christina wrote:
> Hi Luis, 
> 
> Do you still have concerns on the topic discussed below?
> Otherwise, I'd apply Felix latest feedback and push a new version of this series.

Nothing that should block progress on this. My concern was mostly the odd situation
with the hooks. But I can revisit that if it turns out to be a problem.


> 
> Thanks,
> Christina
> 
>> -----Original Message-----
>> From: Schimpe, Christina
>> Sent: Monday, June 10, 2024 5:06 PM
>> To: Luis Machado <luis.machado@arm.com>; Willgerodt, Felix
>> <FeliX.Willgerodt@intel.com>; gdb-patches@sourceware.org
>> Cc: eliz@gnu.org
>> Subject: RE: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
>>
>>
>>
>>> -----Original Message-----
>>> From: Luis Machado <luis.machado@arm.com>
>>> Sent: Monday, June 10, 2024 4:01 PM
>>> To: Schimpe, Christina <christina.schimpe@intel.com>; Willgerodt,
>>> Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
>>> Cc: eliz@gnu.org
>>> Subject: Re: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
>>>
>>> On 6/3/24 15:13, Schimpe, Christina wrote:
>>>> Hi Luis,
>>>>
>>>>>>>> -/* AArch64 implementation of the remove_non_address_bits
>>>>>>>> gdbarch
>>>>>>> hook.
>>>>>>>> Remove
>>>>>>>> -   non address bits from a pointer value.  */
>>>>>>>> -
>>>>>>>> -static CORE_ADDR
>>>>>>>> +CORE_ADDR
>>>>>>>>  aarch64_remove_non_address_bits (struct gdbarch *gdbarch,
>>>>> CORE_ADDR
>>>>>>>> pointer)
>>>>>>>
>>>>>>> Shouldn't there still be some sort of comment for this function
>>>>>>> in the c
>>>>> file?
>>>>>>> At least some "see header file"? Though it seems like no function
>>>>>>> in
>>>>>>> aarch64- tdep.h has any comment, and all comments are in
>>>>>>> aarch64-tdep.c. I would also be fine with that for consistency.
>>>>>>> Maybe Luis can comment how he prefers it.
>>>>>>>
>>>>>>> But I am fine with this patch. Let's see if someone else objects this
>> split.
>>>>>>>
>>>>>>> Reviewed-By: Felix Willgerodt <felix.willgerodt@intel.com>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Felix
>>>>>>
>>>>>> Ok, let's wait for further comments here.
>>>>>
>>>>> Sorry, only spotted this now.
>>>>>
>>>>> The comment on the above function would be a repetition of the hook
>>>>> explanation, and the gdbarch hook explanation covers all the
>>>>> details we need. But if the comment is being removed due to it
>>>>> being in the header, we should point to the header instead.
>>>>>
>>>>> "See aarch64-tdep.h" should do.
>>>>
>>>> Ok, I will add that line.
>>>>
>>>>> There is something odd about this patch though. The aarch64 code is
>>>>> being changed to call aarch64_remove_non_address_bits directly, but
>>>>> 3 new hooks are being set:
>>>>>
>>>>> +  set_gdbarch_remove_non_address_bits_watchpoint
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_breakpoint
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_memory
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>>
>>>>> But the above hooks (the gdbarch_remove_non_address_bits_memory
>> at
>>>>> least) never get used in aarch64 code. Is there a reason for that?
>>>>
>>>> I thought it's wrong to call the
>>>> gdbarch_remove_non_address_bits_memory
>>> hook in aarch64 code.
>>>> The reason is described here:
>>>> https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html
>>>>
>>>> "Also I noticed that the gdbarch function
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> should not be used in several functions of aarch64* files (patch 1)
>>>> due to the following:
>>>>
>>>> The function description for 'gdbarch_remove_non_addr_bits_memory'
>>> states:
>>>>
>>>> "Given a pointer for the architecture, this hook removes all the
>>>> non-significant bits and sign-extends things as needed. It gets used
>>>> to remove non-address bits from any pointer used to access memory. "
>>>>
>>>> We don't know if the pointer passed to
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> by 'gdbarch_tagged_address_p' will be used to access memory.  That's
>>>> why it's wrong to call this function in 'gdbarch_tagged_address_p'.
>>>>
>>>> There are several similar wrong calls of
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> in the changes of patch 1.  I replaced all of them with
>>>> 'aarch64_remove_non_address_bits'."
>>>>
>>>
>>> That's fine, but previously the hook was correct for what is was being
>>> used for in aarch64. This particular change means we no longer have a
>>> hook that
>>> aarch64 can use for the same purposes it used before. This part feels a bit
>> odd.
>>> It feels like the memory-specific one would be appropriate, but it isn't clear.
>>
>> Hm, I thought it is not a problem to remove the hook in the aarch64 files, as
>> it's target specific code.
>>
>>> Also, from the 1/3 patch submission:
>>>
>>> "- In contrast to the implementation for ARM's TBI, the Linux kernel
>> supports
>>>   tagged pointers for memory access."
>>>
>>> Is there a negative impact if we feed the kernel an untagged pointer
>>> for a particular memory access? If not, it sounds like it might be
>>> simpler (from gdb's
>>> perspective) to let gdb always unmask things, no? Of course, there
>>> might be a performance impact of checking if a region is tagged.
>>> I'm just trying to understand if the additional hooks are really
>> needed/useful.
>>>
>>> Right now it looks as if we could make it work with the existing hook
>>> in place, instead of splitting it into 3 different hooks.
>>
>> I think in case of amd64 there is a negative impact.  In contrast to ARM's TBI,
>> LAM can be enabled at runtime.  So we would have to parse the
>> /proc/<pid>/status file every time when the pointer is used to access
>> memory. With the 3 hooks I wanted to avoid this.
>>
>> I tried to make the purpose more clear by adding the two sentences in the
>> commit message for v2:
>>
>> "This way, one can make sure that addresses are only adjusted when
>> necessary.
>> In case of LAM, this avoids unnecessary parsing of the /proc/<pid>/status file
>> to get the untag mask."
>>
>> So yes, it does work with the existing hook, but when it comes to
>> performance, I think 3 hooks would be better.
>>
>> Christina
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928