[PATCH] Fixing MIPS16 vs LA25 stubs

[PATCH] Fixing MIPS16 vs LA25 stubs

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 4030 bytes --]

Hi Richard,
The MIPS16 code has some complicated relations with LA25 stubs
(LUI/ADDUI $25 sequences added by BFD when calling from non-PIC to PIC).

Current PLT support is capable of adding (either prepending or separate
stub) LA25 sequences to create $t9 on entry of a PIC callee. However,
when the called routine is MIPS16 code with a FP-stub (stub is 32-bit),
then we have, for example:

int main (void)
{
  return __fixdfdi (0.0);  // libgcc function
}

Build as:
mips-linux-gnu-gcc -c -O2 testcase.c    # build as 32-bit
mips-linux-gnu-gcc -mips16 testcase.o   # link with MIPS16 libs
./a.out
Segmentation fault

Disassembly:
00400930 <__fn_stub___fixdfdi>:
  400930:       3c1c0002        lui     gp,0x2
  400934:       279c8100        addiu   gp,gp,-32512
  400938:       0399e021        addu    gp,gp,t9
  40093c:       8f998024        lw      t9,-32732(gp)  <--segfault
  400940:       27390621        addiu   t9,t9,1569
  400944:       44056000        mfc1    a1,$f12
  400948:       03200008        jr      t9
  40094c:       44046800        mfc1    a0,$f13

The call is from main:
00400610 <main>:
  400610:       44806000        mtc1    zero,$f12
  400614:       0810024c        j       400930 <__fn_stub___fixdfdi>
  400618:       44806800        mtc1    zero,$f13
  40061c:       00000000        nop

The actual __fixdfdi body, which is MIPS16 code:
00400620 <__fixdfdi>:
  400620:       f000 6a02       li      v0,2
  400624:       f410 0b0c       la      v1,3f8a30 <_DYNAMIC-0x774c>
  400628:       f400 3240       sll     v0,16
  40062c:       e269            addu    v0,v1
  40062e:       659a            move    gp,v0
  400630:       64f6            save    48,ra,s0-s1
  ...

The problem is that 'la $25' headers/trampolines are needed for the
"stub", which is 32-bit code and calculates $gp using $t9, not the
MIPS16 function itself (which as the BFD support already correctly
determines, are never needed due to MIPS16's PC-relative capabilities).
Also note that this problem is avoided when -mno-plt is used.


My patch attached here modifies the LA25 insertion to accommodate the
case where we have a "MIPS16 function with a kept 32-bit stub", and uses
the stub as the point of insertion. The correctly linked results are now
like:
00400610 <main>:
  400610:       44806000        mtc1    zero,$f12
  400614:       0810024c        j       400930 <.pic.__fixdfdi>
  400618:       44806800        mtc1    zero,$f13
  40061c:       00000000        nop
...
00400930 <.pic.__fixdfdi>:
  400930:       3c190040        lui     t9,0x40
  400934:       27390938        addiu   t9,t9,2360

00400938 <__fn_stub___fixdfdi>:
  400938:       3c1c0002        lui     gp,0x2
  40093c:       279c80f8        addiu   gp,gp,-32520
  400940:       0399e021        addu    gp,gp,t9
  400944:       8f998024        lw      t9,-32732(gp)
  400948:       27390621        addiu   t9,t9,1569
  40094c:       44056000        mfc1    a1,$f12
  400950:       03200008        jr      t9
  400954:       44046800        mfc1    a0,$f13

Which should be correctly generated "stub of stub" code :)  This is for
the 32-bit->16-bit case. There's also some handling to skip the LA25
stub for 16-bit->16-bit case.

Thanks,
Chung-Lin

2011-12-10  Chung-Lin Tang  <cltang@codesourcery.com>
	    Catherine Moore  <clm@codesourcery.com>
	    Sandra Loosemore  <sandra@codesourcery.com>

        * elfxx-mips.c (mips_elf_local_pic_function_p): Return true when
        H is a MIPS16 function with a kept 32-bit stub. Update comments.
        (mips_elf_add_la25_intro): Use MIPS16 stub section as place of
        insertion when it exists.
        (mips_elf_add_la25_stub): Always use LUI/ADDIU prepending for
        MIPS16 stubs. Update comments.
        (mips_elf_calculate_relocation): Redirect relocation to point to
        the LA25 stub if it exists, instead of the MIPS16 stub. Don't
	use la25 stub for mips16->mips16 calls.
        (mips_elf_create_la25_stub): Change $25 target calculation to
        base on MIPS16 stub if it exists.




[-- Attachment #2: la25.diff --]
[-- Type: text/plain, Size: 4572 bytes --]

Index: elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.297
diff -u -p -r1.297 elfxx-mips.c
--- elfxx-mips.c	8 Dec 2011 20:47:24 -0000	1.297
+++ elfxx-mips.c	10 Dec 2011 09:23:38 -0000
@@ -1573,7 +1573,9 @@ _bfd_mips_elf_init_stubs (struct bfd_lin
 /* Return true if H is a locally-defined PIC function, in the sense
    that it might need $25 to be valid on entry.  Note that MIPS16
    functions never need $25 to be valid on entry; they set up $gp
-   using PC-relative instructions instead.  */
+   using PC-relative instructions instead.  We do process the case
+   where a MIPS16 function has a 32-bit stub, where the $25 load
+   is added for the 32-bit stub itself, not the 16-bit function.  */
 
 static bfd_boolean
 mips_elf_local_pic_function_p (struct mips_elf_link_hash_entry *h)
@@ -1582,7 +1584,8 @@ mips_elf_local_pic_function_p (struct mi
 	   || h->root.root.type == bfd_link_hash_defweak)
 	  && h->root.def_regular
 	  && !bfd_is_abs_section (h->root.root.u.def.section)
-	  && !ELF_ST_IS_MIPS16 (h->root.other)
+	  && (!ELF_ST_IS_MIPS16 (h->root.other)
+	      || (h->fn_stub && h->need_fn_stub))
 	  && (PIC_OBJECT_P (h->root.root.u.def.section->owner)
 	      || ELF_ST_IS_MIPS_PIC (h->root.other)));
 }
@@ -1610,8 +1613,10 @@ mips_elf_add_la25_intro (struct mips_elf
     return FALSE;
   sprintf (name, ".text.stub.%d", (int) htab_elements (htab->la25_stubs));
 
-  /* Create the section.  */
-  input_section = stub->h->root.root.u.def.section;
+  /* Create the section.  If H is a MIPS16 function with a stub, use the
+     stub function section.  */
+  input_section = (stub->h->fn_stub
+		   ? stub->h->fn_stub : stub->h->root.root.u.def.section);
   s = htab->add_stub_section (name, input_section,
 			      input_section->output_section);
   if (s == NULL)
@@ -1686,10 +1691,12 @@ mips_elf_add_la25_stub (struct bfd_link_
   void **slot;
 
   /* Prefer to use LUI/ADDIU stubs if the function is at the beginning
-     of the section and if we would need no more than 2 nops.  */
+     of the section and if we would need no more than 2 nops.  Also
+     when processing a 32-bit stub for a MIPS16 function, we can always
+     preprend before the stub section.  */
   s = h->root.root.u.def.section;
   value = h->root.root.u.def.value;
-  use_trampoline_p = (value != 0 || s->alignment_power > 4);
+  use_trampoline_p = (value != 0 || s->alignment_power > 4) && !h->fn_stub;
 
   /* Describe the stub we want.  */
   search.stub_section = NULL;
@@ -5196,7 +5203,15 @@ mips_elf_calculate_relocation (bfd *abfd
 	  sec = h->fn_stub;
 	}
 
-      symbol = sec->output_section->vma + sec->output_offset;
+      /* If a LA25 header for the stub itself exists, point to the prepended
+	 LUI/ADDIU sequence.  */
+      if (h != NULL && h->la25_stub)
+	symbol = (h->la25_stub->stub_section->output_section->vma
+		  + h->la25_stub->stub_section->output_offset
+		  + h->la25_stub->offset);
+      else
+	symbol = sec->output_section->vma + sec->output_offset;
+
       /* The target is 16-bit, but the stub isn't.  */
       target_is_16_bit_code_p = FALSE;
     }
@@ -5246,7 +5261,8 @@ mips_elf_calculate_relocation (bfd *abfd
   /* If this is a direct call to a PIC function, redirect to the
      non-PIC stub.  */
   else if (h != NULL && h->la25_stub
-	   && mips_elf_relocation_needs_la25_stub (input_bfd, r_type))
+	   && mips_elf_relocation_needs_la25_stub (input_bfd, r_type)
+	   && !(r_type == R_MIPS16_26 && target_is_16_bit_code_p))
     symbol = (h->la25_stub->stub_section->output_section->vma
 	      + h->la25_stub->stub_section->output_offset
 	      + h->la25_stub->offset);
@@ -9615,10 +9631,16 @@ mips_elf_create_la25_stub (void **slot, 
   /* Work out where in the section this stub should go.  */
   offset = stub->offset;
 
-  /* Work out the target address.  */
-  target = (stub->h->root.root.u.def.section->output_section->vma
-	    + stub->h->root.root.u.def.section->output_offset
-	    + stub->h->root.root.u.def.value);
+  /* Work out the target address.  If a MIPS16 32-bit stub exists,
+     use it as the target instead.  */
+  if (stub->h->fn_stub)
+    target = (stub->h->fn_stub->output_section->vma
+	      + stub->h->fn_stub->output_offset);
+  else
+    target = (stub->h->root.root.u.def.section->output_section->vma
+	      + stub->h->root.root.u.def.section->output_offset
+	      + stub->h->root.root.u.def.value);
+
   target_high = ((target + 0x8000) >> 16) & 0xffff;
   target_low = (target & 0xffff);
 

Re: [PATCH] Fixing MIPS16 vs LA25 stubs

[Richard Sandiford]

Chung-Lin Tang <cltang@codesourcery.com> writes:
> @@ -1573,7 +1573,9 @@ _bfd_mips_elf_init_stubs (struct bfd_lin
>  /* Return true if H is a locally-defined PIC function, in the sense
>     that it might need $25 to be valid on entry.  Note that MIPS16
>     functions never need $25 to be valid on entry; they set up $gp
> -   using PC-relative instructions instead.  */
> +   using PC-relative instructions instead.  We do process the case
> +   where a MIPS16 function has a 32-bit stub, where the $25 load
> +   is added for the 32-bit stub itself, not the 16-bit function.  */

Nitpick, but this feels too tacked on.  How about:

/* Return true if H is a locally-defined PIC function, in the sense
   that it or its fn_stub might need $25 to be valid on entry.
   Note that MIPS16 functions set up $gp using PC-relative instructions,
   so they themselves never need $25 to be valid.  Only non-MIPS16
   entry points are of interest here.  */

> @@ -1610,8 +1613,10 @@ mips_elf_add_la25_intro (struct mips_elf
>      return FALSE;
>    sprintf (name, ".text.stub.%d", (int) htab_elements (htab->la25_stubs));
>  
> -  /* Create the section.  */
> -  input_section = stub->h->root.root.u.def.section;
> +  /* Create the section.  If H is a MIPS16 function with a stub, use the
> +     stub function section.  */
> +  input_section = (stub->h->fn_stub
> +		   ? stub->h->fn_stub : stub->h->root.root.u.def.section);
>    s = htab->add_stub_section (name, input_section,
>  			      input_section->output_section);
>    if (s == NULL)

Let's split this out into:

/* Set *SEC to the input section that contains the target of STUB.
   Return the offset of the target from the start of that section.  */

static bfd_vma
mips_elf_get_la25_target (struct mips_elf_la25_stub *stub,
			  asection **sec)
{
  if (ELF_ST_IS_MIPS16 (stub->h))
    {
      BFD_ASSERT (stub->h->need_fn_stub);
      *sec = stub->h->fn_stub;
      return 0;
    }
  else
    {
      *sec = stub->h->root.root.u.def.section;
      return h->root.root.u.def.value;
    }
}

(just after mips_elf_local_pic_function_p would be a good place).
Then the code above becomes:

  /* Create the section.  */
  mips_elf_get_la25_target (stub, &input_section);

> @@ -1686,10 +1691,12 @@ mips_elf_add_la25_stub (struct bfd_link_
>    void **slot;
>  
>    /* Prefer to use LUI/ADDIU stubs if the function is at the beginning
> -     of the section and if we would need no more than 2 nops.  */
> +     of the section and if we would need no more than 2 nops.  Also
> +     when processing a 32-bit stub for a MIPS16 function, we can always
> +     preprend before the stub section.  */
>    s = h->root.root.u.def.section;
>    value = h->root.root.u.def.value;
> -  use_trampoline_p = (value != 0 || s->alignment_power > 4);
> +  use_trampoline_p = (value != 0 || s->alignment_power > 4) && !h->fn_stub;

This then becomes:

  /* Prefer to use LUI/ADDIU stubs if the function is at the beginning
     of the section and if we would need no more than 2 nops.  */
  value = mips_elf_get_la25_target (stub, &s);
  use_trampoline_p = (value != 0 || s->alignment_power > 4);

(I don't want to special-case stub sections here: if someone does for
whatever reason give stub sections a large alignment, the separate
trampoline is better.)

> @@ -5196,7 +5203,15 @@ mips_elf_calculate_relocation (bfd *abfd
>  	  sec = h->fn_stub;
>  	}
>  
> -      symbol = sec->output_section->vma + sec->output_offset;
> +      /* If a LA25 header for the stub itself exists, point to the prepended
> +	 LUI/ADDIU sequence.  */
> +      if (h != NULL && h->la25_stub)
> +	symbol = (h->la25_stub->stub_section->output_section->vma
> +		  + h->la25_stub->stub_section->output_offset
> +		  + h->la25_stub->offset);
> +      else
> +	symbol = sec->output_section->vma + sec->output_offset;
> +

This becomes a bit confusing, because it's only relevant for the !local_p case.
How about:

      if (local_p)
	{
	  sec = elf_tdata (input_bfd)->local_stubs[r_symndx];
	  value = 0;
	}
      else
	{
	  BFD_ASSERT (h->need_fn_stub);
	  if (h->la25_stub)
	    {
	      sec = h->la25_stub->section;
	      value = h->la25_stub->offset;
	    }
	  else
	    {
	      sec = h->fn_stub;
	      value = 0;
	    }
	}

      symbol = sec->output_section->vma + sec->output_offset + value;
      /* The target is 16-bit, but the stub isn't.  */
      target_is_16_bit_code_p = FALSE;

> @@ -5246,7 +5261,8 @@ mips_elf_calculate_relocation (bfd *abfd
>    /* If this is a direct call to a PIC function, redirect to the
>       non-PIC stub.  */
>    else if (h != NULL && h->la25_stub
> -	   && mips_elf_relocation_needs_la25_stub (input_bfd, r_type))
> +	   && mips_elf_relocation_needs_la25_stub (input_bfd, r_type)
> +	   && !(r_type == R_MIPS16_26 && target_is_16_bit_code_p))
>      symbol = (h->la25_stub->stub_section->output_section->vma
>  	      + h->la25_stub->stub_section->output_offset
>  	      + h->la25_stub->offset);

This logically belongs in mips_elf_relocation_needs_la25_stub.
Let's pass the target_is_16_bit_code_p there:

/* Return TRUE if a relocation of type R_TYPE from INPUT_BFD might
   require an la25 stub.  TARGET_IS_16_BIT_CODE_P is true if the
   target is microMIPS or MIPS16 code.

   See also mips_elf_local_pic_function_p, which determines whether the
   destination function ever requires a stub.  */

static bfd_boolean
mips_elf_relocation_needs_la25_stub (bfd *input_bfd, int r_type,
				     bfd_boolean target_is_16_bit_code_p)
{
  /* 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.  */
  if (PIC_OBJECT_P (input_bfd))
    return FALSE;

  switch (r_type)
    {
    case R_MIPS_26:
    case R_MIPS_PC16:
    case R_MICROMIPS_26_S1:
    case R_MICROMIPS_PC7_S1:
    case R_MICROMIPS_PC10_S1:
    case R_MICROMIPS_PC16_S1:
    case R_MICROMIPS_PC23_S2:
      return TRUE;

    case R_MIPS16_26:
      return !target_is_16_bit_code_p;

    default:
      return FALSE;
    }
}

> @@ -9615,10 +9631,16 @@ mips_elf_create_la25_stub (void **slot, 
>    /* Work out where in the section this stub should go.  */
>    offset = stub->offset;
>  
> -  /* Work out the target address.  */
> -  target = (stub->h->root.root.u.def.section->output_section->vma
> -	    + stub->h->root.root.u.def.section->output_offset
> -	    + stub->h->root.root.u.def.value);
> +  /* Work out the target address.  If a MIPS16 32-bit stub exists,
> +     use it as the target instead.  */
> +  if (stub->h->fn_stub)
> +    target = (stub->h->fn_stub->output_section->vma
> +	      + stub->h->fn_stub->output_offset);
> +  else
> +    target = (stub->h->root.root.u.def.section->output_section->vma
> +	      + stub->h->root.root.u.def.section->output_offset
> +	      + stub->h->root.root.u.def.value);
> +

This becomes:

  /* Work out the target address.  */
  target = mips_elf_get_la25_target (stub, &s);
  target += s->output_section->offset + s->output_offset;

OK with those changes, thanks.

Richard