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

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

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.

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

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