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

[Andrew Burgess <andrew.burgess@embecosm.com>]

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