Re: [PATCH] RISCV-V: Add new relocation type for global array accesses with non-constant indices

Jan Beulich <[email protected]>
Newsgroups gmane.comp.gnu.binutils
Message-ID <[email protected]> 
On 08.07.2026 13:17, 翁丽琴 wrote:
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,3 +1,16 @@
> +2026-06-22  wengliqin  <[email protected]>
> +
> +        * reloc.c (BFD_RELOC_RISCV_BASE_IDX_LO12_I, BFD_RELOC_RISCV_BASE_IDX_LO12_S,
> +        BFD_RELOC_RISCV_BASE_IDX_ADD): New
> +        relocation types.
> +        * bfd-in2.h: Regenerate.
> +        * libbfd.h: Regenerate.
> +        * elfnn-riscv.c (perform_relocation): Add support for new REGREL
> +        relocations.
> +        (riscv_elf_relocate_section): Handle REGREL relocations for global
> +        array accesses with non-constant indices.
> +        * elfxx-riscv.c: Define relocation information for new REGREL types.
> +
>  2025-09-16  Rainer Orth  <[email protected]>
>  
>          * config.bfd <powerpc-*-solaris2*>: Remove.

Btw, as you can see from the adjacent date: ChangeLog files aren't meant to be
patched directly anymore. ChangeLog entries, if you want to have them, are now
supposed to be part of the commit message.

There also looks to be a problem with your mailing of patches: The entry in
context really uses tab indentation (as it is supposed to be), yet throughout
the entire patch there's no single hard tab (when elsewhere they ought to be
used for indentation as well).

> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -876,7 +876,63 @@ static const reloc_howto_type howto_table[] =
>           false,                                /* partial_inplace */
>           0,                                /* src_mask */
>           ENCODE_ITYPE_IMM (-1U),        /* dst_mask */
> -         false)                         /* pcrel_offset */
> +         false),                        /* pcrel_offset */
> +
> +  /* Reserved slots 66-76.  */
> +  EMPTY_HOWTO (66),
> +  EMPTY_HOWTO (67),
> +  EMPTY_HOWTO (68),
> +  EMPTY_HOWTO (69),
> +  EMPTY_HOWTO (70),
> +  EMPTY_HOWTO (71),
> +  EMPTY_HOWTO (72),
> +  EMPTY_HOWTO (73),
> +  EMPTY_HOWTO (74),
> +  EMPTY_HOWTO (75),
> +  EMPTY_HOWTO (76),
> +
> +  /* Global array accesses with non-constant subscript (base + index).  */
> +  HOWTO (R_RISCV_BASE_IDX_LO12_I,   /* type */
> +         0,                            /* rightshift */
> +         4,                            /* size */
> +         32,                            /* bitsize */

I previously commented on this (and that comment applies to all new entries).

Also the comments on the right all want to have their /* align with one
another (to aid legibility).

> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1958,6 +1958,12 @@ riscv_apply_const_reloc (bfd_reloc_code_real_type reloc_type, bfd_vma value)
>      case BFD_RELOC_RISCV_LO12_I:
>        return ENCODE_ITYPE_IMM (value);
>  
> +    case BFD_RELOC_RISCV_BASE_IDX_LO12_S:
> +      return ENCODE_STYPE_IMM (value);
> +
> +    case BFD_RELOC_RISCV_BASE_IDX_LO12_I:
> +      return ENCODE_ITYPE_IMM (value);

I'm not a maintainer of this file, but I find this odd: Why can't all case
labels wanting the same treatment be grouped together, to limit redundancy?

> @@ -2022,7 +2028,8 @@ append_insn (struct riscv_cl_insn *ip, expressionS *address_expr,
>    if (reloc_type == BFD_RELOC_RISCV_HI20
>        || reloc_type == BFD_RELOC_RISCV_PCREL_HI20
>        || reloc_type == BFD_RELOC_RISCV_TPREL_HI20
> -      || reloc_type == BFD_RELOC_RISCV_TPREL_ADD)
> +      || reloc_type == BFD_RELOC_RISCV_TPREL_ADD
> +      || reloc_type == BFD_RELOC_RISCV_BASE_IDX_ADD)
>      {
>        frag_wane (frag_now);
>        frag_new (0);
> @@ -2452,30 +2459,27 @@ static const struct percent_op_match percent_op_utype[] =
>    {0, 0}
>  };
>  
> -static const struct percent_op_match percent_op_itype[] =
> -{
> -  {"lo", BFD_RELOC_RISCV_LO12_I},
> -  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
> -  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
> -  {"tlsdesc_load_lo", BFD_RELOC_RISCV_TLSDESC_LOAD_LO12},
> -  {"tlsdesc_add_lo", BFD_RELOC_RISCV_TLSDESC_ADD_LO12},
> -  {0, 0}
> -};
> -
> -static const struct percent_op_match percent_op_stype[] =
> -{
> -  {"lo", BFD_RELOC_RISCV_LO12_S},
> -  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
> -  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
> -  {0, 0}
> -};
> -
> -static const struct percent_op_match percent_op_relax_only[] =
> -{
> -  {"tlsdesc_call", BFD_RELOC_RISCV_TLSDESC_CALL},
> -  {"tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
> -  {0, 0}
> -};
> +static const struct percent_op_match percent_op_itype[]
> +  = {{"lo", BFD_RELOC_RISCV_LO12_I},
> +     {"base_idx_lo", BFD_RELOC_RISCV_BASE_IDX_LO12_I},
> +     {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
> +     {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
> +     {"tlsdesc_load_lo", BFD_RELOC_RISCV_TLSDESC_LOAD_LO12},
> +     {"tlsdesc_add_lo", BFD_RELOC_RISCV_TLSDESC_ADD_LO12},
> +     {0, 0}};
> +
> +static const struct percent_op_match percent_op_stype[]
> +  = {{"lo", BFD_RELOC_RISCV_LO12_S},
> +     {"base_idx_lo", BFD_RELOC_RISCV_BASE_IDX_LO12_S},
> +     {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
> +     {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
> +     {0, 0}};
> +
> +static const struct percent_op_match percent_op_relax_only[]
> +  = {{"tlsdesc_call", BFD_RELOC_RISCV_TLSDESC_CALL},
> +     {"tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
> +     {"base_idx_add", BFD_RELOC_RISCV_BASE_IDX_ADD},
> +     {0, 0}};
>  
>  static const struct percent_op_match percent_op_null[] =
>  {

There's far more churn here than necessary. Why don't you simply add the
new lines you need, without otherwise altering what's there?

> @@ -3633,15 +3637,24 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
>  
>              case 'j': /* Sign-extended immediate.  */
>                p = percent_op_itype;
> -              *imm_reloc = BFD_RELOC_RISCV_LO12_I;
> +              if (strncmp (asarg, "%base_idx_lo", 9) == 0)
> +                *imm_reloc = BFD_RELOC_RISCV_BASE_IDX_LO12_I;
> +              else
> +                *imm_reloc = BFD_RELOC_RISCV_LO12_I;
>                goto alu_op;
>              case 'q': /* Store displacement.  */
>                p = percent_op_stype;
> -              *imm_reloc = BFD_RELOC_RISCV_LO12_S;
> +              if (strncmp (asarg, "%base_idx_lo", 9) == 0)
> +                *imm_reloc = BFD_RELOC_RISCV_BASE_IDX_LO12_S;
> +              else
> +                *imm_reloc = BFD_RELOC_RISCV_LO12_S;
>                goto load_store;
>              case 'o': /* Load displacement.  */
>                p = percent_op_itype;
> -              *imm_reloc = BFD_RELOC_RISCV_LO12_I;
> +              if (strncmp (asarg, "%base_idx_lo", 9) == 0)
> +                *imm_reloc = BFD_RELOC_RISCV_BASE_IDX_LO12_I;
> +              else
> +                *imm_reloc = BFD_RELOC_RISCV_LO12_I;
>                goto load_store;
>              case '1':
>                /* This is used for TLS relocations that acts as relaxation

Why are these changes needed? parse_relocation() recognizes and stores the
correct reloc type when %base_idx_lo() is found, doesn't it? The pre-filling
of the pointed-to variable here is to cover the case of no relocation, aiui.

> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/base-idx-add.l
> @@ -0,0 +1,10 @@
> +.*: Assembler messages:
> +.*: Error: illegal operands `amoadd.w x8,x9,%base_idx_add\(sym\)\(x10\)'
> +.*: Error: illegal operands `add a5,a5,a0,0'
> +.*: Error: illegal operands `sh1add a0,a0,a5,0'
> +.*: Error: illegal operands `sh1add.uw a0,a0,a5,0'
> +.*: Error: illegal operands `sh2add a0,a0,a5,0'
> +.*: Error: illegal operands `sh2add.uw a0,a0,a5,0'
> +.*: Error: illegal operands `sh3add a0,a0,a5,0'
> +.*: Error: illegal operands `sh3add.uw a0,a0,a5,0'
> +.*: Error: illegal operands `add.uw a0,a0,a5,0'
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/base-idx-add.s
> @@ -0,0 +1,19 @@
> +.option arch, +a
> +    # Don't allow base_idx_add in amoadd.
> +        amoadd.w x8,x9,%base_idx_add(sym)(x10)
> +        # Do require base_idx_add in 4-operand add.
> +        add         a5,a5,a0,0
> +        sh1add            a0,a0,a5,0
> +        sh1add.uw        a0,a0,a5,0
> +        sh2add           a0,a0,a5,0
> +        sh2add.uw        a0,a0,a5,0
> +        sh3add            a0,a0,a5,0
> +        sh3add.uw        a0,a0,a5,0
> +        add.uw      a0,a0,a5,0
> +        .globl        sym
> +        .section        .tbss,"awT",@nobits
> +        .align        2
> +        .type        sym, @object
> +        .size        sym, 4
> +sym:
> +        .zero        4

Please help readability here by (again) padding such that (first) operands
align with one another.

Also shouldn't there be another testcase actually testing correct behavior
with proper input?

> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1378,16 +1378,23 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"rorw",      64, INSN_CLASS_ZBB_OR_ZBKB,  "d,s,<", MATCH_RORIW, MASK_RORIW, match_opcode, INSN_ALIAS },
>  
>  /* Zba instructions.  */
> -{"sh1add",     0, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH1ADD, MASK_SH1ADD, match_opcode, 0 },
> -{"sh2add",     0, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH2ADD, MASK_SH2ADD, match_opcode, 0 },
> -{"sh3add",     0, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH3ADD, MASK_SH3ADD, match_opcode, 0 },
> -{"sh1add.uw", 64, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH1ADD_UW, MASK_SH1ADD_UW, match_opcode, 0 },
> -{"sh2add.uw", 64, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH2ADD_UW, MASK_SH2ADD_UW, match_opcode, 0 },
> -{"sh3add.uw", 64, INSN_CLASS_ZBA,  "d,s,t", MATCH_SH3ADD_UW, MASK_SH3ADD_UW, match_opcode, 0 },
> +{"sh1add",     0, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH1ADD, MASK_SH1ADD, match_opcode, 0 },
> +{"sh1add",     0, INSN_CLASS_ZBA,  "d,s,t,1", MATCH_SH1ADD, MASK_SH1ADD, match_opcode, 0 },
> +{"sh2add",     0, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH2ADD, MASK_SH2ADD, match_opcode, 0 },
> +{"sh2add",     0, INSN_CLASS_ZBA,  "d,s,t,1", MATCH_SH2ADD, MASK_SH2ADD, match_opcode, 0 },
> +{"sh3add",     0, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH3ADD, MASK_SH3ADD, match_opcode, 0 },
> +{"sh3add",     0, INSN_CLASS_ZBA,  "d,s,t,1", MATCH_SH3ADD, MASK_SH3ADD, match_opcode, 0 },
> +{"sh1add.uw", 64, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH1ADD_UW, MASK_SH1ADD_UW, match_opcode, 0 },
> +{"sh1add.uw", 64, INSN_CLASS_ZBA,  "d,s,t,1",   MATCH_SH1ADD_UW, MASK_SH1ADD_UW, match_opcode, 0 },
> +{"sh2add.uw", 64, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH2ADD_UW, MASK_SH2ADD_UW, match_opcode, 0 },
> +{"sh2add.uw", 64, INSN_CLASS_ZBA,  "d,s,t,1",   MATCH_SH2ADD_UW, MASK_SH2ADD_UW, match_opcode, 0 },
> +{"sh3add.uw", 64, INSN_CLASS_ZBA,  "d,s,t",   MATCH_SH3ADD_UW, MASK_SH3ADD_UW, match_opcode, 0 },
> +{"sh3add.uw", 64, INSN_CLASS_ZBA,  "d,s,t,1",   MATCH_SH3ADD_UW, MASK_SH3ADD_UW, match_opcode, 0 },

Same remark again regarding suitable padding ahead of MATCH_*. (Maybe this
becomes irrepevant though with the comment at the bottom.)

>  {"zext.w",    64, INSN_CLASS_ZCB_AND_ZBA,  "Cs,Cw", MATCH_C_ZEXT_W, MASK_C_ZEXT_W, match_opcode, INSN_ALIAS },
>  {"zext.w",    64, INSN_CLASS_ZBA,  "d,s",   MATCH_ADD_UW, MASK_ADD_UW | MASK_RS2, match_opcode, INSN_ALIAS },
>  {"zext.w",    64, INSN_CLASS_I, "d,s",       0, (int) M_ZEXTW, NULL, INSN_MACRO },
>  {"add.uw",    64, INSN_CLASS_ZBA,  "d,s,t", MATCH_ADD_UW, MASK_ADD_UW, match_opcode, 0 },
> +{"add.uw",    64, INSN_CLASS_ZBA,  "d,s,t,1", MATCH_ADD_UW, MASK_ADD_UW, match_opcode, 0 },
>  {"slli.uw",   64, INSN_CLASS_ZBA,  "d,s,>", MATCH_SLLI_UW, MASK_SLLI_UW, match_opcode, 0 },

The "...,1" forms of the insns aren't (at least for the time being) part of
the Zba spec. Their presence here wants commenting upon, so readers aren't
tempted to strip them out again as being against the spec.

I further wonder whether we really need two entries for each of these insns.
Can't the ",1" be made optional just like Vm is, and just like [1] arranges
to have the floating point rounding modes as well?

Jan

[1] https://sourceware.org/pipermail/binutils/2026-June/149695.html
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.