Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.

[Richard Sandiford <richard at codesourcery dot com>]

Richard Sandiford <richard@codesourcery.com> writes:
> Thiemo Seufer <ths@networkno.de> writes:
>> Richard Sandiford wrote:
>>> David Daney <ddaney@avtrex.com> writes:
>>> > ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
>>> > !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
>>> 
>>> Sorry to be a pain, but as I said earlier, I really do think we should
>>> cache the chosen stub size in mips_elf_link_hash_table (and get rid of
>>> this macro entirely).  That will emphasise that always_size_dynamic_sections
>>> is the place that makes the decision, and that it's only safe to use this
>>> value once that function has been called.  I think that will be more robust
>>> and easier to understand in future.
>>> 
>>> Apart from that, and from Thiemo's and Daniel's comments, this looks
>>> really good to me.  Thanks a lot for doing this!
>>
>> Does this followup patch look ok?
>
> Looks good, apart from the fact that you're setting function_stub_size
> in _bfd_mips_elf_create_dynamic_sections rather than the suggested
> always_size_dynamic_sections.  Was there a particular reason for that?
> As David says, create_dynamic_sections is too early; AIUI,
> create_dynamic_sections is called as soon as we know we need
> dynamic sections, so the value of dynsymcount at that point is
> going to be pretty arbitrary.
>
> The reason I suggested always_size_sections is that I think we should
> determine the stub entry size and stub section size at the same time.
>
> Thanks for doing this though!  I volunteer to come up with some
> testcases tomorrow.

OK, here are the testcases.  They showed up a couple of problems:

  (1) We were using "li" for the lower half of 32-bit indexes, thus
      cancelling out the "lui"!  Daniel quoted the definition of
      STUB_LI16U and asked about whether it should be "ori", but I think
      there was some confusion.  We don't want to rename STUB_LI16U to
      STUB_ORI -- it's fine as it is -- but we _do_ want a separate
      STUB_ORI for "ori t8,t8,foo" (as opposed to "ori t8,zero,foo").

  (2) dynsymcount doesn't include the local section symbols at the time
      that the MIPS section-sizing code is run.  Although the exact number
      of such symbols isn't known at that stage, we can get a conservative
      estimate by reusing a loop in _bfd_mips_elf_final_link.

      This loop was originally copied from _bfd_elf_link_renumber_dynsyms.
      We should try to reuse code between the two, as the current MIPS
      version is already out-of-sync, but I'd rather do that separately,
      as it touches non-MIPS code.

Because the estimate in (2) is conservative, we can't insist that a file
with 0x10000 dynsyms use 16-byte stubs.  The tests therefore test a file
with 0xfff1 dynsyms instead.

We often seem to need to update these kinds of tests after changes to
target-independent code.  I've tried to make that easier by including
a base_syms variable.  See the comments for details.

I've attached two patches.  The first is relative to Thiemo's patch
from yesterday; the second is a complete patch against mainline.

Tested on mipsel-linux-gnu, mips64-linux-gnu and mipsisa64-elf.
OK to install?

bfd/
	* elfxx-mips.c (mips_elf_link_hash_table): Add function_stub_size.
	(STUB_ORI): New macro.
	(STUB_LI16U): Fix formatting.
	(MIPS_FUNCTION_STUB_SIZE): Delete.
	(MIPS_FUNCTION_STUB_MAX_SIZE): Likewise.
	(MIPS_FUNCTION_STUB_NORMAL_SIZE): New macro.
	(MIPS_FUNCTION_STUB_BIG_SIZE): Likewise.
	(_bfd_mips_elf_adjust_dynamic_symbol): Use htab->function_stub_size
	instead of MIPS_FUNCTION_STUB_SIZE.
	(count_section_dynsyms): New function, split out from
	_bfd_mips_elf_final_link.
	(_bfd_mips_elf_always_size_sections): Get a worst-case estimate
	of the number of dynamic symbols needed and use it to set up
	function_stub_size.  Use function_stub_size rather than
	MIPS_FUNCTION_STUB_SIZE to determine the size of the stub section.
	Use 16-byte stubs for 0x10000 dynamic symbols.
	(_bfd_mips_elf_size_dynamic_sections): Use htab->function_stub_size
	instead of MIPS_FUNCTION_STUB_SIZE.  Fix formatting.
	(_bfd_mips_elf_finish_dynamic_symbol): Likewise.  Change the
	size of the stub buffer from MIPS_FUNCTION_STUB_MAX_SIZE to
	MIPS_FUNCTION_STUB_BIG_SIZE.  Tweak the check for unhandled dynindxes.
	Use MIPS_FUNCTION_STUB_BIG_SIZE rather than a hard-coded 20.
	Use STUB_ORI rather than STUB_LI16U for big stubs.
	(_bfd_mips_elf_link_hash_table_create): Initialize function_stub_size.
	(_bfd_mips_elf_final_link): Use count_section_dynsyms.

ld/testsuite/
	* ld-mips-elf/stub-dynsym-1.s,
	* ld-mips-elf/stub-dynsym-1.ld,
	* ld-mips-elf/stub-dynsym-1-7fff.d,
	* ld-mips-elf/stub-dynsym-1-8000.d,
	* ld-mips-elf/stub-dynsym-1-fff0.d,
	* ld-mips-elf/stub-dynsym-1-10000.d,
	* ld-mips-elf/stub-dynsym-1-2fe80.d: New test.
	* ld-mips-elf/mips-elf.exp: Run it.

Attachment: stub-selection-rel-thiemo.diff
Description: Text document

Attachment: stub-selection.diff
Description: Text document