Re: [PATCH] Fixing MIPS16 vs LA25 stubs

[Richard Sandiford <rdsandiford at googlemail dot com>]

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