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

Schimpe, Christina
2025-08-05T19:50:17Z

Hi Thiago,

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Tuesday, August 5, 2025 6:30 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org;
> luis.machado@arm.com
> Subject: Re: [PATCH v5 07/12] gdb: amd64 linux coredump support with
> shadow stack.
>
> Hello,
>
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
>
> >> > >> +
> >> > >> + # 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 ?
>
> Interesting point. Perhaps a FAIL Isn't the best result to report in this case, but
> IMHO it would be worth reporting an UNTESTED or perhaps UNSUPPORTED
> result rather than silently returning.
>
> I don't have a strong opinion on this matter though.

I think UNTESTED should be good.
But in GDB we're are actually not consistent with UNTESTED or UNSUPPORTED I think.
In my opinion UNTESTED means that there is something not fine with the setup and one
should investigate, but UNSUPPORTED means it's fine that the test is skipped.

In any case, once we decided for one option it would be good if we'd update the other
corefile tests, too.

>
> >> > > 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?
>
> No, at least in my version of the test the "continue until crash" part is only
> needed once, for the gcore test. The OS corefile test doesn't need it.
>
> > 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
>
> Come to think of it, I think an untested/unsupported would be good here as
> well, if I'm going to be consistent in my opinion.

UNSUPPORTED here then I think, if my above understanding of UNTESTED/UNSUPPORTED is correct.

> > }
> >
> > 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
> > }
> > }
> > ~~~
>
> For reference, my test is here:
>
> https://gitlab.com/bauermann/binutils-gdb/-/blob/guarded-control-stack-
> patches/gdb/testsuite/gdb.arch/aarch64-gcs-core.exp
>
> --
> Thiago

Will check thanks!

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