* [PATCH v1 0/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard @ 2022-11-15 8:14 zengxiao 2022-11-15 8:14 ` [PATCH v1 1/1] " zengxiao 0 siblings, 1 reply; 4+ messages in thread From: zengxiao @ 2022-11-15 8:14 UTC (permalink / raw) To: binutils; +Cc: kito.cheng, andrew, shihua, zengxiao From: zengxiao <zengxiao@eswincomputing.com> Hi all RISC-V folks: When riscv-objdump is used to generate dwarf information, problems are found, like: DW_CFA_??? (User defined call frame op: 0x3c) This error is related to that riscv-objdump does not follow the riscv R_RISCV_SUB6 standard. Riscv-readelf is correct because it follows the R_RISCV_SUB6 standard. There are test cases in https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769 that describe the error in detail. zengxiao (1): RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 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 -- 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-15 8:14 [PATCH v1 0/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard zengxiao @ 2022-11-15 8:14 ` zengxiao 2022-11-16 8:58 ` Nelson Chu 0 siblings, 1 reply; 4+ messages in thread From: zengxiao @ 2022-11-15 8:14 UTC (permalink / raw) To: binutils; +Cc: kito.cheng, andrew, shihua, zengxiao, gaofei 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 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; 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-15 8:14 ` [PATCH v1 1/1] " zengxiao @ 2022-11-16 8:58 ` Nelson Chu 2022-11-21 12:03 ` Xiao Zeng 0 siblings, 1 reply; 4+ messages in thread From: Nelson Chu @ 2022-11-16 8:58 UTC (permalink / raw) To: zengxiao; +Cc: binutils, kito.cheng, andrew, shihua, gaofei 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-16 8:58 ` Nelson Chu @ 2022-11-21 12:03 ` Xiao Zeng 0 siblings, 0 replies; 4+ messages in thread From: Xiao Zeng @ 2022-11-21 12:03 UTC (permalink / raw) To: Nelson Chu; +Cc: binutils, kito.cheng, shihua, gaofei, palmer 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 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-21 12:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-15 8:14 [PATCH v1 0/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard zengxiao 2022-11-15 8:14 ` [PATCH v1 1/1] " zengxiao 2022-11-16 8:58 ` Nelson Chu 2022-11-21 12:03 ` Xiao Zeng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).