Re: [PATCH 06/15] MIPS/GAS/test: Adjust LD for multi-target testing

["Maciej W. Rozycki" <macro@linux-mips.org>]

On Sun, 10 Oct 2010, Richard Sandiford wrote:

> The patch seems to contain some parts that make the matching more strict
> and some that make it less so.  The former look good, but I'm less sure
> about the latter.  For example, with something like:
> 
> [0-9a-f]+ <[^>]*> lw   a0,(0|4096)\(at\)
> [      ]*[0-9a-f]+: [A-Z0-9_]*LO[A-Z0-9_]*     \.data(\+0xfffff000)?
> 
> the difference seems relatively important: on some targets the addend
> must be in-place, on some it mustn't be.  It might make more sense
> to have separate dumps for the two cases.  However, that sort of
> difference is probably picked up by other tests too, so the risk
> might not be great.

 This is certainly strictier than the original that for the record was:

[0-9a-f]+ <[^>]*> lw	a0,0\(at\)
[ 	]*[0-9a-f]+: [A-Z0-9_]*LO[A-Z0-9_]*	.data.*

(and obviously failed for ECOFF because of the different in-place addend).

 I have actually weighed the strictness of the patterns used against 
long-time maintenance effort and I think my proposal is a reasonable 
compromise.  The probability of the addend (the in-place one and/or one in 
the relocation) being wrong exactly such that it would still match all the 
patterns is minimal (which, under the appropriate Murphy's law, is 
guaranteed to happen, I know; or does it only apply to the impossible? ;) 
).  Having a separate dump for ECOFF is OTOH guaranteed to leave it in 
neglection since the moment I will have committed it.  Having the dumps 
combined gives a good chance the ECOFF variant of the testcases involved 
will stay stay either in the working order or close to for a while.

 As observed with some other changes made in this series, ECOFF testsuite 
failures are mainly if not exclusively caused by bitrot in the testsuite 
itself rather than from actual code of binutils malfunctioning.  
Regrettably I found no way to avoid creating a couple of ECOFF-specific 
dumps in this series, but for the reason stated I'd like to avoid them as 
much as possible.

 As a side note, I'd be more than happy to come up with a solution that 
could let us make exact matches of addresses dumped without sacrificing 
the ability to insert or delete pieces of test cases while keeping the 
diffs applied to dumps readable.  What I have in mind is an expression 
that would match an address printed on the previous (or an earlier) line, 
possibly offset by a constant specified in the dump.  That would require 
changes to the test framework more substantial than a small adjustment, so 
while I'm keeping this enhancement in my mind, I don't feel like I have 
resources available at hand (time, in particular) to dig into it.

> Also, how confident are we that the more relaxed tests are correct
> for ECOFF?  You say:
> 
> > 2. Consequently section alignment is requested at 4k so that offsets are 
> >    round and do not change for ECOFF when instructions are added or 
> >    removed (ECOFF seems to make data relocations relative to .text).
> 
> but is that intentional?

 I haven't investigated the way ECOFF relocations work and my knowledge of 
the executable format consists of the notion that it exists.  Someone has 
added these trailing .* regexps though, presumably legitimately (I would 
be ridiculous to assume I'm the only one sane, although sometimes my 
confidence is weak :( ).

> The point here, and here:
> 
> > 3. Missing "$" prefixes are added to the names of FP registers (shows how 
> >    frequently some of the targets are tested, hmm...).
> 
> is precisely that mips*-ecoff has severely bitrotted from lack of
> active interest, so who knows how good the current output actually is?
> There's a danger in simply making the testsuite match what the current
> ECOFF assembler does.

 These "$" prefixes were actually missing for ELF targets, specifically 
mipstx39*-*-* (the only target setting ${gpr_ilocks}); being embedded 
rather than workstation class devices, I wouldn't expect Toshiba chips in 
any ECOFF systems.

> In a similar vein, the difference between endiannesses is something
> that could easily go wrong, but there are separate tests that should
> pick that up.  I'm much more comfortable with the endianness bits.

 We have this endianness leeway in many testcases.  I think it's better to 
have it than to hardcode endianness in the cases, making the the other 
endianness completely untested.  Again, it *might* be possible to improve 
the test framework to dynamically select between alternatives, defined in 
some way, based on the default target's endianness, but it looks all but 
trivial to me.

> Overall the patch is improvement, so it's OK to apply despite
> the concerns above.

 Applied, thanks.  I think my changes are not of the kind that would 
discourage further improvements.

  Maciej