Re: [RFC PATCH, binutils, ARM 2/11, ping] Factor our stub creation in ARM backend

[Christophe Lyon <christophe dot lyon at linaro dot org>]

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..e6afb99512524ac26c0e3842e8dbe293c
>> 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;
>> -                     }
>> -
>> -                   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?

>>                       {
>> -                       free (stub_name);
>> -                       goto error_ret_free_internal;
>> +                       if (!created_stub)
>> +                         goto error_ret_free_internal;
>>                       }
>> -
>> -                   /* For historical reasons, use the existing names for
>> -                      ARM-to-Thumb and Thumb-to-ARM stubs.  */
>> -                   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);
>> -
>> -                   stub_changed = TRUE;
>> +                     stub_changed = TRUE;
>>                   }
>>                 while (0);
>>
>>
>> The patch doesn't show any regression when running the binutils-gdb
>> testsuite for the arm-none-eabi target.
>>
>> Any comments?
>>
>> Best regards,
>>
>> Thomas
>