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

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

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.

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.



> Regards,
> 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