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

["Xiao Zeng" <zengxiao@eswincomputing.com>]

On Wed, Nov 16, 2022 at 12:00:00 AM  Nelson Chu <nelson@rivosinc.com> wrote:
>
>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. 

Yes, I am not familiar with how to construct the dwarf of R_RISCV_SET6/SUB6.
The dwarf-SUB6.s only constructs a failed test case, which does not reflect 
the R_RISCV_SET6/SUB6.
I would be grateful if anyone could provide such a construction use case. 
Or can there be no test case in this place temporarily?
Because the test case is invalid, I will delete it in v2 patch.

>
>> 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);
> 

fix

>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. 

fix

>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

The link given seems to be a Makefile for testing. I don't understand what you mean.
Can you give more detailed steps for testing?
How can I merge my patch into the code base and report the test results of the 
regression to the community?

>
>Thanks
>Nelson 

I will put my v2 patch in https://sourceware.org/pipermail/binutils/2022-November/124547.html. 
Please review the code on it later.

Thanks
Xiao
>
>>      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
>>