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

["Neil Schellenberger (neschell)" <neschell at cisco dot com>]

Hi Maciej,

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.

Thank you very much for your detailed and insightful analysis.  I concur, of course,
with all of your points regarding naming, formatting, and so forth.

>  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.)

>  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!)

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

>  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.)

> > @@ -9510,7 +9524,7 @@ process_symbol_table (FILE * file)
> >        if (fseek (file,
> >  		 (archive_file_offset
> >  		  + offset_from_vma (file, buckets_vma
> > -					   + 4 * (ngnubuckets + maxchain), 4)),
> > +				     + 4 * (ngnubuckets + maxchain), 4)),
> 
>  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.

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

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

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.)

Regards,
Neil