[PATCH v2 1/3] RISC-V: Add support for vendor-specific relocations

[Kito Cheng]

Hi Ethan:

Thanks for your patch, inline comment below :)

> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 8263be49d56..adb35aa342d 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -279,7 +279,28 @@ riscv_info_to_howto_rela (bfd *abfd,
>                           arelent *cache_ptr,
>                           Elf_Internal_Rela *dst)
>  {
> -  cache_ptr->howto = riscv_elf_rtype_to_howto (abfd, ELFNN_R_TYPE (dst->r_info));
> +  static enum elf_riscv_vendor_id vendor_id = RISCV_VENDOR_ID_NONE;

Put this into global var rather than static local, also plz record the addr
of the R_RISCV_VENDOR to make sure the next relocation is the expected
vendor-specific relocation.

And testcase for this commit, e.g.
https://github.com/llvm/llvm-project/tree/main/lld/test/ELF/riscv-vendor-relocations.s

> +  unsigned int r_type = ELFNN_R_TYPE (dst->r_info);
> +
> +  cache_ptr->howto
> +      = riscv_elf_rtype_to_howto (abfd, ELFNN_R_TYPE (dst->r_info), vendor_id);
> +  vendor_id = RISCV_VENDOR_ID_NONE;
> +
> +  /* Record the vendor id for use by the next relocation.  */
> +  if (r_type == R_RISCV_VENDOR)
> +    {
> +      const char *vendor_str = bfd_asymbol_name (*cache_ptr->sym_ptr_ptr);
> +      vendor_id = riscv_vendor_str_to_id (vendor_str);
> +      if (vendor_id == RISCV_VENDOR_ID_NONE)
> +       {
> +         _bfd_error_handler (_("error: %pB: vendor-specific (%s) "
> +                                "relocations are not supported"),
> +                             abfd, vendor_str);
> +         bfd_set_error (bfd_error_bad_value);
> +         return false;
> +       }
> +    }
> +
>    return cache_ptr->howto != NULL;
>  }
>
...
>  reloc_howto_type *
> -riscv_elf_rtype_to_howto (bfd *abfd, unsigned int r_type)
> +riscv_elf_rtype_to_howto (bfd *abfd, unsigned int r_type,
> +                         enum elf_riscv_vendor_id vendor_id)

Plz add riscv_elf_rtype_to_howto_vid and keep riscv_elf_rtype_to_howto
unchanged, so that we don't need to pass RISCV_VENDOR_ID_NONE in many place.

>  {
> -  if (r_type < ARRAY_SIZE (howto_table))
> -    return &howto_table[r_type];
> -  else if (r_type < R_RISCV_max + ARRAY_SIZE (howto_table_internal))
> -    return &howto_table_internal[r_type - R_RISCV_max];
> +  if (vendor_id == RISCV_VENDOR_ID_NONE)
> +    {
> +      if (r_type >= R_RISCV_internal_first
> +         && r_type < (R_RISCV_internal_first
> +                      + ARRAY_SIZE (howto_table_internal)))
> +       return &howto_table_internal[r_type - R_RISCV_internal_first];
> +      else if (r_type < ARRAY_SIZE (howto_table))
> +       return &howto_table[r_type];
> +    }
>    else
>      {
> -      (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"),
> -                            abfd, r_type);
> -      bfd_set_error (bfd_error_bad_value);
> -      return NULL;
> +      if (vendor_id > RISCV_VENDOR_ID_NONE
> +         && vendor_id < RISCV_VENDOR_ID_COUNT
> +         && r_type > R_RISCV_VENDOR
> +         && r_type <= R_RISCV_VENDOR + howto_table_vendor_sizes[vendor_id])

Plz also check the address of this reloc is same as R_RISCV_VENDOR.

> +       return &howto_table_vendors[vendor_id][r_type - R_RISCV_VENDOR - 1];
>      }
> +
> +  if (vendor_id == RISCV_VENDOR_ID_NONE)
> +    (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"), abfd,
> +                          r_type);
> +  else
> +    (*_bfd_error_handler) (
> +       _("%pB: unsupported relocation type %#x for vendor id %d"), abfd,
> +       r_type, (int)vendor_id);
> +
> +  bfd_set_error (bfd_error_bad_value);
> +  return NULL;
> +}
> +
> diff --git a/include/elf/riscv.h b/include/elf/riscv.h
> index f6ca3d4acd8..2dbafb0a0c0 100644
> --- a/include/elf/riscv.h
> +++ b/include/elf/riscv.h
> @@ -95,16 +95,63 @@ START_RELOC_NUMBERS (elf_riscv_reloc_type)
>    RELOC_NUMBER (R_RISCV_TLSDESC_LOAD_LO12, 63)
>    RELOC_NUMBER (R_RISCV_TLSDESC_ADD_LO12, 64)
>    RELOC_NUMBER (R_RISCV_TLSDESC_CALL, 65)
> -END_RELOC_NUMBERS (R_RISCV_max)
> +  EMPTY_RELOC (R_RISCV_internal_first)

Deinfe R_RISCV_internal_first as 256, that could prevent internal relocation
conflict with R_RISCV_VENDOR and all further reolcation in future.

> +/* Given a vendor string and a relocation number, return a relocation name
> +   string.  */
> +static const char *
> +elf_riscv_vendor_reloc_type (const char *vendor_str, unsigned long rtype)

Could you convert vendor_str to vendor_id first.

> +{
> +  unsigned int i;
> +
> +  /* Choose a vendor-defined function according to the vendor string. */
> +  for (i = 0; i < ARRAY_SIZE (elf_riscv_vendor_str_table); i++)
> +    if (elf_riscv_vendor_str_table[i] && vendor_str
> +       && strcmp (elf_riscv_vendor_str_table[i], vendor_str) == 0)

Then we could prevent compare the string again and again?

> +      return elf_riscv_vendor_reloc_type_funcs[i](rtype);
> +
> +  return NULL;
> +}
> +#endif