This is the mail archive of the 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]
Other format: [Raw text]

RE: [PATCH] MIPS support for --hash-style=gnu

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

 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.


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