From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 7803) id 96BE83858422; Thu, 28 Dec 2023 07:04:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 96BE83858422 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Nelson Chu To: bfd-cvs@sourceware.org Subject: [binutils-gdb] RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41 objects X-Act-Checkin: binutils-gdb X-Git-Author: Nelson Chu X-Git-Refname: refs/heads/master X-Git-Oldrev: 64e34e4134edb8a763ecfced808d2bb796796a15 X-Git-Newrev: 73d931e560059a87d76f528fafbb4270a98746bc Message-Id: <20231228070400.96BE83858422@sourceware.org> Date: Thu, 28 Dec 2023 07:04:00 +0000 (GMT) X-BeenThere: binutils-cvs@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Binutils-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Dec 2023 07:04:00 -0000 https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D73d931e56005= 9a87d76f528fafbb4270a98746bc commit 73d931e560059a87d76f528fafbb4270a98746bc Author: Nelson Chu Date: Wed Dec 20 10:37:41 2023 +0800 RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41= objects =20 * Problematic fix commit, 2029e13917d53d2289d3ebb390c4f40bd2112d21 RISC-V: Clarify the behaviors of SET/ADD/SUB relocations =20 * Bugzilla, https://sourceware.org/bugzilla/show_bug.cgi?id=3D31179#c5 =20 The addend of SUB_ULEB128 should be zero if using .uleb128, but we make= it non-zero by accident in assembler before. This causes troubles by appl= ying the above commit, since the calculation is changed to support .reloc *S= UB* relocations with non-zero addend. =20 We encourage people to rebuild their stuff to get the non-zero addend of SUB_ULEB128, but that might need some times, so report warnings to info= rm people need to rebuild their stuff if --check-uleb128 is enabled. =20 Since the failed .reloc cases for ADD/SET/SUB/ULEB128 are rarely to use, it may acceptable that stop supproting them until people rebuld their s= tuff, maybe half-year or a year later. Or maybe we should teach people that = don't write the .reloc R_RISCV_SUB* with non-zero constant, and then report warnings/errors in assembler. =20 bfd/ * elfnn-riscv.c (perform_relocation): Ignore the non-zero adden= d of R_RISCV_SUB_ULEB128. (riscv_elf_relocate_section): Report warnings to inform people = need to rebuild their stuff if --check-uleb128 is enabled. So that = can get the right non-zero addend of R_RISCV_SUB_ULEB128. * elfxx-riscv.h (struct riscv_elf_params): Added bool check_ule= b128. ld/ * NEWS: Updated. * emultempl/riscvelf.em: Added linker risc-v target options, --[no-]check-uleb128, to enable/disable checking if the addend = of uleb128 is non-zero or not. So that people will know they need= to rebuild the objects with binutils 2.42 and up, to get the right= zero addend of SUB_ULEB128 relocation, or they may get troubles if u= sing .reloc. * ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated. * ld/testsuite/ld-riscv-elf/pr31179*: New test cases. Diff: --- bfd/elfnn-riscv.c | 44 ++++++++++++++++++++------= ---- bfd/elfxx-riscv.h | 2 ++ ld/NEWS | 5 ++++ ld/emultempl/riscvelf.em | 17 +++++++++++- ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 2 ++ ld/testsuite/ld-riscv-elf/pr31179-r.d | 10 +++++++ ld/testsuite/ld-riscv-elf/pr31179.d | 11 ++++++++ ld/testsuite/ld-riscv-elf/pr31179.s | 13 +++++++++ 8 files changed, 89 insertions(+), 15 deletions(-) diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c index 042266e791b..509d61e5017 100644 --- a/bfd/elfnn-riscv.c +++ b/bfd/elfnn-riscv.c @@ -1735,19 +1735,9 @@ perform_relocation (const reloc_howto_type *howto, if (howto->pc_relative) value -=3D sec_addr (input_section) + rel->r_offset; =20 - switch (ELFNN_R_TYPE (rel->r_info)) - { - case R_RISCV_SUB6: - case R_RISCV_SUB8: - case R_RISCV_SUB16: - case R_RISCV_SUB32: - case R_RISCV_SUB64: - case R_RISCV_SUB_ULEB128: - value -=3D rel->r_addend; - break; - default: - value +=3D rel->r_addend; - } + /* PR31179, ignore the non-zero addend of R_RISCV_SUB_ULEB128. */ + if (ELFNN_R_TYPE (rel->r_info) !=3D R_RISCV_SUB_ULEB128) + value +=3D rel->r_addend; =20 switch (ELFNN_R_TYPE (rel->r_info)) { @@ -2530,9 +2520,35 @@ riscv_elf_relocate_section (bfd *output_bfd, if (uleb128_set_rel !=3D NULL && uleb128_set_rel->r_offset =3D=3D rel->r_offset) { - relocation =3D uleb128_set_vma - relocation + uleb128_set_rel->r_ad= dend; + relocation =3D uleb128_set_vma - relocation + + uleb128_set_rel->r_addend; uleb128_set_vma =3D 0; uleb128_set_rel =3D NULL; + + /* PR31179, the addend of SUB_ULEB128 should be zero if using + .uleb128, but we make it non-zero by accident in assembler, + so just ignore it in perform_relocation, and make assembler + continue doing the right thing. Don't reset the addend of + SUB_ULEB128 to zero here since it will break the --emit-reloc, + even though the non-zero addend is unexpected. + + We encourage people to rebuild their stuff to get the + non-zero addend of SUB_ULEB128, but that might need some + times, so report warnings to inform people need to rebuild + if --check-uleb128 is enabled. However, since the failed + .reloc cases for ADD/SET/SUB/ULEB128 are rarely to use, it + may acceptable that stop supproting them until people rebuld + their stuff, maybe half-year or one year later. I believe + this might be the least harmful option that we should go. + + Or maybe we should teach people that don't write the + .reloc R_RISCV_SUB* with non-zero constant, and report + warnings/errors in assembler. */ + if (htab->params->check_uleb128 + && rel->r_addend !=3D 0) + _bfd_error_handler (_("%pB: warning: R_RISCV_SUB_ULEB128 with" + " non-zero addend, please rebuild by" + " binutils 2.42 or up"), input_bfd); } else { diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h index abcb409bd78..6df2471952b 100644 --- a/bfd/elfxx-riscv.h +++ b/bfd/elfxx-riscv.h @@ -31,6 +31,8 @@ struct riscv_elf_params { /* Whether to relax code sequences to GP-relative addressing. */ bool relax_gp; + /* Whether to check if SUB_ULEB128 relocation has non-zero addend. */ + bool check_uleb128; }; =20 extern void riscv_elf32_set_options (struct bfd_link_info *, diff --git a/ld/NEWS b/ld/NEWS index 835dc39e24b..adc5c9ece78 100644 --- a/ld/NEWS +++ b/ld/NEWS @@ -1,5 +1,10 @@ -*- text -*- =20 +* On RISC-V, add ld target option --[no-]check-uleb128. Should rebuild the + objects by binutils 2.42 and up if enabling the option and get warnings, + since the non-zero addend of SUB_ULEB128 shouldn't be generated from .ul= eb128 + directives. + * Add support for the KVX instruction set. =20 * A new linker script sorting directive has been added: REVERSE. This rev= erses diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em index bb6298d3e8d..8aaed1f7673 100644 --- a/ld/emultempl/riscvelf.em +++ b/ld/emultempl/riscvelf.em @@ -25,7 +25,8 @@ fragment <