This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Fixing MIPS16 vs LA25 stubs
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Chung-Lin Tang <cltang at codesourcery dot com>
- Cc: binutils <binutils at sourceware dot org>
- Date: Sat, 10 Dec 2011 11:32:07 +0000
- Subject: Re: [PATCH] Fixing MIPS16 vs LA25 stubs
- References: <4EE32BCF.30405@codesourcery.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