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

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

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