Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Luis <luis.machado.foss@gmail.com>
To: Simon Marchi <simark@simark.ca>, gdb-patches@sourceware.org
Cc: thiago.bauermann@linaro.org
Subject: Re: [PATCH 1/2] gdb, aarch64: cache pointer authentication masks per inferior
Date: Mon, 21 Sep 2026 00:47:14 +0100	[thread overview]
Message-ID: <29b277e4-1a9c-4636-aa05-f67a0d900bd9@gmail.com> (raw)
In-Reply-To: <1f40fceb-bcd3-450e-9454-f92054cebcce@simark.ca>

Hi,

On 14/09/2026 16:24, Simon Marchi wrote:
> 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).

True. I had forgotten about that cache for a bit. I'll reword the commit
message.

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

On impact, not a very significant one. Mostly a small local speedup due 
to skipping some function calls when we have a mask cache. Testing on Pi 
4 and 5 I don´t see a significant difference on stepping, memory reads, 
backtraces, software watchpoints etc (native or gdbserver).

A microbenchmark of function calls goes from about 79 ns to 9 ns with a 
warm regcache. So very minor.

There is one other improvement though, see below.

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

Dropped now.

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

Done. Thanks.

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

Switched to using inferior_execd now, alongside the other two 
(inferior_exit and inferior_appeared) Thanks for the heads up.

>> +
>>   /* 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
> 

Yeah. So this is another improvement area. I'm adding another fixup 
patch to this series that addresses this.

With the above check most of the calls in a stepi loop, and nearly all 
of them in a software watchpoint or conditional breakpoint loop, found 
the thread marked running and silently used the default mask.

Using can_access_registers_thread instead makes this work more reliably 
and actually reads the mask value whenever available, only using the 
default masks when we can´t read the registers of the thread.

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

True. I've split some code out into a helper now.

I'll send a v2 for this.

  reply	other threads:[~2026-09-20 23:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-12 22:20 [PATCH 0/2] " Luis Machado
2026-09-12 22:20 ` [PATCH 1/2] " Luis Machado
2026-09-14 15:24   ` Simon Marchi
2026-09-20 23:47     ` Luis [this message]
2026-09-12 22:20 ` [PATCH 2/2] gdb, aarch64: add selftest for the pauth mask cache Luis Machado

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=29b277e4-1a9c-4636-aa05-f67a0d900bd9@gmail.com \
    --to=luis.machado.foss@gmail.com \
    --cc=gdb-patches@sourceware.org \
    --cc=simark@simark.ca \
    --cc=thiago.bauermann@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox