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

[Daniel Thompson <daniel dot thompson at linaro dot org>]

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