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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Part 2 of the review.

This testcase:

	.set	micromips
	.fill	0x80
	b16	.-0x80

produces:

/tmp/foo.s: Assembler messages:
/tmp/foo.s:3: Error: unrecognized opcode `b16 .-0x80'

while:

	.set	micromips
	.fill	0x80
	b	.-0x80

successfully produces a 16-bit insn.

@@ -14813,6 +16230,8 @@ mips_elf_final_processing (void)
      file_ase_mt is true.  */
   if (file_ase_mips16)
     elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_M16;
+  if (file_ase_micromips)
+    elf_elfheader (stdoutput)->e_flags |= EF_MIPS_ARCH_ASE_MICROMIPS;
 #if 0 /* XXX FIXME */
   if (file_ase_mips3d)
     elf_elfheader (stdoutput)->e_flags |= ???;

Do you really only want this flag to be set if -mmicromips was passed
on the command line?  (Yes, the same concern applies to MIPS16 and MDMX.)

Lots of cases where a space is missing before "(".

Please be consistent about "str[n]cmp (...) == 0" vs '!str[n]cmp (...)':
use one or the other.  (IMO the first is clearer.)

micromips_ip obviously started life as a cut-&-paste of mips_ip, and it
would have been nice to factor some code out.  At least split out the
block beginning:

+	    case 'F':
+	    case 'L':
+	    case 'f':
+	    case 'l':
+	      {

which is identical between the two, and far too subtle to copy wholesale.
There may be other good opportunities too.

+	    case '(':
+	      /* Handle optional base register.
+		 Either the base register is omitted or
+		 we must have a left paren.  */
+	      /* This is dependent on the next operand specifier
+		 is a base register specification.  */
+	      gas_assert (args[1] == 'b'
+		      || (args[1] == 'm'
+			  && (args[2] == 'l' || args[2] == 'n'
+			      || args[2] == 's' || args[2] == 'a')));
+	      if (*s == '\0' && args[1] == 'b')
+		return;
+
+	    case ')':		/* these must match exactly */
+	    case '[':
+	    case ']':
+	      if (*s++ == *args)
+		continue;
+	      break;

Mark fallthrough.

+	    case 'D':		/* floating point destination register */
+	    case 'S':		/* floating point source register */
+	    case 'T':		/* floating point target register */
+	    case 'R':		/* floating point source register */
+	    case 'V':
+	      rtype = RTYPE_FPU;
+	      s_reset = s;
+	      if (reg_lookup (&s, rtype, &regno))
+		{
+		  if ((regno & 1) != 0
+		      && HAVE_32BIT_FPRS
+		      && ! mips_oddfpreg_ok (ip->insn_mo, argnum))
+		    as_warn (_("Float register should be even, was %d"),
+			     regno);
+
+		  c = *args;
+		  if (*s == ' ')
+		    ++s;
+		  if (args[1] != *s)
+		    {
+		      if (c == 'V' || c == 'W')
+			{
+			  regno = lastregno;
+			  s = s_reset;
+			  ++args;
+			}
+		    }
+		  switch (c)
+		    {
+		    case 'D':
+		      MICROMIPS_INSERT_OPERAND (FD, *ip, regno);
+		      break;
+		    case 'V':
+		    case 'S':
+		      MICROMIPS_INSERT_OPERAND (FS, *ip, regno);
+		      break;
+
+		    case 'T':
+		      MICROMIPS_INSERT_OPERAND (FT, *ip, regno);
+		      break;
+
+		    case 'R':
+		      MICROMIPS_INSERT_OPERAND (FR, *ip, regno);
+		      break;
+		    }
+		  lastregno = regno;
+		  continue;
+		}
+
+	      switch (*args++)
+		{
+		case 'V':
+		  MICROMIPS_INSERT_OPERAND (FS, *ip, lastregno);
+		  continue;
+		case 'W':
+		  MICROMIPS_INSERT_OPERAND (FT, *ip, lastregno);
+		  continue;
+		}
+	      break;

This block doesn't have a 'W' case (which doesn't seem to be used
for micromips at all), so all the 'W' handling is dead code.

+			    case  4:
+			    case  5:
+			    case  6:

Too many spaces.

+      if (insn + 1 < &micromips_opcodes[bfd_micromips_num_opcodes] &&
+	  !strcmp (insn->name, insn[1].name))

Misplaced "&&".

+    = {BFD_RELOC_UNUSED, BFD_RELOC_UNUSED, BFD_RELOC_UNUSED};;

Double ";".

+	  macro_read_relocs (&args, r);

Not portable in this context (and doesn't build on x86_64-linux-gnu).
You're taking the address of a va_list argument rather than a va_list
local variable.

+	  /*  For microMIPS, we always use relocations for branches.
+	      So, we should not resolve immediate values.  */

Too many spaces.

+	  if (ep->X_op == O_constant)
+	    abort ();
+	  else
+	    *r = BFD_RELOC_MICROMIPS_16_PCREL_S1;

gcc_assert (ep->X_op != O_constant);
*r = BFD_RELOC_MICROMIPS_16_PCREL_S1;

The same cut-&-paste concerns apply to micromips_macro, which obviously
started out as a copy of macro().  I realise there are special cases
for micromips (such as the DADDI assymmetry and the lack of
branch-likely instructions, to name only a few), but most of the
basic decisions are the same.

As it stands, we have a new 3524 line function, of which I imagine at
least 90% is shared with macro().  I really think the new microMIPS
macro handling should be integrated into macro() instead.  Don't be
afraid of factoring out code from macro() if it makes it easier to
integrate the microMIPS code.

 #define CPU_MIPS5       5
 #define CPU_MIPS64      64
 #define CPU_MIPS64R2	65
+#define CPU_MICROMIPS	96
 #define CPU_SB1         12310201        /* octal 'SB', 01.  */
 #define CPU_LOONGSON_2E 3001
 #define CPU_LOONGSON_2F 3002

What's this for?  It doesn't seem to be used.

+   "mA" 7-bit immediate (-63 .. 64) << 2 (MICROMIPSOP_*_IMMA)

Don't you mean (-64 .. 63)?

+   "mB" 3-bit immediate (0, -1, 1, 4, 8, 12, 16, 20, 24) (MICROMIPSOP_*_IMMB)

That's nine values.  Should the 0 really be there?

@@ -630,15 +630,15 @@ proc strip_executable { prog flags test 
 	remote_upload host ${copyfile} tmpdir/striprog
     }
 
-    set result [remote_load target tmpdir/striprog]
-    set status [lindex $result 0]
-    if { ![istarget $host_triplet] } {
-      set status "pass"
-    }
-    if { $status != "pass" } {
-	fail $test
-        return
-    }
+#    set result [remote_load target tmpdir/striprog]
+#    set status [lindex $result 0]
+#    if { ![istarget $host_triplet] } {
+#      set status "pass"
+#    }
+#    if { $status != "pass" } {
+#	fail $test
+#        return
+#    }
 
     set exec_output [binutils_run $NM "$NMFLAGS ${copyfile}"]
     if ![string match "*: no symbols*" $exec_output] {
@@ -673,15 +673,15 @@ proc strip_executable_with_saving_a_symb
 	remote_upload host ${copyfile} tmpdir/striprog
     }
 
-    set result [remote_load target tmpdir/striprog]
-    set status [lindex $result 0]
-    if { ![istarget $host_triplet] } {
-      set status "pass"
-    }
-    if { $status != "pass" } {
-	fail $test
-        return
-    }
+#    set result [remote_load target tmpdir/striprog]
+#    set status [lindex $result 0]
+#    if { ![istarget $host_triplet] } {
+#      set status "pass"
+#    }
+#    if { $status != "pass" } {
+#	fail $test
+#        return
+#    }
 
     set exec_output [binutils_run $NM "$NMFLAGS ${copyfile}"]
     if { [istarget mmix-knuth-mmixware] } {

Looks like these crept in unawares.

    PLT entries and traditional MIPS lazy binding stubs.  We mark the former
    with STO_MIPS_PLT to distinguish them from the latter.  */
 #define STO_MIPS_PLT		0x8
+#define ELF_ST_IS_MIPS_PLT(OTHER) (((OTHER) & 0x8) == STO_MIPS_PLT)
...
 #define STO_MIPS_PIC		0x20
 #define ELF_ST_IS_MIPS_PIC(OTHER) \
-  (((OTHER) & ~ELF_ST_VISIBILITY (-1)) == STO_MIPS_PIC)
+  (((OTHER) & ~ELF_ST_VISIBILITY (-1) & ~0xc0) == STO_MIPS_PIC)
 #define ELF_ST_SET_MIPS_PIC(OTHER) \
-  (STO_MIPS_PIC | ELF_ST_VISIBILITY (OTHER))
+  (STO_MIPS_PIC | ELF_ST_VISIBILITY (OTHER) | ELF_ST_MICROMIPS (OTHER))
...
     case STO_OPTIONAL:  return "OPTIONAL";
     case STO_MIPS16:    return "MIPS16";
-    case STO_MIPS_PLT:	return "MIPS PLT";
-    case STO_MIPS_PIC:	return "MIPS PIC";
-    default:      	return NULL;
+    default:
+	if (ELF_ST_IS_MIPS_PLT (other))
+	  {
+	    if (ELF_ST_IS_MICROMIPS (other))
+	      return "MICROMIPS, MIPS PLT";
+	    else
+	      return "MIPS PLT";
+	  }
+	if (ELF_ST_IS_MIPS_PIC (other))
+	  {
+	    if (ELF_ST_IS_MICROMIPS (other))
+	      return "MICROMIPS, MIPS PIC";
+	    else
+	      return "MIPS PIC";
+	  }
+	if (ELF_ST_IS_MICROMIPS (other))
+	  return "MICROMIPS";
+
+	return NULL;

You don't add support for microMIPS PLTs, so the "MICROMIPS, MIPS PLT"
thing appears to be dead code.  I wouldn't mind, except that it makes
the code inconsistent with MIPS16: the changes above allow both

  STO_MIPS16 | STO_MIPS_PLT

and

  STO_MICROMIPS | STO_MIPS_PLT

neither of which are currently used, but you don't treat the two equally.

In other words, I'm happy with the STO_MIPS_PIC changes but not with
the STO_MIPS_PLT ones.

+  /* 16 bit relocation.  */
+  HOWTO (R_MICROMIPS_16,	/* type */
+	 0,			/* rightshift */
+	 2,			/* size (0 = byte, 1 = short, 2 = long) */
+	 16,			/* bitsize */
+	 FALSE,			/* pc_relative */
+	 0,			/* bitpos */
+	 complain_overflow_dont, /* complain_on_overflow */
+	 _bfd_mips_elf_lo16_reloc, /* special_function */
+	 "R_MICROMIPS_16",	/* name */
+	 TRUE,			/* partial_inplace */
+	 0x0000ffff,		/* src_mask */
+	 0x0000ffff,		/* dst_mask */
+	 FALSE),		/* pcrel_offset */

Is this relocation really a lo16 one?  If so, it's not consistent
with R_MIPS_16 (which isn't).

+/*
+ *  Delay slot size and relaxation support
+ */
+static unsigned long
+relax_delay_slot (bfd *abfd, bfd_byte *ptr, unsigned long* opcode_32)
+{
+  unsigned long bdsize = 0;
+
+  unsigned long fndopc;
+  unsigned long p16opc, p32opc;
+
+  /* Opcodes preceding the current instruction.  */
+  p16opc  = bfd_get_16 (abfd, ptr - 2);
+  p32opc  = bfd_get_16 (abfd, ptr - 4) << 16;

You need to check the section bounds.  The code appears to read off the
beginning of a section if that section starts with a relaxable LUI.

+      /* If this isn't something that can be relaxed, then ignore
+	 this reloc.  */
+      if (ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_HI16 &&
+	  ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_LO16 &&
+	  ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_PC16_S1 &&
+	  ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_26_S1 &&
+	  ELF32_R_TYPE (irel->r_info) != (int) R_MICROMIPS_GPREL16)
+	continue;

&&s at the beginning of lines.

+      /* Get the value of the symbol referred to by the reloc.  */
+      if (ELF32_R_SYM (irel->r_info) < symtab_hdr->sh_info)
+	{
+	  /* A local symbol.  */
+	  Elf_Internal_Sym *isym;
+	  asection *sym_sec;
+
+	  isym = isymbuf + ELF32_R_SYM (irel->r_info);
+	  if (isym->st_shndx == SHN_UNDEF)
+	    sym_sec = bfd_und_section_ptr;
+	  else if (isym->st_shndx == SHN_ABS)
+	    sym_sec = bfd_abs_section_ptr;
+	  else if (isym->st_shndx == SHN_COMMON)
+	    sym_sec = bfd_com_section_ptr;
+	  else
+	    sym_sec = bfd_section_from_elf_index (abfd, isym->st_shndx);
+	  symval = (isym->st_value
+		    + sym_sec->output_section->vma
+		    + sym_sec->output_offset);
+	  target_is_micromips_code_p = ELF_ST_IS_MICROMIPS (isym->st_other);
+	}
+      else
+	{
+	  unsigned long indx;
+	  struct elf_link_hash_entry *h;
+
+	  /* An external symbol.  */
+	  indx = ELF32_R_SYM (irel->r_info) - symtab_hdr->sh_info;
+	  h = elf_sym_hashes (abfd)[indx];
+	  BFD_ASSERT (h != NULL);
+
+	  if (h->root.type != bfd_link_hash_defined
+	      && h->root.type != bfd_link_hash_defweak)
+	    /* This appears to be a reference to an undefined
+	       symbol.  Just ignore it -- it will be caught by the
+	       regular reloc processing.  */
+	    continue;
+
+	  symval = (h->root.u.def.value
+		    + h->root.u.def.section->output_section->vma
+		    + h->root.u.def.section->output_offset);
+	}

Why not set target_is_micromips_code_p for locally-binding global
symbols too?

+      opcode  =   bfd_get_16 (abfd, contents + irel->r_offset    ) << 16;
+      opcode |= (irel->r_offset + 2 < sec->size
+		 ? bfd_get_16 (abfd, contents + irel->r_offset + 2) : 0);

Are there any relaxations we can actually do if irel->r_offset + 2
>= sec->size?  I couldn't see any.  If there aren't, continue instead.

+      /* R_MICROMIPS_HI16 / LUI relaxation to R_MICROMIPS_HI0_LO16 or
+         R_MICROMIPS_PC23_S2.  The R_MICROMIPS_PC23_S2 condition is
+
+           (symval % 4 == 0 && IS_BITSIZE (pcrval, X, 25))
+
+         where the X adjustment compensate for R_MICROMIPS_HI16 and
+         R_MICROMIPS_LO16 being at most X bytes appart when the
+         distance to the target approaches 32 MB.  */

Comment is slightly confusing: we don't relax the HI16 itself to a
H0_LO16 or PC32_S3.  Rather we relax the LO16 (or, if you like,
the HI16/LO16 pair).

+	  /* Assume is possible to delete the LUI instruction:
+	     4 bytes at irel->r_offset.  */

s/Assume is/Assume it is/

+      /* Compact branch relaxation -- due to the multitude of macros
+         employed by the compiler/assembler, compact branches are not
+         aways generated.  Obviously, this can/will be fixed elsewhere,
+         but there is no drawback in double checking it here.  */
+      else if (ELF32_R_TYPE (irel->r_info) == (int) R_MICROMIPS_PC16_S1
+	       && (fndopc = find_match (opcode, bz_insns_32)) != 0
+	       && MATCH (bfd_get_16 (abfd, contents + irel->r_offset + 4),
+			 nop_insn_16))

s/aways/always/.  You should check the section size before reading
past the relocation.  (I realise there ought to be a delay slot,
but we should be robust against brokenness.)

+      /* R_MICROMIPS_26_S1 -- JAL to JALS relaxation for microMIPS targets.  */
+      else if (ELF32_R_TYPE (irel->r_info) == (int) R_MICROMIPS_26_S1
+	       && target_is_micromips_code_p
+	       && MATCH (opcode, jal_insn_32_bd32))
+	{
+	  unsigned long n32opc;
+	  bfd_boolean relaxed = FALSE;
+
+	  n32opc  =
+	    bfd_get_16 (abfd, contents + irel->r_offset + 4    ) << 16;
+	  n32opc |= irel->r_offset + 2 < sec->size?
+	    bfd_get_16 (abfd, contents + irel->r_offset + 6): 0;
+

Here too you should check the size before reading n32opc.  Second
condition looks bogus (did you mean +6?) although the same concerns
apply as for the "+ 2" above.

I'll try to review more soon.

Richard