Re: [PATCH v4 01/11] gdbserver: Add optional runtime register set type.

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

On 6/17/25 13:11, Christina Schimpe wrote:
> Some register sets can be activated and deactivated by the OS during the runtime of
> a process.  One example register is the Intel CET shadow stack pointer.  This patch
> adds a new type of register set to handle such cases.  We shouldn't deactivate these
> regsets and should not show a warning if the register set is not active but supported
> by the kernel.  However, it is safe to deactivate them, if they are unsupported by the
> kernel.  To differentiate those scenarios we can use the errno returned by the ptrace
> call.
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdbserver/linux-low.cc | 50 ++++++++++++++++++++++++++++++------------
>  gdbserver/linux-low.h  |  7 +++++-
>  2 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 1d223c11db9..93fa14ddae9 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5006,23 +5006,31 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
>        if (res < 0)
>  	{
>  	  if (errno == EIO
> -	      || (errno == EINVAL && regset->type == OPTIONAL_REGS))
> +	      || (errno == EINVAL
> +		  && (regset->type == OPTIONAL_REGS
> +		      || regset->type == OPTIONAL_RUNTIME_REGS)))
>  	    {
>  	      /* If we get EIO on a regset, or an EINVAL and the regset is
> -		 optional, do not try it again for this process mode.  */
> +		 optional, do not try it again for this process mode.
> +		 Even if the regset can be enabled at runtime it is safe
> +		 to deactivate the regset in case of EINVAL, as we know
> +		 the regset itself was the invalid argument of the ptrace
> +		 call which means that it's unsupported by the kernel.  */
>  	      disable_regset (regsets_info, regset);
>  	    }
> -	  else if (errno == ENODATA)
> +	  else if (errno == ENODATA
> +		   || (errno == ENODEV
> +		       && regset->type == OPTIONAL_RUNTIME_REGS)
> +		   || errno == ESRCH)
>  	    {
> -	      /* ENODATA may be returned if the regset is currently
> -		 not "active".  This can happen in normal operation,
> -		 so suppress the warning in this case.  */
> -	    }
> -	  else if (errno == ESRCH)
> -	    {
> -	      /* At this point, ESRCH should mean the process is
> -		 already gone, in which case we simply ignore attempts
> -		 to read its registers.  */
> +	      /* ENODATA or ENODEV may be returned if the regset is
> +		 currently not "active".  For ENODEV we additionally check
> +		 if the register set is of type OPTIONAL_RUNTIME_REGS.
> +		 This can happen in normal operation, so suppress the
> +		 warning in this case.
> +		 ESRCH should mean the process is already gone at this
> +		 point, in which case we simply ignore attempts to read
> +		 its registers.  */
>  	    }
>  	  else
>  	    {
> @@ -5104,12 +5112,26 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
>        if (res < 0)
>  	{
>  	  if (errno == EIO
> -	      || (errno == EINVAL && regset->type == OPTIONAL_REGS))
> +	      || (errno == EINVAL
> +		   && (regset->type == OPTIONAL_REGS
> +		       || regset->type == OPTIONAL_RUNTIME_REGS)))
>  	    {
>  	      /* If we get EIO on a regset, or an EINVAL and the regset is
> -		 optional, do not try it again for this process mode.  */
> +		 optional, do not try it again for this process mode.
> +		 Even if the regset can be enabled at runtime it is safe
> +		 to deactivate the regset in case of EINVAL, as we know
> +		 the regset itself was the invalid argument of the ptrace
> +		 call which means that it's unsupported by the kernel.  */
>  	      disable_regset (regsets_info, regset);
>  	    }
> +	  else if (errno == ENODEV
> +		   && regset->type == OPTIONAL_RUNTIME_REGS)
> +	    {
> +	      /* If we get ENODEV on a regset and the regset can be
> +		 enabled at runtime try it again for this process mode.
> +		 This can happen in normal operation, so suppress the
> +		 warning in this case.  */
> +	    }
>  	  else if (errno == ESRCH)
>  	    {
>  	      /* At this point, ESRCH should mean the process is
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 7d3e35fa9b0..8a2b4302c76 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -42,7 +42,12 @@ enum regset_type {
>    GENERAL_REGS,
>    FP_REGS,
>    EXTENDED_REGS,
> -  OPTIONAL_REGS, /* Do not error if the regset cannot be accessed.  */
> +  OPTIONAL_REGS, /* Do not error if the regset cannot be accessed.
> +		    Disable the regset instead.  */
> +  OPTIONAL_RUNTIME_REGS, /* Some optional regsets can only be accessed
> +			    dependent on the execution flow.  For such
> +			    access errors don't show a warning and don't
> +			    disable the regset.  */
>  };
>  
>  /* The arch's regsets array initializer must be terminated with a NULL

This looks OK to me.

Reviewed-By: Luis Machado <luis.machado@arm.com>