Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support

[Richard Sandiford <rdsandiford at googlemail dot com>]

Thanks for the updates.

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> > @@ -3215,25 +3325,19 @@ static bfd_vma
>> >  mips_elf_gotplt_index (struct bfd_link_info *info,
>> >  		       struct elf_link_hash_entry *h)
>> >  {
>> > -  bfd_vma plt_index, got_address, got_value;
>> > +  bfd_vma got_address, got_value;
>> >    struct mips_elf_link_hash_table *htab;
>> >  
>> >    htab = mips_elf_hash_table (info);
>> >    BFD_ASSERT (htab != NULL);
>> >  
>> > -  BFD_ASSERT (h->plt.offset != (bfd_vma) -1);
>> > -
>> > -  /* This function only works for VxWorks, because a non-VxWorks .got.plt
>> > -     section starts with reserved entries.  */
>> > -  BFD_ASSERT (htab->is_vxworks);
>> > -
>> > -  /* Calculate the index of the symbol's PLT entry.  */
>> > -  plt_index = (h->plt.offset - htab->plt_header_size) / htab->plt_entry_size;
>> > +  BFD_ASSERT (h->plt.plist != NULL);
>> > +  BFD_ASSERT (h->plt.plist->gotplt_index != MINUS_ONE);
>> >  
>> >    /* Calculate the address of the associated .got.plt entry.  */
>> >    got_address = (htab->sgotplt->output_section->vma
>> >  		 + htab->sgotplt->output_offset
>> > -		 + plt_index * 4);
>> > +		 + h->plt.plist->gotplt_index * 4);
>> >  
>> >    /* Calculate the value of _GLOBAL_OFFSET_TABLE_.  */
>> >    got_value = (htab->root.hgot->root.u.def.section->output_section->vma
>> 
>> If we remove the is_vxworks assert, I think we should use MIPS_ELF_GOT_SIZE
>> instead of 4.
>
>  Not sure if this is related to this change, but I see no problem with 
> that either.  I've updated all the references throughout.

It was related because the patch kept a VxWorks assumption while removing
the assert for VxWorks.

> Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2013-03-08 11:09:04.000000000 +0000
> +++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2013-03-09 02:43:31.765430204 +0000
> @@ -319,6 +319,32 @@ struct mips_elf_hash_sort_data
>    long max_non_got_dynindx;
>  };
>  
> +/* We make up to two PLT entries if needed, one for standard MIPS code
> +   and one for compressed code, either of MIPS16 or microMIPS one.  We
> +   keep the record of a stub if one is used instead separately, for
> +   easier processing.  */

s/either of MIPS16 or microMIPS one/either a MIPS16 or microMIPS one/.
Suggest "We keep a separate record of traditional lazy-binding stubs,
for easier processing."

> @@ -5124,13 +5232,55 @@ mips_elf_calculate_relocation (bfd *abfd
>  		|| h->root.root.type == bfd_link_hash_defweak)
>  	       && h->root.root.u.def.section)
>  	{
> -	  sec = h->root.root.u.def.section;
> -	  if (sec->output_section)
> -	    symbol = (h->root.root.u.def.value
> -		      + sec->output_section->vma
> -		      + sec->output_offset);
> +	  if (h->use_plt_entry)
> +	    {
> +	      bfd_boolean micromips_p = MICROMIPS_P (abfd);
> +	      bfd_vma plt_offset;
> +	      bfd_vma isa_bit;
> +	      bfd_vma val;
> +
> +	      BFD_ASSERT (h->root.plt.plist != NULL);
> +	      BFD_ASSERT (h->root.plt.plist->mips_offset != MINUS_ONE
> +			  || h->root.plt.plist->comp_offset != MINUS_ONE);
> +
> +	      plt_offset = htab->plt_header_size;
> +	      if (h->root.plt.plist->comp_offset == MINUS_ONE
> +		  || (h->root.plt.plist->mips_offset != MINUS_ONE
> +		      && r_type != R_MIPS16_26 && r_type != R_MICROMIPS_26_S1))
> +		{
> +		  isa_bit = 0;
> +		  target_is_16_bit_code_p = FALSE;
> +		  target_is_micromips_code_p = FALSE;
> +		  plt_offset += h->root.plt.plist->mips_offset;
> +		}
> +	      else
> +		{
> +		  isa_bit = 1;
> +		  target_is_16_bit_code_p = !micromips_p;
> +		  target_is_micromips_code_p = micromips_p;
> +		  plt_offset += (htab->plt_mips_offset
> +				 + h->root.plt.plist->comp_offset);
> +		}
> +	      BFD_ASSERT (plt_offset <= htab->splt->size);
> +
> +	      sec = htab->splt;
> +	      val = plt_offset + isa_bit;
> +	      /* For VxWorks, point at the PLT load stub rather than the
> +	         lazy resolution stub.  */
> +	      if (htab->is_vxworks)
> +		val += 8;
> +	      symbol = sec->output_section->vma + sec->output_offset + val;
> +	    }
>  	  else
> -	    symbol = h->root.root.u.def.value;
> +	    {
> +	      sec = h->root.root.u.def.section;
> +	      if (sec->output_section)
> +		symbol = (h->root.root.u.def.value
> +			  + sec->output_section->vma
> +			  + sec->output_offset);
> +	      else
> +		symbol = h->root.root.u.def.value;
> +	    }
>  	}
>        else if (h->root.root.type == bfd_link_hash_undefweak)
>  	/* We allow relocations against undefined weak symbols, giving
> @@ -5177,12 +5327,6 @@ mips_elf_calculate_relocation (bfd *abfd
>  	{
>  	  return bfd_reloc_notsupported;
>  	}
> -
> -      target_is_16_bit_code_p = ELF_ST_IS_MIPS16 (h->root.other);
> -      /* If the output section is the PLT section,
> -         then the target is not microMIPS.  */
> -      target_is_micromips_code_p = (htab->splt != sec
> -				    && ELF_ST_IS_MICROMIPS (h->root.other));
>      }
>  
>    /* If this is a reference to a 16-bit function with a stub, we need

The block of code that you're changing here is simply calculating the
symbol value.  Now that size_dynamic_sections has set that up for us,
these two hunks should be dropped, and the test for whether to use a
compressed PLT entry instead of the symbol value should be made
afterwards, as it is for things like hard-float and LA25 stubs.
Maybe something like:

  /* If compressed PLT entries are available, make sure that we use them
     for MIPS16 and microMIPS calls.  */
  else if ((r_type == R_MIPS16_26 || r_type == R_MICROMIPS_26_S1)
	   && h != NULL
	   && h->use_plt
	   && h->root.plt.plist->comp_offset != MINUS_ONE)
    {
      sec = htab->splt;
      symbol = (sec->output_section->vma
		+ sec->output_offset
		+ htab->plt_header_size
		+ htab->plt_mips_offset
		+ h->root.plt.plist->comp_offset
		+ 1);
      target_is_16_bit_code_p = !MICROMIPS_P (abfd);
      target_is_micromips_code_p = MICROMIPS_P (abfd);
    }

at the end of the:

  /* If this is a reference to a 16-bit function with a stub, we need
     to redirect the relocation to the stub unless:

chain of ifs.

> -      /* Make room for the .got.plt entry and the R_MIPS_JUMP_SLOT
> -	 relocation.  */
> -      htab->sgotplt->size += MIPS_ELF_GOT_SIZE (dynobj);
> +      htab->splt->size = htab->plt_mips_offset + htab->plt_comp_offset;
> +      htab->sgotplt->size = htab->plt_got_index * MIPS_ELF_GOT_SIZE (dynobj);

These last two lines should be done in size_dynamic_sections rather
than once for each symbol.  I.e.:

> @@ -8985,18 +9318,60 @@ _bfd_mips_elf_size_dynamic_sections (bfd
>  	    = (bfd_byte *) ELF_DYNAMIC_INTERPRETER (output_bfd);
>  	}
>  
> -      /* Create a symbol for the PLT, if we know that we are using it.  */
> -      if (htab->splt && htab->splt->size > 0 && htab->root.hplt == NULL)
> +      /* Figure out the size of the PLT header if we know that we
> +         are using it.  For the sake of cache alignment always use
> +         a standard header whenever any standard entries are present
> +         even if microMIPS entries are present as well.  This also
> +         lets the microMIPS header rely on the value of $v0 only set
> +         by microMIPS entries, for a small size reduction.
> +
> +         Set symbol table entry values for symbols that use the
> +         address of their PLT entry now that we can calculate it.
> +
> +         Also create the _PROCEDURE_LINKAGE_TABLE_ symbol if we
> +         haven't already in _bfd_elf_create_dynamic_sections.  */
> +      if (htab->splt && htab->splt->size > 0)
>  	{
> +	  bfd_boolean micromips_p = (MICROMIPS_P (output_bfd)
> +				     && !htab->plt_mips_offset);
> +	  unsigned int other = micromips_p ? STO_MICROMIPS : 0;
> +	  bfd_vma isa_bit = micromips_p;
>  	  struct elf_link_hash_entry *h;
> +	  bfd_vma size;
>  
>  	  BFD_ASSERT (htab->use_plts_and_copy_relocs);
>  
> -	  h = _bfd_elf_define_linkage_sym (dynobj, info, htab->splt,
> -					   "_PROCEDURE_LINKAGE_TABLE_");
> -	  htab->root.hplt = h;
> -	  if (h == NULL)
> -	    return FALSE;
> +	  if (htab->is_vxworks && info->shared)
> +	    size = 4 * ARRAY_SIZE (mips_vxworks_shared_plt0_entry);
> +	  else if (htab->is_vxworks)
> +	    size = 4 * ARRAY_SIZE (mips_vxworks_exec_plt0_entry);
> +	  else if (ABI_64_P (output_bfd))
> +	    size = 4 * ARRAY_SIZE (mips_n64_exec_plt0_entry);
> +	  else if (ABI_N32_P (output_bfd))
> +	    size = 4 * ARRAY_SIZE (mips_n32_exec_plt0_entry);
> +	  else if (!micromips_p)
> +	    size = 4 * ARRAY_SIZE (mips_o32_exec_plt0_entry);
> +	  else
> +	    size = 2 * ARRAY_SIZE (micromips_o32_exec_plt0_entry);
> +
> +	  htab->plt_header_is_comp = micromips_p;
> +	  htab->plt_header_size = size;
> +	  htab->splt->size += size;

	  htab->splt->size = (size
			      + htab->plt_mips_offset
			      + htab->plt_comp_offset;
	  htab->sgotplt->size = (htab->plt_got_index
				 * MIPS_ELF_GOT_SIZE (dynobj));

> +	      /* ADDIUPC has a span of +/-16MB, check we're in range.  */
> +	      if (gotpc_offset + 0x1000000 >= 0x2000000)
> +		{
> +		  (*_bfd_error_handler)
> +		    (_("%B: `%A' offset of %ld from `%A' "
> +		       "beyond the range of ADDIUPC"),
> +		     output_bfd,
> +		     htab->sgotplt->output_section,
> +		     htab->splt->output_section,
> +		     (long) gotpc_offset);

The last two arguments should be swapped.

> +      /* ADDIUPC has a span of +/-16MB, check we're in range.  */
> +      if (gotpc_offset + 0x1000000 >= 0x2000000)
> +	{
> +	  (*_bfd_error_handler)
> +	    (_("%B: `%A' offset of %ld from `%A' beyond the range of ADDIUPC"),
> +	     output_bfd,
> +	     htab->sgotplt->output_section,
> +	     htab->splt->output_section,
> +	     (long) gotpc_offset);

Same here.

> +  /* Calculating the exact amount of space required for symbols would
> +     require two passes over the PLT, so just pessimise assuming two
> +     PLT slots per relocation.  */
> +  count = relplt->size / hdr->sh_entsize;
> +  counti = count * bed->s->int_rels_per_ext_rel;
> +  size = 2 * count * sizeof (asymbol);
> +  size += count * (sizeof (mipssuffix) +
> +		   (micromips_p ? sizeof (microsuffix) : sizeof (m16suffix)));
> +  addlen = 2 * (sizeof ("+0x") - 1 + 8);
> +#ifdef BFD64
> +  addlen += 2 * 8 * (bed->s->elfclass == ELFCLASS64);
> +#endif

Now that there are no addends (thanks), the last four lines should be dropped.

OK with those changes if they work, otherwise let me know.

Richard