[v1,1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard
Checks
Commit Message
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.
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
Comments
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
>
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
>>
@@ -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;
case R_RISCV_SUB8:
case R_RISCV_SUB16:
case R_RISCV_SUB32:
new file mode 100644
@@ -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
+
new file mode 100644
@@ -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