This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
| Other format: | [Raw text] | |
Hi Richard,
I'm combining a couple of e-mails together, quotations may come from
various people -- hopefully no one will get lost and hopefully I haven't
missed anything. ;) [FIXME] are my annotations for future reference.
NathanF, there's a QEMU question somewhere down the e-mail -- would you
care to answer it?
> > > > (A_BFD_RELOC_HI16_S, A_BFD_RELOC_HI16, A_BFD_RELOC_LO16): New
> > > > relocation wrapper macros.
> > > > (A_BFD_RELOC_GPREL16): Likewise.
> > > > (A_BFD_RELOC_MIPS_GOT16, A_BFD_RELOC_MIPS_GOT_HI16): Likewise.
> > > > (A_BFD_RELOC_MIPS_GOT_LO16, A_BFD_RELOC_MIPS_HIGHEST): Likewise.
> > > > (A_BFD_RELOC_MIPS_HIGHER, A_BFD_RELOC_MIPS_GOT_DISP): Likewise.
> > > > (A_BFD_RELOC_MIPS_GOT_PAGE, A_BFD_RELOC_MIPS_GOT_OFST):
> > Likewise.
> > > > (A_BFD_RELOC_MIPS_SUB, A_BFD_RELOC_MIPS_JALR): Likewise.
> > >
> > > Did you consider doing the translation from non-microMIPS to
> > > microMIPS in the macro_* functions, rather than in their callers?
> > > I fear it'll be too easy to accidentally forget to use
> > A_BFD_* in future.
> >
> > Agreed, I didn't like the new macros from the very
> > beginning. Chao-ying
> > -- any thoughts?
>
> I am fine. But the new method may be bigger than the A_BFD* method.
I have placed the translation in macro_map_reloc() now. That incurs an
O(n) performance penalty because all relocations in the microMIPS mode
have to be iterated over the table of mappings; regrettably BFD_RELOC_*
definitions are sparse so an O(1) lookup table cannot be used. By keeping
it sorted some time can be saved, but that's still O(n). This could be
reduced to O(log n) with a binary search, but I fear with the limited
number of relocs handled the overhead would kill the benefit.
And macro_build_jalr() and macro_build_lui() handle the stuff explicitly
now.
> > (MICROMIPS_TARGET, MICROMIPS_TARGET_LABEL): New macros.
> > (micromips_add_number_label): New function.
>
> +/* For microMIPS macros, we need to generate a local number label
> + as the target of branches. */
> +#define MICROMIPS_TARGET "2147483647f"
> +#define MICROMIPS_TARGET_LABEL 2147483647
> +
> +static void
> +micromips_add_number_label (void)
> +{
> + symbolS *s;
> + fb_label_instance_inc (MICROMIPS_TARGET_LABEL);
> + s = colon (fb_label_name (MICROMIPS_TARGET_LABEL, 0));
> + S_SET_OTHER (s, ELF_ST_SET_MICROMIPS (S_GET_OTHER (s)));
> +}
> +
>
> Ugh, this is a bit hackish. There's nothing stopping a user using
> 2147483647f themselves.
This is now handled by micromips_label_name(), micromips_label_expr(),
micromips_label_inc() and micromips_add_label() in a manner similar to
what dollar_label_name() and fb_label_name(), etc. do, except that the
special character used in symbols generated is ^_, avoiding a clash.
> > > > (macro_start, macro_warning, macro_end): Likewise.
> > >
> > > + else if ((subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
> > > + || (subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND))
> > > + return _("Macro instruction of the wrong size in a
> > branch delay slot"
> > > + " that requires a 16-bit or 32-bit instruction");
> > >
> > > Did you consider adding a flag to distinguish the 32-bit
> > and 16-bit cases?
> > > It'd be nice to be consistent with the non-relaxed error if
> > possible.
> >
> > Chao-ying? I've had a look actually and flag may not be
> > necessary to get
> > the functionality. I'm fixing this up elsewhere already.
>
> I am fine with this functionality. Maybe passing one more parameter to
> macro_warning() can help to distinguish two cases.
Done now. I have added RELAX_DELAY_SLOT_16BIT now.
OTOH, I don't think it is possible to get a macro to expand into a single
16-bit instruction that would be unsuitable for a 32-bit delay slot -- all
the 16-bit instructions that can be emitted by macros this way have a
32-bit counterpart that will be used instead. I think the extra message
is good for sanity though or in case things change in the future.
> > > + /* If either one implementation contains one
> > instruction, we need to check
> > > + the delay slot size requirement. */
> > > + if (mips_macro_warning.num_insns[0] == 1
> > > + || mips_macro_warning.num_insns[1] == 1)
> > > + {
> > > + if (mips_macro_warning.num_insns[0] ==
> > mips_macro_warning.num_insns[1]
> > > + && mips_macro_warning.sizes[0] == mips_macro_warning.sizes[1])
> > > + {
> > > + /* Either the macro has a single implementation or both
> > > + implementations are 1 instruction with the same size.
> > > + Emit the warning now. */
> > > + if ((mips_macro_warning.delay_slot_16bit_p
> > > + && mips_macro_warning.sizes[0] != 2)
> > > + || (mips_macro_warning.delay_slot_32bit_p
> > > + && mips_macro_warning.sizes[0] != 4))
> > > + {
> > > + const char *msg;
> > > + msg = macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST);
> > > + if (msg != 0)
> > > + as_warn (msg);
> > > + }
> > > + }
> > > + else
> > > + {
> > > + relax_substateT subtype;
> > > +
> > > + /* Set up the relaxation warning flags. */
> > > + subtype = 0;
> > > + if (mips_macro_warning.delay_slot_16bit_p)
> > > + {
> > > + if (mips_macro_warning.num_insns[0] != 1
> > > + || mips_macro_warning.sizes[0] != 2)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > > + if (mips_macro_warning.num_insns[1] != 1
> > > + || mips_macro_warning.sizes[1] != 2)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > > + }
> > > + if (mips_macro_warning.delay_slot_32bit_p)
> > > + {
> > > + if (mips_macro_warning.num_insns[0] != 1
> > > + || mips_macro_warning.sizes[0] != 4)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > > + if (mips_macro_warning.num_insns[1] != 1
> > > + || mips_macro_warning.sizes[1] != 4)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > > + }
> > > +
> > > + /* One implementation might need a warning but the other
> > > + definitely doesn't. */
> > > + mips_macro_warning.first_frag->fr_subtype |= subtype;
> > > + }
> > > + }
> > >
> > > Why not work out the subtype, then check whether both
> > ERROR_FIRST and
> > > ERROR_SECOND are set?
> >
> > Chao-ying?
>
> Do you mean this kind of code?
> Ex:
> if (mips_macro_warning.num_insns[0] == 1
> || mips_macro_warning.num_insns[1] == 1)
> {
> if (mips_macro_warning.delay_slot_16bit_p)
> {
> if (mips_macro_warning.num_insns[0] != 1
> || mips_macro_warning.sizes[0] != 2)
> subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> if (mips_macro_warning.num_insns[1] != 1
> || mips_macro_warning.sizes[1] != 2)
> subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> }
> else if (mips_macro_warning.delay_slot_32bit_p)
> {
> if (mips_macro_warning.num_insns[0] != 1
> || mips_macro_warning.sizes[0] != 4)
> subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> if (mips_macro_warning.num_insns[1] != 1
> || mips_macro_warning.sizes[1] != 4)
> subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> }
>
> if ((subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
> && (subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND))
> {
> const char *msg;
> msg = macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST); //
> NEED TO PASS 16-bit or 32-bit info
> if (msg != 0)
> as_warn (msg);
> }
> else
> {
> /* One implementation might need a warning but the other
> definitely doesn't. */
> mips_macro_warning.first_frag->fr_subtype |= subtype;
> }
> }
>
> >
> > > + if (mips_macro_warning.delay_slot_p)
> > > + {
> > > + if (mips_macro_warning.num_insns[0] > 1)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > > + if (mips_macro_warning.num_insns[1] > 1)
> > > + subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > > + }
> > >
> > > I don't get why this hunk is needed. I thought ERROR_FIRST
> > and ERROR_SECOND
> > > controlled cases where a macro has a single-insn expansion
> > that is the
> > > wrong size, which ought to be handled by the block above.
> > If the code
> > > really is needed, you should add a comment explaining why.
> >
> > Chao-ying?
>
>
> I agree. The delay_slot_p check is duplicated, and we can discard it
> for
> ERROR_FIRST and ERROR_SECOND when num_insns[]>1. My old code
> double-checked the condition for
> number of instructions in delay slots.
I have rewritten macro_end() entirely now. I have changed the approach
such that if multiple instructions are emitted into a branch delay slot
and the first of these instructions is out of size, then two warnings are
produced.
The rationale is these are different classes of warnings -- the delay
slot size mismatch is fatal if the call is ever returned from. Multiple
instructions may or may not be a problem depending on the actual piece of
code and author's intentions. We had a discussion about it a few years
ago. ;)
To complete these changes I have modified the change to append_insn()
such that no warning is produced for a delay slot size mismatch if called
for a macro expansion as it would get in the way, especially in the case
of relaxation. The API of this function had to be modified in a trivial
way and all the callers adjusted accordingly.
> > > > (macro_build): Likewise.
> > >
> > > + if (mips_opts.micromips)
> > > + {
> > > + if (strcmp (name, "lui") == 0)
> > > + micromips_macro_build (ep, name, "s,u", args);
> > > + else if (strcmp (fmt, "d,w,<") == 0)
> > > + micromips_macro_build (ep, name, "t,r,<", args);
> > > + else
> > > + micromips_macro_build (ep, name, fmt, args);
> > > + va_end (args);
> > > + return;
> > > + }
> > >
> > > A bit of commentary might help explain the letter switch here.
> >
> > Chao-ying?
>
> Because I didn't change all the code before calling LUI macro for
> microMIPS, I need
> to magically change the operand string for microMIPS to use "s,u" for
> microMIPS LUI.
> "d,w,<" is another case that we need to map it to "t,r,<" for microMIPS.
> If we can search all the places and replace all calls with correct
> operand strings for microMIPS,
> this special code can be dropped.
I have fixed it up now with the introduction of lui_fmt[] and shft_fmt[].
> > > > (macro_build_jalr): Likewise.
> > >
> > > + if (mips_opts.micromips)
> > > + {
> > > + if (HAVE_NEWABI)
> > > + macro_build (NULL, "jalr", "t,s", RA, PIC_CALL_REG);
> > > + else
> > > + macro_build (NULL, "jalr", "mj", PIC_CALL_REG);
> > > + }
> > > + else
> > > + macro_build (NULL, "jalr", "d,s", RA, PIC_CALL_REG);
> > >
> > > Why HAVE_NEWABI? Do you want a 32-bit insn for R_MIPS_JALR?
> > > If so, you should check MIPS_JALR_HINT_P (ep) instead.
> >
> > Chao-ying?
>
> OK. This code is done before I put the R_MIPS_JALR patch into GAS and
> LD.
> The new code is as follows.
> Ex:
> static void
> macro_build_jalr (expressionS *ep)
> {
> char *f = NULL;
>
> if (MIPS_JALR_HINT_P (ep))
> {
> frag_grow (8);
> f = frag_more (0);
> }
> if (mips_opts.micromips)
> macro_build (NULL, "jalr", "t,s", RA, PIC_CALL_REG);
> else
> macro_build (NULL, "jalr", "d,s", RA, PIC_CALL_REG);
> if (MIPS_JALR_HINT_P (ep))
> fix_new_exp (frag_now, f - frag_now->fr_literal,
> 4, ep, FALSE, A_BFD_RELOC_MIPS_JALR); // THIS MAY BE
> FIXED BY A NEW METHOD.
> }
>
> And, we need to modify elfxx-mips.c to support
> BFD_RELOC_MICROMIPS_JALR to convert jalr to bal for microMIPS.
Per your suggestion, I have made this piece use MIPS_JALR_HINT_P() to
select the jump now. Also since for PIC the delay slot is always a NOP in
the reorder mode, I switched to the use of JALRS in this case.
> + case BFD_RELOC_MICROMIPS_7_PCREL_S1:
> + case BFD_RELOC_MICROMIPS_10_PCREL_S1:
> + case BFD_RELOC_MICROMIPS_16_PCREL_S1:
> + /* We adjust the offset back to even. */
> + if ((*valP & 0x1) != 0)
> + --(*valP);
> +
> + if (! fixP->fx_done)
> + break;
> +
> + /* Should never visit here, because we keep the relocation. */
> + abort ();
> + break;
>
> I suppose this silently ignores branches to non-microMIPS code,
> but there again, so does the MIPS16 equivalent...
As noted previously, some diagnostics would be useful, but let's call it
a future enhancement. [FIXME]
> >> > (mips_relax_frag): Handle microMIPS.
> >>
> >> + gas_assert (fixp->fx_r_type == BFD_RELOC_16_PCREL_S2
> >> + || fixp->fx_r_type == BFD_RELOC_MICROMIPS_7_PCREL_S1
> >> + || fixp->fx_r_type == BFD_RELOC_MICROMIPS_10_PCREL_S1
> >> + || fixp->fx_r_type == BFD_RELOC_MICROMIPS_16_PCREL_S1);
> >> +
> >> + /* For 7/10 PCREL_S1, we just need to use
> >> fixp->fx_addnumber. */
> >> + if (fixp->fx_r_type == BFD_RELOC_MICROMIPS_7_PCREL_S1
> >> + || fixp->fx_r_type == BFD_RELOC_MICROMIPS_10_PCREL_S1)
> >> + reloc->addend = fixp->fx_addnumber;
> >> + else
> >> + /* At this point, fx_addnumber is "symbol offset -
> >> pcrel address".
> >> + Relocations want only the symbol offset. */
> >> + reloc->addend = fixp->fx_addnumber + reloc->address;
> >>
> >> A better comment is needed. _Why_ do you just need fx_addnumber?
> >>
> >
> > Thanks for the review! The explanation is in another place as
> > follows.
> > Maybe we need to copy the comment to tc_gen_reloc from md_pcrel_from.
> > Ex:
> > long
> > md_pcrel_from (fixS *fixP)
> > {
> > valueT addr = fixP->fx_where + fixP->fx_frag->fr_address;
> > switch (fixP->fx_r_type)
> > {
> > /* We don't add addr, because it will cause the error checking of
> > "addnumber" fail in write.c for *7/10_PCREL_S1.
> > In tc_gen_reloc, we just use fixp->fx_addnumber. */
> > case BFD_RELOC_MICROMIPS_7_PCREL_S1:
> > case BFD_RELOC_MICROMIPS_10_PCREL_S1:
> > /* Return the beginning of the delay slot from the current insn.
> > */
> > return 2;
> >
> > case BFD_RELOC_MICROMIPS_16_PCREL_S1:
> > case BFD_RELOC_MICROMIPS_JMP:
> > case BFD_RELOC_16_PCREL_S2:
> > case BFD_RELOC_MIPS_JMP:
> > /* Return the address of the delay slot. */
> > return addr + 4;
> > ...
> > The field of *7/10_PCREL_S1 is limited in the 16-bit instructions.
> > If we add the "address", write.c will fail to check these two
> > relocations due to overflow or something (I kind of forgot). From
> > debugging, adding "address" is no use at all, because later "address" is
> > subtracted.
>
> Ah, thanks, that's a good explanation. Yeah, at least a cross-reference
> would be useful if we keep things as they are. However...
>
> ...I think you mean this bit of write.c:
>
> if (fixP->fx_size < sizeof (valueT) && 0)
> {
> valueT mask;
>
> mask = 0;
> mask--; /* Set all bits to one. */
> mask <<= fixP->fx_size * 8 - (fixP->fx_signed ? 1 : 0);
> if ((add_number & mask) != 0 && (add_number & mask) != mask)
> {
> char buf[50], buf2[50];
> sprint_value (buf, fragP->fr_address + fixP->fx_where);
> if (add_number > 1000)
> sprint_value (buf2, add_number);
> else
> sprintf (buf2, "%ld", (long) add_number);
> as_bad_where (fixP->fx_file, fixP->fx_line,
> _("value of %s too large for field of %d bytes
> at %s"),
> buf2, fixP->fx_size, buf);
> } /* Generic error checking. */
> }
>
> That check's bogus for these relocations anyway, since it doesn't take
> the implied shift into account. I think there's an argument to say
> we should set fx_no_overflow for these relocations and leave the
> overflow checking to bfd. You'll still get a "relocation overflow"
> error if the final in-place addend really is too big.
After some investigation I decided to follow your suggestion. I have
removed this change and the corresponding one from tc_gen_reloc() and
adjusted md_convert_frag() instead.
My general conclusion is fixup_segment() code fragment you quoted is
essentially broken and has to be rewritten. For the MIPS target it works
by chance and does not perform any useful check. It shouldn't be checking
the size of the data chunk being relocated, but the size of the
relocatable field only and take the shift value into account. This is all
available from the associated HOWTO structure that can be easily obtained
here.
> > > > (md_convert_frag): Likewise.
> > >
> > > - /* Possibly emit a warning if we've chosen the
> > longer option. */
> > > - if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
> > > - == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
> > > + if (!(fragp->fr_subtype & RELAX_USE_SECOND))
> > > + {
> > > + /* Check if the size in branch delay slot is ok. */
> > > + if (fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
> > > + {
> > > + const char *msg = macro_warning (fragp->fr_subtype);
> > > + if (msg != 0)
> > > + as_warn_where (fragp->fr_file, fragp->fr_line, msg);
> > > + }
> > > + }
> > > + else
> > > {
> > > - const char *msg = macro_warning (fragp->fr_subtype);
> > > - if (msg != 0)
> > > - as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg);
> > > + /* Check if the size in branch delay slot is ok.
> > > + Possibly emit a warning if we've chosen the longer
> > option. */
> > > + if ((fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND)
> > > + || (fragp->fr_subtype & RELAX_SECOND_LONGER))
> > > + {
> > > + const char *msg = macro_warning (fragp->fr_subtype);
> > > + if (msg != 0)
> > > + as_warn_where (fragp->fr_file, fragp->fr_line, msg);
> > > + }
> > > }
> > >
> > > This doesn't accurately preserve the previous:
> > >
> > > if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
> > > == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
> > >
> > > behaviour.
> >
> > Chao-ying?
>
> How about this?
>
> Ex:
>
> if ((((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
> == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
> || (!(fragp->fr_subtype & RELAX_USE_SECOND)
> && (fragp->fr_subtype &
> RELAX_DELAY_SLOT_SIZE_ERROR_FIRST))
> || ((fragp->fr_subtype & RELAX_USE_SECOND)
> && (fragp->fr_subtype &
> RELAX_DELAY_SLOT_SIZE_ERROR_SECOND)))
> {
> const char *msg = macro_warning (fragp->fr_subtype); // MAY
> NEED TO PASS 16-bit or 32-bit info
> if (msg != 0)
> as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg);
> }
I have rewritten it entirely now, matching changes to macro_end(). Two
warnings are emitted if multiple instructions are emitted into a branch
delay slot and the first of these instruction is out of size like with
macro_end().
> > > > (micromips_ip): New function.
> > >
> > > + /* Try to search "16" or "32" in the str. */
> > > + if ((t = strstr (str, "16")) != NULL && t < save_s)
> > > + {
> > > + /* Make sure "16" is before the first '.' if '.' exists. */
> > > + if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > > +
> > > + /* Make sure "16" is at the end of insn name, if no '.'. */
> > > + if ((s = strchr (str, '.')) == NULL
> > > + && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > > +
> > > + micromips_16 = TRUE;
> > > + for (s = t + 2; *s != '\0'; ++s)
> > > + *(s - 2) = *s;
> > > + *(s - 2) = '\0';
> > > +
> > > + for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
> > > + continue;
> > > +
> > > + if (ISSPACE (*s))
> > > + {
> > > + save_c = *s;
> > > + *s++ = '\0';
> > > + }
> > > +
> > > + if ((insn = (struct mips_opcode *) hash_find
> > (micromips_op_hash, str))
> > > + == NULL)
> > > + {
> > > + int i;
> > > + int length;
> > > + micromips_16 = FALSE;
> > > +
> > > + /* Restore the character we overwrite above (if any). */
> > > + if (save_c)
> > > + *(--s) = save_c;
> > > +
> > > + length = strlen (str);
> > > + for (i = length - 1; &str[i] >= t; i--)
> > > + {
> > > + str[i + 2] = str[i];
> > > + if (t == &str[i])
> > > + {
> > > + str[i + 1] = '6';
> > > + str[i] = '1';
> > > + str[length + 2] = '\0';
> > > + break;
> > > + }
> > > + }
> > > +
> > > + insn_error = "unrecognized 16-bit version of
> > microMIPS opcode";
> > > + return;
> > > + }
> > > + }
> > > + else if ((t = strstr (str, "32")) != NULL && t < save_s)
> > > + {
> > > + /* For some instruction names, we already have 32, so we need
> > > + to seek the second 32 to process. Ex: bposge3232,
> > dsra3232. */
> > > + char *new_t;
> > > + if ((new_t = strstr (t + 2, "32")) != NULL && new_t < save_s)
> > > + t = new_t;
> > > +
> > > + /* Make sure "32" is before the first '.' if '.' exists. */
> > > + if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > > +
> > > + /* Make sure "32" is at the end of the name, if no '.'. */
> > > + if ((s = strchr (str, '.')) == NULL
> > > + && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > > +
> > > + micromips_32 = TRUE;
> > > + for (s = t + 2; *s != '\0'; ++s)
> > > + *(s - 2) = *s;
> > > + *(s - 2) = '\0';
> > > +
> > > + for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
> > > + continue;
> > > +
> > > + if (ISSPACE (*s))
> > > + {
> > > + save_c = *s;
> > > + *s++ = '\0';
> > > + }
> > > +
> > > + if ((insn = (struct mips_opcode *) hash_find
> > (micromips_op_hash, str))
> > > + == NULL)
> > > + {
> > > + int i;
> > > + int length;
> > > + micromips_32 = FALSE;
> > > +
> > > + /* Restore the character we overwrite above (if any). */
> > > + if (save_c)
> > > + *(--s) = save_c;
> > > +
> > > + length = strlen (str);
> > > + for (i = length - 1; &str[i] >= t; i--)
> > > + {
> > > + str[i + 2] = str[i];
> > > + if (t == &str[i])
> > > + {
> > > + str[i + 1] = '2';
> > > + str[i] = '3';
> > > + str[length + 2] = '\0';
> > > + break;
> > > + }
> > > + }
> > > +
> > > + insn_error = "unrecognized 32-bit version of
> > microMIPS opcode";
> > > + return;
> > > + }
> > >
> > > Far too much cut-&-paste between the "16" and "32" cases. Also:
> > >
> > > + if ((t = strstr (str, "16")) != NULL && t < save_s)
> > >
> > > t < save_s must surely be true, since save_s is the null terminator.
>
> Yes. I used t < save_s, because I don't know save_s points to NULL at
> this point.
> We can discard save_s now.
>
> > >
> > > + /* Make sure "16" is before the first '.' if '.' exists. */
> > > + if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > > +
> > > + /* Make sure "16" is at the end of insn name, if no '.'. */
> > > + if ((s = strchr (str, '.')) == NULL
> > > + && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > > + {
> > > + insn_error = "unrecognized opcode";
> > > + return;
> > > + }
> > >
> > > Don't call strchr (str, '.') twice like that. Better would be:
> > >
> > > s = strchr (str, '.');
>
> Yes.
>
> > >
> > > followed by the two checks. Isn't the ISSPACE check
> > redundant though,
> > > given that you've terminated the string at the first space? I would
> > > have thought:
> > >
> > > if (t + 2 != (s ? s : save_s))
> > >
> > > would be enough. Errors should start with a capital
> > letter. Missing
> > > internationalisation.
> >
> > Chao-ying?
>
> Yes. "if (t + 2 != (s ? s : save_s))" is enough.
>
> >
> > > You could use alloca to create an opcode without the "16" or "32",
> > > which would make the error-reporting code simpler. It's best not
> > > to change the user's source line if we can help it.
> >
> > Agreed.
>
> Yes.
>
> >
> > > + if (!insn_error)
> > > + {
> > > + static char buf[100];
> > > + sprintf (buf,
> > > + _("opcode not supported on this
> > processor: %s (%s)"),
> > > + mips_cpu_info_from_arch
> > (mips_opts.arch)->name,
> > > + mips_cpu_info_from_isa
> > (mips_opts.isa)->name);
> > > + insn_error = buf;
> > > + }
> > > + if (save_c)
> > > + *(--s) = save_c;
> > > +
> > > + if (micromips_16 || micromips_32)
> > > + {
> > > + int i;
> > > + int length;
> > > +
> > > + length = strlen (str);
> > > + for (i = length - 1; i >= 0; i--)
> > > + {
> > > + str[i + 2] = str[i];
> > > + if (t == &str[i])
> > > + break;
> > > + }
> > > + if (micromips_16)
> > > + {
> > > + insn_error =
> > > + "unrecognized 16-bit version of
> > microMIPS opcode";
> > > + str[i + 1] = '6';
> > > + str[i] = '1';
> > > + }
> > > + else
> > > + {
> > > + insn_error =
> > > + "unrecognized 32-bit version of
> > microMIPS opcode";
> > > + str[i + 1] = '2';
> > > + str[i] = '3';
> > > + }
> > > + str[length + 2] = '\0';
> > > + }
> > > + return;
> > >
> > > Why override the insn_error unconditionally like this? E.g.:
> > >
> > > jar16 $30,$26
> > >
> > > Error: unrecognized 16-bit version of microMIPS opcode
> > `jar16 $30,$26'
> > >
> > > implies there's a 32-bit opcode. I'd also have thought that the
> > > "opcode not supported on this processor" would triumph if
> > it applies.
> >
> > The error you've seen comes from the previous hunk above
> > rather than this
> > one which I think is unnecessary code duplication. It's all rather
> > over-complicated and I'm working on getting it polished.
I have rewritten it from scratch using alloca() as you suggested and
reducing the whole stuff to a small loop executed once or twice. This
enabled chunks of code to be shaved off elsewhere too.
Your JAR16 case is now handled correctly and I have put together test
cases to cover some of this stuff, hopefully exhaustively.
> > too. Also the original loop seems ill-formed to me, with
> > most of code
> > intended to be executed at most once, after the loop's terminating
> > condition triggered -- i.e. that shouldn't be in the loop in
> > the first
> > place.
>
> What code in the loop do you refer to? I am not clear.
One that follows "argsStart = s". My conclusion turned out unjustified
as there are "continue" statements throughout -- I got confused, sorry.
> > * ld-mips-elf/jalx-2-main.s: New.
> > * ld-mips-elf/jalx-2.dd: New.
> > * ld-mips-elf/jalx-2-ex.s: New.
> > * ld-mips-elf/jalx-2-printf.s: New.
> > * ld-mips-elf/mips-elf.exp: Run new test.
>
> Please make the .dd output less susceptible to things like the number
> of sections, size of program headers, etc. One way is to use a linker
> script to place each section at a nice round address. Another is to
> ".*" out the addresses and instruction encodings and just reply on
> the symbolic part of the disassembly. I think the former's better
> here. There are quite a few existing examples.
Fixed by Catherine -- thanks!
> > Stricly speaking some changes, while related, can be split off too (e.g.
> > some MIPS16 or testsuite changes), so I'll look into separating them too
> > -- perhaps that'll make the thing rolling sooner.
>
> Yeah, thanks, that'd be a help if it's easy to do. It doesn't matter
> too much, though. Most of the MIPS16 changes were consistent with the
> microMIPS ones, so were easy to review as a unit.
I was more concerned about grouping functionally independent changes
together. While it may be OK for the review, it often bites later on,
when someone makes excavations on the repository trying to figure out the
original change.
Anyway I did that as you know and it helped with the PIC JAL changes.
> >> From a usability perspective, it's a shame that:
> >>
> >> .set micromips
> >> .ent foo
> >> foo:
> >> b 1f
> >> nop
> >> .end foo
> >> .ent bar
> >> bar:
> >> 1: nop
> >> .end bar
> >>
> >> disassembles as:
> >>
> >> 00000000 <foo>:
> >> 0: cfff b 0 <foo>
> >> 0: R_MICROMIPS_PC10_S1 .L11
> >> 2: 0c00 nop
> >> 4: 0c00 nop
> >>
> >> 00000006 <bar>:
> >> 6: 0c00 nop
> >>
> >> leaving the poor user with no idea what .L11 is.
> >
> > Indeed. This is a general limitation of `objdump' it would seem. This
> > is no different to what you get with:
> >
> > $ cat b.s
> > .globl baz
> > .ent foo
> > foo:
> > b baz
> > nop
> > .end foo
> > .ent bar
> > baz:
> > bar:
> > 1: nop
> > .end bar
> > $ mips-sde-elf-objdump -dr b.o
> >
> > b.o: file format elf32-tradbigmips
> >
> >
> > Disassembly of section .text:
> >
> > 00000000 <foo>:
> > 0: 1000ffff b 0 <foo>
> > 0: R_MIPS_PC16 baz
> > 4: 00000000 nop
> > 8: 00000000 nop
> >
> > 0000000c <bar>:
> > c: 00000000 nop
>
> Well, it's a little different. The user has at least defined two
> named symbols in this case, so they have a good chance of knowing
> what "baz" means. In the microMIPS case we've invented a label
> and are using it in preference to the user-defined one.
That's unfortunate, indeed. [FIXME]
> > I'd just recommend peeking at the symbol table (back to the first
> > program):
> >
> > $ mips-sde-elf-objdump -t b.o
> >
> > b.o: file format elf32-tradbigmips
> >
> > SYMBOL TABLE:
> > 00000000 l d .text 00000000 .text
> > 00000000 l d .data 00000000 .data
> > 00000000 l d .bss 00000000 .bss
> > 00000000 l d .reginfo 00000000 .reginfo
> > 00000000 l d .pdr 00000000 .pdr
> > 00000000 l F .text 00000006 0x80 foo
> > 00000006 l F .text 00000002 0x80 bar
> > 00000006 l .text 00000000 0x80 .L1^B1
>
> I suppose having a symbol with ^B in it is less than ideal too.
> AIUI that name was chosen specifically because it wasn't supposed
> to be written out.
>
> It would be especially confusing if the user or compiler had a ".L11"
> label (without the ^B).
Actually this is a tough problem -- we need to emit a generated symbol
that does not clash with anything the user or GCC may have put in the
source. Any ideas? How is it done in other ports if anywhere?
> >> The following:
> >>
> >> .set micromips
> >> .ent foo
> >> foo:
> >> ld $10,0x1000($11)
> >> .end foo
> >>
> >> generates an assertion failure:
> >>
> >> Assertion failure in micromips_macro_build at gas/config/tc-mips.c line
> 19466.
> >> Please report this bug.
> >>
> >> on mipsisa64-elf with "-mips1 -mabi=32".
> >
> > I can't reproduce it, sorry:
> > [...]
> > Can you debug it and see what the relocation type is that's causing it?
> > I wonder if that might be related to the varargs issue you referring to
> > below and depend on the host architecture, hmm...
>
> Yeah, you're right, sorry. I forgot to mention that in the later reviews.
> This was with a x86_64-linux-gnu host and a botched attempt at working
> around the varags issue. (I probably just added -Wno-error or something,
> I can't remember now.)
>
> I switched to a 32-bit host for parts 2 and 3 of the review, and yeah,
> it doesn't reproduce there.
I have updated this code to pass varargs by a reference. Note that
mips16_macro_build() also attempts to pass them by value which I gather
from some discussions I have tracked down is a grey area of the C language
standard. Sent as separate patch.
> +/* Whether code compression (either of the MIPS16 or the microMIPS ASEs has
>
> s/ASEs has/ASEs) has/
Fixed.
> + if (mips_opts.micromips)
> + return is_micromips_16bit_p (insn->insn_mo)
> + ? 2 : (is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);
>
> This formatting isn't quite right because of the "emacs brackets" rule.
Yeah, now that you mention it, I seem to recall there was something like
this...
> Should be:
>
> return (is_micromips_16bit_p (insn->insn_mo)
> ? 2 : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);
>
> or:
>
> return (is_micromips_16bit_p (insn->insn_mo) ? 2
> : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);
Fixed.
> But I'd be happy with an if-return-if-return-return chain too.
Conditional expressions seem to go with the spirit of this function. ;)
> See below about these insns though...
> -+ if (!mips_opts.mips16 && !mips_opts.micromips)
> ++ if (! HAVE_CODE_COMPRESSION)
>
> The GCC decision a few years back was that no space should be added
> after a unary operator (as with pointer deference, etc). Not important
> enough to do a sed on the whole source base, but we might as well avoid
> changes that go the other way (from no space to space) in GAS.
I didn't know it was standardised at one point. The justification for my
style is the exclamation mark may easily be missed by the reader if next
to a bracket (and she happens not to be eagle-eyed), but I have adjusted
changes accordingly now.
> +static bfd_boolean
> +is_size_valid (const struct mips_opcode *mo)
> +{
> + gas_assert (mips_opts.micromips);
> +
> + if ((micromips_16 || micromips_32) && mo->pinfo == INSN_MACRO)
> + return FALSE;
> + if (micromips_16 && ! is_micromips_16bit_p (mo))
> + return FALSE;
> + if (micromips_32 && ! is_micromips_32bit_p (mo))
> + return FALSE;
> +
> + return TRUE;
> +}
>
> Hmm, seeing this highlighted more makes me wonder whether
> micromips_16 and micromips_32 shouldn't be combined into a
> single variable that represents "the size the user set",
> with 0 meaning "none". As it stands, we'll have checks like:
>
> micromips_16 || micromips_32 || micromips_48
>
> when any future 48-bit support is added. Having separate variables
> also gives the impression that arbitrary combinations are possible.
Good point. I have merged these with mips16_small and mips16_ext and
replaced all of them with forced_insn_length.
> Also, how about replacing is_micromips_XXbit_p with a function
> that just returns the size of a micromips insn? We generally
> deal with byte rather than bit lengths, so both this new function
> and the combined "the size the user set" variable should probably
> both be byte values.
FYI, that's what I did when implementing GDB support (that'll follow
sometime in the future; obviously it depends on some changes to BFD and
opcode made here), so I see no reason why we shouldn't do that here
either, good point. I have merged these into micromips_insn_length() now.
> Seems the code would be a fair bit clearer with those changes,
> but maybe I'm wrong...
No, not a sausage. ;)
> Maybe the mo->match assertions in is_micromips_XXbit_p are better
> done in validate_micromips_insn -- especially if that makes the
> changes above easier -- but I don't mind either way.
Not necessarily any easier for micromips_insn_length(), but it looks like
the right place for these tests, so I moved them over, changing some
negative opcode match tests into positive ones, so as to no value escapes
by accident. Also the advertised argument check was missing from
validate_micromips_insn(), which I have now added, fixing a couple of bugs
throughout it diagnosed.
> char
> mips_nop_opcode (void)
> {
> if (seg_info (now_seg)->tc_segment_info_data.micromips)
> return NOP_OPCODE_MICROMIPS;
>
> return seg_info (now_seg)->tc_segment_info_data.mips16
> ? NOP_OPCODE_MIPS16 : NOP_OPCODE_MIPS;
> }
>
> Same "emacs brackets" thing here. Seems odd to treat microMIPS and MIPS16
> differently like this, so I think if-return-if-return-return makes more
> sense.
Agreed, this function looks better with conditional statements. Fixed.
> The new mips_handle_align looks very good, thanks. I'm afraid it's another
> silly nitpick, but usual style is not to have the brackets in "*(p++)".
I tend to add brackets where operator precedence is not immediately
obvious to the reader. In this case I would have to double-check with the
language standard, so I assumed so would have the reader.
The order of precedence seems natural here for processors with a
post-increment addressing mode, but not everyone has heard of those I
suppose. I removed the brackets anyway.
> 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.
Fixed as a result of earlier changes.
> @@ -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.)
Fixing it up is easy, but I'm not sure what the original intent of these
flags has been. Do they mean:
1. I make use of the FOO feature and I assert it is available?
2. I make use of the FOO feature, but I may go and figure out if it's
available?
3. I make use of the FOO feature and want to prevent linking with any
incompatible ones?
Once we've determined the answer we may modify code accordingly or leave
it as it is. It looks to me the current approach matches #1 and the ELF
program loader may refuse to execute the program if it sees a flag for a
feature that is not supported by the processor to run on. Any .set
fragments within the program are guarded appropriately and hence they do
not need to set the flag (as it would be for #2). Then #3 is sort of
tangential to the two others, but it does matter in this context, hence I
mentioned it (i.e. if #3 was true, then I'd be much happier with #3 & #1
than #3 & #2; otherwise I don't have a strong preference between #1 and
#2).
Current arrangement is similar to that of file_mips_isa even though the
ISA can be overridden by .set too and I think it makes sense to keep all
these flags consistent.
Any thoughts, anyone?
> Lots of cases where a space is missing before "(".
I fix these kinds of errors as I notice them -- some must have obviously
escaped me, sorry. I find it easier to write properly formatted code from
the beginning than to fix it afterwards, but there you go.
Overall I think it would be a good idea to run `indent' over upstream
files we are concerned about at one point (there are plenty of small
problems throughout that sneaked in unnoticed and accumulated over the
time) and then fixing up most of such problems would amount to rerunning
`indent' with the patch in question applied -- without the problem of
reformatting half of the preexisting source code as a side effect.
At least `quilt' warns about trailing white space added (and has an
option to strip it automatically) -- I have cleaned up several places this
way.
> Please be consistent about "str[n]cmp (...) == 0" vs '!str[n]cmp (...)':
> use one or the other. (IMO the first is clearer.)
FWIW I agree first is clearer. I have fixed the couple of others -- they
looked like copied & pasted from existing code though.
> 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.
Next time, I'm afraid. [FIXME]
> + 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.
Another copy & paste case -- fixed (and removed support for [] as
irrelevant).
> + 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.
Fixed. There's some suspicious code around 'V' here and in mips_ip()
BTW -- it doesn't seem right to me to do args++ here even if it seems to
work. [FIXME]
> + case 4:
> + case 5:
> + case 6:
>
> Too many spaces.
Fixed.
> + if (insn + 1 < µmips_opcodes[bfd_micromips_num_opcodes] &&
> + !strcmp (insn->name, insn[1].name))
>
> Misplaced "&&".
Another copy & paste case -- fixed.
> + = {BFD_RELOC_UNUSED, BFD_RELOC_UNUSED, BFD_RELOC_UNUSED};;
>
> Double ";".
Fixed.
> + 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.
Fixed, as noted already.
> + /* For microMIPS, we always use relocations for branches.
> + So, we should not resolve immediate values. */
>
> Too many spaces.
Fixed.
> + 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;
Fixed.
> 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.
Can we make it a follow-up patch sometime in the future? I'm not afraid
of factoring code out of anywhere (I'm speaking of myself only of course),
but at this point the effort is about the same as writing this whole stuff
from scratch. :( [FIXME]
> #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.
Dumped. Whoever will need it can add it back.
> + "mA" 7-bit immediate (-63 .. 64) << 2 (MICROMIPSOP_*_IMMA)
>
> Don't you mean (-64 .. 63)?
Yes, this is for LWGP.
> + "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?
No, fixed (this is for ADDIUR2, so wasting a precious encoding of an
immediate would be a bad idea).
> @@ -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.
Indeed -- that's to avoid running stripped executables that breaks
semihosting. Sorry about that. I've now converted to `quilt'
temporarily, so such stuff will be dropped automatically.
> 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.
I have rewritten both the relevant macro definitions and the piece of
code above. I have cleaned up some of the other STO_* macros too for
consistency and clarity.
> + /* 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).
I don't think it's needed -- I have removed it. Nobody seems to use
%half() anyway and beside defining it in an odd way the ABI is unclear as
to what this is meant for.
> +/*
> + * 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.
I've fixed the off-the-section accesses, but this function and
surrounding code was broken in design in the first place. With a
microMIPS instruction stream you can't just peek at a randomly picked
halfword, interpret it as an opcode apply make arbitrary transformations,
because the halfword may in fact be a lower half of 32-bit instruction.
Therefore I changed the function only to replace branch or jump encodings
that change the size of the delay slot if a relocation has been found at
that location. If no relocation has been found, then, conservatively,
only the LUI itself may be be changed and a heuristics is used to
determine if it might be in a delay slot of a fixed size. If no branch or
jump encoding has been found, then the LUI may be deleted altogether,
otherwise a NOP is substituted, that is 16-bit if the heuristics
determined the possible delay slot will accept such one or 32-bit (i.e.
the same width as the original LUI) otherwise.
Note that in particular means a JALR->JALRS conversion won't happen
unless there's a R_MIPS_JALR relocation against the call instruction; I
made no attempt to change GAS in any way to emit such a relocation in
cases it does not do already. [FIXME]
> + /* 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.
Fixed.
> + /* 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?
Globally-binding, that is. I see no reason -- Chao-ying?
I have changed it now anyway.
> + 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.
Agreed -- all relaxations operate on 32-bit instructions (some also imply
access to the following instruction that wasn't guarded against the
section end and which I have fixed it now too) -- there's no point in
doing anything with half-an-instruction.
> + /* 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).
I have reworded the comment now.
> + /* Assume is possible to delete the LUI instruction:
> + 4 bytes at irel->r_offset. */
>
> s/Assume is/Assume it is/
Fixed.
> + /* 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.)
Ah, you noticed that too -- fixed, as noted above.
> + /* 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.
Fixed, as noted above.
I have fixed the rewrite of immediate addends throughout too as that
matters for REL targets. And fixed zillions of GNU Coding Standar
violations.
Overall I'm a bit concerned about all this linker relaxation stuff -- it
breaks -falign-jumps, -falign-labels and -falign-loops which may have a
severe performance penalty and should therefore be enabled conditionally
only, preferably where all the three options are set to 1 (meaning that
would be naturally implied by -Os). A linker option would be required an
set appropriately by the GCC driver. [FIXME]
> the elf32-mips.c reloc questions in review 2 apply to elf64-mips.c
> and elfn32-mips.c as well. Furthermore:
>
> +static reloc_howto_type micromips_elf64_howto_table_rela[] =
> +{
> + /* 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 */
>
> RELA relocations shouldn't be partial_inplace. Applies to the whole array.
Could be my omission -- fixed, sorry. There were preexisting cases too,
sent as a separate patch.
> Did you actually test this with n64, say with a gcc bootstrap? Same
> comment goes for elfn32-mips.c.
>
> Why only do the linker relaxation for elf32-mips.c (o32, o64 & EABI)?
> Why not for n32 and n64 too?
The answer to all the questions is negative. There is no 64-bit
microMIPS hardware available at the moment. The best bet might be QEMU --
NathanF, have you implemented any of the 64-bit instructions in QEMU?
Otherwise there's probably no way do do any of 64-bit microMIPS testing
beside what's done in binutils. And no library work of any kind has
started for 64-bit microMIPS support AFAIK. Hence there has been little
incentive to attempt anything but rudimentary support for 64-bit ABIs.
I envisage all the relaxation stuff to be either moved over to
elfxx-mips.c or copied to elfn32-mips.c and elf64-mips.c, as applicable,
once we have real 64-bit support.
> +#define LA25_LUI_MICROMIPS_1(VAL) (0x41b9) /* lui t9,VAL */
> +#define LA25_LUI_MICROMIPS_2(VAL) (VAL)
> +#define LA25_J_MICROMIPS_1(VAL) (0xd400 | (((VAL) >> 17) & 0x3ff)) /* j VAL */
> +#define LA25_J_MICROMIPS_2(VAL) (0xd4000000 | (((VAL) >> 1) & 0xffff))
> +#define LA25_ADDIU_MICROMIPS_1(VAL) (0x3339) /* addiu t9,t9,VAL */
> +#define LA25_ADDIU_MICROMIPS_2(VAL) (VAL)
>
> LA25_J_MICROMIPS_2 is a 16-bit opcode, so the "0xd4000000 | ..."
> thing is bogus.
Fixed.
> That said, why split these up? bfd_{get,put}_32
> don't require aligned addresses.
I think it's easier and more readable this way as with bfd_{get,put}_32()
you'd have to halfword-swap the bit patterns produced based on the target
endianness.
> + value = s->size;
> + if (ELF_ST_IS_MICROMIPS (stub->h->root.other))
> + value |= 1;
> +
> /* Create a symbol for the stub. */
> - mips_elf_create_stub_symbol (info, stub->h, ".pic.", s, s->size, 8);
> + mips_elf_create_stub_symbol (info, stub->h, ".pic.", s, value, 8);
>
> Do this in mips_elf_create_stub_symbol rather than in each caller.
Fixed.
> + return r_type == R_MIPS_GOT16 || r_type == R_MIPS16_GOT16
> + || r_type == R_MICROMIPS_GOT16;
>
> GNU indentation requires brackets here. Also, once it becomes too
> long for one line, let's keep one item per line:
>
> return (r_type == R_MIPS_GOT16
> || r_type == R_MIPS16_GOT16
> || r_type == R_MICROMIPS_GOT16);
>
> Same for later functions.
Fixed.
> - if (r_type == R_MIPS_TLS_GOTTPREL)
> + if (r_type == R_MIPS_TLS_GOTTPREL || r_type == R_MICROMIPS_TLS_GOTTPREL)
>
> Hide these differences in analogues of the got16_reloc_p functions.
> Same for all other relocs with MICROMIPS variants.
Done. Found and fixed some MIPS16 bugs on the way.
> @@ -3187,8 +3244,12 @@ mips_elf_got_page (bfd *abfd, bfd *ibfd,
> struct mips_got_entry *entry;
>
> page = (value + 0x8000) & ~(bfd_vma) 0xffff;
> - entry = mips_elf_create_local_got_entry (abfd, info, ibfd, page, 0,
> - NULL, R_MIPS_GOT_PAGE);
> + if (elf_elfheader (abfd)->e_flags & EF_MIPS_ARCH_ASE_MICROMIPS)
> + entry = mips_elf_create_local_got_entry (abfd, info, ibfd, page, 0,
> + NULL, R_MICROMIPS_GOT_PAGE);
> + else
> + entry = mips_elf_create_local_got_entry (abfd, info, ibfd, page, 0,
> + NULL, R_MIPS_GOT_PAGE);
>
> if (!entry)
> return MINUS_ONE;
>
> Why is this necessary?
This looks bogus to me -- most mips_elf_create_local_got_entry() cares
about is whether r_type is a TLS reloc or not. It does some further
checks for TLS relocs, but it doesn't bother otherwise. Removed.
> @@ -5127,12 +5200,26 @@ mips_elf_calculate_relocation (bfd *abfd
> + h->la25_stub->stub_section->output_offset
> + h->la25_stub->offset);
>
> + /* Make sure MIPS16 and microMIPS are not used together. */
> + if ((r_type == R_MIPS16_26 && target_is_micromips_code_p)
> + || (r_type == R_MICROMIPS_26_S1 && target_is_16_bit_code_p))
> + {
> + (*_bfd_error_handler)
> + (_("MIPS16 and microMIPS functions cannot call each other"));
> + return bfd_reloc_notsupported;
> + }
>
> Should this be extended to check for branches too?
That would be a useful improvement I suppose, but MIPS16 code doesn't
care either (you can't use a branch to switch the ISA mode). Overall I
think it's a corner case -- branches to external symbols are a rarity.
[FIXME]
> + case R_MIPS_26:
> + /* Make sure the target of JALX is word-aligned.
> + Bit 0 must be 1 (MIPS16/microMIPS mode), and bit 1 must be 0. */
> + if (*cross_mode_jump_p == TRUE && (symbol & 3) != 1)
> + return bfd_reloc_outofrange;
>
> == TRUE has been banned by act of parliament.
Fixed.
> If we're checking alignment for R_MIPS_26 and R_MICROMIPS_26, we should
> check it for R_MIPS16_26 too. Something like:
>
> /* Make sure the target of JALX is word-aligned.
> Bit 0 must be the correct ISA mode selector and bit 1 must be 0. */
> if (*cross_mode_jump_p && (symbol & 3) != (r_type == R_MIPS_26))
> return bfd_reloc_outofrange;
>
> for both cases would be fine.
>
> + case R_MICROMIPS_26_S1:
> + /* Make sure the target of jalx is word-aligned. */
> + if (*cross_mode_jump_p == TRUE && (symbol & 3) != 0)
> + return bfd_reloc_outofrange;
> + if (local_p)
> + {
> + /* For jalx, the offset is shifted right by two bits. */
> + if (*cross_mode_jump_p == TRUE)
> + value = ((addend | ((p + 4) & 0xf0000000)) + symbol) >> 2;
> + else
> + value = ((addend | ((p + 4) & 0xf8000000)) + symbol) >> 1;
> + }
> + else
> + {
> + /* For jalx, the offset is shifted right by two bits. */
> + if (*cross_mode_jump_p == TRUE)
> + {
> + value = (_bfd_mips_elf_sign_extend (addend, 28) + symbol) >> 2;
> + if (h->root.root.type != bfd_link_hash_undefweak)
> + overflowed_p = (value >> 26) != ((p + 4) >> 28);
> + }
> + else
> + {
> + value = (_bfd_mips_elf_sign_extend (addend, 27) + symbol) >> 1;
> + if (h->root.root.type != bfd_link_hash_undefweak)
> + overflowed_p = (value >> 26) != ((p + 4) >> 27);
> + }
> + }
> + value &= howto->dst_mask;
> + break;
>
> This is a strict extension of the R_MIPS_26 and R_MIPS16_26 behaviour,
> so I'd prefer to see them integrated.
I think R_MIPS_26 should be used for microMIPS JALX. I don't like the
idea of overloading R_MICROMIPS_26_S1 this way, especially given another
relocation already fits. What do you think? It is too late to fix the
ABI probably though; I'm thinking what the consequences might be...
I have merged this piece now and integrated your comment update.
> @@ -5372,6 +5516,9 @@ mips_elf_calculate_relocation (bfd *abfd
> both reloc addends by 4. */
> if (r_type == R_MIPS16_HI16)
> value = mips_elf_high (addend + gp - p - 4);
> + else if (r_type == R_MICROMIPS_HI16)
> + /* The low bit of $t9 is set for microMIPS calls. */
> + value = mips_elf_high (addend + gp - p - 1);
> else
> value = mips_elf_high (addend + gp - p);
> overflowed_p = mips_elf_overflow_p (value, 16);
>
> This statement is also true for MIPS16, so in context, the comment
> is a bit confusing on its own. How about:
>
> /* The microMIPS .cpload sequence uses the same assembly
> instructions as the traditional psABI version, but the
> incoming $t9 has the low bit set. */
Fine by me.
> @@ -5486,8 +5647,38 @@ mips_elf_calculate_relocation (bfd *abfd
> value &= howto->dst_mask;
> break;
>
> + case R_MICROMIPS_PC7_S1:
> + value = symbol + _bfd_mips_elf_sign_extend (addend, 8) - p;
> + overflowed_p = mips_elf_overflow_p (value, 8);
> + value >>= howto->rightshift;
> + value &= howto->dst_mask;
> + break;
> +
> + case R_MICROMIPS_PC10_S1:
> + value = symbol + _bfd_mips_elf_sign_extend (addend, 11) - p;
> + overflowed_p = mips_elf_overflow_p (value, 11);
> + value >>= howto->rightshift;
> + value &= howto->dst_mask;
> + break;
> +
> + case R_MICROMIPS_PC16_S1:
> + value = symbol + _bfd_mips_elf_sign_extend (addend, 17) - p;
> + overflowed_p = mips_elf_overflow_p (value, 17);
> + value >>= howto->rightshift;
> + value &= howto->dst_mask;
> + break;
> +
> + case R_MICROMIPS_PC23_S2:
> + value = symbol + _bfd_mips_elf_sign_extend (addend, 25) - (p &
> 0xfffffffc);
> + overflowed_p = mips_elf_overflow_p (value, 25);
> + value >>= howto->rightshift;
> + value &= howto->dst_mask;
> + break;
>
> I realise you probably based this on R_MIPS_PC16 and R_MIPS_GNU_REL16_S2,
> but even so, you're careful to check for underflow elsewhere but ignore
> it here.
I'm not sure what you mean. R_MICROMIPS_PC23_S2 is used for ADDIUPC so
you have to strip out the two LSBs.
> Use of 0xfffffffc isn't portable for 64-bit addresses;
> use "-4" or "~(bfd_vma) 3" instead.
I'm nasty enough to use (foo | 3) ^ 3 and avoid type width and promotion
dependencies altogether in such cases and hope for GCC to be smart and do
the right thing and combine the two ops.
> @@ -6150,13 +6355,18 @@ _bfd_mips_elf_symbol_processing (bfd *ab
> break;
> }
>
> - /* If this is an odd-valued function symbol, assume it's a MIPS16 one. */
> + /* If this is an odd-valued function symbol, assume it's a MIPS16
> + or microMIPS one. */
> if (ELF_ST_TYPE (elfsym->internal_elf_sym.st_info) == STT_FUNC
> && (asym->value & 1) != 0)
> {
> asym->value--;
> - elfsym->internal_elf_sym.st_other
> - = ELF_ST_SET_MIPS16 (elfsym->internal_elf_sym.st_other);
> + if (elf_elfheader (abfd)->e_flags & EF_MIPS_ARCH_ASE_MICROMIPS)
> + elfsym->internal_elf_sym.st_other
> + = ELF_ST_SET_MICROMIPS (elfsym->internal_elf_sym.st_other);
> + else
> + elfsym->internal_elf_sym.st_other
> + = ELF_ST_SET_MIPS16 (elfsym->internal_elf_sym.st_other);
> }
> }
>
>
> So a file can't mix MIPS16 and microMIPS code? We should probably
> detect that explicitly. I'd like a clear statement of what the
> interoperability restrictions are.
>
> This goes back to the question of when EF_MIPS_ARCH_ASE_MICROMIPS
> should be set (see previous reviews).
Background information first -- you can't have a piece of MIPS hardware,
either real or simulated, with the MIPS16 ASE and the microMIPS ASE
implemented both at a time. This is a design assumption that allowed the
ISA bit to be overloaded.
That obviously does not mean -- in principle -- that you can't have an
executable (or one plus a mixture of shared libraries) where some
functions are MIPS16 code and some are microMIPS code, either of which
only called once the presence of the respective ASE has been determined.
While a valid configuration this is also a rare exotic corner case -- I
could imagine a piece of generic, processor-independent console/bootstrap
firmware like this for example, but little beyond that.
We had a discussion about it and the conclusion was from the user's
perspective the most beneficial configuration is one where mixing MIPS16
and microMIPS code within a single executable is forbidden by default.
The reason is a build-time error is always better than a run-time one
(especially when the device has been placed out there in the field
already; hopefully not an Ariane 5 rocket) and the case where mixing
MIPS16 and microMIPS code would almost always happen is when the user
picked the wrong library by mistake. It is therefore most productive to
fail at this point rather than later.
A future enhancement could add assembler and linker switches to override
this default and mix the two ASEs in a single executable. [FIXME]
> @@ -6865,7 +7075,8 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd
> /* If this is a mips16 text symbol, add 1 to the value to make it
> odd. This will cause something like .word SYM to come up with
> the right value when it is loaded into the PC. */
> - if (ELF_ST_IS_MIPS16 (sym->st_other))
> + if (ELF_ST_IS_MIPS16 (sym->st_other)
> + || ELF_ST_IS_MICROMIPS (sym->st_other))
> ++*valp;
>
> return TRUE;
>
> As with GAS, I'd like an ODD_SYMBOL_P or some-such.
ELF_ST_IS_COMPRESSED it is then, for consistency. Note it doesn't seem
possible to me to limit the macro to read its argument once only without
resorting to GNU extensions; I'll be happy to be proved wrong though.
> @@ -7166,6 +7378,8 @@ mips_elf_add_lo16_rel_addend (bfd *abfd,
> r_type = ELF_R_TYPE (abfd, rel->r_info);
> if (mips16_reloc_p (r_type))
> lo16_type = R_MIPS16_LO16;
> + else if (micromips_reloc_shuffle_p (r_type))
> + lo16_type = R_MICROMIPS_LO16;
> else
> lo16_type = R_MIPS_LO16;
>
> Conceptually, this ought to be plain micromips_reloc_p. Whether we need
> to shuffle or not isn't an issue here, even though I realise the shuffle
> condition produces the right result.
Of course.
> @@ -9215,6 +9479,12 @@ _bfd_mips_elf_relocate_section (bfd *out
> case bfd_reloc_ok:
> break;
>
> + case bfd_reloc_outofrange:
> + msg = _("internal error: jalx jumps to not word-aligned address");
> + info->callbacks->warning
> + (info, msg, name, input_bfd, input_section, rel->r_offset);
> + return FALSE;
> +
> default:
> abort ();
> break;
>
> Why's that an internal error? Surely it could be triggered by
> badly-formed input, rather than just as a result of internal
> confusion?
>
> The error is more specific than the error code, so you should also add:
>
> BFD_ASSERT (jal_reloc_p (howto->type));
I've made it a conditional instead. Preexisting cases of
bfd_reloc_outofrange will fall through to abort() below as they do now.
> @@ -976,7 +976,7 @@ bfd_install_relocation (bfd *abfd,
> asection *reloc_target_output_section;
> asymbol *symbol;
> bfd_byte *data;
> -
> +
> symbol = *(reloc_entry->sym_ptr_ptr);
> if (bfd_is_abs_section (symbol->section))
> {
>
> Bogus change.
Fixed.
> @@ -1652,6 +1652,8 @@ ENUMX
> ENUMX
> BFD_RELOC_16
> ENUMX
> + BFD_RELOC_MICROMIPS_16
> +ENUMX
> BFD_RELOC_14
> ENUMX
> BFD_RELOC_8
>
> Here and elsewhere, keep the MIPS-specific stuff separate from the
> generic relocs. See the MIPS16 relocs for examples.
I agree in principle, but no longer relevant as I have discarded the
reloc.
> I'll take your word for it that micromips-opc.c is correct. ;-)
There were a couple of problems with it, but I did my best to get it
right. :)
> + else if ((insn & 0x1c00) != 0x0400
> + && (insn & 0x1c00) != 0x0800
> + && (insn & 0x1c00) != 0x0c00)
> + {
> + /* This is a 32-bit microMIPS instruction. */
> + higher = insn;
> +
> + status = (*info->read_memory_func) (memaddr + 2, buffer, 2, info);
> + if (status != 0)
> + {
> + (*info->fprintf_func) (info->stream, "micromips 0x%x",
> + (unsigned int) higher);
> + (*info->memory_error_func) (status, memaddr + 2, info);
> + return -1;
> + }
>
> Why print "micromips " here but not in the 48-byte case?
It looks to me it's been modelled after the MIPS16 version where "extend
0x..." is printed in the case of a read error. I'm not sure if either
makes sense (.byte foo or suchlike that some other ports output in such a
case looks more reasonable to me), but at least it is consistent.
> Also,
> s/0x%02x/0x%x/ would be better IMO.
The highest byte is always 0x7c, so no need to care about leading zeroes.
I have rewritten pieces of this code so that all output available is
always produced in a single place.
> Watch your formatting in this function. There are lots of cases of
> things like:
>
> (*info->fprintf_func) (info->stream, "%s",
> mips_gpr_names[lastregno]);
>
> which isn't right. Arguments must be aligned to the column after
> the "(". Don't be afraid to split lines into several statements
> if it makes things look more natural.
Agreed, this was a horrible piece of unreadable code. I created some
auxiliary variables and a macro to compress the source (and, in the former
case, to get potentially better code), improving readablity and getting
rid of most indentation problems as a side effect.
The rule of thumb is if you have a problem with source code of your
program hitting the right margin on an 80-column terminal, then you're
probably putting too much in a single function. This rule isn't kept of
course in many places throughout binutils, but that does not mean it's not
a valid one.
> The gas testsuite changes look very nice, thanks.
You are welcome. :)
Thank you for the review -- I know it's been a lot effort. Here's a new
version of the change. Regression-tested succesfully on mips-sde-elf and
mips-linux-gnu targets. Changes for the MCU ASE and the M14K/c cores will
follow.
Maciej
bfd/
2010-07-26 Chao-ying Fu <fu@mips.com>
Ilie Garbacea <ilie@mips.com>
Maciej W. Rozycki <macro@codesourcery.com>
Joseph Myers <joseph@codesourcery.com>
Catherine Moore <clm@codesourcery.com>
* archures.c (bfd_mach_mips_micromips): New macro.
* cpu-mips.c (I_micromips): New enum value.
(arch_info_struct): Add bfd_mach_mips_micromips.
* elfxx-mips.h: (_bfd_mips16_elf_reloc_unshuffle): Rename to...
(_bfd_mips_elf_reloc_unshuffle): ... this. Handle microMIPS.
(_bfd_mips16_elf_reloc_shuffle): Rename to...
(_bfd_mips_elf_reloc_shuffle): ... this. Handle microMIPS.
(gprel16_reloc_p): Handle microMIPS.
(literal_reloc_p): New function.
* elf32-mips.c (elf_micromips_howto_table_rel): New variable.
(_bfd_mips_elf32_gprel16_reloc): Handle microMIPS.
(mips16_gprel_reloc): Update for _bfd_mips_elf_reloc_unshuffle
and _bfd_mips_elf_reloc_shuffle changes.
(mips_elf_gprel32_reloc): Update comment.
(micromips_reloc_map): New variable.
(bfd_elf32_bfd_reloc_type_lookup): Handle microMIPS.
(mips_elf32_rtype_to_howto): Likewise.
(mips_info_to_howto_rel): Likewise.
(elf32_mips_relax_delete_bytes): New function.
(opcode_descriptor): New structure.
(b_insns_32, b_insn_16): New variables.
(BZ32_REG, BZ32_REG_FIELD): New macros.
(bz_insns_32, bzc_insns_32, bz_insns_16): New variables.
(BZ16_VALID_REG, BZ16_REG_FIELD): New macros.
(jal_insn_32_bd16, jal_insn_32_bd32): New variables.
(call_insn_32_bd16, call_insn_32_bd32): Likewise.
(ds_insns_32_bd16, jalx_insn_32_bd32): Likewise.
(jalr_insn_16_bd16, jalr_insn_16_bd32): Likewise.
(ds_insns_16_bd16): Likewise.
(lui_insn, addiu_insn, addiupc_insn): Likewise.
(ADDIU_REG, ADDIUPC_VALID_REG, ADDIUPC_REG_FIELD): New macros.
(lwgp_insn_32, lwgp_insn_16): New functions.
(LWGP32_REG, LWGP16_VALID_REG, LWGP16_REG_FIELD): New macros.
(MOVE32_RD, MOVE32_RS): Likewise.
(MOVE16_RD_FIELD, MOVE16_RS_FIELD): Likewise.
(move_insns_32, move_insns_16): New variables.
(nop_insn_32, nop_insn_16): Likewise.
(MATCH): New macro.
(find_match): New function.
(relax_dslot_norel16, relax_dslot_norel32): Likewise.
(relax_dslot_rel): Likewise.
(IS_BITSIZE): New macro.
(elf32_mips_relax_section): New function.
(bfd_elf32_bfd_relax_section): Define.
* elf64-mips.c (micromips_elf64_howto_table_rel): New variable.
(micromips_elf64_howto_table_rela): Likewise.
(mips16_gprel_reloc): Update for _bfd_mips_elf_reloc_unshuffle
and _bfd_mips_elf_reloc_shuffle changes.
(micromips_reloc_map): Likewise.
(bfd_elf64_bfd_reloc_type_lookup): Handle microMIPS.
(bfd_elf64_bfd_reloc_name_lookup): Likewise.
(mips_elf64_rtype_to_howto): Likewise.
* elfn32-mips.c (elf_micromips_howto_table_rel): New variable.
(elf_micromips_howto_table_rela): Likewise.
(mips16_gprel_reloc): Update for _bfd_mips_elf_reloc_unshuffle
and _bfd_mips_elf_reloc_shuffle changes.
(micromips_reloc_map): Likewise.
(bfd_elf32_bfd_reloc_type_lookup): Handle microMIPS.
(bfd_elf32_bfd_reloc_name_lookup): Likewise.
(mips_elf_n32_rtype_to_howto): Likewise.
* elfxx-mips.c (LA25_LUI_MICROMIPS_1): New macro.
(LA25_LUI_MICROMIPS_2): Likewise.
(LA25_J_MICROMIPS_1, LA25_J_MICROMIPS_2): Likewise.
(LA25_ADDIU_MICROMIPS_1, LA25_ADDIU_MICROMIPS_2): Likewise.
(TLS_RELOC_P): Handle microMIPS.
(mips_elf_create_stub_symbol): Adjust value of stub symbol if
target is a microMIPS function.
(micromips_reloc_p): New function.
(micromips_reloc_shuffle_p): Likewise.
(got16_reloc_p, call16_reloc_p): Handle microMIPS.
(got_disp_reloc_p, got_page_reloc_p): New functions.
(got_ofst_reloc_p): Likewise.
(got_hi16_reloc_p, got_lo16_reloc_p): Likewise.
(call_hi16_reloc_p, call_lo16_reloc_p): Likewise.
(hi16_reloc_p, lo16_reloc_p, jal_reloc_p): Handle microMIPS.
(tls_gd_reloc_p, tls_ldm_reloc_p): New functions.
(tls_gottprel_reloc_p): Likewise.
(_bfd_mips16_elf_reloc_unshuffle): Rename to...
(_bfd_mips_elf_reloc_unshuffle): ... this. Handle microMIPS.
(_bfd_mips16_elf_reloc_shuffle): Rename to...
(_bfd_mips_elf_reloc_shuffle): ... this. Handle microMIPS.
(_bfd_mips_elf_lo16_reloc): Handle microMIPS.
(mips_tls_got_index, mips_elf_got_page): Likewise.
(mips_elf_create_local_got_entry): Likewise.
(mips_elf_relocation_needs_la25_stub): Likewise.
(mips_elf_calculate_relocation): Likewise.
(mips_elf_perform_relocation): Likewise.
(_bfd_mips_elf_symbol_processing): Likewise.
(_bfd_mips_elf_add_symbol_hook): Likewise.
(_bfd_mips_elf_link_output_symbol_hook): Likewise.
(mips_elf_add_lo16_rel_addend): Likewise.
(_bfd_mips_elf_check_relocs): Likewise.
(mips_elf_adjust_addend): Likewise.
(_bfd_mips_elf_relocate_section): Likewise.
(mips_elf_create_la25_stub): Likewise.
(_bfd_mips_vxworks_finish_dynamic_symbol): Likewise.
(_bfd_mips_elf_gc_sweep_hook): Likewise.
(_bfd_mips_elf_print_private_bfd_data): Likewise.
* reloc.c (BFD_RELOC_MICROMIPS_7_PCREL_S1): New relocation.
(BFD_RELOC_MICROMIPS_10_PCREL_S1): Likewise.
(BFD_RELOC_MICROMIPS_16_PCREL_S1): Likewise.
(BFD_RELOC_MICROMIPS_GPREL16): Likewise.
(BFD_RELOC_MICROMIPS_JMP, BFD_RELOC_MICROMIPS_HI16): Likewise.
(BFD_RELOC_MICROMIPS_HI16_S): Likewise.
(BFD_RELOC_MICROMIPS_LO16): Likewise.
(BFD_RELOC_MICROMIPS_LITERAL): Likewise.
(BFD_RELOC_MICROMIPS_GOT16): Likewise.
(BFD_RELOC_MICROMIPS_CALL16): Likewise.
(BFD_RELOC_MICROMIPS_GOT_HI16): Likewise.
(BFD_RELOC_MICROMIPS_GOT_LO16): Likewise.
(BFD_RELOC_MICROMIPS_CALL_HI16): Likewise.
(BFD_RELOC_MICROMIPS_CALL_LO16): Likewise.
(BFD_RELOC_MICROMIPS_SUB): Likewise.
(BFD_RELOC_MICROMIPS_GOT_PAGE): Likewise.
(BFD_RELOC_MICROMIPS_GOT_OFST): Likewise.
(BFD_RELOC_MICROMIPS_GOT_DISP): Likewise.
(BFD_RELOC_MICROMIPS_HIGHEST): Likewise.
(BFD_RELOC_MICROMIPS_HIGHER): Likewise.
(BFD_RELOC_MICROMIPS_SCN_DISP): Likewise.
(BFD_RELOC_MICROMIPS_JALR): Likewise.
(BFD_RELOC_MICROMIPS_TLS_GD): Likewise.
(BFD_RELOC_MICROMIPS_TLS_LDM): Likewise.
(BFD_RELOC_MICROMIPS_TLS_DTPREL_HI16): Likewise.
(BFD_RELOC_MICROMIPS_TLS_DTPREL_LO16): Likewise.
(BFD_RELOC_MICROMIPS_TLS_GOTTPREL): Likewise.
(BFD_RELOC_MICROMIPS_TLS_TPREL_HI16): Likewise.
(BFD_RELOC_MICROMIPS_TLS_TPREL_LO16): Likewise.
* bfd-in2.h: Regenerate.
* libbfd.h: Regenerate.
binutils/
2010-07-26 Chao-ying Fu <fu@mips.com>
* readelf.c (get_machine_flags): Handle microMIPS.
(get_mips_symbol_other): Likewise.
gas/
2010-07-26 Chao-ying Fu <fu@mips.com>
Maciej W. Rozycki <macro@codesourcery.com>
* config/tc-mips.h (mips_segment_info): Add one bit for
microMIPS.
(TC_LABEL_IS_LOCAL): New macro.
(mips_label_is_local): New prototype.
* config/tc-mips.c (emit_branch_likely_macro): New variable.
(mips_set_options): Add micromips.
(mips_opts): Initialise micromips to -1.
(file_ase_micromips): New variable.
(CPU_HAS_MICROMIPS): New macro.
(HAVE_CODE_COMPRESSION): Likewise.
(micromips_op_hash): New variable.
(micromips_nop16_insn, micromips_nop32_insn): New variables.
(NOP_INSN): Handle microMIPS.
(mips32_to_micromips_reg_b_map): New macro.
(mips32_to_micromips_reg_c_map): Likewise.
(mips32_to_micromips_reg_d_map): Likewise.
(mips32_to_micromips_reg_e_map): Likewise.
(mips32_to_micromips_reg_f_map): Likewise.
(mips32_to_micromips_reg_g_map): Likewise.
(mips32_to_micromips_reg_l_map): Likewise.
(mips32_to_micromips_reg_n_map): Likewise.
(mips32_to_micromips_reg_h_map): New variable.
(mips32_to_micromips_reg_m_map): Likewise.
(mips32_to_micromips_reg_q_map): Likewise.
(micromips_to_32_reg_h_map): New variable.
(micromips_to_32_reg_i_map): Likewise.
(micromips_to_32_reg_m_map): Likewise.
(micromips_to_32_reg_q_map): Likewise.
(micromips_to_32_reg_b_map): New macro.
(micromips_to_32_reg_c_map): Likewise.
(micromips_to_32_reg_d_map): Likewise.
(micromips_to_32_reg_e_map): Likewise.
(micromips_to_32_reg_f_map): Likewise.
(micromips_to_32_reg_g_map): Likewise.
(micromips_to_32_reg_l_map): Likewise.
(micromips_to_32_reg_n_map): Likewise.
(micromips_imm_b_map, micromips_imm_c_map): New macros.
(RELAX_DELAY_SLOT_16BIT): New macro.
(RELAX_DELAY_SLOT_SIZE_FIRST): Likewise.
(RELAX_DELAY_SLOT_SIZE_SECOND): Likewise.
(RELAX_MICROMIPS_ENCODE, RELAX_MICROMIPS_P): New macros.
(RELAX_MICROMIPS_TYPE, RELAX_MICROMIPS_USER_16BIT): Likewise.
(RELAX_MICROMIPS_UNCOND, RELAX_MICROMIPS_LINK): Likewise.
(RELAX_MICROMIPS_TOOFAR, RELAX_MICROMIPS_MARK_TOOFAR): Likewise.
(RELAX_MICROMIPS_CLEAR_TOOFAR): Likewise.
(RELAX_MICROMIPS_EXTENDED): Likewise.
(RELAX_MICROMIPS_MARK_EXTENDED): Likewise.
(RELAX_MICROMIPS_CLEAR_EXTENDED): Likewise.
(MICROMIPS_INSERT_OPERAND, MICROMIPS_EXTRACT_OPERAND): New
macros.
(mips_macro_warning): Add delay_slot_16bit_p, delay_slot_32bit_p,
fsize and insns.
(mips16_small, mips16_ext): Remove variables, replacing with...
(forced_insn_size): ... this.
(append_insn, mips16_ip): Update accordingly.
(micromips_insn_length): New function.
(insn_length): Return the length of microMIPS instructions.
(mips_record_mips16_mode): Rename to...
(mips_record_mips16_micromips_mode): ... this. Handle
microMIPS.
(install_insn): Handle microMIPS.
(is_size_valid, is_delay_slot_valid): New functions.
(md_begin): Handle microMIPS.
(md_assemble): Likewise. Update for append_insn interface
change.
(micromips_reloc_p): New function.
(got16_reloc_p): Handle microMIPS.
(hi16_reloc_p): Likewise.
(lo16_reloc_p): Likewise.
(matching_lo_reloc): Likewise.
(mips_move_labels): Likewise.
(mips_mark_labels): New function.
(mips16_mark_labels): Rename to...
(mips_compressed_mark_labels): ... this. Handle microMIPS.
(insns_between): Handle microMIPS.
(MICROMIPS_LABEL_CHAR): New macro.
(micromips_target_label, micromips_target_name): New variables.
(micromips_label_name, micromips_label_expr): New functions.
(micromips_label_inc, micromips_add_label): Likewise.
(mips_label_is_local): Likewise.
(append_insn): Add expansionp argument. Handle microMIPS.
(start_noreorder, end_noreorder): Handle microMIPS.
(macro_start, macro_warning, macro_end): Likewise.
(lui_fmt, shft_fmt): New variables.
(LUI_FMT, SHFT_FMT): New macros.
(macro_map_reloc): New function.
(macro_build): Handle microMIPS. Update for append_insn
interface change.
(mips16_macro_build): Update for append_insn interface change.
(macro_build_jalr): Handle microMIPS.
(macro_build_lui): Likewise. Update for append_insn interface
change.
(macro_build_ldst_constoffset): Use relocation wrappers.
(set_at): Likewise.
(load_register): Likewise.
(load_address): Likewise.
(move_register): Handle microMIPS.
(load_got_offset): Use relocation wrappers.
(add_got_offset): Likewise.
(add_got_offset_hilo): Likewise.
(macro): Handle microMIPS.
(validate_micromips_insn): New function.
(micromips_percent_op): New variable.
(parse_relocation): Handle microMIPS.
(my_getExpression): Likewise.
(options): Add OPTION_MICROMIPS and OPTION_NO_MICROMIPS.
(md_longopts): Add mmicromips and mno-micromips.
(md_parse_option): Handle OPTION_MICROMIPS and
OPTION_NO_MICROMIPS.
(mips_after_parse_args): Handle microMIPS.
(md_pcrel_from): Handle microMIPS relocations.
(mips_force_relocation): Likewise.
(md_apply_fix): Likewise.
(mips_align): Handle microMIPS.
(s_mipsset): Likewise.
(s_cpload, s_cpsetup, s_cpreturn): Use relocation wrappers.
(s_dtprel_internal): Likewise.
(s_gpword, s_gpdword): Likewise.
(s_insn): Handle microMIPS.
(s_mips_stab): Likewise.
(relaxed_micromips_32bit_branch_length): New function.
(relaxed_micromips_16bit_branch_length): New function.
(md_estimate_size_before_relax): Handle microMIPS.
(mips_fix_adjustable): Likewise.
(tc_gen_reloc): Handle microMIPS relocations.
(mips_relax_frag): Handle microMIPS.
(md_convert_frag): Likewise.
(mips_frob_file_after_relocs): Likewise.
(mips_elf_final_processing): Likewise.
(mips_nop_opcode): Likewise.
(mips_handle_align): Likewise.
(md_show_usage): Handle microMIPS options.
(micromips_ip): New function.
(micromips_macro_build): Likewise.
(micromips_macro): Likewise.
* symbols.c (TC_LABEL_IS_LOCAL): New macro.
(S_IS_LOCAL): Add a TC_LABEL_IS_LOCAL check.
* doc/as.texinfo (Target MIPS options): Add -mmicromips and
-mno-micromips.
(-mmicromips, -mno-micromips): New options.
* doc/c-mips.texi (-mmicromips, -mno-micromips): New options.
(MIPS ISA): Document .set micromips and .set nomicromips.
gas/testsuite/
2010-07-26 Maciej W. Rozycki <macro@codesourcery.com>
Chao-ying Fu <fu@mips.com>
* gas/mips/micromips.d: New test.
* gas/mips/micromips-trap.d: Likewise.
* gas/mips/micromips-branch-relax-pic.d: Likewise.
* gas/mips/micromips-branch-relax-pic.d: Likewise.
* gas/mips/micromips.l: New stderr output.
* gas/mips/micromips-branch-relax.l: Likewise.
* gas/mips/micromips-branch-relax-pic.l: Likewise.
* gas/mips/micromips.s: New test source.
* gas/mips/micromips-branch-relax.s: Likewise.
* gas/mips/mips.exp: Run the new tests.
* gas/mips/micromips@abs.d: New test.
* gas/mips/micromips@add.d: Likewise.
* gas/mips/micromips@and.d: Likewise.
* gas/mips/micromips@beq.d: Likewise.
* gas/mips/micromips@bge.d: Likewise.
* gas/mips/micromips@bgeu.d: Likewise.
* gas/mips/micromips@blt.d: Likewise.
* gas/mips/micromips@bltu.d: Likewise.
* gas/mips/micromips@branch-likely.d: Likewise.
* gas/mips/micromips@branch-misc-1.d: Likewise.
* gas/mips/micromips@branch-misc-2-64.d: Likewise.
* gas/mips/micromips@branch-misc-2.d: Likewise.
* gas/mips/micromips@branch-misc-2pic-64.d: Likewise.
* gas/mips/micromips@branch-misc-2pic.d: Likewise.
* gas/mips/micromips@dli.d: Likewise.
* gas/mips/micromips@elf-jal.d: Likewise.
* gas/mips/micromips@elf-rel2.d: Likewise.
* gas/mips/micromips@elf-rel4.d: Likewise.
* gas/mips/micromips@lb-svr4pic-ilocks.d: Likewise.
* gas/mips/micromips@li.d: Likewise.
* gas/mips/micromips@mips1-fp.d: Likewise.
* gas/mips/micromips@mips32-cp2.d: Likewise.
* gas/mips/micromips@mips32-imm.d: Likewise.
* gas/mips/micromips@mips32-sf32.d: Likewise.
* gas/mips/micromips@mips32.d: Likewise.
* gas/mips/micromips@mips32r2-cp2.d: Likewise.
* gas/mips/micromips@mips32r2-fp32.d: Likewise.
* gas/mips/micromips@mips32r2.d: Likewise.
* gas/mips/micromips@mips4-branch-likely.d: Likewise.
* gas/mips/micromips@mips4-fp.d: Likewise.
* gas/mips/micromips@mips4.d: Likewise.
* gas/mips/micromips@mips5.d: Likewise.
* gas/mips/micromips@mips64-cp2.d: Likewise.
* gas/mips/micromips@mips64.d: Likewise.
* gas/mips/micromips@mips64r2.d: Likewise.
* gas/mips/micromips@rol-hw.d: Likewise.
* gas/mips/micromips@uld2-eb.d: Likewise.
* gas/mips/micromips@uld2-el.d: Likewise.
* gas/mips/micromips@ulh2-eb.d: Likewise.
* gas/mips/micromips@ulh2-el.d: Likewise.
* gas/mips/micromips@ulw2-eb-ilocks.d: Likewise.
* gas/mips/micromips@ulw2-el-ilocks.d: Likewise.
* gas/mips/mips32-imm.d: Likewise.
* gas/mips/mips32.d: Update immediates.
* gas/mips/micromips@mips32-cp2.s: New test source.
* gas/mips/micromips@mips32-imm.s: Likewise.
* gas/mips/micromips@mips32r2-cp2.s: Likewise.
* gas/mips/micromips@mips64-cp2.s: Likewise.
* gas/mips/mips32-imm.s: Likewise.
* gas/mips/mips32.s: Handle microMIPS.
* gas/mips/elf-rel27.d: Likewise.
* gas/mips/mips.exp: Add the micromips arch. Exclude mips16e
from micromips. Run mips32-imm.
* gas/mips/jal-mask-11.d: New test.
* gas/mips/jal-mask-12.d: Likewise.
* gas/mips/micromips@jal-mask-11.d: Likewise.
* gas/mips/jal-mask-1.s: Source for the new tests.
* gas/mips/jal-mask-21.d: New test.
* gas/mips/jal-mask-22.d: Likewise.
* gas/mips/micromips@jal-mask-12.d: Likewise.
* gas/mips/jal-mask-2.s: Source for the new tests.
* gas/mips/mips.exp: Run the new tests.
* gas/mips/and.s: Adjust padding.
* gas/mips/beq.s: Likewise.
* gas/mips/bge.s: Likewise.
* gas/mips/bgeu.s: Likewise.
* gas/mips/blt.s: Likewise.
* gas/mips/bltu.s: Likewise.
* gas/mips/branch-misc-2.s: Likewise.
* gas/mips/jal.s: Likewise.
* gas/mips/li.s: Likewise.
* gas/mips/mips1-fp.s: Likewise.
* gas/mips/mips32r2-fp32.s: Likewise.
* gas/mips/mips64.s: Likewise.
* gas/mips/mips4.s: Likewise.
* gas/mips/mips4-fp.s: Likewise.
* gas/mips/and.d: Update accordingly.
* gas/mips/elf-jal.d: Likewise.
* gas/mips/jal.d: Likewise.
* gas/mips/li.d: Likewise.
* gas/mips/mips1-fp.d: Likewise.
* gas/mips/mips32r2-fp32.d: Likewise.
* gas/mips/mips64.d: Likewise.
include/elf/
2010-07-26 Chao-ying Fu <fu@mips.com>
* mips.h (R_MICROMIPS_min): New relocations.
(R_MICROMIPS_26_S1): Likewise.
(R_MICROMIPS_HI16, R_MICROMIPS_LO16): Likewise.
(R_MICROMIPS_GPREL16, R_MICROMIPS_LITERAL): Likewise.
(R_MICROMIPS_GOT16, R_MICROMIPS_PC7_S1): Likewise.
(R_MICROMIPS_PC10_S1, R_MICROMIPS_PC16_S1): Likewise.
(R_MICROMIPS_CALL16, R_MICROMIPS_GOT_DISP): Likewise.
(R_MICROMIPS_GOT_PAGE, R_MICROMIPS_GOT_OFST): Likewise.
(R_MICROMIPS_GOT_HI16, R_MICROMIPS_GOT_LO16): Likewise.
(R_MICROMIPS_SUB, R_MICROMIPS_HIGHER): Likewise.
(R_MICROMIPS_HIGHEST, R_MICROMIPS_CALL_HI16): Likewise.
(R_MICROMIPS_CALL_LO16, R_MICROMIPS_SCN_DISP): Likewise.
(R_MICROMIPS_JALR, R_MICROMIPS_HI0_LO16): Likewise.
(R_MICROMIPS_TLS_GD, R_MICROMIPS_TLS_LDM): Likewise.
(R_MICROMIPS_TLS_DTPREL_HI, R_MICROMIPS_TLS_DTPREL_LO): Likewise.
(R_MICROMIPS_TLS_GOTTPREL): Likewise.
(R_MICROMIPS_TLS_TPREL_HI16): Likewise.
(R_MICROMIPS_TLS_TPREL_LO16): Likewise.
(R_MICROMIPS_GPREL7_S2, R_MICROMIPS_PC23_S2): Likewise.
(R_MICROMIPS_max): Likewise.
(EF_MIPS_ARCH_ASE_MICROMIPS): New macro.
(STO_MIPS_ISA, STO_MIPS_FLAGS): Likewise.
(ELF_ST_IS_MIPS_PLT, ELF_ST_SET_MIPS_PLT): Likewise.
(STO_MICROMIPS): Likewise.
(ELF_ST_IS_MICROMIPS, ELF_ST_SET_MICROMIPS): Likewise.
(ELF_ST_IS_COMPRESSED): Likewise.
(STO_MIPS_PLT, STO_MIPS_PIC): Rework.
(ELF_ST_IS_MIPS_PIC, ELF_ST_SET_MIPS_PIC): Likewise.
(STO_MIPS16, ELF_ST_IS_MIPS16, ELF_ST_SET_MIPS16): Likewise.
include/opcode/
2010-07-26 Chao-ying Fu <fu@mips.com>
Maciej W. Rozycki <macro@codesourcery.com>
* mips.h
(INSN2_BRANCH_DELAY_16BIT, INSN2_BRANCH_DELAY_32BIT): New macros.
(INSN2_WRITE_GPR_S, INSN2_READ_FPR_D): Likewise.
(INSN2_MOD_GPR_MB, INSN2_MOD_GPR_MC, INSN2_MOD_GPR_MD): Likewise.
(INSN2_MOD_GPR_ME, INSN2_MOD_GPR_MF, INSN2_MOD_GPR_MG): Likewise.
(INSN2_MOD_GPR_MJ, INSN2_MOD_GPR_MP, INSN2_MOD_GPR_MQ): Likewise.
(INSN2_MOD_SP, INSN2_READ_GPR_31): Likewise.
(INSN2_READ_GP, INSN2_READ_PC): Likewise.
(CPU_MICROMIPS): New macro.
(M_BC1FL, M_BC1TL, M_BC2FL, M_BC2TL, M_BEQL, M_BGEZL): New enum
values.
(M_BGEZALL, M_BGTZL, M_BLEZL, M_BLTZL, M_BLTZALL): Likewise.
(M_CACHE_OB, M_LDC2_OB, M_LDL_OB, M_LDM_AB, M_LDM_OB): Likewise.
(M_LDP_AB, M_LDP_OB, M_LDR_OB, M_LL_OB, M_LLD_OB): Likewise.
(M_LWC2_OB, M_LWL_OB, M_LWM_AB, M_LWP_AB, M_LWP_OB): Likewise.
(M_LWR_OB, M_LWU_OB, M_PREF_OB, M_SC_OB, M_SCD_OB): Likewise.
(M_SDC2_OB, M_SDL_OB, M_SDM_AB, M_SDM_OB, M_SDP_AB): Likewise.
(M_SDP_OB, M_SDR_OB, M_SWC2_OB, M_SWL_OB, M_SWM_AB): Likewise.
(M_SWM_OB, M_SWP_AB, M_SWP_OB, M_SWR_OB): Likewise.
(MICROMIPSOP_MASK_MAJOR, MICROMIPSOP_SH_MAJOR): New macros.
(MICROMIPSOP_MASK_IMMEDIATE, MICROMIPSOP_SH_IMMEDIATE): Likewise.
(MICROMIPSOP_MASK_DELTA, MICROMIPSOP_SH_DELTA): Likewise.
(MICROMIPSOP_MASK_CODE10, MICROMIPSOP_SH_CODE10): Likewise.
(MICROMIPSOP_MASK_TRAP, MICROMIPSOP_SH_TRAP): Likewise.
(MICROMIPSOP_MASK_SHAMT, MICROMIPSOP_SH_SHAMT): Likewise.
(MICROMIPSOP_MASK_TARGET, MICROMIPSOP_SH_TARGET): Likewise.
(MICROMIPSOP_MASK_EXTLSB, MICROMIPSOP_SH_EXTLSB): Likewise.
(MICROMIPSOP_MASK_EXTMSBD, MICROMIPSOP_SH_EXTMSBD): Likewise.
(MICROMIPSOP_MASK_INSMSB, MICROMIPSOP_SH_INSMSB): Likewise.
(MICROMIPSOP_MASK_BREAKCODE, MICROMIPSOP_SH_BREAKCODE): Likewise.
(MICROMIPSOP_SH_BREAKCODE2): Likewise.
(MICROMIPSOP_MASK_CACHEOP, MICROMIPSOP_SH_CACHEOP): Likewise.
(MICROMIPSOP_MASK_COPSEL, MICROMIPSOP_SH_COPSEL): Likewise.
(MICROMIPSOP_MASK_OFFSET12, MICROMIPSOP_SH_OFFSET12): Likewise.
(MICROMIPSOP_MASK_3BITPOS, MICROMIPSOP_SH_3BITPOS): Likewise.
(MICROMIPSOP_MASK_STYPE, MICROMIPSOP_SH_STYPE): Likewise.
(MICROMIPSOP_MASK_OFFSET10, MICROMIPSOP_SH_OFFSET10): Likewise.
(MICROMIPSOP_MASK_RS, MICROMIPSOP_SH_RS): Likewise.
(MICROMIPSOP_MASK_RT, MICROMIPSOP_SH_RT): Likewise.
(MICROMIPSOP_MASK_RD, MICROMIPSOP_SH_RD): Likewise.
(MICROMIPSOP_MASK_FS, MICROMIPSOP_SH_FS): Likewise.
(MICROMIPSOP_MASK_FT, MICROMIPSOP_SH_FT): Likewise.
(MICROMIPSOP_MASK_FD, MICROMIPSOP_SH_FD): Likewise.
(MICROMIPSOP_MASK_FR, MICROMIPSOP_SH_FR): Likewise.
(MICROMIPSOP_MASK_RS3, MICROMIPSOP_SH_RS3): Likewise.
(MICROMIPSOP_MASK_PREFX, MICROMIPSOP_SH_PREFX): Likewise.
(MICROMIPSOP_MASK_BCC, MICROMIPSOP_SH_BCC): Likewise.
(MICROMIPSOP_MASK_CCC, MICROMIPSOP_SH_CCC): Likewise.
(MICROMIPSOP_MASK_COPZ, MICROMIPSOP_SH_COPZ): Likewise.
(MICROMIPSOP_MASK_MB, MICROMIPSOP_SH_MB): Likewise.
(MICROMIPSOP_MASK_MC, MICROMIPSOP_SH_MC): Likewise.
(MICROMIPSOP_MASK_MD, MICROMIPSOP_SH_MD): Likewise.
(MICROMIPSOP_MASK_ME, MICROMIPSOP_SH_ME): Likewise.
(MICROMIPSOP_MASK_MF, MICROMIPSOP_SH_MF): Likewise.
(MICROMIPSOP_MASK_MG, MICROMIPSOP_SH_MG): Likewise.
(MICROMIPSOP_MASK_MJ, MICROMIPSOP_SH_MJ): Likewise.
(MICROMIPSOP_MASK_ML, MICROMIPSOP_SH_ML): Likewise.
(MICROMIPSOP_MASK_MP, MICROMIPSOP_SH_MP): Likewise.
(MICROMIPSOP_MASK_MQ, MICROMIPSOP_SH_MQ): Likewise.
(MICROMIPSOP_MASK_MZ, MICROMIPSOP_SH_MZ): Likewise.
(MICROMIPSOP_MASK_IMMA, MICROMIPSOP_SH_IMMA): Likewise.
(MICROMIPSOP_MASK_IMMB, MICROMIPSOP_SH_IMMB): Likewise.
(MICROMIPSOP_MASK_IMMC, MICROMIPSOP_SH_IMMC): Likewise.
(MICROMIPSOP_MASK_IMMD, MICROMIPSOP_SH_IMMD): Likewise.
(MICROMIPSOP_MASK_IMME, MICROMIPSOP_SH_IMME): Likewise.
(MICROMIPSOP_MASK_IMMF, MICROMIPSOP_SH_IMMF): Likewise.
(MICROMIPSOP_MASK_IMMG, MICROMIPSOP_SH_IMMG): Likewise.
(MICROMIPSOP_MASK_IMMH, MICROMIPSOP_SH_IMMH): Likewise.
(MICROMIPSOP_MASK_IMMI, MICROMIPSOP_SH_IMMI): Likewise.
(MICROMIPSOP_MASK_IMMJ, MICROMIPSOP_SH_IMMJ): Likewise.
(MICROMIPSOP_MASK_IMML, MICROMIPSOP_SH_IMML): Likewise.
(MICROMIPSOP_MASK_IMMM, MICROMIPSOP_SH_IMMM): Likewise.
(MICROMIPSOP_MASK_IMMN, MICROMIPSOP_SH_IMMN): Likewise.
(MICROMIPSOP_MASK_IMMO, MICROMIPSOP_SH_IMMO): Likewise.
(MICROMIPSOP_MASK_IMMP, MICROMIPSOP_SH_IMMP): Likewise.
(MICROMIPSOP_MASK_IMMQ, MICROMIPSOP_SH_IMMQ): Likewise.
(MICROMIPSOP_MASK_IMMU, MICROMIPSOP_SH_IMMU): Likewise.
(MICROMIPSOP_MASK_IMMW, MICROMIPSOP_SH_IMMW): Likewise.
(MICROMIPSOP_MASK_IMMX, MICROMIPSOP_SH_IMMX): Likewise.
(MICROMIPSOP_MASK_IMMY, MICROMIPSOP_SH_IMMY): Likewise.
(micromips_opcodes): New declaration.
(bfd_micromips_num_opcodes): Likewise.
* mips.h (INSN2_UNCOND_BRANCH, INSN2_COND_BRANCH): New macros.
* mips.h (INSN2_MOD_GPR_MHI): New macro.
(INSN2_MOD_GPR_MM, INSN2_MOD_GPR_MN): Likewise.
(MICROMIPSOP_MASK_MH, MICROMIPSOP_SH_MH): Likewise.
(MICROMIPSOP_MASK_MI, MICROMIPSOP_SH_MI): Likewise.
(MICROMIPSOP_MASK_MM, MICROMIPSOP_SH_MM): Likewise.
(MICROMIPSOP_MASK_MN, MICROMIPSOP_SH_MN): Likewise.
ld/testsuite/
2010-07-26 Catherine Moore <clm@codesourcery.com>
Chao-ying Fu <fu@mips.com>
Maciej W. Rozycki <macro@codesourcery.com>
* ld-mips-elf/jalx-1.s: New test source.
* ld-mips-elf/jalx-1.d: New test output.
* ld-mips-elf/jalx-1.ld: New test linker script.
* ld-mips-elf/jalx-2-main.s: New test source.
* ld-mips-elf/jalx-2-ex.s: Likewise.
* ld-mips-elf/jalx-2-printf.s: Likewise.
* ld-mips-elf/jalx-2.dd: New test output.
* ld-mips-elf/jalx-2.ld: New test linker script.
* ld-mips-elf/mips-elf.exp: Run the new tests
opcodes/
2010-07-26 Chao-ying Fu <fu@mips.com>
Maciej W. Rozycki <macro@codesourcery.com>
* micromips-opc.c: New file.
* mips-dis.c (micromips_to_32_reg_b_map): New array.
(micromips_to_32_reg_c_map, micromips_to_32_reg_d_map): Likewise.
(micromips_to_32_reg_e_map, micromips_to_32_reg_f_map): Likewise.
(micromips_to_32_reg_g_map, micromips_to_32_reg_l_map): Likewise.
(micromips_to_32_reg_q_map): Likewise.
(micromips_imm_b_map, micromips_imm_c_map): Likewise.
(print_insn_micromips): New function.
(is_micromips_mode_p): New function.
(_print_insn_mips): Handle microMIPS instructions.
* Makefile.am (CFILES): Add micromips-opc.c.
* configure.in (bfd_mips_arch): Add micromips-opc.lo.
* Makefile.in: Regenerate.
* configure: Regenerate.
* mips-dis.c (micromips_to_32_reg_h_map): New variable.
(micromips_to_32_reg_i_map): Likewise.
(micromips_to_32_reg_m_map): Likewise.
(micromips_to_32_reg_n_map): New macro.Attachment:
binutils-fsf-2.20.51-20100726-umips-62.patch.bz2
Description: Binary data
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |