bfd/RISC-V: Repeat a single target relax pass instead of 3

Message ID CAJd+PRMpjDQGZrezb7aYxBhqTh8fFvuOjmOmcQgpYruDLYXRCA@mail.gmail.com
State New
Headers show
Series
  • bfd/RISC-V: Repeat a single target relax pass instead of 3
Related show

Commit Message

Lewis Revill Oct. 7, 2021, 9:01 p.m.
Commit abd20cb637008da9d32018b4b03973e119388a0a and
ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4 introduced additional
complexity into the paths run by the RISC-V relaxation pass in order to
resolve the issue of accurately keeping track of pcrel_hi and pcrel_lo
pairs. The first commit split up relaxation of these relocs into a pass
which occurred after other relaxations in order to prevent the situation
where bytes were deleted in between a pcrel_lo/pcrel_hi pair, inhibiting
our ability to find the corresponding pcrel_hi relocation from the
address attached to the pcrel_lo.

Since the relaxation was split into two passes the 'again' parameter
could not be used to perform the entire relaxation process again and so
the second commit added a way to restart ldelf_map_segments, thus
starting the whole process again.

Unfortunately this process could not account for the fact that we were
not finished with the relaxation process so in some cases - such as the
case where code would not fit in a memory region before the
R_RISCV_ALIGN relocation was relaxed - sanity checks in generic code
would fail.

This patch fixes all three of these concerns by reverting back to a
system of having only one target relax pass but updating entries in the
table of pcrel_hi/pcrel_lo relocs every time any bytes are deleted. Thus
we can keep track of the pairs accurately, and we can use the 'again'
parameter to restart the entire target relax pass, behaving in the way
that generic code expects.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=28410

bfd/
* elfnn-riscv.c: Revert restart_relax changes.
(_bfd_riscv_relax_pc): Delete bytes immediately rather than
using R_RISCV_DELETE.
(_bfd_riscv_relax_delete): Remove.
(riscv_relax_delete_bytes): Update tpgp relocs whenever bytes
are deleted.
(riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi
section offsets.
(riscv_update_pcgp_reloc_symvals): Add function to update
pcrel_hi symbol values.
(_bfd_riscv_relax_section): Use only one pass for all target
relaxations.
* elfxx-riscv.h: Revert restart_relax changes.

ld/
* emultempl/riscvelf.em: Revert restart_relax changes and set
relax_pass to 2.
---
 bfd/ChangeLog                                 |  16 +
 bfd/elfnn-riscv.c                             | 403 +++++++++---------
 bfd/elfxx-riscv.h                             |   5 -
 ld/ChangeLog                                  |   5 +
 ld/emultempl/riscvelf.em                      |   8 +-
 .../ld-riscv-elf/align-small-region.d         |  12 +
 .../ld-riscv-elf/align-small-region.ld        |  12 +
 .../ld-riscv-elf/align-small-region.s         |   7 +
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |   1 +
 9 files changed, 251 insertions(+), 218 deletions(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

-- 
2.25.1

Comments

Nelson Chu Oct. 8, 2021, 8:52 a.m. | #1
Hi Lewis,

This patch is actually option 3 rather than option 2 in the pr28410,
but personally I prefer option 3, so the solution is what I expected,
thank you.  However, I need more time to run the regressions, and make
sure the patch won't cause side effects.  But before I start to run
the regressions, I have a quick review, and do see some places that
could be improved.

On Fri, Oct 8, 2021 at 5:01 AM Lewis Revill <lewis.revill@embecosm.com> wrote:
>

> Commit abd20cb637008da9d32018b4b03973e119388a0a and

> ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4 introduced additional

> complexity into the paths run by the RISC-V relaxation pass in order to

> resolve the issue of accurately keeping track of pcrel_hi and pcrel_lo

> pairs. The first commit split up relaxation of these relocs into a pass

> which occurred after other relaxations in order to prevent the situation

> where bytes were deleted in between a pcrel_lo/pcrel_hi pair, inhibiting

> our ability to find the corresponding pcrel_hi relocation from the

> address attached to the pcrel_lo.

>

> Since the relaxation was split into two passes the 'again' parameter

> could not be used to perform the entire relaxation process again and so

> the second commit added a way to restart ldelf_map_segments, thus

> starting the whole process again.

>

> Unfortunately this process could not account for the fact that we were

> not finished with the relaxation process so in some cases - such as the

> case where code would not fit in a memory region before the

> R_RISCV_ALIGN relocation was relaxed - sanity checks in generic code

> would fail.

>

> This patch fixes all three of these concerns by reverting back to a

> system of having only one target relax pass but updating entries in the

> table of pcrel_hi/pcrel_lo relocs every time any bytes are deleted. Thus

> we can keep track of the pairs accurately, and we can use the 'again'

> parameter to restart the entire target relax pass, behaving in the way

> that generic code expects.

>

> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=28410

>

> bfd/

> * elfnn-riscv.c: Revert restart_relax changes.

> (_bfd_riscv_relax_pc): Delete bytes immediately rather than

> using R_RISCV_DELETE.

> (_bfd_riscv_relax_delete): Remove.

> (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes

> are deleted.

> (riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi

> section offsets.

> (riscv_update_pcgp_reloc_symvals): Add function to update

> pcrel_hi symbol values.

> (_bfd_riscv_relax_section): Use only one pass for all target

> relaxations.

> * elfxx-riscv.h: Revert restart_relax changes.

>

> ld/

> * emultempl/riscvelf.em: Revert restart_relax changes and set

> relax_pass to 2.

> ---

>  bfd/ChangeLog                                 |  16 +

>  bfd/elfnn-riscv.c                             | 403 +++++++++---------

>  bfd/elfxx-riscv.h                             |   5 -

>  ld/ChangeLog                                  |   5 +

>  ld/emultempl/riscvelf.em                      |   8 +-

>  .../ld-riscv-elf/align-small-region.d         |  12 +

>  .../ld-riscv-elf/align-small-region.ld        |  12 +

>  .../ld-riscv-elf/align-small-region.s         |   7 +

>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |   1 +

>  9 files changed, 251 insertions(+), 218 deletions(-)

>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d

>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld

>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

>

> diff --git a/bfd/ChangeLog b/bfd/ChangeLog

> index 2a08ff7cfb4..7bcba3a090f 100644

> --- a/bfd/ChangeLog

> +++ b/bfd/ChangeLog

> @@ -1,3 +1,19 @@

> +2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>

> +

> + * elfnn-riscv.c: Revert restart_relax changes.

> + (_bfd_riscv_relax_pc): Delete bytes immediately rather than

> + using R_RISCV_DELETE.

> + (_bfd_riscv_relax_delete): Remove.

> + (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes

> + are deleted.

> + (riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi

> + section offsets.

> + (riscv_update_pcgp_reloc_symvals): Add function to update

> + pcrel_hi symbol values.

> + (_bfd_riscv_relax_section): Use only one pass for all target

> + relaxations.

> + * elfxx-riscv.h: Revert restart_relax changes.

> +

>  2021-09-27  Nick Alcock  <nick.alcock@oracle.com>

>

>   * configure: Regenerate.

> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c

> index 2e8df72fa2a..95e2d5536d1 100644

> --- a/bfd/elfnn-riscv.c

> +++ b/bfd/elfnn-riscv.c

> @@ -38,9 +38,6 @@

>  #define CHAR_BIT 8

>  #endif

>

> -/* Internal relocations used exclusively by the relaxation pass.  */

> -#define R_RISCV_DELETE (R_RISCV_max + 1)

> -


This looks interesting since you are removing the relax pass, which
deletes the bytes that pass 1 made obsolete.  I'm not sure if the
change will cause any side effects, so I would suggest keeping the
delete relax pass separating, like what we always did before.

>  #define ARCH_SIZE NN

>

>  #define MINUS_ONE ((bfd_vma)0 - 1)

> @@ -131,9 +128,6 @@ struct riscv_elf_link_hash_table

>    /* The index of the last unused .rel.iplt slot.  */

>    bfd_vma last_iplt_index;

>

> -  /* Re-run the relaxations from relax pass 0 if TRUE.  */

> -  bool restart_relax;

> -

>    /* The data segment phase, don't relax the section

>       when it is exp_seg_relro_adjust.  */

>    int *data_segment_phase;

> @@ -405,7 +399,6 @@ riscv_elf_link_hash_table_create (bfd *abfd)

>      }

>

>    ret->max_alignment = (bfd_vma) -1;

> -  ret->restart_relax = false;

>

>    /* Create hash table for local ifunc.  */

>    ret->loc_hash_table = htab_try_create (1024,

> @@ -1714,9 +1707,6 @@ perform_relocation (const reloc_howto_type *howto,

>      case R_RISCV_TLS_DTPREL64:

>        break;

>

> -    case R_RISCV_DELETE:

> -      return bfd_reloc_ok;

> -

>      default:

>        return bfd_reloc_notsupported;

>      }

> @@ -2326,7 +2316,6 @@ riscv_elf_relocate_section (bfd *output_bfd,

>   case R_RISCV_SET16:

>   case R_RISCV_SET32:

>   case R_RISCV_32_PCREL:

> - case R_RISCV_DELETE:

>    /* These require no special handling beyond perform_relocation.  */

>    break;

>

> @@ -3923,115 +3912,6 @@ _bfd_riscv_elf_merge_private_bfd_data (bfd *ibfd,

> struct bfd_link_info *info)

>    return false;

>  }

>

> -/* Delete some bytes from a section while relaxing.  */

> -

> -static bool

> -riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t

> count,

> -  struct bfd_link_info *link_info)

> -{

> -  unsigned int i, symcount;

> -  bfd_vma toaddr = sec->size;

> -  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);

> -  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;

> -  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);

> -  struct bfd_elf_section_data *data = elf_section_data (sec);

> -  bfd_byte *contents = data->this_hdr.contents;

> -

> -  /* Actually delete the bytes.  */

> -  sec->size -= count;

> -  memmove (contents + addr, contents + addr + count, toaddr - addr -

> count);

> -

> -  /* Adjust the location of all of the relocs.  Note that we need not

> -     adjust the addends, since all PC-relative references must be against

> -     symbols, which we will adjust below.  */

> -  for (i = 0; i < sec->reloc_count; i++)

> -    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <

> toaddr)

> -      data->relocs[i].r_offset -= count;

> -

> -  /* Adjust the local symbols defined in this section.  */

> -  for (i = 0; i < symtab_hdr->sh_info; i++)

> -    {

> -      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +

> i;

> -      if (sym->st_shndx == sec_shndx)

> - {

> -  /* If the symbol is in the range of memory we just moved, we

> -     have to adjust its value.  */

> -  if (sym->st_value > addr && sym->st_value <= toaddr)

> -    sym->st_value -= count;

> -

> -  /* If the symbol *spans* the bytes we just deleted (i.e. its

> -     *end* is in the moved bytes but its *start* isn't), then we

> -     must adjust its size.

> -

> -     This test needs to use the original value of st_value, otherwise

> -     we might accidentally decrease size when deleting bytes right

> -     before the symbol.  But since deleted relocs can't span across

> -     symbols, we can't have both a st_value and a st_size decrease,

> -     so it is simpler to just use an else.  */

> -  else if (sym->st_value <= addr

> -   && sym->st_value + sym->st_size > addr

> -   && sym->st_value + sym->st_size <= toaddr)

> -    sym->st_size -= count;

> - }

> -    }

> -

> -  /* Now adjust the global symbols defined in this section.  */

> -  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))

> -      - symtab_hdr->sh_info);

> -

> -  for (i = 0; i < symcount; i++)

> -    {

> -      struct elf_link_hash_entry *sym_hash = sym_hashes[i];

> -

> -      /* The '--wrap SYMBOL' option is causing a pain when the object file,

> - containing the definition of __wrap_SYMBOL, includes a direct

> - call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference

> - the same symbol (which is __wrap_SYMBOL), but still exist as two

> - different symbols in 'sym_hashes', we don't want to adjust

> - the global symbol __wrap_SYMBOL twice.

> -

> - The same problem occurs with symbols that are versioned_hidden, as

> - foo becomes an alias for foo@BAR, and hence they need the same

> - treatment.  */

> -      if (link_info->wrap_hash != NULL

> -  || sym_hash->versioned != unversioned)

> - {

> -  struct elf_link_hash_entry **cur_sym_hashes;

> -

> -  /* Loop only over the symbols which have already been checked.  */

> -  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];

> -       cur_sym_hashes++)

> -    {

> -      /* If the current symbol is identical to 'sym_hash', that means

> - the symbol was already adjusted (or at least checked).  */

> -      if (*cur_sym_hashes == sym_hash)

> - break;

> -    }

> -  /* Don't adjust the symbol again.  */

> -  if (cur_sym_hashes < &sym_hashes[i])

> -    continue;

> - }

> -

> -      if ((sym_hash->root.type == bfd_link_hash_defined

> -   || sym_hash->root.type == bfd_link_hash_defweak)

> -  && sym_hash->root.u.def.section == sec)

> - {

> -  /* As above, adjust the value if needed.  */

> -  if (sym_hash->root.u.def.value > addr

> -      && sym_hash->root.u.def.value <= toaddr)

> -    sym_hash->root.u.def.value -= count;

> -

> -  /* As above, adjust the size if needed.  */

> -  else if (sym_hash->root.u.def.value <= addr

> -   && sym_hash->root.u.def.value + sym_hash->size > addr

> -   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)

> -    sym_hash->size -= count;

> - }

> -    }

> -

> -  return true;

> -}

> -

>  /* A second format for recording PC-relative hi relocations.  This stores

> the

>     information required to relax them to GP-relative addresses.  */

>

> @@ -4162,6 +4042,166 @@ riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p,

> bfd_vma hi_sec_off)

>    return false;

>  }

>

> +static bool

> +riscv_update_pcgp_reloc_offs (riscv_pcgp_relocs *p, bfd_vma

> deleted_sec_off,

> +      size_t deleted_count)

> +{

> +  riscv_pcgp_lo_reloc *l;

> +  riscv_pcgp_hi_reloc *h;

> +  bool updated = false;

> +

> +  for (l = p->lo; l != NULL; l = l->next)

> +    if (l->hi_sec_off > deleted_sec_off)

> +      {

> + l->hi_sec_off -= deleted_count;

> + updated = true;

> +      }

> +

> +  for (h = p->hi; h != NULL; h = h->next)

> +    if (h->hi_sec_off > deleted_sec_off)

> +      {

> + h->hi_sec_off -= deleted_count;

> + updated = true;

> +      }

> +

> +  return updated;

> +}


The returned boolean `updated' looks useless and redundant, so we
probably can remove it.

> +static bool

> +riscv_update_pcgp_reloc_symvals(riscv_pcgp_relocs *p, bfd_vma old_value,

> + size_t deleted_count)

> +{

> +  riscv_pcgp_hi_reloc *h;

> +  bool updated = false;

> +

> +  for (h = p->hi; h != NULL; h = h->next)

> +    if (h->hi_addr == old_value)

> +      {

> + h->hi_addr -= deleted_count;

> + updated = true;

> +      }

> +  return updated;

> +}


Likewise.  Besides, I suppose you also need to check if the `sym_sec'
of riscv_pcgp_relocs is the same as the section in which we are
deleting the bytes.  That is  - we can only modify the symbol value
when the symbol section is the same as the deleting section.

> +/* Delete some bytes from a section while relaxing.  */

> +

> +static bool

> +riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t

> count,

> +  struct bfd_link_info *link_info, riscv_pcgp_relocs *p)

> +{

> +  unsigned int i, symcount;

> +  bfd_vma toaddr = sec->size;

> +  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);

> +  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;

> +  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);

> +  struct bfd_elf_section_data *data = elf_section_data (sec);

> +  bfd_byte *contents = data->this_hdr.contents;

> +

> +  /* Actually delete the bytes.  */

> +  sec->size -= count;

> +  memmove (contents + addr, contents + addr + count, toaddr - addr -

> count);

> +

> +  /* Adjust the location of all of the relocs.  Note that we need not

> +     adjust the addends, since all PC-relative references must be against

> +     symbols, which we will adjust below.  */

> +  for (i = 0; i < sec->reloc_count; i++)

> +    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <

> toaddr)

> +      data->relocs[i].r_offset -= count;

> +

> +  /* Adjust the hi_sec_off of entries in the pcgp relocs table.  */

> +  riscv_update_pcgp_reloc_offs (p, addr, count);

> +

> +  /* Adjust the local symbols defined in this section.  */

> +  for (i = 0; i < symtab_hdr->sh_info; i++)

> +    {

> +      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +

> i;

> +      if (sym->st_shndx == sec_shndx)

> + {

> +  /* If the symbol is in the range of memory we just moved, we

> +     have to adjust its value.  */

> +  if (sym->st_value > addr && sym->st_value <= toaddr)

> +    {

> +      riscv_update_pcgp_reloc_symvals (p, sym->st_value, count);


In this place we make sure that only the symbols defined in the same
`sec' will be modified.  But you only pass the symbol value into the
riscv_update_pcgp_reloc_symvals, and use the value to decide if the
pcgp table's symbols are the same one.  I think this is dangerous
since you may modify the symbols which are actually defined in other
sections.

I think `riscv_update_pcgp_reloc_symvals' should be moved outside the
if statement, and you only need to trace the whole pcgp tables once,
with checking the `sym_sec' and `hi_addr', maybe something like this,

if (sym_sec == sec && hi_addr > addr && hi_addr <= toaddr)

> +      sym->st_value -= count;

> +    }

> +

> +  /* If the symbol *spans* the bytes we just deleted (i.e. its

> +     *end* is in the moved bytes but its *start* isn't), then we

> +     must adjust its size.

> +

> +     This test needs to use the original value of st_value, otherwise

> +     we might accidentally decrease size when deleting bytes right

> +     before the symbol.  But since deleted relocs can't span across

> +     symbols, we can't have both a st_value and a st_size decrease,

> +     so it is simpler to just use an else.  */

> +  else if (sym->st_value <= addr

> +   && sym->st_value + sym->st_size > addr

> +   && sym->st_value + sym->st_size <= toaddr)

> +    sym->st_size -= count;

> + }

> +    }

> +

> +  /* Now adjust the global symbols defined in this section.  */

> +  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))

> +      - symtab_hdr->sh_info);

> +

> +  for (i = 0; i < symcount; i++)

> +    {

> +      struct elf_link_hash_entry *sym_hash = sym_hashes[i];

> +

> +      /* The '--wrap SYMBOL' option is causing a pain when the object file,

> + containing the definition of __wrap_SYMBOL, includes a direct

> + call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference

> + the same symbol (which is __wrap_SYMBOL), but still exist as two

> + different symbols in 'sym_hashes', we don't want to adjust

> + the global symbol __wrap_SYMBOL twice.

> +

> + The same problem occurs with symbols that are versioned_hidden, as

> + foo becomes an alias for foo@BAR, and hence they need the same

> + treatment.  */

> +      if (link_info->wrap_hash != NULL

> +  || sym_hash->versioned != unversioned)

> + {

> +  struct elf_link_hash_entry **cur_sym_hashes;

> +

> +  /* Loop only over the symbols which have already been checked.  */

> +  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];

> +       cur_sym_hashes++)

> +    {

> +      /* If the current symbol is identical to 'sym_hash', that means

> + the symbol was already adjusted (or at least checked).  */

> +      if (*cur_sym_hashes == sym_hash)

> + break;

> +    }

> +  /* Don't adjust the symbol again.  */

> +  if (cur_sym_hashes < &sym_hashes[i])

> +    continue;

> + }

> +

> +      if ((sym_hash->root.type == bfd_link_hash_defined

> +   || sym_hash->root.type == bfd_link_hash_defweak)

> +  && sym_hash->root.u.def.section == sec)

> + {

> +  /* As above, adjust the value if needed.  */

> +  if (sym_hash->root.u.def.value > addr

> +      && sym_hash->root.u.def.value <= toaddr)

> +    {

> +      riscv_update_pcgp_reloc_symvals (p, sym_hash->root.u.def.value,

> +       count);


Likewise, just checking the symbol value looks dangerous.

> +      sym_hash->root.u.def.value -= count;

> +    }

> +

> +  /* As above, adjust the size if needed.  */

> +  else if (sym_hash->root.u.def.value <= addr

> +   && sym_hash->root.u.def.value + sym_hash->size > addr

> +   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)

> +    sym_hash->size -= count;

> + }

> +    }

> +

> +  return true;

> +}

> +

>  typedef bool (*relax_func_t) (bfd *, asection *, asection *,

>        struct bfd_link_info *,

>        Elf_Internal_Rela *,

> @@ -4179,7 +4219,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,

> asection *sym_sec,

>         bfd_vma max_alignment,

>         bfd_vma reserve_size ATTRIBUTE_UNUSED,

>         bool *again,

> -       riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

> +       riscv_pcgp_relocs *pcgp_relocs,

>         bool undefined_weak ATTRIBUTE_UNUSED)

>  {

>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

> @@ -4243,7 +4283,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,

> asection *sym_sec,

>    /* Delete unnecessary JALR.  */

>    *again = true;

>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + len, 8 - len,

> -   link_info);

> +   link_info, pcgp_relocs);

>  }

>

>  /* Traverse all output sections and return the max alignment.  */

> @@ -4275,7 +4315,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>        bfd_vma max_alignment,

>        bfd_vma reserve_size,

>        bool *again,

> -      riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

> +      riscv_pcgp_relocs *pcgp_relocs,

>        bool undefined_weak)

>  {

>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

> @@ -4337,7 +4377,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>    rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>    *again = true;

>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

> -   link_info);

> +   link_info, pcgp_relocs);

>

>   default:

>    abort ();

> @@ -4370,7 +4410,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>

>        *again = true;

>        return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + 2, 2,

> -       link_info);

> +       link_info, pcgp_relocs);

>      }

>

>    return true;

> @@ -4388,7 +4428,7 @@ _bfd_riscv_relax_tls_le (bfd *abfd,

>   bfd_vma max_alignment ATTRIBUTE_UNUSED,

>   bfd_vma reserve_size ATTRIBUTE_UNUSED,

>   bool *again,

> - riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED,

> + riscv_pcgp_relocs *prcel_relocs,

>   bool undefined_weak ATTRIBUTE_UNUSED)

>  {

>    /* See if this symbol is in range of tp.  */

> @@ -4411,15 +4451,15 @@ _bfd_riscv_relax_tls_le (bfd *abfd,

>        /* We can delete the unnecessary instruction and reloc.  */

>        rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>        *again = true;

> -      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

> link_info);

> +      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

> link_info,

> +       prcel_relocs);

>

>      default:

>        abort ();

>      }

>  }

>

> -/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.

> -   Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */

> +/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.  */

>

>  static bool

>  _bfd_riscv_relax_align (bfd *abfd, asection *sec,

> @@ -4430,7 +4470,7 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>   bfd_vma max_alignment ATTRIBUTE_UNUSED,

>   bfd_vma reserve_size ATTRIBUTE_UNUSED,

>   bool *again ATTRIBUTE_UNUSED,

> - riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED,

> + riscv_pcgp_relocs *pcgp_relocs,

>   bool undefined_weak ATTRIBUTE_UNUSED)

>  {

>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

> @@ -4442,6 +4482,9 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>    bfd_vma aligned_addr = ((symval - 1) & ~(alignment - 1)) + alignment;

>    bfd_vma nop_bytes = aligned_addr - symval;

>

> +  /* Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */

> +  sec->sec_flg0 = true;

> +

>    /* Make sure there are enough NOPs to actually achieve the alignment.  */

>    if (rel->r_addend < nop_bytes)

>      {

> @@ -4471,13 +4514,13 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>

>    /* Delete the excess bytes.  */

>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + nop_bytes,

> -   rel->r_addend - nop_bytes, link_info);

> +   rel->r_addend - nop_bytes, link_info, pcgp_relocs);

>  }


The alignment relaxations are behind the pcgc relaxations, so we don't
need to update the pcgp tables in fact.

>  /* Relax PC-relative references to GP-relative references.  */

>

>  static bool

> -_bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

> +_bfd_riscv_relax_pc (bfd *abfd,

>       asection *sec,

>       asection *sym_sec,

>       struct bfd_link_info *link_info,

> @@ -4485,7 +4528,7 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>       bfd_vma symval,

>       bfd_vma max_alignment,

>       bfd_vma reserve_size,

> -     bool *again ATTRIBUTE_UNUSED,

> +     bool *again,

>       riscv_pcgp_relocs *pcgp_relocs,

>       bool undefined_weak)

>  {

> @@ -4614,9 +4657,11 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>        sym_sec,

>        undefined_weak);

>    /* We can delete the unnecessary AUIPC and reloc.  */

> -  rel->r_info = ELFNN_R_INFO (0, R_RISCV_DELETE);

> -  rel->r_addend = 4;

> -  return true;

> +

> +  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

> +  *again = true;

> +  return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4, link_info,

> +   pcgp_relocs);

>

>   default:

>    abort ();

> @@ -4626,29 +4671,6 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>    return true;

>  }

>

> -/* Delete the bytes for R_RISCV_DELETE.  */

> -

> -static bool

> -_bfd_riscv_relax_delete (bfd *abfd,

> - asection *sec,

> - asection *sym_sec ATTRIBUTE_UNUSED,

> - struct bfd_link_info *link_info,

> - Elf_Internal_Rela *rel,

> - bfd_vma symval ATTRIBUTE_UNUSED,

> - bfd_vma max_alignment ATTRIBUTE_UNUSED,

> - bfd_vma reserve_size ATTRIBUTE_UNUSED,

> - bool *again,

> - riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

> - bool undefined_weak ATTRIBUTE_UNUSED)

> -{

> -  if (!riscv_relax_delete_bytes (abfd, sec, rel->r_offset, rel->r_addend,

> - link_info))

> -    return false;

> -  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

> -  *again = true;

> -  return true;

> -}

> -


Likewise, I would suggest keeping the _bfd_riscv_relax_delete.

>  /* Called by after_allocation to set the information of data segment

>     before relaxing.  */

>

> @@ -4660,35 +4682,10 @@ bfd_elfNN_riscv_set_data_segment_info (struct

> bfd_link_info *info,

>    htab->data_segment_phase = data_segment_phase;

>  }

>

> -/* Called by after_allocation to check if we need to run the whole

> -   relaxations again.  */

> -

> -bool

> -bfd_elfNN_riscv_restart_relax_sections (struct bfd_link_info *info)

> -{

> -  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);

> -  bool restart = htab->restart_relax;

> -  /* Reset the flag.  */

> -  htab->restart_relax = false;

> -  return restart;

> -}

> -

>  /* Relax a section.

>

> -   Pass 0: Shortens code sequences for LUI/CALL/TPREL relocs.

> -   Pass 1: Shortens code sequences for PCREL relocs.

> -   Pass 2: Deletes the bytes that pass 1 made obsolete.

> -   Pass 3: Which cannot be disabled, handles code alignment directives.

> -

> -   The `again` is used to determine whether the relax pass itself needs to

> -   run again.  And the `restart_relax` is used to determine if we need to

> -   run the whole relax passes again from 0 to 2.  Once we have deleted the

> -   code between relax pass 0 to 2, the restart_relax will be set to TRUE,

> -   and we should run the whole relaxations again to give them more chances

> -   to shorten the code.

> -

> -   Since we can't relax anything else once we start to handle the

> alignments,

> -   we will only enter into the relax pass 3 when the restart_relax is

> FALSE.  */

> +   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.

> +   Pass 1: Which cannot be disabled, handles code alignment directives.  */

>

>  static bool

>  _bfd_riscv_relax_section (bfd *abfd, asection *sec,

> @@ -4707,12 +4704,11 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>    *again = false;

>

>    if (bfd_link_relocatable (info)

> +      || sec->sec_flg0

>        || (sec->flags & SEC_RELOC) == 0

>        || sec->reloc_count == 0

>        || (info->disable_target_specific_optimizations

> -  && info->relax_pass < 2)

> -      || (htab->restart_relax

> -  && info->relax_pass == 3)

> +  && info->relax_pass == 0)

>        /* The exp_seg_relro_adjust is enum phase_enum (0x4),

>   and defined in ld/ldexp.h.  */

>        || *(htab->data_segment_phase) == 4)

> @@ -4757,31 +4753,27 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>        || type == R_RISCV_CALL_PLT)

>      relax_func = _bfd_riscv_relax_call;

>    else if (type == R_RISCV_HI20

> -   || type == R_RISCV_LO12_I

> -   || type == R_RISCV_LO12_S)

> +    || type == R_RISCV_LO12_I

> +    || type == R_RISCV_LO12_S)

>      relax_func = _bfd_riscv_relax_lui;

>    else if (type == R_RISCV_TPREL_HI20

> -   || type == R_RISCV_TPREL_ADD

> -   || type == R_RISCV_TPREL_LO12_I

> -   || type == R_RISCV_TPREL_LO12_S)

> +    || type == R_RISCV_TPREL_ADD

> +    || type == R_RISCV_TPREL_LO12_I

> +    || type == R_RISCV_TPREL_LO12_S)

>      relax_func = _bfd_riscv_relax_tls_le;

> +  else if (!bfd_link_pic (info) && (type == R_RISCV_PCREL_HI20

> +    || type == R_RISCV_PCREL_LO12_I

> +    || type == R_RISCV_PCREL_LO12_S))

> +    relax_func = _bfd_riscv_relax_pc;

>    else

>      continue;

>   }

> -      else if (info->relax_pass == 1

> -       && !bfd_link_pic (info)

> -       && (type == R_RISCV_PCREL_HI20

> -   || type == R_RISCV_PCREL_LO12_I

> -   || type == R_RISCV_PCREL_LO12_S))

> - relax_func = _bfd_riscv_relax_pc;

> -      else if (info->relax_pass == 2 && type == R_RISCV_DELETE)

> - relax_func = _bfd_riscv_relax_delete;

> -      else if (info->relax_pass == 3 && type == R_RISCV_ALIGN)

> +      else if (info->relax_pass == 1 && type == R_RISCV_ALIGN)

>   relax_func = _bfd_riscv_relax_align;

>        else

>   continue;

>

> -      if (info->relax_pass < 2)

> +      if (info->relax_pass == 0)

>   {

>    /* Only relax this reloc if it is paired with R_RISCV_RELAX.  */

>    if (i == sec->reloc_count - 1

> @@ -4954,9 +4946,6 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>      free (relocs);

>    riscv_free_pcgp_relocs (&pcgp_relocs, abfd, sec);

>

> -  if (*again)

> -    htab->restart_relax = true;

> -

>    return ret;

>  }

>

> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h

> index e691a97ecda..3af8fd99d9a 100644

> --- a/bfd/elfxx-riscv.h

> +++ b/bfd/elfxx-riscv.h

> @@ -92,11 +92,6 @@ riscv_estimate_digit (unsigned);

>  extern int

>  riscv_compare_subsets (const char *, const char *);

>

> -extern bool

> -bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);

> -extern bool

> -bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);

> -

>  extern void

>  bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);

>  extern void

> diff --git a/ld/ChangeLog b/ld/ChangeLog

> index 808191bd14e..9cfcdd88754 100644

> --- a/ld/ChangeLog

> +++ b/ld/ChangeLog

> @@ -1,3 +1,8 @@

> +2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>

> +

> + * emultempl/riscvelf.em: Revert restart_relax changes and set

> + relax_pass to 2.

> +

>  2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>

>

>   * scripttempl/pru.sc (.resource_table): Align the output

> diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em

> index c625a631fe5..865e3abd494 100644

> --- a/ld/emultempl/riscvelf.em

> +++ b/ld/emultempl/riscvelf.em

> @@ -42,7 +42,7 @@ riscv_elf_before_allocation (void)

>   ENABLE_RELAXATION;

>      }

>

> -  link_info.relax_pass = 4;

> +  link_info.relax_pass = 2;

>  }

>

>  static void

> @@ -76,11 +76,7 @@ gld${EMULATION_NAME}_after_allocation (void)

>    enum phase_enum *phase = &(expld.dataseg.phase);

>    bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *)

> phase);

>

> -  do

> -    {

> -      ldelf_map_segments (need_layout);

> -    }

> -  while (bfd_elf${ELFSIZE}_riscv_restart_relax_sections (&link_info));

> +  ldelf_map_segments (need_layout);

>  }

>

>  /* This is a convenient point to tell BFD about target specific flags.

> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d

> b/ld/testsuite/ld-riscv-elf/align-small-region.d

> new file mode 100644

> index 00000000000..3799129c665

> --- /dev/null

> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.d

> @@ -0,0 +1,12 @@

> +#source: align-small-region.s

> +#as: -march=rv32i

> +#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100

> +#objdump: -d

> +

> +.*:[ ]+file format .*

> +

> +Disassembly of section \.entry:

> +

> +00000000 <_reset>:

> +.*:[ ]+[0-9a-f]+[ ]+j[ ]+100[ ]+<_start>

> +#pass

> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld

> b/ld/testsuite/ld-riscv-elf/align-small-region.ld

> new file mode 100644

> index 00000000000..6a3e6638b26

> --- /dev/null

> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld

> @@ -0,0 +1,12 @@

> +MEMORY

> +{

> + reset : ORIGIN = 0x0, LENGTH = 32

> +}

> +

> +SECTIONS

> +{

> + .entry :

> + {

> + KEEP (*(.entry))

> + } > reset

> +}

> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s

> b/ld/testsuite/ld-riscv-elf/align-small-region.s

> new file mode 100644

> index 00000000000..ace81d640ed

> --- /dev/null

> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.s

> @@ -0,0 +1,7 @@

> + .section .entry, "xa"

> + .align 5

> + .globl _reset

> + .type _reset, @function

> +_reset:

> + tail _start

> + .size _reset, . - _reset

> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

> index 11977611733..739cc04b889 100644

> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

> @@ -119,6 +119,7 @@ proc run_relax_twice_test {} {

>  }

>

>  if [istarget "riscv*-*-*"] {

> +    run_dump_test "align-small-region"

>      run_dump_test "call-relax"

>      run_dump_test "pcgp-relax"

>      run_dump_test "c-lui"

> --

> 2.25.1
Lewis Revill Oct. 8, 2021, 11:17 a.m. | #2
Hi Nelson,

Thanks for the review!

On Fri, 8 Oct 2021 at 09:52, Nelson Chu <nelson.chu@sifive.com> wrote:
>

> This looks interesting since you are removing the relax pass, which

> deletes the bytes that pass 1 made obsolete.  I'm not sure if the

> change will cause any side effects, so I would suggest keeping the

> delete relax pass separating, like what we always did before.


So this pass is no longer necessary since we only used R_RISCV_DELETE to
delay deleting bytes from within the relax_pc pass - because it was
dangerous to delete bytes while still doing the pcrel_hi/pcrel_lo
tracking. Now that this pass adds the capability to safely delete bytes
while still keeping track of pcrel_hi locations and symbols this is no
longer the case. In addition adding that second target pass would mean
that we would be back to needing to solve the restart_relax problem
since we couldn't use 'again' to start the whole process again.

> The returned boolean `updated' looks useless and redundant, so we

> probably can remove it.


Thanks, you're right.

> Likewise.  Besides, I suppose you also need to check if the `sym_sec'

> of riscv_pcgp_relocs is the same as the section in which we are

> deleting the bytes.  That is  - we can only modify the symbol value

> when the symbol section is the same as the deleting section.


Ah yes, good catch!

> In this place we make sure that only the symbols defined in the same

> `sec' will be modified.  But you only pass the symbol value into the

> riscv_update_pcgp_reloc_symvals, and use the value to decide if the

> pcgp table's symbols are the same one.  I think this is dangerous

> since you may modify the symbols which are actually defined in other

> sections.

>

> I think `riscv_update_pcgp_reloc_symvals' should be moved outside the

> if statement, and you only need to trace the whole pcgp tables once,

> with checking the `sym_sec' and `hi_addr', maybe something like this,

>

> if (sym_sec == sec && hi_addr > addr && hi_addr <= toaddr)


Hmm okay I'll see what I can do to update it like this. I think I
understand what you mean by 'only need to trace the whole pcgp tables
once'. So I would be able to just go through the entire table and check
if the deleted bytes occurred before the hi_addr right?

> The alignment relaxations are behind the pcgc relaxations, so we don't

> need to update the pcgp tables in fact.


Good point, I can probably pass null for this pass and check on the
other end.
Lewis Revill Oct. 8, 2021, 12:36 p.m. | #3
Updated patch from feedback:

Commit abd20cb637008da9d32018b4b03973e119388a0a and
ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4 introduced additional
complexity into the paths run by the RISC-V relaxation pass in order to
resolve the issue of accurately keeping track of pcrel_hi and pcrel_lo
pairs. The first commit split up relaxation of these relocs into a pass
which occurred after other relaxations in order to prevent the situation
where bytes were deleted in between a pcrel_lo/pcrel_hi pair, inhibiting
our ability to find the corresponding pcrel_hi relocation from the
address attached to the pcrel_lo.

Since the relaxation was split into two passes the 'again' parameter
could not be used to perform the entire relaxation process again and so
the second commit added a way to restart ldelf_map_segments, thus
starting the whole process again.

Unfortunately this process could not account for the fact that we were
not finished with the relaxation process so in some cases - such as the
case where code would not fit in a memory region before the
R_RISCV_ALIGN relocation was relaxed - sanity checks in generic code
would fail.

This patch fixes all three of these concerns by reverting back to a
system of having only one target relax pass but updating entries in the
table of pcrel_hi/pcrel_lo relocs every time any bytes are deleted. Thus
we can keep track of the pairs accurately, and we can use the 'again'
parameter to restart the entire target relax pass, behaving in the way
that generic code expects.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=28410

bfd/
* elfnn-riscv.c: Revert restart_relax changes.
(_bfd_riscv_relax_pc): Delete bytes immediately rather than
using R_RISCV_DELETE.
(_bfd_riscv_relax_delete): Remove.
(riscv_relax_delete_bytes): Update tpgp relocs whenever bytes
are deleted.
(riscv_update_pcgp_relocs): Add function to update pcrel_hi
section offsets and symbol values.
(_bfd_riscv_relax_section): Use only one pass for all target
relaxations.
* elfxx-riscv.h: Revert restart_relax changes.

ld/
* emultempl/riscvelf.em: Revert restart_relax changes and set
relax_pass to 2.
---
 bfd/ChangeLog                                 |  14 +
 bfd/elfnn-riscv.c                             | 385 ++++++++----------
 bfd/elfxx-riscv.h                             |   5 -
 ld/ChangeLog                                  |   5 +
 ld/emultempl/riscvelf.em                      |   8 +-
 .../ld-riscv-elf/align-small-region.d         |  12 +
 .../ld-riscv-elf/align-small-region.ld        |  12 +
 .../ld-riscv-elf/align-small-region.s         |   7 +
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |   1 +
 9 files changed, 231 insertions(+), 218 deletions(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 2a08ff7cfb4..0874f3fde72 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,17 @@
+2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * elfnn-riscv.c: Revert restart_relax changes.
+ (_bfd_riscv_relax_pc): Delete bytes immediately rather than
+ using R_RISCV_DELETE.
+ (_bfd_riscv_relax_delete): Remove.
+ (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes
+ are deleted.
+ (riscv_update_pcgp_relocs): Add function to update pcrel_hi
+ section offsets and symbol values.
+ (_bfd_riscv_relax_section): Use only one pass for all target
+ relaxations.
+ * elfxx-riscv.h: Revert restart_relax changes.
+
 2021-09-27  Nick Alcock  <nick.alcock@oracle.com>

  * configure: Regenerate.
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 2e8df72fa2a..545e3324411 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -38,9 +38,6 @@
 #define CHAR_BIT 8
 #endif

-/* Internal relocations used exclusively by the relaxation pass.  */
-#define R_RISCV_DELETE (R_RISCV_max + 1)
-
 #define ARCH_SIZE NN

 #define MINUS_ONE ((bfd_vma)0 - 1)
@@ -131,9 +128,6 @@ struct riscv_elf_link_hash_table
   /* The index of the last unused .rel.iplt slot.  */
   bfd_vma last_iplt_index;

-  /* Re-run the relaxations from relax pass 0 if TRUE.  */
-  bool restart_relax;
-
   /* The data segment phase, don't relax the section
      when it is exp_seg_relro_adjust.  */
   int *data_segment_phase;
@@ -405,7 +399,6 @@ riscv_elf_link_hash_table_create (bfd *abfd)
     }

   ret->max_alignment = (bfd_vma) -1;
-  ret->restart_relax = false;

   /* Create hash table for local ifunc.  */
   ret->loc_hash_table = htab_try_create (1024,
@@ -1714,9 +1707,6 @@ perform_relocation (const reloc_howto_type *howto,
     case R_RISCV_TLS_DTPREL64:
       break;

-    case R_RISCV_DELETE:
-      return bfd_reloc_ok;
-
     default:
       return bfd_reloc_notsupported;
     }
@@ -2326,7 +2316,6 @@ riscv_elf_relocate_section (bfd *output_bfd,
  case R_RISCV_SET16:
  case R_RISCV_SET32:
  case R_RISCV_32_PCREL:
- case R_RISCV_DELETE:
   /* These require no special handling beyond perform_relocation.  */
   break;

@@ -3923,115 +3912,6 @@ _bfd_riscv_elf_merge_private_bfd_data (bfd *ibfd,
struct bfd_link_info *info)
   return false;
 }

-/* Delete some bytes from a section while relaxing.  */
-
-static bool
-riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
-  struct bfd_link_info *link_info)
-{
-  unsigned int i, symcount;
-  bfd_vma toaddr = sec->size;
-  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
-  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
-  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
-  struct bfd_elf_section_data *data = elf_section_data (sec);
-  bfd_byte *contents = data->this_hdr.contents;
-
-  /* Actually delete the bytes.  */
-  sec->size -= count;
-  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
-
-  /* Adjust the location of all of the relocs.  Note that we need not
-     adjust the addends, since all PC-relative references must be against
-     symbols, which we will adjust below.  */
-  for (i = 0; i < sec->reloc_count; i++)
-    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
-      data->relocs[i].r_offset -= count;
-
-  /* Adjust the local symbols defined in this section.  */
-  for (i = 0; i < symtab_hdr->sh_info; i++)
-    {
-      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
-      if (sym->st_shndx == sec_shndx)
- {
-  /* If the symbol is in the range of memory we just moved, we
-     have to adjust its value.  */
-  if (sym->st_value > addr && sym->st_value <= toaddr)
-    sym->st_value -= count;
-
-  /* If the symbol *spans* the bytes we just deleted (i.e. its
-     *end* is in the moved bytes but its *start* isn't), then we
-     must adjust its size.
-
-     This test needs to use the original value of st_value, otherwise
-     we might accidentally decrease size when deleting bytes right
-     before the symbol.  But since deleted relocs can't span across
-     symbols, we can't have both a st_value and a st_size decrease,
-     so it is simpler to just use an else.  */
-  else if (sym->st_value <= addr
-   && sym->st_value + sym->st_size > addr
-   && sym->st_value + sym->st_size <= toaddr)
-    sym->st_size -= count;
- }
-    }
-
-  /* Now adjust the global symbols defined in this section.  */
-  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
-      - symtab_hdr->sh_info);
-
-  for (i = 0; i < symcount; i++)
-    {
-      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
-
-      /* The '--wrap SYMBOL' option is causing a pain when the object file,
- containing the definition of __wrap_SYMBOL, includes a direct
- call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
- the same symbol (which is __wrap_SYMBOL), but still exist as two
- different symbols in 'sym_hashes', we don't want to adjust
- the global symbol __wrap_SYMBOL twice.
-
- The same problem occurs with symbols that are versioned_hidden, as
- foo becomes an alias for foo@BAR, and hence they need the same
- treatment.  */
-      if (link_info->wrap_hash != NULL
-  || sym_hash->versioned != unversioned)
- {
-  struct elf_link_hash_entry **cur_sym_hashes;
-
-  /* Loop only over the symbols which have already been checked.  */
-  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
-       cur_sym_hashes++)
-    {
-      /* If the current symbol is identical to 'sym_hash', that means
- the symbol was already adjusted (or at least checked).  */
-      if (*cur_sym_hashes == sym_hash)
- break;
-    }
-  /* Don't adjust the symbol again.  */
-  if (cur_sym_hashes < &sym_hashes[i])
-    continue;
- }
-
-      if ((sym_hash->root.type == bfd_link_hash_defined
-   || sym_hash->root.type == bfd_link_hash_defweak)
-  && sym_hash->root.u.def.section == sec)
- {
-  /* As above, adjust the value if needed.  */
-  if (sym_hash->root.u.def.value > addr
-      && sym_hash->root.u.def.value <= toaddr)
-    sym_hash->root.u.def.value -= count;
-
-  /* As above, adjust the size if needed.  */
-  else if (sym_hash->root.u.def.value <= addr
-   && sym_hash->root.u.def.value + sym_hash->size > addr
-   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
-    sym_hash->size -= count;
- }
-    }
-
-  return true;
-}
-
 /* A second format for recording PC-relative hi relocations.  This stores
the
    information required to relax them to GP-relative addresses.  */

@@ -4162,6 +4042,148 @@ riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p,
bfd_vma hi_sec_off)
   return false;
 }

+static void
+riscv_update_pcgp_relocs (riscv_pcgp_relocs *p, asection *deleted_sec,
+  bfd_vma deleted_addr, size_t deleted_count)
+{
+  /* Bytes have already been deleted and toaddr should match the old
section
+     size for our checks, so adjust it here.  */
+  bfd_vma toaddr = deleted_sec->size + deleted_count;
+  riscv_pcgp_lo_reloc *l;
+  riscv_pcgp_hi_reloc *h;
+
+  /* Update section offsets of corresponding pcrel_hi relocs for the
pcrel_lo
+     entries where they occur after the deleted bytes.  */
+  for (l = p->lo; l != NULL; l = l->next)
+    if (l->hi_sec_off > deleted_addr && l->hi_sec_off < toaddr)
+      l->hi_sec_off -= deleted_count;
+
+  /* Update both section offsets, and symbol values of pcrel_hi relocs
where
+     these values occur after the deleted bytes.  */
+  for (h = p->hi; h != NULL; h = h->next)
+    {
+      if (h->hi_sec_off > deleted_addr && h->hi_sec_off < toaddr)
+ h->hi_sec_off -= deleted_count;
+      if (h->sym_sec == deleted_sec && h->hi_addr > deleted_addr
+  && h->hi_addr < toaddr)
+ h->hi_addr -= deleted_count;
+    }
+}
+
+/* Delete some bytes from a section while relaxing.  */
+
+static bool
+riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
+  struct bfd_link_info *link_info, riscv_pcgp_relocs *p)
+{
+  unsigned int i, symcount;
+  bfd_vma toaddr = sec->size;
+  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
+  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
+  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
+  struct bfd_elf_section_data *data = elf_section_data (sec);
+  bfd_byte *contents = data->this_hdr.contents;
+
+  /* Actually delete the bytes.  */
+  sec->size -= count;
+  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
+
+  /* Adjust the location of all of the relocs.  Note that we need not
+     adjust the addends, since all PC-relative references must be against
+     symbols, which we will adjust below.  */
+  for (i = 0; i < sec->reloc_count; i++)
+    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
+      data->relocs[i].r_offset -= count;
+
+  /* Adjust the hi_sec_off, and the hi_addr of any entries in the pcgp
relocs
+     table for which these values occur after the deleted bytes.  */
+  if (p)
+    riscv_update_pcgp_relocs (p, sec, addr, count);
+
+  /* Adjust the local symbols defined in this section.  */
+  for (i = 0; i < symtab_hdr->sh_info; i++)
+    {
+      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
+      if (sym->st_shndx == sec_shndx)
+ {
+  /* If the symbol is in the range of memory we just moved, we
+     have to adjust its value.  */
+  if (sym->st_value > addr && sym->st_value <= toaddr)
+    sym->st_value -= count;
+
+  /* If the symbol *spans* the bytes we just deleted (i.e. its
+     *end* is in the moved bytes but its *start* isn't), then we
+     must adjust its size.
+
+     This test needs to use the original value of st_value, otherwise
+     we might accidentally decrease size when deleting bytes right
+     before the symbol.  But since deleted relocs can't span across
+     symbols, we can't have both a st_value and a st_size decrease,
+     so it is simpler to just use an else.  */
+  else if (sym->st_value <= addr
+   && sym->st_value + sym->st_size > addr
+   && sym->st_value + sym->st_size <= toaddr)
+    sym->st_size -= count;
+ }
+    }
+
+  /* Now adjust the global symbols defined in this section.  */
+  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
+      - symtab_hdr->sh_info);
+
+  for (i = 0; i < symcount; i++)
+    {
+      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
+
+      /* The '--wrap SYMBOL' option is causing a pain when the object file,
+ containing the definition of __wrap_SYMBOL, includes a direct
+ call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
+ the same symbol (which is __wrap_SYMBOL), but still exist as two
+ different symbols in 'sym_hashes', we don't want to adjust
+ the global symbol __wrap_SYMBOL twice.
+
+ The same problem occurs with symbols that are versioned_hidden, as
+ foo becomes an alias for foo@BAR, and hence they need the same
+ treatment.  */
+      if (link_info->wrap_hash != NULL
+  || sym_hash->versioned != unversioned)
+ {
+  struct elf_link_hash_entry **cur_sym_hashes;
+
+  /* Loop only over the symbols which have already been checked.  */
+  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
+       cur_sym_hashes++)
+    {
+      /* If the current symbol is identical to 'sym_hash', that means
+ the symbol was already adjusted (or at least checked).  */
+      if (*cur_sym_hashes == sym_hash)
+ break;
+    }
+  /* Don't adjust the symbol again.  */
+  if (cur_sym_hashes < &sym_hashes[i])
+    continue;
+ }
+
+      if ((sym_hash->root.type == bfd_link_hash_defined
+   || sym_hash->root.type == bfd_link_hash_defweak)
+  && sym_hash->root.u.def.section == sec)
+ {
+  /* As above, adjust the value if needed.  */
+  if (sym_hash->root.u.def.value > addr
+      && sym_hash->root.u.def.value <= toaddr)
+    sym_hash->root.u.def.value -= count;
+
+  /* As above, adjust the size if needed.  */
+  else if (sym_hash->root.u.def.value <= addr
+   && sym_hash->root.u.def.value + sym_hash->size > addr
+   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
+    sym_hash->size -= count;
+ }
+    }
+
+  return true;
+}
+
 typedef bool (*relax_func_t) (bfd *, asection *, asection *,
       struct bfd_link_info *,
       Elf_Internal_Rela *,
@@ -4179,7 +4201,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
        bfd_vma max_alignment,
        bfd_vma reserve_size ATTRIBUTE_UNUSED,
        bool *again,
-       riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+       riscv_pcgp_relocs *pcgp_relocs,
        bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4243,7 +4265,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
   /* Delete unnecessary JALR.  */
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + len, 8 - len,
-   link_info);
+   link_info, pcgp_relocs);
 }

 /* Traverse all output sections and return the max alignment.  */
@@ -4275,7 +4297,7 @@ _bfd_riscv_relax_lui (bfd *abfd,
       bfd_vma max_alignment,
       bfd_vma reserve_size,
       bool *again,
-      riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+      riscv_pcgp_relocs *pcgp_relocs,
       bool undefined_weak)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4337,7 +4359,7 @@ _bfd_riscv_relax_lui (bfd *abfd,
   rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
-   link_info);
+   link_info, pcgp_relocs);

  default:
   abort ();
@@ -4370,7 +4392,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

       *again = true;
       return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + 2, 2,
-       link_info);
+       link_info, pcgp_relocs);
     }

   return true;
@@ -4388,7 +4410,7 @@ _bfd_riscv_relax_tls_le (bfd *abfd,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again,
- riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *prcel_relocs,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   /* See if this symbol is in range of tp.  */
@@ -4411,15 +4433,15 @@ _bfd_riscv_relax_tls_le (bfd *abfd,
       /* We can delete the unnecessary instruction and reloc.  */
       rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
       *again = true;
-      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info);
+      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info,
+       prcel_relocs);

     default:
       abort ();
     }
 }

-/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.
-   Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.  */

 static bool
 _bfd_riscv_relax_align (bfd *abfd, asection *sec,
@@ -4430,7 +4452,7 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again ATTRIBUTE_UNUSED,
- riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4442,6 +4464,9 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
   bfd_vma aligned_addr = ((symval - 1) & ~(alignment - 1)) + alignment;
   bfd_vma nop_bytes = aligned_addr - symval;

+  /* Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+  sec->sec_flg0 = true;
+
   /* Make sure there are enough NOPs to actually achieve the alignment.  */
   if (rel->r_addend < nop_bytes)
     {
@@ -4471,13 +4496,13 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

   /* Delete the excess bytes.  */
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + nop_bytes,
-   rel->r_addend - nop_bytes, link_info);
+   rel->r_addend - nop_bytes, link_info, NULL);
 }

 /* Relax PC-relative references to GP-relative references.  */

 static bool
-_bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
+_bfd_riscv_relax_pc (bfd *abfd,
      asection *sec,
      asection *sym_sec,
      struct bfd_link_info *link_info,
@@ -4485,7 +4510,7 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
      bfd_vma symval,
      bfd_vma max_alignment,
      bfd_vma reserve_size,
-     bool *again ATTRIBUTE_UNUSED,
+     bool *again,
      riscv_pcgp_relocs *pcgp_relocs,
      bool undefined_weak)
 {
@@ -4614,9 +4639,11 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
       sym_sec,
       undefined_weak);
   /* We can delete the unnecessary AUIPC and reloc.  */
-  rel->r_info = ELFNN_R_INFO (0, R_RISCV_DELETE);
-  rel->r_addend = 4;
-  return true;
+
+  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
+  *again = true;
+  return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4, link_info,
+   pcgp_relocs);

  default:
   abort ();
@@ -4626,29 +4653,6 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
   return true;
 }

-/* Delete the bytes for R_RISCV_DELETE.  */
-
-static bool
-_bfd_riscv_relax_delete (bfd *abfd,
- asection *sec,
- asection *sym_sec ATTRIBUTE_UNUSED,
- struct bfd_link_info *link_info,
- Elf_Internal_Rela *rel,
- bfd_vma symval ATTRIBUTE_UNUSED,
- bfd_vma max_alignment ATTRIBUTE_UNUSED,
- bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bool *again,
- riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
- bool undefined_weak ATTRIBUTE_UNUSED)
-{
-  if (!riscv_relax_delete_bytes (abfd, sec, rel->r_offset, rel->r_addend,
- link_info))
-    return false;
-  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
-  *again = true;
-  return true;
-}
-
 /* Called by after_allocation to set the information of data segment
    before relaxing.  */

@@ -4660,35 +4664,10 @@ bfd_elfNN_riscv_set_data_segment_info (struct
bfd_link_info *info,
   htab->data_segment_phase = data_segment_phase;
 }

-/* Called by after_allocation to check if we need to run the whole
-   relaxations again.  */
-
-bool
-bfd_elfNN_riscv_restart_relax_sections (struct bfd_link_info *info)
-{
-  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
-  bool restart = htab->restart_relax;
-  /* Reset the flag.  */
-  htab->restart_relax = false;
-  return restart;
-}
-
 /* Relax a section.

-   Pass 0: Shortens code sequences for LUI/CALL/TPREL relocs.
-   Pass 1: Shortens code sequences for PCREL relocs.
-   Pass 2: Deletes the bytes that pass 1 made obsolete.
-   Pass 3: Which cannot be disabled, handles code alignment directives.
-
-   The `again` is used to determine whether the relax pass itself needs to
-   run again.  And the `restart_relax` is used to determine if we need to
-   run the whole relax passes again from 0 to 2.  Once we have deleted the
-   code between relax pass 0 to 2, the restart_relax will be set to TRUE,
-   and we should run the whole relaxations again to give them more chances
-   to shorten the code.
-
-   Since we can't relax anything else once we start to handle the
alignments,
-   we will only enter into the relax pass 3 when the restart_relax is
FALSE.  */
+   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.
+   Pass 1: Which cannot be disabled, handles code alignment directives.  */

 static bool
 _bfd_riscv_relax_section (bfd *abfd, asection *sec,
@@ -4707,12 +4686,11 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
   *again = false;

   if (bfd_link_relocatable (info)
+      || sec->sec_flg0
       || (sec->flags & SEC_RELOC) == 0
       || sec->reloc_count == 0
       || (info->disable_target_specific_optimizations
-  && info->relax_pass < 2)
-      || (htab->restart_relax
-  && info->relax_pass == 3)
+  && info->relax_pass == 0)
       /* The exp_seg_relro_adjust is enum phase_enum (0x4),
  and defined in ld/ldexp.h.  */
       || *(htab->data_segment_phase) == 4)
@@ -4757,31 +4735,27 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
       || type == R_RISCV_CALL_PLT)
     relax_func = _bfd_riscv_relax_call;
   else if (type == R_RISCV_HI20
-   || type == R_RISCV_LO12_I
-   || type == R_RISCV_LO12_S)
+    || type == R_RISCV_LO12_I
+    || type == R_RISCV_LO12_S)
     relax_func = _bfd_riscv_relax_lui;
   else if (type == R_RISCV_TPREL_HI20
-   || type == R_RISCV_TPREL_ADD
-   || type == R_RISCV_TPREL_LO12_I
-   || type == R_RISCV_TPREL_LO12_S)
+    || type == R_RISCV_TPREL_ADD
+    || type == R_RISCV_TPREL_LO12_I
+    || type == R_RISCV_TPREL_LO12_S)
     relax_func = _bfd_riscv_relax_tls_le;
+  else if (!bfd_link_pic (info) && (type == R_RISCV_PCREL_HI20
+    || type == R_RISCV_PCREL_LO12_I
+    || type == R_RISCV_PCREL_LO12_S))
+    relax_func = _bfd_riscv_relax_pc;
   else
     continue;
  }
-      else if (info->relax_pass == 1
-       && !bfd_link_pic (info)
-       && (type == R_RISCV_PCREL_HI20
-   || type == R_RISCV_PCREL_LO12_I
-   || type == R_RISCV_PCREL_LO12_S))
- relax_func = _bfd_riscv_relax_pc;
-      else if (info->relax_pass == 2 && type == R_RISCV_DELETE)
- relax_func = _bfd_riscv_relax_delete;
-      else if (info->relax_pass == 3 && type == R_RISCV_ALIGN)
+      else if (info->relax_pass == 1 && type == R_RISCV_ALIGN)
  relax_func = _bfd_riscv_relax_align;
       else
  continue;

-      if (info->relax_pass < 2)
+      if (info->relax_pass == 0)
  {
   /* Only relax this reloc if it is paired with R_RISCV_RELAX.  */
   if (i == sec->reloc_count - 1
@@ -4954,9 +4928,6 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
     free (relocs);
   riscv_free_pcgp_relocs (&pcgp_relocs, abfd, sec);

-  if (*again)
-    htab->restart_relax = true;
-
   return ret;
 }

diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
index e691a97ecda..3af8fd99d9a 100644
--- a/bfd/elfxx-riscv.h
+++ b/bfd/elfxx-riscv.h
@@ -92,11 +92,6 @@ riscv_estimate_digit (unsigned);
 extern int
 riscv_compare_subsets (const char *, const char *);

-extern bool
-bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);
-extern bool
-bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);
-
 extern void
 bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);
 extern void
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 808191bd14e..9cfcdd88754 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,8 @@
+2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * emultempl/riscvelf.em: Revert restart_relax changes and set
+ relax_pass to 2.
+
 2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>

  * scripttempl/pru.sc (.resource_table): Align the output
diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
index c625a631fe5..865e3abd494 100644
--- a/ld/emultempl/riscvelf.em
+++ b/ld/emultempl/riscvelf.em
@@ -42,7 +42,7 @@ riscv_elf_before_allocation (void)
  ENABLE_RELAXATION;
     }

-  link_info.relax_pass = 4;
+  link_info.relax_pass = 2;
 }

 static void
@@ -76,11 +76,7 @@ gld${EMULATION_NAME}_after_allocation (void)
   enum phase_enum *phase = &(expld.dataseg.phase);
   bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *)
phase);

-  do
-    {
-      ldelf_map_segments (need_layout);
-    }
-  while (bfd_elf${ELFSIZE}_riscv_restart_relax_sections (&link_info));
+  ldelf_map_segments (need_layout);
 }

 /* This is a convenient point to tell BFD about target specific flags.
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d
b/ld/testsuite/ld-riscv-elf/align-small-region.d
new file mode 100644
index 00000000000..3799129c665
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.d
@@ -0,0 +1,12 @@
+#source: align-small-region.s
+#as: -march=rv32i
+#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100
+#objdump: -d
+
+.*:[ ]+file format .*
+
+Disassembly of section \.entry:
+
+00000000 <_reset>:
+.*:[ ]+[0-9a-f]+[ ]+j[ ]+100[ ]+<_start>
+#pass
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld
b/ld/testsuite/ld-riscv-elf/align-small-region.ld
new file mode 100644
index 00000000000..6a3e6638b26
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld
@@ -0,0 +1,12 @@
+MEMORY
+{
+ reset : ORIGIN = 0x0, LENGTH = 32
+}
+
+SECTIONS
+{
+ .entry :
+ {
+ KEEP (*(.entry))
+ } > reset
+}
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s
b/ld/testsuite/ld-riscv-elf/align-small-region.s
new file mode 100644
index 00000000000..ace81d640ed
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.s
@@ -0,0 +1,7 @@
+ .section .entry, "xa"
+ .align 5
+ .globl _reset
+ .type _reset, @function
+_reset:
+ tail _start
+ .size _reset, . - _reset
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index 11977611733..739cc04b889 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -119,6 +119,7 @@ proc run_relax_twice_test {} {
 }

 if [istarget "riscv*-*-*"] {
+    run_dump_test "align-small-region"
     run_dump_test "call-relax"
     run_dump_test "pcgp-relax"
     run_dump_test "c-lui"
-- 
2.25.1
Palmer Dabbelt Oct. 8, 2021, 4:24 p.m. | #4
On Fri, 08 Oct 2021 01:52:12 PDT (-0700), nelson.chu@sifive.com wrote:
> Hi Lewis,

>

> This patch is actually option 3 rather than option 2 in the pr28410,

> but personally I prefer option 3, so the solution is what I expected,

> thank you.  However, I need more time to run the regressions, and make

> sure the patch won't cause side effects.  But before I start to run

> the regressions, I have a quick review, and do see some places that

> could be improved.


I haven't looked at the patch (and must have missed the earlier 
discussion), but the specific case that I was trying to solve with the 
R_RISCV_DELETE split was something like


    r0: auipc t0, symbol0
    r1: auipc t1, symbol1
        ld    t0, symbol0, r0 # reloc points to r0
        ld    t1, symbol1, r1 # reloc points to r1

in this case, if the auipc at r0 gets deleted then we end up with an 
aliasing issue and need some way to disambiguate which of the two 
symbols we're targeting.  I don't remember if we ever put a test case in 
there for this, but it did manifest in real code (IIRC it was a Linux 
build, but you need to twiddle all the compiler flags so it'll schedule 
the auipc-based pairs).

The idea behing the extra pass here was to split this into two phases, 
so we could resolve the addresses before creating the ambiguities.

>

> On Fri, Oct 8, 2021 at 5:01 AM Lewis Revill <lewis.revill@embecosm.com> wrote:

>>

>> Commit abd20cb637008da9d32018b4b03973e119388a0a and

>> ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4 introduced additional

>> complexity into the paths run by the RISC-V relaxation pass in order to

>> resolve the issue of accurately keeping track of pcrel_hi and pcrel_lo

>> pairs. The first commit split up relaxation of these relocs into a pass

>> which occurred after other relaxations in order to prevent the situation

>> where bytes were deleted in between a pcrel_lo/pcrel_hi pair, inhibiting

>> our ability to find the corresponding pcrel_hi relocation from the

>> address attached to the pcrel_lo.

>>

>> Since the relaxation was split into two passes the 'again' parameter

>> could not be used to perform the entire relaxation process again and so

>> the second commit added a way to restart ldelf_map_segments, thus

>> starting the whole process again.

>>

>> Unfortunately this process could not account for the fact that we were

>> not finished with the relaxation process so in some cases - such as the

>> case where code would not fit in a memory region before the

>> R_RISCV_ALIGN relocation was relaxed - sanity checks in generic code

>> would fail.

>>

>> This patch fixes all three of these concerns by reverting back to a

>> system of having only one target relax pass but updating entries in the

>> table of pcrel_hi/pcrel_lo relocs every time any bytes are deleted. Thus

>> we can keep track of the pairs accurately, and we can use the 'again'

>> parameter to restart the entire target relax pass, behaving in the way

>> that generic code expects.

>>

>> Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=28410

>>

>> bfd/

>> * elfnn-riscv.c: Revert restart_relax changes.

>> (_bfd_riscv_relax_pc): Delete bytes immediately rather than

>> using R_RISCV_DELETE.

>> (_bfd_riscv_relax_delete): Remove.

>> (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes

>> are deleted.

>> (riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi

>> section offsets.

>> (riscv_update_pcgp_reloc_symvals): Add function to update

>> pcrel_hi symbol values.

>> (_bfd_riscv_relax_section): Use only one pass for all target

>> relaxations.

>> * elfxx-riscv.h: Revert restart_relax changes.

>>

>> ld/

>> * emultempl/riscvelf.em: Revert restart_relax changes and set

>> relax_pass to 2.

>> ---

>>  bfd/ChangeLog                                 |  16 +

>>  bfd/elfnn-riscv.c                             | 403 +++++++++---------

>>  bfd/elfxx-riscv.h                             |   5 -

>>  ld/ChangeLog                                  |   5 +

>>  ld/emultempl/riscvelf.em                      |   8 +-

>>  .../ld-riscv-elf/align-small-region.d         |  12 +

>>  .../ld-riscv-elf/align-small-region.ld        |  12 +

>>  .../ld-riscv-elf/align-small-region.s         |   7 +

>>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |   1 +

>>  9 files changed, 251 insertions(+), 218 deletions(-)

>>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d

>>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld

>>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

>>

>> diff --git a/bfd/ChangeLog b/bfd/ChangeLog

>> index 2a08ff7cfb4..7bcba3a090f 100644

>> --- a/bfd/ChangeLog

>> +++ b/bfd/ChangeLog

>> @@ -1,3 +1,19 @@

>> +2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>

>> +

>> + * elfnn-riscv.c: Revert restart_relax changes.

>> + (_bfd_riscv_relax_pc): Delete bytes immediately rather than

>> + using R_RISCV_DELETE.

>> + (_bfd_riscv_relax_delete): Remove.

>> + (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes

>> + are deleted.

>> + (riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi

>> + section offsets.

>> + (riscv_update_pcgp_reloc_symvals): Add function to update

>> + pcrel_hi symbol values.

>> + (_bfd_riscv_relax_section): Use only one pass for all target

>> + relaxations.

>> + * elfxx-riscv.h: Revert restart_relax changes.

>> +

>>  2021-09-27  Nick Alcock  <nick.alcock@oracle.com>

>>

>>   * configure: Regenerate.

>> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c

>> index 2e8df72fa2a..95e2d5536d1 100644

>> --- a/bfd/elfnn-riscv.c

>> +++ b/bfd/elfnn-riscv.c

>> @@ -38,9 +38,6 @@

>>  #define CHAR_BIT 8

>>  #endif

>>

>> -/* Internal relocations used exclusively by the relaxation pass.  */

>> -#define R_RISCV_DELETE (R_RISCV_max + 1)

>> -

>

> This looks interesting since you are removing the relax pass, which

> deletes the bytes that pass 1 made obsolete.  I'm not sure if the

> change will cause any side effects, so I would suggest keeping the

> delete relax pass separating, like what we always did before.

>

>>  #define ARCH_SIZE NN

>>

>>  #define MINUS_ONE ((bfd_vma)0 - 1)

>> @@ -131,9 +128,6 @@ struct riscv_elf_link_hash_table

>>    /* The index of the last unused .rel.iplt slot.  */

>>    bfd_vma last_iplt_index;

>>

>> -  /* Re-run the relaxations from relax pass 0 if TRUE.  */

>> -  bool restart_relax;

>> -

>>    /* The data segment phase, don't relax the section

>>       when it is exp_seg_relro_adjust.  */

>>    int *data_segment_phase;

>> @@ -405,7 +399,6 @@ riscv_elf_link_hash_table_create (bfd *abfd)

>>      }

>>

>>    ret->max_alignment = (bfd_vma) -1;

>> -  ret->restart_relax = false;

>>

>>    /* Create hash table for local ifunc.  */

>>    ret->loc_hash_table = htab_try_create (1024,

>> @@ -1714,9 +1707,6 @@ perform_relocation (const reloc_howto_type *howto,

>>      case R_RISCV_TLS_DTPREL64:

>>        break;

>>

>> -    case R_RISCV_DELETE:

>> -      return bfd_reloc_ok;

>> -

>>      default:

>>        return bfd_reloc_notsupported;

>>      }

>> @@ -2326,7 +2316,6 @@ riscv_elf_relocate_section (bfd *output_bfd,

>>   case R_RISCV_SET16:

>>   case R_RISCV_SET32:

>>   case R_RISCV_32_PCREL:

>> - case R_RISCV_DELETE:

>>    /* These require no special handling beyond perform_relocation.  */

>>    break;

>>

>> @@ -3923,115 +3912,6 @@ _bfd_riscv_elf_merge_private_bfd_data (bfd *ibfd,

>> struct bfd_link_info *info)

>>    return false;

>>  }

>>

>> -/* Delete some bytes from a section while relaxing.  */

>> -

>> -static bool

>> -riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t

>> count,

>> -  struct bfd_link_info *link_info)

>> -{

>> -  unsigned int i, symcount;

>> -  bfd_vma toaddr = sec->size;

>> -  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);

>> -  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;

>> -  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);

>> -  struct bfd_elf_section_data *data = elf_section_data (sec);

>> -  bfd_byte *contents = data->this_hdr.contents;

>> -

>> -  /* Actually delete the bytes.  */

>> -  sec->size -= count;

>> -  memmove (contents + addr, contents + addr + count, toaddr - addr -

>> count);

>> -

>> -  /* Adjust the location of all of the relocs.  Note that we need not

>> -     adjust the addends, since all PC-relative references must be against

>> -     symbols, which we will adjust below.  */

>> -  for (i = 0; i < sec->reloc_count; i++)

>> -    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <

>> toaddr)

>> -      data->relocs[i].r_offset -= count;

>> -

>> -  /* Adjust the local symbols defined in this section.  */

>> -  for (i = 0; i < symtab_hdr->sh_info; i++)

>> -    {

>> -      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +

>> i;

>> -      if (sym->st_shndx == sec_shndx)

>> - {

>> -  /* If the symbol is in the range of memory we just moved, we

>> -     have to adjust its value.  */

>> -  if (sym->st_value > addr && sym->st_value <= toaddr)

>> -    sym->st_value -= count;

>> -

>> -  /* If the symbol *spans* the bytes we just deleted (i.e. its

>> -     *end* is in the moved bytes but its *start* isn't), then we

>> -     must adjust its size.

>> -

>> -     This test needs to use the original value of st_value, otherwise

>> -     we might accidentally decrease size when deleting bytes right

>> -     before the symbol.  But since deleted relocs can't span across

>> -     symbols, we can't have both a st_value and a st_size decrease,

>> -     so it is simpler to just use an else.  */

>> -  else if (sym->st_value <= addr

>> -   && sym->st_value + sym->st_size > addr

>> -   && sym->st_value + sym->st_size <= toaddr)

>> -    sym->st_size -= count;

>> - }

>> -    }

>> -

>> -  /* Now adjust the global symbols defined in this section.  */

>> -  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))

>> -      - symtab_hdr->sh_info);

>> -

>> -  for (i = 0; i < symcount; i++)

>> -    {

>> -      struct elf_link_hash_entry *sym_hash = sym_hashes[i];

>> -

>> -      /* The '--wrap SYMBOL' option is causing a pain when the object file,

>> - containing the definition of __wrap_SYMBOL, includes a direct

>> - call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference

>> - the same symbol (which is __wrap_SYMBOL), but still exist as two

>> - different symbols in 'sym_hashes', we don't want to adjust

>> - the global symbol __wrap_SYMBOL twice.

>> -

>> - The same problem occurs with symbols that are versioned_hidden, as

>> - foo becomes an alias for foo@BAR, and hence they need the same

>> - treatment.  */

>> -      if (link_info->wrap_hash != NULL

>> -  || sym_hash->versioned != unversioned)

>> - {

>> -  struct elf_link_hash_entry **cur_sym_hashes;

>> -

>> -  /* Loop only over the symbols which have already been checked.  */

>> -  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];

>> -       cur_sym_hashes++)

>> -    {

>> -      /* If the current symbol is identical to 'sym_hash', that means

>> - the symbol was already adjusted (or at least checked).  */

>> -      if (*cur_sym_hashes == sym_hash)

>> - break;

>> -    }

>> -  /* Don't adjust the symbol again.  */

>> -  if (cur_sym_hashes < &sym_hashes[i])

>> -    continue;

>> - }

>> -

>> -      if ((sym_hash->root.type == bfd_link_hash_defined

>> -   || sym_hash->root.type == bfd_link_hash_defweak)

>> -  && sym_hash->root.u.def.section == sec)

>> - {

>> -  /* As above, adjust the value if needed.  */

>> -  if (sym_hash->root.u.def.value > addr

>> -      && sym_hash->root.u.def.value <= toaddr)

>> -    sym_hash->root.u.def.value -= count;

>> -

>> -  /* As above, adjust the size if needed.  */

>> -  else if (sym_hash->root.u.def.value <= addr

>> -   && sym_hash->root.u.def.value + sym_hash->size > addr

>> -   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)

>> -    sym_hash->size -= count;

>> - }

>> -    }

>> -

>> -  return true;

>> -}

>> -

>>  /* A second format for recording PC-relative hi relocations.  This stores

>> the

>>     information required to relax them to GP-relative addresses.  */

>>

>> @@ -4162,6 +4042,166 @@ riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p,

>> bfd_vma hi_sec_off)

>>    return false;

>>  }

>>

>> +static bool

>> +riscv_update_pcgp_reloc_offs (riscv_pcgp_relocs *p, bfd_vma

>> deleted_sec_off,

>> +      size_t deleted_count)

>> +{

>> +  riscv_pcgp_lo_reloc *l;

>> +  riscv_pcgp_hi_reloc *h;

>> +  bool updated = false;

>> +

>> +  for (l = p->lo; l != NULL; l = l->next)

>> +    if (l->hi_sec_off > deleted_sec_off)

>> +      {

>> + l->hi_sec_off -= deleted_count;

>> + updated = true;

>> +      }

>> +

>> +  for (h = p->hi; h != NULL; h = h->next)

>> +    if (h->hi_sec_off > deleted_sec_off)

>> +      {

>> + h->hi_sec_off -= deleted_count;

>> + updated = true;

>> +      }

>> +

>> +  return updated;

>> +}

>

> The returned boolean `updated' looks useless and redundant, so we

> probably can remove it.

>

>> +static bool

>> +riscv_update_pcgp_reloc_symvals(riscv_pcgp_relocs *p, bfd_vma old_value,

>> + size_t deleted_count)

>> +{

>> +  riscv_pcgp_hi_reloc *h;

>> +  bool updated = false;

>> +

>> +  for (h = p->hi; h != NULL; h = h->next)

>> +    if (h->hi_addr == old_value)

>> +      {

>> + h->hi_addr -= deleted_count;

>> + updated = true;

>> +      }

>> +  return updated;

>> +}

>

> Likewise.  Besides, I suppose you also need to check if the `sym_sec'

> of riscv_pcgp_relocs is the same as the section in which we are

> deleting the bytes.  That is  - we can only modify the symbol value

> when the symbol section is the same as the deleting section.

>

>> +/* Delete some bytes from a section while relaxing.  */

>> +

>> +static bool

>> +riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t

>> count,

>> +  struct bfd_link_info *link_info, riscv_pcgp_relocs *p)

>> +{

>> +  unsigned int i, symcount;

>> +  bfd_vma toaddr = sec->size;

>> +  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);

>> +  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;

>> +  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);

>> +  struct bfd_elf_section_data *data = elf_section_data (sec);

>> +  bfd_byte *contents = data->this_hdr.contents;

>> +

>> +  /* Actually delete the bytes.  */

>> +  sec->size -= count;

>> +  memmove (contents + addr, contents + addr + count, toaddr - addr -

>> count);

>> +

>> +  /* Adjust the location of all of the relocs.  Note that we need not

>> +     adjust the addends, since all PC-relative references must be against

>> +     symbols, which we will adjust below.  */

>> +  for (i = 0; i < sec->reloc_count; i++)

>> +    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <

>> toaddr)

>> +      data->relocs[i].r_offset -= count;

>> +

>> +  /* Adjust the hi_sec_off of entries in the pcgp relocs table.  */

>> +  riscv_update_pcgp_reloc_offs (p, addr, count);

>> +

>> +  /* Adjust the local symbols defined in this section.  */

>> +  for (i = 0; i < symtab_hdr->sh_info; i++)

>> +    {

>> +      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +

>> i;

>> +      if (sym->st_shndx == sec_shndx)

>> + {

>> +  /* If the symbol is in the range of memory we just moved, we

>> +     have to adjust its value.  */

>> +  if (sym->st_value > addr && sym->st_value <= toaddr)

>> +    {

>> +      riscv_update_pcgp_reloc_symvals (p, sym->st_value, count);

>

> In this place we make sure that only the symbols defined in the same

> `sec' will be modified.  But you only pass the symbol value into the

> riscv_update_pcgp_reloc_symvals, and use the value to decide if the

> pcgp table's symbols are the same one.  I think this is dangerous

> since you may modify the symbols which are actually defined in other

> sections.

>

> I think `riscv_update_pcgp_reloc_symvals' should be moved outside the

> if statement, and you only need to trace the whole pcgp tables once,

> with checking the `sym_sec' and `hi_addr', maybe something like this,

>

> if (sym_sec == sec && hi_addr > addr && hi_addr <= toaddr)

>

>> +      sym->st_value -= count;

>> +    }

>> +

>> +  /* If the symbol *spans* the bytes we just deleted (i.e. its

>> +     *end* is in the moved bytes but its *start* isn't), then we

>> +     must adjust its size.

>> +

>> +     This test needs to use the original value of st_value, otherwise

>> +     we might accidentally decrease size when deleting bytes right

>> +     before the symbol.  But since deleted relocs can't span across

>> +     symbols, we can't have both a st_value and a st_size decrease,

>> +     so it is simpler to just use an else.  */

>> +  else if (sym->st_value <= addr

>> +   && sym->st_value + sym->st_size > addr

>> +   && sym->st_value + sym->st_size <= toaddr)

>> +    sym->st_size -= count;

>> + }

>> +    }

>> +

>> +  /* Now adjust the global symbols defined in this section.  */

>> +  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))

>> +      - symtab_hdr->sh_info);

>> +

>> +  for (i = 0; i < symcount; i++)

>> +    {

>> +      struct elf_link_hash_entry *sym_hash = sym_hashes[i];

>> +

>> +      /* The '--wrap SYMBOL' option is causing a pain when the object file,

>> + containing the definition of __wrap_SYMBOL, includes a direct

>> + call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference

>> + the same symbol (which is __wrap_SYMBOL), but still exist as two

>> + different symbols in 'sym_hashes', we don't want to adjust

>> + the global symbol __wrap_SYMBOL twice.

>> +

>> + The same problem occurs with symbols that are versioned_hidden, as

>> + foo becomes an alias for foo@BAR, and hence they need the same

>> + treatment.  */

>> +      if (link_info->wrap_hash != NULL

>> +  || sym_hash->versioned != unversioned)

>> + {

>> +  struct elf_link_hash_entry **cur_sym_hashes;

>> +

>> +  /* Loop only over the symbols which have already been checked.  */

>> +  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];

>> +       cur_sym_hashes++)

>> +    {

>> +      /* If the current symbol is identical to 'sym_hash', that means

>> + the symbol was already adjusted (or at least checked).  */

>> +      if (*cur_sym_hashes == sym_hash)

>> + break;

>> +    }

>> +  /* Don't adjust the symbol again.  */

>> +  if (cur_sym_hashes < &sym_hashes[i])

>> +    continue;

>> + }

>> +

>> +      if ((sym_hash->root.type == bfd_link_hash_defined

>> +   || sym_hash->root.type == bfd_link_hash_defweak)

>> +  && sym_hash->root.u.def.section == sec)

>> + {

>> +  /* As above, adjust the value if needed.  */

>> +  if (sym_hash->root.u.def.value > addr

>> +      && sym_hash->root.u.def.value <= toaddr)

>> +    {

>> +      riscv_update_pcgp_reloc_symvals (p, sym_hash->root.u.def.value,

>> +       count);

>

> Likewise, just checking the symbol value looks dangerous.

>

>> +      sym_hash->root.u.def.value -= count;

>> +    }

>> +

>> +  /* As above, adjust the size if needed.  */

>> +  else if (sym_hash->root.u.def.value <= addr

>> +   && sym_hash->root.u.def.value + sym_hash->size > addr

>> +   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)

>> +    sym_hash->size -= count;

>> + }

>> +    }

>> +

>> +  return true;

>> +}

>> +

>>  typedef bool (*relax_func_t) (bfd *, asection *, asection *,

>>        struct bfd_link_info *,

>>        Elf_Internal_Rela *,

>> @@ -4179,7 +4219,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,

>> asection *sym_sec,

>>         bfd_vma max_alignment,

>>         bfd_vma reserve_size ATTRIBUTE_UNUSED,

>>         bool *again,

>> -       riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

>> +       riscv_pcgp_relocs *pcgp_relocs,

>>         bool undefined_weak ATTRIBUTE_UNUSED)

>>  {

>>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

>> @@ -4243,7 +4283,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,

>> asection *sym_sec,

>>    /* Delete unnecessary JALR.  */

>>    *again = true;

>>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + len, 8 - len,

>> -   link_info);

>> +   link_info, pcgp_relocs);

>>  }

>>

>>  /* Traverse all output sections and return the max alignment.  */

>> @@ -4275,7 +4315,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>>        bfd_vma max_alignment,

>>        bfd_vma reserve_size,

>>        bool *again,

>> -      riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

>> +      riscv_pcgp_relocs *pcgp_relocs,

>>        bool undefined_weak)

>>  {

>>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

>> @@ -4337,7 +4377,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>>    rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>>    *again = true;

>>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

>> -   link_info);

>> +   link_info, pcgp_relocs);

>>

>>   default:

>>    abort ();

>> @@ -4370,7 +4410,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

>>

>>        *again = true;

>>        return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + 2, 2,

>> -       link_info);

>> +       link_info, pcgp_relocs);

>>      }

>>

>>    return true;

>> @@ -4388,7 +4428,7 @@ _bfd_riscv_relax_tls_le (bfd *abfd,

>>   bfd_vma max_alignment ATTRIBUTE_UNUSED,

>>   bfd_vma reserve_size ATTRIBUTE_UNUSED,

>>   bool *again,

>> - riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED,

>> + riscv_pcgp_relocs *prcel_relocs,

>>   bool undefined_weak ATTRIBUTE_UNUSED)

>>  {

>>    /* See if this symbol is in range of tp.  */

>> @@ -4411,15 +4451,15 @@ _bfd_riscv_relax_tls_le (bfd *abfd,

>>        /* We can delete the unnecessary instruction and reloc.  */

>>        rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>>        *again = true;

>> -      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

>> link_info);

>> +      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,

>> link_info,

>> +       prcel_relocs);

>>

>>      default:

>>        abort ();

>>      }

>>  }

>>

>> -/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.

>> -   Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */

>> +/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.  */

>>

>>  static bool

>>  _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>> @@ -4430,7 +4470,7 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>>   bfd_vma max_alignment ATTRIBUTE_UNUSED,

>>   bfd_vma reserve_size ATTRIBUTE_UNUSED,

>>   bool *again ATTRIBUTE_UNUSED,

>> - riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED,

>> + riscv_pcgp_relocs *pcgp_relocs,

>>   bool undefined_weak ATTRIBUTE_UNUSED)

>>  {

>>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;

>> @@ -4442,6 +4482,9 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>>    bfd_vma aligned_addr = ((symval - 1) & ~(alignment - 1)) + alignment;

>>    bfd_vma nop_bytes = aligned_addr - symval;

>>

>> +  /* Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */

>> +  sec->sec_flg0 = true;

>> +

>>    /* Make sure there are enough NOPs to actually achieve the alignment.  */

>>    if (rel->r_addend < nop_bytes)

>>      {

>> @@ -4471,13 +4514,13 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

>>

>>    /* Delete the excess bytes.  */

>>    return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + nop_bytes,

>> -   rel->r_addend - nop_bytes, link_info);

>> +   rel->r_addend - nop_bytes, link_info, pcgp_relocs);

>>  }

>

> The alignment relaxations are behind the pcgc relaxations, so we don't

> need to update the pcgp tables in fact.

>

>>  /* Relax PC-relative references to GP-relative references.  */

>>

>>  static bool

>> -_bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>> +_bfd_riscv_relax_pc (bfd *abfd,

>>       asection *sec,

>>       asection *sym_sec,

>>       struct bfd_link_info *link_info,

>> @@ -4485,7 +4528,7 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>>       bfd_vma symval,

>>       bfd_vma max_alignment,

>>       bfd_vma reserve_size,

>> -     bool *again ATTRIBUTE_UNUSED,

>> +     bool *again,

>>       riscv_pcgp_relocs *pcgp_relocs,

>>       bool undefined_weak)

>>  {

>> @@ -4614,9 +4657,11 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>>        sym_sec,

>>        undefined_weak);

>>    /* We can delete the unnecessary AUIPC and reloc.  */

>> -  rel->r_info = ELFNN_R_INFO (0, R_RISCV_DELETE);

>> -  rel->r_addend = 4;

>> -  return true;

>> +

>> +  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>> +  *again = true;

>> +  return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4, link_info,

>> +   pcgp_relocs);

>>

>>   default:

>>    abort ();

>> @@ -4626,29 +4671,6 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,

>>    return true;

>>  }

>>

>> -/* Delete the bytes for R_RISCV_DELETE.  */

>> -

>> -static bool

>> -_bfd_riscv_relax_delete (bfd *abfd,

>> - asection *sec,

>> - asection *sym_sec ATTRIBUTE_UNUSED,

>> - struct bfd_link_info *link_info,

>> - Elf_Internal_Rela *rel,

>> - bfd_vma symval ATTRIBUTE_UNUSED,

>> - bfd_vma max_alignment ATTRIBUTE_UNUSED,

>> - bfd_vma reserve_size ATTRIBUTE_UNUSED,

>> - bool *again,

>> - riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,

>> - bool undefined_weak ATTRIBUTE_UNUSED)

>> -{

>> -  if (!riscv_relax_delete_bytes (abfd, sec, rel->r_offset, rel->r_addend,

>> - link_info))

>> -    return false;

>> -  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);

>> -  *again = true;

>> -  return true;

>> -}

>> -

>

> Likewise, I would suggest keeping the _bfd_riscv_relax_delete.

>

>>  /* Called by after_allocation to set the information of data segment

>>     before relaxing.  */

>>

>> @@ -4660,35 +4682,10 @@ bfd_elfNN_riscv_set_data_segment_info (struct

>> bfd_link_info *info,

>>    htab->data_segment_phase = data_segment_phase;

>>  }

>>

>> -/* Called by after_allocation to check if we need to run the whole

>> -   relaxations again.  */

>> -

>> -bool

>> -bfd_elfNN_riscv_restart_relax_sections (struct bfd_link_info *info)

>> -{

>> -  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);

>> -  bool restart = htab->restart_relax;

>> -  /* Reset the flag.  */

>> -  htab->restart_relax = false;

>> -  return restart;

>> -}

>> -

>>  /* Relax a section.

>>

>> -   Pass 0: Shortens code sequences for LUI/CALL/TPREL relocs.

>> -   Pass 1: Shortens code sequences for PCREL relocs.

>> -   Pass 2: Deletes the bytes that pass 1 made obsolete.

>> -   Pass 3: Which cannot be disabled, handles code alignment directives.

>> -

>> -   The `again` is used to determine whether the relax pass itself needs to

>> -   run again.  And the `restart_relax` is used to determine if we need to

>> -   run the whole relax passes again from 0 to 2.  Once we have deleted the

>> -   code between relax pass 0 to 2, the restart_relax will be set to TRUE,

>> -   and we should run the whole relaxations again to give them more chances

>> -   to shorten the code.

>> -

>> -   Since we can't relax anything else once we start to handle the

>> alignments,

>> -   we will only enter into the relax pass 3 when the restart_relax is

>> FALSE.  */

>> +   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.

>> +   Pass 1: Which cannot be disabled, handles code alignment directives.  */

>>

>>  static bool

>>  _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>> @@ -4707,12 +4704,11 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>>    *again = false;

>>

>>    if (bfd_link_relocatable (info)

>> +      || sec->sec_flg0

>>        || (sec->flags & SEC_RELOC) == 0

>>        || sec->reloc_count == 0

>>        || (info->disable_target_specific_optimizations

>> -  && info->relax_pass < 2)

>> -      || (htab->restart_relax

>> -  && info->relax_pass == 3)

>> +  && info->relax_pass == 0)

>>        /* The exp_seg_relro_adjust is enum phase_enum (0x4),

>>   and defined in ld/ldexp.h.  */

>>        || *(htab->data_segment_phase) == 4)

>> @@ -4757,31 +4753,27 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>>        || type == R_RISCV_CALL_PLT)

>>      relax_func = _bfd_riscv_relax_call;

>>    else if (type == R_RISCV_HI20

>> -   || type == R_RISCV_LO12_I

>> -   || type == R_RISCV_LO12_S)

>> +    || type == R_RISCV_LO12_I

>> +    || type == R_RISCV_LO12_S)

>>      relax_func = _bfd_riscv_relax_lui;

>>    else if (type == R_RISCV_TPREL_HI20

>> -   || type == R_RISCV_TPREL_ADD

>> -   || type == R_RISCV_TPREL_LO12_I

>> -   || type == R_RISCV_TPREL_LO12_S)

>> +    || type == R_RISCV_TPREL_ADD

>> +    || type == R_RISCV_TPREL_LO12_I

>> +    || type == R_RISCV_TPREL_LO12_S)

>>      relax_func = _bfd_riscv_relax_tls_le;

>> +  else if (!bfd_link_pic (info) && (type == R_RISCV_PCREL_HI20

>> +    || type == R_RISCV_PCREL_LO12_I

>> +    || type == R_RISCV_PCREL_LO12_S))

>> +    relax_func = _bfd_riscv_relax_pc;

>>    else

>>      continue;

>>   }

>> -      else if (info->relax_pass == 1

>> -       && !bfd_link_pic (info)

>> -       && (type == R_RISCV_PCREL_HI20

>> -   || type == R_RISCV_PCREL_LO12_I

>> -   || type == R_RISCV_PCREL_LO12_S))

>> - relax_func = _bfd_riscv_relax_pc;

>> -      else if (info->relax_pass == 2 && type == R_RISCV_DELETE)

>> - relax_func = _bfd_riscv_relax_delete;

>> -      else if (info->relax_pass == 3 && type == R_RISCV_ALIGN)

>> +      else if (info->relax_pass == 1 && type == R_RISCV_ALIGN)

>>   relax_func = _bfd_riscv_relax_align;

>>        else

>>   continue;

>>

>> -      if (info->relax_pass < 2)

>> +      if (info->relax_pass == 0)

>>   {

>>    /* Only relax this reloc if it is paired with R_RISCV_RELAX.  */

>>    if (i == sec->reloc_count - 1

>> @@ -4954,9 +4946,6 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,

>>      free (relocs);

>>    riscv_free_pcgp_relocs (&pcgp_relocs, abfd, sec);

>>

>> -  if (*again)

>> -    htab->restart_relax = true;

>> -

>>    return ret;

>>  }

>>

>> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h

>> index e691a97ecda..3af8fd99d9a 100644

>> --- a/bfd/elfxx-riscv.h

>> +++ b/bfd/elfxx-riscv.h

>> @@ -92,11 +92,6 @@ riscv_estimate_digit (unsigned);

>>  extern int

>>  riscv_compare_subsets (const char *, const char *);

>>

>> -extern bool

>> -bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);

>> -extern bool

>> -bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);

>> -

>>  extern void

>>  bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);

>>  extern void

>> diff --git a/ld/ChangeLog b/ld/ChangeLog

>> index 808191bd14e..9cfcdd88754 100644

>> --- a/ld/ChangeLog

>> +++ b/ld/ChangeLog

>> @@ -1,3 +1,8 @@

>> +2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>

>> +

>> + * emultempl/riscvelf.em: Revert restart_relax changes and set

>> + relax_pass to 2.

>> +

>>  2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>

>>

>>   * scripttempl/pru.sc (.resource_table): Align the output

>> diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em

>> index c625a631fe5..865e3abd494 100644

>> --- a/ld/emultempl/riscvelf.em

>> +++ b/ld/emultempl/riscvelf.em

>> @@ -42,7 +42,7 @@ riscv_elf_before_allocation (void)

>>   ENABLE_RELAXATION;

>>      }

>>

>> -  link_info.relax_pass = 4;

>> +  link_info.relax_pass = 2;

>>  }

>>

>>  static void

>> @@ -76,11 +76,7 @@ gld${EMULATION_NAME}_after_allocation (void)

>>    enum phase_enum *phase = &(expld.dataseg.phase);

>>    bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *)

>> phase);

>>

>> -  do

>> -    {

>> -      ldelf_map_segments (need_layout);

>> -    }

>> -  while (bfd_elf${ELFSIZE}_riscv_restart_relax_sections (&link_info));

>> +  ldelf_map_segments (need_layout);

>>  }

>>

>>  /* This is a convenient point to tell BFD about target specific flags.

>> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d

>> b/ld/testsuite/ld-riscv-elf/align-small-region.d

>> new file mode 100644

>> index 00000000000..3799129c665

>> --- /dev/null

>> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.d

>> @@ -0,0 +1,12 @@

>> +#source: align-small-region.s

>> +#as: -march=rv32i

>> +#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100

>> +#objdump: -d

>> +

>> +.*:[ ]+file format .*

>> +

>> +Disassembly of section \.entry:

>> +

>> +00000000 <_reset>:

>> +.*:[ ]+[0-9a-f]+[ ]+j[ ]+100[ ]+<_start>

>> +#pass

>> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld

>> b/ld/testsuite/ld-riscv-elf/align-small-region.ld

>> new file mode 100644

>> index 00000000000..6a3e6638b26

>> --- /dev/null

>> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld

>> @@ -0,0 +1,12 @@

>> +MEMORY

>> +{

>> + reset : ORIGIN = 0x0, LENGTH = 32

>> +}

>> +

>> +SECTIONS

>> +{

>> + .entry :

>> + {

>> + KEEP (*(.entry))

>> + } > reset

>> +}

>> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s

>> b/ld/testsuite/ld-riscv-elf/align-small-region.s

>> new file mode 100644

>> index 00000000000..ace81d640ed

>> --- /dev/null

>> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.s

>> @@ -0,0 +1,7 @@

>> + .section .entry, "xa"

>> + .align 5

>> + .globl _reset

>> + .type _reset, @function

>> +_reset:

>> + tail _start

>> + .size _reset, . - _reset

>> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

>> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

>> index 11977611733..739cc04b889 100644

>> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

>> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp

>> @@ -119,6 +119,7 @@ proc run_relax_twice_test {} {

>>  }

>>

>>  if [istarget "riscv*-*-*"] {

>> +    run_dump_test "align-small-region"

>>      run_dump_test "call-relax"

>>      run_dump_test "pcgp-relax"

>>      run_dump_test "c-lui"

>> --

>> 2.25.1
Lewis Revill Oct. 11, 2021, 8:43 a.m. | #5
On Fri, 8 Oct 2021 at 17:24, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>

> I haven't looked at the patch (and must have missed the earlier

> discussion), but the specific case that I was trying to solve with the

> R_RISCV_DELETE split was something like

>

>

>     r0: auipc t0, symbol0

>     r1: auipc t1, symbol1

>         ld    t0, symbol0, r0 # reloc points to r0

>         ld    t1, symbol1, r1 # reloc points to r1

>

> in this case, if the auipc at r0 gets deleted then we end up with an

> aliasing issue and need some way to disambiguate which of the two

> symbols we're targeting.  I don't remember if we ever put a test case in

> there for this, but it did manifest in real code (IIRC it was a Linux

> build, but you need to twiddle all the compiler flags so it'll schedule

> the auipc-based pairs).

>

> The idea behing the extra pass here was to split this into two phases,

> so we could resolve the addresses before creating the ambiguities.


Thanks Palmer, I've checked the behaviour of my patch versus before my
patch and it appears the same. But just to be sure do you maybe have a
full testcase with more context, EG location of symbol0 and symbol1? I
will also have a look through and try to reason why my patch would still
be able to handle this.
Lewis Revill Oct. 13, 2021, 10:14 a.m. | #6
Any further thoughts or test results for this approach? I've gone
through it a couple of times and I can't spot any areas of concern.
Nelson Chu Oct. 18, 2021, 6:37 a.m. | #7
On Mon, Oct 11, 2021 at 4:43 PM Lewis Revill <lewis.revill@embecosm.com> wrote:
>

> On Fri, 8 Oct 2021 at 17:24, Palmer Dabbelt <palmer@dabbelt.com> wrote:

> >

> > I haven't looked at the patch (and must have missed the earlier

> > discussion), but the specific case that I was trying to solve with the

> > R_RISCV_DELETE split was something like

> >

> >

> >     r0: auipc t0, symbol0

> >     r1: auipc t1, symbol1

> >         ld    t0, symbol0, r0 # reloc points to r0

> >         ld    t1, symbol1, r1 # reloc points to r1

> >

> > in this case, if the auipc at r0 gets deleted then we end up with an

> > aliasing issue and need some way to disambiguate which of the two

> > symbols we're targeting.  I don't remember if we ever put a test case in

> > there for this, but it did manifest in real code (IIRC it was a Linux

> > build, but you need to twiddle all the compiler flags so it'll schedule

> > the auipc-based pairs).


Umm I think we don't have the one in the current ld testcases, but
maybe it is worth having.

> > The idea behing the extra pass here was to split this into two phases,

> > so we could resolve the addresses before creating the ambiguities.

>

> Thanks Palmer, I've checked the behaviour of my patch versus before my

> patch and it appears the same. But just to be sure do you maybe have a

> full testcase with more context, EG location of symbol0 and symbol1? I

> will also have a look through and try to reason why my patch would still

> be able to handle this.


Consider the following testcase, and compile with the patch,

nelson@LAPTOP-QFSGI1F2:~/test$ cat tmp.s
        .text
        .globl _start
_start:
.L1:    auipc   a0, %pcrel_hi(data_a)
.L2:    auipc   a1, %pcrel_hi(data_b)
        addi    a0, a0, %pcrel_lo(.L1)
        addi    a1, a1, %pcrel_lo(.L2)

        .data
        .word 0x0
        .globl data_a
data_a:
        .word 0x1

        .section .rodata
        .globl data_b
data_b:
        .word 0x2
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-as
tmp.s -o tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-ld
tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-objdump
-d -Mno-aliases a.out

a.out:     file format elf64-littleriscv


Disassembly of section .text:

00000000000100e8 <_start>:
   100e8:       00000597                auipc   a1,0x0
   100ec:       80418513                addi    a0,gp,-2044 # 110fc <data_a>
   100f0:       80418593                addi    a1,gp,-2044 # 110fc <data_a>

The second addi looks wrong, since the symbol should be data_b in the
rodata, which cannot be relaxed generally.  What Palmer is concerned
about is that - it is hard to connect the high and low parts of pcrel
relocations correctly when we are also deleting instructions at the
same time.  The expected output should be like that,

nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-ld
tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-objdump
-d -Mno-aliases a.out

a.out:     file format elf64-littleriscv


Disassembly of section .text:

00000000000100e8 <_start>:
   100e8:       00000597                auipc   a1,0x0
   100ec:       80418513                addi    a0,gp,-2044 # 110fc <data_a>
   100f0:       00c58593                addi    a1,a1,12 # 100f4 <data_b>


However, to make sure we are now aligned, my imagination is that,

* relax pass 0 (lui, call, tprel, update the pcgp table when deleting
the code; pcrel but don't delete auipc at this pass, just marked as
R_RISCV_DELETE)
* relax pass 1 (delete auipc for pcrel)
* relax pass 2 (alignment) a

I understand that using another relax pass to delete the auipc should
also reduce the chance of all relaxations, but the correctness should
be the first priority.  I would suggest that we still delete the auipc
in another relax pass like what we did as usual.  The advantage is
that we don’t have to spend too much time to bear the risk of the
large changes, and can also resolve the small region problem and the
issue of commit abd20cb.  On the issue of the reduction of relaxed
chances, we could try to improve this in the future patches, it isn't
very urgent for me at this stage.

Oh, another thing is that, I cannot apply the patch directly since the
updated patch looks corrupt with many newlines?  Anyway, we don't need
to update the ChangeLogs files when sending the patch, just writing
the ChangeLogs in the commit comments should be enough.  Otherwise we
always need to resolve the conflicts since the ChangeLogs files are
changed frequently.  However, maybe using the git send-mail to send
the patch, or attach the format patch in the mail directly should be
more convenient.

Thanks
Nelson
Lewis Revill Oct. 19, 2021, 9:29 a.m. | #8
On Mon, 18 Oct 2021 at 07:37, Nelson Chu <nelson.chu@sifive.com> wrote:
>

> I understand that using another relax pass to delete the auipc should

> also reduce the chance of all relaxations, but the correctness should

> be the first priority.  I would suggest that we still delete the auipc

> in another relax pass like what we did as usual.  The advantage is

> that we don’t have to spend too much time to bear the risk of the

> large changes, and can also resolve the small region problem and the

> issue of commit abd20cb.  On the issue of the reduction of relaxed

> chances, we could try to improve this in the future patches, it isn't

> very urgent for me at this stage.


Thanks Nelson,

I think you're right, the best approach for now would be to add the
R_RISCV_DELETE pass back in, and just accept that we can only run it one
time once we're done repeating pass 0.

I'll go ahead and do that and send an update. Not used git send-mail
before so I'll just do a normal patch file without modifying the
ChangeLog itself.

Lewis
Lewis Revill Oct. 19, 2021, 10:53 a.m. | #9
From 26714459d05048e959fcf821f8cf61f0166839da Mon Sep 17 00:00:00 2001
From: Lewis Revill <lewis.revill@embecosm.com>

Date: Tue, 19 Oct 2021 11:52:30 +0100
Subject: [PATCH] RISC-V: Repeat a single target relax pass instead of 3

Commit abd20cb637008da9d32018b4b03973e119388a0a and
ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4 introduced additional
complexity into the paths run by the RISC-V relaxation pass in order to
resolve the issue of accurately keeping track of pcrel_hi and pcrel_lo
pairs. The first commit split up relaxation of these relocs into a pass
which occurred after other relaxations in order to prevent the situation
where bytes were deleted in between a pcrel_lo/pcrel_hi pair, inhibiting
our ability to find the corresponding pcrel_hi relocation from the
address attached to the pcrel_lo.

Since the relaxation was split into two passes the 'again' parameter
could not be used to perform the entire relaxation process again and so
the second commit added a way to restart ldelf_map_segments, thus
starting the whole process again.

Unfortunately this process could not account for the fact that we were
not finished with the relaxation process so in some cases - such as the
case where code would not fit in a memory region before the
R_RISCV_ALIGN relocation was relaxed - sanity checks in generic code
would fail.

This patch fixes all three of these concerns by reverting back to a
system of having only one target relax pass but updating entries in the
table of pcrel_hi/pcrel_lo relocs every time any bytes are deleted. Thus
we can keep track of the pairs accurately, and we can use the 'again'
parameter to restart the entire target relax pass, behaving in the way
that generic code expects. Unfortunately we must still have an
additional pass to delay deleting AUIPC bytes to avoid ambiguity between
pcrel_hi relocs stored in the table after deletion. This pass can only
be run once so we may potentially miss out on relaxation opportunities
but this is likely to be rare.

Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=28410

bfd/
* elfnn-riscv.c: Revert restart_relax changes.
(riscv_relax_delete_bytes): Update tpgp relocs whenever bytes
are deleted.
(riscv_update_pcgp_relocs): Add function to update pcrel_hi
section offsets and symbol values.
(_bfd_riscv_relax_section): Use only one pass for all target
relaxations.
* elfxx-riscv.h: Revert restart_relax changes.

ld/
* emultempl/riscvelf.em: Revert restart_relax changes and set
relax_pass to 3.
---
 bfd/elfnn-riscv.c                             | 341 +++++++++---------
 bfd/elfxx-riscv.h                             |   5 -
 ld/emultempl/riscvelf.em                      |   8 +-
 .../ld-riscv-elf/align-small-region.d         |  12 +
 .../ld-riscv-elf/align-small-region.ld        |  12 +
 .../ld-riscv-elf/align-small-region.s         |   7 +
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |   1 +
 7 files changed, 205 insertions(+), 181 deletions(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 2e8df72fa2a..211e736f60c 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -131,9 +131,6 @@ struct riscv_elf_link_hash_table
   /* The index of the last unused .rel.iplt slot.  */
   bfd_vma last_iplt_index;

-  /* Re-run the relaxations from relax pass 0 if TRUE.  */
-  bool restart_relax;
-
   /* The data segment phase, don't relax the section
      when it is exp_seg_relro_adjust.  */
   int *data_segment_phase;
@@ -405,7 +402,6 @@ riscv_elf_link_hash_table_create (bfd *abfd)
     }

   ret->max_alignment = (bfd_vma) -1;
-  ret->restart_relax = false;

   /* Create hash table for local ifunc.  */
   ret->loc_hash_table = htab_try_create (1024,
@@ -3923,115 +3919,6 @@ _bfd_riscv_elf_merge_private_bfd_data (bfd *ibfd,
struct bfd_link_info *info)
   return false;
 }

-/* Delete some bytes from a section while relaxing.  */
-
-static bool
-riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
-  struct bfd_link_info *link_info)
-{
-  unsigned int i, symcount;
-  bfd_vma toaddr = sec->size;
-  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
-  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
-  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
-  struct bfd_elf_section_data *data = elf_section_data (sec);
-  bfd_byte *contents = data->this_hdr.contents;
-
-  /* Actually delete the bytes.  */
-  sec->size -= count;
-  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
-
-  /* Adjust the location of all of the relocs.  Note that we need not
-     adjust the addends, since all PC-relative references must be against
-     symbols, which we will adjust below.  */
-  for (i = 0; i < sec->reloc_count; i++)
-    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
-      data->relocs[i].r_offset -= count;
-
-  /* Adjust the local symbols defined in this section.  */
-  for (i = 0; i < symtab_hdr->sh_info; i++)
-    {
-      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
-      if (sym->st_shndx == sec_shndx)
- {
-  /* If the symbol is in the range of memory we just moved, we
-     have to adjust its value.  */
-  if (sym->st_value > addr && sym->st_value <= toaddr)
-    sym->st_value -= count;
-
-  /* If the symbol *spans* the bytes we just deleted (i.e. its
-     *end* is in the moved bytes but its *start* isn't), then we
-     must adjust its size.
-
-     This test needs to use the original value of st_value, otherwise
-     we might accidentally decrease size when deleting bytes right
-     before the symbol.  But since deleted relocs can't span across
-     symbols, we can't have both a st_value and a st_size decrease,
-     so it is simpler to just use an else.  */
-  else if (sym->st_value <= addr
-   && sym->st_value + sym->st_size > addr
-   && sym->st_value + sym->st_size <= toaddr)
-    sym->st_size -= count;
- }
-    }
-
-  /* Now adjust the global symbols defined in this section.  */
-  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
-      - symtab_hdr->sh_info);
-
-  for (i = 0; i < symcount; i++)
-    {
-      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
-
-      /* The '--wrap SYMBOL' option is causing a pain when the object file,
- containing the definition of __wrap_SYMBOL, includes a direct
- call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
- the same symbol (which is __wrap_SYMBOL), but still exist as two
- different symbols in 'sym_hashes', we don't want to adjust
- the global symbol __wrap_SYMBOL twice.
-
- The same problem occurs with symbols that are versioned_hidden, as
- foo becomes an alias for foo@BAR, and hence they need the same
- treatment.  */
-      if (link_info->wrap_hash != NULL
-  || sym_hash->versioned != unversioned)
- {
-  struct elf_link_hash_entry **cur_sym_hashes;
-
-  /* Loop only over the symbols which have already been checked.  */
-  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
-       cur_sym_hashes++)
-    {
-      /* If the current symbol is identical to 'sym_hash', that means
- the symbol was already adjusted (or at least checked).  */
-      if (*cur_sym_hashes == sym_hash)
- break;
-    }
-  /* Don't adjust the symbol again.  */
-  if (cur_sym_hashes < &sym_hashes[i])
-    continue;
- }
-
-      if ((sym_hash->root.type == bfd_link_hash_defined
-   || sym_hash->root.type == bfd_link_hash_defweak)
-  && sym_hash->root.u.def.section == sec)
- {
-  /* As above, adjust the value if needed.  */
-  if (sym_hash->root.u.def.value > addr
-      && sym_hash->root.u.def.value <= toaddr)
-    sym_hash->root.u.def.value -= count;
-
-  /* As above, adjust the size if needed.  */
-  else if (sym_hash->root.u.def.value <= addr
-   && sym_hash->root.u.def.value + sym_hash->size > addr
-   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
-    sym_hash->size -= count;
- }
-    }
-
-  return true;
-}
-
 /* A second format for recording PC-relative hi relocations.  This stores
the
    information required to relax them to GP-relative addresses.  */

@@ -4162,6 +4049,148 @@ riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p,
bfd_vma hi_sec_off)
   return false;
 }

+static void
+riscv_update_pcgp_relocs (riscv_pcgp_relocs *p, asection *deleted_sec,
+  bfd_vma deleted_addr, size_t deleted_count)
+{
+  /* Bytes have already been deleted and toaddr should match the old
section
+     size for our checks, so adjust it here.  */
+  bfd_vma toaddr = deleted_sec->size + deleted_count;
+  riscv_pcgp_lo_reloc *l;
+  riscv_pcgp_hi_reloc *h;
+
+  /* Update section offsets of corresponding pcrel_hi relocs for the
pcrel_lo
+     entries where they occur after the deleted bytes.  */
+  for (l = p->lo; l != NULL; l = l->next)
+    if (l->hi_sec_off > deleted_addr && l->hi_sec_off < toaddr)
+      l->hi_sec_off -= deleted_count;
+
+  /* Update both section offsets, and symbol values of pcrel_hi relocs
where
+     these values occur after the deleted bytes.  */
+  for (h = p->hi; h != NULL; h = h->next)
+    {
+      if (h->hi_sec_off > deleted_addr && h->hi_sec_off < toaddr)
+ h->hi_sec_off -= deleted_count;
+      if (h->sym_sec == deleted_sec && h->hi_addr > deleted_addr
+  && h->hi_addr < toaddr)
+ h->hi_addr -= deleted_count;
+    }
+}
+
+/* Delete some bytes from a section while relaxing.  */
+
+static bool
+riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
+  struct bfd_link_info *link_info, riscv_pcgp_relocs *p)
+{
+  unsigned int i, symcount;
+  bfd_vma toaddr = sec->size;
+  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
+  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
+  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
+  struct bfd_elf_section_data *data = elf_section_data (sec);
+  bfd_byte *contents = data->this_hdr.contents;
+
+  /* Actually delete the bytes.  */
+  sec->size -= count;
+  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
+
+  /* Adjust the location of all of the relocs.  Note that we need not
+     adjust the addends, since all PC-relative references must be against
+     symbols, which we will adjust below.  */
+  for (i = 0; i < sec->reloc_count; i++)
+    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
+      data->relocs[i].r_offset -= count;
+
+  /* Adjust the hi_sec_off, and the hi_addr of any entries in the pcgp
relocs
+     table for which these values occur after the deleted bytes.  */
+  if (p)
+    riscv_update_pcgp_relocs (p, sec, addr, count);
+
+  /* Adjust the local symbols defined in this section.  */
+  for (i = 0; i < symtab_hdr->sh_info; i++)
+    {
+      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
+      if (sym->st_shndx == sec_shndx)
+ {
+  /* If the symbol is in the range of memory we just moved, we
+     have to adjust its value.  */
+  if (sym->st_value > addr && sym->st_value <= toaddr)
+    sym->st_value -= count;
+
+  /* If the symbol *spans* the bytes we just deleted (i.e. its
+     *end* is in the moved bytes but its *start* isn't), then we
+     must adjust its size.
+
+     This test needs to use the original value of st_value, otherwise
+     we might accidentally decrease size when deleting bytes right
+     before the symbol.  But since deleted relocs can't span across
+     symbols, we can't have both a st_value and a st_size decrease,
+     so it is simpler to just use an else.  */
+  else if (sym->st_value <= addr
+   && sym->st_value + sym->st_size > addr
+   && sym->st_value + sym->st_size <= toaddr)
+    sym->st_size -= count;
+ }
+    }
+
+  /* Now adjust the global symbols defined in this section.  */
+  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
+      - symtab_hdr->sh_info);
+
+  for (i = 0; i < symcount; i++)
+    {
+      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
+
+      /* The '--wrap SYMBOL' option is causing a pain when the object file,
+ containing the definition of __wrap_SYMBOL, includes a direct
+ call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
+ the same symbol (which is __wrap_SYMBOL), but still exist as two
+ different symbols in 'sym_hashes', we don't want to adjust
+ the global symbol __wrap_SYMBOL twice.
+
+ The same problem occurs with symbols that are versioned_hidden, as
+ foo becomes an alias for foo@BAR, and hence they need the same
+ treatment.  */
+      if (link_info->wrap_hash != NULL
+  || sym_hash->versioned != unversioned)
+ {
+  struct elf_link_hash_entry **cur_sym_hashes;
+
+  /* Loop only over the symbols which have already been checked.  */
+  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
+       cur_sym_hashes++)
+    {
+      /* If the current symbol is identical to 'sym_hash', that means
+ the symbol was already adjusted (or at least checked).  */
+      if (*cur_sym_hashes == sym_hash)
+ break;
+    }
+  /* Don't adjust the symbol again.  */
+  if (cur_sym_hashes < &sym_hashes[i])
+    continue;
+ }
+
+      if ((sym_hash->root.type == bfd_link_hash_defined
+   || sym_hash->root.type == bfd_link_hash_defweak)
+  && sym_hash->root.u.def.section == sec)
+ {
+  /* As above, adjust the value if needed.  */
+  if (sym_hash->root.u.def.value > addr
+      && sym_hash->root.u.def.value <= toaddr)
+    sym_hash->root.u.def.value -= count;
+
+  /* As above, adjust the size if needed.  */
+  else if (sym_hash->root.u.def.value <= addr
+   && sym_hash->root.u.def.value + sym_hash->size > addr
+   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
+    sym_hash->size -= count;
+ }
+    }
+
+  return true;
+}
+
 typedef bool (*relax_func_t) (bfd *, asection *, asection *,
       struct bfd_link_info *,
       Elf_Internal_Rela *,
@@ -4179,7 +4208,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
        bfd_vma max_alignment,
        bfd_vma reserve_size ATTRIBUTE_UNUSED,
        bool *again,
-       riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+       riscv_pcgp_relocs *pcgp_relocs,
        bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4243,7 +4272,7 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
   /* Delete unnecessary JALR.  */
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + len, 8 - len,
-   link_info);
+   link_info, pcgp_relocs);
 }

 /* Traverse all output sections and return the max alignment.  */
@@ -4275,7 +4304,7 @@ _bfd_riscv_relax_lui (bfd *abfd,
       bfd_vma max_alignment,
       bfd_vma reserve_size,
       bool *again,
-      riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+      riscv_pcgp_relocs *pcgp_relocs,
       bool undefined_weak)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4337,7 +4366,7 @@ _bfd_riscv_relax_lui (bfd *abfd,
   rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
-   link_info);
+   link_info, pcgp_relocs);

  default:
   abort ();
@@ -4370,7 +4399,7 @@ _bfd_riscv_relax_lui (bfd *abfd,

       *again = true;
       return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + 2, 2,
-       link_info);
+       link_info, pcgp_relocs);
     }

   return true;
@@ -4388,7 +4417,7 @@ _bfd_riscv_relax_tls_le (bfd *abfd,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again,
- riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   /* See if this symbol is in range of tp.  */
@@ -4411,15 +4440,15 @@ _bfd_riscv_relax_tls_le (bfd *abfd,
       /* We can delete the unnecessary instruction and reloc.  */
       rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
       *again = true;
-      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info);
+      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info,
+       pcgp_relocs);

     default:
       abort ();
     }
 }

-/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.
-   Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.  */

 static bool
 _bfd_riscv_relax_align (bfd *abfd, asection *sec,
@@ -4430,7 +4459,7 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again ATTRIBUTE_UNUSED,
- riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4442,6 +4471,9 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,
   bfd_vma aligned_addr = ((symval - 1) & ~(alignment - 1)) + alignment;
   bfd_vma nop_bytes = aligned_addr - symval;

+  /* Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+  sec->sec_flg0 = true;
+
   /* Make sure there are enough NOPs to actually achieve the alignment.  */
   if (rel->r_addend < nop_bytes)
     {
@@ -4471,7 +4503,7 @@ _bfd_riscv_relax_align (bfd *abfd, asection *sec,

   /* Delete the excess bytes.  */
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + nop_bytes,
-   rel->r_addend - nop_bytes, link_info);
+   rel->r_addend - nop_bytes, link_info, NULL);
 }

 /* Relax PC-relative references to GP-relative references.  */
@@ -4637,15 +4669,14 @@ _bfd_riscv_relax_delete (bfd *abfd,
  bfd_vma symval ATTRIBUTE_UNUSED,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bool *again,
+ bool *again ATTRIBUTE_UNUSED,
  riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   if (!riscv_relax_delete_bytes (abfd, sec, rel->r_offset, rel->r_addend,
- link_info))
+ link_info, NULL))
     return false;
   rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
-  *again = true;
   return true;
 }

@@ -4660,35 +4691,11 @@ bfd_elfNN_riscv_set_data_segment_info (struct
bfd_link_info *info,
   htab->data_segment_phase = data_segment_phase;
 }

-/* Called by after_allocation to check if we need to run the whole
-   relaxations again.  */
-
-bool
-bfd_elfNN_riscv_restart_relax_sections (struct bfd_link_info *info)
-{
-  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
-  bool restart = htab->restart_relax;
-  /* Reset the flag.  */
-  htab->restart_relax = false;
-  return restart;
-}
-
 /* Relax a section.

-   Pass 0: Shortens code sequences for LUI/CALL/TPREL relocs.
-   Pass 1: Shortens code sequences for PCREL relocs.
-   Pass 2: Deletes the bytes that pass 1 made obsolete.
-   Pass 3: Which cannot be disabled, handles code alignment directives.
-
-   The `again` is used to determine whether the relax pass itself needs to
-   run again.  And the `restart_relax` is used to determine if we need to
-   run the whole relax passes again from 0 to 2.  Once we have deleted the
-   code between relax pass 0 to 2, the restart_relax will be set to TRUE,
-   and we should run the whole relaxations again to give them more chances
-   to shorten the code.
-
-   Since we can't relax anything else once we start to handle the
alignments,
-   we will only enter into the relax pass 3 when the restart_relax is
FALSE.  */
+   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.
+   Pass 1: Deletes the bytes that PCREL relaxation in pass 0 made obsolete.
+   Pass 1: Which cannot be disabled, handles code alignment directives.  */

 static bool
 _bfd_riscv_relax_section (bfd *abfd, asection *sec,
@@ -4707,12 +4714,11 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
   *again = false;

   if (bfd_link_relocatable (info)
+      || sec->sec_flg0
       || (sec->flags & SEC_RELOC) == 0
       || sec->reloc_count == 0
       || (info->disable_target_specific_optimizations
-  && info->relax_pass < 2)
-      || (htab->restart_relax
-  && info->relax_pass == 3)
+  && info->relax_pass == 0)
       /* The exp_seg_relro_adjust is enum phase_enum (0x4),
  and defined in ld/ldexp.h.  */
       || *(htab->data_segment_phase) == 4)
@@ -4765,23 +4771,21 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
    || type == R_RISCV_TPREL_LO12_I
    || type == R_RISCV_TPREL_LO12_S)
     relax_func = _bfd_riscv_relax_tls_le;
+  else if (!bfd_link_pic (info) && (type == R_RISCV_PCREL_HI20
+   || type == R_RISCV_PCREL_LO12_I
+   || type == R_RISCV_PCREL_LO12_S))
+    relax_func = _bfd_riscv_relax_pc;
   else
     continue;
  }
-      else if (info->relax_pass == 1
-       && !bfd_link_pic (info)
-       && (type == R_RISCV_PCREL_HI20
-   || type == R_RISCV_PCREL_LO12_I
-   || type == R_RISCV_PCREL_LO12_S))
- relax_func = _bfd_riscv_relax_pc;
-      else if (info->relax_pass == 2 && type == R_RISCV_DELETE)
- relax_func = _bfd_riscv_relax_delete;
-      else if (info->relax_pass == 3 && type == R_RISCV_ALIGN)
+      else if (info->relax_pass == 1 && type == R_RISCV_DELETE)
+        relax_func = _bfd_riscv_relax_delete;
+      else if (info->relax_pass == 2 && type == R_RISCV_ALIGN)
  relax_func = _bfd_riscv_relax_align;
       else
  continue;

-      if (info->relax_pass < 2)
+      if (info->relax_pass == 0)
  {
   /* Only relax this reloc if it is paired with R_RISCV_RELAX.  */
   if (i == sec->reloc_count - 1
@@ -4954,9 +4958,6 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
     free (relocs);
   riscv_free_pcgp_relocs (&pcgp_relocs, abfd, sec);

-  if (*again)
-    htab->restart_relax = true;
-
   return ret;
 }

diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
index e691a97ecda..3af8fd99d9a 100644
--- a/bfd/elfxx-riscv.h
+++ b/bfd/elfxx-riscv.h
@@ -92,11 +92,6 @@ riscv_estimate_digit (unsigned);
 extern int
 riscv_compare_subsets (const char *, const char *);

-extern bool
-bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);
-extern bool
-bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);
-
 extern void
 bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);
 extern void
diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
index c625a631fe5..80b7b3707d5 100644
--- a/ld/emultempl/riscvelf.em
+++ b/ld/emultempl/riscvelf.em
@@ -42,7 +42,7 @@ riscv_elf_before_allocation (void)
  ENABLE_RELAXATION;
     }

-  link_info.relax_pass = 4;
+  link_info.relax_pass = 3;
 }

 static void
@@ -76,11 +76,7 @@ gld${EMULATION_NAME}_after_allocation (void)
   enum phase_enum *phase = &(expld.dataseg.phase);
   bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *)
phase);

-  do
-    {
-      ldelf_map_segments (need_layout);
-    }
-  while (bfd_elf${ELFSIZE}_riscv_restart_relax_sections (&link_info));
+  ldelf_map_segments (need_layout);
 }

 /* This is a convenient point to tell BFD about target specific flags.
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d
b/ld/testsuite/ld-riscv-elf/align-small-region.d
new file mode 100644
index 00000000000..3799129c665
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.d
@@ -0,0 +1,12 @@
+#source: align-small-region.s
+#as: -march=rv32i
+#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100
+#objdump: -d
+
+.*:[ ]+file format .*
+
+Disassembly of section \.entry:
+
+00000000 <_reset>:
+.*:[ ]+[0-9a-f]+[ ]+j[ ]+100[ ]+<_start>
+#pass
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld
b/ld/testsuite/ld-riscv-elf/align-small-region.ld
new file mode 100644
index 00000000000..6a3e6638b26
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld
@@ -0,0 +1,12 @@
+MEMORY
+{
+ reset : ORIGIN = 0x0, LENGTH = 32
+}
+
+SECTIONS
+{
+ .entry :
+ {
+ KEEP (*(.entry))
+ } > reset
+}
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s
b/ld/testsuite/ld-riscv-elf/align-small-region.s
new file mode 100644
index 00000000000..ace81d640ed
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.s
@@ -0,0 +1,7 @@
+ .section .entry, "xa"
+ .align 5
+ .globl _reset
+ .type _reset, @function
+_reset:
+ tail _start
+ .size _reset, . - _reset
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index 11977611733..739cc04b889 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -119,6 +119,7 @@ proc run_relax_twice_test {} {
 }

 if [istarget "riscv*-*-*"] {
+    run_dump_test "align-small-region"
     run_dump_test "call-relax"
     run_dump_test "pcgp-relax"
     run_dump_test "c-lui"
-- 
2.25.1
Andreas Schwab Oct. 19, 2021, 12:14 p.m. | #10
On Okt 19 2021, Lewis Revill wrote:

> +   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.

> +   Pass 1: Deletes the bytes that PCREL relaxation in pass 0 made obsolete.

> +   Pass 1: Which cannot be disabled, handles code alignment directives.  */


Pass 2?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Lewis Revill Oct. 19, 2021, 2:37 p.m. | #11
On Tue, 19 Oct 2021 at 13:14, Andreas Schwab <schwab@linux-m68k.org> wrote:
>

> Pass 2?

>

> Andreas.


Ah good catch... I guess I'll give it a while to see if anyone else has
any comments and then just mail another patch....

Patch

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 2a08ff7cfb4..7bcba3a090f 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,19 @@ 
+2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * elfnn-riscv.c: Revert restart_relax changes.
+ (_bfd_riscv_relax_pc): Delete bytes immediately rather than
+ using R_RISCV_DELETE.
+ (_bfd_riscv_relax_delete): Remove.
+ (riscv_relax_delete_bytes): Update tpgp relocs whenever bytes
+ are deleted.
+ (riscv_update_pcgp_reloc_offs): Add function to update pcrel_hi
+ section offsets.
+ (riscv_update_pcgp_reloc_symvals): Add function to update
+ pcrel_hi symbol values.
+ (_bfd_riscv_relax_section): Use only one pass for all target
+ relaxations.
+ * elfxx-riscv.h: Revert restart_relax changes.
+
 2021-09-27  Nick Alcock  <nick.alcock@oracle.com>

  * configure: Regenerate.
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 2e8df72fa2a..95e2d5536d1 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -38,9 +38,6 @@ 
 #define CHAR_BIT 8
 #endif

-/* Internal relocations used exclusively by the relaxation pass.  */
-#define R_RISCV_DELETE (R_RISCV_max + 1)
-
 #define ARCH_SIZE NN

 #define MINUS_ONE ((bfd_vma)0 - 1)
@@ -131,9 +128,6 @@  struct riscv_elf_link_hash_table
   /* The index of the last unused .rel.iplt slot.  */
   bfd_vma last_iplt_index;

-  /* Re-run the relaxations from relax pass 0 if TRUE.  */
-  bool restart_relax;
-
   /* The data segment phase, don't relax the section
      when it is exp_seg_relro_adjust.  */
   int *data_segment_phase;
@@ -405,7 +399,6 @@  riscv_elf_link_hash_table_create (bfd *abfd)
     }

   ret->max_alignment = (bfd_vma) -1;
-  ret->restart_relax = false;

   /* Create hash table for local ifunc.  */
   ret->loc_hash_table = htab_try_create (1024,
@@ -1714,9 +1707,6 @@  perform_relocation (const reloc_howto_type *howto,
     case R_RISCV_TLS_DTPREL64:
       break;

-    case R_RISCV_DELETE:
-      return bfd_reloc_ok;
-
     default:
       return bfd_reloc_notsupported;
     }
@@ -2326,7 +2316,6 @@  riscv_elf_relocate_section (bfd *output_bfd,
  case R_RISCV_SET16:
  case R_RISCV_SET32:
  case R_RISCV_32_PCREL:
- case R_RISCV_DELETE:
   /* These require no special handling beyond perform_relocation.  */
   break;

@@ -3923,115 +3912,6 @@  _bfd_riscv_elf_merge_private_bfd_data (bfd *ibfd,
struct bfd_link_info *info)
   return false;
 }

-/* Delete some bytes from a section while relaxing.  */
-
-static bool
-riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
-  struct bfd_link_info *link_info)
-{
-  unsigned int i, symcount;
-  bfd_vma toaddr = sec->size;
-  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
-  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
-  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
-  struct bfd_elf_section_data *data = elf_section_data (sec);
-  bfd_byte *contents = data->this_hdr.contents;
-
-  /* Actually delete the bytes.  */
-  sec->size -= count;
-  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
-
-  /* Adjust the location of all of the relocs.  Note that we need not
-     adjust the addends, since all PC-relative references must be against
-     symbols, which we will adjust below.  */
-  for (i = 0; i < sec->reloc_count; i++)
-    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
-      data->relocs[i].r_offset -= count;
-
-  /* Adjust the local symbols defined in this section.  */
-  for (i = 0; i < symtab_hdr->sh_info; i++)
-    {
-      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
-      if (sym->st_shndx == sec_shndx)
- {
-  /* If the symbol is in the range of memory we just moved, we
-     have to adjust its value.  */
-  if (sym->st_value > addr && sym->st_value <= toaddr)
-    sym->st_value -= count;
-
-  /* If the symbol *spans* the bytes we just deleted (i.e. its
-     *end* is in the moved bytes but its *start* isn't), then we
-     must adjust its size.
-
-     This test needs to use the original value of st_value, otherwise
-     we might accidentally decrease size when deleting bytes right
-     before the symbol.  But since deleted relocs can't span across
-     symbols, we can't have both a st_value and a st_size decrease,
-     so it is simpler to just use an else.  */
-  else if (sym->st_value <= addr
-   && sym->st_value + sym->st_size > addr
-   && sym->st_value + sym->st_size <= toaddr)
-    sym->st_size -= count;
- }
-    }
-
-  /* Now adjust the global symbols defined in this section.  */
-  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
-      - symtab_hdr->sh_info);
-
-  for (i = 0; i < symcount; i++)
-    {
-      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
-
-      /* The '--wrap SYMBOL' option is causing a pain when the object file,
- containing the definition of __wrap_SYMBOL, includes a direct
- call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
- the same symbol (which is __wrap_SYMBOL), but still exist as two
- different symbols in 'sym_hashes', we don't want to adjust
- the global symbol __wrap_SYMBOL twice.
-
- The same problem occurs with symbols that are versioned_hidden, as
- foo becomes an alias for foo@BAR, and hence they need the same
- treatment.  */
-      if (link_info->wrap_hash != NULL
-  || sym_hash->versioned != unversioned)
- {
-  struct elf_link_hash_entry **cur_sym_hashes;
-
-  /* Loop only over the symbols which have already been checked.  */
-  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
-       cur_sym_hashes++)
-    {
-      /* If the current symbol is identical to 'sym_hash', that means
- the symbol was already adjusted (or at least checked).  */
-      if (*cur_sym_hashes == sym_hash)
- break;
-    }
-  /* Don't adjust the symbol again.  */
-  if (cur_sym_hashes < &sym_hashes[i])
-    continue;
- }
-
-      if ((sym_hash->root.type == bfd_link_hash_defined
-   || sym_hash->root.type == bfd_link_hash_defweak)
-  && sym_hash->root.u.def.section == sec)
- {
-  /* As above, adjust the value if needed.  */
-  if (sym_hash->root.u.def.value > addr
-      && sym_hash->root.u.def.value <= toaddr)
-    sym_hash->root.u.def.value -= count;
-
-  /* As above, adjust the size if needed.  */
-  else if (sym_hash->root.u.def.value <= addr
-   && sym_hash->root.u.def.value + sym_hash->size > addr
-   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
-    sym_hash->size -= count;
- }
-    }
-
-  return true;
-}
-
 /* A second format for recording PC-relative hi relocations.  This stores
the
    information required to relax them to GP-relative addresses.  */

@@ -4162,6 +4042,166 @@  riscv_find_pcgp_lo_reloc (riscv_pcgp_relocs *p,
bfd_vma hi_sec_off)
   return false;
 }

+static bool
+riscv_update_pcgp_reloc_offs (riscv_pcgp_relocs *p, bfd_vma
deleted_sec_off,
+      size_t deleted_count)
+{
+  riscv_pcgp_lo_reloc *l;
+  riscv_pcgp_hi_reloc *h;
+  bool updated = false;
+
+  for (l = p->lo; l != NULL; l = l->next)
+    if (l->hi_sec_off > deleted_sec_off)
+      {
+ l->hi_sec_off -= deleted_count;
+ updated = true;
+      }
+
+  for (h = p->hi; h != NULL; h = h->next)
+    if (h->hi_sec_off > deleted_sec_off)
+      {
+ h->hi_sec_off -= deleted_count;
+ updated = true;
+      }
+
+  return updated;
+}
+
+static bool
+riscv_update_pcgp_reloc_symvals(riscv_pcgp_relocs *p, bfd_vma old_value,
+ size_t deleted_count)
+{
+  riscv_pcgp_hi_reloc *h;
+  bool updated = false;
+
+  for (h = p->hi; h != NULL; h = h->next)
+    if (h->hi_addr == old_value)
+      {
+ h->hi_addr -= deleted_count;
+ updated = true;
+      }
+  return updated;
+}
+
+/* Delete some bytes from a section while relaxing.  */
+
+static bool
+riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t
count,
+  struct bfd_link_info *link_info, riscv_pcgp_relocs *p)
+{
+  unsigned int i, symcount;
+  bfd_vma toaddr = sec->size;
+  struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (abfd);
+  Elf_Internal_Shdr *symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
+  unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
+  struct bfd_elf_section_data *data = elf_section_data (sec);
+  bfd_byte *contents = data->this_hdr.contents;
+
+  /* Actually delete the bytes.  */
+  sec->size -= count;
+  memmove (contents + addr, contents + addr + count, toaddr - addr -
count);
+
+  /* Adjust the location of all of the relocs.  Note that we need not
+     adjust the addends, since all PC-relative references must be against
+     symbols, which we will adjust below.  */
+  for (i = 0; i < sec->reloc_count; i++)
+    if (data->relocs[i].r_offset > addr && data->relocs[i].r_offset <
toaddr)
+      data->relocs[i].r_offset -= count;
+
+  /* Adjust the hi_sec_off of entries in the pcgp relocs table.  */
+  riscv_update_pcgp_reloc_offs (p, addr, count);
+
+  /* Adjust the local symbols defined in this section.  */
+  for (i = 0; i < symtab_hdr->sh_info; i++)
+    {
+      Elf_Internal_Sym *sym = (Elf_Internal_Sym *) symtab_hdr->contents +
i;
+      if (sym->st_shndx == sec_shndx)
+ {
+  /* If the symbol is in the range of memory we just moved, we
+     have to adjust its value.  */
+  if (sym->st_value > addr && sym->st_value <= toaddr)
+    {
+      riscv_update_pcgp_reloc_symvals (p, sym->st_value, count);
+      sym->st_value -= count;
+    }
+
+  /* If the symbol *spans* the bytes we just deleted (i.e. its
+     *end* is in the moved bytes but its *start* isn't), then we
+     must adjust its size.
+
+     This test needs to use the original value of st_value, otherwise
+     we might accidentally decrease size when deleting bytes right
+     before the symbol.  But since deleted relocs can't span across
+     symbols, we can't have both a st_value and a st_size decrease,
+     so it is simpler to just use an else.  */
+  else if (sym->st_value <= addr
+   && sym->st_value + sym->st_size > addr
+   && sym->st_value + sym->st_size <= toaddr)
+    sym->st_size -= count;
+ }
+    }
+
+  /* Now adjust the global symbols defined in this section.  */
+  symcount = ((symtab_hdr->sh_size / sizeof (ElfNN_External_Sym))
+      - symtab_hdr->sh_info);
+
+  for (i = 0; i < symcount; i++)
+    {
+      struct elf_link_hash_entry *sym_hash = sym_hashes[i];
+
+      /* The '--wrap SYMBOL' option is causing a pain when the object file,
+ containing the definition of __wrap_SYMBOL, includes a direct
+ call to SYMBOL as well. Since both __wrap_SYMBOL and SYMBOL reference
+ the same symbol (which is __wrap_SYMBOL), but still exist as two
+ different symbols in 'sym_hashes', we don't want to adjust
+ the global symbol __wrap_SYMBOL twice.
+
+ The same problem occurs with symbols that are versioned_hidden, as
+ foo becomes an alias for foo@BAR, and hence they need the same
+ treatment.  */
+      if (link_info->wrap_hash != NULL
+  || sym_hash->versioned != unversioned)
+ {
+  struct elf_link_hash_entry **cur_sym_hashes;
+
+  /* Loop only over the symbols which have already been checked.  */
+  for (cur_sym_hashes = sym_hashes; cur_sym_hashes < &sym_hashes[i];
+       cur_sym_hashes++)
+    {
+      /* If the current symbol is identical to 'sym_hash', that means
+ the symbol was already adjusted (or at least checked).  */
+      if (*cur_sym_hashes == sym_hash)
+ break;
+    }
+  /* Don't adjust the symbol again.  */
+  if (cur_sym_hashes < &sym_hashes[i])
+    continue;
+ }
+
+      if ((sym_hash->root.type == bfd_link_hash_defined
+   || sym_hash->root.type == bfd_link_hash_defweak)
+  && sym_hash->root.u.def.section == sec)
+ {
+  /* As above, adjust the value if needed.  */
+  if (sym_hash->root.u.def.value > addr
+      && sym_hash->root.u.def.value <= toaddr)
+    {
+      riscv_update_pcgp_reloc_symvals (p, sym_hash->root.u.def.value,
+       count);
+      sym_hash->root.u.def.value -= count;
+    }
+
+  /* As above, adjust the size if needed.  */
+  else if (sym_hash->root.u.def.value <= addr
+   && sym_hash->root.u.def.value + sym_hash->size > addr
+   && sym_hash->root.u.def.value + sym_hash->size <= toaddr)
+    sym_hash->size -= count;
+ }
+    }
+
+  return true;
+}
+
 typedef bool (*relax_func_t) (bfd *, asection *, asection *,
       struct bfd_link_info *,
       Elf_Internal_Rela *,
@@ -4179,7 +4219,7 @@  _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
        bfd_vma max_alignment,
        bfd_vma reserve_size ATTRIBUTE_UNUSED,
        bool *again,
-       riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+       riscv_pcgp_relocs *pcgp_relocs,
        bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4243,7 +4283,7 @@  _bfd_riscv_relax_call (bfd *abfd, asection *sec,
asection *sym_sec,
   /* Delete unnecessary JALR.  */
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + len, 8 - len,
-   link_info);
+   link_info, pcgp_relocs);
 }

 /* Traverse all output sections and return the max alignment.  */
@@ -4275,7 +4315,7 @@  _bfd_riscv_relax_lui (bfd *abfd,
       bfd_vma max_alignment,
       bfd_vma reserve_size,
       bool *again,
-      riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
+      riscv_pcgp_relocs *pcgp_relocs,
       bool undefined_weak)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4337,7 +4377,7 @@  _bfd_riscv_relax_lui (bfd *abfd,
   rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
   *again = true;
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
-   link_info);
+   link_info, pcgp_relocs);

  default:
   abort ();
@@ -4370,7 +4410,7 @@  _bfd_riscv_relax_lui (bfd *abfd,

       *again = true;
       return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + 2, 2,
-       link_info);
+       link_info, pcgp_relocs);
     }

   return true;
@@ -4388,7 +4428,7 @@  _bfd_riscv_relax_tls_le (bfd *abfd,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again,
- riscv_pcgp_relocs *prcel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *prcel_relocs,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   /* See if this symbol is in range of tp.  */
@@ -4411,15 +4451,15 @@  _bfd_riscv_relax_tls_le (bfd *abfd,
       /* We can delete the unnecessary instruction and reloc.  */
       rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
       *again = true;
-      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info);
+      return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4,
link_info,
+       prcel_relocs);

     default:
       abort ();
     }
 }

-/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.
-   Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+/* Implement R_RISCV_ALIGN by deleting excess alignment NOPs.  */

 static bool
 _bfd_riscv_relax_align (bfd *abfd, asection *sec,
@@ -4430,7 +4470,7 @@  _bfd_riscv_relax_align (bfd *abfd, asection *sec,
  bfd_vma max_alignment ATTRIBUTE_UNUSED,
  bfd_vma reserve_size ATTRIBUTE_UNUSED,
  bool *again ATTRIBUTE_UNUSED,
- riscv_pcgp_relocs *pcrel_relocs ATTRIBUTE_UNUSED,
+ riscv_pcgp_relocs *pcgp_relocs,
  bool undefined_weak ATTRIBUTE_UNUSED)
 {
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
@@ -4442,6 +4482,9 @@  _bfd_riscv_relax_align (bfd *abfd, asection *sec,
   bfd_vma aligned_addr = ((symval - 1) & ~(alignment - 1)) + alignment;
   bfd_vma nop_bytes = aligned_addr - symval;

+  /* Once we've handled an R_RISCV_ALIGN, we can't relax anything else.  */
+  sec->sec_flg0 = true;
+
   /* Make sure there are enough NOPs to actually achieve the alignment.  */
   if (rel->r_addend < nop_bytes)
     {
@@ -4471,13 +4514,13 @@  _bfd_riscv_relax_align (bfd *abfd, asection *sec,

   /* Delete the excess bytes.  */
   return riscv_relax_delete_bytes (abfd, sec, rel->r_offset + nop_bytes,
-   rel->r_addend - nop_bytes, link_info);
+   rel->r_addend - nop_bytes, link_info, pcgp_relocs);
 }

 /* Relax PC-relative references to GP-relative references.  */

 static bool
-_bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
+_bfd_riscv_relax_pc (bfd *abfd,
      asection *sec,
      asection *sym_sec,
      struct bfd_link_info *link_info,
@@ -4485,7 +4528,7 @@  _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
      bfd_vma symval,
      bfd_vma max_alignment,
      bfd_vma reserve_size,
-     bool *again ATTRIBUTE_UNUSED,
+     bool *again,
      riscv_pcgp_relocs *pcgp_relocs,
      bool undefined_weak)
 {
@@ -4614,9 +4657,11 @@  _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
       sym_sec,
       undefined_weak);
   /* We can delete the unnecessary AUIPC and reloc.  */
-  rel->r_info = ELFNN_R_INFO (0, R_RISCV_DELETE);
-  rel->r_addend = 4;
-  return true;
+
+  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
+  *again = true;
+  return riscv_relax_delete_bytes (abfd, sec, rel->r_offset, 4, link_info,
+   pcgp_relocs);

  default:
   abort ();
@@ -4626,29 +4671,6 @@  _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
   return true;
 }

-/* Delete the bytes for R_RISCV_DELETE.  */
-
-static bool
-_bfd_riscv_relax_delete (bfd *abfd,
- asection *sec,
- asection *sym_sec ATTRIBUTE_UNUSED,
- struct bfd_link_info *link_info,
- Elf_Internal_Rela *rel,
- bfd_vma symval ATTRIBUTE_UNUSED,
- bfd_vma max_alignment ATTRIBUTE_UNUSED,
- bfd_vma reserve_size ATTRIBUTE_UNUSED,
- bool *again,
- riscv_pcgp_relocs *pcgp_relocs ATTRIBUTE_UNUSED,
- bool undefined_weak ATTRIBUTE_UNUSED)
-{
-  if (!riscv_relax_delete_bytes (abfd, sec, rel->r_offset, rel->r_addend,
- link_info))
-    return false;
-  rel->r_info = ELFNN_R_INFO (0, R_RISCV_NONE);
-  *again = true;
-  return true;
-}
-
 /* Called by after_allocation to set the information of data segment
    before relaxing.  */

@@ -4660,35 +4682,10 @@  bfd_elfNN_riscv_set_data_segment_info (struct
bfd_link_info *info,
   htab->data_segment_phase = data_segment_phase;
 }

-/* Called by after_allocation to check if we need to run the whole
-   relaxations again.  */
-
-bool
-bfd_elfNN_riscv_restart_relax_sections (struct bfd_link_info *info)
-{
-  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
-  bool restart = htab->restart_relax;
-  /* Reset the flag.  */
-  htab->restart_relax = false;
-  return restart;
-}
-
 /* Relax a section.

-   Pass 0: Shortens code sequences for LUI/CALL/TPREL relocs.
-   Pass 1: Shortens code sequences for PCREL relocs.
-   Pass 2: Deletes the bytes that pass 1 made obsolete.
-   Pass 3: Which cannot be disabled, handles code alignment directives.
-
-   The `again` is used to determine whether the relax pass itself needs to
-   run again.  And the `restart_relax` is used to determine if we need to
-   run the whole relax passes again from 0 to 2.  Once we have deleted the
-   code between relax pass 0 to 2, the restart_relax will be set to TRUE,
-   and we should run the whole relaxations again to give them more chances
-   to shorten the code.
-
-   Since we can't relax anything else once we start to handle the
alignments,
-   we will only enter into the relax pass 3 when the restart_relax is
FALSE.  */
+   Pass 0: Shortens code sequences for LUI/CALL/TPREL/PCREL relocs.
+   Pass 1: Which cannot be disabled, handles code alignment directives.  */

 static bool
 _bfd_riscv_relax_section (bfd *abfd, asection *sec,
@@ -4707,12 +4704,11 @@  _bfd_riscv_relax_section (bfd *abfd, asection *sec,
   *again = false;

   if (bfd_link_relocatable (info)
+      || sec->sec_flg0
       || (sec->flags & SEC_RELOC) == 0
       || sec->reloc_count == 0
       || (info->disable_target_specific_optimizations
-  && info->relax_pass < 2)
-      || (htab->restart_relax
-  && info->relax_pass == 3)
+  && info->relax_pass == 0)
       /* The exp_seg_relro_adjust is enum phase_enum (0x4),
  and defined in ld/ldexp.h.  */
       || *(htab->data_segment_phase) == 4)
@@ -4757,31 +4753,27 @@  _bfd_riscv_relax_section (bfd *abfd, asection *sec,
       || type == R_RISCV_CALL_PLT)
     relax_func = _bfd_riscv_relax_call;
   else if (type == R_RISCV_HI20
-   || type == R_RISCV_LO12_I
-   || type == R_RISCV_LO12_S)
+    || type == R_RISCV_LO12_I
+    || type == R_RISCV_LO12_S)
     relax_func = _bfd_riscv_relax_lui;
   else if (type == R_RISCV_TPREL_HI20
-   || type == R_RISCV_TPREL_ADD
-   || type == R_RISCV_TPREL_LO12_I
-   || type == R_RISCV_TPREL_LO12_S)
+    || type == R_RISCV_TPREL_ADD
+    || type == R_RISCV_TPREL_LO12_I
+    || type == R_RISCV_TPREL_LO12_S)
     relax_func = _bfd_riscv_relax_tls_le;
+  else if (!bfd_link_pic (info) && (type == R_RISCV_PCREL_HI20
+    || type == R_RISCV_PCREL_LO12_I
+    || type == R_RISCV_PCREL_LO12_S))
+    relax_func = _bfd_riscv_relax_pc;
   else
     continue;
  }
-      else if (info->relax_pass == 1
-       && !bfd_link_pic (info)
-       && (type == R_RISCV_PCREL_HI20
-   || type == R_RISCV_PCREL_LO12_I
-   || type == R_RISCV_PCREL_LO12_S))
- relax_func = _bfd_riscv_relax_pc;
-      else if (info->relax_pass == 2 && type == R_RISCV_DELETE)
- relax_func = _bfd_riscv_relax_delete;
-      else if (info->relax_pass == 3 && type == R_RISCV_ALIGN)
+      else if (info->relax_pass == 1 && type == R_RISCV_ALIGN)
  relax_func = _bfd_riscv_relax_align;
       else
  continue;

-      if (info->relax_pass < 2)
+      if (info->relax_pass == 0)
  {
   /* Only relax this reloc if it is paired with R_RISCV_RELAX.  */
   if (i == sec->reloc_count - 1
@@ -4954,9 +4946,6 @@  _bfd_riscv_relax_section (bfd *abfd, asection *sec,
     free (relocs);
   riscv_free_pcgp_relocs (&pcgp_relocs, abfd, sec);

-  if (*again)
-    htab->restart_relax = true;
-
   return ret;
 }

diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
index e691a97ecda..3af8fd99d9a 100644
--- a/bfd/elfxx-riscv.h
+++ b/bfd/elfxx-riscv.h
@@ -92,11 +92,6 @@  riscv_estimate_digit (unsigned);
 extern int
 riscv_compare_subsets (const char *, const char *);

-extern bool
-bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);
-extern bool
-bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);
-
 extern void
 bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);
 extern void
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 808191bd14e..9cfcdd88754 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,8 @@ 
+2021-10-07  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * emultempl/riscvelf.em: Revert restart_relax changes and set
+ relax_pass to 2.
+
 2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>

  * scripttempl/pru.sc (.resource_table): Align the output
diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
index c625a631fe5..865e3abd494 100644
--- a/ld/emultempl/riscvelf.em
+++ b/ld/emultempl/riscvelf.em
@@ -42,7 +42,7 @@  riscv_elf_before_allocation (void)
  ENABLE_RELAXATION;
     }

-  link_info.relax_pass = 4;
+  link_info.relax_pass = 2;
 }

 static void
@@ -76,11 +76,7 @@  gld${EMULATION_NAME}_after_allocation (void)
   enum phase_enum *phase = &(expld.dataseg.phase);
   bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *)
phase);

-  do
-    {
-      ldelf_map_segments (need_layout);
-    }
-  while (bfd_elf${ELFSIZE}_riscv_restart_relax_sections (&link_info));
+  ldelf_map_segments (need_layout);
 }

 /* This is a convenient point to tell BFD about target specific flags.
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d
b/ld/testsuite/ld-riscv-elf/align-small-region.d
new file mode 100644
index 00000000000..3799129c665
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.d
@@ -0,0 +1,12 @@ 
+#source: align-small-region.s
+#as: -march=rv32i
+#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100
+#objdump: -d
+
+.*:[ ]+file format .*
+
+Disassembly of section \.entry:
+
+00000000 <_reset>:
+.*:[ ]+[0-9a-f]+[ ]+j[ ]+100[ ]+<_start>
+#pass
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld
b/ld/testsuite/ld-riscv-elf/align-small-region.ld
new file mode 100644
index 00000000000..6a3e6638b26
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld
@@ -0,0 +1,12 @@ 
+MEMORY
+{
+ reset : ORIGIN = 0x0, LENGTH = 32
+}
+
+SECTIONS
+{
+ .entry :
+ {
+ KEEP (*(.entry))
+ } > reset
+}
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s
b/ld/testsuite/ld-riscv-elf/align-small-region.s
new file mode 100644
index 00000000000..ace81d640ed
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.s
@@ -0,0 +1,7 @@ 
+ .section .entry, "xa"
+ .align 5
+ .globl _reset
+ .type _reset, @function
+_reset:
+ tail _start
+ .size _reset, . - _reset
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index 11977611733..739cc04b889 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -119,6 +119,7 @@  proc run_relax_twice_test {} {
 }

 if [istarget "riscv*-*-*"] {
+    run_dump_test "align-small-region"
     run_dump_test "call-relax"
     run_dump_test "pcgp-relax"
     run_dump_test "c-lui"