Re: [PATCH,v3] [aarch64] Fix removal of non-address bits for PAuth

[Simon Marchi <simark@simark.ca>]

> @@ -1961,6 +1962,47 @@ aarch64_linux_decode_memtag_section (struct gdbarch *gdbarch,
>    return tags;
>  }
>  
> +/* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
> +   non address bits from a pointer value.  */
> +
> +static CORE_ADDR
> +aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
> +{
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +
> +  /* By default, we assume TBI and discard the top 8 bits plus the VA range
> +     select bit (55).  */
> +  CORE_ADDR mask = AARCH64_TOP_BITS_MASK;
Declare mask in the context where it's needed.

> +
> +  if (tdep->has_pauth ())
> +    {
> +      /* Fetch the PAC masks.  These masks are per-process, so we can just
> +	 fetch data from whatever thread we have at the moment.
> +
> +	 Also, we have both a code mask and a data mask.  For now they are the
> +	 same, but this may change in the future.  */
> +      struct regcache *regs = get_current_regcache ();
> +      CORE_ADDR cmask, dmask;
> +
> +      if (regs->cooked_read (tdep->pauth_reg_base, &dmask) != REG_VALID)
> +	dmask = mask;
> +
> +      if (regs->cooked_read (tdep->pauth_reg_base + 1, &cmask) != REG_VALID)
> +	cmask = mask;
> +
> +      if (dmask != cmask)
> +	{
> +	  /* Warn if the masks are different.  */
> +	  aarch64_pauth_mask_warning ();
> +	  mask |= dmask > cmask? dmask : cmask;

I think we typically put a space before the ? operator, like for other
operators.

> @@ -143,4 +156,8 @@ enum aarch64_regnum
>  /* Maximum supported VQ value.  Increase if required.  */
>  #define AARCH64_MAX_SVE_VQ  16
>  
> +/* Mask with 1's in bits 55~63, used to remove the top byte of pointers
> +   (Top Byte Ignore).  */
> +#define AARCH64_TOP_BITS_MASK (~((((CORE_ADDR) 1) << (64 - 9)) - 1))

I think I would find it clearer to just have the numerical value right
there.

> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 71aa5991fbe..25b6d69d2d9 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -1116,15 +1116,23 @@ possible it should be in TARGET_READ_PC instead).
>      invalid=False,
>  )
>  
> -Value(
> +Method(
>      comment="""
> -On some machines, not all bits of an address word are significant.
> -For example, on AArch64, the top bits of an address known as the "tag"
> -are ignored by the kernel, the hardware, etc. and can be regarded as
> -additional data associated with the address.
> +On some architectures, not all bits of a pointer are significant.
> +On AArch64, for example, the top bits of a pointer may carry a "tag", which
> +can be ignored by the kernel and the hardware. The "tag" can be regarded as
> +additional data associated with the pointer, but it is not part of the address.
> +
> +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 data pointers (for example, removing the AArch64 MTE tag
> +bits from a pointer) and from code pointers (removing the AArch64 PAC signature
> +from a pointer containing the return address).

Two spaces after period above, a few times.

> diff --git a/gdb/testsuite/gdb.arch/aarch64-non-address-bits.exp b/gdb/testsuite/gdb.arch/aarch64-non-address-bits.exp
> new file mode 100644
> index 00000000000..2b5a7b2f517
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-non-address-bits.exp
> @@ -0,0 +1,121 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# This file is part of the gdb testsuite.
> +#
> +# Test that GDB for AArch64/Linux can properly handle pointers with
> +# the upper 16 bits (PAC) or 8 bits (Tag) set, as well as the
> +# VA_RANGE_SELECT bit (55).
> +
> +if {![is_aarch64_target]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +global hex
> +global decimal

I don't think it's useful to put a global declaration like this in the
global scope.  And as you probably know by know, I encourage people to
use the $::hex notation anyway, which doesn't need the global keyword.

> +
> +standard_testfile
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# We need to iterate over two distinct ranges, separated by a single bit.
> +# This bit is 55 (VA_RANGE_SELECT) which tells us if we have a kernel-space
> +# address or a user-space address.
> +
> +# The tag field has 8 bits.
> +set tag_bits_count 8
> +
> +# The pac field has 7 bits.
> +set pac_bits_count 7
> +
> +# A couple patterns that we reuse for the tests later.  One is for a successful
> +# memory read and the other is for a memory read failure.
> +set memory_read_ok_pattern "$hex\( <l>\)?:\[ \t\]+$hex"
> +set memory_read_fail_pattern "$hex:\[ \t\]+Cannot access memory at address $hex"
> +
> +set pac_enabled 0
> +
> +# Check if PAC is enabled.
> +gdb_test_multiple "ptype \$pauth_cmask" "fetch PAC cmask" {
> +    -re "type = long\r\n$gdb_prompt" {
> +	set pac_enabled 1
> +    }
> +    -re "type = void\r\n$gdb_prompt" {
> +    }

Pedantically, you probably want to consume a space after the
$gdb_prompt.  You could also use -wrap in all three cases to simplify
them (and in the gdb_test_multiples below too).

> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index db508696261..155706f1138 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -508,21 +508,36 @@ aarch64_target::low_remove_point (raw_bkpt_type type, CORE_ADDR addr,
>    return ret;
>  }
>  
> -/* Return the address only having significant bits.  This is used to ignore
> -   the top byte (TBI).  */
> -
>  static CORE_ADDR
> -address_significant (CORE_ADDR addr)
> +aarch64_remove_non_address_bits (CORE_ADDR pointer)
>  {
> -  /* Clear insignificant bits of a target address and sign extend resulting
> -     address.  */
> -  int addr_bit = 56;
> +  /* By default, we assume TBI and discard the top 8 bits plus the
> +     VA range select bit (55).  */
> +  CORE_ADDR mask = AARCH64_TOP_BITS_MASK;
> +
> +  /* Check if PAC is available for this target.  */
> +  if (tdesc_contains_feature (current_process ()->tdesc,
> +			      "org.gnu.gdb.aarch64.pauth"))
> +    {
> +      /* Fetch the PAC masks.  These masks are per-process, so we can just
> +	 fetch data from whatever thread we have at the moment.
> +
> +	 Also, we have both a code mask and a data mask.  For now they are the
> +	 same, but this may change in the future.  */
>  
> -  CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1);
> -  addr &= ((CORE_ADDR) 1 << addr_bit) - 1;
> -  addr = (addr ^ sign) - sign;
> +      struct regcache *regs = get_thread_regcache (current_thread, 1);
> +      CORE_ADDR dmask = regcache_raw_get_unsigned_by_name (regs, "pauth_dmask");
> +      CORE_ADDR cmask = regcache_raw_get_unsigned_by_name (regs, "pauth_cmask");
> +
> +      if (dmask != cmask && dmask != 0 && cmask != 0)
> +	{
> +	  /* Warn if the masks are different.  */
> +	  aarch64_pauth_mask_warning ();
> +	  mask |= (dmask > cmask)? dmask : cmask;

I think you can omit the parenthesis (like you did in the GDB version of
the code.

It would be nice to factor out what we can to the common code.  Do you
think you can move this part to the common code?

      if (dmask != cmask && dmask != 0 && cmask != 0)
	{
	  /* Warn if the masks are different.  */
	  aarch64_pauth_mask_warning ();
	  mask |= (dmask > cmask)? dmask : cmask;
	}

Simon