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

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

Hi Simon,

On 12/9/22 19:14, Simon Marchi wrote:
>> @@ -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.
> 

I decided to declare it here in case we skip the if block, then we return the default value.

Did you have something else in mind?

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

Fixed.

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

Done.

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

Oops. Fixed.

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

Indeed. Fixed now to use the global access notation.

>> +
>> +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).
> 

Fixed it to use wrap now. Thanks!

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

Done.

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

Done now. I'll send a v4.

Thanks for the review.