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

[Keith Seitz <keiths@redhat.com>]

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