Re: [PATCH 1/2] gdb, aarch64: cache pointer authentication masks per inferior

[Simon Marchi <simark@simark.ca>]

On 9/12/26 6:20 PM, Luis Machado wrote:
> aarch64_remove_non_address_bits recomputed the pointer authentication
> masks on every call by walking the current inferior's thread list,
> looking up its regcache, and reading the dmask/cmask registers from
> the target. This function is called from memory_xfer_partial for
> every memory transfer GDB performs, as well as from the watchpoint
> and breakpoint address-masking hooks, so the lookup happens far more
> often than the masks can possibly change.

Not against this patch, but just a precision: after the first read (and
until the thread resumes), the registers are cached in the regcache, so
a read wouldn't have to read the registers from the target again (which
is probably the most expensive part).

And just wondering, do you see a real world impact with this patch, is
there some measurable improvement?

> The masks are fixed for the life of a process, since they reflect the
> kernel's VA-size configuration at exec time, and are shared by all of
> a process' threads. Cache the computed mask per inferior (one slot
> each for the low and high VA ranges) instead of recomputing it on
> every call, only populating the cache when a thread is actually
> stopped so a transient "thread running" state never gets cached as a
> permanent answer. The cache is invalidated on inferior exit,
> inferior appeared, and exec, since those are the only points where a
> process' mask configuration could legitimately change.
> ---
>  gdb/aarch64-tdep.c | 154 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 112 insertions(+), 42 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 950ad4f6aae..c7003687eab 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -57,6 +57,9 @@
>  
>  /* For inferior_ptid and current_inferior ().  */
>  #include "inferior.h"
> +/* For gdb::observers::inferior_exit et al, used to invalidate the pauth
> +   mask cache.  */
> +#include "observable.h"

IMO these comments are useless.

>  /* For std::sqrt and std::pow.  */
>  #include <cmath>
>  
> @@ -4333,6 +4336,47 @@ aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>    return string_printf ("0x%s", phex_nz (tag));
>  }
>  
> +/* Cached pointer authentication masks for an inferior.  The masks are
> +   fixed for the life of a process (they reflect the kernel's VA-size
> +   configuration at exec time) and are shared by all its threads.  We only
> +   need to compute them once per inferior instead of on every call to
> +   aarch64_remove_non_address_bits.  LOW is used for user-space (low VA
> +   range) pointers, HIGH is used for kernel-space (high VA range)
> +   pointers.  HIGH is only ever populated on targets that provide the
> +   high-range mask registers.  */
> +
> +struct aarch64_pauth_mask_cache
> +{
> +  std::optional<CORE_ADDR> low;
> +  std::optional<CORE_ADDR> high;
> +};
> +
> +/* Per-inferior pauth mask cache.  */
> +
> +static const registry<inferior>::key<aarch64_pauth_mask_cache>
> +  aarch64_pauth_mask_cache_data;

To follow the conventions used elsewhere, I would suggest make a
per-inferior object, with a pauth mask field in it:

struct aarch64_per_inferior
{
  struct pauth_mask
  {
    std::optional<CORE_ADDR> low;
    std::optional<CORE_ADDR> high;
  };
};

The registry key variable would be called "aarch64_per_inferior_data",
"aarch64_per_inferior_key", or something like that.

Then you'd have a function "get_aarch64_per_inferior" that uses
try_emplace to abstract the "create if needed" operation.

> +
> +/* Drop INF's cached pauth masks.  */
> +
> +static void
> +aarch64_invalidate_pauth_mask_cache (inferior *inf)
> +{
> +  aarch64_pauth_mask_cache_data.clear (inf);
> +}
> +
> +/* Drop the pauth mask cache of every inferior sharing PSPACE.  This is
> +   attached to the all_objfiles_removed observer, which fires on exec.
> +   Exec is the only point after startup where a process' VA-size
> +   configuration, and hence its masks, could legitimately change.  */
> +
> +static void
> +aarch64_pauth_mask_cache_objfiles_removed (program_space *pspace)
> +{
> +  for (inferior *inf : all_inferiors ())
> +    if (inf->pspace == pspace)
> +      aarch64_invalidate_pauth_mask_cache (inf);
> +}

We have an observable inferior_execd, can't you use that?

When we need to reset some cached inferior state, we often use the trio
of observables:

 - inferior created/appeared
 - inferior execd
 - inferior exited

I never remember the different between created and appeared, it might be
related to run vs attach, not sure.  There is probably one more correct
that the other.

> +
>  /* See aarch64-tdep.h.  */
>  
>  CORE_ADDR
> @@ -4353,54 +4397,72 @@ aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
>       momentarily), we use the inferior ptid.  */
>    if (inferior_ptid != null_ptid)
>      {
> -      /* If we do have an inferior, attempt to fetch its thread's thread_info
> -	 struct.  */
> -      thread_info *thread = current_inferior ()->find_thread (inferior_ptid);
> +      inferior *inf = current_inferior ();
> +      bool kernel_address = (pointer & VA_RANGE_SELECT_BIT_MASK) != 0;
>  
> -      /* If the thread is running, we will not be able to fetch the mask
> -	 registers.  */
> -      if (thread != nullptr && thread->state () != THREAD_RUNNING)
> -	{
> -	  /* Otherwise, fetch the register cache and the masks.  */
> -	  struct regcache *regs
> -	    = get_thread_regcache (current_inferior ()->process_target (),
> -				   inferior_ptid);
> -
> -	  /* Use the gdbarch from the register cache to check for pointer
> -	     authentication support, as it matches the features found in
> -	     that particular thread.  */
> -	  aarch64_gdbarch_tdep *tdep
> -	    = gdbarch_tdep<aarch64_gdbarch_tdep> (regs->arch ());
> +      aarch64_pauth_mask_cache *cache
> +	= aarch64_pauth_mask_cache_data.get (inf);
> +      if (cache == nullptr)
> +	cache = &aarch64_pauth_mask_cache_data.emplace (inf);
>  
> -	  /* Is there pointer authentication support?  */
> -	  if (tdep->has_pauth ())
> +      std::optional<CORE_ADDR> &cached_mask
> +	= kernel_address ? cache->high : cache->low;
> +
> +      if (cached_mask.has_value ())
> +	mask = *cached_mask;
> +      else
> +	{
> +	  /* If we do have an inferior, attempt to fetch its thread's
> +	     thread_info struct.  */
> +	  thread_info *thread = inf->find_thread (inferior_ptid);
> +
> +	  /* If the thread is running, we will not be able to fetch the mask
> +	     registers.  Leave the cache empty for this slot, we'll get
> +	     another chance to compute and cache it once some thread of
> +	     this inferior is next stopped.  */
> +	  if (thread != nullptr && thread->state () != THREAD_RUNNING)

I know it's pre-existing, but checking for THREAD_RUNNING seems wrong to
me.  The thread can be running from the point of view of the user by
really stopped at the ptrace level.  It happens for instance when
evaluating a breakpoint condition, since we haven't decided yet if the
breakpoint should cause a user visible stop or not.  I

>  	    {
> -	      CORE_ADDR cmask, dmask;
> -	      int dmask_regnum
> -		= AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base);
> -	      int cmask_regnum
> -		= AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
> -
> -	      /* If we have a kernel address and we have kernel-mode address
> -		 mask registers, use those instead.  */
> -	      if (tdep->pauth_reg_count > 2
> -		  && pointer & VA_RANGE_SELECT_BIT_MASK)
> +	      /* Otherwise, fetch the register cache and the masks.  */
> +	      struct regcache *regs
> +		= get_thread_regcache (inf->process_target (), inferior_ptid);
> +
> +	      /* Use the gdbarch from the register cache to check for pointer
> +		 authentication support, as it matches the features found in
> +		 that particular thread.  */
> +	      aarch64_gdbarch_tdep *tdep
> +		= gdbarch_tdep<aarch64_gdbarch_tdep> (regs->arch ());
> +
> +	      /* Is there pointer authentication support?  */
> +	      if (tdep->has_pauth ())
>  		{
> -		  dmask_regnum
> -		    = AARCH64_PAUTH_DMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> -		  cmask_regnum
> -		    = AARCH64_PAUTH_CMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> +		  CORE_ADDR cmask, dmask;
> +		  int dmask_regnum
> +		    = AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base);
> +		  int cmask_regnum
> +		    = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
> +
> +		  /* If we have a kernel address and we have kernel-mode
> +		     address mask registers, use those instead.  */
> +		  if (tdep->pauth_reg_count > 2 && kernel_address)
> +		    {
> +		      dmask_regnum
> +			= AARCH64_PAUTH_DMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> +		      cmask_regnum
> +			= AARCH64_PAUTH_CMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> +		    }
> +
> +		  /* We have both a code mask and a data mask.  For now they
> +		     are the same, but this may change in the future.  */
> +		  if (regs->cooked_read (dmask_regnum, &dmask) != REG_VALID)
> +		    dmask = mask;
> +
> +		  if (regs->cooked_read (cmask_regnum, &cmask) != REG_VALID)
> +		    cmask = mask;
> +
> +		  mask |= aarch64_mask_from_pac_registers (cmask, dmask);
>  		}
>  
> -	      /* We have both a code mask and a data mask.  For now they are
> -		 the same, but this may change in the future.  */
> -	      if (regs->cooked_read (dmask_regnum, &dmask) != REG_VALID)
> -		dmask = mask;
> -
> -	      if (regs->cooked_read (cmask_regnum, &cmask) != REG_VALID)
> -		cmask = mask;
> -
> -	      mask |= aarch64_mask_from_pac_registers (cmask, dmask);
> +	      cached_mask = mask;

This function becomes a bit complicated, I wouldn't find if you wanted
to move this scope (fetch the masks from the inferior for real) to a
helper function.

Simon