Re: [PATCH] RISC-V: Support subtraction of .uleb128.

[Kito Cheng <kito.cheng@gmail.com>]

LGTM, thanks!

This patch actually has been verified within SiFive internally for
years and works well, and finally the psABI spec part has settled down
now, it's really good to see this supported in binutils.

NOTE: LLVM/LLD patch is review in progress:
https://reviews.llvm.org/D142879 https://reviews.llvm.org/D142880

On Thu, May 18, 2023 at 5:23 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
> From: Kuan-Lin Chen <rufus@andestech.com>
>
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/96d6e190e9fc04a8517f9ff7fb9aed3e9876cbd6
>
> There are some known limitations for now,
>
> * Do not shrink the length of the uleb128 value, even if the value is reduced
> after relaxations.  Also reports error if the length grows up.
>
> * The R_RISCV_SET_ULEB128 needs to be paired with and be placed before the
> R_RISCV_SUB_ULEB128.
>
> bfd/
>         * bfd-in2.h: Regenerated.
>         * elfnn-riscv.c (perform_relocation): Perform R_RISCV_SUB_ULEB128 and
>         R_RISCV_SET_ULEB128 relocations.  Do not shrink the length of the
>         uleb128 value, and report error if the length grows up.  Called the
>         generic functions, _bfd_read_unsigned_leb128 and _bfd_write_unsigned_leb128,
>         to encode the uleb128 into the section contents.
>         (riscv_elf_relocate_section): Make sure that the R_RISCV_SET_ULEB128
>         must be paired with and be placed before the R_RISCV_SUB_ULEB128.
>         * elfxx-riscv.c (howto_table): Added R_RISCV_SUB_ULEB128 and
>         R_RISCV_SET_ULEB128.
>         (riscv_reloc_map): Likewise.
>         (riscv_elf_ignore_reloc): New function.
>         * libbfd.h: Regenerated.
>         * reloc.c (BFD_RELOC_RISCV_SET_ULEB128, BFD_RELOC_RISCV_SUB_ULEB128):
>         New relocations to support .uleb128 subtraction.
> gas/
>         * config/tc-riscv.c (md_apply_fix): Added BFD_RELOC_RISCV_SET_ULEB128
>         and BFD_RELOC_RISCV_SUB_ULEB128.
>         (s_riscv_leb128): Updated to allow uleb128 subtraction.
>         (riscv_insert_uleb128_fixes): New function, scan uleb128 subtraction
>         expressions and insert fixups for them.
>         (riscv_md_finish): Called riscv_insert_uleb128_fixes for all sections.
> include/
>         * elf/riscv.h ((R_RISCV_SET_ULEB128, (R_RISCV_SUB_ULEB128): Defined.
> ld/
>         * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
>         * testsuite/ld-riscv-elf/uleb128*: New testcase for uleb128 subtraction.
> binutils/
>         * testsuite/binutils-all/nm.exp: Updated since RISCV supports .uleb128.
> ---
>  bfd/bfd-in2.h                              |  2 +
>  bfd/elfnn-riscv.c                          | 83 ++++++++++++++++++++++
>  bfd/elfxx-riscv.c                          | 54 ++++++++++++++
>  bfd/libbfd.h                               |  2 +
>  bfd/reloc.c                                |  4 ++
>  binutils/testsuite/binutils-all/nm.exp     |  2 -
>  gas/config/tc-riscv.c                      | 56 ++++++++++++++-
>  include/elf/riscv.h                        |  3 +
>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp |  1 +
>  ld/testsuite/ld-riscv-elf/uleb128.d        | 18 +++++
>  ld/testsuite/ld-riscv-elf/uleb128.s        | 18 +++++
>  11 files changed, 239 insertions(+), 4 deletions(-)
>  create mode 100644 ld/testsuite/ld-riscv-elf/uleb128.d
>  create mode 100644 ld/testsuite/ld-riscv-elf/uleb128.s
>
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 7be18db20a8..363a6b35a5c 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -5394,6 +5394,8 @@ number for the SBIC, SBIS, SBI and CBI instructions  */
>    BFD_RELOC_RISCV_SET16,
>    BFD_RELOC_RISCV_SET32,
>    BFD_RELOC_RISCV_32_PCREL,
> +  BFD_RELOC_RISCV_SET_ULEB128,
> +  BFD_RELOC_RISCV_SUB_ULEB128,
>
>  /* Renesas RL78 Relocations.  */
>    BFD_RELOC_RL78_NEG8,
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index a23b91ac15c..75af040cf92 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -1792,6 +1792,55 @@ perform_relocation (const reloc_howto_type *howto,
>         value = ENCODE_CITYPE_LUI_IMM (RISCV_CONST_HIGH_PART (value));
>        break;
>
> +    /* SUB_ULEB128 must be applied after SET_ULEB128, so we only write the
> +       value back for SUB_ULEB128 should be enough.  */
> +    case R_RISCV_SET_ULEB128:
> +      break;
> +    case R_RISCV_SUB_ULEB128:
> +      {
> +       unsigned int len = 0;
> +       _bfd_read_unsigned_leb128 (input_bfd, contents + rel->r_offset, &len);
> +
> +       /* Clean the contents value to zero (0x80), but keep the original
> +          length.  */
> +       bfd_byte *p = contents + rel->r_offset;
> +       bfd_byte *endp = p + len - 1;
> +       memset (p, 0x80, len - 1);
> +       *(endp) = 0;
> +
> +       /* Make sure the length of the new uleb128 value within the
> +          original (available) length.  */
> +       unsigned int new_len = 0;
> +       unsigned int val_t = value;
> +       do
> +         {
> +           new_len++;
> +           val_t >>= 7;
> +         }
> +       while (val_t);
> +       if (new_len > len)
> +         {
> +           _bfd_error_handler
> +             (_("final size of uleb128 value at offset 0x%lx in %pA from "
> +                "%pB exceeds available space"),
> +              (long) rel->r_offset, input_section, input_bfd);
> +           return bfd_reloc_dangerous;
> +         }
> +       else
> +         {
> +           p = _bfd_write_unsigned_leb128 (p, endp, value);
> +           BFD_ASSERT (p);
> +
> +           /* If the length of the value is reduced and shorter than the
> +              original uleb128 length, then _bfd_write_unsigned_leb128 may
> +              clear the 0x80 to 0x0 for the last byte that was written.
> +              So reset it to keep the the original uleb128 length.  */
> +           if (--p < endp)
> +             *p |= 0x80;
> +         }
> +       return bfd_reloc_ok;
> +      }
> +
>      case R_RISCV_32:
>      case R_RISCV_64:
>      case R_RISCV_ADD8:
> @@ -2086,6 +2135,8 @@ riscv_elf_relocate_section (bfd *output_bfd,
>    Elf_Internal_Shdr *symtab_hdr = &elf_symtab_hdr (input_bfd);
>    struct elf_link_hash_entry **sym_hashes = elf_sym_hashes (input_bfd);
>    bfd_vma *local_got_offsets = elf_local_got_offsets (input_bfd);
> +  bfd_vma uleb128_set_vma = 0;
> +  Elf_Internal_Rela *uleb128_set_rel = NULL;
>    bool absolute;
>
>    if (!riscv_init_pcrel_relocs (&pcrel_relocs))
> @@ -2427,6 +2478,38 @@ riscv_elf_relocate_section (bfd *output_bfd,
>           /* These require no special handling beyond perform_relocation.  */
>           break;
>
> +       case R_RISCV_SET_ULEB128:
> +         if (uleb128_set_rel == NULL)
> +           {
> +             /* Saved for later usage.  */
> +             uleb128_set_vma = relocation;
> +             uleb128_set_rel = rel;
> +             continue;
> +           }
> +         else
> +           {
> +             msg = ("Mismatched R_RISCV_SET_ULEB128, it must be paired with"
> +                    "and applied before R_RISCV_SUB_ULEB128");
> +             r = bfd_reloc_dangerous;
> +           }
> +         break;
> +
> +       case R_RISCV_SUB_ULEB128:
> +         if (uleb128_set_rel != NULL
> +             && uleb128_set_rel->r_offset == rel->r_offset)
> +           {
> +             relocation = uleb128_set_vma - relocation;
> +             uleb128_set_vma = 0;
> +             uleb128_set_rel = NULL;
> +           }
> +         else
> +           {
> +             msg = ("Mismatched R_RISCV_SUB_ULEB128, it must be paired with"
> +                    "and applied after R_RISCV_SET_ULEB128");
> +             r = bfd_reloc_dangerous;
> +           }
> +         break;
> +
>         case R_RISCV_GOT_HI20:
>           if (h != NULL)
>             {
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index bd0c7c81329..7f453246449 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -38,6 +38,8 @@
>     relocations for the debug info.  */
>  static bfd_reloc_status_type riscv_elf_add_sub_reloc
>    (bfd *, arelent *, asymbol *, void *, asection *, bfd *, char **);
> +static bfd_reloc_status_type riscv_elf_ignore_reloc
> +  (bfd *, arelent *, asymbol *, void *, asection *, bfd *, char **);
>
>  /* The relocation table used for SHT_RELA sections.  */
>
> @@ -844,6 +846,39 @@ static reloc_howto_type howto_table[] =
>          0,                             /* src_mask */
>          0xffffffff,                    /* dst_mask */
>          false),                        /* pcrel_offset */
> +
> +  /* Reserved for R_RISCV_PLT32.  */
> +  EMPTY_HOWTO (59),
> +
> +  /* N-bit in-place setting, for unsigned-leb128 local label subtraction.  */
> +  HOWTO (R_RISCV_SET_ULEB128,          /* type */
> +        0,                             /* rightshift */
> +        0,                             /* size */
> +        0,                             /* bitsize */
> +        false,                         /* pc_relative */
> +        0,                             /* bitpos */
> +        complain_overflow_dont,        /* complain_on_overflow */
> +        riscv_elf_ignore_reloc,        /* special_function */
> +        "R_RISCV_SET_ULEB128",         /* name */
> +        false,                         /* partial_inplace */
> +        0,                             /* src_mask */
> +        0,                             /* dst_mask */
> +        false),                        /* pcrel_offset */
> +
> +  /* N-bit in-place addition, for unsigned-leb128 local label subtraction.  */
> +  HOWTO (R_RISCV_SUB_ULEB128,          /* type */
> +        0,                             /* rightshift */
> +        0,                             /* size */
> +        0,                             /* bitsize */
> +        false,                         /* pc_relative */
> +        0,                             /* bitpos */
> +        complain_overflow_dont,        /* complain_on_overflow */
> +        riscv_elf_ignore_reloc,        /* special_function */
> +        "R_RISCV_SUB_ULEB128",         /* name */
> +        false,                         /* partial_inplace */
> +        0,                             /* src_mask */
> +        0,                             /* dst_mask */
> +        false),                        /* pcrel_offset */
>  };
>
>  /* A mapping from BFD reloc types to RISC-V ELF reloc types.  */
> @@ -905,6 +940,8 @@ static const struct elf_reloc_map riscv_reloc_map[] =
>    { BFD_RELOC_RISCV_SET16, R_RISCV_SET16 },
>    { BFD_RELOC_RISCV_SET32, R_RISCV_SET32 },
>    { BFD_RELOC_RISCV_32_PCREL, R_RISCV_32_PCREL },
> +  { BFD_RELOC_RISCV_SET_ULEB128, R_RISCV_SET_ULEB128 },
> +  { BFD_RELOC_RISCV_SUB_ULEB128, R_RISCV_SUB_ULEB128 },
>  };
>
>  /* Given a BFD reloc type, return a howto structure.  */
> @@ -1010,6 +1047,23 @@ riscv_elf_add_sub_reloc (bfd *abfd,
>    return bfd_reloc_ok;
>  }
>
> +/* Special handler for relocations which don't have to be relocated.
> +   This function just simply return bfd_reloc_ok.  */
> +
> +static bfd_reloc_status_type
> +riscv_elf_ignore_reloc (bfd *abfd ATTRIBUTE_UNUSED,
> +                       arelent *reloc_entry,
> +                       asymbol *symbol ATTRIBUTE_UNUSED,
> +                       void *data ATTRIBUTE_UNUSED,
> +                       asection *input_section,
> +                       bfd *output_bfd,
> +                       char **error_message ATTRIBUTE_UNUSED)
> +{
> +  if (output_bfd != NULL)
> +    reloc_entry->address += input_section->output_offset;
> +  return bfd_reloc_ok;
> +}
> +
>  /* Always add the IMPLICIT for the SUBSET.  */
>
>  static bool
> diff --git a/bfd/libbfd.h b/bfd/libbfd.h
> index 05508c986ad..f29a65a4b22 100644
> --- a/bfd/libbfd.h
> +++ b/bfd/libbfd.h
> @@ -2417,6 +2417,8 @@ static const char *const bfd_reloc_code_real_names[] = { "@@uninitialized@@",
>    "BFD_RELOC_RISCV_SET16",
>    "BFD_RELOC_RISCV_SET32",
>    "BFD_RELOC_RISCV_32_PCREL",
> +  "BFD_RELOC_RISCV_SET_ULEB128",
> +  "BFD_RELOC_RISCV_SUB_ULEB128",
>    "BFD_RELOC_RL78_NEG8",
>    "BFD_RELOC_RL78_NEG16",
>    "BFD_RELOC_RL78_NEG24",
> diff --git a/bfd/reloc.c b/bfd/reloc.c
> index aab5d49bdb3..c7d11cf5f40 100644
> --- a/bfd/reloc.c
> +++ b/bfd/reloc.c
> @@ -5048,6 +5048,10 @@ ENUMX
>    BFD_RELOC_RISCV_SET32
>  ENUMX
>    BFD_RELOC_RISCV_32_PCREL
> +ENUMX
> +  BFD_RELOC_RISCV_SET_ULEB128
> +ENUMX
> +  BFD_RELOC_RISCV_SUB_ULEB128
>  ENUMDOC
>    RISC-V relocations.
>
> diff --git a/binutils/testsuite/binutils-all/nm.exp b/binutils/testsuite/binutils-all/nm.exp
> index d0f4c3e275c..91b519d9074 100644
> --- a/binutils/testsuite/binutils-all/nm.exp
> +++ b/binutils/testsuite/binutils-all/nm.exp
> @@ -246,8 +246,6 @@ if [is_elf_format] {
>      # Test nm --line-numbers on DWARF-4 debug info.
>      set testname "nm --line-numbers on DWARF-4 debug info"
>
> -    # The RISCV target does not (currently) support .uleb128.
> -    setup_xfail "riscv*-*-*"
>      # The SH targets complain that the pseudo-ops used to construct
>      # the DWARF data are misaligned.
>      setup_xfail "sh*-*-*"
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 0cc2484b049..fceb53e54a6 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -3950,6 +3950,9 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
>      case BFD_RELOC_RISCV_SUB32:
>      case BFD_RELOC_RISCV_SUB64:
>      case BFD_RELOC_RISCV_RELAX:
> +    /* cvt_frag_to_fill () has called output_leb128 ().  */
> +    case BFD_RELOC_RISCV_SET_ULEB128:
> +    case BFD_RELOC_RISCV_SUB_ULEB128:
>        break;
>
>      case BFD_RELOC_RISCV_TPREL_HI20:
> @@ -4714,8 +4717,11 @@ s_riscv_leb128 (int sign)
>    char *save_in = input_line_pointer;
>
>    expression (&exp);
> -  if (exp.X_op != O_constant)
> -    as_bad (_("non-constant .%cleb128 is not supported"), sign ? 's' : 'u');
> +  if (sign && exp.X_op != O_constant)
> +    as_bad (_("non-constant .sleb128 is not supported"));
> +  else if (!sign && exp.X_op != O_constant && exp.X_op != O_subtract)
> +    as_bad (_(".uleb128 only supports constant or subtract expressions"));
> +
>    demand_empty_rest_of_line ();
>
>    input_line_pointer = save_in;
> @@ -4835,12 +4841,58 @@ riscv_set_public_attributes (void)
>      riscv_write_out_attrs ();
>  }
>
> +/* Scan uleb128 subtraction expressions and insert fixups for them.
> +   e.g., .uleb128 .L1 - .L0
> +   Because relaxation may change the value of the subtraction, we
> +   must resolve them at link-time.  */
> +
> +static void
> +riscv_insert_uleb128_fixes (bfd *abfd ATTRIBUTE_UNUSED,
> +                           asection *sec, void *xxx ATTRIBUTE_UNUSED)
> +{
> +  segment_info_type *seginfo = seg_info (sec);
> +  struct frag *fragP;
> +
> +  subseg_set (sec, 0);
> +
> +  for (fragP = seginfo->frchainP->frch_root;
> +       fragP; fragP = fragP->fr_next)
> +    {
> +      expressionS *exp, *exp_dup;
> +
> +      if (fragP->fr_type != rs_leb128  || fragP->fr_symbol == NULL)
> +       continue;
> +
> +      exp = symbol_get_value_expression (fragP->fr_symbol);
> +
> +      if (exp->X_op != O_subtract)
> +       continue;
> +
> +      /* Only unsigned leb128 can be handled.  */
> +      gas_assert (fragP->fr_subtype == 0);
> +      exp_dup = xmemdup (exp, sizeof (*exp), sizeof (*exp));
> +      exp_dup->X_op = O_symbol;
> +      exp_dup->X_op_symbol = NULL;
> +
> +      /* Insert relocations to resolve the subtraction at link-time.
> +        Emit the SET relocation first in riscv.  */
> +      exp_dup->X_add_symbol = exp->X_add_symbol;
> +      fix_new_exp (fragP, fragP->fr_fix, 0,
> +                  exp_dup, 0, BFD_RELOC_RISCV_SET_ULEB128);
> +      exp_dup->X_add_symbol = exp->X_op_symbol;
> +      fix_new_exp (fragP, fragP->fr_fix, 0,
> +                  exp_dup, 0, BFD_RELOC_RISCV_SUB_ULEB128);
> +    }
> +}
> +
>  /* Called after all assembly has been done.  */
>
>  void
>  riscv_md_finish (void)
>  {
>    riscv_set_public_attributes ();
> +  if (riscv_opts.relax)
> +    bfd_map_over_sections (stdoutput, riscv_insert_uleb128_fixes, NULL);
>  }
>
>  /* Adjust the symbol table.  */
> diff --git a/include/elf/riscv.h b/include/elf/riscv.h
> index aabc71cf979..0aa8b3359c4 100644
> --- a/include/elf/riscv.h
> +++ b/include/elf/riscv.h
> @@ -87,6 +87,9 @@ START_RELOC_NUMBERS (elf_riscv_reloc_type)
>    RELOC_NUMBER (R_RISCV_SET32, 56)
>    RELOC_NUMBER (R_RISCV_32_PCREL, 57)
>    RELOC_NUMBER (R_RISCV_IRELATIVE, 58)
> +  /* Reserved 59 for R_RISCV_PLT32.  */
> +  RELOC_NUMBER (R_RISCV_SET_ULEB128, 60)
> +  RELOC_NUMBER (R_RISCV_SUB_ULEB128, 61)
>  END_RELOC_NUMBERS (R_RISCV_max)
>
>  /* Processor specific flags for the ELF header e_flags field.  */
> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> index 9e103b283f7..947a266ba72 100644
> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> @@ -172,6 +172,7 @@ if [istarget "riscv*-*-*"] {
>      run_dump_test "attr-merge-priv-spec-failed-06"
>      run_dump_test "attr-phdr"
>      run_dump_test "relax-max-align-gp"
> +    run_dump_test "uleb128"
>      run_ld_link_tests [list \
>         [list "Weak reference 32" "-T weakref.ld -m[riscv_choose_ilp32_emul]" "" \
>             "-march=rv32i -mabi=ilp32" {weakref32.s} \
> diff --git a/ld/testsuite/ld-riscv-elf/uleb128.d b/ld/testsuite/ld-riscv-elf/uleb128.d
> new file mode 100644
> index 00000000000..a921478e988
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/uleb128.d
> @@ -0,0 +1,18 @@
> +#source: uleb128.s
> +#as: -march=rv32ic
> +#ld: -melf32lriscv
> +#objdump: -d
> +
> +.*:[   ]+file format .*
> +
> +Disassembly of section .text:
> +
> +.* <_start>:
> +.*jal.*<bar>
> +.*jal.*<bar>
> +.*jal.*<bar>
> +.*jal.*<bar>
> +.*jal.*<bar>
> +.*jal.*<bar>
> +.*:[   ]+0e0c.*
> +#pass
> diff --git a/ld/testsuite/ld-riscv-elf/uleb128.s b/ld/testsuite/ld-riscv-elf/uleb128.s
> new file mode 100644
> index 00000000000..f7d23be1635
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/uleb128.s
> @@ -0,0 +1,18 @@
> +.text
> +.globl bar
> +.globl _start
> +.option rvc
> +.align 2
> +_start:
> +.L0:
> +        .rept 6
> +        call bar
> +        .endr
> +.align 2
> +.L1:
> +        .uleb128 .L1 - .L0
> +        .uleb128 .L2 - .L0
> +.L2:
> +.align 2
> +bar:
> +        nop
> --
> 2.39.2 (Apple Git-143)
>