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

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

On 6/3/24 09:40, Schimpe, Christina wrote:
> Thank you for the review, Felix.
> 
> Also it seems that my series introduces a regression on arm detected by linaro CI:
> 
> https://ci.linaro.org/job/tcwg_gdb_check--master-arm-precommit/2549/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
> 
> The reason is probably this patch, but I am unable to reproduce this locally and haven't been able to figure out what might have caused this ... 
> 
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index
>>> 8d0553f3d7c..af62d3ae1ec 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -4086,10 +4086,7 @@ aarch64_stack_frame_destroyed_p (struct
>> gdbarch
>>> *gdbarch, CORE_ADDR pc)
>>>    return streq (inst.opcode->name, "ret");  }
>>>
>>> -/* 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.

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?

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