[PATCH 0/2] Include count of unexpected core files in gdb.sum summary

[PATCH 0/2] Include count of unexpected core files in gdb.sum summary

[Pedro Alves]

It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
during teardown, and thus such a crash won't be noticed by looking at
the gdb.sum file at all.  This series aims at improving that, by
including a new "unexpected core files" line in the gdb.sum summary.

For example, here's what I get on x86-64 Ubuntu 20.04, with this
series:

		 === gdb Summary ===

 # of unexpected core files      12          << new info
 # of expected passes            107557
 # of unexpected failures        35
 # of expected failures          77
 # of unknown successes          2
 # of known failures             114
 # of untested testcases         31
 # of unsupported tests          139

That is done by patch #2.

Patch #1 fixes some testcases to make sure they dump their cores under
their output dir instead of in build/gdb/testsuite/, as those cores
are not unexpected cores, and makes the testcases be able to find
their kernel-generated cores when the core file names aren't exactly
"gdb" or "gdb.PID".

Pedro Alves (2):
  Improve core file path detection & put cores in output dir
  Include count of unexpected core files in gdb.sum summary

 gdb/testsuite/Makefile.in                   |  9 ++-
 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/dg-add-core-file-count.sh | 40 ++++++++++
 gdb/testsuite/lib/gdb.exp                   | 86 ++++++++++++++++++++-
 7 files changed, 165 insertions(+), 55 deletions(-)
 create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh


base-commit: c86acd3f180419c3d9825170492363fe2322fa8d
-- 
2.36.0

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

[Pedro Alves]

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)"
+    }
+}
+
 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

[PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[Pedro Alves]

If GDB, GDBserver, a testcase program, Valgrind, etc. unexpectedly
crash while running the GDB testsuite, and you've setup your machine
such that core files are dumped in the current directory instead of
being shoved somewhere by abrt, apport, or similar (as you should for
proper GDB testing), you'll end up with an unexpected core file in the
$build/gdb/testsuite/ directory.

It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
during teardown, and thus such a crash won't be noticed by looking at
the gdb.sum file at all.  This commit aims at improving that, by
including a new "unexpected core files" line in the testrun summary.

For example, here's what I get on x86-64 Ubuntu 20.04, with this
patch:

		 === gdb Summary ===

 # of unexpected core files      12          << new info
 # of expected passes            107557
 # of unexpected failures        35
 # of expected failures          77
 # of unknown successes          2
 # of known failures             114
 # of untested testcases         31
 # of unsupported tests          139

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

and so I get these core files:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.216191.nelson.1656002431
 testsuite/core.connect-with-no.217729.nelson.1656002431
 testsuite/core.gdb.194247.nelson.1656002423
 testsuite/core.gdb.226014.nelson.1656002435
 testsuite/core.gdb.232078.nelson.1656002438
 testsuite/core.gdb.352268.nelson.1656002441
 testsuite/core.gdb.4152093.nelson.1656002337
 testsuite/core.gdb.4154515.nelson.1656002338
 testsuite/core.gdb.4156668.nelson.1656002339
 testsuite/core.gdb.4158871.nelson.1656002341
 testsuite/core.gdb.468495.nelson.1656002444
 testsuite/core.vgdb.4192247.nelson.1656002366

where we can see that GDB crashed a number of times, but also
Valgrind's vgdb, and a couple testcase programs.  Neither of which is
good.

If your core_pattern is just "core" (but why??), then I guess that you
may end up with just a single core file in testsuite/.  Still, that is
one core file too many.

Above, we see a couple cores for "connect-with-no", which are the
result of gdb.server/connect-with-no-symbol-file.exp.  This is a case
mentioned above -- while the program crashed, that happens during
testcase teardown, and it goes unnoticed (without this commit) by
gdb.sum results.  Vis:

 $ make check TESTS="gdb.server/connect-with-no-symbol-file.exp"
 ...
		 === gdb Summary ===

 # of unexpected core files      2
 # of expected passes            8

 ...
 $

The tests fully passed, but still the testcase program crashed
somehow:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.941561.nelson.1656003317
 testsuite/core.connect-with-no.941682.nelson.1656003317

Against --target_board=native-extended-gdbserver it's even worse.  I
get:

 # of unexpected core files      26

and note that when GDBserver hits an assertion failure, it exits with
error, instead of crashing with SIGABRT.  I think that should be
changed, at least on development builds, but that would be for another
patch.  After such patch, I suspect the number of unexpected cores
will be higher, as there are likely teardown GDBserver assertions that
we're not noticing.

I decided to put this new info in the "gdb Summary" section, as that's
a place people already are used to looking at, either when looking at
the tail of gdb.sum, or when diffing gdb.sum files, and we've already
extended this section before, to include the count of DUPLICATE and
PATH problems, so there's precedent.

Implementation-wise, the new line is appended after DejaGnu is
finished, with a shell script that is invoked by the Makefile.  It is
done this way so that serial and parallel testing work the same way.
My initial cut at an implementation was in TCL, straight in
testsuite/lib/check-test-names.exp, where DUPLICATES and PATH are
handled, like so:

 @@ -148,6 +159,10 @@ namespace eval ::CheckTestNames {
	     $counts(paths,$which)
	 maybe_show_count "# of duplicate test names\t" \
	     $counts(duplicates,$which)
 +
 +       set cores [glob -nocomplain -directory $::objdir core*]
 +       maybe_show_count "# of unexpected core files\t" \
 +           [llength $cores]
      }

But that would only work for serial testing, as in parallel testing,
the final gdb.sum is generated by aggregating the results of all the
indididual gdb.sum files, and dg-extract-results.sh doesn't know about
our new summary line.  And I don't think that dg-extract-results.sh
should be taught about it, since the count of core files is not
something that we want to count many times, once per testcase, and
then add up the subcounts at the end.  Every time we count the core
files, we're already counting the final count.

I considered using the Tcl implementation in serial mode, and the
script approach for parallel testing, but that has the obvious
downside of implementing and maintaining the same thing twice.  In the
end, I settled on the script approach for serial mode too, which
requires making the "check-single" rule print the tail end of the
gdb.sum file, with a side effect being that if you look at the
terminal after a run (instead of at the gdb.sum file), you'll see the
"gdb Summary" section twice, once without the unexpected core lines
printed, and then another with.  IMO, this isn't an issue; when
testing in parallel mode, if you look at the terminal after "make -jN
check", you'll also see multiple "gdb Summary" sections printed.

Change-Id: I190b8d41856d49ad143854b6e3e6ccd7caa04491
---
 gdb/testsuite/Makefile.in                   |  9 ++++-
 gdb/testsuite/lib/dg-add-core-file-count.sh | 40 +++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 790b9e022cc..6a020e05d88 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -208,7 +208,12 @@ check-gdb.%:
 	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
+	-rm -f core*
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
+	result=$$?; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
+	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
+	exit $$result
 
 check-single-racy:
 	-rm -rf cache racy_outputs temp
@@ -231,6 +236,7 @@ check-single-racy:
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
 
 check-parallel:
+	-rm -f core*
 	-rm -rf cache outputs temp
 	$(MAKE) -k do-check-parallel; \
 	result=$$?; \
@@ -238,6 +244,7 @@ check-parallel:
 	  `find outputs -name gdb.sum -print` > gdb.sum; \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
 	  `find outputs -name gdb.log -print` > gdb.log; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
 	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
 	exit $$result
 
diff --git a/gdb/testsuite/lib/dg-add-core-file-count.sh b/gdb/testsuite/lib/dg-add-core-file-count.sh
new file mode 100755
index 00000000000..0e7903c2490
--- /dev/null
+++ b/gdb/testsuite/lib/dg-add-core-file-count.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Count number of core files in the current directory and if non-zero,
+# add a line to the gdb.sum file.  This scripts assumes it is run from
+# the build/gdb/testsuite/ directory.  It is normally invoked by the
+# Makefile.
+
+# Count core files portably, using POSIX compliant shell, avoiding ls,
+# find, wc, etc.  Spawning a subshell isn't strictly needed, but it's
+# clearer.
+cores=$(set -- core*; [ $# -eq 1 -a ! -e "$1" ] && shift; echo $#)
+
+# If no cores found, then don't add our summary line.
+if [ "$cores" -eq "0" ]; then
+    exit
+fi
+
+# Add our line to the summary line.
+sed "/=== gdb Summary ===/{
+n
+a\
+# of unexpected core files	$cores
+}" -i gdb.sum
-- 
2.36.0

Re: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[Pedro Alves]

On 2022-06-23 19:30, Pedro Alves wrote:

> +# Add our line to the summary line.
> +sed "/=== gdb Summary ===/{
> +n
> +a\
> +# of unexpected core files	$cores
> +}" -i gdb.sum

I knew this -i wasn't portable as is, but then forgot to fix it before posting.  I now tried
the script on a FreeBSD machine on the gcc compile farm, and tweaked this so it works with BSD
seds too.  That sed invocation now looks like:

 # Add our line to the summary line.
 sed -i'' -e "/=== gdb Summary ===/{
 n
 a\\
 # of unexpected core files     $cores
 }" gdb.sum

If -i doesn't work elsewhere, we can just do:

 sed .... < gdb.sum > gdb.sum.tmp
 mv gdb.sum.tmp gdb.sum

Here's the updated patch.

From 48d48ca9ddeaee581b229a13ba40b3dd5534568e Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 22 Jun 2022 20:33:01 +0100
Subject: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

If GDB, GDBserver, a testcase program, Valgrind, etc. unexpectedly
crash while running the GDB testsuite, and you've setup your machine
such that core files are dumped in the current directory instead of
being shoved somewhere by abrt, apport, or similar (as you should for
proper GDB testing), you'll end up with an unexpected core file in the
$build/gdb/testsuite/ directory.

It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
during teardown, and thus such a crash won't be noticed by looking at
the gdb.sum file at all.  This commit aims at improving that, by
including a new "unexpected core files" line in the testrun summary.

For example, here's what I get on x86-64 Ubuntu 20.04, with this
patch:

		 === gdb Summary ===

 # of unexpected core files      12          << new info
 # of expected passes            107557
 # of unexpected failures        35
 # of expected failures          77
 # of unknown successes          2
 # of known failures             114
 # of untested testcases         31
 # of unsupported tests          139

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

and so I get these core files:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.216191.nelson.1656002431
 testsuite/core.connect-with-no.217729.nelson.1656002431
 testsuite/core.gdb.194247.nelson.1656002423
 testsuite/core.gdb.226014.nelson.1656002435
 testsuite/core.gdb.232078.nelson.1656002438
 testsuite/core.gdb.352268.nelson.1656002441
 testsuite/core.gdb.4152093.nelson.1656002337
 testsuite/core.gdb.4154515.nelson.1656002338
 testsuite/core.gdb.4156668.nelson.1656002339
 testsuite/core.gdb.4158871.nelson.1656002341
 testsuite/core.gdb.468495.nelson.1656002444
 testsuite/core.vgdb.4192247.nelson.1656002366

where we can see that GDB crashed a number of times, but also
Valgrind's vgdb, and a couple testcase programs.  Neither of which is
good.

If your core_pattern is just "core" (but why??), then I guess that you
may end up with just a single core file in testsuite/.  Still, that is
one core file too many.

Above, we see a couple cores for "connect-with-no", which are the
result of gdb.server/connect-with-no-symbol-file.exp.  This is a case
mentioned above -- while the program crashed, that happens during
testcase teardown, and it goes unnoticed (without this commit) by
gdb.sum results.  Vis:

 $ make check TESTS="gdb.server/connect-with-no-symbol-file.exp"
 ...
		 === gdb Summary ===

 # of unexpected core files      2
 # of expected passes            8

 ...
 $

The tests fully passed, but still the testcase program crashed
somehow:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.941561.nelson.1656003317
 testsuite/core.connect-with-no.941682.nelson.1656003317

Against --target_board=native-extended-gdbserver it's even worse.  I
get:

 # of unexpected core files      26

and note that when GDBserver hits an assertion failure, it exits with
error, instead of crashing with SIGABRT.  I think that should be
changed, at least on development builds, but that would be for another
patch.  After such patch, I suspect the number of unexpected cores
will be higher, as there are likely teardown GDBserver assertions that
we're not noticing.

I decided to put this new info in the "gdb Summary" section, as that's
a place people already are used to looking at, either when looking at
the tail of gdb.sum, or when diffing gdb.sum files, and we've already
extended this section before, to include the count of DUPLICATE and
PATH problems, so there's precedent.

Implementation-wise, the new line is appended after DejaGnu is
finished, with a shell script that is invoked by the Makefile.  It is
done this way so that serial and parallel testing work the same way.
My initial cut at an implementation was in TCL, straight in
testsuite/lib/check-test-names.exp, where DUPLICATES and PATH are
handled, like so:

 @@ -148,6 +159,10 @@ namespace eval ::CheckTestNames {
	     $counts(paths,$which)
	 maybe_show_count "# of duplicate test names\t" \
	     $counts(duplicates,$which)
 +
 +       set cores [glob -nocomplain -directory $::objdir core*]
 +       maybe_show_count "# of unexpected core files\t" \
 +           [llength $cores]
      }

But that would only work for serial testing, as in parallel testing,
the final gdb.sum is generated by aggregating the results of all the
individual gdb.sum files, and dg-extract-results.sh doesn't know about
our new summary line.  And I don't think that dg-extract-results.sh
should be taught about it, since the count of core files is not
something that we want to count many times, once per testcase, and
then add up the subcounts at the end.  Every time we count the core
files, we're already counting the final count.

I considered using the Tcl implementation in serial mode, and the
script approach for parallel testing, but that has the obvious
downside of implementing and maintaining the same thing twice.  In the
end, I settled on the script approach for serial mode too, which
requires making the "check-single" rule print the tail end of the
gdb.sum file, with a side effect being that if you look at the
terminal after a run (instead of at the gdb.sum file), you'll see the
"gdb Summary" section twice, once without the unexpected core lines
printed, and then another with.  IMO, this isn't an issue; when
testing in parallel mode, if you look at the terminal after "make -jN
check", you'll also see multiple "gdb Summary" sections printed.

Change-Id: I190b8d41856d49ad143854b6e3e6ccd7caa04491
---
 gdb/testsuite/Makefile.in                   |  9 ++++-
 gdb/testsuite/lib/dg-add-core-file-count.sh | 40 +++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 790b9e022cc..6a020e05d88 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -208,7 +208,12 @@ check-gdb.%:
 	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
+	-rm -f core*
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
+	result=$$?; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
+	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
+	exit $$result
 
 check-single-racy:
 	-rm -rf cache racy_outputs temp
@@ -231,6 +236,7 @@ check-single-racy:
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
 
 check-parallel:
+	-rm -f core*
 	-rm -rf cache outputs temp
 	$(MAKE) -k do-check-parallel; \
 	result=$$?; \
@@ -238,6 +244,7 @@ check-parallel:
 	  `find outputs -name gdb.sum -print` > gdb.sum; \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
 	  `find outputs -name gdb.log -print` > gdb.log; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
 	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
 	exit $$result
 
diff --git a/gdb/testsuite/lib/dg-add-core-file-count.sh b/gdb/testsuite/lib/dg-add-core-file-count.sh
new file mode 100755
index 00000000000..abeb0b1c711
--- /dev/null
+++ b/gdb/testsuite/lib/dg-add-core-file-count.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Count number of core files in the current directory and if non-zero,
+# add a line to the gdb.sum file.  This scripts assumes it is run from
+# the build/gdb/testsuite/ directory.  It is normally invoked by the
+# Makefile.
+
+# Count core files portably, using POSIX compliant shell, avoiding ls,
+# find, wc, etc.  Spawning a subshell isn't strictly needed, but it's
+# clearer.
+cores=$(set -- core*; [ $# -eq 1 -a ! -e "$1" ] && shift; echo $#)
+
+# If no cores found, then don't add our summary line.
+if [ "$cores" -eq "0" ]; then
+    exit
+fi
+
+# Add our line to the summary line.
+sed -i'' -e "/=== gdb Summary ===/{
+n
+a\\
+# of unexpected core files	$cores
+}" gdb.sum
-- 
2.36.0

Re: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[John Baldwin]

On 6/23/22 12:12 PM, Pedro Alves wrote:
> On 2022-06-23 19:30, Pedro Alves wrote:
> 
>> +# Add our line to the summary line.
>> +sed "/=== gdb Summary ===/{
>> +n
>> +a\
>> +# of unexpected core files	$cores
>> +}" -i gdb.sum
> 
> I knew this -i wasn't portable as is, but then forgot to fix it before posting.  I now tried
> the script on a FreeBSD machine on the gcc compile farm, and tweaked this so it works with BSD
> seds too.  That sed invocation now looks like:
> 
>   # Add our line to the summary line.
>   sed -i'' -e "/=== gdb Summary ===/{
>   n
>   a\\
>   # of unexpected core files     $cores
>   }" gdb.sum
> 
> If -i doesn't work elsewhere, we can just do:
> 
>   sed .... < gdb.sum > gdb.sum.tmp
>   mv gdb.sum.tmp gdb.sum
> 
> Here's the updated patch.

Thanks for testing on FreeBSD. :)

I like the idea.  The only thing I see is that you might want to use a
pattern of '*core*' to look for cores instead of 'core*.'.  On BSD's at
least core files by default are named <program>.core (e.g. gdb.core) and
don't match the 'core*' glob.  If '*core*' has too many false positives
perhaps you can use 'core* *.core' as the patterns to look for?

-- 
John Baldwin

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

[Andrew Burgess]

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

Re: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2022-06-23 19:30, Pedro Alves wrote:
>
>> +# Add our line to the summary line.
>> +sed "/=== gdb Summary ===/{
>> +n
>> +a\
>> +# of unexpected core files	$cores
>> +}" -i gdb.sum
>
> I knew this -i wasn't portable as is, but then forgot to fix it before posting.  I now tried
> the script on a FreeBSD machine on the gcc compile farm, and tweaked this so it works with BSD
> seds too.  That sed invocation now looks like:
>
>  # Add our line to the summary line.
>  sed -i'' -e "/=== gdb Summary ===/{
>  n
>  a\\
>  # of unexpected core files     $cores
>  }" gdb.sum
>
> If -i doesn't work elsewhere, we can just do:
>
>  sed .... < gdb.sum > gdb.sum.tmp
>  mv gdb.sum.tmp gdb.sum
>
> Here's the updated patch.
>
> From 48d48ca9ddeaee581b229a13ba40b3dd5534568e Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 22 Jun 2022 20:33:01 +0100
> Subject: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary
>
> If GDB, GDBserver, a testcase program, Valgrind, etc. unexpectedly
> crash while running the GDB testsuite, and you've setup your machine
> such that core files are dumped in the current directory instead of
> being shoved somewhere by abrt, apport, or similar (as you should for
> proper GDB testing), you'll end up with an unexpected core file in the
> $build/gdb/testsuite/ directory.
>
> It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
> during teardown, and thus such a crash won't be noticed by looking at
> the gdb.sum file at all.  This commit aims at improving that, by
> including a new "unexpected core files" line in the testrun summary.
>
> For example, here's what I get on x86-64 Ubuntu 20.04, with this
> patch:
>
> 		 === gdb Summary ===
>
>  # of unexpected core files      12          << new info
>  # of expected passes            107557
>  # of unexpected failures        35
>  # of expected failures          77
>  # of unknown successes          2
>  # of known failures             114
>  # of untested testcases         31
>  # of unsupported tests          139
>
> 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
>
> and so I get these core files:
>
>  $ ls -1 testsuite/core.*
>  testsuite/core.connect-with-no.216191.nelson.1656002431
>  testsuite/core.connect-with-no.217729.nelson.1656002431
>  testsuite/core.gdb.194247.nelson.1656002423
>  testsuite/core.gdb.226014.nelson.1656002435
>  testsuite/core.gdb.232078.nelson.1656002438
>  testsuite/core.gdb.352268.nelson.1656002441
>  testsuite/core.gdb.4152093.nelson.1656002337
>  testsuite/core.gdb.4154515.nelson.1656002338
>  testsuite/core.gdb.4156668.nelson.1656002339
>  testsuite/core.gdb.4158871.nelson.1656002341
>  testsuite/core.gdb.468495.nelson.1656002444
>  testsuite/core.vgdb.4192247.nelson.1656002366
>
> where we can see that GDB crashed a number of times, but also
> Valgrind's vgdb, and a couple testcase programs.  Neither of which is
> good.
>
> If your core_pattern is just "core" (but why??), then I guess that you
> may end up with just a single core file in testsuite/.  Still, that is
> one core file too many.
>
> Above, we see a couple cores for "connect-with-no", which are the
> result of gdb.server/connect-with-no-symbol-file.exp.  This is a case
> mentioned above -- while the program crashed, that happens during
> testcase teardown, and it goes unnoticed (without this commit) by
> gdb.sum results.  Vis:
>
>  $ make check TESTS="gdb.server/connect-with-no-symbol-file.exp"
>  ...
> 		 === gdb Summary ===
>
>  # of unexpected core files      2
>  # of expected passes            8
>
>  ...
>  $
>
> The tests fully passed, but still the testcase program crashed
> somehow:
>
>  $ ls -1 testsuite/core.*
>  testsuite/core.connect-with-no.941561.nelson.1656003317
>  testsuite/core.connect-with-no.941682.nelson.1656003317
>
> Against --target_board=native-extended-gdbserver it's even worse.  I
> get:
>
>  # of unexpected core files      26
>
> and note that when GDBserver hits an assertion failure, it exits with
> error, instead of crashing with SIGABRT.  I think that should be
> changed, at least on development builds, but that would be for another
> patch.  After such patch, I suspect the number of unexpected cores
> will be higher, as there are likely teardown GDBserver assertions that
> we're not noticing.
>
> I decided to put this new info in the "gdb Summary" section, as that's
> a place people already are used to looking at, either when looking at
> the tail of gdb.sum, or when diffing gdb.sum files, and we've already
> extended this section before, to include the count of DUPLICATE and
> PATH problems, so there's precedent.
>
> Implementation-wise, the new line is appended after DejaGnu is
> finished, with a shell script that is invoked by the Makefile.  It is
> done this way so that serial and parallel testing work the same way.
> My initial cut at an implementation was in TCL, straight in
> testsuite/lib/check-test-names.exp, where DUPLICATES and PATH are
> handled, like so:
>
>  @@ -148,6 +159,10 @@ namespace eval ::CheckTestNames {
> 	     $counts(paths,$which)
> 	 maybe_show_count "# of duplicate test names\t" \
> 	     $counts(duplicates,$which)
>  +
>  +       set cores [glob -nocomplain -directory $::objdir core*]
>  +       maybe_show_count "# of unexpected core files\t" \
>  +           [llength $cores]
>       }
>
> But that would only work for serial testing, as in parallel testing,
> the final gdb.sum is generated by aggregating the results of all the
> individual gdb.sum files, and dg-extract-results.sh doesn't know about
> our new summary line.  And I don't think that dg-extract-results.sh
> should be taught about it, since the count of core files is not
> something that we want to count many times, once per testcase, and
> then add up the subcounts at the end.  Every time we count the core
> files, we're already counting the final count.
>
> I considered using the Tcl implementation in serial mode, and the
> script approach for parallel testing, but that has the obvious
> downside of implementing and maintaining the same thing twice.  In the
> end, I settled on the script approach for serial mode too, which
> requires making the "check-single" rule print the tail end of the
> gdb.sum file, with a side effect being that if you look at the
> terminal after a run (instead of at the gdb.sum file), you'll see the
> "gdb Summary" section twice, once without the unexpected core lines
> printed, and then another with.  IMO, this isn't an issue; when
> testing in parallel mode, if you look at the terminal after "make -jN
> check", you'll also see multiple "gdb Summary" sections printed.

LGTM.

Thanks,
Andrew

>
> Change-Id: I190b8d41856d49ad143854b6e3e6ccd7caa04491
> ---
>  gdb/testsuite/Makefile.in                   |  9 ++++-
>  gdb/testsuite/lib/dg-add-core-file-count.sh | 40 +++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh
>
> diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
> index 790b9e022cc..6a020e05d88 100644
> --- a/gdb/testsuite/Makefile.in
> +++ b/gdb/testsuite/Makefile.in
> @@ -208,7 +208,12 @@ check-gdb.%:
>  	$(MAKE) check TESTS="gdb.$*/*.exp"
>  
>  check-single:
> -	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
> +	-rm -f core*
> +	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
> +	result=$$?; \
> +	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
> +	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
> +	exit $$result
>  
>  check-single-racy:
>  	-rm -rf cache racy_outputs temp
> @@ -231,6 +236,7 @@ check-single-racy:
>  	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
>  
>  check-parallel:
> +	-rm -f core*
>  	-rm -rf cache outputs temp
>  	$(MAKE) -k do-check-parallel; \
>  	result=$$?; \
> @@ -238,6 +244,7 @@ check-parallel:
>  	  `find outputs -name gdb.sum -print` > gdb.sum; \
>  	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
>  	  `find outputs -name gdb.log -print` > gdb.log; \
> +	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
>  	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
>  	exit $$result
>  
> diff --git a/gdb/testsuite/lib/dg-add-core-file-count.sh b/gdb/testsuite/lib/dg-add-core-file-count.sh
> new file mode 100755
> index 00000000000..abeb0b1c711
> --- /dev/null
> +++ b/gdb/testsuite/lib/dg-add-core-file-count.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +# Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +# This file is part of GDB.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Count number of core files in the current directory and if non-zero,
> +# add a line to the gdb.sum file.  This scripts assumes it is run from
> +# the build/gdb/testsuite/ directory.  It is normally invoked by the
> +# Makefile.
> +
> +# Count core files portably, using POSIX compliant shell, avoiding ls,
> +# find, wc, etc.  Spawning a subshell isn't strictly needed, but it's
> +# clearer.
> +cores=$(set -- core*; [ $# -eq 1 -a ! -e "$1" ] && shift; echo $#)
> +
> +# If no cores found, then don't add our summary line.
> +if [ "$cores" -eq "0" ]; then
> +    exit
> +fi
> +
> +# Add our line to the summary line.
> +sed -i'' -e "/=== gdb Summary ===/{
> +n
> +a\\
> +# of unexpected core files	$cores
> +}" gdb.sum
> -- 
> 2.36.0

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

[Pedro Alves]

On 2022-06-24 10:50, Andrew Burgess wrote:

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

...

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

Yeah, I had just copied from that the annota testcases (see above).
I'll make that change before pushing this.  Thanks!

Re: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[Pedro Alves]

On 2022-06-23 23:09, John Baldwin wrote:
> On 6/23/22 12:12 PM, Pedro Alves wrote:
>> On 2022-06-23 19:30, Pedro Alves wrote:
>>
>>> +# Add our line to the summary line.
>>> +sed "/=== gdb Summary ===/{
>>> +n
>>> +a\
>>> +# of unexpected core files    $cores
>>> +}" -i gdb.sum
>>
>> I knew this -i wasn't portable as is, but then forgot to fix it before posting.  I now tried
>> the script on a FreeBSD machine on the gcc compile farm, and tweaked this so it works with BSD
>> seds too.  That sed invocation now looks like:
>>
>>   # Add our line to the summary line.
>>   sed -i'' -e "/=== gdb Summary ===/{
>>   n
>>   a\\
>>   # of unexpected core files     $cores
>>   }" gdb.sum
>>
>> If -i doesn't work elsewhere, we can just do:
>>
>>   sed .... < gdb.sum > gdb.sum.tmp
>>   mv gdb.sum.tmp gdb.sum
>>
>> Here's the updated patch.
> 
> Thanks for testing on FreeBSD. :)
> 
> I like the idea.  The only thing I see is that you might want to use a
> pattern of '*core*' to look for cores instead of 'core*.'.  On BSD's at
> least core files by default are named <program>.core (e.g. gdb.core) and
> don't match the 'core*' glob.  If '*core*' has too many false positives
> perhaps you can use 'core* *.core' as the patterns to look for?
> 

*core* currently doesn't match anything else, so I think that would be fine.

Here's the updated patch that I plan to merge.  I included a comment
about the lax pattern.

From fd9dee6e56d0f7e352014213f4221fbb46e973c4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 22 Jun 2022 20:33:01 +0100
Subject: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

If GDB, GDBserver, a testcase program, Valgrind, etc. unexpectedly
crash while running the GDB testsuite, and you've setup your machine
such that core files are dumped in the current directory instead of
being shoved somewhere by abrt, apport, or similar (as you should for
proper GDB testing), you'll end up with an unexpected core file in the
$build/gdb/testsuite/ directory.

It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
during teardown, and thus such a crash won't be noticed by looking at
the gdb.sum file at all.  This commit aims at improving that, by
including a new "unexpected core files" line in the testrun summary.

For example, here's what I get on x86-64 Ubuntu 20.04, with this
patch:

		 === gdb Summary ===

 # of unexpected core files      12          << new info
 # of expected passes            107557
 # of unexpected failures        35
 # of expected failures          77
 # of unknown successes          2
 # of known failures             114
 # of untested testcases         31
 # of unsupported tests          139

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

and so I get these core files:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.216191.nelson.1656002431
 testsuite/core.connect-with-no.217729.nelson.1656002431
 testsuite/core.gdb.194247.nelson.1656002423
 testsuite/core.gdb.226014.nelson.1656002435
 testsuite/core.gdb.232078.nelson.1656002438
 testsuite/core.gdb.352268.nelson.1656002441
 testsuite/core.gdb.4152093.nelson.1656002337
 testsuite/core.gdb.4154515.nelson.1656002338
 testsuite/core.gdb.4156668.nelson.1656002339
 testsuite/core.gdb.4158871.nelson.1656002341
 testsuite/core.gdb.468495.nelson.1656002444
 testsuite/core.vgdb.4192247.nelson.1656002366

where we can see that GDB crashed a number of times, but also
Valgrind's vgdb, and a couple testcase programs.  Neither of which is
good.

If your core_pattern is just "core" (but why??), then I guess that you
may end up with just a single core file in testsuite/.  Still, that is
one core file too many.

Above, we see a couple cores for "connect-with-no", which are the
result of gdb.server/connect-with-no-symbol-file.exp.  This is a case
mentioned above -- while the program crashed, that happens during
testcase teardown, and it goes unnoticed (without this commit) by
gdb.sum results.  Vis:

 $ make check TESTS="gdb.server/connect-with-no-symbol-file.exp"
 ...
		 === gdb Summary ===

 # of unexpected core files      2
 # of expected passes            8

 ...
 $

The tests fully passed, but still the testcase program crashed
somehow:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.941561.nelson.1656003317
 testsuite/core.connect-with-no.941682.nelson.1656003317

Against --target_board=native-extended-gdbserver it's even worse.  I
get:

 # of unexpected core files      26

and note that when GDBserver hits an assertion failure, it exits with
error, instead of crashing with SIGABRT.  I think that should be
changed, at least on development builds, but that would be for another
patch.  After such patch, I suspect the number of unexpected cores
will be higher, as there are likely teardown GDBserver assertions that
we're not noticing.

I decided to put this new info in the "gdb Summary" section, as that's
a place people already are used to looking at, either when looking at
the tail of gdb.sum, or when diffing gdb.sum files, and we've already
extended this section before, to include the count of DUPLICATE and
PATH problems, so there's precedent.

Implementation-wise, the new line is appended after DejaGnu is
finished, with a shell script that is invoked by the Makefile.  It is
done this way so that serial and parallel testing work the same way.
My initial cut at an implementation was in TCL, straight in
testsuite/lib/check-test-names.exp, where DUPLICATES and PATH are
handled, like so:

 @@ -148,6 +159,10 @@ namespace eval ::CheckTestNames {
	     $counts(paths,$which)
	 maybe_show_count "# of duplicate test names\t" \
	     $counts(duplicates,$which)
 +
 +       set cores [glob -nocomplain -directory $::objdir core*]
 +       maybe_show_count "# of unexpected core files\t" \
 +           [llength $cores]
      }

But that would only work for serial testing, as in parallel testing,
the final gdb.sum is generated by aggregating the results of all the
individual gdb.sum files, and dg-extract-results.sh doesn't know about
our new summary line.  And I don't think that dg-extract-results.sh
should be taught about it, since the count of core files is not
something that we want to count many times, once per testcase, and
then add up the subcounts at the end.  Every time we count the core
files, we're already counting the final count.

I considered using the Tcl implementation in serial mode, and the
script approach for parallel testing, but that has the obvious
downside of implementing and maintaining the same thing twice.  In the
end, I settled on the script approach for serial mode too, which
requires making the "check-single" rule print the tail end of the
gdb.sum file, with a side effect being that if you look at the
terminal after a run (instead of at the gdb.sum file), you'll see the
"gdb Summary" section twice, once without the unexpected core lines
printed, and then another with.  IMO, this isn't an issue; when
testing in parallel mode, if you look at the terminal after "make -jN
check", you'll also see multiple "gdb Summary" sections printed.

Change-Id: I190b8d41856d49ad143854b6e3e6ccd7caa04491
---
 gdb/testsuite/Makefile.in                   |  9 ++++-
 gdb/testsuite/lib/dg-add-core-file-count.sh | 41 +++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 790b9e022cc..6a020e05d88 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -208,7 +208,12 @@ check-gdb.%:
 	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
+	-rm -f core*
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
+	result=$$?; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
+	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
+	exit $$result
 
 check-single-racy:
 	-rm -rf cache racy_outputs temp
@@ -231,6 +236,7 @@ check-single-racy:
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
 
 check-parallel:
+	-rm -f core*
 	-rm -rf cache outputs temp
 	$(MAKE) -k do-check-parallel; \
 	result=$$?; \
@@ -238,6 +244,7 @@ check-parallel:
 	  `find outputs -name gdb.sum -print` > gdb.sum; \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
 	  `find outputs -name gdb.log -print` > gdb.log; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
 	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
 	exit $$result
 
diff --git a/gdb/testsuite/lib/dg-add-core-file-count.sh b/gdb/testsuite/lib/dg-add-core-file-count.sh
new file mode 100755
index 00000000000..702be062e86
--- /dev/null
+++ b/gdb/testsuite/lib/dg-add-core-file-count.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Count number of core files in the current directory and if non-zero,
+# add a line to the gdb.sum file.  This scripts assumes it is run from
+# the build/gdb/testsuite/ directory.  It is normally invoked by the
+# Makefile.
+
+# Count core files portably, using POSIX compliant shell, avoiding ls,
+# find, wc, etc.  Spawning a subshell isn't strictly needed, but it's
+# clearer.  The "*core*" pattern is this lax in order to find all of
+# "core", "core.PID", "core.<program>.PID", "<program>.core", etc.
+cores=$(set -- *core*; [ $# -eq 1 -a ! -e "$1" ] && shift; echo $#)
+
+# If no cores found, then don't add our summary line.
+if [ "$cores" -eq "0" ]; then
+    exit
+fi
+
+# Add our line to the summary.
+sed -i'' -e "/=== gdb Summary ===/{
+n
+a\\
+# of unexpected core files	$cores
+}" gdb.sum
-- 
2.36.0

Re: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

[Pedro Alves]

On 2022-06-24 12:13, Pedro Alves wrote:
> On 2022-06-23 23:09, John Baldwin wrote:

>> Thanks for testing on FreeBSD. :)
>>
>> I like the idea.  The only thing I see is that you might want to use a
>> pattern of '*core*' to look for cores instead of 'core*.'.  On BSD's at
>> least core files by default are named <program>.core (e.g. gdb.core) and
>> don't match the 'core*' glob.  If '*core*' has too many false positives
>> perhaps you can use 'core* *.core' as the patterns to look for?
>>
> 
> *core* currently doesn't match anything else, so I think that would be fine.
> 
> Here's the updated patch that I plan to merge.  I included a comment
> about the lax pattern.

... and ... I forgot to update the "rm" commands in the Makefile.  Fixed in this version...

Nth time's the charm?

From 5a728f1d057efcc3022d7b38c9539682d79f6a52 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 22 Jun 2022 20:33:01 +0100
Subject: [PATCH 2/2] Include count of unexpected core files in gdb.sum summary

If GDB, GDBserver, a testcase program, Valgrind, etc. unexpectedly
crash while running the GDB testsuite, and you've setup your machine
such that core files are dumped in the current directory instead of
being shoved somewhere by abrt, apport, or similar (as you should for
proper GDB testing), you'll end up with an unexpected core file in the
$build/gdb/testsuite/ directory.

It can happen that GDB, GDBserver, etc. even crashes _after_ gdb_exit,
during teardown, and thus such a crash won't be noticed by looking at
the gdb.sum file at all.  This commit aims at improving that, by
including a new "unexpected core files" line in the testrun summary.

For example, here's what I get on x86-64 Ubuntu 20.04, with this
patch:

		 === gdb Summary ===

 # of unexpected core files      12          << new info
 # of expected passes            107557
 # of unexpected failures        35
 # of expected failures          77
 # of unknown successes          2
 # of known failures             114
 # of untested testcases         31
 # of unsupported tests          139

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

and so I get these core files:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.216191.nelson.1656002431
 testsuite/core.connect-with-no.217729.nelson.1656002431
 testsuite/core.gdb.194247.nelson.1656002423
 testsuite/core.gdb.226014.nelson.1656002435
 testsuite/core.gdb.232078.nelson.1656002438
 testsuite/core.gdb.352268.nelson.1656002441
 testsuite/core.gdb.4152093.nelson.1656002337
 testsuite/core.gdb.4154515.nelson.1656002338
 testsuite/core.gdb.4156668.nelson.1656002339
 testsuite/core.gdb.4158871.nelson.1656002341
 testsuite/core.gdb.468495.nelson.1656002444
 testsuite/core.vgdb.4192247.nelson.1656002366

where we can see that GDB crashed a number of times, but also
Valgrind's vgdb, and a couple testcase programs.  Neither of which is
good.

If your core_pattern is just "core" (but why??), then I guess that you
may end up with just a single core file in testsuite/.  Still, that is
one core file too many.

Above, we see a couple cores for "connect-with-no", which are the
result of gdb.server/connect-with-no-symbol-file.exp.  This is a case
mentioned above -- while the program crashed, that happens during
testcase teardown, and it goes unnoticed (without this commit) by
gdb.sum results.  Vis:

 $ make check TESTS="gdb.server/connect-with-no-symbol-file.exp"
 ...
		 === gdb Summary ===

 # of unexpected core files      2
 # of expected passes            8

 ...
 $

The tests fully passed, but still the testcase program crashed
somehow:

 $ ls -1 testsuite/core.*
 testsuite/core.connect-with-no.941561.nelson.1656003317
 testsuite/core.connect-with-no.941682.nelson.1656003317

Against --target_board=native-extended-gdbserver it's even worse.  I
get:

 # of unexpected core files      26

and note that when GDBserver hits an assertion failure, it exits with
error, instead of crashing with SIGABRT.  I think that should be
changed, at least on development builds, but that would be for another
patch.  After such patch, I suspect the number of unexpected cores
will be higher, as there are likely teardown GDBserver assertions that
we're not noticing.

I decided to put this new info in the "gdb Summary" section, as that's
a place people already are used to looking at, either when looking at
the tail of gdb.sum, or when diffing gdb.sum files, and we've already
extended this section before, to include the count of DUPLICATE and
PATH problems, so there's precedent.

Implementation-wise, the new line is appended after DejaGnu is
finished, with a shell script that is invoked by the Makefile.  It is
done this way so that serial and parallel testing work the same way.
My initial cut at an implementation was in TCL, straight in
testsuite/lib/check-test-names.exp, where DUPLICATES and PATH are
handled, like so:

 @@ -148,6 +159,10 @@ namespace eval ::CheckTestNames {
	     $counts(paths,$which)
	 maybe_show_count "# of duplicate test names\t" \
	     $counts(duplicates,$which)
 +
 +       set cores [glob -nocomplain -directory $::objdir core*]
 +       maybe_show_count "# of unexpected core files\t" \
 +           [llength $cores]
      }

But that would only work for serial testing, as in parallel testing,
the final gdb.sum is generated by aggregating the results of all the
individual gdb.sum files, and dg-extract-results.sh doesn't know about
our new summary line.  And I don't think that dg-extract-results.sh
should be taught about it, since the count of core files is not
something that we want to count many times, once per testcase, and
then add up the subcounts at the end.  Every time we count the core
files, we're already counting the final count.

I considered using the Tcl implementation in serial mode, and the
script approach for parallel testing, but that has the obvious
downside of implementing and maintaining the same thing twice.  In the
end, I settled on the script approach for serial mode too, which
requires making the "check-single" rule print the tail end of the
gdb.sum file, with a side effect being that if you look at the
terminal after a run (instead of at the gdb.sum file), you'll see the
"gdb Summary" section twice, once without the unexpected core lines
printed, and then another with.  IMO, this isn't an issue; when
testing in parallel mode, if you look at the terminal after "make -jN
check", you'll also see multiple "gdb Summary" sections printed.

Change-Id: I190b8d41856d49ad143854b6e3e6ccd7caa04491
---
 gdb/testsuite/Makefile.in                   |  9 ++++-
 gdb/testsuite/lib/dg-add-core-file-count.sh | 41 +++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100755 gdb/testsuite/lib/dg-add-core-file-count.sh

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 790b9e022cc..87ba522c9e0 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -208,7 +208,12 @@ check-gdb.%:
 	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
+	-rm -f *core*
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
+	result=$$?; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
+	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
+	exit $$result
 
 check-single-racy:
 	-rm -rf cache racy_outputs temp
@@ -231,6 +236,7 @@ check-single-racy:
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
 
 check-parallel:
+	-rm -f *core*
 	-rm -rf cache outputs temp
 	$(MAKE) -k do-check-parallel; \
 	result=$$?; \
@@ -238,6 +244,7 @@ check-parallel:
 	  `find outputs -name gdb.sum -print` > gdb.sum; \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
 	  `find outputs -name gdb.log -print` > gdb.log; \
+	$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
 	sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
 	exit $$result
 
diff --git a/gdb/testsuite/lib/dg-add-core-file-count.sh b/gdb/testsuite/lib/dg-add-core-file-count.sh
new file mode 100755
index 00000000000..702be062e86
--- /dev/null
+++ b/gdb/testsuite/lib/dg-add-core-file-count.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This file is part of GDB.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Count number of core files in the current directory and if non-zero,
+# add a line to the gdb.sum file.  This scripts assumes it is run from
+# the build/gdb/testsuite/ directory.  It is normally invoked by the
+# Makefile.
+
+# Count core files portably, using POSIX compliant shell, avoiding ls,
+# find, wc, etc.  Spawning a subshell isn't strictly needed, but it's
+# clearer.  The "*core*" pattern is this lax in order to find all of
+# "core", "core.PID", "core.<program>.PID", "<program>.core", etc.
+cores=$(set -- *core*; [ $# -eq 1 -a ! -e "$1" ] && shift; echo $#)
+
+# If no cores found, then don't add our summary line.
+if [ "$cores" -eq "0" ]; then
+    exit
+fi
+
+# Add our line to the summary.
+sed -i'' -e "/=== gdb Summary ===/{
+n
+a\\
+# of unexpected core files	$cores
+}" gdb.sum
-- 
2.36.0