RE: [PATCH] Add support for MIPS64r6

[Andrew Bennett <Andrew dot Bennett at imgtec dot com>]

> Sorry for taking so long to get to this.

That's ok.  We are working on quite a few patches at the moment, so 
sorry for the delay in replying.
 
> > +static const bfd_vma mipsr6_exec_plt_entry[] =
> > +{
> > +  0x3c0f0000,	/* lui $15, %hi(.got.plt entry)			*/
> > +  0x01f90000,	/* l[wd] $25, %lo(.got.plt entry)($15)		*/
> > +  0x25f80000,	/* addiu $24, $15, %lo(.got.plt entry)		*/
> > +  0x03200009	/* jr $25					*/
> > +};
> 
> We don't need to worry about MIPS I compatibility here, so we could put
> the JR before the ADDIU and avoid executing the LUI from the following
> PLT.  What you have is fine too if you think it's better though.

Agreed.  We hadn't understood the rationale for the existing structure
but as you say the MIPS I issue is no longer relevant.
 
> > +    case R_MIPS_PCHI16:
> > +      if (howto->partial_inplace)
> > +	addend = _bfd_mips_elf_sign_extend (addend, 16);
> > +      value = mips_elf_high (symbol + addend - p);
> > +      BFD_ASSERT (howto->rightshift == 16);
> > +      overflowed_p = mips_elf_overflow_p (value, 16);
> > +      value &= howto->dst_mask;
> > +      break;
> 
> Is the REL handling deliberately different from other HI16s here?
> Normally the idea is that the HI16 is paired with a following LO16
> and you combine the in-place addends from both to get the full
> HI16 addend.  "addend" would then already be the full addend
> in this case.

You are correct here: there should be no difference in the REL handling.  I 
I will add some code to correctly calculate the addend for the 
R_MIPS_PCHI16 relocation.

> > diff --git a/binutils/testsuite/binutils-all/objcopy.exp
> b/binutils/testsuite/binutils-all/objcopy.exp
> > index 6159b9d..9b22744 100644
> > --- a/binutils/testsuite/binutils-all/objcopy.exp
> > +++ b/binutils/testsuite/binutils-all/objcopy.exp
> > @@ -986,6 +986,7 @@ if [is_elf_format] {
> >      # targ_defvec=bfd_elf32_nlittlemips_vec in config.bfd.  When syncing,
> >      # don't forget that earlier case-matches trump later ones.
> >      if { ![istarget "mips*-sde-elf*"] && ![istarget "mips*-mti-elf*"]
> > +	 && ![istarget "mips*-img-elf*"]
> >           && ![istarget "mips64*-*-openbsd*"] } {
> >  	setup_xfail "mips*-*-irix5*" "mips*-*-irix6*" "mips*-*-elf*" \
> >  	    "mips*-*-rtems*" "mips*-*-windiss" "mips*-*-none" \
> > diff --git a/binutils/testsuite/binutils-all/readelf.exp
> b/binutils/testsuite/binutils-all/readelf.exp
> > index 2a6bc6a..e45d6ea 100644
> > --- a/binutils/testsuite/binutils-all/readelf.exp
> > +++ b/binutils/testsuite/binutils-all/readelf.exp
> > @@ -103,6 +103,7 @@ proc readelf_test { options binary_file regexp_file
> xfails } {
> >  	if { [istarget "mips*-*-*linux*"]
> >  	     || [istarget "mips*-sde-elf*"]
> >  	     || [istarget "mips*-mti-elf*"]
> > +	     || [istarget "mips*-img-elf*"]
> >  	     || [istarget "mips*-*freebsd*"] } then {
> >  	    set target_machine tmips
> >  	} else {
> 
> Please submit the img-elf bits separately.

Done.

> > @@ -5245,11 +5483,13 @@ match_pc_operand (struct mips_arg_info *arg)
> >     register that we need to match.  */
> >
> >  static bfd_boolean
> > -match_tied_reg_operand (struct mips_arg_info *arg, unsigned int
> other_regno)
> > +match_tied_reg_operand (struct mips_arg_info *arg,
> > +			enum mips_reg_operand_type type,
> > +			unsigned int other_regno)
> >  {
> >    unsigned int regno;
> >
> > -  return match_reg (arg, OP_REG_GP, &regno) && regno == other_regno;
> > +  return match_reg (arg, type, &regno) && regno == other_regno;
> >  }
> >
> >  /* Read a floating-point constant from S for LI.S or LI.D.  LENGTH is
> > @@ -5495,10 +5735,10 @@ match_operand (struct mips_arg_info *arg,
> >        return match_mdmx_imm_reg_operand (arg, operand);
> >
> >      case OP_REPEAT_DEST_REG:
> > -      return match_tied_reg_operand (arg, arg->dest_regno);
> > +      return match_tied_reg_operand (arg, OP_REG_GP, arg->dest_regno);
> >
> >      case OP_REPEAT_PREV_REG:
> > -      return match_tied_reg_operand (arg, arg->last_regno);
> > +      return match_tied_reg_operand (arg, OP_REG_GP, arg->last_regno);
> >
> >      case OP_PC:
> >        return match_pc_operand (arg);
> 
> Couldn't tell off-hand why you need this (although it looks OK).

The introduction of the new argument is actually left over from intermediate
work on the R6 ISA.  There were plans for supporting some floating-point
mnemonics which required register matching but these were dropped.  We will
remove this and add it if we do end up with a need to match registers in
FP instructions.

> > @@ -6571,6 +6838,46 @@ append_insn (struct mips_cl_insn *ip, expressionS
> *address_expr,
> >  	  }
> >  	  break;
> >
> > +	case BFD_RELOC_MIPS_21_PCREL_S2:
> > +	  {
> > +	    int shift;
> > +
> > +	    shift = 2;
> > +	    if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
> > +	      as_bad (_("branch to misaligned address (0x%lx)"),
> > +		      (unsigned long) address_expr->X_add_number);
> > +	    if (!mips_relax_branch)
> > +	      {
> > +		if ((address_expr->X_add_number + (1 << (shift + 20)))
> > +		    & ~((1 << (shift + 21)) - 1))
> > +		  as_bad (_("branch address range overflow (0x%lx)"),
> > +			  (unsigned long) address_expr->X_add_number);
> > +		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
> > +				    & 0x1fffff);
> > +	      }
> > +	  }
> > +	  break;
> > +
> > +	case BFD_RELOC_MIPS_26_PCREL_S2:
> > +	  {
> > +	    int shift;
> > +
> > +	    shift = 2;
> > +	    if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
> > +	      as_bad (_("branch to misaligned address (0x%lx)"),
> > +		      (unsigned long) address_expr->X_add_number);
> > +	    if (!mips_relax_branch)
> > +	      {
> > +		if ((address_expr->X_add_number + (1 << (shift + 25)))
> > +		    & ~((1 << (shift + 26)) - 1))
> > +		  as_bad (_("branch address range overflow (0x%lx)"),
> > +			  (unsigned long) address_expr->X_add_number);
> > +		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
> > +				    & 0x3ffffff);
> > +	      }
> > +	  }
> > +	  break;
> 
> I think we should drop the !mips_relax_branch test until branch relaxation
> is supported for r6.

Thats fine.  The code was there as a marker to remind us that branch relaxation 
support needs to be added for R6.

> > @@ -14462,6 +14836,81 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg
> ATTRIBUTE_UNUSED)
> >  	md_number_to_chars (buf, *valP, fixP->fx_size);
> >        break;
> >
> > +    case BFD_RELOC_MIPS_21_PCREL_S2:
> > +      if ((*valP & 0x3) != 0)
> > +	as_bad_where (fixP->fx_file, fixP->fx_line,
> > +		      _("branch to misaligned address (%lx)"), (long) *valP);
> > +
> > +      /* We need to save the bits in the instruction since fixup_segment()
> > +	 might be deleting the relocation entry (i.e., a branch within
> > +	 the current segment).  */
> > +      if (! fixP->fx_done)
> > +	break;
> > +
> > +      /* Update old instruction data.  */
> > +      insn = read_insn (buf);
> > +
> > +      if (*valP + 0x400000 <= 0x7fffff)
> > +	{
> > +	  insn |= (*valP >> 2) & 0x1fffff;
> > +	  write_insn (buf, insn);
> > +	}
> > +      else
> > +	{
> > +	  /* CFU FIXME.  */
> > +	  gas_assert (0);
> > +	}
> > +      break;
> > +
> > +    case BFD_RELOC_MIPS_26_PCREL_S2:
> > +      if ((*valP & 0x3) != 0)
> > +	as_bad_where (fixP->fx_file, fixP->fx_line,
> > +		      _("branch to misaligned address (%lx)"), (long) *valP);
> > +
> > +      /* We need to save the bits in the instruction since fixup_segment()
> > +	 might be deleting the relocation entry (i.e., a branch within
> > +	 the current segment).  */
> > +      if (! fixP->fx_done)
> > +	break;
> > +
> > +      /* Update old instruction data.  */
> > +      insn = read_insn (buf);
> > +
> > +      if (*valP + 0x8000000 <= 0xfffffff)
> > +	{
> > +	  insn |= (*valP >> 2) & 0x3ffffff;
> > +	  write_insn (buf, insn);
> > +	}
> > +      else
> > +	{
> > +	  /* CFU FIXME.  */
> > +	  gas_assert (0);
> > +	}
> > +      break;
> 
> Can fx_done ever happen given the mips_force_relocation change?
> I think we should assert if not, like you do for the others.

You are correct.   I will make the code the same as the other cases.

> 
> > +    case BFD_RELOC_MIPS_18_PCREL_S3:
> > +      if ((*valP & 0x3) != 0)
> > +	as_bad_where (fixP->fx_file, fixP->fx_line,
> > +		      _("pc rel from misaligned address (%lx)"),
> > +		      (long) *valP);
> > +
> > +      gas_assert(!fixP->fx_done);
> > +      break;
> 
> Should this be 7 rather than 3?

Yes.

> > @@ -1089,7 +1146,8 @@ struct mips_opcode
> >     (mips_isa_table[(Y & INSN_ISA_MASK) - 1] >> ((X & INSN_ISA_MASK) - 1)) &
> 1
> >     is non-zero.  */
> >  static const unsigned int mips_isa_table[] =
> > -  { 0x0001, 0x0003, 0x0607, 0x1e0f, 0x3e1f, 0x0a23, 0x3e63, 0x3ebf, 0x3fff
> };
> > +  { 0x0001, 0x0003, 0x0607, 0x1e0f, 0x3e1f, 0x0a23, 0x3e63, 0x3ebf, 0x3fff,
> > +    0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x7e63, 0xffff };
> 
> Is it really true that r6 allows everything, given the incompatibility?

The problem is that the removed instructions in R6 come from different ISAs.  One 
approach to solve this is to describe in the membership field the different ISAs an 
instruction belongs to.  This would require us to create a large amount of different ISA
combinations which is hard to manage.  A cleaner approach (and implemented in the patch) 
is to say that R6 is an extension of R5 and then to deal with the removed instructions by 
adding instruction exclusions for R6.  

> > @@ -1426,9 +1542,27 @@ print_insn_args (struct disassemble_info *info,
> >  		infprintf (is, "$%d,%d", reg, sel);
> >  	    }
> >  	  else
> > -	    print_insn_arg (info, &state, opcode, operand, base_pc,
> > -			    mips_extract_operand (operand, insn));
> > -	  if (*s == 'm' || *s == '+')
> > +	    {
> > +	      bfd_vma base_pc = insn_pc;
> > +
> > +	      /* Adjust the PC relative base so that branch/jump insns use
> > +		 the following PC as the base but genuinely PC relative
> > +		 operands use the current PC.  */
> > +	      if (operand->type == OP_PCREL)
> > +		{
> > +		  const struct mips_pcrel_operand *pcrel_op;
> > +
> > +		  pcrel_op = (const struct mips_pcrel_operand *) operand;
> > +		  /* The include_isa_bit flag is sufficient to distinguish
> > +		     branch/jump from other PC relative operands.  */
> > +		  if (pcrel_op->include_isa_bit)
> > +		    base_pc += length;
> > +		}
> > +
> > +	      print_insn_arg (info, &state, opcode, operand, base_pc,
> > +			      mips_extract_operand (operand, insn));
> > +	    }
> > +	  if (*s == 'm' || *s == '+' || *s == '-')
> >  	    ++s;
> >  	  break;
> >  	}
> > @@ -1494,9 +1628,60 @@ print_insn_mips (bfd_vma memaddr,
> >  	      && !(no_aliases && (op->pinfo2 & INSN2_ALIAS))
> >  	      && (word & op->mask) == op->match)
> >  	    {
> > +	      if (strcmp (op->name, "bgezc") == 0
> > +		  || strcmp (op->name, "bltzc") == 0
> > +		  || strcmp (op->name, "bgezalc") == 0
> > +		  || strcmp (op->name, "bltzalc") == 0)
> > +		{
> > +		  if (((word >> 16) & 31) != ((word >> 21) & 31)
> > +		      || ((word >> 16) & 31) == 0)
> > +		    continue;
> > +		}
> > +	      else if (strcmp (op->name, "blezalc") == 0
> > +		       || strcmp (op->name, "bgtzalc") == 0
> > +		       || strcmp (op->name, "blezc") == 0
> > +		       || strcmp (op->name, "bgtzc") == 0
> > +		       || strcmp (op->name, "beqzalc") == 0
> > +		       || strcmp (op->name, "bnezalc") == 0)
> > +		{
> > +		  if (((word >> 16) & 31) == 0)
> > +		    continue;
> > +		}
> > +	      else if (strcmp (op->name, "bgec") == 0
> > +		       || strcmp (op->name, "bltc") == 0
> > +		       || strcmp (op->name, "bbec") == 0
> > +		       || strcmp (op->name, "bstc") == 0)
> > +		{
> > +		  if (((word >> 16) & 31) == ((word >> 21) & 31)
> > +		      || ((word >> 21) & 31) == 0
> > +		      || ((word >> 16) & 31) == 0)
> > +		    continue;
> > +		}
> > +	      else if (strcmp (op->name, "beqc") == 0
> > +		       || strcmp (op->name, "bnec") == 0)
> > +		{
> > +		  if (((word >> 21) & 31) >= ((word >> 16) & 31)
> > +		      || ((word >> 21) & 31) == 0)
> > +		    continue;
> > +		}
> > +	      else if (strcmp (op->name, "bovc") == 0
> > +		       || strcmp (op->name, "bnvc") == 0)
> > +		{
> > +		  if (((word >> 21) & 31) < ((word >> 16) & 31))
> > +		    continue;
> > +		}
> > +	      else if (strcmp (op->name, "beqzc") == 0
> > +		       || strcmp (op->name, "bnezc") == 0)
> > +		{
> > +		  if (((word >> 21) & 31) == 0)
> > +		    continue;
> > +		}
> 
> Looks like this is just reinforcing the restrictions from the OP_*s,
> is that right?  If so, I think it would be cleaner to use the
> mips_operand information rather than checks for specific instructions.

I agree with you that the code could be better here.  The problem is that
for some of the R6 instructions the match and mask fields are identical 
(one example is the bnvc and bnec instructions).  Currently the print_insn_mips 
function uses the match and mask fields to find the first instruction in the 
opcode table which matches the instruction to disassemble.  Then it takes each 
of the operands in turn and checks that they are valid.  This approach will obviously 
not work when decoding some R6 instructions.  The current fix is to skip over 
R6 instructions where the operands don't match (which as we said before is not 
very maintainable).  We could do a more generic fix, but that would require rewriting 
the print_insn_mips, print_insn_args, and print_insn_arg functions to find an 
instruction in the opcode table where firstly the match and mask fields match the 
instruction to disassemble; and secondly all the operand constraints are met.  If 
this is the case the instruction could then be printed out.

I was wondering what you felt would be the best approach here?


Regards,


Andrew