Re: [PATCH 02/12] gdbserver: Add optional runtime register set type.

[Guinevere Larsen <guinevere@redhat.com>]

On 12/20/24 5:04 PM, Schimpe, Christina 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 adds a new type of register set to handle such cases.  We
> shouldn't deactivate these regsets and should not show a warning if we
> fail to read them.
> ---
>   gdbserver/linux-low.cc | 40 ++++++++++++++++++++++++++++------------
>   gdbserver/linux-low.h  |  7 ++++++-
>   2 files changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 50ce2b44927..355b28d9fe4 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5007,23 +5007,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.  */
>   	      disable_regset (regsets_info, regset);

I'm somewhat confused by this patch.

The commit message and other comments here say that 
optional_runtime_regs shouldn't be disabled. However, in here, if we get 
EINVAL we *will* disable the regset. Did you mean to use != instead of == ?

I'll be honest, I don't know enough about the regset subsystem to know 
which is the correct option, I just think it has to be consistent.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>   	    }
> -	  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
>   	    {
> @@ -5111,6 +5119,14 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
>   		 optional, do not try it again for this process mode.  */
>   	      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 5be00b8c98c..da5aa26a993 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