Re: [PATCH] RISC-V: Add --[no-]relax-gp to ld

[Nelson Chu <nelson@rivosinc.com>]

Thanks!  All LGTM :-)

Nelson

On Mon, Feb 20, 2023 at 11:46 AM Fangrui Song <maskray@google.com> wrote:
>
> On 2023-02-20, Nelson Chu wrote:
> >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
>
> Thank you for the feedback!
>
> >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.
>
> I'm going to use:
>
> else if (!bfd_link_pic (info) && htab->params->relax_gp
>           && (type == R_RISCV_PCREL_HI20
>          ...
>
> >> > -                  && (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.
>
> I think the start value is arbitrary.  I stick with the ppc style which is
> well maintained by Alan :)
>
> >> > +
> >> > +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.
>
> Ack.  I did not know PARSE_AND_LIST_OPTIONS.
>
> 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"));
> '
>
> >> > +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, &params);
> >> >  }
> >> >
> >> >  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.