Re: [PATCH 07/12] gdb, bfd: amd64 linux coredump support with shadow stack.

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> From: Felix Willgerodt <felix.willgerodt@intel.com>
>
> Intel's Control-Flow Enforcement Technology (CET) provides the shadow
> stack feature for the x86 architecture.
>
> This commit adds support to write and read the shadow-stack node in
> corefiles.  This helps debugging return address violations post-mortem.
> The format is synced with the linux kernel commit "x86: Add PTRACE
> interface for shadow stack".  As the linux kernel restricts shadow
> stack support to 64-bit, apply the fix for amd64 only.
>
> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> ---
>  bfd/elf.c                                     | 24 +++++++++

I don't know what is current practice: should BFD changes be sent
separately to the binutils mailing list, or in the same patch or patch
series as the GDB changes that use them?

If the latter, then the binutils mailing list needs to at least be
on Cc:

>  gdb/amd64-linux-tdep.c                        | 52 +++++++++++++++++--
>  .../gdb.arch/amd64-shadow-stack-corefile.exp  | 50 ++++++++++++++++++
>  3 files changed, 122 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp

Just some minor comments.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

> @@ -1630,6 +1642,30 @@ static const struct regset amd64_linux_xstateregset =
>      amd64_linux_collect_xstateregset
>    };
>
> +static void
> +amd64_linux_supply_ssp (const struct regset *regset,
> +			struct regcache *regcache, int regnum,
> +			const void *ssp, size_t len)
> +{
> +  x86_supply_ssp (regcache, *static_cast<const uint64_t *> (ssp));
> +}
> +
> +static void
> +amd64_linux_collect_ssp (const struct regset *regset,
> +			 const struct regcache *regcache, int regnum,
> +			 void *ssp, size_t len)
> +{
> +  x86_collect_ssp (regcache, *static_cast<uint64_t *> (ssp));
> +}

These functions should have documentation comments.

> +
> +/* Shadow stack pointer register.  */
> +
> +static const struct regset amd64_linux_ssp_register
> +  {
> +    NULL, amd64_linux_supply_ssp, amd64_linux_collect_ssp
> +  };
> +
> +

Just one line of separation between functions is enough.

>  /* Iterate over core file register note sections.  */
>
>  static void

--
Thiago