Re: [PATCH] MIPS: Enable NewABI tests for SDE targets

["Maciej W. Rozycki" <macro@codesourcery.com>]

On Thu, 2 Aug 2012, Richard Sandiford wrote:

> > Index: binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp
> > ===================================================================
> > --- binutils-fsf-trunk-quilt.orig/ld/testsuite/lib/ld-lib.exp	2012-07-24 15:29:41.000000000 +0100
> > +++ binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp	2012-07-26 03:08:32.951798913 +0100
> > @@ -431,7 +431,7 @@ proc ld_simple_link_defsyms {} {
> >      return $flags
> >  }
> > 
> > -# run_dump_test FILE
> > +# run_dump_test FILE (optional:) EXTRA_OPTIONS
> >  # Copied from gas testsuite, tweaked and further extended.
> >  #
> >  # Assemble a .s file, then run some utility on it and check the output.
> > @@ -456,6 +456,12 @@ proc ld_simple_link_defsyms {} {
> >  # list ends with the first line that doesn't match the above syntax
> >  # (hmm, not great for error detection).
> >  #
> > +# The optional EXTRA_OPTIONS argument to `run_dump_test' is a list of
> > +# two-element lists.  The first element of each is an option name, and
> > +# the second additional arguments to be added on to the end of the
> > +# option list as given in FILE.d.  (If omitted, no additional options
> > +# are added.)
> > +#
> >  # The interesting options are:
> >  #
> >  #   name: TEST-NAME
> > @@ -503,6 +509,11 @@ proc ld_simple_link_defsyms {} {
> >  #       More than one "source" directive can be given, which is useful
> >  #       when testing linking.
> >  #
> > +#   dump: DUMP
> > +#	Match against DUMP.d.  If omitted, this defaults to FILE.d.  This
> > +#	is useful if several .d files differ by options only.  Options are
> > +#	always read from FILE.d.
> > +#
> >  #   xfail: TARGET
> >  #       The test is expected to fail on TARGET.  This may occur more than
> >  #       once.
> > @@ -534,7 +545,7 @@ proc ld_simple_link_defsyms {} {
> >  # regexps in FILE.d.  `regexp_diff' is defined in binutils-common.exp;
> >  # see further comments there.
> >  #
> > -proc run_dump_test { name } {
> > +proc run_dump_test { name {extra_options {}} } {
> >      global subdir srcdir
> >      global OBJDUMP NM AS OBJCOPY READELF LD
> >      global OBJDUMPFLAGS NMFLAGS ASFLAGS OBJCOPYFLAGS READELFFLAGS LDFLAGS
> 
> I think extra_options ought to be a one-level list, i.e.
> {key1 value key2 value2 ...}.  That makes the calls simpler
> (because there's one fewer [list ...]) and:
> 
> > +    foreach i $extra_options {
> > +	set opt_name [lindex $i 0]
> > +	set opt_val [lindex $i 1]
> > +	if ![info exists opts($opt_name)] {
> > +	    perror "unknown option $opt_name given in extra_opts"
> > +	    unresolved $subdir/$name
> > +	    return
> > +	}
> > +	# Add extra option to end of existing option, adding space
> > +	# if necessary.
> > +	if { ![regexp "warning|error" $opt_name]
> > +	     && [string length $opts($opt_name)] } {
> > +	    append opts($opt_name) " "
> > +	}
> > +	append opts($opt_name) $opt_val
> > +    }
> 
> reduces to:
> 
>     foreach { opt_name opt_val } $extra_options {
> 	if ![info exists opts($opt_name)] {
> 	    perror "unknown option $opt_name given in extra_opts"
> 	    unresolved $subdir/$name
> 	    return
> 	}
> 	# Add extra option to end of existing option, adding space
> 	# if necessary.
> 	if { ![regexp "warning|error" $opt_name]
> 	     && [string length $opts($opt_name)] } {
> 	    append opts($opt_name) " "
> 	}
> 	append opts($opt_name) $opt_val
>     }
> 
> OK with that change, thanks.  OK for the follow-up too.

 A list of lists seems more in the spirit of TCL to me, while a vector is 
more C-like, but I have no strong preference either way.  However please 
note that implementing your suggestion will make the two run_dump_test 
implementations confusingly different -- do you still want me to proceed?  
If so, wouldn't it make sense to update the GAS version and any callers 
accordingly so that the two versions don't diverge?

 I wonder if the two couldn't simply be built around a common core moved 
over to binutils/testsuite/lib/binutils-common.exp instead.  There's not 
much difference really and TCL has ways do such stuff more easily than for 
example C.

 Also some calls to "subst" over key values, specifically "as" and "ld" 
(set opt_val [subst $opt_val]), look mysterious to me.  While they might 
be useful for options to have parts referring to TCL variables, this 
however is not going to be particularly useful given that they are 
executed in the context of run_dump_test.  It looks to me it would make 
more sense if they were executed in the caller's context with "uplevel".  
Do you or anybody know what the original purpose of this substitution was?

  Maciej