This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 06/15] MIPS/GAS/test: Adjust LD for multi-target testing
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