Re: [PATCH] [PR ld/22263] RISC-V: Avoid spurious R_RISCV_NONE for pr22263-1 test.

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 4690 bytes --]

Committed since passed the riscv-gnu-toolchain gcc/binutils regressions.

Thanks
Nelson

On Sat, May 27, 2023 at 9:36 AM Nelson Chu <nelson@rivosinc.com> wrote:

> For TLS GD/IE, add the same condition with the relocate_section in the
> allocate_dynrelocs, to make sure we won't reserve redundant spaces
> for dynamic relocations since the conservative estimatation.
>
> After applying this patch, ld seems no longer generate the spurious
> R_RISCV_NONE for pr22263-1 test.
>
> bfd/
>         PR ld/22263
>         * elfnn-riscv.c (RISCV_TLS_GD_IE_NEED_DYN_RELOC): New defined.
>         Set NEED_RELOC to true if TLS GD/IE needs dynamic relocations,
>         and INDX will be the dynamic index.
>         (allocate_dynrelocs): Don't reserve extra spaces in the rela.got
>         if RISCV_TLS_GD_IE_NEED_DYN_RELOC set need_reloc to false.  This
>         condition needs to be same as relocate_section.
>         (relocate_section): Likewise, use the same condition as
>         allocate_dynrelocs.
> ---
>  bfd/elfnn-riscv.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 762ea231c0b..30d2faa405d 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -111,6 +111,25 @@
>     || (bfd_link_pic (INFO) \
>         && SYMBOL_REFERENCES_LOCAL ((INFO), (H))))
>
> +/* Set NEED_RELOC to true if TLS GD/IE needs dynamic relocations, and
> INDX will
> +   be the dynamic index.  PR22263, use the same check in
> allocate_dynrelocs and
> +   riscv_elf_relocate_section for TLS GD/IE.  */
> +#define RISCV_TLS_GD_IE_NEED_DYN_RELOC(INFO, DYN, H, INDX, NEED_RELOC) \
> +  do \
> +    { \
> +      if ((H) != NULL \
> +         && (H)->dynindx != -1 \
> +         && WILL_CALL_FINISH_DYNAMIC_SYMBOL ((DYN), bfd_link_pic (INFO),
> (H)) \
> +         && (bfd_link_dll (INFO) || !SYMBOL_REFERENCES_LOCAL ((INFO),
> (H)))) \
> +       (INDX) = (H)->dynindx; \
> +      if ((bfd_link_dll (INFO) || (INDX) != 0) \
> +         && ((H) == NULL \
> +             || ELF_ST_VISIBILITY ((H)->other) == STV_DEFAULT \
> +             || (H)->root.type != bfd_link_hash_undefweak)) \
> +       (NEED_RELOC) = true; \
> +    } \
> +  while (0)
> +
>  /* Internal relocations used exclusively by the relaxation pass.  */
>  #define R_RISCV_DELETE (R_RISCV_max + 1)
>
> @@ -1297,18 +1316,24 @@ allocate_dynrelocs (struct elf_link_hash_entry *h,
> void *inf)
>        dyn = htab->elf.dynamic_sections_created;
>        if (tls_type & (GOT_TLS_GD | GOT_TLS_IE))
>         {
> +         int indx = 0;
> +         bool need_reloc = false;
> +         RISCV_TLS_GD_IE_NEED_DYN_RELOC(info, dyn, h, indx, need_reloc);
> +
>           /* TLS_GD needs two dynamic relocs and two GOT slots.  */
>           if (tls_type & GOT_TLS_GD)
>             {
>               s->size += 2 * RISCV_ELF_WORD_BYTES;
> -             htab->elf.srelgot->size += 2 * sizeof (ElfNN_External_Rela);
> +             if (need_reloc)
> +               htab->elf.srelgot->size += 2 * sizeof
> (ElfNN_External_Rela);
>             }
>
>           /* TLS_IE needs one dynamic reloc and one GOT slot.  */
>           if (tls_type & GOT_TLS_IE)
>             {
>               s->size += RISCV_ELF_WORD_BYTES;
> -             htab->elf.srelgot->size += sizeof (ElfNN_External_Rela);
> +             if (need_reloc)
> +               htab->elf.srelgot->size += sizeof (ElfNN_External_Rela);
>             }
>         }
>        else
> @@ -2882,20 +2907,10 @@ riscv_elf_relocate_section (bfd *output_bfd,
>                 abort ();
>
>               bool dyn = elf_hash_table (info)->dynamic_sections_created;
> -             if (h != NULL
> -                 && h->dynindx != -1
> -                 && WILL_CALL_FINISH_DYNAMIC_SYMBOL (dyn, bfd_link_pic
> (info), h)
> -                 && (bfd_link_dll (info) || !SYMBOL_REFERENCES_LOCAL
> (info, h)))
> -               indx = h->dynindx;
> +             RISCV_TLS_GD_IE_NEED_DYN_RELOC (info, dyn, h, indx,
> need_relocs);
>
>               /* The GOT entries have not been initialized yet.  Do it
>                  now, and emit any relocations.  */
> -             if ((bfd_link_dll (info) || indx != 0)
> -                 && (h == NULL
> -                     || ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
> -                     || h->root.type != bfd_link_hash_undefweak))
> -               need_relocs = true;
> -
>               if (tls_type & GOT_TLS_GD)
>                 {
>                   if (need_relocs)
> --
> 2.39.2 (Apple Git-143)
>
>