Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Fu, Chao-Ying" <fu@mips.com> writes:
> Richard Sandiford wrote:
>> > 	(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.

Richard