From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24994 invoked by alias); 9 Mar 2013 09:58:16 -0000 Received: (qmail 24978 invoked by uid 22791); 9 Mar 2013 09:58:15 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f176.google.com (HELO mail-wi0-f176.google.com) (209.85.212.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 09 Mar 2013 09:58:06 +0000 Received: by mail-wi0-f176.google.com with SMTP id hm14so149948wib.9 for ; Sat, 09 Mar 2013 01:58:05 -0800 (PST) X-Received: by 10.194.121.6 with SMTP id lg6mr9183722wjb.22.1362823085154; Sat, 09 Mar 2013 01:58:05 -0800 (PST) Received: from localhost ([2.26.169.65]) by mx.google.com with ESMTPS id eo1sm3081086wib.8.2013.03.09.01.58.03 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 09 Mar 2013 01:58:04 -0800 (PST) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,Joel Brobecker , Catherine Moore , , , rdsandiford@googlemail.com Cc: Joel Brobecker , Catherine Moore , , Subject: Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support References: <87621mwt3l.fsf@talisman.default> Date: Sat, 09 Mar 2013 09:58:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Sat, 9 Mar 2013 04:03:16 +0000") Message-ID: <87ehfozyhg.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00414.txt.bz2 Thanks for the updates. "Maciej W. Rozycki" writes: >> "Maciej W. Rozycki" 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