[PATCH 0/4] Automatic detection of test name problems

[PATCH 0/4] Automatic detection of test name problems

[Andrew Burgess]

This started as fixing some test names in
gdb.btrace/multi-inferior.exp, but then I wanted to try and prevent
these problems creeping back in again.

So, this series extends our testing framework so that Dejagnu will now
produce two new result types, these are:

  # of paths in test names
  # of duplicate test names

So, the test gdb.btrace/multi-inferior.exp (without the fix in this
series) would produce results like this:

  		=== gdb Summary ===
  
  # of expected passes		10
  # of paths in test names	2

This indicates that two test names include either the build or source
path, which should really be resolved.

Alternatively, the test gdb.ada/fun_overload_menu.exp currently
produces results like this:

  		=== gdb Summary ===
  
  # of expected passes		8
  # of duplicate test names	2

Indicating that two test names are duplicated.

It is expected that users will treat these new results just like
FAILs, they should never increase these numbers when adding new tests.

It's possible for a test run to include both of the new test result
types.

I've updated the dg-extract-results scripts so that the new result
types are merged over when running the tests in parallel.  I know this
change needs to go through GCC, but I wanted to discuss these patches
here first, before posting the dg-extract-results changes to the gcc
list.

I'll merge the first patch in this series (the
gdb.btrace/multi-inferior.exp fix) relatively soon as obvious.

Feedback and thoughts welcome.

Thanks,
Andrew



--

Andrew Burgess (4):
  gdb/testsuite: Remove build paths from test names
  gdb/testsuite: Detect and warn if paths are used in test names
  gdb/testsuite: Detect and warn about duplicate test names
  contrib: Handle GDB specific test result types

 contrib/ChangeLog                           |   5 +
 contrib/dg-extract-results.py               |   4 +-
 contrib/dg-extract-results.sh               |  10 ++
 gdb/testsuite/ChangeLog                     |  17 +++
 gdb/testsuite/gdb.btrace/multi-inferior.exp |   6 +-
 gdb/testsuite/lib/check-test-names.exp      | 108 ++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                   |   1 +
 7 files changed, 148 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

-- 
2.25.3

[PATCH 1/4] gdb/testsuite: Remove build paths from test names

[Andrew Burgess]

Having paths in test names makes it harder to compare results from
different builds of GDB.

gdb/testsuite/ChangeLog:

	* gdb.btrace/multi-inferior.exp: Avoid paths in test names.
---
 gdb/testsuite/ChangeLog                     | 4 ++++
 gdb/testsuite/gdb.btrace/multi-inferior.exp | 6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp b/gdb/testsuite/gdb.btrace/multi-inferior.exp
index 87a99332e03..fdf889f5ed8 100644
--- a/gdb/testsuite/gdb.btrace/multi-inferior.exp
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
@@ -40,7 +40,8 @@ with_test_prefix "inferior 1" {
 }
 
 with_test_prefix "inferior 2" {
-    gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*"
+    gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*" \
+	"add second inferior"
     gdb_test "inferior 2" "Switching to inferior 2.*"
 
     if ![runto_main] {
@@ -59,7 +60,8 @@ with_test_prefix "inferior 1" {
 }
 
 with_test_prefix "inferior 3" {
-    gdb_test "add-inferior -exec ${binfile}" "Added inferior 3.*"
+    gdb_test "add-inferior -exec ${binfile}" "Added inferior 3.*" \
+	"add third inferior"
     gdb_test "inferior 3" "Switching to inferior 3.*"
 
     if ![runto_main] {
-- 
2.25.3

[PATCH 2/4] gdb/testsuite: Detect and warn if paths are used in test names

[Andrew Burgess]

A new library is introduced that hooks into the core of Dejagnu and
detects when a test's name includes either the source or build paths.
If any offending test names are detected then Dejagnu will print a
new result type, '# of paths in test names'.  Users should treat this
result type just like other bad results types, and aim not to increase
this number.

Currently for a local run on my machine, I don't see any offending
test names, but it is possible that different targets, or different
configurations, might currently be breaking the no paths rule.

In order to get this working I have needed to wrap two core Dejagnu
functions, log_summary, and reset_vars.  Relying on core functions
that are not part of any API is always going to be risky, given the
relatively slow rate of Dejagnu change this is probably OK for now,
and we can possibly upstream some changes to Dejagnu that would allow
this functionality to be supported in a more official way later on.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  A later commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp: Include check-test-names.exp library.
	* lib/check-test-names.exp: New file.
---
 gdb/testsuite/ChangeLog                |  5 ++
 gdb/testsuite/lib/check-test-names.exp | 79 ++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |  1 +
 3 files changed, 85 insertions(+)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
new file mode 100644
index 00000000000..6377ace3fc3
--- /dev/null
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -0,0 +1,79 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# 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/>.
+
+# This library provides some protection against the introduction of
+# tests that include either the source of build paths in the test
+# name.  When a test includes the path in its test name it is harder
+# to compare results between two runs of GDB from different trees.
+
+# Count the number of test names that contain either the source path,
+# or the build path.
+set paths_in_test_names 0
+
+# Check if MESSAGE contains either the source path or the build path.
+# This will result in test names that can't easily be compared between
+# different runs of GDB.
+#
+# Any offending paths in message cause PATHS_IN_TEST_NAMES to be
+# incremented.
+proc check_test_names { message } {
+    global srcdir objdir
+    global paths_in_test_names
+
+    foreach pattern [list $srcdir $objdir] {
+	if { [ regexp $pattern $message ] } {
+	    # Count each test just once.
+	    incr paths_in_test_names
+	    return
+	}
+    }
+}
+
+# Arrange for CHECK_TEST_NAMES to be called.
+set local_record_procs(pass) "check_test_names"
+set local_record_procs(fail) "check_test_names"
+set local_record_procs(xfail) "check_test_names"
+set local_record_procs(kfail) "check_test_names"
+set local_record_procs(xpass) "check_test_names"
+set local_record_procs(kpass) "check_test_names"
+set local_record_procs(unresolved) "check_test_names"
+set local_record_procs(untested) "check_test_names"
+set local_record_procs(unsupported) "check_test_names"
+
+# Wrapper around the global Dejagnu LOG_SUMMARY procedure.  Prints a
+# warning if any tests were found that contained either the source or
+# build paths.
+rename log_summary original_log_summary
+proc log_summary { args } {
+    global paths_in_test_names
+
+    # If ARGS is the empty list then we don't want to pass a single
+    # empty string as a parameter here.
+    eval "original_log_summary $args"
+
+    if { $paths_in_test_names > 0 } {
+	clone_output "# of paths in test names\t${paths_in_test_names}"
+    }
+}
+
+# Wrapper around the global Dejagnu RESET_VARS procedure, resets our
+# PATHS_IN_TEST_NAMES counter.
+rename reset_vars original_reset_vars
+proc reset_vars {} {
+    global paths_in_test_names
+
+    original_reset_vars
+    set paths_in_test_names 0
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8418c3d8753..a294b53f2c3 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -29,6 +29,7 @@ load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
 load_lib memory.exp
+load_lib check-test-names.exp
 
 global GDB
 
-- 
2.25.3

[PATCH 3/4] gdb/testsuite: Detect and warn about duplicate test names

[Andrew Burgess]

Spots when tests reuse the same test name and prints a new result type
'# of duplicate test names' in the result summary.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  A later commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/check-test-names.exp (all_test_names): New global.
	(duplicate_test_names_seen): New global.
	(check_test_names): Check for duplicate test names.
	(log_summary): Warn about duplicate test names.
	(reset_vars): Reset counters for duplicate test names.
---
 gdb/testsuite/ChangeLog                |  8 +++++++
 gdb/testsuite/lib/check-test-names.exp | 29 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
index 6377ace3fc3..85520191f27 100644
--- a/gdb/testsuite/lib/check-test-names.exp
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -22,6 +22,13 @@
 # or the build path.
 set paths_in_test_names 0
 
+# An associative array of all test names to the number of times each
+# name is seen.  Used to detect duplicate test names.
+array set all_test_names {}
+
+# A count of the number of duplicte test names seen.
+set duplicate_test_names_seen 0
+
 # Check if MESSAGE contains either the source path or the build path.
 # This will result in test names that can't easily be compared between
 # different runs of GDB.
@@ -31,6 +38,8 @@ set paths_in_test_names 0
 proc check_test_names { message } {
     global srcdir objdir
     global paths_in_test_names
+    global all_test_names
+    global duplicate_test_names_seen
 
     foreach pattern [list $srcdir $objdir] {
 	if { [ regexp $pattern $message ] } {
@@ -39,6 +48,16 @@ proc check_test_names { message } {
 	    return
 	}
     }
+
+    # Initialise a count, or increment the count for this test name.
+    if {![info exists all_test_names($message)]} {
+	set all_test_names($message) 0
+    } else {
+	if {$all_test_names($message) == 0} {
+	    incr duplicate_test_names_seen
+	}
+	incr all_test_names($message)
+    }
 }
 
 # Arrange for CHECK_TEST_NAMES to be called.
@@ -58,6 +77,7 @@ set local_record_procs(unsupported) "check_test_names"
 rename log_summary original_log_summary
 proc log_summary { args } {
     global paths_in_test_names
+    global duplicate_test_names_seen
 
     # If ARGS is the empty list then we don't want to pass a single
     # empty string as a parameter here.
@@ -66,6 +86,10 @@ proc log_summary { args } {
     if { $paths_in_test_names > 0 } {
 	clone_output "# of paths in test names\t${paths_in_test_names}"
     }
+
+    if { $duplicate_test_names_seen > 0 } {
+	clone_output "# of duplicate test names\t${duplicate_test_names_seen}"
+    }
 }
 
 # Wrapper around the global Dejagnu RESET_VARS procedure, resets our
@@ -73,7 +97,12 @@ proc log_summary { args } {
 rename reset_vars original_reset_vars
 proc reset_vars {} {
     global paths_in_test_names
+    global all_test_names
+    global duplicate_test_names_seen
 
     original_reset_vars
+
     set paths_in_test_names 0
+    unset all_test_names
+    set duplicate_test_names_seen 0
 }
-- 
2.25.3

[PATCH 4/4] contrib: Handle GDB specific test result types

[Andrew Burgess]

NOTE: I know that this patch needs to go through the GCC list and then
be back-ported to the binutils-gdb repository.  I'm posting it here
for completeness while the other patches in this series are
discussed.  If the general idea is approved for GDB then I'll get this
merged through the GCC route.

When running Dejagnu on GDB we can now (sometimes) see two additional
test result types, these are '# of paths in test names' and '# of
duplicate test names'.

If the test is run in parallel mode (make -j...) then these extra test
results will appear in the individual test summary files, but are not
merged into the final summary file.

This commit adds support to the merge scripts to carry over these
extra result types.

contrib/ChangeLog:

	* dg-extract-results.py: Handle GDB specific test types.
	* dg-extract-results.sh: Likewise.
---
 contrib/ChangeLog             |  5 +++++
 contrib/dg-extract-results.py |  4 +++-
 contrib/dg-extract-results.sh | 10 ++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/contrib/dg-extract-results.py b/contrib/dg-extract-results.py
index 7100794d42a..b86a4af6b60 100644
--- a/contrib/dg-extract-results.py
+++ b/contrib/dg-extract-results.py
@@ -143,7 +143,9 @@ class Prog:
             '# of known failures\t\t',
             '# of untested testcases\t\t',
             '# of unresolved testcases\t',
-            '# of unsupported tests\t\t'
+            '# of unsupported tests\t\t',
+            '# of paths in test names\t',
+            '# of duplicate test names\t'
         ]
         self.runs = dict()
 
diff --git a/contrib/dg-extract-results.sh b/contrib/dg-extract-results.sh
index f948088370e..021a4f407ac 100755
--- a/contrib/dg-extract-results.sh
+++ b/contrib/dg-extract-results.sh
@@ -400,6 +400,7 @@ BEGIN {
   variant="$VAR"
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kpasscnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0;
+  pathcnt=0; dupcnt=0
   curvar=""; insummary=0
 }
 /^Running target /		{ curvar = \$3; next }
@@ -414,6 +415,8 @@ BEGIN {
 /^# of untested testcases/	{ if (insummary == 1) untstcnt += \$5; next; }
 /^# of unresolved testcases/	{ if (insummary == 1) unrescnt += \$5; next; }
 /^# of unsupported tests/	{ if (insummary == 1) unsupcnt += \$5; next; }
+/^# of paths in test names/	{ if (insummary == 1) pathcnt += \$7; next; }
+/^# of duplicate test names/	{ if (insummary == 1) dupcnt += \$6; next; }
 /^$/				{ if (insummary == 1)
 				    { insummary = 0; curvar = "" }
 				  next
@@ -431,6 +434,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
@@ -452,6 +457,7 @@ cat << EOF > $TOTAL_AWK
 BEGIN {
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0
+  pathcnt=0; dupcnt=0
 }
 /^# of DejaGnu errors/		{ dgerrorcnt += \$5 }
 /^# of expected passes/		{ passcnt += \$5 }
@@ -463,6 +469,8 @@ BEGIN {
 /^# of untested testcases/	{ untstcnt += \$5 }
 /^# of unresolved testcases/	{ unrescnt += \$5 }
 /^# of unsupported tests/	{ unsupcnt += \$5 }
+/^# of paths in test names/	{ pathcnt += \$7 }
+/^# of duplicate test names/	{ dupcnt += \$6 }
 END {
   printf ("\n\t\t=== %s Summary ===\n\n", tool)
   if (dgerrorcnt != 0) printf ("# of DejaGnu errors\t\t%d\n", dgerrorcnt)
@@ -475,6 +483,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
-- 
2.25.3

Re: [PATCH 0/4] Automatic detection of test name problems

[Keith Seitz]

On 4/23/20 10:53 AM, Andrew Burgess wrote:
> So, this series extends our testing framework so that Dejagnu will now
> produce two new result types, these are:
> 
>   # of paths in test names
>   # of duplicate test names

[snip]

> I'll merge the first patch in this series (the
> gdb.btrace/multi-inferior.exp fix) relatively soon as obvious.
> 
> Feedback and thoughts welcome.

This has been a constant source of annoyance and is long overdue. Bravo!

I've looked over the series, sending queries as required. Overall, my
only general comment is, "Why didn't *I* think of this?!?"

Otherwise LGTM.

Going one step further (maybe you've already considered this),
maybe we can [bug Sergio to?] add something to the buildbot so that when it
detects duplicates or pathnames, it emails the list as for regressions?

Keith

Re: [PATCH 2/4] gdb/testsuite: Detect and warn if paths are used in test names

[Keith Seitz]

On 4/23/20 10:53 AM, Andrew Burgess wrote:
> A new library is introduced that hooks into the core of Dejagnu and
> detects when a test's name includes either the source or build paths.
Just a few comments/questions inline.

Keith

> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> new file mode 100644
> index 00000000000..6377ace3fc3
> --- /dev/null
> +++ b/gdb/testsuite/lib/check-test-names.exp
> @@ -0,0 +1,79 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +# This library provides some protection against the introduction of
> +# tests that include either the source of build paths in the test
> +# name.  When a test includes the path in its test name it is harder
> +# to compare results between two runs of GDB from different trees.
> +
> +# Count the number of test names that contain either the source path,
> +# or the build path.
> +set paths_in_test_names 0

This patch series introduces several new entities into the global scope.
Can they be wrapped in a namespace to protect them from users/collisions?

This is the approach taken by other libraries such as the dwarf assembler.

> +# Check if MESSAGE contains either the source path or the build path.
> +# This will result in test names that can't easily be compared between
> +# different runs of GDB.
> +#
> +# Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> +# incremented.
> +proc check_test_names { message } {
> +    global srcdir objdir
> +    global paths_in_test_names
> +
> +    foreach pattern [list $srcdir $objdir> +	if { [ regexp $pattern $message ] } {

Are srcdir/objdir really regular expressions?

`regexp' seems really heavy-handed if they are just strings. `string first
 would be much faster. [Not that it really matters here, but the Tcl pedantic
in me escapes every now and again. I shall attempt to apprehend him.]

> +	    # Count each test just once.
> +	    incr paths_in_test_names
> +	    return
> +	}
> +    }
> +}
> > +# Arrange for CHECK_TEST_NAMES to be called.

I admit, I had to grep sources to figure out that this is an
(AFAICT undocumented) DejaGNU feature. Perhaps "Arrange for
DejaGNU to call ..." or similar might be clearer?

> +set local_record_procs(pass) "check_test_names"
> +set local_record_procs(fail) "check_test_names"
> +set local_record_procs(xfail) "check_test_names"
> +set local_record_procs(kfail) "check_test_names"
> +set local_record_procs(xpass) "check_test_names"
> +set local_record_procs(kpass) "check_test_names"
> +set local_record_procs(unresolved) "check_test_names"
> +set local_record_procs(untested) "check_test_names"
> +set local_record_procs(unsupported) "check_test_names"

Since I failed to contain the Tcl pedantic in me, the above
can be more Tcl-ishly written with `array set', but I am not
requesting that you change anything. Just FYI.

> +# Wrapper around the global Dejagnu LOG_SUMMARY procedure.  Prints a
> +# warning if any tests were found that contained either the source or
> +# build paths.
> +rename log_summary original_log_summary
> +proc log_summary { args } {

Yeah, yuck. I see what you mean. IMO, it is what it is.
Not to mention we'd have to backport to numerous flavors of dejagnu or
get everyone using the same one. Double yuck.

Lesser of two evils.

> +    global paths_in_test_names
> +
> +    # If ARGS is the empty list then we don't want to pass a single
> +    # empty string as a parameter here.
> +    eval "original_log_summary $args"
> +
> +    if { $paths_in_test_names > 0 } {
> +	clone_output "# of paths in test names\t${paths_in_test_names}"
> +    }
> +}
> +
> +# Wrapper around the global Dejagnu RESET_VARS procedure, resets our
> +# PATHS_IN_TEST_NAMES counter.
> +rename reset_vars original_reset_vars
> +proc reset_vars {} {
> +    global paths_in_test_names
> +
> +    original_reset_vars
> +    set paths_in_test_names 0
> +}
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 8418c3d8753..a294b53f2c3 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -29,6 +29,7 @@ load_lib libgloss.exp
>  load_lib cache.exp
>  load_lib gdb-utils.exp
>  load_lib memory.exp
> +load_lib check-test-names.exp
>  
>  global GDB
>  
> 

Re: [PATCH 3/4] gdb/testsuite: Detect and warn about duplicate test names

[Keith Seitz]

On 4/23/20 10:53 AM, Andrew Burgess wrote:
> Spots when tests reuse the same test name and prints a new result type
> '# of duplicate test names' in the result summary.

The above reads awkwardly. Or I'm awkward. Take your pick. :-)

> Currently if the tests are run in parallel mode the new result type is
> not merged into the combined summary file so users will need to run in
> non-parallel mode to check this result.  A later commit will fix this.

I don't know how others approach testing, but when I add new/modify
tests, I always run them separately first without parallelization. This would
still catch any mistakes I might make.

So from my perspective, it's a still a win!

My only comment is a reiteration of the namespace wrapping I encouraged
in the last patch.

Keith

> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> index 6377ace3fc3..85520191f27 100644
> --- a/gdb/testsuite/lib/check-test-names.exp
> +++ b/gdb/testsuite/lib/check-test-names.exp
> @@ -22,6 +22,13 @@
>  # or the build path.
>  set paths_in_test_names 0
>  
> +# An associative array of all test names to the number of times each
> +# name is seen.  Used to detect duplicate test names.
> +array set all_test_names {}
> +
> +# A count of the number of duplicte test names seen.
> +set duplicate_test_names_seen 0
> +
>  # Check if MESSAGE contains either the source path or the build path.
>  # This will result in test names that can't easily be compared between
>  # different runs of GDB.
> @@ -31,6 +38,8 @@ set paths_in_test_names 0
>  proc check_test_names { message } {
>      global srcdir objdir
>      global paths_in_test_names
> +    global all_test_names
> +    global duplicate_test_names_seen
>  
>      foreach pattern [list $srcdir $objdir] {
>  	if { [ regexp $pattern $message ] } {
> @@ -39,6 +48,16 @@ proc check_test_names { message } {
>  	    return
>  	}
>      }
> +
> +    # Initialise a count, or increment the count for this test name.
> +    if {![info exists all_test_names($message)]} {
> +	set all_test_names($message) 0
> +    } else {
> +	if {$all_test_names($message) == 0} {
> +	    incr duplicate_test_names_seen
> +	}
> +	incr all_test_names($message)
> +    }
>  }
>  
>  # Arrange for CHECK_TEST_NAMES to be called.
> @@ -58,6 +77,7 @@ set local_record_procs(unsupported) "check_test_names"
>  rename log_summary original_log_summary
>  proc log_summary { args } {
>      global paths_in_test_names
> +    global duplicate_test_names_seen
>  
>      # If ARGS is the empty list then we don't want to pass a single
>      # empty string as a parameter here.
> @@ -66,6 +86,10 @@ proc log_summary { args } {
>      if { $paths_in_test_names > 0 } {
>  	clone_output "# of paths in test names\t${paths_in_test_names}"
>      }
> +
> +    if { $duplicate_test_names_seen > 0 } {
> +	clone_output "# of duplicate test names\t${duplicate_test_names_seen}"
> +    }
>  }
>  
>  # Wrapper around the global Dejagnu RESET_VARS procedure, resets our
> @@ -73,7 +97,12 @@ proc log_summary { args } {
>  rename reset_vars original_reset_vars
>  proc reset_vars {} {
>      global paths_in_test_names
> +    global all_test_names
> +    global duplicate_test_names_seen
>  
>      original_reset_vars
> +
>      set paths_in_test_names 0
> +    unset all_test_names
> +    set duplicate_test_names_seen 0
>  }
> 

Re: [PATCH 1/4] gdb/testsuite: Remove build paths from test names

[Simon Marchi]

On 2020-04-23 1:53 p.m., Andrew Burgess wrote:
> Having paths in test names makes it harder to compare results from
> different builds of GDB.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.btrace/multi-inferior.exp: Avoid paths in test names.
> ---
>  gdb/testsuite/ChangeLog                     | 4 ++++
>  gdb/testsuite/gdb.btrace/multi-inferior.exp | 6 ++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp b/gdb/testsuite/gdb.btrace/multi-inferior.exp
> index 87a99332e03..fdf889f5ed8 100644
> --- a/gdb/testsuite/gdb.btrace/multi-inferior.exp
> +++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
> @@ -40,7 +40,8 @@ with_test_prefix "inferior 1" {
>  }
>  
>  with_test_prefix "inferior 2" {
> -    gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*"
> +    gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*" \
> +	"add second inferior"
>      gdb_test "inferior 2" "Switching to inferior 2.*"
>  
>      if ![runto_main] {
> @@ -59,7 +60,8 @@ with_test_prefix "inferior 1" {
>  }
>  
>  with_test_prefix "inferior 3" {
> -    gdb_test "add-inferior -exec ${binfile}" "Added inferior 3.*"
> +    gdb_test "add-inferior -exec ${binfile}" "Added inferior 3.*" \
> +	"add third inferior"
>      gdb_test "inferior 3" "Switching to inferior 3.*"
>  
>      if ![runto_main] {
> -- 
> 2.25.3
> 

This LGTM, regardless of the rest of the series.

Simon

Re: [PATCH 2/4] gdb/testsuite: Detect and warn if paths are used in test names

[Andrew Burgess]

* Keith Seitz <keiths@redhat.com> [2020-04-23 13:26:30 -0700]:

> On 4/23/20 10:53 AM, Andrew Burgess wrote:
> > A new library is introduced that hooks into the core of Dejagnu and
> > detects when a test's name includes either the source or build paths.
> Just a few comments/questions inline.
> 
> Keith
> 
> > diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> > new file mode 100644
> > index 00000000000..6377ace3fc3
> > --- /dev/null
> > +++ b/gdb/testsuite/lib/check-test-names.exp
> > @@ -0,0 +1,79 @@
> > +# Copyright 2020 Free Software Foundation, Inc.
> > +
> > +# 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/>.
> > +
> > +# This library provides some protection against the introduction of
> > +# tests that include either the source of build paths in the test
> > +# name.  When a test includes the path in its test name it is harder
> > +# to compare results between two runs of GDB from different trees.
> > +
> > +# Count the number of test names that contain either the source path,
> > +# or the build path.
> > +set paths_in_test_names 0
> 
> This patch series introduces several new entities into the global scope.
> Can they be wrapped in a namespace to protect them from users/collisions?
> 
> This is the approach taken by other libraries such as the dwarf assembler.
> 
> > +# Check if MESSAGE contains either the source path or the build path.
> > +# This will result in test names that can't easily be compared between
> > +# different runs of GDB.
> > +#
> > +# Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> > +# incremented.
> > +proc check_test_names { message } {
> > +    global srcdir objdir
> > +    global paths_in_test_names
> > +
> > +    foreach pattern [list $srcdir $objdir> +	if { [ regexp $pattern $message ] } {
> 
> Are srcdir/objdir really regular expressions?
> 
> `regexp' seems really heavy-handed if they are just strings. `string first
>  would be much faster. [Not that it really matters here, but the Tcl pedantic
> in me escapes every now and again. I shall attempt to apprehend him.]
> 
> > +	    # Count each test just once.
> > +	    incr paths_in_test_names
> > +	    return
> > +	}
> > +    }
> > +}
> > > +# Arrange for CHECK_TEST_NAMES to be called.
> 
> I admit, I had to grep sources to figure out that this is an
> (AFAICT undocumented) DejaGNU feature. Perhaps "Arrange for
> DejaGNU to call ..." or similar might be clearer?
> 
> > +set local_record_procs(pass) "check_test_names"
> > +set local_record_procs(fail) "check_test_names"
> > +set local_record_procs(xfail) "check_test_names"
> > +set local_record_procs(kfail) "check_test_names"
> > +set local_record_procs(xpass) "check_test_names"
> > +set local_record_procs(kpass) "check_test_names"
> > +set local_record_procs(unresolved) "check_test_names"
> > +set local_record_procs(untested) "check_test_names"
> > +set local_record_procs(unsupported) "check_test_names"
> 
> Since I failed to contain the Tcl pedantic in me, the above
> can be more Tcl-ishly written with `array set', but I am not
> requesting that you change anything. Just FYI.

Thanks for your feedback, I'm making a few changes to the patch, but I
wanted to follow up on the 'array set' issue, I'd like to get things
right where I can, however, in this case....

  $ tclsh
  % array set foo {}
  % array set foo(bar) 1
  list must have an even number of elements

This is inline with what I see when I try to use 'array set ....'
within the Dejagnu script, I get the error 'list must have an even
number of elements'.

Is there something I'm doing wrong here? Or is 'array set' not
actually needed in this case?

Thanks,
Andrew


> 
> > +# Wrapper around the global Dejagnu LOG_SUMMARY procedure.  Prints a
> > +# warning if any tests were found that contained either the source or
> > +# build paths.
> > +rename log_summary original_log_summary
> > +proc log_summary { args } {
> 
> Yeah, yuck. I see what you mean. IMO, it is what it is.
> Not to mention we'd have to backport to numerous flavors of dejagnu or
> get everyone using the same one. Double yuck.
> 
> Lesser of two evils.
> 
> > +    global paths_in_test_names
> > +
> > +    # If ARGS is the empty list then we don't want to pass a single
> > +    # empty string as a parameter here.
> > +    eval "original_log_summary $args"
> > +
> > +    if { $paths_in_test_names > 0 } {
> > +	clone_output "# of paths in test names\t${paths_in_test_names}"
> > +    }
> > +}
> > +
> > +# Wrapper around the global Dejagnu RESET_VARS procedure, resets our
> > +# PATHS_IN_TEST_NAMES counter.
> > +rename reset_vars original_reset_vars
> > +proc reset_vars {} {
> > +    global paths_in_test_names
> > +
> > +    original_reset_vars
> > +    set paths_in_test_names 0
> > +}
> > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> > index 8418c3d8753..a294b53f2c3 100644
> > --- a/gdb/testsuite/lib/gdb.exp
> > +++ b/gdb/testsuite/lib/gdb.exp
> > @@ -29,6 +29,7 @@ load_lib libgloss.exp
> >  load_lib cache.exp
> >  load_lib gdb-utils.exp
> >  load_lib memory.exp
> > +load_lib check-test-names.exp
> >  
> >  global GDB
> >  
> > 
> 

Re: [PATCH 2/4] gdb/testsuite: Detect and warn if paths are used in test names

[Keith Seitz]

On 4/27/20 8:58 AM, Andrew Burgess wrote:
> * Keith Seitz <keiths@redhat.com> [2020-04-23 13:26:30 -0700]:
>>> +set local_record_procs(pass) "check_test_names"
>>> +set local_record_procs(fail) "check_test_names"
>>> +set local_record_procs(xfail) "check_test_names"
>>> +set local_record_procs(kfail) "check_test_names"
>>> +set local_record_procs(xpass) "check_test_names"
>>> +set local_record_procs(kpass) "check_test_names"
>>> +set local_record_procs(unresolved) "check_test_names"
>>> +set local_record_procs(untested) "check_test_names"
>>> +set local_record_procs(unsupported) "check_test_names"
>>
>> Since I failed to contain the Tcl pedantic in me, the above
>> can be more Tcl-ishly written with `array set', but I am not
>> requesting that you change anything. Just FYI.
> 
> Thanks for your feedback, I'm making a few changes to the patch, but I
> wanted to follow up on the 'array set' issue, I'd like to get things
> right where I can, however, in this case....
> 
>   $ tclsh
>   % array set foo {}
>   % array set foo(bar) 1
>   list must have an even number of elements
> 
> This is inline with what I see when I try to use 'array set ....'
> within the Dejagnu script, I get the error 'list must have an even
> number of elements'.
> 
> Is there something I'm doing wrong here? Or is 'array set' not
> actually needed in this case?

To be clear -- I apologize if I wasn't earlier -- you do not /need/
to use array set. As I mentioned, the more Tcl-ish way to write large
array initializations is

array set local_record_procs {
  pass check_test_names
  fail check_test_names
  # ...
}

"array set" takes a list of key-value pairs, so there must be
an even number of elements in the list.

However, in hindsight, since all these are being set to the same
value, an even more Tcl-ish way:

foreach nm {pass fail xfail kfail xpass kpass unresolved untested unsupported} {
  set local_record_procs($nm) check_test_names
}

[While tempting to use "[array names local_record_procs]", reading framework.exp
shows that this array is not actually expected to exist, so we have to
explicitly list out all test result outcomes.]

Okay, that was probably way more hassle than necessary... I promise to
backburner my [counterproductive] Tcl pedantic-ness[sic]!

Sorry about that,
Keith

Re: [PATCH 2/4] gdb/testsuite: Detect and warn if paths are used in test names

[Andrew Burgess]

* Keith Seitz <keiths@redhat.com> [2020-04-27 09:42:30 -0700]:

> On 4/27/20 8:58 AM, Andrew Burgess wrote:
> > * Keith Seitz <keiths@redhat.com> [2020-04-23 13:26:30 -0700]:
> >>> +set local_record_procs(pass) "check_test_names"
> >>> +set local_record_procs(fail) "check_test_names"
> >>> +set local_record_procs(xfail) "check_test_names"
> >>> +set local_record_procs(kfail) "check_test_names"
> >>> +set local_record_procs(xpass) "check_test_names"
> >>> +set local_record_procs(kpass) "check_test_names"
> >>> +set local_record_procs(unresolved) "check_test_names"
> >>> +set local_record_procs(untested) "check_test_names"
> >>> +set local_record_procs(unsupported) "check_test_names"
> >>
> >> Since I failed to contain the Tcl pedantic in me, the above
> >> can be more Tcl-ishly written with `array set', but I am not
> >> requesting that you change anything. Just FYI.
> > 
> > Thanks for your feedback, I'm making a few changes to the patch, but I
> > wanted to follow up on the 'array set' issue, I'd like to get things
> > right where I can, however, in this case....
> > 
> >   $ tclsh
> >   % array set foo {}
> >   % array set foo(bar) 1
> >   list must have an even number of elements
> > 
> > This is inline with what I see when I try to use 'array set ....'
> > within the Dejagnu script, I get the error 'list must have an even
> > number of elements'.
> > 
> > Is there something I'm doing wrong here? Or is 'array set' not
> > actually needed in this case?
> 
> To be clear -- I apologize if I wasn't earlier -- you do not /need/
> to use array set. As I mentioned, the more Tcl-ish way to write large
> array initializations is
> 
> array set local_record_procs {
>   pass check_test_names
>   fail check_test_names
>   # ...
> }
> 
> "array set" takes a list of key-value pairs, so there must be
> an even number of elements in the list.
> 
> However, in hindsight, since all these are being set to the same
> value, an even more Tcl-ish way:
> 
> foreach nm {pass fail xfail kfail xpass kpass unresolved untested unsupported} {
>   set local_record_procs($nm) check_test_names
> }
> 
> [While tempting to use "[array names local_record_procs]", reading framework.exp
> shows that this array is not actually expected to exist, so we have to
> explicitly list out all test result outcomes.]
> 
> Okay, that was probably way more hassle than necessary... I promise to
> backburner my [counterproductive] Tcl pedantic-ness[sic]!

No, it's not a problem.  Dejagnu / TCL isn't likely to go anywhere
anytime soon, so it's always good to learn more.

I'll incorporate your suggested foreach loop improvement.

Thanks,
Andrew

[PATCHv2 0/3] Automatic detection of test name problems

[Andrew Burgess]

Changes since v1:

  1. Original patch #1 is now merged.

  2. Functionality is now placed inside a namespace.
  
  3. Better counting for multi-variant test runs, this is inline with
     how Dejagnu's muti-variant result counting works.

  4. Use 'string first' instead of 'regexp'.

  5. Reworded commit message on what is now patch #2.

Further feedback welcome.

Thanks,
Andrew

---

Andrew Burgess (3):
  gdb/testsuite: Detect and warn if paths are used in test names
  gdb/testsuite: Detect and warn about duplicate test names
  contrib: Handle GDB specific test result types

 contrib/ChangeLog                      |   5 +
 contrib/dg-extract-results.py          |   4 +-
 contrib/dg-extract-results.sh          |  10 ++
 gdb/testsuite/ChangeLog                |  13 +++
 gdb/testsuite/lib/check-test-names.exp | 135 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   1 +
 6 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

-- 
2.25.3

[PATCHv2 1/3] gdb/testsuite: Detect and warn if paths are used in test names

[Andrew Burgess]

A new library is introduced that hooks into the core of Dejagnu and
detects when a test's name includes either the source or build paths.
If any offending test names are detected then Dejagnu will print a
new result type, '# of paths in test names'.  Users should treat this
result type just like other bad results types, and aim not to increase
this number.

Currently for a local run on my machine, I don't see any offending
test names, but it is possible that different targets, or different
configurations, might currently be breaking the no paths rule.

In order to get this working I have needed to wrap two core Dejagnu
functions, log_summary, and reset_vars.  Relying on core functions
that are not part of any API is always going to be risky, given the
relatively slow rate of Dejagnu change this is probably OK for now,
and we can possibly upstream some changes to Dejagnu that would allow
this functionality to be supported in a more official way later on.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  A later commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp: Include check-test-names.exp library.
	* lib/check-test-names.exp: New file.
---
 gdb/testsuite/ChangeLog                |   5 ++
 gdb/testsuite/lib/check-test-names.exp | 115 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   1 +
 3 files changed, 121 insertions(+)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
new file mode 100644
index 00000000000..702dc6ef406
--- /dev/null
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -0,0 +1,115 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# 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/>.
+
+# This library provides some protection against the introduction of
+# tests that include either the source of build paths in the test
+# name.  When a test includes the path in its test name it is harder
+# to compare results between two runs of GDB from different trees.
+
+namespace eval ::CheckTestNames {
+    # An associative array of counts of tests that include a path in their
+    # test name.  There are two counts, 'count', which counts occurrences
+    # within a single variant run, and 'total', which counts across all
+    # variants.
+    variable counts
+    array set counts {}
+    foreach nm {paths} {
+	set counts($nm,count) 0
+	set counts($nm,total) 0
+    }
+
+    # Increment the count, and total count for TYPE.
+    proc inc_count { type } {
+	variable counts
+
+	incr counts($type,count)
+	incr counts($type,total)
+    }
+
+    # Check if MESSAGE contains either the source path or the build path.
+    # This will result in test names that can't easily be compared between
+    # different runs of GDB.
+    #
+    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
+    # incremented.
+    proc check { message } {
+	global srcdir objdir
+
+	foreach path [list $srcdir $objdir] {
+	    if { [ string first $path $message ] >= 0 } {
+		# Count each test just once.
+		inc_count paths
+		return
+	    }
+	}
+    }
+
+    # If COUNT is greater than zero, disply PREFIX followed by COUNT.
+    proc maybe_show_count { prefix count } {
+	if { $count > 0 } {
+	    clone_output "$prefix$count"
+	}
+    }
+
+    # Rename Dejagnu's log_summary procedure, and create do_log_summary to
+    # replace it.  We arrange to have do_log_summary called later.
+    rename ::log_summary log_summary
+    proc do_log_summary { args } {
+	variable counts
+
+	# If ARGS is the empty list then we don't want to pass a single
+	# empty string as a parameter here.
+	eval "CheckTestNames::log_summary $args"
+
+	if { [llength $args] == 0 } {
+	    set which "count"
+	} else {
+	    set which [lindex $args 0]
+	}
+
+	maybe_show_count "# of paths in test names\t" \
+	    $counts(paths,$which)
+    }
+
+    # Rename Dejagnu's reset_vars procedure, and create do_reset_vars to
+    # replace it.  We arrange to have do_reset_vars called later.
+    rename ::reset_vars reset_vars
+    proc do_reset_vars {} {
+	variable counts
+
+	CheckTestNames::reset_vars
+
+	foreach nm {paths} {
+	    set counts($nm,count) 0
+	}
+    }
+}
+
+# Arrange for Dejagnu to call CheckTestNames::check for each test result.
+foreach nm {pass fail xfail kfail xpass kpass unresolved untested \
+		unsupported} {
+    set local_record_procs($nm) "CheckTestNames::check"
+}
+
+# Create new global log_summary to replace Dejagnu's.
+proc log_summary { args } {
+    eval "CheckTestNames::do_log_summary $args"
+}
+
+# Create new global reset_vars to replace Dejagnu's.
+proc reset_vars {} {
+    eval "CheckTestNames::do_reset_vars"
+}
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2208f3a1a9b..aa95f6da933 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -29,6 +29,7 @@ load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
 load_lib memory.exp
+load_lib check-test-names.exp
 
 global GDB
 
-- 
2.25.3

[PATCHv2 2/3] gdb/testsuite: Detect and warn about duplicate test names

[Andrew Burgess]

Building on the previous commit, this patch detects when two tests
have the same test name and causes Dejagnu to print a new result type
'# of duplicate test names' in the result summary.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  A later commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/check-test-names.exp (all_test_names): New global.
	(duplicate_test_names_seen): New global.
	(check_test_names): Check for duplicate test names.
	(log_summary): Warn about duplicate test names.
	(reset_vars): Reset counters for duplicate test names.
---
 gdb/testsuite/ChangeLog                |  8 +++++++
 gdb/testsuite/lib/check-test-names.exp | 32 +++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
index 702dc6ef406..15c5544c8e4 100644
--- a/gdb/testsuite/lib/check-test-names.exp
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -19,13 +19,18 @@
 # to compare results between two runs of GDB from different trees.
 
 namespace eval ::CheckTestNames {
-    # An associative array of counts of tests that include a path in their
-    # test name.  There are two counts, 'count', which counts occurrences
-    # within a single variant run, and 'total', which counts across all
-    # variants.
+    # An associative array of all test names to the number of times each
+    # name is seen.  Used to detect duplicate test names.
+    variable all_test_names
+    array set all_test_names {}
+
+    # An associative array of counts of tests that either include a path in
+    # their test name, or have a duplicate test name.  There are two counts
+    # for each issue, 'count', which counts occurrences within a single
+    # variant run, and 'total', which counts across all variants.
     variable counts
     array set counts {}
-    foreach nm {paths} {
+    foreach nm {paths duplicates} {
 	set counts($nm,count) 0
 	set counts($nm,total) 0
     }
@@ -46,6 +51,7 @@ namespace eval ::CheckTestNames {
     # incremented.
     proc check { message } {
 	global srcdir objdir
+	variable all_test_names
 
 	foreach path [list $srcdir $objdir] {
 	    if { [ string first $path $message ] >= 0 } {
@@ -54,6 +60,16 @@ namespace eval ::CheckTestNames {
 		return
 	    }
 	}
+
+	# Initialise a count, or increment the count for this test name.
+	if {![info exists all_test_names($message)]} {
+	    set all_test_names($message) 0
+	} else {
+	    if {$all_test_names($message) == 0} {
+		inc_count duplicates
+	    }
+	    incr all_test_names($message)
+	}
     }
 
     # If COUNT is greater than zero, disply PREFIX followed by COUNT.
@@ -81,17 +97,21 @@ namespace eval ::CheckTestNames {
 
 	maybe_show_count "# of paths in test names\t" \
 	    $counts(paths,$which)
+	maybe_show_count "# of duplicate test names\t" \
+	    $counts(duplicates,$which)
     }
 
     # Rename Dejagnu's reset_vars procedure, and create do_reset_vars to
     # replace it.  We arrange to have do_reset_vars called later.
     rename ::reset_vars reset_vars
     proc do_reset_vars {} {
+	variable all_test_names
 	variable counts
 
 	CheckTestNames::reset_vars
 
-	foreach nm {paths} {
+	unset all_test_names
+	foreach nm {paths duplicates} {
 	    set counts($nm,count) 0
 	}
     }
-- 
2.25.3

[PATCHv2 3/3] contrib: Handle GDB specific test result types

[Andrew Burgess]

NOTE: I know that this patch needs to go through the GCC list and then
be back-ported to the binutils-gdb repository.  I'm posting it here
for completeness while the other patches in this series are
discussed.  If the general idea is approved for GDB then I'll get this
merged through the GCC route.

When running Dejagnu on GDB we can now (sometimes) see two additional
test result types, these are '# of paths in test names' and '# of
duplicate test names'.

If the test is run in parallel mode (make -j...) then these extra test
results will appear in the individual test summary files, but are not
merged into the final summary file.

This commit adds support to the merge scripts to carry over these
extra result types.

contrib/ChangeLog:

	* dg-extract-results.py: Handle GDB specific test types.
	* dg-extract-results.sh: Likewise.
---
 contrib/ChangeLog             |  5 +++++
 contrib/dg-extract-results.py |  4 +++-
 contrib/dg-extract-results.sh | 10 ++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/contrib/dg-extract-results.py b/contrib/dg-extract-results.py
index 7100794d42a..b86a4af6b60 100644
--- a/contrib/dg-extract-results.py
+++ b/contrib/dg-extract-results.py
@@ -143,7 +143,9 @@ class Prog:
             '# of known failures\t\t',
             '# of untested testcases\t\t',
             '# of unresolved testcases\t',
-            '# of unsupported tests\t\t'
+            '# of unsupported tests\t\t',
+            '# of paths in test names\t',
+            '# of duplicate test names\t'
         ]
         self.runs = dict()
 
diff --git a/contrib/dg-extract-results.sh b/contrib/dg-extract-results.sh
index f948088370e..021a4f407ac 100755
--- a/contrib/dg-extract-results.sh
+++ b/contrib/dg-extract-results.sh
@@ -400,6 +400,7 @@ BEGIN {
   variant="$VAR"
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kpasscnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0;
+  pathcnt=0; dupcnt=0
   curvar=""; insummary=0
 }
 /^Running target /		{ curvar = \$3; next }
@@ -414,6 +415,8 @@ BEGIN {
 /^# of untested testcases/	{ if (insummary == 1) untstcnt += \$5; next; }
 /^# of unresolved testcases/	{ if (insummary == 1) unrescnt += \$5; next; }
 /^# of unsupported tests/	{ if (insummary == 1) unsupcnt += \$5; next; }
+/^# of paths in test names/	{ if (insummary == 1) pathcnt += \$7; next; }
+/^# of duplicate test names/	{ if (insummary == 1) dupcnt += \$6; next; }
 /^$/				{ if (insummary == 1)
 				    { insummary = 0; curvar = "" }
 				  next
@@ -431,6 +434,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
@@ -452,6 +457,7 @@ cat << EOF > $TOTAL_AWK
 BEGIN {
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0
+  pathcnt=0; dupcnt=0
 }
 /^# of DejaGnu errors/		{ dgerrorcnt += \$5 }
 /^# of expected passes/		{ passcnt += \$5 }
@@ -463,6 +469,8 @@ BEGIN {
 /^# of untested testcases/	{ untstcnt += \$5 }
 /^# of unresolved testcases/	{ unrescnt += \$5 }
 /^# of unsupported tests/	{ unsupcnt += \$5 }
+/^# of paths in test names/	{ pathcnt += \$7 }
+/^# of duplicate test names/	{ dupcnt += \$6 }
 END {
   printf ("\n\t\t=== %s Summary ===\n\n", tool)
   if (dgerrorcnt != 0) printf ("# of DejaGnu errors\t\t%d\n", dgerrorcnt)
@@ -475,6 +483,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
-- 
2.25.3

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Keith Seitz]

On 4/27/20 3:01 PM, Andrew Burgess wrote:
> Changes since v1:
> 
>   1. Original patch #1 is now merged.
> 
>   2. Functionality is now placed inside a namespace.
>   
>   3. Better counting for multi-variant test runs, this is inline with
>      how Dejagnu's muti-variant result counting works.
> 
>   4. Use 'string first' instead of 'regexp'.
> 
>   5. Reworded commit message on what is now patch #2.
> 
> Further feedback welcome.

Wow, that's almost a complete rewrite! While not what I was really
suggesting, I have to admit, a /big/ smile came to my face while
reading this. It is so nicely done!

Thank you!

Keith

PS. As you know, IANAM, but you are, so I encourage you to approve
your patch. ;-)

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Andrew Burgess]

* Keith Seitz <keiths@redhat.com> [2020-04-28 12:08:25 -0700]:

> On 4/27/20 3:01 PM, Andrew Burgess wrote:
> > Changes since v1:
> > 
> >   1. Original patch #1 is now merged.
> > 
> >   2. Functionality is now placed inside a namespace.
> >   
> >   3. Better counting for multi-variant test runs, this is inline with
> >      how Dejagnu's muti-variant result counting works.
> > 
> >   4. Use 'string first' instead of 'regexp'.
> > 
> >   5. Reworded commit message on what is now patch #2.
> > 
> > Further feedback welcome.
> 
> Wow, that's almost a complete rewrite! While not what I was really
> suggesting, I have to admit, a /big/ smile came to my face while
> reading this. It is so nicely done!
> 
> Thank you!
> 
> Keith
> 
> PS. As you know, IANAM, but you are, so I encourage you to approve
> your patch. ;-)

Thanks for your feedback, especially your TCL suggestions.

I'll let the patch sit for a while to see if anyone else has any
input.

Thanks,
Andrew

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Simon Marchi]

On 2020-04-29 5:02 a.m., Andrew Burgess wrote:
> * Keith Seitz <keiths@redhat.com> [2020-04-28 12:08:25 -0700]:
> 
>> On 4/27/20 3:01 PM, Andrew Burgess wrote:
>>> Changes since v1:
>>>
>>>   1. Original patch #1 is now merged.
>>>
>>>   2. Functionality is now placed inside a namespace.
>>>   
>>>   3. Better counting for multi-variant test runs, this is inline with
>>>      how Dejagnu's muti-variant result counting works.
>>>
>>>   4. Use 'string first' instead of 'regexp'.
>>>
>>>   5. Reworded commit message on what is now patch #2.
>>>
>>> Further feedback welcome.
>>
>> Wow, that's almost a complete rewrite! While not what I was really
>> suggesting, I have to admit, a /big/ smile came to my face while
>> reading this. It is so nicely done!
>>
>> Thank you!
>>
>> Keith
>>
>> PS. As you know, IANAM, but you are, so I encourage you to approve
>> your patch. ;-)
> 
> Thanks for your feedback, especially your TCL suggestions.
> 
> I'll let the patch sit for a while to see if anyone else has any
> input.
> 
> Thanks,
> Andrew
> 

Hi Andrew,

Thanks for doing this, it's nice.  And by testing this patchset, I've found an offender
and sent a patch for it :)

  https://sourceware.org/pipermail/gdb-patches/2020-April/168052.html

When you detect an offender, do you think you could print something?  A bit like a "FAIL"
is printed when a test fails.  For example:

  DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5

That would make it easier to spot the problematic test(s).  But even without that, your
patchset looks good and useful to me.

Simon

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2020-04-29 11:04:40 -0400]:

> On 2020-04-29 5:02 a.m., Andrew Burgess wrote:
> > * Keith Seitz <keiths@redhat.com> [2020-04-28 12:08:25 -0700]:
> > 
> >> On 4/27/20 3:01 PM, Andrew Burgess wrote:
> >>> Changes since v1:
> >>>
> >>>   1. Original patch #1 is now merged.
> >>>
> >>>   2. Functionality is now placed inside a namespace.
> >>>   
> >>>   3. Better counting for multi-variant test runs, this is inline with
> >>>      how Dejagnu's muti-variant result counting works.
> >>>
> >>>   4. Use 'string first' instead of 'regexp'.
> >>>
> >>>   5. Reworded commit message on what is now patch #2.
> >>>
> >>> Further feedback welcome.
> >>
> >> Wow, that's almost a complete rewrite! While not what I was really
> >> suggesting, I have to admit, a /big/ smile came to my face while
> >> reading this. It is so nicely done!
> >>
> >> Thank you!
> >>
> >> Keith
> >>
> >> PS. As you know, IANAM, but you are, so I encourage you to approve
> >> your patch. ;-)
> > 
> > Thanks for your feedback, especially your TCL suggestions.
> > 
> > I'll let the patch sit for a while to see if anyone else has any
> > input.
> > 
> > Thanks,
> > Andrew
> > 
> 
> Hi Andrew,
> 
> Thanks for doing this, it's nice.  And by testing this patchset, I've found an offender
> and sent a patch for it :)
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-April/168052.html
> 
> When you detect an offender, do you think you could print something?  A bit like a "FAIL"
> is printed when a test fails.  For example:
> 
>   DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5
> 
> That would make it easier to spot the problematic test(s).  But even without that, your
> patchset looks good and useful to me.

Ask and you shall receive!

Below is a patch that applies on top of the existing series just to
try.  If you like this I'll fold this back into the relevant patches.

We now print "PATH: ...." or "DUPLICATE: ...." when we see an
offending test.

On printing DUPLICATE, things are a little .... odd.  I currently
count the number of unique duplicates, so:

  PASS: foo
  PASS: foo
  PASS: foo
  PASS: bar
  PASS: bar
  PASS: bar

Will give a duplicate count of '2', that is 'foo' and 'bar' are
duplicated.

However, I chose to print "DUPLICATE: ...." for _every_ duplicate, so
you would get this:

  PASS: foo
  PASS: foo
  DUPLICATE: foo
  PASS: foo
  DUPLICATE: foo
  PASS: bar
  PASS: bar
  DUPLICATE: bar
  PASS: bar
  DUPLICATE: bar

Obviously we don't get a DUPLICATE message for the first 'foo', or the
first 'bar', we can't know by that point that these are going to be
duplicated.

But, it might be confusing that this test will report 2 duplicates,
but contain 4 DUPLICATE lines.  Would it in fact be better to report a
count of 4 in this case I wonder?

Thoughts welcome.

Thanks,
Andrew

---

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
index 15c5544c8e4..8b525897ba3 100644
--- a/gdb/testsuite/lib/check-test-names.exp
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -43,24 +43,27 @@ namespace eval ::CheckTestNames {
 	incr counts($type,total)
     }
 
-    # Check if MESSAGE contains either the source path or the build path.
-    # This will result in test names that can't easily be compared between
-    # different runs of GDB.
-    #
-    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
-    # incremented.
-    proc check { message } {
+    # Check if MESSAGE contains a build or source path, if it does increment
+    # the relevant counter and return 1, otherwise, return 0.
+    proc _check_paths { message } {
 	global srcdir objdir
-	variable all_test_names
 
 	foreach path [list $srcdir $objdir] {
 	    if { [ string first $path $message ] >= 0 } {
 		# Count each test just once.
 		inc_count paths
-		return
+		return 1
 	    }
 	}
 
+	return 0
+    }
+
+    # Check if MESSAGE is a duplicate, if it is then increment the
+    # duplicates counter and return 1, otherwise, return 0.
+    proc _check_duplicates { message } {
+	variable all_test_names
+
 	# Initialise a count, or increment the count for this test name.
 	if {![info exists all_test_names($message)]} {
 	    set all_test_names($message) 0
@@ -69,6 +72,43 @@ namespace eval ::CheckTestNames {
 		inc_count duplicates
 	    }
 	    incr all_test_names($message)
+	    return 1
+	}
+
+	return 0
+    }
+
+    # Remove the leading Dejagnu status marker from MESSAGE, and return the
+    # remainder of MESSAGE.  A status marker is something like 'PASS: '.  If
+    # no status marker is found, then just return MESSAGE unmodified.
+    proc _strip_status { message } {
+	foreach status {PASS FAIL XFAIL KFAIL XPASS KPASS UNRESOLVED \
+			    UNTESTED UNSUPPORTED} {
+	    set txt "${status}: "
+	    if { [string compare -length [string len $txt] \
+		      $txt $message] == 0 } {
+		return  [string range $message [string len $txt] end]
+	    }
+	}
+
+	return $message
+    }
+
+    # Check if MESSAGE contains either the source path or the build path.
+    # This will result in test names that can't easily be compared between
+    # different runs of GDB.
+    #
+    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
+    # incremented.
+    proc check { message } {
+	set message [ _strip_status $message ]
+
+	if [ _check_paths $message ] {
+	    clone_output "PATH: $message"
+	}
+
+	if [ _check_duplicates $message ] {
+	    clone_output "DUPLICATE: $message"
 	}
     }
 

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Keith Seitz]

On 4/29/20 8:38 AM, Andrew Burgess wrote:
> * Simon Marchi <simark@simark.ca> [2020-04-29 11:04:40 -0400]:
> 
>> When you detect an offender, do you think you could print something?  A bit like a "FAIL"
>> is printed when a test fails.  For example:
>>
>>   DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5
>>
>> That would make it easier to spot the problematic test(s).  But even without that, your
>> patchset looks good and useful to me.

That is a great idea, Simon. [Again -- why didn't I think of that?]

> However, I chose to print "DUPLICATE: ...." for _every_ duplicate, so
> you would get this:
> 
>   PASS: foo
>   PASS: foo
>   DUPLICATE: foo
>   PASS: foo
>   DUPLICATE: foo
>   PASS: bar
>   PASS: bar
>   DUPLICATE: bar
>   PASS: bar
>   DUPLICATE: bar
> 
> Obviously we don't get a DUPLICATE message for the first 'foo', or the
> first 'bar', we can't know by that point that these are going to be
> duplicated.
> 
> But, it might be confusing that this test will report 2 duplicates,
> but contain 4 DUPLICATE lines.  Would it in fact be better to report a
> count of 4 in this case I wonder?

I don't really care one way or the other. This is an exceptional case, and
is /far/ less offensive than, e.g., assertions aborting gdb. Seeing messages
just reinforces the message IMO. YMMV.

Just one or two Tcl comments. Of course. ;-)

> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
> index 15c5544c8e4..8b525897ba3 100644
> --- a/gdb/testsuite/lib/check-test-names.exp
> +++ b/gdb/testsuite/lib/check-test-names.exp
> @@ -43,24 +43,27 @@ namespace eval ::CheckTestNames {
>  	incr counts($type,total)
>      }
>  
> -    # Check if MESSAGE contains either the source path or the build path.
> -    # This will result in test names that can't easily be compared between
> -    # different runs of GDB.
> -    #
> -    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> -    # incremented.
> -    proc check { message } {
> +    # Check if MESSAGE contains a build or source path, if it does increment
> +    # the relevant counter and return 1, otherwise, return 0.
> +    proc _check_paths { message } {
>  	global srcdir objdir
> -	variable all_test_names
>  
>  	foreach path [list $srcdir $objdir] {
>  	    if { [ string first $path $message ] >= 0 } {
>  		# Count each test just once.
>  		inc_count paths
> -		return
> +		return 1
>  	    }
>  	}
>  
> +	return 0
> +    }

Tcl supports booleans. This could return true/false.

> +
> +    # Check if MESSAGE is a duplicate, if it is then increment the
> +    # duplicates counter and return 1, otherwise, return 0.
> +    proc _check_duplicates { message } {
> +	variable all_test_names
> +
>  	# Initialise a count, or increment the count for this test name.
>  	if {![info exists all_test_names($message)]} {
>  	    set all_test_names($message) 0
> @@ -69,6 +72,43 @@ namespace eval ::CheckTestNames {
>  		inc_count duplicates
>  	    }
>  	    incr all_test_names($message)
> +	    return 1
> +	}
> +
> +	return 0
> +    }
> +
> +    # Remove the leading Dejagnu status marker from MESSAGE, and return the
> +    # remainder of MESSAGE.  A status marker is something like 'PASS: '.  If
> +    # no status marker is found, then just return MESSAGE unmodified.
> +    proc _strip_status { message } {
> +	foreach status {PASS FAIL XFAIL KFAIL XPASS KPASS UNRESOLVED \
> +			    UNTESTED UNSUPPORTED} {
> +	    set txt "${status}: "
> +	    if { [string compare -length [string len $txt] \
> +		      $txt $message] == 0 } {
> +		return  [string range $message [string len $txt] end]

Maybe it doesn't matter, but we know /all/ test result messages from DejaGNU are
going to start "STATUS: $result_message". Would it be worth simply looking for ": "
from the start of the string?

That would eliminate the loop here and greatly simplify this. It is, of course, less
precise than the above.

Keith

> +	    }
> +	}
> +
> +	return $message
> +    }
> +
> +    # Check if MESSAGE contains either the source path or the build path.
> +    # This will result in test names that can't easily be compared between
> +    # different runs of GDB.
> +    #
> +    # Any offending paths in message cause PATHS_IN_TEST_NAMES to be
> +    # incremented.
> +    proc check { message } {
> +	set message [ _strip_status $message ]
> +
> +	if [ _check_paths $message ] {
> +	    clone_output "PATH: $message"
> +	}
> +
> +	if [ _check_duplicates $message ] {
> +	    clone_output "DUPLICATE: $message"
>  	}
>      }
>  
> 

Re: [PATCHv2 0/3] Automatic detection of test name problems

[Simon Marchi]

On 2020-04-29 12:03 p.m., Keith Seitz wrote:
> On 4/29/20 8:38 AM, Andrew Burgess wrote:
>> * Simon Marchi <simark@simark.ca> [2020-04-29 11:04:40 -0400]:
>>
>>> When you detect an offender, do you think you could print something?  A bit like a "FAIL"
>>> is printed when a test fails.  For example:
>>>
>>>   DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5
>>>
>>> That would make it easier to spot the problematic test(s).  But even without that, your
>>> patchset looks good and useful to me.
> 
> That is a great idea, Simon. [Again -- why didn't I think of that?]
> 
>> However, I chose to print "DUPLICATE: ...." for _every_ duplicate, so
>> you would get this:
>>
>>   PASS: foo
>>   PASS: foo
>>   DUPLICATE: foo
>>   PASS: foo
>>   DUPLICATE: foo
>>   PASS: bar
>>   PASS: bar
>>   DUPLICATE: bar
>>   PASS: bar
>>   DUPLICATE: bar
>>
>> Obviously we don't get a DUPLICATE message for the first 'foo', or the
>> first 'bar', we can't know by that point that these are going to be
>> duplicated.
>>
>> But, it might be confusing that this test will report 2 duplicates,
>> but contain 4 DUPLICATE lines.  Would it in fact be better to report a
>> count of 4 in this case I wonder?
> 
> I don't really care one way or the other. This is an exceptional case, and
> is /far/ less offensive than, e.g., assertions aborting gdb. Seeing messages
> just reinforces the message IMO. YMMV.

I also don't mind, as long as it pops in my face.  It could be printed only once
and it would be enough.

This looks good to me:

DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5
DUPLICATE: gdb.base/break.exp: set breakpoint via non-integer convenience variable disallowed

Simon

[PATCHv3 0/3] Automatic detection of test name problems

[Andrew Burgess]

Changes since v2:

  - Now print PATH: .... or DUPLICATE: .... into the summary and log
    files when an offending test name is seen.

  - The dg-extract-results.* script merge these new status lines into
    the single unified file when running tests in parallel.

All feedback welcome,

Thanks,
Andrew


---

Andrew Burgess (3):
  gdb/testsuite: Detect and warn if paths are used in test names
  gdb/testsuite: Detect and warn about duplicate test names
  contrib: Handle GDB specific test result types

 contrib/ChangeLog                      |   5 +
 contrib/dg-extract-results.py          |   6 +-
 contrib/dg-extract-results.sh          |  12 +-
 gdb/testsuite/ChangeLog                |  15 +++
 gdb/testsuite/lib/check-test-names.exp | 175 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   1 +
 6 files changed, 211 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

-- 
2.25.3

[PATCHv3 1/3] gdb/testsuite: Detect and warn if paths are used in test names

[Andrew Burgess]

A new library is introduced that hooks into the core of Dejagnu and
detects when a test's name includes either the source or build paths.
If any offending test names are detected then Dejagnu will print a
new result type, '# of paths in test names'.  Users should treat this
result type just like other bad results types, and aim not to increase
this number.

As well as displaying the total number of offending tests as part of
the final results, a new marker is included in both the gdb.log and
gdb.sum files, this marker starts with 'PATH: ', so an offending test
would be expected to appear like this:

  PASS: gdb.base/sometest.exp: Loaded /path/to/build/testsuite/foo.exe
  PATH: gdb.base/sometest.exp: Loaded /path/to/build/testsuite/foo.exe

This should make it easier to track down offending tests.

Currently for a local run on my machine, I don't see any offending
test names, but it is possible that different targets, or different
configurations, might currently be breaking the no paths rule.

In order to get this working I have needed to wrap two core Dejagnu
functions, log_summary, and reset_vars.  Relying on core functions
that are not part of any API is always going to be risky, given the
relatively slow rate of Dejagnu change this is probably OK for now,
and we can possibly upstream some changes to Dejagnu that would allow
this functionality to be supported in a more official way later on.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  Similarly, the 'PATH: '
markers will not be merged into the combined summary file.  A later
commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp: Include check-test-names.exp library.
	* lib/check-test-names.exp: New file.
---
 gdb/testsuite/ChangeLog                |   5 +
 gdb/testsuite/lib/check-test-names.exp | 143 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   1 +
 3 files changed, 149 insertions(+)
 create mode 100644 gdb/testsuite/lib/check-test-names.exp

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
new file mode 100644
index 00000000000..9a35ce7c5db
--- /dev/null
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -0,0 +1,143 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# 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/>.
+
+# This library provides some protection against the introduction of
+# tests that include either the source of build paths in the test
+# name.  When a test includes the path in its test name it is harder
+# to compare results between two runs of GDB from different trees.
+
+namespace eval ::CheckTestNames {
+    # An associative array of counts of tests that include a path in their
+    # test name.  There are two counts, 'count', which counts occurrences
+    # within a single variant run, and 'total', which counts across all
+    # variants.
+    variable counts
+    array set counts {}
+    foreach nm {paths} {
+	set counts($nm,count) 0
+	set counts($nm,total) 0
+    }
+
+    # Increment the count, and total count for TYPE.
+    proc inc_count { type } {
+	variable counts
+
+	incr counts($type,count)
+	incr counts($type,total)
+    }
+
+    # Check if MESSAGE contains a build or source path, if it does increment
+    # the relevant counter and return true, otherwise, return false.
+    proc _check_paths { message } {
+	global srcdir objdir
+
+	foreach path [list $srcdir $objdir] {
+	    if { [ string first $path $message ] >= 0 } {
+		# Count each test just once.
+		inc_count paths
+		return true
+	    }
+	}
+
+	return false
+    }
+
+    # Remove the leading Dejagnu status marker from MESSAGE, and
+    # return the remainder of MESSAGE.  A status marker is something
+    # like 'PASS: '.  It is assumed that MESSAGE does contain such a
+    # marker.  If it doesn't then MESSAGE is returned unmodified.
+    proc _strip_status { message } {
+	# Find the position of the first ': ' string.
+	set pos [string first ": " $message]
+	if { $pos > -1 } {
+	    # The '+ 2' is so we skip the ': ' we found above.
+	    return  [string range $message [expr $pos + 2] end]
+	}
+
+	return $message
+    }
+
+    # Check if MESSAGE contains either the source path or the build path.
+    # This will result in test names that can't easily be compared between
+    # different runs of GDB.
+    #
+    # Any offending test names cause the corresponding count to be
+    # incremented, and an extra message to be printed into the log
+    # file.
+    proc check { message } {
+	set message [ _strip_status $message ]
+
+	if [ _check_paths $message ] {
+	    clone_output "PATH: $message"
+	}
+    }
+
+    # If COUNT is greater than zero, disply PREFIX followed by COUNT.
+    proc maybe_show_count { prefix count } {
+	if { $count > 0 } {
+	    clone_output "$prefix$count"
+	}
+    }
+
+    # Rename Dejagnu's log_summary procedure, and create do_log_summary to
+    # replace it.  We arrange to have do_log_summary called later.
+    rename ::log_summary log_summary
+    proc do_log_summary { args } {
+	variable counts
+
+	# If ARGS is the empty list then we don't want to pass a single
+	# empty string as a parameter here.
+	eval "CheckTestNames::log_summary $args"
+
+	if { [llength $args] == 0 } {
+	    set which "count"
+	} else {
+	    set which [lindex $args 0]
+	}
+
+	maybe_show_count "# of paths in test names\t" \
+	    $counts(paths,$which)
+    }
+
+    # Rename Dejagnu's reset_vars procedure, and create do_reset_vars to
+    # replace it.  We arrange to have do_reset_vars called later.
+    rename ::reset_vars reset_vars
+    proc do_reset_vars {} {
+	variable counts
+
+	CheckTestNames::reset_vars
+
+	foreach nm {paths} {
+	    set counts($nm,count) 0
+	}
+    }
+}
+
+# Arrange for Dejagnu to call CheckTestNames::check for each test result.
+foreach nm {pass fail xfail kfail xpass kpass unresolved untested \
+		unsupported} {
+    set local_record_procs($nm) "CheckTestNames::check"
+}
+
+# Create new global log_summary to replace Dejagnu's.
+proc log_summary { args } {
+    eval "CheckTestNames::do_log_summary $args"
+}
+
+# Create new global reset_vars to replace Dejagnu's.
+proc reset_vars {} {
+    eval "CheckTestNames::do_reset_vars"
+}
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index b72ce0cda7f..07e4ed83bc0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -29,6 +29,7 @@ load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
 load_lib memory.exp
+load_lib check-test-names.exp
 
 global GDB
 
-- 
2.25.3

[PATCHv3 2/3] gdb/testsuite: Detect and warn about duplicate test names

[Andrew Burgess]

Building on the previous commit, this patch detects when two tests
have the same test name and causes Dejagnu to print a new result type
'# of duplicate test names' in the result summary.  A line starting
with 'DUPLICATE: ' is also added to the gdb.sum and gdb.log files.

The DUPLICATE markers will be printed the second time a duplicate test
name is seen, and every time after that.  So you might see:

  PASS: gdb.base/sometest.exp: foo
  PASS: gdb.base/sometest.exp: bar
  PASS: gdb.base/sometest.exp: foo
  DUPLICATE: gdb.base/sometest.exp: foo
  PASS: gdb.base/sometest.exp: baz
  PASS: gdb.base/sometest.exp: foo
  DUPLICATE: gdb.base/sometest.exp: foo

However, the results will report a duplicate count of 1, indicating
that just one test name (foo) was duplicated.

Currently if the tests are run in parallel mode the new result type is
not merged into the combined summary file so users will need to run in
non-parallel mode to check this result.  Similarly, the 'DUPLICATE: '
markers will not be merged into the final gdb.sum file.  A later
commit will fix this.

gdb/testsuite/ChangeLog:

	* lib/check-test-names.exp (all_test_names): New module variable.
	(counts): Add 'duplicates' field.
	(_check_duplicates): New procedure.
	(check): Also check for duplicates.
	(do_log_summary): Print duplicates count.
	(do_reset_vars): Reset counter for duplicate test names, and
	discard all know test names.
---
 gdb/testsuite/ChangeLog                | 10 ++++++
 gdb/testsuite/lib/check-test-names.exp | 44 ++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
index 9a35ce7c5db..e07e2f1426f 100644
--- a/gdb/testsuite/lib/check-test-names.exp
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -19,13 +19,18 @@
 # to compare results between two runs of GDB from different trees.
 
 namespace eval ::CheckTestNames {
-    # An associative array of counts of tests that include a path in their
-    # test name.  There are two counts, 'count', which counts occurrences
-    # within a single variant run, and 'total', which counts across all
-    # variants.
+    # An associative array of all test names to the number of times each
+    # name is seen.  Used to detect duplicate test names.
+    variable all_test_names
+    array set all_test_names {}
+
+    # An associative array of counts of tests that either include a path in
+    # their test name, or have a duplicate test name.  There are two counts
+    # for each issue, 'count', which counts occurrences within a single
+    # variant run, and 'total', which counts across all variants.
     variable counts
     array set counts {}
-    foreach nm {paths} {
+    foreach nm {paths duplicates} {
 	set counts($nm,count) 0
 	set counts($nm,total) 0
     }
@@ -54,6 +59,25 @@ namespace eval ::CheckTestNames {
 	return false
     }
 
+    # Check if MESSAGE is a duplicate, if it is then increment the
+    # duplicates counter and return true, otherwise, return false.
+    proc _check_duplicates { message } {
+	variable all_test_names
+
+	# Initialise a count, or increment the count for this test name.
+	if {![info exists all_test_names($message)]} {
+	    set all_test_names($message) 0
+	} else {
+	    if {$all_test_names($message) == 0} {
+		inc_count duplicates
+	    }
+	    incr all_test_names($message)
+	    return true
+	}
+
+	return false
+    }
+
     # Remove the leading Dejagnu status marker from MESSAGE, and
     # return the remainder of MESSAGE.  A status marker is something
     # like 'PASS: '.  It is assumed that MESSAGE does contain such a
@@ -82,6 +106,10 @@ namespace eval ::CheckTestNames {
 	if [ _check_paths $message ] {
 	    clone_output "PATH: $message"
 	}
+
+	if [ _check_duplicates $message ] {
+	    clone_output "DUPLICATE: $message"
+	}
     }
 
     # If COUNT is greater than zero, disply PREFIX followed by COUNT.
@@ -109,17 +137,21 @@ namespace eval ::CheckTestNames {
 
 	maybe_show_count "# of paths in test names\t" \
 	    $counts(paths,$which)
+	maybe_show_count "# of duplicate test names\t" \
+	    $counts(duplicates,$which)
     }
 
     # Rename Dejagnu's reset_vars procedure, and create do_reset_vars to
     # replace it.  We arrange to have do_reset_vars called later.
     rename ::reset_vars reset_vars
     proc do_reset_vars {} {
+	variable all_test_names
 	variable counts
 
 	CheckTestNames::reset_vars
 
-	foreach nm {paths} {
+	unset all_test_names
+	foreach nm {paths duplicates} {
 	    set counts($nm,count) 0
 	}
     }
-- 
2.25.3

[PATCHv3 3/3] contrib: Handle GDB specific test result types

[Andrew Burgess]

NOTE: I know that this patch needs to go through the GCC list and then
be back-ported to the binutils-gdb repository.  I'm posting it here
for completeness while the other patches in this series are
discussed.  If the general idea is approved for GDB then I'll get this
merged through the GCC route.

When running Dejagnu on GDB we can now (sometimes) see two additional
test result types, these are '# of paths in test names' and '# of
duplicate test names'.

If the test is run in parallel mode (make -j...) then these extra test
results will appear in the individual test summary files, but are not
merged into the final summary file.

Additionally, two new types of test summary line can be printed, these
are 'PATH: ...' and 'DUPLICATE: ....' these lines allow users to
quickly search the test summary to track down where the offending test
names are.  These lines are similarly not merged into the unified
gdb.sum file after a parallel test run.

This commit adds support to the merge scripts to carry over these
extra result types, and the corresponding test status lines.

contrib/ChangeLog:

	* dg-extract-results.py: Handle GDB specific test types.
	* dg-extract-results.sh: Likewise.
---
 contrib/ChangeLog             |  5 +++++
 contrib/dg-extract-results.py |  6 ++++--
 contrib/dg-extract-results.sh | 12 +++++++++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/contrib/dg-extract-results.py b/contrib/dg-extract-results.py
index 7100794d42a..30aa68771d4 100644
--- a/contrib/dg-extract-results.py
+++ b/contrib/dg-extract-results.py
@@ -117,7 +117,7 @@ class Prog:
         self.tool_re = re.compile (r'^\t\t=== (.*) tests ===$')
         self.result_re = re.compile (r'^(PASS|XPASS|FAIL|XFAIL|UNRESOLVED'
                                      r'|WARNING|ERROR|UNSUPPORTED|UNTESTED'
-                                     r'|KFAIL|KPASS):\s*(.+)')
+                                     r'|KFAIL|KPASS|PATH|DUPLICATE):\s*(.+)')
         self.completed_re = re.compile (r'.* completed at (.*)')
         # Pieces of text to write at the head of the output.
         # start_line is a pair in which the first element is a datetime
@@ -143,7 +143,9 @@ class Prog:
             '# of known failures\t\t',
             '# of untested testcases\t\t',
             '# of unresolved testcases\t',
-            '# of unsupported tests\t\t'
+            '# of unsupported tests\t\t',
+            '# of paths in test names\t',
+            '# of duplicate test names\t'
         ]
         self.runs = dict()
 
diff --git a/contrib/dg-extract-results.sh b/contrib/dg-extract-results.sh
index f948088370e..ff6c50d029c 100755
--- a/contrib/dg-extract-results.sh
+++ b/contrib/dg-extract-results.sh
@@ -326,7 +326,7 @@ BEGIN {
   }
 }
 /^\t\t=== .* ===$/ { curvar = ""; next }
-/^(PASS|XPASS|FAIL|XFAIL|UNRESOLVED|WARNING|ERROR|UNSUPPORTED|UNTESTED|KFAIL|KPASS):/ {
+/^(PASS|XPASS|FAIL|XFAIL|UNRESOLVED|WARNING|ERROR|UNSUPPORTED|UNTESTED|KFAIL|KPASS|PATH|DUPLICATE):/ {
   testname=\$2
   # Ugly hack for gfortran.dg/dg.exp
   if ("$TOOL" == "gfortran" && testname ~ /^gfortran.dg\/g77\//)
@@ -400,6 +400,7 @@ BEGIN {
   variant="$VAR"
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kpasscnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0;
+  pathcnt=0; dupcnt=0
   curvar=""; insummary=0
 }
 /^Running target /		{ curvar = \$3; next }
@@ -414,6 +415,8 @@ BEGIN {
 /^# of untested testcases/	{ if (insummary == 1) untstcnt += \$5; next; }
 /^# of unresolved testcases/	{ if (insummary == 1) unrescnt += \$5; next; }
 /^# of unsupported tests/	{ if (insummary == 1) unsupcnt += \$5; next; }
+/^# of paths in test names/	{ if (insummary == 1) pathcnt += \$7; next; }
+/^# of duplicate test names/	{ if (insummary == 1) dupcnt += \$6; next; }
 /^$/				{ if (insummary == 1)
 				    { insummary = 0; curvar = "" }
 				  next
@@ -431,6 +434,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
@@ -452,6 +457,7 @@ cat << EOF > $TOTAL_AWK
 BEGIN {
   tool="$TOOL"
   passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0
+  pathcnt=0; dupcnt=0
 }
 /^# of DejaGnu errors/		{ dgerrorcnt += \$5 }
 /^# of expected passes/		{ passcnt += \$5 }
@@ -463,6 +469,8 @@ BEGIN {
 /^# of untested testcases/	{ untstcnt += \$5 }
 /^# of unresolved testcases/	{ unrescnt += \$5 }
 /^# of unsupported tests/	{ unsupcnt += \$5 }
+/^# of paths in test names/	{ pathcnt += \$7 }
+/^# of duplicate test names/	{ dupcnt += \$6 }
 END {
   printf ("\n\t\t=== %s Summary ===\n\n", tool)
   if (dgerrorcnt != 0) printf ("# of DejaGnu errors\t\t%d\n", dgerrorcnt)
@@ -475,6 +483,8 @@ END {
   if (untstcnt != 0) printf ("# of untested testcases\t\t%d\n", untstcnt)
   if (unrescnt != 0) printf ("# of unresolved testcases\t%d\n", unrescnt)
   if (unsupcnt != 0) printf ("# of unsupported tests\t\t%d\n", unsupcnt)
+  if (pathcnt != 0) printf ("# of paths in test names\t%d\n", pathcnt)
+  if (dupcnt != 0) printf ("# of duplicate test names\t%d\n", dupcnt)
 }
 EOF
 
-- 
2.25.3

Re: [PATCHv3 0/3] Automatic detection of test name problems

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Changes since v2:
Andrew>   - Now print PATH: .... or DUPLICATE: .... into the summary and log
Andrew>     files when an offending test name is seen.

Andrew>   - The dg-extract-results.* script merge these new status lines into
Andrew>     the single unified file when running tests in parallel.

FWIW this all seems good to me.

Tom

Re: [PATCHv3 0/3] Automatic detection of test name problems

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-04-30 12:20:08 +0100]:

> Changes since v2:
> 
>   - Now print PATH: .... or DUPLICATE: .... into the summary and log
>     files when an offending test name is seen.
> 
>   - The dg-extract-results.* script merge these new status lines into
>     the single unified file when running tests in parallel.
> 
> All feedback welcome,

I've now pushed patches #1 and #2 from this series.  Patch #3 I will
post to the GCC list in order to get it merged.  Then I'll back-port
to our repository after that.

Thanks,
Andrew

Re: [PATCHv3 0/3] Automatic detection of test name problems

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-05-11 22:30:17 +0100]:

> * Andrew Burgess <andrew.burgess@embecosm.com> [2020-04-30 12:20:08 +0100]:
> 
> > Changes since v2:
> > 
> >   - Now print PATH: .... or DUPLICATE: .... into the summary and log
> >     files when an offending test name is seen.
> > 
> >   - The dg-extract-results.* script merge these new status lines into
> >     the single unified file when running tests in parallel.
> > 
> > All feedback welcome,
> 
> I've now pushed patches #1 and #2 from this series.  Patch #3 I will
> post to the GCC list in order to get it merged.  Then I'll back-port
> to our repository after that.

Apologies!

I confess that I never actually tested patches #1 and #2 without patch
#3 also being present.  I (stupidly) assumed that any unknown count
lines would just be ignored by the dg-extract-results scripts.  Turns
out they are not.

I have now pushed the patch below as a short term bridge until the
dg-extract-results script changes are merged.

The patch below disables my new tests when running the gdb tests in
parallel, but does allow them when running non-parallel.

As soon as the dg-extract-results changes are in place I will revert
this patch.

Sorry for any broken test runs since I pushed my changes.

Thanks,
Andrew

---

commit 843f4d93576eef02139f7b1b3fa1cea7b0f286f1
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue May 12 17:38:17 2020 +0100

    gdb/testsuite: Disable path and duplicate checks when parallel testing
    
    This commit disables the recently added checking for paths in test
    names, and for duplicate test names, when the gdb tests are run in
    parallel.
    
    When running the gdb tests in parallel the extra result count lines
    produced cause the dg-extract-results scripts to exit with an error.
    
    The patches for the dg-extract-results scripts have been posted to the
    gcc-patches mailing list here:
    
    https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545562.html
    
    Once they are merged there then these changes can be merged over to
    binutils-gdb, and this commit can be reverted.
    
    gdb/testsuite/ChangeLog:
    
            * lib/check-test-names.exp: Disable when testing is being run in
            parallel.

diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
index 4c0fde6e4ea..79139adea7a 100644
--- a/gdb/testsuite/lib/check-test-names.exp
+++ b/gdb/testsuite/lib/check-test-names.exp
@@ -18,6 +18,26 @@
 # name.  When a test includes the path in its test name it is harder
 # to compare results between two runs of GDB from different trees.
 
+# This is a short term hack (12-May-2020).  If we are running tests in
+# parallel then we need support in the contrib/dg-extract-results.*
+# scripts to merge the new result types generated by this file back
+# into the single unified summary file.  If this support is not in
+# place then the dg-extract-results script will exit with an error.
+#
+# The script changes need to first be merged into the gcc repository,
+# then copied over to the binutils-gdb repository.  The required
+# changes have been posted to the gcc list here:
+#
+# https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545562.html
+#
+# But until these are merged into binutils-gdb the extra checks
+# offered by this file can only be done when the tests are not running
+# in parallel.
+if {[info exists GDB_PARALLEL]} {
+    # Don't load this file.
+    return
+}
+
 namespace eval ::CheckTestNames {
     # An associative array of all test names to the number of times each
     # name is seen.  Used to detect duplicate test names.

Re: [PATCHv3 2/3] gdb/testsuite: Detect and warn about duplicate test names

[Simon Marchi]

On 2020-04-30 7:20 a.m., Andrew Burgess wrote:
> Building on the previous commit, this patch detects when two tests
> have the same test name and causes Dejagnu to print a new result type
> '# of duplicate test names' in the result summary.  A line starting
> with 'DUPLICATE: ' is also added to the gdb.sum and gdb.log files.
> 
> The DUPLICATE markers will be printed the second time a duplicate test
> name is seen, and every time after that.  So you might see:
> 
>   PASS: gdb.base/sometest.exp: foo
>   PASS: gdb.base/sometest.exp: bar
>   PASS: gdb.base/sometest.exp: foo
>   DUPLICATE: gdb.base/sometest.exp: foo
>   PASS: gdb.base/sometest.exp: baz
>   PASS: gdb.base/sometest.exp: foo
>   DUPLICATE: gdb.base/sometest.exp: foo
> 
> However, the results will report a duplicate count of 1, indicating
> that just one test name (foo) was duplicated.
> 
> Currently if the tests are run in parallel mode the new result type is
> not merged into the combined summary file so users will need to run in
> non-parallel mode to check this result.  Similarly, the 'DUPLICATE: '
> markers will not be merged into the final gdb.sum file.  A later
> commit will fix this.

Hi Andrew,

When testing on Red Hat 7.8 (machine gcc135), I get this, which I think is related to this
change.

Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/O2_float_param.exp ...
can't unset "all_test_names": no such variable
    while executing
"unset all_test_names"

This machine has Dejagnu 1.5.1, which I suppose is the version that comes with this distro
version.

I'm kind of curious that this has not been reported before (or I missed it?).  Do you think it
would be easy to make this feature compatible with that version of Dejagnu?  Otherwise, could
we document the required version of dejagnu required to run the testsuite?

Simon

Re: [PATCHv3 2/3] gdb/testsuite: Detect and warn about duplicate test names

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2020-07-31 17:34:06 -0400]:

> On 2020-04-30 7:20 a.m., Andrew Burgess wrote:
> > Building on the previous commit, this patch detects when two tests
> > have the same test name and causes Dejagnu to print a new result type
> > '# of duplicate test names' in the result summary.  A line starting
> > with 'DUPLICATE: ' is also added to the gdb.sum and gdb.log files.
> > 
> > The DUPLICATE markers will be printed the second time a duplicate test
> > name is seen, and every time after that.  So you might see:
> > 
> >   PASS: gdb.base/sometest.exp: foo
> >   PASS: gdb.base/sometest.exp: bar
> >   PASS: gdb.base/sometest.exp: foo
> >   DUPLICATE: gdb.base/sometest.exp: foo
> >   PASS: gdb.base/sometest.exp: baz
> >   PASS: gdb.base/sometest.exp: foo
> >   DUPLICATE: gdb.base/sometest.exp: foo
> > 
> > However, the results will report a duplicate count of 1, indicating
> > that just one test name (foo) was duplicated.
> > 
> > Currently if the tests are run in parallel mode the new result type is
> > not merged into the combined summary file so users will need to run in
> > non-parallel mode to check this result.  Similarly, the 'DUPLICATE: '
> > markers will not be merged into the final gdb.sum file.  A later
> > commit will fix this.
> 
> Hi Andrew,
> 
> When testing on Red Hat 7.8 (machine gcc135), I get this, which I think is related to this
> change.
> 
> Running /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/O2_float_param.exp ...
> can't unset "all_test_names": no such variable
>     while executing
> "unset all_test_names"
> 
> This machine has Dejagnu 1.5.1, which I suppose is the version that comes with this distro
> version.

`all_test_names` is local to gdb/testsuite/lib/check-test-names.exp,
so shouldn't depend on the version of dejagnu being run.

I tried installing version 1.5.1 of dejagnu, and as expected that
works fine.

My guess is that this is either a problem with an older version of
tcl, or maybe expect (?)

Or else, there's something specific about exactly what test you're
running, with the particular settings you're using that's triggering
this issue.

Can you include the output of `runtest --version` so we can see which
tcl/expect are being used, and provide the full `make check` command
you're running.

Thanks,
Andrew

Re: [PATCHv3 2/3] gdb/testsuite: Detect and warn about duplicate test names

[Simon Marchi]

On 2020-08-03 6:02 a.m., Andrew Burgess wrote:
> `all_test_names` is local to gdb/testsuite/lib/check-test-names.exp,
> so shouldn't depend on the version of dejagnu being run.
> 
> I tried installing version 1.5.1 of dejagnu, and as expected that
> works fine.
> 
> My guess is that this is either a problem with an older version of
> tcl, or maybe expect (?)
> 
> Or else, there's something specific about exactly what test you're
> running, with the particular settings you're using that's triggering
> this issue.
> 
> Can you include the output of `runtest --version` so we can see which
> tcl/expect are being used, and provide the full `make check` command
> you're running.

Ah apologies for reaching a wrong conclusion, I sent this quite in a hurry.

Here it is:

[simark@gcc135 gdb]$ runtest --version
WARNING: Couldn't find the global config file.
Expect version is       5.45
Tcl version is          8.5
Framework version is    1.5.1

I'm simply running "make check".

Simon