[PATCH] RISC-V: Add --[no-]relax-gp to ld
Nelson Chu
nelson@rivosinc.com
Mon Feb 20 01:55:49 GMT 2023
This is just a target option, and it's not only for compatibility with
lld, sometimes this option is also convenient to catch whether the gp
relaxation has a bug without rebuilding the linker to disable the
specific relaxation. Personally I would say this is a good idea at
least for debugging, we just default enable the relax flags in the
riscv_elf_params, so that everything won't change. Perhaps from the
debug point of view, it may be the right direction to have other
developer target options to close the remaining specific relaxations.
Anyway, this target option works for me, it's not only good for
developer debugging, but also keeps it compatible with lld. However,
it would be better to just wait so as not to miss some of the other
suggestions, I think 1-2 weeks should be enough.
Thanks
Nelson
On Sat, Feb 18, 2023 at 3:31 PM Fangrui Song <i@maskray.me> wrote:
>
> On Wed, Jul 13, 2022 at 1:26 AM Fangrui Song <maskray@google.com> wrote:
> >
> > From: Fangrui Song <i@maskray.me>
> >
> > --relax enables all relaxations. --no-relax-gp disables GP relaxation to
> > allow measuring its effect.
> >
> > Link: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/298
> >
> > bfd/
> > * elfnn-riscv.c (struct riscv_elf_link_hash_table): Add params.
> > (riscv_elfNN_set_options): New.
> > (riscv_info_to_howto_rela): Check relax_gp.
> > (_bfd_riscv_relax_section): Likewise.
> > * elfxx-riscv.h (struct riscv_elf_params): New.
> > (riscv_elf32_set_options): New.
> > (riscv_elf64_set_options): New.
> > ld/
> > * emultempl/riscvelf.em: Add option parsing.
> > * testsuite/ld-riscv-elf/code-model-relax-medlow-01-norelaxgp.d: New.
> > * testsuite/ld-riscv-elf/pcgp-relax-01-norelaxgp.d: New.
> > * testsuite/ld-riscv-elf/pcgp-relax-02.d: Test --relax --relax-gp can be
> > used together.
> > ---
> > bfd/elfnn-riscv.c | 22 ++++++++++---
> > bfd/elfxx-riscv.h | 11 +++++++
> > ld/emultempl/riscvelf.em | 31 +++++++++++++++++++
> > .../code-model-relax-medlow-01-norelaxgp.d | 4 +++
> > ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 2 ++
> > .../ld-riscv-elf/pcgp-relax-01-norelaxgp.d | 18 +++++++++++
> > ld/testsuite/ld-riscv-elf/pcgp-relax-02.d | 2 +-
> > 7 files changed, 84 insertions(+), 6 deletions(-)
> > create mode 100644 ld/testsuite/ld-riscv-elf/code-model-relax-medlow-01-norelaxgp.d
> > create mode 100644 ld/testsuite/ld-riscv-elf/pcgp-relax-01-norelaxgp.d
> >
> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > index 8f9f0d8a86a..527c3e74e97 100644
> > --- a/bfd/elfnn-riscv.c
> > +++ b/bfd/elfnn-riscv.c
> > @@ -118,6 +118,9 @@ struct riscv_elf_link_hash_table
> > {
> > struct elf_link_hash_table elf;
> >
> > + /* Various options and other info passed from the linker. */
> > + struct riscv_elf_params *params;
> > +
> > /* Short-cuts to get to dynamic linker sections. */
> > asection *sdyntdata;
> >
> > @@ -157,6 +160,13 @@ struct riscv_elf_link_hash_table
> > && elf_hash_table_id (elf_hash_table (p)) == RISCV_ELF_DATA) \
> > ? (struct riscv_elf_link_hash_table *) (p)->hash : NULL)
> >
> > +void
> > +riscv_elfNN_set_options (struct bfd_link_info *link_info,
> > + struct riscv_elf_params *params)
> > +{
> > + riscv_elf_hash_table (link_info)->params = params;
> > +}
> > +
> > static bool
> > riscv_info_to_howto_rela (bfd *abfd,
> > arelent *cache_ptr,
> > @@ -4289,7 +4299,9 @@ _bfd_riscv_relax_lui (bfd *abfd,
> > bool undefined_weak)
> > {
> > bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
> > - bfd_vma gp = riscv_global_pointer_value (link_info);
> > + bfd_vma gp = riscv_elf_hash_table (link_info)->params->relax_gp
> > + ? riscv_global_pointer_value (link_info)
> > + : 0;
> > int use_rvc = elf_elfheader (abfd)->e_flags & EF_RISCV_RVC;
> >
> > BFD_ASSERT (rel->r_offset + 4 <= sec->size);
> > @@ -4754,10 +4766,10 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> > || type == R_RISCV_TPREL_LO12_I
> > || type == R_RISCV_TPREL_LO12_S)
> > relax_func = _bfd_riscv_relax_tls_le;
> > - else if (!bfd_link_pic (info)
Probably just add "riscv_elf_hash_table (info)->params->relax_gp"
here, to make the coding style same as above.
> > - && (type == R_RISCV_PCREL_HI20
> > - || type == R_RISCV_PCREL_LO12_I
> > - || type == R_RISCV_PCREL_LO12_S))
> > + else if ((type == R_RISCV_PCREL_HI20 || type == R_RISCV_PCREL_LO12_I
> > + || type == R_RISCV_PCREL_LO12_S)
> > + && !bfd_link_pic (info)
> > + && riscv_elf_hash_table (info)->params->relax_gp)
> > relax_func = _bfd_riscv_relax_pc;
> > else
> > continue;
> > diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> > index ea7126bdb4d..f9bd0ad882c 100644
> > --- a/bfd/elfxx-riscv.h
> > +++ b/bfd/elfxx-riscv.h
> > @@ -27,6 +27,17 @@
> >
> > #define RISCV_UNKNOWN_VERSION -1
> >
> > +struct riscv_elf_params
> > +{
> > + /* Whether to relax code sequences to GP-relative addressing. */
> > + bool relax_gp;
> > +};
> > +
> > +extern void riscv_elf32_set_options (struct bfd_link_info *,
> > + struct riscv_elf_params *);
> > +extern void riscv_elf64_set_options (struct bfd_link_info *,
> > + struct riscv_elf_params *);
> > +
> > extern reloc_howto_type *
> > riscv_reloc_name_lookup (bfd *, const char *);
> >
> > diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
> > index 645a807f239..37a0fd08e96 100644
> > --- a/ld/emultempl/riscvelf.em
> > +++ b/ld/emultempl/riscvelf.em
> > @@ -25,6 +25,35 @@ fragment <<EOF
> > #include "elf/riscv.h"
> > #include "elfxx-riscv.h"
> >
> > +static struct riscv_elf_params params = { .relax_gp = 1 };
> > +EOF
> > +
> > +# Define some shell vars to insert bits of code into the standard elf
> > +# parse_args and list_options functions. */
> > +PARSE_AND_LIST_PROLOGUE=${PARSE_AND_LIST_PROLOGUE}'
> > +enum risccv_opt
> > +{
> > + OPTION_RELAX_GP = 321,
> > + OPTION_NO_RELAX_GP,
> > +};
> > +'
Not really sure if the value of base enum is flexible or has some
meaning. I see ppc starts with 321, mips and nds32 start with 301,
and aarch64 starts with 309. If the start value doesn't mean
anything, then it's fine here.
> > +
> > +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 },
> > +'
> > +
It's good to also define PARSE_AND_LIST_OPTIONS, so that --target-help
should work for the new options.
> > +PARSE_AND_LIST_ARGS_CASES=${PARSE_AND_LIST_ARGS_CASES}'
> > + case OPTION_RELAX_GP:
> > + params.relax_gp = 1;
> > + break;
> > +
> > + case OPTION_NO_RELAX_GP:
> > + params.relax_gp = 0;
> > + break;
> > +'
> > +
> > +fragment <<EOF
> > static void
> > riscv_elf_before_allocation (void)
> > {
> > @@ -96,6 +125,8 @@ riscv_create_output_section_statements (void)
> > " whilst linking %s binaries\n"), "RISC-V");
> > return;
> > }
> > +
> > + riscv_elf${ELFSIZE}_set_options (&link_info, ¶ms);
> > }
> >
> > EOF
> > diff --git a/ld/testsuite/ld-riscv-elf/code-model-relax-medlow-01-norelaxgp.d b/ld/testsuite/ld-riscv-elf/code-model-relax-medlow-01-norelaxgp.d
> > new file mode 100644
> > index 00000000000..8e40cc5f32d
> > --- /dev/null
> > +++ b/ld/testsuite/ld-riscv-elf/code-model-relax-medlow-01-norelaxgp.d
> > @@ -0,0 +1,4 @@
> > +#source: code-model.s
> > +#as: -march=rv64i -mabi=lp64 --defsym __medlow__=1
> > +#ld: -Tcode-model-01.ld -melf64lriscv --no-relax-gp --relax
> > +#error: .*relocation truncated to fit: R_RISCV_HI20 against `symbolL'
> > diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > index 272424b33e3..1711d12ba23 100644
> > --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > @@ -122,6 +122,7 @@ if [istarget "riscv*-*-*"] {
> > run_dump_test "align-small-region"
> > run_dump_test "call-relax"
> > run_dump_test "pcgp-relax-01"
> > + run_dump_test "pcgp-relax-01-norelaxgp"
> > run_dump_test "pcgp-relax-02"
> > run_dump_test "c-lui"
> > run_dump_test "c-lui-2"
> > @@ -141,6 +142,7 @@ if [istarget "riscv*-*-*"] {
> > run_dump_test "code-model-medany-weakref-01"
> > run_dump_test "code-model-medany-weakref-02"
> > run_dump_test "code-model-relax-medlow-01"
> > + run_dump_test "code-model-relax-medlow-01-norelaxgp"
> > run_dump_test "code-model-relax-medlow-02"
> > run_dump_test "code-model-relax-medlow-weakref-01"
> > run_dump_test "code-model-relax-medlow-weakref-02"
> > diff --git a/ld/testsuite/ld-riscv-elf/pcgp-relax-01-norelaxgp.d b/ld/testsuite/ld-riscv-elf/pcgp-relax-01-norelaxgp.d
> > new file mode 100644
> > index 00000000000..d1344576ff3
> > --- /dev/null
> > +++ b/ld/testsuite/ld-riscv-elf/pcgp-relax-01-norelaxgp.d
> > @@ -0,0 +1,18 @@
> > +#source: pcgp-relax-01.s
> > +#ld: --no-relax-gp --relax
> > +#objdump: -d -Mno-aliases
> > +
> > +.*:[ ]+file format .*
> > +
> > +
> > +Disassembly of section \.text:
> > +
> > +0+[0-9a-f]+ <_start>:
> > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a0,a0,[0-9]+
> > +.*:[ ]+[0-9a-f]+[ ]+jal[ ]+ra,[0-9a-f]+ <_start>
> > +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+a1,0x[0-9a-f]+
> > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a1,a1,[0-9]+ # [0-9a-f]+ <data_g>
> > +.*:[ ]+[0-9a-f]+[ ]+lui[ ]+a2,0x[0-9a-f]+
> > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a2,a2,[0-9]+ # [0-9a-f]+ <data_g>
> > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a3,tp,0 # 0 <data_t>
> > +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+a0,0x[0-9a-f]+
> > diff --git a/ld/testsuite/ld-riscv-elf/pcgp-relax-02.d b/ld/testsuite/ld-riscv-elf/pcgp-relax-02.d
> > index c6c73c54265..984d457aed9 100644
> > --- a/ld/testsuite/ld-riscv-elf/pcgp-relax-02.d
> > +++ b/ld/testsuite/ld-riscv-elf/pcgp-relax-02.d
> > @@ -1,6 +1,6 @@
> > #source: pcgp-relax-02.s
> > #as:
> > -#ld: --relax
> > +#ld: --relax --relax-gp
> > #objdump: -d
> >
> > .*:[ ]+file format .*
> > --
> > 2.37.0.144.g8ac04bfd2-goog
> >
>
> Ping. I maintain lld/ELF and we are considering a new opt-in option
> --relax-gp to perform GP relaxation.
> It will be opt-in since many users don't find the relaxation useful
> and some feel GP relaxation will conflict with using GP for other
> purposes (some ABI variants).
> I wish that the two linkers agree on the same options.
More information about the Binutils
mailing list