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

[Nelson Chu <nelson@rivosinc.com>]

On Tue, Nov 15, 2022 at 4:15 PM <zengxiao@eswincomputing.com> wrote:
>
> 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):
>
> binutils/ChangeLog:
>
>         * testsuite/binutils-all/riscv/dwarf-SUB6.d: New test.
>         * testsuite/binutils-all/riscv/dwarf-SUB6.s: New test.

I tried but it seems like the testcases won't generate any
R_RISCV_SET6/SUB6, so it is useless in fact.  Maybe reducing your test
case here is the right thing to do,
https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, I can
reproduce the problem when testing the link.

> reviewed-by: gaofei@eswincomputing.com
>              jinyanjiang@eswincomputing.com
>
> Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
> ---
>  bfd/elfxx-riscv.c                             |  7 +++++
>  .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++
>  .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++
>  3 files changed, 50 insertions(+)
>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 300ccf49534..e71d4a456f2 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;

Seems like it will cause problems if we are dumping the debug
information of the object by the objdump, before the final link.
There are two issues here, one is the overflow checking, and another
is to have the same calculations for both riscv_elf_add_sub_reloc and
riscv_elf_relocate_section.

For the former, I just talked to Palmer today, and talked to Kito a
few months ago, we all agree that we should have overflow checks for
these kinds of relocations, including ADD/SUB/SET, even if we don't
have any of them for now.  I don't remember the details when I tried
to add the overflow checks before, but that caused the debug
information broken for the rv64 toolchains when running the gcc
regressions.  Since sometimes we still generate the ADD32/SUB32
relocations in rv64, but that seems dangerous though.  Anyway, I'm not
the expert for that, so I don't have any useful thoughts for now.

For the latter, I think llvm did the things right,
https://llvm.org/doxygen/RelocationResolver_8cpp_source.html#l00469.
So,

1. relocation = (old_value & ~howto->dst_mask)
                         | (((old_value & howto->dst_mask) - relocation)
                            & howto->dst_mask);

2. We should have the same behavior in riscv_elf_relocate_section -
filter to get the valid least 6-bit address for the old value,
https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L2429.
Btw, we don't need to do anything else when encoding, since
perform_relocation already did the right thing.

It would be great if we have the testcase, but the testcase in this
patch doesn't seems useful, so we need a reduced case from here,
https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, to show
something like "DW_ CFA_??? (User defined call frame op: 0x3c)" before
the fix.  Or if it is too difficult to reduce, then it's okay for me
to ignore the testcase.  Just remember to pass the riscv-gnu-toolchain
regressions, that should be enough to prove we won't break something
basically.

Please see the regressions here,
https://github.com/riscv-collab/riscv-gnu-toolchain/tree/master/regression

Thanks
Nelson

>      case R_RISCV_SUB8:
>      case R_RISCV_SUB16:
>      case R_RISCV_SUB32:
> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
> new file mode 100644
> index 00000000000..47d5ae570d7
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
> @@ -0,0 +1,31 @@
> +#PROG: objcopy
> +#objdump: --dwarf=frames
> +
> +tmpdir/riscvcopy.o:     file format elf32-littleriscv
> +
> +Contents of the .eh_frame section:
> +
> +
> +00000000 00000020 00000000 CIE
> +  Version:               3
> +  Augmentation:          "zR"
> +  Code alignment factor: 1
> +  Data alignment factor: -4
> +  Return address column: 1
> +  Augmentation data:     1b
> +  DW_CFA_def_cfa_register: r2 \(sp\)
> +  DW_CFA_def_cfa_offset: 48
> +  DW_CFA_offset: r1 \(ra\) at cfa-4
> +  DW_CFA_offset: r8 \(s0\) at cfa-8
> +  DW_CFA_def_cfa: r8 \(s0\) ofs 0
> +  DW_CFA_restore: r1 \(ra\)
> +  DW_CFA_restore: r8 \(s0\)
> +  DW_CFA_def_cfa: r2 \(sp\) ofs 48
> +  DW_CFA_def_cfa_offset: 0
> +  DW_CFA_nop
> +
> +00000024 00000010 00000028 FDE cie=00000000 pc=0000002c..0000002c
> +  DW_CFA_nop
> +  DW_CFA_nop
> +  DW_CFA_nop
> +
> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
> new file mode 100644
> index 00000000000..fe959f59d9b
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
> @@ -0,0 +1,12 @@
> +        .attribute arch, "rv32i2p0_m2p0_a2p0_f2p0_c2p0"
> +        .cfi_startproc
> +        .cfi_def_cfa_offset 48
> +        .cfi_offset 1, -4
> +        .cfi_offset 8, -8
> +        .cfi_def_cfa 8, 0
> +        .cfi_restore 1
> +        .cfi_restore 8
> +        .cfi_def_cfa 2, 48
> +        .cfi_def_cfa_offset 0
> +        .cfi_endproc
> +
> \ No newline at end of file
> --
> 2.34.1
>