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

Schimpe, Christina
2025-08-04T15:29:16Z

Hi Thiago and Andrew,

Now that I have tested this a bit I actually have some more comments on the test.

> -----Original Message-----
> From: Schimpe, Christina
> Sent: Wednesday, July 30, 2025 1:43 PM
> To: Thiago Jung Bauermann <thiago.bauermann@linaro.org>; Andrew
> Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org; luis.machado@arm.com
> Subject: RE: [PATCH v5 07/12] gdb: amd64 linux coredump support with
> shadow stack.
>
> 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.

If no OS corefile is found we will see a FAIL here.
The usual coredump testing doesn't fail in case the coredump file is not found.
So all gdb.base/corefile*.exp tests have sth. like:

set corefile [core_find $binfile {}]
if {$corefile == ""} {
return
}

This can happen in case corefiles are managed, for instance, by apport on ubuntu.
Do we want a different behaviour ?

> > > 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
> > }

Do you also do the "continue until crash part" again?
It would make sense to move it into a proc then:
~~~
# Continue until the expected crash at the return instruction.

proc continue_until_crash {} {
global hex decimal

# The line with the hex number is optional because it's printed by the
# test program, and doesn't appear in the expect buffer when testing a
# remote target.
gdb_test "continue" \
[...]
~~~

And my gcore testing now looks as follows:

~~~
if ![gcore_cmd_available] {
return
}

clean_restart $binfile

if ![runto $linespec] {
return
}

with_test_prefix "gcore corefile" {
continue_until_crash

set ssp_in_gcore [get_valueof "/x" "\$pl3_ssp" "*unknown*"]

# Generate the gcore core file.
set gcore_filename [standard_output_file "${testfile}.gcore"]
set gcore_generated [gdb_gcore_cmd "$gcore_filename" "generate gcore file"]

gdb_assert { $gcore_generated } "gcore corefile created"
if { $gcore_generated } {
clean_restart $binfile
check_core_file $gcore_filename $ssp_in_gcore
}
}
~~~

> > 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

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