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

Richard Sandiford
2013-03-09T09:58:16Z

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