Re: [PATCH] MIPS: microMIPS ASE support

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  As it has turned out in the course of sorting out some earlier concerns 
> the microMIPS change needs a couple of updates.  For your reference I'm 
> sending the current version of the original patch as it had to be 
> regenerated.  On top of this I'm sending the following updates:

Everything except binutils-gas-umips-swap.diff is OK (as one commit,
like you say), with the changes below.  If you don't agree with some
of the requested changes, let me know.

I thinkb inutils-gas-umips-swap.diff should go in as a separate commit,
and I'll review it separately.

> - binutils-umips-opcode-trap.diff -- a complementing microMIPS change to 
>   the trap/no-delay slot annotation made to standard MIPS/MIPS16 code,

Nit:

-      target_is_micromips_code_p = (htab->splt != sec)
-				    && ELF_ST_IS_MICROMIPS (h->root.other);
+      target_is_micromips_code_p = ((htab->splt != sec)
+				    && ELF_ST_IS_MICROMIPS (h->root.other));

should be:

      target_is_micromips_code_p = (htab->splt != sec
				    && ELF_ST_IS_MICROMIPS (h->root.other));

-      if (isym->st_shndx == sec_shndx
-	  && isym->st_value > addr
-	  && isym->st_value < toaddr)
+      bfd_vma value;
+
+      if (isym->st_shndx != sec_shndx)
+	continue;
+
+      value = isym->st_value;
+      if (ELF_ST_IS_MICROMIPS (isym->st_other))
+	value &= MINUS_TWO;
+      if (value > addr)
 	isym->st_value -= count;

I still don't understand why we need to mask the low bit here.
As per the original review, aren't these symbols already even?
Only those entered into the hash table are odd.  OK as:

      if (isym->st_shndx == sec_shndx
	  && isym->st_value > addr)
	isym->st_value -= count;

if that's correct, but please let me know if it isn't.

@@ -11936,12 +11933,17 @@ mips_elf_relax_delete_bytes (bfd *abfd,
   for (; sym_hashes < end_hashes; sym_hashes++)
     {
       struct elf_link_hash_entry *sym_hash = *sym_hashes;
+      bfd_vma value;
 
-      if ((sym_hash->root.type == bfd_link_hash_defined
-	   || sym_hash->root.type == bfd_link_hash_defweak)
-	  && sym_hash->root.u.def.section == sec
-	  && sym_hash->root.u.def.value > addr
-	  && sym_hash->root.u.def.value < toaddr)
+      if ((sym_hash->root.type != bfd_link_hash_defined
+	   && sym_hash->root.type != bfd_link_hash_defweak)
+	  || sym_hash->root.u.def.section != sec)
+	continue;
+
+      value = sym_hash->root.u.def.value;
+      if (ELF_ST_IS_MICROMIPS (sym_hash->other))
+	value &= MINUS_TWO;
+      if (value > addr)
 	sym_hash->root.u.def.value -= count;
     }

Very much nit stage, but "continue" seems overkill here.  I preferred
the original style, which doesn't have the combination of positive
and negative tests.  OK as:

      if ((sym_hash->root.type == bfd_link_hash_defined
	   || sym_hash->root.type == bfd_link_hash_defweak)
	  && sym_hash->root.u.def.section == sec)
	{
	  bfd_vma value;

	  value = sym_hash->root.u.def.value;
	  if (ELF_ST_IS_MICROMIPS (sym_hash->other))
	    value &= MINUS_TWO;
	  if (value > addr)
	    sym_hash->root.u.def.value -= count;
	}

+	  /* See if there is a jump or a branch reloc preceding the
+	     LUI instruction immediately.  */
+	  for (ibrrel = internal_relocs; ibrrel < irelend; ibrrel++)
+	    {
+	      offset = irel->r_offset - ibrrel->r_offset;
+	      if (offset != 2 && offset != 4)
+		continue;
+
+	      br_r_type = ELF32_R_TYPE (ibrrel->r_info);
+	      if (offset == 2
+		  && (br_r_type == R_MICROMIPS_PC7_S1
+		      || br_r_type == R_MICROMIPS_PC10_S1
+		      || br_r_type == R_MICROMIPS_JALR))
+		break;
+	      if (offset == 4
+		  && (br_r_type == R_MICROMIPS_26_S1
+		      || br_r_type == R_MICROMIPS_PC16_S1
+		      || br_r_type == R_MICROMIPS_JALR))
+		{
+		  bfd_byte *ptr = contents + ibrrel->r_offset;
+		  unsigned long bropc;
+
+		  bropc   = bfd_get_16 (abfd, ptr);
+		  bropc <<= 16;
+		  bropc  |= bfd_get_16 (abfd, ptr + 2);
+		  /* Compact branches are OK.  */
+		  if (find_match (opcode, bzc_insns_32) >= 0)
+		    brc = TRUE;
+		  break;
+		}
+	    }
+	  /* A delay slot was found, give up, sigh...  */
+	  if (!brc && ibrrel < irelend)
+	    continue;
+
+	  /* Otherwise see if the LUI instruction *might* be in a
+	     branch delay slot.  */
+	  if (!brc)
+	    {
+	      bfd_byte *ptr = contents + irel->r_offset;
+
+	      if (irel->r_offset >= 2)
+		bdsize = check_br16_dslot (abfd, ptr - 2);
+	      /* A branch possibly found, give up, sigh...  */
+	      if (bdsize > 0)
+		continue;
+	      if (irel->r_offset >= 4)
+		bdsize = check_br32_dslot (abfd, ptr - 4);
+	      /* A branch possibly found, give up, sigh...  */
+	      if (bdsize > 0)
+		continue;
+	    }

ISTR discussing this before, but with the new approach, it ought not to
be necessary to check the relocations for this get-out:

	  /* A delay slot was found, give up, sigh...  */
	  if (!brc && ibrrel < irelend)
	    continue;

because the following code ought to detect the same cases (and do
so much more cheaply).  If you want to avoid this:

	      if (irel->r_offset >= 2)
		bdsize = check_br16_dslot (abfd, ptr - 2);
	      /* A branch possibly found, give up, sigh...  */
	      if (bdsize > 0)
		continue;

triggering for cases where the relocs tell us that the instruction
is actually a BRC, then I think it would be better to split the
search out into a separate function and only use it when we would
otherwise continue.  OK as:

/* See if relocations [INTERNAL_RELOCS, IRELEND) confirm that there
   is a 4-byte branch at offset OFFSET.  */

static boolean
check_4byte_branch (Elf_Internal_Rela *internal_relocs,
		    Elf_Internal_Rela *irelend, bfd_vma offset)
{
  Elf_Internal_Rela *irel;
  unsigned long r_type;

  for (irel = internal_relocs; irel < irelend; irel++)
    if (irel->r_offset == offset)
      {
	r_type = ELF32_R_TYPE (ibrrel->r_info);
	if (br_r_type == R_MICROMIPS_26_S1
	    || br_r_type == R_MICROMIPS_PC16_S1
	    || br_r_type == R_MICROMIPS_JALR)
	  return TRUE;
      }
  return FALSE;
}
...

	  /* See if the LUI instruction *might* be in a branch delay slot.  */
	  if (irel->r_offset >= 2
	      && check_br16_dslot (abfd, ptr - 2) > 0
	      && !(irel->r_offset >= 4
		   /* If the instruction is actually a 4-byte branch,
		      the value of check_br16_dslot doesn't matter.
		      We should use check_br32_dslot to check whether
		      the branch has a delay slot.  */
		   && check_4byte_branch (internal_relocs, irelend,
					  irel->r_offset - 4)))
	    continue;
	  if (irel->r_offset >= 4
	      && check_br32_dslot (abfd, ptr - 4) > 0)
	    continue;

if that's correct (with trivial fixes to make it compile :-)),
otherwise please let me know.

Of course, this could be generalised so that if the relocations say
we have any type of 4-byte instruction, check_br16_dslot doesn't matter,
and vice versa.  But even if you'd like to do that, it's follow-on material.
Let's get the current code in first.

bdsize should be dead after the changes above.

-	  /* Give up if not the same register used with both relocations.  */
+	  /* Give up unless the same register used with both relocations.  */

Should be:

	  /* Give up unless the same register is used with both relocations.  */

+/* Check if S points at a valid register list according to TYPES.
+   If so, then return 1, advance S to consume the list and store
+   the registers present on the list as a bitmask of ones in REGLISTP,
+   otherwise return 0.  A valid list comprises a comma-separated
+   enumeration of valid single registers and/or dash-separated
+   contiguous register ranges as determined by their numbers.
+
+   As a special exception if one of s0-s7 registers is specified as
+   the range's lower delimiter and s8 (fp) is its upper one, then no
+   registers whose numbers place them between s7 and s8 (i.e. $24-$29)
+   are selected; they have to be named separately if needed.  */
+
+static int
+reglist_lookup (char **s, unsigned int types, unsigned int *reglistp)
+{
+  unsigned int reglist = 0;
+  unsigned int lastregno;
+  bfd_boolean ok = TRUE;
+  unsigned int regmask;
+  unsigned int regno;
+  char *s_reset = *s;
+  char *s_comma = *s;
+
+  while (reg_lookup (s, types, &regno))
+    {
+      lastregno = regno;
+      if (**s == '-')
+	{
+	  (*s)++;
+	  ok = reg_lookup (s, types, &lastregno);
+	  if (ok && lastregno < regno)
+	    ok = FALSE;
+	  if (!ok)
+	    break;
+	}
+
+      if (lastregno == FP && regno >= S0 && regno <= S7)
+	{
+	  lastregno = S7;
+	  reglist |= 1 << FP;
+	}
+      regmask = 1 << lastregno;
+      regmask = (regmask << 1) - 1;
+      regmask ^= (1 << regno) - 1;
+      reglist |= regmask;
+
+      s_comma = *s;
+      if (**s != ',')
+	break;
+      (*s)++;
+    }
+
+  if (ok)
+    *s = s_comma;
+  else
+    *s = s_reset;
+  if (reglistp)
+    *reglistp = reglist;
+  return ok && reglist != 0;
+}

I found s_comma a confusing name for something that often doesn't
point to a comma.  OK as "s_end_of_reglist", otherwise let me know.

+      /* If the previous instruction has an incorrect size for a fixed
+         branch delay slot in the microMIPS mode, we cannot swap.  */

OK as "in microMIPS mode".

As discussed later, from the original patch:

+/* These are the bitmasks and shift counts used for the different
+   fields in the instruction formats.  Other than OP, no masks are
+   provided for the fixed portions of an instruction, since they are
+   not needed.  */

Just drop the "Other than OP, ".

> - binutils-gas-mips-fix-adjust-reloc.diff -- a fix for relocation handling 
>   problems discovered while fixing the issue with PC-relative relocations 
>   discussed earlier; a summary of the changes:
>
>   * BFD_RELOC_MICROMIPS_JALR relocs are now explicitly excluded like their
>     standard MIPS counterpart; this bug was covered by all microMIPS being 
>     converted to section-relative ones,
>
>   * unlike MIPS16 code we don't have call stubs in the microMIPS mode and 
>     therefore of the remaing relocs affecting microMIPS code only jump 
>     relocations against microMIPS text symbols on REL targets are 
>     converted to section-relative ones as the in-place relocatable field 
>     strips out the ISA bit,
>
>   * therefore we don't have to tag symbols that have a microMIPS jump 
>     relocation against them, because they're going to be handled just fine 
>     as long as the symbol is not a microMIPS text one,

Makes sense.  OK as long as you split this:

@@ -17189,7 +17187,9 @@ mips_fix_adjustable (fixS *fixp)
+	  || fixp->fx_r_type == BFD_RELOC_MIPS_JALR
+	  || fixp->fx_r_type == BFD_RELOC_MICROMIPS_JALR))

into a jalr_reloc_p, which should be defined alongside the existing
*_reloc_p functions.  Likewise:

+	      && (fixp->fx_r_type == BFD_RELOC_MIPS_JMP
+		  || fixp->fx_r_type == BFD_RELOC_MICROMIPS_JMP))))

and jmp_reloc_p.  (I think you already use this condition elsewhere,
so please change those too.)

> - binutils-umips-relax16.diff -- the original 16-bit->32-bit->out-of-range 
>   branch relaxation change, regenerated,

+      fragp->fr_var= length;

Missing space.

+      /* Handle 32-bit branches that fit or forced to fit.  */

"are forced to fit"

      /* Check the short-delay-slot bit.  */
      if (al && (insn & 0x02000000) != 0)
 	{
	  jal = 0x74000000;				/* jals  */
	  jalr = 0x45e0;				/* jalrs  */
	}

This is now quite far (and IMO confusingly far) from the code that sets
the default insns.  OK if you replace:

+      unsigned long jal = 0xf4000000;			/* jal  */
+      unsigned long jalr = 0x45c0;			/* jalr  */

with:

+      unsigned long jal, jalr;

and add:

      else
	{
	  jal = 0xf4000000;				/* jal  */
	  jalr = 0x45c0;				/* jalr  */
	}

to the condition above.

I think it'd be more consistent to set "jr" here too, but it's OK
either way.

> - binutils-umips-fix-reloc.diff -- the original microMIPS relocation 
>   handling divergence reduction change, regenerated,

+	      {
+		bfd_reloc_code_real_type reloc;
+		int shift;
+
+		reloc = micromips_map_reloc (orig_reloc);
+		shift = reloc == BFD_RELOC_MICROMIPS_JMP ? 1 : 2;
+		if ((address_expr->X_add_number & ((1 << shift) - 1)) != 0)
+		  as_bad (_("jump to misaligned address (0x%lx)"),
+			  (unsigned long) address_expr->X_add_number);
+		ip->insn_opcode |= ((address_expr->X_add_number >> shift)
+				    & 0x3ffffff);
+	      }

OK if you replace:

+		reloc = micromips_map_reloc (orig_reloc);
+		shift = reloc == BFD_RELOC_MICROMIPS_JMP ? 1 : 2;

with:

+		shift = mips_opts.micromips ? 1 : 2;

Same for BFD_RELOC_16_PCREL_S2.

+	  reloc = micromips_map_reloc (reloc_type[i - 1]);
+	  howto = bfd_reloc_type_lookup (stdoutput, reloc);
 	  if (howto == NULL)
 	    {
 	      /* To reproduce this failure try assembling gas/testsuites/
 		 gas/mips/mips16-intermix.s with a mips-ecoff targeted
 		 assembler.  */
-	      as_bad (_("Unsupported MIPS relocation number %d"), reloc_type[i - 1]);
+	      as_bad (_("Unsupported MIPS relocation number %d"), reloc);
 	      howto = bfd_reloc_type_lookup (stdoutput, BFD_RELOC_16);
 	    }
-	  
+
+	  reloc = micromips_map_reloc (orig_reloc);

In the usual case, this calls micromips_map_reloc twice for the same thing.
Seems better IMO to replace:

	  /* In a compound relocation, it is the final (outermost)
	     operator that determines the relocated field.  */
	  for (i = 1; i < 3; i++)
	    if (reloc_type[i] == BFD_RELOC_UNUSED)
	      break;

	  howto = bfd_reloc_type_lookup (stdoutput, reloc_type[i - 1]);

with:

	  bfd_reloc_code_real_type final_type[3];

	  /* Perform any necessary conversion to microMIPS relocations
	     and find out how many relocations there actually are.  */
	  for (i = 0; i < 3 && reloc_type[i] != BFD_RELOC_UNUSED; i++)
	    final_type[i] = micromips_map_reloc (reloc_type[i]);

	  /* In a compound relocation, it is the final (outermost)
	     operator that determines the relocated field.  */
	  howto = bfd_reloc_type_lookup (stdoutput, final_type[i - 1]);

Then use final_type instead of reloc_type.  OK with that change,
otherwise please let me know.

Richard