RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility with 2.41 objects
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Testing passed
|
Commit Message
* 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.
gas/
* NEWS: Updated.
ld/
* 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 new binutils 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.
---
bfd/elfnn-riscv.c | 44 +++++++++++++++-------
bfd/elfxx-riscv.h | 2 +
gas/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(-)
create mode 100644 ld/testsuite/ld-riscv-elf/pr31179-r.d
create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.d
create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.s
Comments
+Nick, as we're getting into release juggling mode here...
On Tue, 19 Dec 2023 22:50:03 PST (-0800), nelson@rivosinc.com wrote:
> * 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.
Nelson and I were just talking about this.
I don't have any issues with the actual code here, though I haven't
given it a super thorough look and with all the ABI considerations it's
a bit tricky. IMO we should be trying to avoid forced rebuilds where we
can, even if it causes pain and the fix is a bit of a heuristic. So I'd go
with something along these lines, rather than just saying "oops we had a
bug, everybody has to rebuild binaries for 2.42". IIUC that's a pretty
standard way to go for binutils.
I remember wanting to comment on some English issues in the
arguments/docs, but we're all pretty buried right now so it might take
me a bit to actually be able to say something coherent there. I don't
think that needs to block merging the patch, though -- maybe normally it
would, but with the holidays and the release it's probably best to get
things out to the distros for testing ASAP.
I do still think we want some way of marking new binaries as not having
the bug. At a bare minimum that'll let distro people scrub for what
should be rebuilt (which is important for this heuristic type of fix),
but it'll also let us avoid triggering the heuristic on new binaries
that don't suffer from the bug. I'm not entirely sure how to do that,
but Nelson said he'd look into it.
and that's where we get into the Nick questions: we're getting really
close to the release, so I don't want to hold things up now and cause
chaos in a few weeks. So I think if you want it merged soon that's fine
with me, but if you're OK holding off for a bit (with the holidays that
might be a bit longer than normal) then I'd like to try and get at least
a sketch of that binary tagging together. That way at least we know the
whole picture.
> 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.
> gas/
> * NEWS: Updated.
> ld/
> * 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 new binutils 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.
> ---
> bfd/elfnn-riscv.c | 44 +++++++++++++++-------
> bfd/elfxx-riscv.h | 2 +
> gas/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(-)
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179-r.d
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.d
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.s
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 042266e791b..6a1f1d9884f 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 new"
> + " binutils"), input_bfd);
> }
> else
> {
> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> index abcb409bd78..6959ec1b61f 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 with non-zero addend. */
> + bool check_uleb128;
> };
>
> extern void riscv_elf32_set_options (struct bfd_link_info *,
> diff --git a/gas/NEWS b/gas/NEWS
> index 4c8d5946690..8647d9699ad 100644
> --- a/gas/NEWS
> +++ b/gas/NEWS
> @@ -3,6 +3,11 @@
> * On RISC-V macro instructions expanding to AUIPC and a load, store, or branch
> no longer accept x0 as an intermediate and/or destination register.
>
> +* On RISC-V, add ld target option --[no-]check-uleb128. Should rebuild the
> + objects by new assembler 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 Reliability, Availability and Serviceability extension v2
> (RASv2) for AArch64.
>
> diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
> index bb6298d3e8d..7dbd264c0e8 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 with non-zero addend\n"));
> + fprintf (file, _(" --no-check-uleb128 Don'\''t check if SUB_ULEB128 with 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..366b32ea6ab
> --- /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 new binutils
> +
> +.*:[ ]+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
Committed that revert the linker behavior, passed regression of
riscv-gnu-toolchain.
Thanks
Nelson
On Fri, Dec 22, 2023 at 10:31 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> +Nick, as we're getting into release juggling mode here...
>
> On Tue, 19 Dec 2023 22:50:03 PST (-0800), nelson@rivosinc.com wrote:
> > * 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.
>
> Nelson and I were just talking about this.
>
> I don't have any issues with the actual code here, though I haven't
> given it a super thorough look and with all the ABI considerations it's
> a bit tricky. IMO we should be trying to avoid forced rebuilds where we
> can, even if it causes pain and the fix is a bit of a heuristic. So I'd
> go
> with something along these lines, rather than just saying "oops we had a
> bug, everybody has to rebuild binaries for 2.42". IIUC that's a pretty
> standard way to go for binutils.
>
> I remember wanting to comment on some English issues in the
> arguments/docs, but we're all pretty buried right now so it might take
> me a bit to actually be able to say something coherent there. I don't
> think that needs to block merging the patch, though -- maybe normally it
> would, but with the holidays and the release it's probably best to get
> things out to the distros for testing ASAP.
>
> I do still think we want some way of marking new binaries as not having
> the bug. At a bare minimum that'll let distro people scrub for what
> should be rebuilt (which is important for this heuristic type of fix),
> but it'll also let us avoid triggering the heuristic on new binaries
> that don't suffer from the bug. I'm not entirely sure how to do that,
> but Nelson said he'd look into it.
>
> and that's where we get into the Nick questions: we're getting really
> close to the release, so I don't want to hold things up now and cause
> chaos in a few weeks. So I think if you want it merged soon that's fine
> with me, but if you're OK holding off for a bit (with the holidays that
> might be a bit longer than normal) then I'd like to try and get at least
> a sketch of that binary tagging together. That way at least we know the
> whole picture.
>
> > 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.
> > gas/
> > * NEWS: Updated.
> > ld/
> > * 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 new binutils 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.
> > ---
> > bfd/elfnn-riscv.c | 44 +++++++++++++++-------
> > bfd/elfxx-riscv.h | 2 +
> > gas/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(-)
> > create mode 100644 ld/testsuite/ld-riscv-elf/pr31179-r.d
> > create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.d
> > create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.s
> >
> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > index 042266e791b..6a1f1d9884f 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
> new"
> > + " binutils"), input_bfd);
> > }
> > else
> > {
> > diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> > index abcb409bd78..6959ec1b61f 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 with non-zero addend.
> */
> > + bool check_uleb128;
> > };
> >
> > extern void riscv_elf32_set_options (struct bfd_link_info *,
> > diff --git a/gas/NEWS b/gas/NEWS
> > index 4c8d5946690..8647d9699ad 100644
> > --- a/gas/NEWS
> > +++ b/gas/NEWS
> > @@ -3,6 +3,11 @@
> > * On RISC-V macro instructions expanding to AUIPC and a load, store, or
> branch
> > no longer accept x0 as an intermediate and/or destination register.
> >
> > +* On RISC-V, add ld target option --[no-]check-uleb128. Should rebuild
> the
> > + objects by new assembler 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 Reliability, Availability and Serviceability
> extension v2
> > (RASv2) for AArch64.
> >
> > diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
> > index bb6298d3e8d..7dbd264c0e8 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
> with non-zero addend\n"));
> > + fprintf (file, _(" --no-check-uleb128 Don'\''t check if
> SUB_ULEB128 with 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..366b32ea6ab
> > --- /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
> new binutils
> > +
> > +.*:[ ]+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
>
@@ -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 new"
+ " binutils"), input_bfd);
}
else
{
@@ -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 with non-zero addend. */
+ bool check_uleb128;
};
extern void riscv_elf32_set_options (struct bfd_link_info *,
@@ -3,6 +3,11 @@
* On RISC-V macro instructions expanding to AUIPC and a load, store, or branch
no longer accept x0 as an intermediate and/or destination register.
+* On RISC-V, add ld target option --[no-]check-uleb128. Should rebuild the
+ objects by new assembler 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 Reliability, Availability and Serviceability extension v2
(RASv2) for AArch64.
@@ -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 with non-zero addend\n"));
+ fprintf (file, _(" --no-check-uleb128 Don'\''t check if SUB_ULEB128 with 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
@@ -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} \
new file mode 100644
@@ -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
new file mode 100644
@@ -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 new binutils
+
+.*:[ ]+file format .*
+
+Contents of section .text:
+
+[ ]+[0-9a-f]+[ ]+00000303[ ]+.*
new file mode 100644
@@ -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