This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFC PATCH, binutils, ARM 2/11, ping] Factor our stub creation in ARM backend
- From: Thomas Preudhomme <thomas dot preudhomme at foss dot arm dot com>
- To: Christophe Lyon <christophe dot lyon at linaro dot org>
- Cc: binutils at sourceware dot org
- Date: Mon, 04 Apr 2016 15:01 +0100
- Subject: Re: [RFC PATCH, binutils, ARM 2/11, ping] Factor our stub creation in ARM backend
- Authentication-results: sourceware.org; auth=none
- References: <004801d13d57$4a13b670$de3b2350$ at foss dot arm dot com> <2632328 dot d9AUHOZn5J at e108577-lin> <CAKdteOa7=_Q4qZLqF+e1yk3qUKkCyfSoKz6XUJ6+qbH5yopORg at mail dot gmail dot com>
On Wednesday 30 March 2016 11:11:01 Christophe Lyon wrote:
> On 29 March 2016 at 16:33, Thomas Preudhomme
>
> <thomas.preudhomme@foss.arm.com> wrote:
> > Ping?
> >
> > On Wednesday 23 December 2015 15:55:26 Thomas Preud'homme wrote:
> >> Hi,
> >>
> >> [Posting patch series as RFC]
> >>
> >> This patch is part of a patch series to add support for ARMv8-M security
> >> extension[1] to GNU ld. This specific patch factors out the code to
> >> create
> >> a stub in ARM backend to avoid code duplication in subsequent patches
> >> adding new code for creating Secure Gateway veneers.
> >>
> >>
> >> [1] Software requirements for ARMv8-M security extension are described in
> >> document ARM-ECM-0359818 [2] [2] Available on http://infocenter.arm.com
> >> in
> >> Developer guides and articles > Software development > ARMÂv8-M Security
> >> Extensions: Requirements on Development Tools
> >>
> >> ChangeLog entries is as follows:
> >>
> >>
> >> *** bfd/ChangeLog ***
> >>
> >> 2015-12-16 Thomas Preud'homme <thomas.preudhomme@arm.com>
> >>
> >> * elf32-arm.c (elf32_arm_create_stub): New function.
> >> (elf32_arm_size_stubs): Use elf32_arm_create_stub for stub
> >> creation.
> >>
> >> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> >> index
> >> 639fe0f125688ccf218b449467c746686841386f..e6afb99512524ac26c0e3842e8dbe29
> >> 3c
> >> 58bacad 100644 --- a/bfd/elf32-arm.c
> >> +++ b/bfd/elf32-arm.c
> >> @@ -5031,6 +5031,103 @@ cortex_a8_erratum_scan (bfd *input_bfd,
> >>
> >> return FALSE;
> >>
> >> }
> >>
> >> +/* Create or update a stub entry depending on whether the stub can
> >> already
> >> be + found in HTAB. The stub is identified by:
> >> + - its type STUB_TYPE
> >> + - its source branch (note that several can share the same stub) whose
> >> + section and relocation (if any) are given by SECTION and IRELA
> >> + respectively
> >> + - its target symbol whose input section, hash, name, value and branch
> >> type + are given in SYM_SEC, HASH, SYM_NAME, SYM_VALUE and
> >> BRANCH_TYPE
> >> + respectively
> >> +
> >> + If found, the value of the stub's target symbol is updated from
> >> SYM_VALUE + and *NEW_STUB is set to FALSE. Otherwise, *NEW_STUB is set
> >> to + TRUE and the stub entry is initialized.
> >> +
> >> + Returns whether the stub could be successfully created or updated, or
> >> FALSE + if an error occured. */
> >> +
> >> +static bfd_boolean
> >> +elf32_arm_create_stub (struct elf32_arm_link_hash_table *htab,
> >> + enum elf32_arm_stub_type stub_type, asection
> >> *section,
> >> + Elf_Internal_Rela *irela, asection *sym_sec,
> >> + struct elf32_arm_link_hash_entry *hash, char
> >> *sym_name, + bfd_vma sym_value, enum
> >> arm_st_branch_type branch_type, + bfd_boolean
> >> *new_stub)
> >> +{
> >> + const asection *id_sec;
> >> + char *stub_name;
> >> + struct elf32_arm_stub_hash_entry *stub_entry;
> >> + unsigned int r_type;
> >> +
> >> + BFD_ASSERT (stub_type != arm_stub_none);
> >> + *new_stub = FALSE;
> >> +
> >> + BFD_ASSERT (irela);
> >> + BFD_ASSERT (section);
> >> +
> >> + /* Support for grouping stub sections. */
> >> + id_sec = htab->stub_group[section->id].link_sec;
> >> +
> >> + /* Get the name of this stub. */
> >> + stub_name = elf32_arm_stub_name (id_sec, sym_sec, hash, irela,
> >> stub_type); + if (!stub_name)
> >> + return FALSE;
> >> +
> >> + stub_entry = arm_stub_hash_lookup (&htab->stub_hash_table, stub_name,
> >> FALSE, + FALSE);
> >> + /* The proper stub has already been created, just update its value.
> >> */
> >> + if (stub_entry != NULL)
> >> + {
> >> + free (stub_name);
> >> + stub_entry->target_value = sym_value;
> >> + return TRUE;
> >> + }
> >> +
> >> + stub_entry = elf32_arm_add_stub (stub_name, section, htab);
> >> + if (stub_entry == NULL)
> >> + {
> >> + free (stub_name);
> >> + return FALSE;
> >> + }
> >> +
> >> + stub_entry->target_value = sym_value;
> >> + stub_entry->target_section = sym_sec;
> >> + stub_entry->stub_type = stub_type;
> >> + stub_entry->h = hash;
> >> + stub_entry->branch_type = branch_type;
> >> +
> >> + if (sym_name == NULL)
> >> + sym_name = "unnamed";
> >> + stub_entry->output_name = (char *)
> >> + bfd_alloc (htab->stub_bfd, sizeof (THUMB2ARM_GLUE_ENTRY_NAME)
> >> + + strlen (sym_name));
> >> + if (stub_entry->output_name == NULL)
> >> + {
> >> + free (stub_name);
> >> + return FALSE;
> >> + }
> >> +
> >> + /* For historical reasons, use the existing names for ARM-to-Thumb and
> >> + Thumb-to-ARM stubs. */
> >> + r_type = ELF32_R_TYPE (irela->r_info);
> >> + if ((r_type == (unsigned int) R_ARM_THM_CALL
> >> + || r_type == (unsigned int) R_ARM_THM_JUMP24
> >> + || r_type == (unsigned int) R_ARM_THM_JUMP19)
> >> + && branch_type == ST_BRANCH_TO_ARM)
> >> + sprintf (stub_entry->output_name, THUMB2ARM_GLUE_ENTRY_NAME,
> >> sym_name); + else if ((r_type == (unsigned int) R_ARM_CALL
> >> + || r_type == (unsigned int) R_ARM_JUMP24)
> >> + && branch_type == ST_BRANCH_TO_THUMB)
> >> + sprintf (stub_entry->output_name, ARM2THUMB_GLUE_ENTRY_NAME,
> >> sym_name); + else
> >> + sprintf (stub_entry->output_name, STUB_ENTRY_NAME, sym_name);
> >> +
> >> + *new_stub = TRUE;
> >> + return TRUE;
> >> +}
> >> +
> >>
> >> /* Determine and set the size of the stub section for a final link.
> >>
> >> The basic idea here is to examine all the relocations looking for
> >>
> >> @@ -5174,14 +5271,11 @@ elf32_arm_size_stubs (bfd *output_bfd,
> >>
> >> {
> >>
> >> unsigned int r_type, r_indx;
> >> enum elf32_arm_stub_type stub_type;
> >>
> >> - struct elf32_arm_stub_hash_entry *stub_entry;
> >>
> >> asection *sym_sec;
> >> bfd_vma sym_value;
> >> bfd_vma destination;
> >> struct elf32_arm_link_hash_entry *hash;
> >> const char *sym_name;
> >>
> >> - char *stub_name;
> >> - const asection *id_sec;
> >>
> >> unsigned char st_type;
> >> enum arm_st_branch_type branch_type;
> >> bfd_boolean created_stub = FALSE;
> >>
> >> @@ -5364,6 +5458,8 @@ elf32_arm_size_stubs (bfd *output_bfd,
> >>
> >> do
> >>
> >> {
> >>
> >> + bfd_boolean new_stub;
> >> +
> >>
> >> /* Determine what (if any) linker stub is needed. */
> >> stub_type = arm_type_of_stub (info, section, irela,
> >>
> >> st_type, &branch_type,
> >>
> >> @@ -5372,74 +5468,21 @@ elf32_arm_size_stubs (bfd *output_bfd,
> >>
> >> if (stub_type == arm_stub_none)
> >>
> >> break;
> >>
> >> - /* Support for grouping stub sections. */
> >> - id_sec = htab->stub_group[section->id].link_sec;
> >> -
> >> - /* Get the name of this stub. */
> >> - stub_name = elf32_arm_stub_name (id_sec, sym_sec,
> >> hash,
> >> - irela, stub_type);
> >> - if (!stub_name)
> >> - goto error_ret_free_internal;
> >> -
> >>
> >> /* We've either created a stub for this reloc
> >> already,
> >>
> >> or we are about to. */
> >>
> >> - created_stub = TRUE;
> >> -
> >> - stub_entry = arm_stub_hash_lookup
> >> - (&htab->stub_hash_table, stub_name,
> >> - FALSE, FALSE);
> >> - if (stub_entry != NULL)
> >> - {
> >> - /* The proper stub has already been created. */
> >> - free (stub_name);
> >> - stub_entry->target_value = sym_value;
> >> - break;
> >> - }
Please take note of this lookup + conditional block.
> >> -
> >> - stub_entry = elf32_arm_add_stub (stub_name, section,
> >> - htab);
> >> - if (stub_entry == NULL)
> >> - {
> >> - free (stub_name);
> >> - goto error_ret_free_internal;
> >> - }
> >> + created_stub =
> >> + elf32_arm_create_stub (htab, stub_type, section,
> >> irela, + sym_sec, hash,
> >> + (char *) sym_name,
> >> sym_value,
> >> + branch_type, &new_stub);
> >>
> >> - stub_entry->target_value = sym_value;
> >> - stub_entry->target_section = sym_sec;
> >> - stub_entry->stub_type = stub_type;
> >> - stub_entry->h = hash;
> >> - stub_entry->branch_type = branch_type;
> >> -
> >> - if (sym_name == NULL)
> >> - sym_name = "unnamed";
> >> - stub_entry->output_name = (char *)
> >> - bfd_alloc (htab->stub_bfd,
> >> - sizeof (THUMB2ARM_GLUE_ENTRY_NAME)
> >> - + strlen (sym_name));
> >> - if (stub_entry->output_name == NULL)
> >> + if (!created_stub || !new_stub)
>
> I'm not sure to understand why you check both?
Good catch! That's a result of the iterative process that for the create_stub
function. It should be a break for the !new_stub case, as in the code I
hilighted above.
I'll respin the patch.
Best regards,
Thomas