[PATCH] MIPS support for --hash-style=gnu
Maciej W. Rozycki
macro@imgtec.com
Thu Apr 21 01:19:00 GMT 2016
Neil, Nick --
Nick: I have included a request to you -- please see the second paragraph
below and feel free to ignore the rest of this e-mail.
> First, my most sincere apologies for somehow missing this when it was
> posted! My Outlook (blech) server side filters apparently decided to
> have a little lie down.
Well, the later you reply, the later you'll get a followup -- it's just
that simple. I'm glad that you've found the missing e-mail though.
> > Have you been able to sort out your copyright assignment paperwork with
> > FSF meanwhile?
>
> Sadly this remains stuck with Cisco's lawyers (despite repeated prodding).
> I think my next move will be to try to get them to assign the code to me
> so that I can assign it to the FSF. (Sigh.)
Jim's observations notwithstanding, if you find things going really bad,
then would perhaps a one-off copyright disclaimer for this piece of code
only be an option to consider? I suppose it might be easier to arrange
and my understanding is that, while not encouraged, this option is offered
by FSF to make contributions easier in cases like this, where getting an
assignment might be difficult.
Nick, as the head maintainer would you please confirm or deny and if this
is feasible provide Neil with the details? I don't have access to the
relevant FSF's resources myself.
> > One important point I need to make here is that for many environments it
> > is going to be necessary to keep a standard ELF hash section in binaries
> > along your newly introduced GNU hash section, because they will have to be
> > cooperative with the existing tools out there. This is indeed a standard
> > arrangement supported by GNU LD (with the `--hash-style=both' option) in
> > addition to producing binaries embedding a single kind of a hash section
> > of your choice. And this has already been used with other targets which
> > support using a GNU hash section. In fact I have previously experienced
> > problems myself in a configuration which stopped producing ELF hash
> > sections along GNU hash as that caused a tool, the prelinker (more on the
> > tool below), to stop working as it didn't support the GNU hash back then.
>
> I agree entirely. We have been running successfully with both sections
> present on builds with a loader that only understood the old hash as well
> as builds which understood both. So there is some reason to believe
> this "should" work. (I won't claim any extensive testing, though!)
Having reviewed your code I believe it will be safe to install once the
issues noted have been addressed, but I could put your change through
glibc regression testing myself if you are not set up for doing it, to
make sure that ELF hash support hasn't regressed and that GNU hash support
gives the same results. This is I believe the most comprehensive way of
testing dynamic loading we have available at hand.
> > Fair enough, however as a matter of interest -- have you actually
> > benchmarked the difference between your choice and a `.gnu.xhash' layout
> > where parts 4 and 5 are intermixed i.e.:
> >
> > struct {
> > Elf32_Word hashval;
> > Elf32_Word indxlat;
> > } xchains[ngnusyms];
> >
> > -- maybe in reality that doesn't matter that much, especially with a set
> > associative cache?
>
> Unfortunately, I have not. I stuck with a very simple extension to the existing
> section because I feared that, in my extreme ignorance, I would break
> something very subtle somewhere.
Ack, fair enough. Though obviously having performance figures for
comparison would be desirable even if the alternative implementation was
deemed too risky to deploy -- to know if we're losing or gaining anything
by making a particular choice.
> > Yes, you absolutely have to avoid alignment issues in 64-bit ELF, and
> > using DT_MIPS_SYMTABNO is the right choice -- the presence of this tag's
> > entry is mandatory in the dynamic structure, as per Figure 5-7: "Dynamic
> > Array Tags d_tag" in the MIPS psABI[1]. This entry is needed for the
> > dynamic linker, to process the global parts of the GOT and the dynamic
> > symbol table which are mapped to each other and the very cause of this all
> > hassle, so you can rely on it; DT_MIPS_SYMTABNO=symndx+ngnusyms.
>
> Now that I understand this all a bit better, I tend to agree. The only, very
> theoretical, counterargument is that this would tie .gnu.xhash to MIPS only,
> which isn't technically necessary. Pragmatically, though, it will almost
> certainly never be used anywhere else.... (One can only hope.)
I don't think this is a problem at all actually -- after all the very
reason you needed to implement .gnu.xhash for the MIPS target is the same
which makes DT_MIPS_SYMTABNO necessary. So any other target requiring the
mapping will likely have a similar dynamic entry (tag) already defined in
its psABI, and if not, then we can require such a tag to be added as a GNU
extension, just as such an extension .gnu.xhash already is. Any tool
which produces .gnu.xhash may as well produce such a dynamic entry. So as
I say, I don't think there is a technical limitation here, not even a
potential one.
> > Gratuitous change, please remove. Formatting is wrong here (also after
> > your change), but please don't mix coding style changes and code updates
> > unless changing the ill-formatted line anyway.
>
> Sorry, I didn't intend to include that in the patch. It slipped through my
> proofreading.
No worries, things happen sometimes (or rather all the time), and it's
one of the purposes of the review process to get them shaken out.
> > Given that these tests (and `hash1a.d') were added along code to handle
> > our non-support for GNU hash on the MIPS target in `mips_after_parse' with
> > commit 73934d319dae it looks to me they were meant to verify that we bail
> > out gracefully. Now that this code is going away I think merely silencing
> > the tests in this manner is not the way to go.
> >
> > With `mips_after_parse' removed they serve no purpose anymore, but I
> > think at the very least we should have minimum coverage for the actual
> > feature. So instead please arrange for a dynamic symbol to be produced
> > and then verify that the machinery works e.g. by passing the DSO built
> > through `readelf -I'. Especially as it seems we have little if any
> > coverage here or at least I have troubles finding any relevant test cases.
> > All I found is `ld/testsuite/ld-elf/hash.d' (which BTW needs to be updated
> > accordingly; please include it with the next version of your patch) and
> > that is really a bare minimum.
> >
> > I can help you with making such an update to these test cases if you find
> > yourself having troubles with it.
> >
> > Then as the next step I think we should actually verify the contents of
> > the section generated, so in addition to the minimal tests outlined above
> > a `readelf -x .gnu.xhash' or `objdump -s -j .gnu.xhash' test would be good
> > to have, with more than one dynamic symbol involved so as to make it less
> > trivial. This further test is not a prerequisite for the acceptance of
> > your patch as far as I'm concerned, however if you'd like to experiment,
> > learn a bit about our test suite and also to verify your work, then I
> > encourage you and will greatly appreciate such an update.
> >
>
> I'm not too familiar with the dejagnu rig being used. It isn't totally clear to
> me exactly how to go about adding arbitrarily complex tests to it so I went
> with something simple. That and, well, deadlines....
It's quite easy actually. First you need a source to build, preferably
an assembly one, though in some cases a C one might be acceptable.
Then you need a recipe to build it, as if entered at a shell's command
line. Since you need to verify dynamic load data here, this will have to
include a way to assemble and link your binary to be verified.
Finally you need to pick the tool you want to do verification with (i.e.
`readelf -I' as I suggested), and produce a pattern to match the output
from that tool against. The pattern is a text file of TCL regular
expressions -- in reality for many cases (and I believe in this one in
particular) you just want to match output literally, so all you need to do
is escaping characters that have a special meaning in TCL's regexp parser
(documented here: <http://tmml.sourceforge.net/doc/tcl/re_syntax.html>
BTW; it's a superset of the standard regular expression syntax).
Since it's LD that you'll be verifying that it produces the expected hash
contents the test case has to be a part of the LD testsuite (and the MIPS
part thereof), located under ld/testsuite/ld-mips-elf/. Take a look at
`ld/testsuite/ld-mips-elf/got-dump-2.d' as an easy example (test cases of
this kind are given a .d suffix; this is required by our test framework).
At the top of the file you can see a series of lines starting with #
followed by a keyword, colon and then some text. These are commands for
the test framework. All the commands you'll need for your test case are
present there:
- name -- defines a description for the test, which will be printed and
logged as the test executes,
- source -- names a file to assemble, it can be followed with options to
pass to GAS for the assembly of this file if needed; if you
need to assemble more than one file, then you can include
further `source' commands and corresponding GAS options on
separate lines each,
- as -- gives options to pass to GAS globally for the assembly of all
sources, in addition to any options specified with `source',
- ld -- gives options to pass to LD for linking; the names of all
objects assembled by `source' commands will be passed to LD
as well,
- readelf -- this gives the name of the tool to produce the result to
verify against the regexp included with the file (other
possibilities are `objdump' or `nm'), and the options to pass
to it to get its output.
There is a further explanation available in `ld/testsuite/lib/ld-lib.exp',
near `run_dump_test', but I think the short reference above should be
enough to get you set up.
Then the rest of the file is the regexp pattern to match, line by line.
Empty lines are ignored both in this file and in input matched against, so
they can be used to improve readability. As you can see parentheses are
escaped so that they lose their special meaning and are matched literally
instead. You don't need to write this part by hand of course, you just
produce it with the tool later used for matching.
With a randomly picked up non-MIPS dynamic binary I have on the machine
I'm writing this on and the output of `readelf -I' executed with that
binary and passed through `sed 's/\([()\.]\)/\\\1/g'' I get this:
Histogram for bucket list length \(total of 1017 buckets\):
Length Number % of total Coverage
0 108 \( 10\.6%\)
1 266 \( 26\.2%\) 12\.3%
2 273 \( 26\.8%\) 37\.4%
3 207 \( 20\.4%\) 66\.1%
4 103 \( 10\.1%\) 85\.1%
5 41 \( 4\.0%\) 94\.5%
6 15 \( 1\.5%\) 98\.7%
7 3 \( 0\.3%\) 99\.6%
8 1 \( 0\.1%\) 100\.0%
Histogram for `\.gnu\.hash' bucket list length \(total of 1011 buckets\):
Length Number % of total Coverage
0 108 \( 10\.7%\)
1 242 \( 23\.9%\) 11\.2%
2 294 \( 29\.1%\) 38\.4%
3 205 \( 20\.3%\) 66\.8%
4 113 \( 11\.2%\) 87\.7%
5 30 \( 3\.0%\) 94\.6%
6 17 \( 1\.7%\) 99\.4%
7 2 \( 0\.2%\) 100\.0%
-- so you'll have to include something similar as your pattern to match
against.
Now you're set up to wire your test case into the test framework, and you
do it by adding:
run_dump_test "<name-of-your-test>"
to `ld/testsuite/ld-mips-elf/mips-elf.exp', with <name-of-your-test>
substituted with the name you chose, sans the .d suffix. I suggest that
you just reuse `hash1a', `hash1b' and `hash1c' (which are already wired)
and substitute their contents appropriately.
There is a second, more complicated way to wire tests into the LD test
suite, which requires some knowledge of the TCL programming language, but
I don't think it's needed in your case. It would be if there were more
complex requirements, such as multiple shared libraries first built and
then linked against. It's clearly not the case here, so I won't be
introducing you to that part.
As to the change to `ld/testsuite/ld-elf/hash.d' I suggested -- first you
need to remove:
#notarget: mips*-*-*
from that file (so that the test case is now run with the MIPS target as
well) and see what happens. Perhaps the test case will just pass, so no
need to do anything else. If it fails, then you need to see what happened
and whether it's what's expected. If in doubt, you can always ask here,
or just me off the list.
> > The only drawback of prelinking I know of is that, by the nature of its
> > operation, its incompatible with ASLR, so unless you need this feature the
> > tool may be worth trying.
>
> That was one of my first suggestions too. Unfortunately, ASLR is a mandatory
> part of the system in question.
OK, I see.
> Thank you again for your detailed review and again my apologies for my
> tardy response. I will once again prod our legal folks to see if we can't
> find some sort of solution. However, in the meantime, this will remain
> in limbo I'm afraid. (Sorry.)
Understood. I hope you can work out a solution from information provided
in the course of this discussion. Please let me know once you've made any
progress here.
Regardless, please let me know if you find anything above unclear, or if
you have any other questions or comments.
Maciej
More information about the Binutils
mailing list