From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7267 invoked by alias); 12 Dec 2010 13:04:46 -0000 Received: (qmail 7257 invoked by uid 22791); 12 Dec 2010 13:04:44 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST,TW_FX,TW_XF X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 12 Dec 2010 13:04:39 +0000 Received: by wyj26 with SMTP id 26so5310942wyj.0 for ; Sun, 12 Dec 2010 05:04:36 -0800 (PST) Received: by 10.227.32.74 with SMTP id b10mr975340wbd.213.1292159074413; Sun, 12 Dec 2010 05:04:34 -0800 (PST) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id f35sm3632736wbf.2.2010.12.12.05.04.30 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sun, 12 Dec 2010 05:04:32 -0800 (PST) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,binutils@sourceware.org, Chao-ying Fu , Rich Fuhler , David Lau , Kevin Mills , Ilie Garbacea , Catherine Moore , Nathan Sidwell , Joseph Myers , Nathan Froyd , rdsandiford@googlemail.com Cc: binutils@sourceware.org, Chao-ying Fu , Rich Fuhler , David Lau , Kevin Mills , Ilie Garbacea , Catherine Moore , Nathan Sidwell , Joseph Myers , Nathan Froyd Subject: Re: [PATCH] MIPS: microMIPS ASE support References: <87y6fa9u3t.fsf@firetop.home> <876302kqvu.fsf@firetop.home> Date: Sun, 12 Dec 2010 14:59:00 -0000 Message-ID: <871v5n9m7e.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2010-12/txt/msg00430.txt.bz2 This is a review of everything up to the end of elfxx-mips.c. Not sure when I'll be able to do the rest. I think we're getting close, so could you post any updates as patches relative to this one, rather than reposting the whole thing? "Maciej W. Rozycki" writes: > @@ -2372,7 +3002,14 @@ bfd_elf64_bfd_reloc_name_lookup (bfd *ab > i++) > if (mips16_elf64_howto_table_rela[i].name != NULL > && strcasecmp (mips16_elf64_howto_table_rela[i].name, r_name) == 0) > - return &mips16_elf64_howto_table_rela[i]; > + > + for (i = 0; > + i < (sizeof (micromips_elf64_howto_table_rela) > + / sizeof (micromips_elf64_howto_table_rela[0])); > + i++) > + if (micromips_elf64_howto_table_rela[i].name != NULL > + && strcasecmp (micromips_elf64_howto_table_rela[i].name, r_name) == 0) > + return µmips_elf64_howto_table_rela[i]; > > if (strcasecmp (elf_mips_gnu_vtinherit_howto.name, r_name) == 0) > return &elf_mips_gnu_vtinherit_howto; This hunk looks wrong. I doubt you meant to remove the mips16 line. Same for elfn32-mips.c. > @@ -5040,6 +5159,10 @@ mips_elf_calculate_relocation (bfd *abfd > } > > 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 Formatting nit: /* 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)); More importantly, the comment isn't any help. When do we create statically-resolved relocations against .plt? > /* Calls from 16-bit code to 32-bit code and vice versa require the > - mode change. */ > - *cross_mode_jump_p = !info->relocatable > - && ((r_type == R_MIPS16_26 && !target_is_16_bit_code_p) > - || ((r_type == R_MIPS_26 || r_type == R_MIPS_JALR) > - && target_is_16_bit_code_p)); > + mode change. This is not required for calls to undefined weak > + symbols, which should never be executed at runtime. */ But why do we need to go out of our way to check for them? I'm sure there's a good reason, but the comment doesn't give much clue what it is. > @@ -9223,6 +9499,16 @@ _bfd_mips_elf_relocate_section (bfd *out > case bfd_reloc_ok: > break; > > + case bfd_reloc_outofrange: > + if (jal_reloc_p (howto->type)) > + { > + msg = _("JALX to not a word-aligned address"); > + info->callbacks->warning > + (info, msg, name, input_bfd, input_section, rel->r_offset); > + return FALSE; > + } > + /* Fall through. */ > + > default: > abort (); > break; I suppose that should be something like "JALX to a non-word-aligned address". > + /* microMIPS local and global symbols have the least significant bit > + set. Because all values are either multiple of 2 or multiple of > + 4, compensating for this bit is as simple as setting it in > + comparisons. Just to be sure, check anyway before proceeding. */ > + BFD_ASSERT (addr % 2 == 0); > + BFD_ASSERT (toaddr % 2 == 0); > + BFD_ASSERT (count % 2 == 0); I'm suspicious about the toaddr assert. You can create text sections with odd-valued sizes: .section .text.foo,"ax",@progbits .byte 1 and as discussed elsewhere, asserts are really to double-check conditions that we think must already hold. > + addr |= 1; > + toaddr |= 1; > + > + /* Adjust the local symbols defined in this section. */ > + symtab_hdr = &elf_tdata (abfd)->symtab_hdr; > + isym = (Elf_Internal_Sym *) symtab_hdr->contents; > + for (isymend = isym + symtab_hdr->sh_info; isym < isymend; isym++) > + { > + if (isym->st_shndx == sec_shndx > + && isym->st_value > addr > + && isym->st_value < toaddr) > + isym->st_value -= count; > + } Hmm, microMIPS symbols created in the proper, blessed way (by proper, blessed ways of defining MIPS16 labels in assembly) should be even in the input object, and IIRC, the linker keeps local symbols that way internally. Only symbols entered into the hash table are odd. (c.f. mips_elf_calculate_relocation, where we have to add 1 to local symbols.) So I would have expected the "|= 1"s to be applied after this block rather than before it. Even then: > + /* Now adjust the global symbols defined in this section. */ > + symcount = (symtab_hdr->sh_size / sizeof (Elf32_External_Sym) > + - symtab_hdr->sh_info); > + sym_hashes = start_hashes = elf_sym_hashes (abfd); > + end_hashes = sym_hashes + symcount; > + > + for (; sym_hashes < end_hashes; sym_hashes++) > + { > + struct elf_link_hash_entry *sym_hash = *sym_hashes; > + > + if ((sym_hash->root.type == bfd_link_hash_defined > + || sym_hash->root.type == bfd_link_hash_defweak) > + && sym_hash->root.u.def.section == sec > + && sym_hash->root.u.def.value > addr > + && sym_hash->root.u.def.value < toaddr) > + sym_hash->root.u.def.value -= count; > + } ...if we're willing to extend the upper bound in this way, I wonder whether there's really any point having an upper bound at all. Then again, why doesn't the standard range (used by most targets) include toaddr? If you define an end marker: end_of_section: # nothing after this then wouldn't end_of_section == toaddr, and shouldn't it be included? I see some targets rightly adjust st_size too. We should do the same here. > +static unsigned long > +find_match (unsigned long opcode, const struct opcode_descriptor insn[]) > +{ > + unsigned long indx; > + > + /* First opcode_table[] entry is ignored. */ > + for (indx = 1; insn[indx].mask != 0; indx++) > + if (MATCH (opcode, insn[indx])) > + return indx; > + > + return 0; > +} But _why_ is the first entry ignored? > +/* Check if there *might* be a 16-bit branch or jump at OFFSET > + with a fixed or variable length delay slot. */ > + > +static int > +relax_dslot_norel16 (bfd *abfd, bfd_byte *ptr) There's no parameter called "offset". How about: /* If PTR is known to point to a 16-bit branch or jump, return the minimum length of its delay slot, otherwise return 0. */ At least, that's what the code seems to do, since it returns 2 for things like: > + { /* "b(g|l)(e|t)z", "s,p", */ 0x40000000, 0xff200000 }, which as far as I can tell from the opcodes patch allows both 16-bit and 32-bit delay slots. (IV-g doesn't seem to be public yet, so I can't check the spec.) But from the way the function is used, it looks like 0 really means "no size constraints", so why doesn't the function return 0 for instructions like these? Why are these routines checking for branches anyway? We don't support branches without relocations when doing this kind of relaxation. I'd have expected only indirect jumps to be handled here. Likewise relax_dslot_norel32. > + /* We don't have to do anything for a relocatable link, if > + this section does not have relocs, or if this is not a > + code section. */ > + > + if (link_info->relocatable > + || (sec->flags & SEC_RELOC) == 0 > + || sec->reloc_count == 0 > + || (sec->flags & SEC_CODE) == 0) > + return TRUE; Should we also check whether the micromips bit is set in the ELF header flags? > + nextopc = bfd_get_16 (abfd, contents + irel->r_offset + 4); > + /* B16 */ > + if (MATCH (nextopc, b_insn_16)) > + break; > + /* JR16 */ > + if (MATCH (nextopc, jr_insn_16) && reg != JR16_REG (nextopc)) > + break; > + /* BEQZ16, BNEZ16 */ > + if (MATCH (nextopc, bz_insn_16) && reg != BZ16_REG (nextopc)) > + break; > + /* JALR16 */ > + if (MATCH (nextopc, jalr_insn_16_bd32) > + && reg != JR16_REG (nextopc) && reg != RA) > + break; > + continue; I think this should be split out into a subfunction, like the relax_* ones. /* PTR points to a 2-byte instruction. Return true if it is a 16-bit branch that does not use register REG. */ static bfd_boolean is_16bit_branch_p (bfd *abfd, bfd_byte *ptr, unsigned int reg) Likewise for the 4-byte case. > + /* Give up if not the same register used with both relocations. */ "Give up unless the same register is used with both relocations." > + /* Now adjust pcrval, subtracting the offset to the LO16 reloc, > + adding the size of the LUI instruction deleted and rounding > + up to take masking of the two LSBs into account. */ > + pcrval = ((pcrval - offset + 4 + 3) | 3) ^ 3; Maybe its just me, but since pcrval was calculated much earlier, and hasn't been used since being set, I'd find it easier to understand if we simply calculate the pcrval for irel[1] directly: /* Calculate the pc-relative distance of the target from the LO16, assuming that we can delete the 4-byte LUI. */ pcrval = (symval - (sec->output_section->vma + sec->output_offset) - (irel[1].r_offset - 4)); /* Round up to take the masking of the two LSBs into account. */ pcrval = (prcval + 3) & -4; What happens when we can't delete the LUI, because of its being in a branch delay slot? > + int bdsize = -1; [...] > + /* See if there is a jump or a branch reloc preceding the > + LUI instruction immediately. If so, then perform jump > + or branch relaxation. */ > + for (ibrrel = internal_relocs; ibrrel < irelend; ibrrel++) > + { > + offset = irel->r_offset - ibrrel->r_offset; > + if (offset != 2 && offset != 4) > + continue; > + > + br_r_type = ELF32_R_TYPE (ibrrel->r_info); > + if (offset == 2 > + && br_r_type != R_MICROMIPS_PC7_S1 > + && br_r_type != R_MICROMIPS_PC10_S1 > + && br_r_type != R_MICROMIPS_JALR) > + continue; > + if (offset == 4 > + && br_r_type != R_MICROMIPS_26_S1 > + && br_r_type != R_MICROMIPS_PC16_S1 > + && br_r_type != R_MICROMIPS_JALR) > + continue; > + > + bdsize = relax_dslot_rel (abfd, > + contents + ibrrel->r_offset, offset); > + break; > + } > + > + /* Otherwise see if the LUI instruction *might* be in a > + branch delay slot. */ > + if (bdsize == -1) > + { > + bfd_byte *ptr = contents + ibrrel->r_offset; > + > + /* Assume the 32-bit LUI instruction is in a fixed delay slot > + and cannot be optimised away. */ > + bdsize = 4; > + > + if (ibrrel->r_offset >= 2) > + bdsize16 = relax_dslot_norel16 (abfd, ptr - 2); > + if (ibrrel->r_offset >= 4) > + bdsize32 = relax_dslot_norel32 (abfd, ptr - 4); I think the "ibrrel"s in the last block should be "irel"s instead. As best I can tell, "ibrrel" == "irelend" here. Is this case covered by the testsuite? I'm not sure whether I like the idea of making this function quadratic simply to decide whether the preceding instruction is a branch, but there's probably not much we can do about that. Richard