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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Part 3 of 3.

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.
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?

+#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.  That said, why split these up?  bfd_{get,put}_32
don't require aligned addresses.

+  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.

+  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.

-  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.

@@ -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?

@@ -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?

+    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.

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.

@@ -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.  */

@@ -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.  Use of 0xfffffffc isn't portable for 64-bit addresses;
use "-4" or "~(bfd_vma) 3" instead.

@@ -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).

@@ -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.

@@ -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.

@@ -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));

@@ -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.

@@ -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'll take your word for it that micromips-opc.c is correct. ;-)

+  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?  Also,
s/0x%02x/0x%x/ would be better IMO.

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.

The gas testsuite changes look very nice, thanks.

Richard