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

["Schimpe, Christina" <christina.schimpe@intel.com>]

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