[PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard
Xiao Zeng
zengxiao@eswincomputing.com
Mon Nov 21 12:03:40 GMT 2022
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
>>
More information about the Binutils
mailing list