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

Luis Machado
2025-06-19T09:30:15Z

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>