Re: [PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[Nelson Chu <nelson@rivosinc.com>]

On Fri, Nov 18, 2022 at 1:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Fri, 11 Nov 2022 07:32:49 PST (-0800), shihua@iscas.ac.cn wrote:
> > LGTM,and I think it would be better to have a test example.
> >
> >
> >
> >
> >
> >> From: zengxiao <zengxiao@eswincomputing.com>
> >>
> >> This patch makes R_RISCV_SUB6 conforms to riscv abi standard.
> >> R_RISCV_SUB6 only the lower 6 bits of the code are valid.
> >> The proposed specification which can be found in 8.5. Relocations of,
> >> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0-rc4/riscv-abi.pdf
> >>
> >> bfd/ChangeLog:
> >>
> >> * elfxx-riscv.c (riscv_elf_add_sub_reloc): Take the lower
> >> 6 bits as the significant bit
> >>
> >> reviewed-by: gaofei@eswincomputing.com
> >>              jinyanjiang@eswincomputing.com
>
> Is this trying to say that both of you reviewed it?
>
> >> Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
>
>
> >> ---
> >>  bfd/elfxx-riscv.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> >> index f0c91cc97f7..0fbfedd17fe 100644
> >> --- a/bfd/elfxx-riscv.c
> >> +++ b/bfd/elfxx-riscv.c
> >> @@ -994,6 +994,13 @@ riscv_elf_add_sub_reloc (bfd *abfd,
> >>        relocation = old_value + relocation;
> >>        break;
> >>      case R_RISCV_SUB6:
> >> +      {
> >> +        bfd_vma six_bit_valid_value = old_value & howto->dst_mask;
> >> +        six_bit_valid_value -= relocation;
> >> +        relocation = (six_bit_valid_value & howto->dst_mask) |
> >> +              (old_value & ~howto->dst_mask);
> >> +      }
> >> +      break;
>
> Unless I'm missing something here, this just just silently truncates the
> relocation to 6 bits because the range check still assumes an 8-bit
> relocation range.  I'm not sure if there's a way to massage the howto
> entry to make bfd_reloc_offset_in_range() understand this is a 6-bit
> relocation, if that's not viable then we should just check for the
> overflow here and return bfd_reloc_outofrange.

Yeah agreed, we shouldn't call bfd_reloc_offset_in_range for
R_RISCV_SUB6 since we are assuming it is an 8-bit relocation.  That
means if the R_RISCV_SUB6 is used to relocate the last 6-bit of the
section, then ld will always report bfd_reloc_outofrange since it
assumes at least 8-bit is needed, although the case seems minor.

As for the overflow of relocation values, we don't have any checks of
them, so I think we can just ignore them in the short-term.

Thanks
Nelson

> That also means there should be at least two test cases, on within range
> and one outside of it.
>
> >>      case R_RISCV_SUB8:
> >>      case R_RISCV_SUB16:
> >>      case R_RISCV_SUB32:
> >> --
> >> 2.34.1