Re: [PATCH] [PR gdb/23127] [AArch64] Fix tagged pointer support

Omair Javaid
2018-05-08T09:32:03Z

On 1 May 2018 at 20:02, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 01/05/18 02:31, Omair Javaid wrote:
>>
>> This patch fixes tagged pointer support for AArch64 GDB. Linux kernel
>> debugging
>> failure was reported after tagged pointer support was committed.
>>
>> After a discussion around best path forward to manage tagged pointers on
>> GDB
>> side we are going to disable tagged pointers support for
>> aarch64-none-elf-gdb
>> because for non-linux applications we cant be sure if tagged pointers will
>> be
>> used by MMU or not.
>>
>> Also for aarch64-linux-gdb we are going to sign extend user-space address
>> after
>> clearing tag bits. This will help us debug both kernel and user-space
>> addresses
>> based on information from linux kernel documentation given below:
>>
>> According to AArch64 memory map:
>> https://www.kernel.org/doc/Documentation/arm64/memory.txt
>>
>> "User addresses have bits 63:48 set to 0 while the kernel addresses have
>> the same bits set to 1."
>>
>> According to AArch64 tagged pointers document:
>> https://www.kernel.org/doc/Documentation/arm64/tagged-pointers.txt
>>
>> The kernel configures the translation tables so that translations made
>> via TTBR0 (i.e. userspace mappings) have the top byte (bits 63:56) of
>> the virtual address ignored by the translation hardware. This frees up
>> this byte for application use.
>>
>> Running gdb testsuite after applying this patch introduces no regressions
>> and
>> tagged pointer test cases still pass.
>
>
> ... and I kicked the tyres a little bit using kgdb.
>
> print worked as expected, backtrace no longer provokes a gdb panic and
> breakpoints work (albeit for rather approximate definition of work... and
> the need for approximation is not gdb's fault).
>
>
> Daniel.
>
>
>
>> gdb/ChangeLog:
>> 2018-05-01 Omair Javaid <omair.javaid@linaro.org>
>>
>> * aarch64-linux-tdep.c (aarch64_linux_init_abi): Add call to
>> set_gdbarch_significant_addr_bit.
>> * aarch64-tdep.c (aarch64_gdbarch_init): Remove call to
>> set_gdbarch_significant_addr_bit.
>> * utils.c (address_significant): Update to sign extend addr.
>> ---
>> gdb/aarch64-linux-tdep.c | 5 +++++
>> gdb/aarch64-tdep.c | 5 -----
>> gdb/utils.c | 14 +++++++++-----
>> 3 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 1f3e888..ba5757d 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -1062,6 +1062,11 @@ aarch64_linux_init_abi (struct gdbarch_info info,
>> struct gdbarch *gdbarch)
>> /* Syscall record. */
>> tdep->aarch64_syscall_record = aarch64_linux_syscall_record;
>> + /* The top byte of a user space address known as the "tag",
>> + is ignored by the kernel and can be regarded as additional
>> + data associated with the address. */
>> + set_gdbarch_significant_addr_bit (gdbarch, 56);
>> +
>> /* Initialize the aarch64_linux_record_tdep. */
>> /* These values are the size of the type that will be used in a system
>> call. They are obtained from Linux Kernel source. */
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 01566b4..3c1f389 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2972,11 +2972,6 @@ aarch64_gdbarch_init (struct gdbarch_info info,
>> struct gdbarch_list *arches)
>> set_tdesc_pseudo_register_reggroup_p (gdbarch,
>>
>> aarch64_pseudo_register_reggroup_p);
>> - /* The top byte of an address is known as the "tag" and is
>> - ignored by the kernel, the hardware, etc. and can be regarded
>> - as additional data associated with the address. */
>> - set_gdbarch_significant_addr_bit (gdbarch, 56);
>> -
>> /* ABI */
>> set_gdbarch_short_bit (gdbarch, 16);
>> set_gdbarch_int_bit (gdbarch, 32);
>> diff --git a/gdb/utils.c b/gdb/utils.c
>> index b957b0d..1f9be8f 100644
>> --- a/gdb/utils.c
>> +++ b/gdb/utils.c
>> @@ -2704,14 +2704,18 @@ When set, debugging messages will be marked with
>> seconds and microseconds."),
>> CORE_ADDR
>> address_significant (gdbarch *gdbarch, CORE_ADDR addr)
>> {
>> - /* Truncate address to the significant bits of a target address,
>> - avoiding shifts larger or equal than the width of a CORE_ADDR.
>> - The local variable ADDR_BIT stops the compiler reporting a shift
>> - overflow when it won't occur. */
>> + /* Clear insignificant bits of a target address and sign extend
>> resulting
>> + address, avoiding shifts larger or equal than the width of a
>> CORE_ADDR.
>> + The local variable ADDR_BIT stops the compiler reporting a shift
>> overflow
>> + when it won't occur. */
>> int addr_bit = gdbarch_significant_addr_bit (gdbarch);
>> if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
>> - addr &= ((CORE_ADDR) 1 << addr_bit) - 1;
>> + {
>> + CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1);
>> + addr &= ((CORE_ADDR) 1 << addr_bit) - 1;
>> + addr = (addr ^ sign) - sign;
>> + }
>> return addr;
>> }
>>
>

Ping!

Hi Pedro,

I was wondering if you can kindly help review this patch.
This is a critical bug as it blocks kernel debugging on AArch64.
Also can we push this to GDB 8.1.1 once it gets accepted?

Thanks!