This is the mail archive of the binutils@sourceware.cygnus.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: PATCH for 64-bit MIPS ELF buglets


>>>>> "Ian" == Ian Lance Taylor <ian@zembu.com> writes:

    Ian> I'm afraid that there isn't any documentation on those
    Ian> relocations.  I just wrote them so that they worked.  I can
    Ian> probably answer any questions you have.

Oh, dear.  OK.

    Ian> The mips16 jump instruction is weird because the address is
    Ian> stored in a permuted format so that it can be picked up by
    Ian> the 16 bit instruction decoder.

Thanks for the hint.

    Ian> Some of the mips16 stub support seems to have disappeared as
    Ian> well.  That all needs to get restored too.

I'll do my best.  In retrospect, using R_MIPS_64 for two different
relocations was probably not an ideal design choice; in the future, we
should probably try to use non-standard relocations names for
non-standard relocations.  (As you did for R_MIPS_16*.)  

Of course, sometimes we have to deal with other tools/ABIs that do
things in less than ideal ways. :-)

    Ian> I find a case like omitting mips16 support pretty scary,
    Ian> because there isn't much testing for this sort of thing, so
    Ian> it is likely to get caught months later by somebody who has
    Ian> no idea what has happened to the code.  

The scariest thing is that there is no testing of this functionality.
That's what makes it so easy to break.  The more people work on
binutils, the lower will be the percentage of people with your skill
and experience and the harder it will be for you to verify every
change.  In GCC, we tend to break these kinds of corner cases/obscure
platforms relatively often.  I don't think that's because we're bad
engineers; it's just what happens.  We use release cycles for
stabilization.  On the C++ side, we add regression tests for almost
every bug fix.  That practice has really made a huge difference, and
markedly reduces the amount of "this program used to work with 1.x but
doesn't with 2.x" kind of bug reports we see.

Perhaps it would be worthwhile for someone to produce some .o's that
could become part of the testsuite.  If ABI's are stable, then .o's
should continue to work, and there could actually be tests comparing
the output of a link to a fixed image.  (Of course, there'd have to be
some fudging for the things that *are* allowed to change, but it seems
like we could probably things up to at least test the basic
functionality.)  For example, in this case, testing that R_MIPS16_*
didn't cause an assertion failure/abort would probably have worked.
Better would be to put the relocation target at an absolute address
(via a linker script) and then verify that the relocated contents are
correct.  It should be relatively simple to right a program (using
BFD) that opens an executable image, and examines the value at a fixed
location in a particular section.  I bet that regression tests using
such a framework would do a *ton* towards avoiding problems like this
in the future.

Note that I'm not suggesting you spend your time on this; I'm sure
you're too busy.  But, perhaps Cygnus, or someone else with a strong
interest in these under-tested platforms, would be motivated to work
on such platform-specific test-suites.  We would certainly be willing
to work on this, if anyone is interested in contracting the work.

    Ian> I assumed that you broke apart the functions without changing
    Ian> the functionality; had I noticed that you were actually
    Ian> changing what they did, I would not have approved the patch.

I'm sorry the patch description was unclear.  The way in which the
relocation values was calculated was not conducive to the
multiple-relocs-at-same-location bit in the MIPS ABI, so some
considerable changes were necessitated.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]