Re: [RFC] Add IFUNC support for MIPS (v3)

[Richard Sandiford <rdsandiford at googlemail dot com>]

Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
>>> +/* hash_traverse callback that is called before sizing sections.
>>> +   DATA points to a mips_htab_traverse_info structure.  */
>>> +
>>> +static bfd_boolean
>>> +mips_elf_check_local_symbols (void **slot, void *data)
>>> +{
>>> +  struct mips_htab_traverse_info *hti =
>>> +    (struct mips_htab_traverse_info *) data;
>>> +  struct mips_elf_link_hash_entry *h =
>>> +    (struct mips_elf_link_hash_entry *) *slot;
>>> +
>>> +  /* If the referenced symbol is ifunc, allocate an iplt for it.  */
>>> +  if (h && !h->needs_iplt && h->root.type == STT_GNU_IFUNC
>>> +      && h->root.def_regular)
>>> +    {
>>> +      struct bfd_link_info *info = hti->info;
>>> +      elf_tdata (info->output_bfd)->has_gnu_symbols |= elf_gnu_symbol_ifunc;
>>> +
>>> +      /* .iplt entry is needed only for executable objects.  */
>>> +      if (!bfd_link_pic (info) &&
>>> +	  !mips_elf_allocate_iplt (info, mips_elf_hash_table (info), h))
>>> +	return FALSE;
>>> +
>>> +      /* IRELATIVE fixup will be needed for each local IFUNC.  */
>>> +      if (!mips_elf_allocate_ireloc (info, mips_elf_hash_table (info), h))
>>> +	return FALSE;
>>> +    }
>>> +
>>> +  return TRUE;
>>> +}
>>
>> I might be missing something, but I think for local symbols the
>> condition for whether an IPLT is needed should be h->has_static_relocs
>> rather than !bfd_link_pic.  E.g., if an executable uses GOT and CALL
>> relocs for all references to a symbol, it would be more efficient not
>> to have an IPLT and simply use an entry in the general GOT area
>> with an IRELATIVE relocation against it.
>
> has_static_relocs does not currently track local IFUNCs or relocations in 
> data sections. The first seems easy to fix, but I couldn't figure out
> the latter.

I didn't follow this, sorry.  Why are relocations in data sections special
in this context?  At face value the logic:

      if (h)
	{
	  if (constrain_symbol_p)
	    {
	      if (!can_make_dynamic_p)
		((struct mips_elf_link_hash_entry *) h)->has_static_relocs = 1;

	      if (!call_reloc_p)
		h->pointer_equality_needed = 1;

	      /* We must not create a stub for a symbol that has
		 relocations related to taking the function's address.
		 This doesn't apply to VxWorks, where CALL relocs refer
		 to a .got.plt entry instead of a normal .got entry.  */
	      if (!htab->is_vxworks && (!can_make_dynamic_p || !call_reloc_p))
		((struct mips_elf_link_hash_entry *) h)->no_fn_stub = TRUE;
	    }

applies equally to local ifuncs, and check_relocs should see all relocations
against local ifuncs.

>>> @@ -3263,9 +3478,12 @@ mips_elf_count_got_entry (struct bfd_link_info *info,
>>>  					entry->symndx < 0
>>>  					? &entry->d.h->root : NULL);
>>>      }
>>> -  else if (entry->symndx >= 0 || entry->d.h->global_got_area == GGA_NONE)
>>> +  /* Skip IFUNCs from local/global GOT, they are already counted as general
>>> +     GOT entries with explicit relocations.  */
>>> +  else if (entry->symndx >= 0 || (entry->d.h->global_got_area == GGA_NONE
>>> +				  && !entry->d.h->needs_ireloc))
>>>      g->local_gotno += 1;
>>> -  else
>>> +  else if (!entry->d.h->needs_ireloc)
>>>      g->global_gotno += 1;
>>>  }
>> 
>> I think this is the place where we should be setting general_gotno,
>> rather than mips_elf_count_got_symbols.  mips_elf_count_got_symbols
>> is supposed to:
>> 
>>    Count the number of global symbols that are in the primary GOT only
>>    because they have relocations against them (reloc_only_gotno).
>> 
>> and shouldn't be counting explicit GOT entries.
>
> Are we not expected to have a GOT entry for a global IFUNC symbol if
> there are no references to it (say within a shared library)? Something
> in your discussions with jcarter led me to believe that this was
> expected. If not it can removed.

No, I don't think we need that.  The psABI normally requires this for
general global symbols because of the way that the GOT is resolved,
but the point of the new GOT area is to avoid that.

>>> @@ -5200,6 +5515,75 @@ mips_elf_relocation_needs_la25_stub (bfd *input_bfd, int r_type,
>>>        return FALSE;
>>>      }
>>>  }
>>> +
>>> +/* Find and/or create a hash entry for local symbol.  */
>>> +
>>> +static struct mips_elf_link_hash_entry *
>>> +get_local_sym_hash (struct mips_elf_link_hash_table *htab,
>>> +		    bfd *abfd, const Elf_Internal_Rela *rel)
>>> +{
>>> +  struct mips_elf_link_hash_entry e, *ret;
>>> +  asection *sec;
>>> +  hashval_t h;
>>> +  void **slot;
>>> +  Elf_Internal_Sym *isym;
>>> +  Elf_Internal_Shdr *symtab_hdr;
>>> +  char *namep;
>>> +
>>> +  sec = bfd_get_section_by_name (abfd, ".text");
>> 
>> I'm not sure it's valid to assume that every input has a .text section.
>> At least we should have some error code if it doesn't.
>> 
>> But I suppose this comes back to my objection to the "local hash" stuff.
>> It just seems wrong to pick an arbitrary section index, regardless of
>> where the symbol is actually defined.
>
> I see the pitfall of ".text". I think I'll stick to the i386 method, which is 
> to pick the first section id.

OK.

>>> @@ -5554,6 +5947,17 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>>>        target_is_16_bit_code_p = !micromips_p;
>>>        target_is_micromips_code_p = micromips_p;
>>>      }
>>> +  /* If this symbol is an ifunc, point to the iplt stub for it.  */
>>> +  else if (h && h->needs_iplt)
>>> +    {
>>> +      BFD_ASSERT (htab->root.iplt != NULL);
>>> +      symbol = (htab->root.iplt->output_section->vma
>>> +		+ htab->root.iplt->output_offset
>>> +		+ h->iplt_offset);
>>> +      /* Set ISA bit in address for compressed code.  */
>>> +      if (ELF_ST_IS_COMPRESSED (h->root.other))
>>> +	symbol |= 1;
>>> +    }
>> 
>> I think you need to set target_is_16_bit_code_p and
>> target_is_micromips_code_p too (based on the equivalent conditions
>> for abfd).
>> 
>> I'm a bit nervous about how this interacts with the other redirections,
>> e.g. for la25 and mips16 hard-float stubs.
>
> I  will dig further in to this. Any hints on what I should look for?

Well, it's a question of whether the earlier redirections in the
if-else are mutually exclusive with the new one or not, and whether
we handle redirections correctly for the ifunc resolver.  (The second
part isn't really related to this code.)  E.g.:

  - if a MIPS16 hard float function calls an ifunc, do we correctly
    redirect the call to a stub that calls the iplt (or, if the stub
    uses a CALL16, indirectly by loading the resolved ifunc from the
    GOT)?

  - if there are MIPS16 and non-MIPS16 calls to the same iplt,
    what happens?

  - if a PIC ifunc F (i.e. one that needs $25 to be valid on entry)
    is used in a non-PIC executable, does the IRELATIVE relocation
    refer to the "la $25" stub for F, or to the original F?

  - if we have a MIPS16 hard float ifunc F, does the IRELATIVE
    resolve the stub for F, or to F itself?

It might well be that all this just works, it just wasn't obvious.

>>> @@ -5940,8 +6354,11 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>>>  	 R_MIPS*_GOT16; every relocation evaluates to "G".  */
>>>        if (!htab->is_vxworks && local_p)
>>>  	{
>>> +	  /* Local IFUNC symbols must be accessed through GOT, similar to
>>> +	     global symbols, to allow for indirection.  */
>>>  	  value = mips_elf_got16_entry (abfd, input_bfd, info,
>>> -					symbol + addend, !was_local_p);
>>> +					symbol + addend,
>>> +					!was_local_p || local_gnu_ifunc_p, h);
>>>  	  if (value == MINUS_ONE)
>>>  	    return bfd_reloc_outofrange;
>>>  	  value
>> 
>> I agree this is correct, but I think it should be specified explicitly
>> in the ABI document that R_MIPS_GOT16 relocations against local IFUNC
>> symbols are treated in this way.  Using the usual R_MIPS_GOT16/R_MIPS_LO16
>> pair for local symbols would give the wrong result for ifuncs.
>
> I am not quite sure which ABI document you mean,

The document you attached earlier where you described the new relocations.

> but we are planning a change in gcc to reflect this. It shouldn't be
> generating GOT+LO sequences for IFUNCs.  In theory, LO16(ifunc) is
> nonsensical for PIC, but we can force it to zero just to be defensive.

I don't think ignoring the addend is a good idea.  Seems better to make
it a hard error or make it work like global symbols (as the above does).

>>> +  else
>>> +    {
>>> +      bfd_byte *loc;
>>> +      bfd_vma igot_index;
>>> +      gotsect = htab->root.igotplt;
>>> +      igot_offset = hmips->igot_offset;
>>> +
>>> +      /* Calculate the address of the IGOT entry.  */
>>> +      igot_index = igot_offset / MIPS_ELF_GOT_SIZE (dynobj);
>>> +
>>> +      if (!gotsect->contents)
>>> +	{
>>> +	  gotsect->contents = bfd_zalloc (output_bfd, gotsect->size);
>>> +	  if (!gotsect->contents)
>>> +	    return FALSE;
>>> +	}
>>> +
>>> +      /* Initially point the .igot entry at the IFUNC resolver routine.  */
>>> +      loc = (bfd_byte *) gotsect->contents
>>> +	+ igot_index * MIPS_ELF_GOT_SIZE (dynobj);
>>> +
>>> +      if (ABI_64_P (output_bfd))
>>> +	bfd_put_64 (output_bfd, value, loc);
>>> +      else
>>> +	bfd_put_32 (output_bfd, value, loc);
>>> +    }
>>> +
>>> +  igotplt_address = (gotsect->output_section->vma + gotsect->output_offset
>>> +		     + igot_offset);
>>> +
>>> +  relsect = mips_get_irel_section (info, htab);
>>> +
>>> +  if (igot_offset >= 0)
>>> +    {
>>> +      if (hmips->needs_iplt && relsect->contents == NULL)
>>> +	    {
>> 
>> Isn't needs_iplt always true in this case?
>
> No. The code is generalized so that igot_offset could mean got_offset
> or igot_offset. For the former, needs_plt is false. In other words,
> needs_iplt implies (igot_offset >= 0), but not the converse. OTOH,
> memory for contents is allocated as usual in
> _bfd_mips_elf_size_dynamic_sections so I could just replace this block
> with an assertion.

Hmm, OK, I'd missed the bit about igot_offset meaning got_offset in some
cases, sorry.  But let's step back a bit.

(A) When the symbol doesn't bind locally we should treat the ifunc
    like any other symbol, except that we use an R_MIPS_REL32 in the
    new global GOT area to ensure it is resolved safely.  (Normal global
    GOT relocation is too early if we do end up using the local definition.)

(B) When the symbol binds locally I think there are 6 cases:

    PIC relocs?  non-PIC relocs?  all call-only?   variant
    n            y                y                (1)
    y            n                y                (2)
    y            y                y                (3)
    n            y                n                (4)
    y            n                n                (2) or (5) (*)
    y            y                n                (5)

    (1) Need an IPLT.  Nothing other than the IPLT needs the associated
        IRELATIVE GOT entry, so the entry can go in .igot.

    (2) No need for an IPLT, we can just have a .got entry with an IRELATIVE
        relocation.

    (3) Need an IPLT.  The associated IRELATIVE GOT entry should go in .got
        so that it can be used by the PIC references.

    (4) Same as (1), using the IPLT as the canonical function address.

    (5) Need an IPLT, which acts as the canonical function address.
        Nothing other than the IPLT needs the associated IRELATIVE GOT entry,
        so that entry can go in .igot.  However, we need a separate
        .got entry to satisfy the PIC references.  This can be a normal
        local GOT entry (i.e. not use the new area).

    (*) Could be (2) assuming that the ifunc always returns the same pointer,
        otherwise (5).  Should be rare so maybe (5) is safer.

    If the symbol is exported then for (1)-(3) the symbol definition
    is the IFUNC.  For (4) and (5) it's the IPLT.

We should know by size_dynamic_sections which case applies.

The only cases that don't need an IPLT are (2) (where all references to
the ifunc are via the GOT) and (A).  So it seems strange to have this code:

+  if (!hmips->needs_iplt)
+    {
+      gotsect = htab->sgot;
+      /* Check if IFUNC symbol already has an assigned GOT slots; assign
+	 a new slot if necessary.  */
+      igot_offset = mips_elf_check_local_got_index (output_bfd, info,
+						    hmips, value);
+      if (igot_offset < 0)
+	igot_offset = mips_elf_local_got_index (output_bfd, output_bfd, info,
+						value, hmips->root.dynindx,
+						hmips, R_MIPS_REL32);
+    }

in a function called by finish_dynamic_symbol.  The GOT layout should
already be final at this point and we should already have created the
GOT entry needed by the PIC references to the symbol.

Also in:

+      if (hmips->needs_iplt || hmips->root.dynindx < 0)
+	/* Emit an R_MIPS_IRELATIVE relocation against the [I]GOT entry.  */
+	mips_elf_output_dynamic_relocation (output_bfd, relsect,
+					    relsect->reloc_count++, 0,
+					    R_MIPS_IRELATIVE, igotplt_address);
+      else
+	{
+	  /* Emit an R_MIPS_REL32 relocation against global GOT entry for
+	     a preemptible symbol.  */
+	  asection *sec = hmips->root.root.u.def.section;
+	  Elf_Internal_Rela rel[3];
+
+	  memset (rel, 0, sizeof (rel));
+	  rel[0].r_info = ELF_R_INFO (output_bfd, 0, R_MIPS_REL32);
+	  rel[0].r_offset = rel[1].r_offset = rel[2].r_offset = igot_offset;
+
+	  mips_elf_create_dynamic_relocation (output_bfd, info, rel, hmips,
+					      sec, value, NULL,
+					      gotsect);

if the else is for (A) then "hmips->root.dynindx < 0" doesn't seem
strong enough, since it would include (2) in cases where the symbol
is also exported.

Thanks,
Richard