RFC: Adding non-PIC executable support to MIPS

[Daniel Jacobowitz]

Thanks for the review.  I have just a few questions left for you
(search for "?" :-).  As this is 99% about the binutils changes, I've
trimmed the CC list a bit.

On Sun, Jul 27, 2008 at 10:10:23AM +0100, Richard Sandiford wrote:
> 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...

In that case, I suggest we go with even sizes.

> 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!)

Yeah, thanks :-)  I spent a merry morning adjusting the comments in
binutils, but wasn't as thorough for GCC.  Fixed.

> I think the gcc side is good to go, modulo the _mcount thing.

Ok.  Want me to find a fix, or will you?

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

Whoops.  I've had recurring problems with this - there were several
other things our previous patch did based on the first time a pic0
input was seen and this was one of them.  I had in my notes that
the _P_L_T_ symbol was going to be tricky to get right and I just just
drop it.  But obviously I didn't.

I'll define the symbol later.  This is straightforward, as nothing (in
non-VxWorks anyway) refers to the symbol.

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

I'm not sure about this.  While pointer equality, the concept, is
needed, I don't think it mandates a valid address on the PLT entry
(which is what this flag controls).  Won't the dynamic linker do the
right thing?

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

Right, it turned out I needed three separate sequences fairly late in
the patch's life cycle.  Fixed.

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

I like your version, thanks.  However, I moved most of it to the new
function mips_elf_relocation_needs_la25_stub rather than the use here.

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

Right.  I added a comment:

  /* This function only works for VxWorks, because a non-VxWorks .got.plt
     section starts with reserved entries.  */

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

Changed.  I moved the input_bfd check too; that seemed like the most
sensible logical unit.

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

Yeah, good idea.

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

Right.  I'd prefer to leave it - it did find a number of useful
problems in the course of our development.  If someone bumps into it
with a legitimate use case I'm happy to relax it further.

> Why !info->shared?

No particular reason; as you know it makes no difference in the
current code, and I don't know what shared PLTs might look like yet.
I removed it, since that's safer than not.

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

OK.  Could you check that I got this right?

  /* The PLT sequence is not safe for N64 if .got.plt's address can
     not be loaded in two instructions.  */
  BFD_ASSERT ((gotplt_value & ~(bfd_vma) 0x7fffffff) == 0
              || ~(gotplt_value | 0x7fffffff) == 0);

> +/* 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)

> Were you able to test this

No.  I figured I'd add this, since it has a slightly non-obvious bit,
in case someone comes up with a clever way to use .gnu.hash on MIPS
(with an entirely non-PIC binary, it should be quite feasible, but
it'd be an ABI change).  I'm happy to drop it though, so I've done so.

> "- 17" rather than "- 28"?

Yes, you're right.

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

They're a pain to update when something does change; I spent most of
an entire day fixing hardcoded addresses in the test scripts, and like
Ian, I'm starting to wonder about the value.

I put the addresses back for symbols, which sounds like a fine
compromise.

-- 
Daniel Jacobowitz
CodeSourcery