* [binutils-gdb] RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41 objects
@ 2023-12-28 7:04 Nelson Chu
0 siblings, 0 replies; only message in thread
From: Nelson Chu @ 2023-12-28 7:04 UTC (permalink / raw)
To: bfd-cvs
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=73d931e560059a87d76f528fafbb4270a98746bc
commit 73d931e560059a87d76f528fafbb4270a98746bc
Author: Nelson Chu <nelson@rivosinc.com>
Date: Wed Dec 20 10:37:41 2023 +0800
RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41 objects
* Problematic fix commit,
2029e13917d53d2289d3ebb390c4f40bd2112d21
RISC-V: Clarify the behaviors of SET/ADD/SUB relocations
* Bugzilla,
https://sourceware.org/bugzilla/show_bug.cgi?id=31179#c5
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 applying
the above commit, since the calculation is changed to support .reloc *SUB*
relocations with non-zero addend.
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 their stuff if --check-uleb128 is enabled.
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 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.
bfd/
* elfnn-riscv.c (perform_relocation): Ignore the non-zero addend 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_uleb128.
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 using
.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 -= sec_addr (input_section) + rel->r_offset;
- 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 -= rel->r_addend;
- break;
- default:
- value += rel->r_addend;
- }
+ /* PR31179, ignore the non-zero addend of R_RISCV_SUB_ULEB128. */
+ if (ELFNN_R_TYPE (rel->r_info) != R_RISCV_SUB_ULEB128)
+ value += rel->r_addend;
switch (ELFNN_R_TYPE (rel->r_info))
{
@@ -2530,9 +2520,35 @@ riscv_elf_relocate_section (bfd *output_bfd,
if (uleb128_set_rel != NULL
&& uleb128_set_rel->r_offset == rel->r_offset)
{
- relocation = uleb128_set_vma - relocation + uleb128_set_rel->r_addend;
+ relocation = uleb128_set_vma - relocation
+ + uleb128_set_rel->r_addend;
uleb128_set_vma = 0;
uleb128_set_rel = 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 != 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;
};
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 -*-
+* 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 .uleb128
+ directives.
+
* Add support for the KVX instruction set.
* A new linker script sorting directive has been added: REVERSE. This reverses
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 <<EOF
#include "elf/riscv.h"
#include "elfxx-riscv.h"
-static struct riscv_elf_params params = { .relax_gp = 1 };
+static struct riscv_elf_params params = { .relax_gp = 1,
+ .check_uleb128 = 0};
EOF
# Define some shell vars to insert bits of code into the standard elf
@@ -35,17 +36,23 @@ enum risccv_opt
{
OPTION_RELAX_GP = 321,
OPTION_NO_RELAX_GP,
+ OPTION_CHECK_ULEB128,
+ OPTION_NO_CHECK_ULEB128,
};
'
PARSE_AND_LIST_LONGOPTS=${PARSE_AND_LIST_LONGOPTS}'
{ "relax-gp", no_argument, NULL, OPTION_RELAX_GP },
{ "no-relax-gp", no_argument, NULL, OPTION_NO_RELAX_GP },
+ { "check-uleb128", no_argument, NULL, OPTION_CHECK_ULEB128 },
+ { "no-check-uleb128", no_argument, NULL, OPTION_NO_CHECK_ULEB128 },
'
PARSE_AND_LIST_OPTIONS=${PARSE_AND_LIST_OPTIONS}'
fprintf (file, _(" --relax-gp Perform GP relaxation\n"));
fprintf (file, _(" --no-relax-gp Don'\''t perform GP relaxation\n"));
+ fprintf (file, _(" --check-uleb128 Check if SUB_ULEB128 has non-zero addend\n"));
+ fprintf (file, _(" --no-check-uleb128 Don'\''t check if SUB_ULEB128 has non-zero addend\n"));
'
PARSE_AND_LIST_ARGS_CASES=${PARSE_AND_LIST_ARGS_CASES}'
@@ -56,6 +63,14 @@ PARSE_AND_LIST_ARGS_CASES=${PARSE_AND_LIST_ARGS_CASES}'
case OPTION_NO_RELAX_GP:
params.relax_gp = 0;
break;
+
+ case OPTION_CHECK_ULEB128:
+ params.check_uleb128 = 1;
+ break;
+
+ case OPTION_NO_CHECK_ULEB128:
+ params.check_uleb128 = 0;
+ break;
'
fragment <<EOF
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index 947a266ba72..1d793da51e5 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -173,6 +173,8 @@ if [istarget "riscv*-*-*"] {
run_dump_test "attr-phdr"
run_dump_test "relax-max-align-gp"
run_dump_test "uleb128"
+ run_dump_test "pr31179"
+ run_dump_test "pr31179-r"
run_ld_link_tests [list \
[list "Weak reference 32" "-T weakref.ld -m[riscv_choose_ilp32_emul]" "" \
"-march=rv32i -mabi=ilp32" {weakref32.s} \
diff --git a/ld/testsuite/ld-riscv-elf/pr31179-r.d b/ld/testsuite/ld-riscv-elf/pr31179-r.d
new file mode 100644
index 00000000000..cd5c98e62f7
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/pr31179-r.d
@@ -0,0 +1,10 @@
+#source: pr31179.s
+#as:
+#readelf: -Wr
+
+Relocation section '.rela.text' at .*
+[ ]+Offset[ ]+Info[ ]+Type[ ]+.*
+[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_SET_ULEB128[ ]+[0-9a-f]+[ ]+bar \+ 1
+[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_SUB_ULEB128[ ]+[0-9a-f]+[ ]+foo \+ 0
+[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_SET_ULEB128[ ]+[0-9a-f]+[ ]+bar \+ 1
+[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_SUB_ULEB128[ ]+[0-9a-f]+[ ]+foo \+ 1
diff --git a/ld/testsuite/ld-riscv-elf/pr31179.d b/ld/testsuite/ld-riscv-elf/pr31179.d
new file mode 100644
index 00000000000..a3228db8a09
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/pr31179.d
@@ -0,0 +1,11 @@
+#source: pr31179.s
+#as:
+#ld: --check-uleb128
+#objdump: -sj .text
+#warning: .*R_RISCV_SUB_ULEB128 with non-zero addend, please rebuild by binutils 2.42 or up
+
+.*:[ ]+file format .*
+
+Contents of section .text:
+
+[ ]+[0-9a-f]+[ ]+00000303[ ]+.*
diff --git a/ld/testsuite/ld-riscv-elf/pr31179.s b/ld/testsuite/ld-riscv-elf/pr31179.s
new file mode 100644
index 00000000000..5c4b4b54230
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/pr31179.s
@@ -0,0 +1,13 @@
+.globl _start
+_start:
+
+foo:
+.2byte 0
+bar:
+
+.uleb128 bar - foo + 1
+
+reloc:
+.reloc reloc, R_RISCV_SET_ULEB128, bar + 1
+.reloc reloc, R_RISCV_SUB_ULEB128, foo + 1
+.byte 0x0
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2023-12-28 7:04 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-28 7:04 [binutils-gdb] RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41 objects Nelson Chu
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).