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

[Omair Javaid <omair dot javaid at linaro dot org>]

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!