From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10383 invoked by alias); 26 May 2010 20:19:36 -0000 Received: (qmail 10346 invoked by uid 22791); 26 May 2010 20:19:21 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,TW_DF,TW_YM X-Spam-Check-By: sourceware.org Received: from mail-ww0-f41.google.com (HELO mail-ww0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 26 May 2010 20:19:14 +0000 Received: by wwi18 with SMTP id 18so1603380wwi.0 for ; Wed, 26 May 2010 13:19:11 -0700 (PDT) Received: by 10.227.145.77 with SMTP id c13mr9108964wbv.140.1274905150875; Wed, 26 May 2010 13:19:10 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id l23sm2846525wbb.14.2010.05.26.13.19.08 (version=TLSv1/SSLv3 cipher=RC4-MD5); Wed, 26 May 2010 13:19:09 -0700 (PDT) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,binutils@sourceware.org, Chao-ying Fu , Ilie Garbacea , Joseph Myers , Catherine Moore , Daniel Jacobowitz , rdsandiford@googlemail.com Cc: binutils@sourceware.org, Chao-ying Fu , Ilie Garbacea , Joseph Myers , Catherine Moore , Daniel Jacobowitz Subject: Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support References: Date: Wed, 26 May 2010 20:19:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Tue, 18 May 2010 19:18:29 +0100 (BST)") Message-ID: <87wruqxvpg.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (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-05/txt/msg00367.txt.bz2 Part 2 of the review. This testcase: .set micromips .fill 0x80 b16 .-0x80 produces: /tmp/foo.s: Assembler messages: /tmp/foo.s:3: Error: unrecognized opcode `b16 .-0x80' while: .set micromips .fill 0x80 b .-0x80 successfully produces a 16-bit insn. @@ -14813,6 +16230,8 @@ mips_elf_final_processing (void) file_ase_mt is true. */ if (file_ase_mips16) elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_M16; + if (file_ase_micromips) + elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_MICROMIPS; #if 0 /* XXX FIXME */ if (file_ase_mips3d) elf_elfheader (stdoutput)->e_flags |= ???; Do you really only want this flag to be set if -mmicromips was passed on the command line? (Yes, the same concern applies to MIPS16 and MDMX.) Lots of cases where a space is missing before "(". Please be consistent about "str[n]cmp (...) == 0" vs '!str[n]cmp (...)': use one or the other. (IMO the first is clearer.) micromips_ip obviously started life as a cut-&-paste of mips_ip, and it would have been nice to factor some code out. At least split out the block beginning: + case 'F': + case 'L': + case 'f': + case 'l': + { which is identical between the two, and far too subtle to copy wholesale. There may be other good opportunities too. + case '(': + /* Handle optional base register. + Either the base register is omitted or + we must have a left paren. */ + /* This is dependent on the next operand specifier + is a base register specification. */ + gas_assert (args[1] == 'b' + || (args[1] == 'm' + && (args[2] == 'l' || args[2] == 'n' + || args[2] == 's' || args[2] == 'a'))); + if (*s == '\0' && args[1] == 'b') + return; + + case ')': /* these must match exactly */ + case '[': + case ']': + if (*s++ == *args) + continue; + break; Mark fallthrough. + case 'D': /* floating point destination register */ + case 'S': /* floating point source register */ + case 'T': /* floating point target register */ + case 'R': /* floating point source register */ + case 'V': + rtype = RTYPE_FPU; + s_reset = s; + if (reg_lookup (&s, rtype, ®no)) + { + if ((regno & 1) != 0 + && HAVE_32BIT_FPRS + && ! mips_oddfpreg_ok (ip->insn_mo, argnum)) + as_warn (_("Float register should be even, was %d"), + regno); + + c = *args; + if (*s == ' ') + ++s; + if (args[1] != *s) + { + if (c == 'V' || c == 'W') + { + regno = lastregno; + s = s_reset; + ++args; + } + } + switch (c) + { + case 'D': + MICROMIPS_INSERT_OPERAND (FD, *ip, regno); + break; + case 'V': + case 'S': + MICROMIPS_INSERT_OPERAND (FS, *ip, regno); + break; + + case 'T': + MICROMIPS_INSERT_OPERAND (FT, *ip, regno); + break; + + case 'R': + MICROMIPS_INSERT_OPERAND (FR, *ip, regno); + break; + } + lastregno = regno; + continue; + } + + switch (*args++) + { + case 'V': + MICROMIPS_INSERT_OPERAND (FS, *ip, lastregno); + continue; + case 'W': + MICROMIPS_INSERT_OPERAND (FT, *ip, lastregno); + continue; + } + break; This block doesn't have a 'W' case (which doesn't seem to be used for micromips at all), so all the 'W' handling is dead code. + case 4: + case 5: + case 6: Too many spaces. + if (insn + 1 < µmips_opcodes[bfd_micromips_num_opcodes] && + !strcmp (insn->name, insn[1].name)) Misplaced "&&". + = {BFD_RELOC_UNUSED, BFD_RELOC_UNUSED, BFD_RELOC_UNUSED};; Double ";". + macro_read_relocs (&args, r); Not portable in this context (and doesn't build on x86_64-linux-gnu). You're taking the address of a va_list argument rather than a va_list local variable. + /* For microMIPS, we always use relocations for branches. + So, we should not resolve immediate values. */ Too many spaces. + if (ep->X_op == O_constant) + abort (); + else + *r = BFD_RELOC_MICROMIPS_16_PCREL_S1; gcc_assert (ep->X_op != O_constant); *r = BFD_RELOC_MICROMIPS_16_PCREL_S1; The same cut-&-paste concerns apply to micromips_macro, which obviously started out as a copy of macro(). I realise there are special cases for micromips (such as the DADDI assymmetry and the lack of branch-likely instructions, to name only a few), but most of the basic decisions are the same. As it stands, we have a new 3524 line function, of which I imagine at least 90% is shared with macro(). I really think the new microMIPS macro handling should be integrated into macro() instead. Don't be afraid of factoring out code from macro() if it makes it easier to integrate the microMIPS code. #define CPU_MIPS5 5 #define CPU_MIPS64 64 #define CPU_MIPS64R2 65 +#define CPU_MICROMIPS 96 #define CPU_SB1 12310201 /* octal 'SB', 01. */ #define CPU_LOONGSON_2E 3001 #define CPU_LOONGSON_2F 3002 What's this for? It doesn't seem to be used. + "mA" 7-bit immediate (-63 .. 64) << 2 (MICROMIPSOP_*_IMMA) Don't you mean (-64 .. 63)? + "mB" 3-bit immediate (0, -1, 1, 4, 8, 12, 16, 20, 24) (MICROMIPSOP_*_IMMB) That's nine values. Should the 0 really be there? @@ -630,15 +630,15 @@ proc strip_executable { prog flags test remote_upload host ${copyfile} tmpdir/striprog } - set result [remote_load target tmpdir/striprog] - set status [lindex $result 0] - if { ![istarget $host_triplet] } { - set status "pass" - } - if { $status != "pass" } { - fail $test - return - } +# set result [remote_load target tmpdir/striprog] +# set status [lindex $result 0] +# if { ![istarget $host_triplet] } { +# set status "pass" +# } +# if { $status != "pass" } { +# fail $test +# return +# } set exec_output [binutils_run $NM "$NMFLAGS ${copyfile}"] if ![string match "*: no symbols*" $exec_output] { @@ -673,15 +673,15 @@ proc strip_executable_with_saving_a_symb remote_upload host ${copyfile} tmpdir/striprog } - set result [remote_load target tmpdir/striprog] - set status [lindex $result 0] - if { ![istarget $host_triplet] } { - set status "pass" - } - if { $status != "pass" } { - fail $test - return - } +# set result [remote_load target tmpdir/striprog] +# set status [lindex $result 0] +# if { ![istarget $host_triplet] } { +# set status "pass" +# } +# if { $status != "pass" } { +# fail $test +# return +# } set exec_output [binutils_run $NM "$NMFLAGS ${copyfile}"] if { [istarget mmix-knuth-mmixware] } { Looks like these crept in unawares. PLT entries and traditional MIPS lazy binding stubs. We mark the former with STO_MIPS_PLT to distinguish them from the latter. */ #define STO_MIPS_PLT 0x8 +#define ELF_ST_IS_MIPS_PLT(OTHER) (((OTHER) & 0x8) == STO_MIPS_PLT) ... #define STO_MIPS_PIC 0x20 #define ELF_ST_IS_MIPS_PIC(OTHER) \ - (((OTHER) & ~ELF_ST_VISIBILITY (-1)) == STO_MIPS_PIC) + (((OTHER) & ~ELF_ST_VISIBILITY (-1) & ~0xc0) == STO_MIPS_PIC) #define ELF_ST_SET_MIPS_PIC(OTHER) \ - (STO_MIPS_PIC | ELF_ST_VISIBILITY (OTHER)) + (STO_MIPS_PIC | ELF_ST_VISIBILITY (OTHER) | ELF_ST_MICROMIPS (OTHER)) ... case STO_OPTIONAL: return "OPTIONAL"; case STO_MIPS16: return "MIPS16"; - case STO_MIPS_PLT: return "MIPS PLT"; - case STO_MIPS_PIC: return "MIPS PIC"; - default: return NULL; + default: + if (ELF_ST_IS_MIPS_PLT (other)) + { + if (ELF_ST_IS_MICROMIPS (other)) + return "MICROMIPS, MIPS PLT"; + else + return "MIPS PLT"; + } + if (ELF_ST_IS_MIPS_PIC (other)) + { + if (ELF_ST_IS_MICROMIPS (other)) + return "MICROMIPS, MIPS PIC"; + else + return "MIPS PIC"; + } + if (ELF_ST_IS_MICROMIPS (other)) + return "MICROMIPS"; + + return NULL; You don't add support for microMIPS PLTs, so the "MICROMIPS, MIPS PLT" thing appears to be dead code. I wouldn't mind, except that it makes the code inconsistent with MIPS16: the changes above allow both STO_MIPS16 | STO_MIPS_PLT and STO_MICROMIPS | STO_MIPS_PLT neither of which are currently used, but you don't treat the two equally. In other words, I'm happy with the STO_MIPS_PIC changes but not with the STO_MIPS_PLT ones. + /* 16 bit relocation. */ + HOWTO (R_MICROMIPS_16, /* type */ + 0, /* rightshift */ + 2, /* size (0 = byte, 1 = short, 2 = long) */ + 16, /* bitsize */ + FALSE, /* pc_relative */ + 0, /* bitpos */ + complain_overflow_dont, /* complain_on_overflow */ + _bfd_mips_elf_lo16_reloc, /* special_function */ + "R_MICROMIPS_16", /* name */ + TRUE, /* partial_inplace */ + 0x0000ffff, /* src_mask */ + 0x0000ffff, /* dst_mask */ + FALSE), /* pcrel_offset */ Is this relocation really a lo16 one? If so, it's not consistent with R_MIPS_16 (which isn't). +/* + * Delay slot size and relaxation support + */ +static unsigned long +relax_delay_slot (bfd *abfd, bfd_byte *ptr, unsigned long* opcode_32) +{ + unsigned long bdsize = 0; + + unsigned long fndopc; + unsigned long p16opc, p32opc; + + /* Opcodes preceding the current instruction. */ + p16opc = bfd_get_16 (abfd, ptr - 2); + p32opc = bfd_get_16 (abfd, ptr - 4) << 16; You need to check the section bounds. The code appears to read off the beginning of a section if that section starts with a relaxable LUI. + /* If this isn't something that can be relaxed, then ignore + this reloc. */ + if (ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_HI16 && + ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_LO16 && + ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_PC16_S1 && + ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_26_S1 && + ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_GPREL16) + continue; &&s at the beginning of lines. + /* Get the value of the symbol referred to by the reloc. */ + if (ELF32_R_SYM (irel->r_info) < symtab_hdr->sh_info) + { + /* A local symbol. */ + Elf_Internal_Sym *isym; + asection *sym_sec; + + isym = isymbuf + ELF32_R_SYM (irel->r_info); + if (isym->st_shndx == SHN_UNDEF) + sym_sec = bfd_und_section_ptr; + else if (isym->st_shndx == SHN_ABS) + sym_sec = bfd_abs_section_ptr; + else if (isym->st_shndx == SHN_COMMON) + sym_sec = bfd_com_section_ptr; + else + sym_sec = bfd_section_from_elf_index (abfd, isym->st_shndx); + symval = (isym->st_value + + sym_sec->output_section->vma + + sym_sec->output_offset); + target_is_micromips_code_p = ELF_ST_IS_MICROMIPS (isym->st_other); + } + else + { + unsigned long indx; + struct elf_link_hash_entry *h; + + /* An external symbol. */ + indx = ELF32_R_SYM (irel->r_info) - symtab_hdr->sh_info; + h = elf_sym_hashes (abfd)[indx]; + BFD_ASSERT (h != NULL); + + if (h->root.type != bfd_link_hash_defined + && h->root.type != bfd_link_hash_defweak) + /* This appears to be a reference to an undefined + symbol. Just ignore it -- it will be caught by the + regular reloc processing. */ + continue; + + symval = (h->root.u.def.value + + h->root.u.def.section->output_section->vma + + h->root.u.def.section->output_offset); + } Why not set target_is_micromips_code_p for locally-binding global symbols too? + opcode = bfd_get_16 (abfd, contents + irel->r_offset ) << 16; + opcode |= (irel->r_offset + 2 < sec->size + ? bfd_get_16 (abfd, contents + irel->r_offset + 2) : 0); Are there any relaxations we can actually do if irel->r_offset + 2 >= sec->size? I couldn't see any. If there aren't, continue instead. + /* R_MICROMIPS_HI16 / LUI relaxation to R_MICROMIPS_HI0_LO16 or + R_MICROMIPS_PC23_S2. The R_MICROMIPS_PC23_S2 condition is + + (symval % 4 == 0 && IS_BITSIZE (pcrval, X, 25)) + + where the X adjustment compensate for R_MICROMIPS_HI16 and + R_MICROMIPS_LO16 being at most X bytes appart when the + distance to the target approaches 32 MB. */ Comment is slightly confusing: we don't relax the HI16 itself to a H0_LO16 or PC32_S3. Rather we relax the LO16 (or, if you like, the HI16/LO16 pair). + /* Assume is possible to delete the LUI instruction: + 4 bytes at irel->r_offset. */ s/Assume is/Assume it is/ + /* Compact branch relaxation -- due to the multitude of macros + employed by the compiler/assembler, compact branches are not + aways generated. Obviously, this can/will be fixed elsewhere, + but there is no drawback in double checking it here. */ + else if (ELF32_R_TYPE (irel->r_info) == (int) R_MICROMIPS_PC16_S1 + && (fndopc = find_match (opcode, bz_insns_32)) != 0 + && MATCH (bfd_get_16 (abfd, contents + irel->r_offset + 4), + nop_insn_16)) s/aways/always/. You should check the section size before reading past the relocation. (I realise there ought to be a delay slot, but we should be robust against brokenness.) + /* R_MICROMIPS_26_S1 -- JAL to JALS relaxation for microMIPS targets. */ + else if (ELF32_R_TYPE (irel->r_info) == (int) R_MICROMIPS_26_S1 + && target_is_micromips_code_p + && MATCH (opcode, jal_insn_32_bd32)) + { + unsigned long n32opc; + bfd_boolean relaxed = FALSE; + + n32opc = + bfd_get_16 (abfd, contents + irel->r_offset + 4 ) << 16; + n32opc |= irel->r_offset + 2 < sec->size? + bfd_get_16 (abfd, contents + irel->r_offset + 6): 0; + Here too you should check the size before reading n32opc. Second condition looks bogus (did you mean +6?) although the same concerns apply as for the "+ 2" above. I'll try to review more soon. Richard