Re: RFC: Adding non-PIC executable support to MIPS

[Richard Sandiford <rdsandiford at googlemail dot com>]

Daniel Jacobowitz <dan@debian.org> writes:
> All comments welcome - Richard, especially from you.  How would you
> like to proceed?  I think the first step should be to get your other
> binutils/gcc patches merged, including MIPS16 PIC; I used those as a
> base.  But see a few of the notes for potential problems with those
> patches.

Yeah, Nick's approved most of the remaining binutils changes (thanks).
I haven't applied them yet because of the doubt over whether st_size
should be even or odd for ISA-encoded MIPS16 symbols.  I don't really
have an opinion, so I'll accept a maintainerly decision...

Anyway, the gcc patch looks good to me, thanks.  The only niggle
I could see was that you didn't update the comment for:

+/* True if the output file is marked as ".abicalls; .option pic0"
+   (-call_mixed).  This is a GNU extension.  */
+#define TARGET_ABICALLS_PIC0 \
+  (TARGET_ABSOLUTE_ABICALLS && TARGET_PLT)

(That kind of thing was inevitable given the amount of code you had
to wade through.  I'm impressed if there's really only one instance!)
I think the gcc side is good to go, modulo the _mcount thing.

As far as binutils goes, I saw a couple of potential problems:

(1) The patch adds the following code to
    _bfd_mips_elf_create_dynamic_sections:

+      if (htab->use_plts_and_copy_relocs && htab->root.hplt == NULL)
+	{
+	  h = _bfd_elf_define_linkage_sym (abfd, info, s,
+					   "_PROCEDURE_LINKAGE_TABLE_");
+	  htab->root.hplt = h;
+	  if (h == NULL)
+	    return FALSE;
+	  h->type = STT_FUNC;
+	}

    But use_plts_and_copy_relocs is only set after all input bfds have
    been read in.
    
(2) The patch sets pointer_equality_needed as follows:

@@ -7432,9 +7484,18 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
 		elf_hash_table (info)->dynobj = dynobj = abfd;
 	      break;
 	    }
-	  /* Fall through  */
+	  /* Fall through.  */
 
 	default:
+	  /* Most static relocations require pointer equality, except
+	     for branches.  */
+	  if (h)
+	    h->pointer_equality_needed = TRUE;
+	  /* Fall through.  */
+
+	case R_MIPS_26:
+	case R_MIPS_PC16:
+	case R_MIPS16_26:
 	  if (h)
 	    ((struct mips_elf_link_hash_entry *) h)->has_static_relocs = TRUE;
 	  break;

    But pointer equality is needed for non-call GOT relocations too.
    I couldn't see anything that explicitly handled that case.

    I think it would be more robust to set pointer_equality_needed in a
    separate block, rather than combining it with the existing switch
    statements.  It might then be clearer to set has_nonpic_branches
    in the new block too, so that you don't need two copies of:

	  if (h && !PIC_OBJECT_P (abfd))
	    ((struct mips_elf_link_hash_entry *) h)->has_nonpic_branches = TRUE;

Some minor nits too:

+  0x03990000,	/* l[wd] $25, %lo(&GOTPLT[0])($28)			*/
+  0x01d90000,	/* l[wd] $25, %lo(&GOTPLT[0])($14)			*/
+  0x01d90000,	/* l[wd] $25, %lo(&GOTPLT[0])($14)			*/

These are all fixed as either lw or ld.

@@ -1649,13 +1695,16 @@ mips_elf_check_symbols (struct mips_elf_
       /* H is a function that might need $25 to be valid on entry.
 	 If we're creating a non-PIC relocatable object, mark H as
 	 being PIC.  If we're creating a non-relocatable object with
-	 non-PIC references to H, make sure that H has an la25 stub.  */
+	 branches to H, make sure that H has an la25 stub.  Only
+	 use the stub for branches from non-PIC objects; GCC's
+	 -mno-shared uses branches from PIC objects to functions
+	 which do not require $25.  */
       if (hti->info->relocatable)
 	{
 	  if (!PIC_OBJECT_P (hti->output_bfd))
 	    h->root.other = ELF_ST_SET_MIPS_PIC (h->root.other);
 	}
-      else if (h->non_pic_ref && !mips_elf_add_la25_stub (hti->info, h))
+      else if (h->has_nonpic_branches && !mips_elf_add_la25_stub (hti->info, h))
 	{
 	  hti->error = TRUE;
 	  return FALSE;

How about something like the following:

-	 non-PIC references to H, make sure that H has an la25 stub.  */
+	 non-PIC branches and jumps to H, make sure that H has an la25 stub.
+	 We specifically ignore branches and jumps from EF_PIC objects,
+	 where the onus is on the compiler or programmer to perform any
+	 necessary initialization of $25.  Sometimes such initialization
+	 is unnecessary; for example, -mno-shared functions do not use
+	 the incoming value of $25, and may therefore be called directly.  */

(Wordsmith as necessary.)  The original wording made it sound like we'd
created a stub if there were any branches at all, but that the stub
would only be used for branches from non-PIC objects.

@@ -2928,6 +2977,7 @@ mips_elf_gotplt_index (struct bfd_link_i
   struct mips_elf_link_hash_table *htab;
 
   htab = mips_elf_hash_table (info);
+  BFD_ASSERT (htab->is_vxworks);
   BFD_ASSERT (h->plt.offset != (bfd_vma) -1);
 
   /* Calculate the index of the symbol's PLT entry.  */

I think this deserves a comment.  (It's because the .got.plt
address calculation is no longer accurate after the addition
of the reserved entries, right?)  I realise this function isn't
used for non-VxWorks before the patch, but it's a generic concept
even so...

@@ -4969,6 +5009,14 @@ mips_elf_calculate_relocation (bfd *abfd
       BFD_ASSERT (sec->size > 0);
       symbol = sec->output_section->vma + sec->output_offset;
     }
+  /* If this is a direct call to a PIC function, redirect to the
+     non-PIC stub.  */
+  else if (h != NULL && h->la25_stub && !PIC_OBJECT_P (input_bfd)
+	   && (r_type == R_MIPS_26 || r_type == R_MIPS_PC16
+	       || r_type == R_MIPS16_26))
+    symbol = (h->la25_stub->stub_section->output_section->vma
+	      + h->la25_stub->stub_section->output_offset
+	      + h->la25_stub->offset);
 
   /* Calls from 16-bit code to 32-bit code and vice versa require the
      special jalx instruction.  */

I'd prefer to see some part of:

   (h != NULL
    && !PIC_OBJECT_P (input_bfd)
    && (r_type == R_MIPS_26 || r_type == R_MIPS_PC16
        || r_type == R_MIPS16_26))

be a separate function that is used both here and in
_bfd_mips_elf_check_relocs.  At least the r_type check; I don't mind how
much else.  I just think the reloc code generally is tricky enough that
even small decisions like this are worth abstracting out.

-      htab->plt_header_size = 4 * ARRAY_SIZE (mips_exec_plt0_entry);
+      htab->plt_header_size = 4 * ARRAY_SIZE (mips_o32_exec_plt0_entry);

Probably worth a comment saying that all the plt headers are the same size.
It looked a bit odd using something with "o32" in it for n32 and n64.

	  if (h && !PIC_OBJECT_P (abfd))
	    ((struct mips_elf_link_hash_entry *) h)->has_nonpic_branches = TRUE;

+      /* Refuse some position-dependent relocations when creating a
+	 shared library.  Do not refuse R_MIPS_32 / R_MIPS_64; they're
+	 not PIC, but we can create dynamic relocations and the result
+	 will be fine.  Also do not refuse R_MIPS_LO16, which can be
+	 combined with R_MIPS_GOT16.  */
+      if (info->shared)
+	{
+	  switch (r_type)
+	    {
+	    case R_MIPS16_HI16:
+	    case R_MIPS_HI16:
+	    case R_MIPS_HIGHER:
+	    case R_MIPS_HIGHEST:
+	      /* Don't refuse a high part relocation if it's against
+		 no symbol (e.g. part of a compound relocation).  */
+	      if (r_symndx == 0)
+		break;
+
+	      /* R_MIPS_HI16 against _gp_disp is used for $gp setup,
+		 and has a special meaning.  */
+	      if (!NEWABI_P (abfd) && h != NULL
+		  && strcmp (h->root.root.string, "_gp_disp") == 0)
+		break;
+
+	      /* FALLTHROUGH */
+
+	    case R_MIPS16_26:
+	    case R_MIPS_26:
+	      howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, FALSE);
+	      (*_bfd_error_handler)
+		(_("%B: relocation %s against `%s' can not be used when making a shared object; recompile with -fPIC"),
+		 abfd, howto->name,
+		 (h) ? h->root.root.string : "a local symbol");
+	      bfd_set_error (bfd_error_bad_value);
+	      return FALSE;
+	    default:
+	      break;
+	    }
+	}

FWIW, I thought about adding this too, but rejected it because I was
afraid of trapping valid uses.  E.g. it makes it impossible to use
relocations against SHN_ABS symbols, even though such symbols aren't
(generally) subject to runtime relocation.  I realise you copied this
from other ports though.

@@ -8430,10 +8540,16 @@ _bfd_mips_elf_size_dynamic_sections (bfd
       else if (SGI_COMPAT (output_bfd)
 	       && CONST_STRNEQ (name, ".compact_rel"))
 	s->size += mips_elf_hash_table (info)->compact_rel_size;
+      else if (s == htab->splt)
+	{
+	  /* If the last PLT entry has a branch delay slot, allocate
+	     room for an extra nop to fill the delay slot.  */
+	  if (!htab->is_vxworks && !info->shared && s->size > 0)
+	    s->size += 4;
+	}
       else if (! CONST_STRNEQ (name, ".init")
 	       && s != htab->sgot
 	       && s != htab->sgotplt
-	       && s != htab->splt
 	       && s != htab->sstubs
 	       && s != htab->sdynbss)
 	{

Why !info->shared?

@@ -9604,17 +9727,21 @@ mips_finish_exec_plt (bfd *output_bfd, s
   gotplt_value_high = ((gotplt_value + 0x8000) >> 16) & 0xffff;
   gotplt_value_low = gotplt_value & 0xffff;
 
+  /* The PLT sequence is not safe for N64 if .got.plt is above the 2GB
+     mark.  */
+  BFD_ASSERT ((gotplt_value >> 31) == 0);
+
   /* Pick the load opcode.  */
   load = MIPS_ELF_LOAD_WORD (output_bfd);
 
   /* Install the PLT header.  */
   loc = htab->splt->contents;
-  bfd_put_32 (output_bfd, plt_entry[0] | got_value_high, loc);
-  bfd_put_32 (output_bfd, plt_entry[1] | got_value_low | load, loc + 4);
-  bfd_put_32 (output_bfd, plt_entry[2] | got_value_low, loc + 8);
-  bfd_put_32 (output_bfd, plt_entry[3] | gotplt_value_high, loc + 12);
+  bfd_put_32 (output_bfd, plt_entry[0] | gotplt_value_high, loc);
+  bfd_put_32 (output_bfd, plt_entry[1] | gotplt_value_low | load, loc + 4);
+  bfd_put_32 (output_bfd, plt_entry[2] | gotplt_value_low, loc + 8);
+  bfd_put_32 (output_bfd, plt_entry[3], loc + 12);
   bfd_put_32 (output_bfd, plt_entry[4], loc + 16);
-  bfd_put_32 (output_bfd, plt_entry[5] | gotplt_value_low, loc + 20);
+  bfd_put_32 (output_bfd, plt_entry[5], loc + 20);
   bfd_put_32 (output_bfd, plt_entry[6], loc + 24);
   bfd_put_32 (output_bfd, plt_entry[7], loc + 28);
 }

I think it'd be good to tighten the assert, as it would currently trigger
for valid X >= -0x80000000 addresses (both in 32-bit and 64-bit code).
I don't know of any system out there that allows dynamic executables
in kernel space, but I've ceased to be surprised by such things ;)

+/* Return TRUE if symbol should be hashed in the `.gnu.hash' section.  */
+
+bfd_boolean
+_bfd_mips_elf_hash_symbol (struct elf_link_hash_entry *h)
+{
+  struct mips_elf_link_hash_entry *hmips;
+
+  hmips = (struct mips_elf_link_hash_entry *) h;
+  if (h->plt.offset != MINUS_ONE
+      && hmips->no_fn_stub
+      && !h->def_regular
+      && !h->pointer_equality_needed)
+    return FALSE;
+
+  return _bfd_elf_hash_symbol (h);
+}

Were you able to test this, given:

static void
mips_after_parse (void)
{
  /* .gnu.hash and the MIPS ABI require .dynsym to be sorted in different
     ways.  .gnu.hash needs symbols to be grouped by hash code whereas the
     MIPS ABI requires a mapping between the GOT and the symbol table.  */
  if (link_info.emit_gnu_hash)
    {
      einfo ("%X%P: .gnu.hash is incompatible with the MIPS ABI\n");
      link_info.emit_hash = TRUE;
      link_info.emit_gnu_hash = FALSE;
    }
  after_parse_default ();
}

?  FWIW, if it's dead code, I'd prefer to leave it out.

+      printf (_(" Entries:\n"));
+      printf (_("  %*s %*s %*s %-7s %3s %s\n"),
+	      addr_size * 2, "Address",
+	      addr_size * 2, "Initial",
+	      addr_size * 2, "Sym.Val.", "Type", "Ndx", "Name");
+      sym_width = (is_32bit_elf ? 80 : 160) - 28 - addr_size * 6 - 1;

"- 17" rather than "- 28"?  In the global GOT code, "28 - addr_size * 6"
is the width of the format before the final "%s".  However, the global GOT
entries have an additional 10-char field and space separator, so the width
there is 11 greater than here.

Index: binutils-mainline/ld/testsuite/ld-mips-elf/pic-and-nonpic-3b.dd
===================================================================
--- binutils-mainline.orig/ld/testsuite/ld-mips-elf/pic-and-nonpic-3b.dd	2008-07-16 09:25:17.000000000 -0700
+++ binutils-mainline/ld/testsuite/ld-mips-elf/pic-and-nonpic-3b.dd	2008-07-22 10:43:08.000000000 -0700
@@ -2,50 +2,52 @@
 #
 # -32752: lazy resolution function
 # -32748: reserved for module pointer
-# -32744: PLT resolution function
-# -32740: GOT page entry.
-# -32736: bar's GOT entry
+# -32744: GOT page entry.
+# -32740: bar's GOT entry
 
 .*
 
 Disassembly of section \.plt:
 
-00043010 <.*>:
-   43010:	3c0f000a 	lui	t7,0xa
-   43014:	8df90008 	lw	t9,8\(t7\)
-   43018:	25ef0008 	addiu	t7,t7,8
-   4301c:	3c0e0008 	lui	t6,0x8
-   43020:	03200008 	jr	t9
-   43024:	25ce1000 	addiu	t6,t6,4096
-	\.\.\.
+.* <.*>:
+.*:	3c1c0008 	lui	gp,0x8
+.*:	8f991000 	lw	t9,4096\(gp\)
+.*:	279c1000 	addiu	gp,gp,4096
+.*:	031cc023 	subu	t8,t8,gp
+.*:	03e07821 	move	t7,ra
+.*:	0018c082 	srl	t8,t8,0x2
+.*:	0320f809 	jalr	t9
+.*:	2718fffe 	addiu	t8,t8,-2
+
+.* <foo@plt>:
+.*:	3c0f0008 	lui	t7,0x8
+.*:	8df91008 	lw	t9,4104\(t7\)
+.*:	25f81008 	addiu	t8,t7,4104
+.*:	03200008 	jr	t9
+.*:	00000000 	nop
 
-00043030 <foo@plt>:
-   43030:	3c180008 	lui	t8,0x8
-   43034:	8f191000 	lw	t9,4096\(t8\)
-   43038:	03200008 	jr	t9
-   4303c:	27181000 	addiu	t8,t8,4096
 Disassembly of section \.text:
 
-00044000 <__start>:
-   44000:	0c010c0c 	jal	43030 <foo@plt>
-   44004:	00000000 	nop
-   44008:	08011004 	j	44010 <ext>
-   4400c:	00000000 	nop
-
-00044010 <ext>:
-   44010:	3c1c000a 	lui	gp,0xa
-   44014:	279c7ff0 	addiu	gp,gp,32752
-   44018:	8f82801c 	lw	v0,-32740\(gp\)
-   4401c:	24421000 	addiu	v0,v0,4096
-   44020:	8f998020 	lw	t9,-32736\(gp\)
-   44024:	03200008 	jr	t9
-   44028:	00000000 	nop
-   4402c:	00000000 	nop
+.* <__start>:
+.*:	0c010c10 	jal	43040 <foo@plt>
+.*:	00000000 	nop
+.*:	08011004 	j	44010 <ext>
+.*:	00000000 	nop
+
+.* <ext>:
+.*:	3c1c000a 	lui	gp,0xa
+.*:	279c7ff0 	addiu	gp,gp,32752
+.*:	8f828018 	lw	v0,-32744\(gp\)
+.*:	24421000 	addiu	v0,v0,4096
+.*:	8f99801c 	lw	t9,-32740\(gp\)
+.*:	03200008 	jr	t9
+.*:	00000000 	nop
+.*:	00000000 	nop
 Disassembly of section .MIPS.stubs:
 
-00044030 <\.MIPS\.stubs>:
-   44030:	8f998010 	lw	t9,-32752\(gp\)
-   44034:	03e07821 	move	t7,ra
-   44038:	0320f809 	jalr	t9
-   4403c:	24180007 	li	t8,7
+.* <\.MIPS\.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	0320f809 	jalr	t9
+.*:	24180007 	li	t8,7
 	\.\.\.

Please keep the "... <...>:" addresses at least.  (You did this
in some of the other tests, thanks.)  The non-disassembly dumps
rely on functions being at certain addresses, and I think it's
easier to follow the test if those addresses are explicit in
the disassembly.

(I've used custom scripts for this sort of test to try to
protect them from fluctuations in the size of the dynamic info.
The addresses should be pretty stable.)

Richard