[PATCH] MIPS: Enable NewABI tests for SDE targets

[PATCH] MIPS: Enable NewABI tests for SDE targets

[Maciej W. Rozycki]

[-- Attachment #1: Type: text/plain, Size: 4489 bytes --]

Hi,

 As easily observed in config.bfd SDE targets support NewABI these days as 
well and therefore need testing.  The change below enables NewABI tests 
for GAS and LD.

 Regrettably this turned out not as straightforward as it would seem, 
especially for LD, as there is assumption about The NewABI Target being 
Linux made in a few places.  The change therefore makes additional changes 
to make this assumption go away.  As a side effect some IRIX failures have 
gone away.

 These additional changes required updates to the LD version of 
run_dump_test -- I carried over features added to the GAS version of same, 
specifically the optional EXTRA_OPTIONS argument and the "dump" keyword.  
Then mips-elf.exp makes use of these changes to abstract the handling of 
GAS and LD options across targets.

 In the course of developing these changes the "dump" keyword turned out 
unneeded, but I propose to keep it for consistency between the two 
implementations of run_dump_test, especially as it looks reasonable to use 
by targets whose maintainers prefer to bulk-run dump tests through a glob 
pattern -- that obviously precludes calling the same test with 
EXTRA_OPTIONS depending on configuration as I do here.

 The rest of the changes are the usual padding/alignment fixes as 
bare-iron MIPS targets align known sections to 4 while other MIPS targets 
use 8.

 I have regression-tested this using Alan's clever script (thanks, Alan!), 
although with more interesting MIPS targets added (these included 
mips-freebsd, mips64el-freebsd, mips-sgi-irix5 and mips-sgi-irix6 in 
particular as these use incompatible LD emulations).  This produced no 
regressions and the following progressions:

mips-sgi-irix6  -FAIL: MIPS ELF got reloc n32
mips-sgi-irix6  -FAIL: MIPS ELF xgot reloc n32
mips-sgi-irix6  -FAIL: MIPS ELF got reloc n64
mips-sgi-irix6  -FAIL: MIPS ELF xgot reloc n64
mips-sgi-irix6  -ERROR:  -mips32r2 .../ld/testsuite/ld-mips-elf/mips16-call-global-1.s: assembly failed
mips-sgi-irix6  -ERROR:  -mips32r2 .../ld/testsuite/ld-mips-elf/mips16-intermix-1.s: assembly failed

 Alan, while at it -- may I suggest that you add UNRESOLVED to the list of 
test results watched?  According to DejaGNU/POSIX test suite standard 
documentation that test result means there is a problem with the test case 
itself and such should not be missed.  I've seen it happen where an 
exception was thrown by the TCL interpreter for example because a 
reference to an uninitialised variable was made and the appearance (or 
removal) of such problems should be highlighted when regression testing.  
I have such change applied to my copy of your script already.

2012-08-01  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/testsuite/
	* gas/mips/mips.exp: Set has_newabi for mips*-sde-elf* too.
	* gas/mips/elf-rel-got-n32.s: Adjust padding.
	* gas/mips/elf-rel-got-n64.s: Likewise.
	* gas/mips/elf-rel23.s: Likewise.
	* gas/mips/elf-rel28.s: Likewise.
	* gas/mips/n32-consec.s: Likewise.
	* gas/mips/elf-rel-xgot-n32.d: Adjust output expected.
	* gas/mips/elf-rel-xgot-n64.d: Likewise.
	* gas/mips/elf-rel23.d: Likewise.
	* gas/mips/elf-rel23a.d: Likewise.
	* gas/mips/elf-rel23b.d: Likewise.
	* gas/mips/elf-rel28-n32.d: Likewise.

	ld/testsuite/
	* ld-mips-elf/emit-relocs-1a.s: Make section alignment uniform 
	across targets.
	* ld-mips-elf/emit-relocs-1b.s: Likewise.
	* ld-mips-elf/jalbal.s: Adjust padding.
	* ld-mips-elf/elf-rel-got-n32-embed.d: New test.
	* ld-mips-elf/elf-rel-got-n64-embed.d: New test.
	* ld-mips-elf/elf-rel-xgot-n32-embed.d: New test.
	* ld-mips-elf/elf-rel-xgot-n64-embed.d: New test.
	* ld-mips-elf/elf-rel-got-n32.d: Remove -melf32btsmipn32.
	* ld-mips-elf/elf-rel-got-n64.d: Remove -melf64btsmip.  Adjust
	output.
	* ld-mips-elf/elf-rel-got-n64-linux.d: Remove -melf64btsmip.
	* ld-mips-elf/elf-rel-xgot-n32.d: Remove -melf32btsmipn32.
	Adjust output.
	* ld-mips-elf/elf-rel-xgot-n64.d: Remove -melf64btsmip.  Adjust
	output.
	* ld-mips-elf/elf-rel-xgot-n64-linux.d: Likewise.
	* ld-mips-elf/reloc-1-n64.d: Remove -melf64btsmip.
	* ld-mips-elf/mips-elf.exp: Set has_newabi for mips*-sde-elf*
	too.  Move tool flags from o32_as_flags and o32_ld_flags
	variables into abi_asflags and abi_ldflags arrays.  Adjust test 
	cases run to use them.  Run the new tests.
	* lib/ld-lib.exp (run_dump_test): Implement the EXTRA_OPTIONS
	argument and the "dump" keyword.

 OK to apply?

  Maciej

binutils-mips-sde-test-newabi.diff
[Attached compressed due to its size.]

[-- Attachment #2: Type: application/octet-stream, Size: 16797 bytes --]

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

[Maciej W. Rozycki]

Hi,

> 	gas/testsuite/
> 	* gas/mips/mips.exp: Set has_newabi for mips*-sde-elf* too.
> 	* gas/mips/elf-rel-got-n32.s: Adjust padding.
> 	* gas/mips/elf-rel-got-n64.s: Likewise.
> 	* gas/mips/elf-rel23.s: Likewise.
> 	* gas/mips/elf-rel28.s: Likewise.
> 	* gas/mips/n32-consec.s: Likewise.
> 	* gas/mips/elf-rel-xgot-n32.d: Adjust output expected.
> 	* gas/mips/elf-rel-xgot-n64.d: Likewise.
> 	* gas/mips/elf-rel23.d: Likewise.
> 	* gas/mips/elf-rel23a.d: Likewise.
> 	* gas/mips/elf-rel23b.d: Likewise.
> 	* gas/mips/elf-rel28-n32.d: Likewise.
> 
> 	ld/testsuite/
> 	* ld-mips-elf/emit-relocs-1a.s: Make section alignment uniform 
> 	across targets.
> 	* ld-mips-elf/emit-relocs-1b.s: Likewise.
> 	* ld-mips-elf/jalbal.s: Adjust padding.
> 	* ld-mips-elf/elf-rel-got-n32-embed.d: New test.
> 	* ld-mips-elf/elf-rel-got-n64-embed.d: New test.
> 	* ld-mips-elf/elf-rel-xgot-n32-embed.d: New test.
> 	* ld-mips-elf/elf-rel-xgot-n64-embed.d: New test.
> 	* ld-mips-elf/elf-rel-got-n32.d: Remove -melf32btsmipn32.
> 	* ld-mips-elf/elf-rel-got-n64.d: Remove -melf64btsmip.  Adjust
> 	output.
> 	* ld-mips-elf/elf-rel-got-n64-linux.d: Remove -melf64btsmip.
> 	* ld-mips-elf/elf-rel-xgot-n32.d: Remove -melf32btsmipn32.
> 	Adjust output.
> 	* ld-mips-elf/elf-rel-xgot-n64.d: Remove -melf64btsmip.  Adjust
> 	output.
> 	* ld-mips-elf/elf-rel-xgot-n64-linux.d: Likewise.
> 	* ld-mips-elf/reloc-1-n64.d: Remove -melf64btsmip.
> 	* ld-mips-elf/mips-elf.exp: Set has_newabi for mips*-sde-elf*
> 	too.  Move tool flags from o32_as_flags and o32_ld_flags
> 	variables into abi_asflags and abi_ldflags arrays.  Adjust test 
> 	cases run to use them.  Run the new tests.
> 	* lib/ld-lib.exp (run_dump_test): Implement the EXTRA_OPTIONS
> 	argument and the "dump" keyword.

 Regrettably the next change I was about to submit uncovered a couple of
regressions affecting little-endian targets.  They are fixed with the 
update below.  No further issues found.  I have further widened MIPS 
target coverage now.

 For the record -- I'll be testing these MIPS targets with submissions 
from now on: mips-ecoff mips-elf mips-sde-elf mips-sgi-irix5 
mips-sgi-irix6 mips-freebsd mips-linux mips64-freebsd mips64-linux 
mips64el-freebsd mips64el-linux mipsel-ecoff mipsel-elf mipsel-freebsd 
mipsel-linux mipsisa32-elf mipsisa32-linux mipsisa32el-elf 
mipsisa32el-linux mipsisa64-elf mipsisa64-linux mipsisa64el-elf 
mipsisa64el-linux -- have I missed anything important here?

2012-08-01  Maciej W. Rozycki  <macro@codesourcery.com>

	ld/testsuite/
	* ld-mips-elf/reloc-1-n32.d: Add -EB to GAS flags.

 No need to update mips-elf.exp's entry as the update is intended to be 
merged with the original change.

  Maciej

binutils-mips-sde-test-newabi-el.diff
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/ld/testsuite/ld-mips-elf/mips-elf.exp	2012-08-01 17:20:56.000000000 +0100
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/mips-elf.exp	2012-08-01 17:41:53.621165721 +0100
@@ -357,14 +357,14 @@ if { $has_newabi } {
 	[list \
 	    "reloc test 6a" \
 	    "-shared $abi_ldflags(n32)" \
-	    "-n32" \
+	    "$abi_asflags(n32)" \
 	    "reloc-6a.s" \
 	    {} \
 	    "reloc-6a.so"] \
 	[list \
 	    "reloc test 6b" \
 	    "$abi_ldflags(n32) tmpdir/reloc-6a.so" \
-	    "-n32" \
+	    "$abi_asflags(n32)" \
 	    "reloc-6b.s" \
 	    {} \
 	    "reloc-6b"]]
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/reloc-1-n32.d
===================================================================
--- binutils-fsf-trunk-quilt.orig/ld/testsuite/ld-mips-elf/reloc-1-n32.d	2012-07-21 05:03:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/reloc-1-n32.d	2012-08-01 17:40:56.580512731 +0100
@@ -1,5 +1,5 @@
-#source: reloc-1a.s -mabi=n32
-#source: reloc-1b.s -mabi=n32
+#source: reloc-1a.s -mabi=n32 -EB
+#source: reloc-1b.s -mabi=n32 -EB
 #ld: -r
 #readelf: --relocs
 

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

[Richard Sandiford]

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

Richard

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

[Maciej W. Rozycki]

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

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

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 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

Not sure about that.  "foreach { a b .. } ..." is provided exactly to make
flat lists easy to handle.  [string map ...] takes a flat list rather than
a list of lists.  Flat lists are also what is used by [array set ...]
and [array get ...], so that one easy way of handling unordered key-value
list arguments is:

    array set foo $argument
    if { [info exists $foo(key)] } ...

If you're lucky enough to be able to rely on Tcl 8.5, then dict operates
in just the same way.  Flat lists can even be used as dicts directly,
without any conversion.

I don't know of any native Tcl operation that uses lists of lists instead.

> However please note that implementing your suggestion will make the
> two run_dump_test implementations confusingly different -- do you
> still want me to proceed?

That's a clincher though.  Please go with your original patch.

Richard

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

[Maciej W. Rozycki]

On Sat, 4 Aug 2012, Richard Sandiford wrote:

> >  A list of lists seems more in the spirit of TCL to me, while a vector is 
> > more C-like
> 
> Not sure about that.  "foreach { a b .. } ..." is provided exactly to make
> flat lists easy to handle.  [string map ...] takes a flat list rather than
> a list of lists.  Flat lists are also what is used by [array set ...]
> and [array get ...], so that one easy way of handling unordered key-value
> list arguments is:
> 
>     array set foo $argument
>     if { [info exists $foo(key)] } ...
> 
> If you're lucky enough to be able to rely on Tcl 8.5, then dict operates
> in just the same way.  Flat lists can even be used as dicts directly,
> without any conversion.
> 
> I don't know of any native Tcl operation that uses lists of lists instead.

 They're easily traversed and many examples in the reference manual use 
nested lists, but your arguments sound convincing to me, I agree.

> > However please note that implementing your suggestion will make the
> > two run_dump_test implementations confusingly different -- do you
> > still want me to proceed?
> 
> That's a clincher though.  Please go with your original patch.

 I have therefore applied it now (and the update, although separately, 
sorry about that), thanks for your review.  Let's wait for some rainy 
evening to get the two procedures converted to a flat list.

  Maciej