Re: [PATCH 1/2] Improve core file path detection & put cores in output dir

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> After a testrun, I noticed that I have some kernel-produced cores for
> testcase programs, under build/gdb/testsuite/, which shouldn't be
> there:
>
>  $ ls -1 testsuite/core.*
>  testsuite/core.annota1.1274351.nelson.1656004407
>  testsuite/core.annota3.1288474.nelson.1656004414
>  testsuite/core.exitsignal.1240674.nelson.1656004391
>
> I have my core pattern setup like this:
>
>  $ cat /proc/sys/kernel/core_pattern
>  core.%e.%p.%h.%t
>
> That's:
>
>  %e: executable filename
>  %p: pid
>  %h: hostname
>  %t: UNIX time of dump
>
> so it's easy to tell which program produced the core from the core
> file name.
>
> From above, we can tell that the corresponding testcases are
> gdb.base/annota1.exp, gdb.base/annota3.exp and
> gdb.base/exitsignal.exp.
>
> At least gdb.base/annota1.exp and gdb.base/annota3.exp have code in
> them to delete the core file.  However, that isn't working for me,
> because said code only looks for cores named exactly either "core" or
> "core.PID", and my core_pattern doesn't match that.
>
> Another issue I noticed, is that I have not been running
> gdb.base/bigcore.exp, for a similar reason.  I get:
>
>   Program terminated with signal SIGABRT, Aborted.
>   The program no longer exists.
>   (gdb) PASS: gdb.base/bigcore.exp: signal SIGABRT
>   UNTESTED: gdb.base/bigcore.exp: can't generate a core file
>
> But I actually have a core file under the testcase's output dir:
>
>  $ find . -name "core.*"
>  ./testsuite/outputs/gdb.base/bigcore/core.bigcore.2306705.nelson.1656005213
>  $
>
> This commit fixes these things, by adding find_core_file routine that
> searches core files in a way that works with my pattern as well.  This
> then also adds a convenience remove_core routine as a wrapper around
> find_core_file that removes the found core file.
>
> In addition, it changes some testcases that expect to have their
> program dump core, to switch the inferior's cwd to the testcase's
> output dir, so that the core is dumped there instead of in
> build/gdb/testsuite/.  Some testcases were already doing that, but not
> all.  The idea is that any core file dumped in build/gdb/testsuite/ is
> an unexpected core file.  The next patch will add a count of such
> unexpected core files to gdb.sum.
>
> Another change is that the directory changing is now done with "set
> cwd" instead of with "cd".  "set cwd" only affects the inferior cwd,
> while "cd" affects GDB's cwd too.  By using "set cwd" instead of "cd",
> if GDB dumps core in these testcases, the GDB core dump will still end
> up in build/gdb/testsuite/, and can thus be detected as an unexpected
> core.
>
> Change-Id: I45068f21ffd4814350aaa8a3cc65cad5e3107607
> ---
>  gdb/testsuite/gdb.base/annota1.exp    | 18 +++---
>  gdb/testsuite/gdb.base/annota3.exp    | 18 +++---
>  gdb/testsuite/gdb.base/bigcore.exp    | 38 +++---------
>  gdb/testsuite/gdb.base/exitsignal.exp | 11 +++-
>  gdb/testsuite/lib/gdb.exp             | 86 ++++++++++++++++++++++++++-
>  5 files changed, 117 insertions(+), 54 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
> index ac6ad6478f4..42a261e5614 100644
> --- a/gdb/testsuite/gdb.base/annota1.exp
> +++ b/gdb/testsuite/gdb.base/annota1.exp
> @@ -385,6 +385,12 @@ gdb_test_multiple "display value" "set up display" {
>      }
>  }
>  
> +# Get the core into the output directory.
> +if {![is_remote host]} {
> +    gdb_test -prompt "$gdb_prompt$" \
> +	"set cwd [file dirname $binfile]" "" \
> +	"set inferior cwd to test directory"
> +}
>  
>  # should ask query. Test annotate-query.
>  # we don't care about anything else here, only the query.
> @@ -479,17 +485,7 @@ if [target_info exists gdb,nosignals] {
>  }
>  
>  # Check for production of a core file and remove it!
> -
> -set test "cleanup core file"
> -if { [remote_file host exists core] } {
> -    remote_file host delete core
> -    pass "$test (removed)"
> -} elseif { $pid != -1 && [remote_file host exists core.$pid] } {
> -    remote_file host delete core.$pid
> -    pass "$test (removed)"
> -} else {
> -    pass "$test (not dumped)"
> -}
> +remove_core $pid
>  
>  proc thread_test {} {
>      global subdir srcdir testfile srcfile binfile
> diff --git a/gdb/testsuite/gdb.base/annota3.exp b/gdb/testsuite/gdb.base/annota3.exp
> index b747e03718c..4fae6f066ab 100644
> --- a/gdb/testsuite/gdb.base/annota3.exp
> +++ b/gdb/testsuite/gdb.base/annota3.exp
> @@ -273,6 +273,12 @@ gdb_expect_list "set up display" "$gdb_prompt$" {
>      "1: value = 7\r\n"
>  }
>  
> +# Get the core into the output directory.
> +if {![is_remote host]} {
> +    gdb_test -prompt "$gdb_prompt$" \
> +	"set cwd [file dirname $binfile]" "" \
> +	"set inferior cwd to test directory"
> +}
>  
>  # should ask query. Test annotate-query.
>  # we don't care about anything else here, only the query.
> @@ -379,17 +385,7 @@ if [target_info exists gdb,nosignals] {
>  
>  
>  # Check for production of a core file and remove it!
> -
> -set test "cleanup core file"
> -if { [remote_file host exists core] } {
> -    remote_file host delete core
> -    pass "$test (removed)"
> -} elseif { $pid != -1 && [remote_file host exists core.$pid] } {
> -    remote_file host delete core.$pid
> -    pass "$test (removed)"
> -} else {
> -    pass "$test (not dumped)"
> -}
> +remove_core $pid
>  
>  # restore the original prompt for the rest of the testsuite
>  
> diff --git a/gdb/testsuite/gdb.base/bigcore.exp b/gdb/testsuite/gdb.base/bigcore.exp
> index a03a30eca32..13b70f7dd5d 100644
> --- a/gdb/testsuite/gdb.base/bigcore.exp
> +++ b/gdb/testsuite/gdb.base/bigcore.exp
> @@ -53,10 +53,7 @@ gdb_test_no_output "set print sevenbit-strings"
>  gdb_test_no_output "set width 0"
>  
>  # Get the core into the output directory.
> -if {![is_remote host]} {
> -    gdb_test "cd [file dirname $corefile]" "Working directory .*" \
> -	"cd to test directory"
> -}
> +set_inferior_cwd_to_output_dir
>  
>  if ![runto_main] then {
>      return 0
> @@ -116,17 +113,7 @@ gdb_test_no_output "set \$bytes_allocated = bytes_allocated" "save heap size"
>  # May 2003) create cores named "core.PID".
>  
>  # Save the process ID.  Some systems dump the core into core.PID.
> -set test "grab pid"
> -gdb_test_multiple "info program" $test {
> -    -re "child process (\[0-9\]+).*$gdb_prompt $" {
> -	set inferior_pid $expect_out(1,string)
> -	pass $test
> -    }
> -    -re "$gdb_prompt $" {
> -	set inferior_pid unknown
> -	pass $test
> -    }
> -}
> +set inferior_pid [get_inferior_pid]
>  
>  # Dump core using SIGABRT
>  set oldtimeout $timeout
> @@ -134,18 +121,11 @@ set timeout 600
>  gdb_test "signal SIGABRT" "Program terminated with signal SIGABRT, .*"
>  set timeout $oldtimeout
>  
> -# Find the corefile
> -set file ""
> -foreach pat [list core.${inferior_pid} ${testfile}.core core] {
> -    set names [glob -nocomplain [standard_output_file $pat]]
> -    if {[llength $names] == 1} {
> -	set file [lindex $names 0]
> -	remote_exec build "mv $file $corefile"
> -	break
> -    }
> -}
> -
> -if { $file == "" } {
> +# Find the corefile.
> +set file [find_core_file $inferior_pid]
> +if { $file != "" } {
> +    remote_exec build "mv $file $corefile"
> +} else {
>      untested "can't generate a core file"
>      return 0
>  }
> @@ -186,9 +166,7 @@ if {! $core_ok} {
>  # Now load up that core file
>  
>  set test "load corefile"
> -# We use [file tail] because gdb is still "cd"d to the
> -# output directory.
> -gdb_test_multiple "core [file tail $corefile]" "$test" {
> +gdb_test_multiple "core $corefile" "$test" {
>      -re "A program is being debugged already.  Kill it. .y or n. " {
>  	send_gdb "y\n"
>  	exp_continue
> diff --git a/gdb/testsuite/gdb.base/exitsignal.exp b/gdb/testsuite/gdb.base/exitsignal.exp
> index 27eb6c5d263..b139bac1be4 100644
> --- a/gdb/testsuite/gdb.base/exitsignal.exp
> +++ b/gdb/testsuite/gdb.base/exitsignal.exp
> @@ -32,11 +32,17 @@ if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
>      return -1
>  }
>  
> -# Run to main
> +# Run to main.  But, before, change cwd to get the core into the
> +# output directory.
> +set_inferior_cwd_to_output_dir
> +
>  if { ![runto_main] } {
>      return -1
>  }
>  
> +# Get the inferior's PID for later.
> +set pid [get_inferior_pid]
> +
>  # Print $_exitsignal.  It should be void now, because nothing
>  # happened.
>  gdb_test "print \$_exitsignal" " = void" \
> @@ -53,6 +59,9 @@ gdb_test "continue" "Program received signal SIGSEGV.*" "trigger SIGSEGV"
>  gdb_test "continue" "Program terminated with signal SIGSEGV.*" \
>      "program terminated with SIGSEGV"
>  
> +# We don't need the core file, remove it.
> +remove_core $pid
> +
>  # Now, print $_exitsignal again.  It should be 11 (SIGSEGV).
>  gdb_test "print \$_exitsignal" " = 11" \
>      "\$_exitsignal is 11 (SIGSEGV) after SIGSEGV."
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 4bc5f4f144c..f3fbb8a6391 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -7368,6 +7368,88 @@ if {[info exists GDB_PARALLEL]} {
>      }
>  }
>  
> +# Set the inferior's cwd to the output directory, in order to have it
> +# dump core there.  This must be called before the inferior is
> +# started.
> +
> +proc set_inferior_cwd_to_output_dir {} {
> +    # Note this sets the inferior's cwd ("set cwd"), not GDB's ("cd").
> +    # If GDB crashes, we want its core dump in gdb/testsuite/, not in
> +    # the testcase's dir, so we can detect the unexpected core at the
> +    # end of the test run.
> +    if {![is_remote host]} {
> +	set output_dir [standard_output_file ""]
> +	gdb_test_no_output "set cwd $output_dir" \
> +	    "set inferior cwd to test directory"
> +    }
> +}
> +
> +# Get the inferior's PID.
> +
> +proc get_inferior_pid {} {
> +    set pid -1
> +    gdb_test_multiple "inferior" "get inferior pid" {
> +	-re "process (\[0-9\]*).*$::gdb_prompt $" {
> +	    set pid $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }
> +    return $pid
> +}
> +
> +# Find the kernel-produced core file dumped for the current testfile
> +# program.  PID was the inferior's pid, saved before the inferior
> +# exited with a signal, or -1 if not known.  If not on a remote host,
> +# this assumes the core was generated in the output directory.
> +# Returns the name of the core dump, or empty string is not found.
> +
> +proc find_core_file {pid} {
> +    # For non-remote hosts, since cores are assumed to be in the
> +    # output dir, which we control, we use a laxer "core.*" glob.  For
> +    # remote hosts, as we don't know whether the dir is being reused
> +    # for parallel runs, we use stricter names with no globs.  It is
> +    # not clear whether this is really important, but it preserves
> +    # status quo ante.
> +    set files {}
> +    if {![is_remote host]} {
> +	lappend files core.*
> +    } elseif {$pid != -1} {
> +	lappend files core.$pid
> +    }
> +    lappend files [list ${::testfile}.core core]
> +
> +    foreach file $files {
> +	if {![is_remote host]} {
> +	    set names [glob -nocomplain [standard_output_file $file]]
> +	    if {[llength $names] == 1} {
> +		return [lindex $names 0]
> +	    }
> +	} else {
> +	    if {[remote_file host exists $file]} {
> +		return $file
> +	    }
> +	}
> +    }
> +    return ""
> +}
> +
> +# Check for production of a core file and remove it.  PID is the
> +# inferior's pid or -1 if not known.  TEST is the test's message.
> +
> +proc remove_core {pid {test ""}} {
> +    if {$test == ""} {
> +	set test "cleanup core file"
> +    }
> +
> +    set file [find_core_file $pid]
> +    if {$file != ""} {
> +	remote_file host delete $file
> +	pass "$test (removed)"
> +    } else {
> +	pass "$test (not dumped)"

I wonder if 'not found' would be a more acurate description for this
last pass?  With a particular core file pattern it might be that there
is a core file, we just can't find it, right?

Otherwise, this looks good.

Thanks,
Andrew


> +    }
> +}
> +
>  proc core_find {binfile {deletefiles {}} {arg ""}} {
>      global objdir subdir
>  
> @@ -7398,7 +7480,9 @@ proc core_find {binfile {deletefiles {}} {arg ""}} {
>  	    set found 1
>  	}
>      }
> -    # Check for "core.PID".
> +    # Check for "core.PID", "core.EXEC.PID.HOST.TIME", etc.  It's fine
> +    # to use a glob here as we're looking inside a directory we
> +    # created.  Also, this procedure only works on non-remote hosts.
>      if { $found == 0 } {
>  	set names [glob -nocomplain -directory $coredir core.*]
>  	if {[llength $names] == 1} {
> -- 
> 2.36.0