RE: [PATCH v5 07/12] gdb: amd64 linux coredump support with shadow stack.

Schimpe, Christina
2025-07-30T11:43:29Z

Hi Thiago and Andrew,

Thanks a lot for your feedback.

I should have mentioned that I created this test based on the test code from Thiago's series GCS
core dump tests. I only mentioned it in the cover letter so far.

I'll do it similar to Thiago when he based his return command testing (aarch64-gcs-return.exp )
on CET shadow stack tests:
https://sourceware.org/pipermail/gdb-patches/2025-June/218895.html

So I'll add a comment in the test description

# Test the shadow stack pointer note in core dumps.
# Based on the corefile tests in gdb.arch/aarch64-gcs-core.exp.

and also a comment in the commit message

---
The code and testcase are lightly adapted from:

[PATCH v3 5/9] GDB, gdbserver: aarch64-linux: Initial Guarded Control Stack support

https://sourceware.org/pipermail/gdb-patches/2025-June/218892.html

in the next version.
Also I'll fix my test based on Andrew's feedback as described by Thiago below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Wednesday, July 30, 2025 3:55 AM
> To: Andrew Burgess <aburgess@redhat.com>
> Cc: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org; luis.machado@arm.com
> Subject: Re: [PATCH v5 07/12] gdb: amd64 linux coredump support with
> shadow stack.
>
> Hello Andrew,
>
> Christina based the testcase in this patch on aarch64-gcs-core.exp from my
> GCS patch series and your comments on it also apply to my patch, so I'm
> replying on account of that.
>
> Andrew Burgess <aburgess@redhat.com> writes:
>
> > Christina Schimpe <christina.schimpe@intel.com> writes:
> >
> >> + # Generate the gcore core file.
> >> + set gcore_filename [standard_output_file "${testfile}.gcore"]
> >> + set gcore_generated [gdb_gcore_cmd "$gcore_filename" "generate
> >> + gcore file"]
> >> +
> >> + # Obtain an OS-generated core file. Save test program output to
> >> + # ${binfile}.out.
> >> + set core_filename [core_find $binfile {} {} "${binfile}.out"]
> >> + set core_generated [expr {$core_filename != ""}]
> >> + set os_core_name "${binfile}.core"
> >> + remote_exec build "mv $core_filename $os_core_name"
> >> + set core_filename $os_core_name
> >
> > I'm wondering what the point of this core_filename / os_core_name
> > stuff is? My reading of `core_find` is that the returned
> > core_filename will be '${binfile}.core', so this whole thing feels
> > redundant, but maybe I'm missing something here?
>
> I wrote this part. The answer to your question is very simple: I don't know
> what I was thinking. I just deleted the last 3 lines from the GCS testcase.
>
> >> +
> >> + # At this point we have a couple of core files, the gcore one generated
> by
> >> + # GDB and the one generated by the operating system. Make sure
> GDB can
> >> + # read both correctly.
> >> +
> >> + if {$gcore_generated} {
> >> + clean_restart $binfile
> >> +
> >> + with_test_prefix "gcore corefile" {
> >> + check_core_file $gcore_filename $ssp_in_gcore
> >> + }
> >> + } else {
> >> + fail "gcore corefile not generated"
> >
> > It's better, where possible, to avoid having pass/fail results that
> > only show up down some code paths.
> >
> > In this case it's easy to avoid having a stray 'fail' by restructuring
> > the code too:
> >
> > gdb_assert { $gcore_generated } "gcore corefile created"
> > if { $gcore_generated } {
> > ... etc ...
> > }
> >
> > Now you'll always have either a pass or fail based on the gcore being
> > generated.
>
> Good idea. I did that for aarch64-gcs-core.exp.
>
> > There is also the helper proc `gcore_cmd_available`. I'd guess for
> > any
> > x86 target that supports SSP, gcore will be available, but in theory
> > you could consider using this to avoid a fail when gcore is not
> > available maybe?
>
> In the GCS core testcase, I moved the gcore tests after the OS corefile ones,
> with an
>
> if ![gcore_cmd_available] {
> return
> }
>
> in the middle, and after that I had to redo some GDB setup:
>
> clean_restart $binfile
>
> if ![runto $linespec] {
> return
> }
>
> But I agree it does make it more resilient/correct. Thanks for the suggestion.
>
> >> + }
> >> +
> >> + if {$core_generated} {
> >> + clean_restart $binfile
> >> +
> >> + with_test_prefix "OS corefile" {
> >> + # Read ssp value from saved output of the test program.
> >> + set out_id [open ${binfile}.out "r"]
> >> + set ssp_in_gcore [gets $out_id]
> >> +
> >> + close $out_id
> >> + check_core_file $core_filename $ssp_in_gcore
> >
> > I'd move the blank line after the 'close' personally.
>
> I also had that layout. Changed.
>
> --
> Thiago

Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928