From: Richard Sandiford <rdsandiford@googlemail.com>
To: Chung-Lin Tang <cltang@codesourcery.com>
Cc: binutils <binutils@sourceware.org>
Subject: Re: [PATCH] Fixing MIPS16 vs LA25 stubs
Date: Sat, 10 Dec 2011 11:32:00 -0000 [thread overview]
Message-ID: <87ehwcek7c.fsf@firetop.home> (raw)
In-Reply-To: <4EE32BCF.30405@codesourcery.com> (Chung-Lin Tang's message of "Sat, 10 Dec 2011 17:52:15 +0800")
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
prev parent reply other threads:[~2011-12-10 11:32 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-10 9:52 Chung-Lin Tang
2011-12-10 11:32 ` Richard Sandiford [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ehwcek7c.fsf@firetop.home \
--to=rdsandiford@googlemail.com \
--cc=binutils@sourceware.org \
--cc=cltang@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).