From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2b.google.com (mail-oa1-x2b.google.com [IPv6:2001:4860:4864:20::2b]) by sourceware.org (Postfix) with ESMTPS id EB9743836CC8 for ; Mon, 20 Feb 2023 10:30:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EB9743836CC8 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-oa1-x2b.google.com with SMTP id 586e51a60fabf-172129e9cf1so307047fac.8 for ; Mon, 20 Feb 2023 02:30:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=B6Na/9v2lgbhR72ksG3dcsRNRT4YWXIQLQHebXIFnnk=; b=Cwe0g29NDUP1ebYLXbYIwn9mPwmRxCyISp+gc6RBNiOZ+hFGAtHwJb1oEWAXxYsHGa Inm78xaQJgIaI9PdbqyTn15WkxJLUCj+QmhKW0BfbvFdFmZ9JJyWBI5Vn/wxvKGNkpdV LkGjZHO4eFBIlliwUVn1ZBp+EcrtBayxa5j0/TZW0N7WNNytSYv/8C7Tp8KbaIlckV+i vk6IEavwFsOBMORN4+UPuOYLi7g39HGZvjefmmZZlLbunJmi4A5gh5O9h9fYAeCAZAud 8Xlo3kSVFafDtg9gXgduPTysehiHW0lWK61gfw6mjXXEzfeUTCTAf2NuC1kcqw+PWq1I c0fQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=B6Na/9v2lgbhR72ksG3dcsRNRT4YWXIQLQHebXIFnnk=; b=YjNt6Gv8AbDXiK9OnYUo213Ne2iBTPVAhFqgXMSF8lOBqIGowXxYe9Hi8+2lzc0E64 iaVE+iqoOwxwvSmb8hQnPXq6Y6dEYM92M1Gj3hgix4rrdRh+GIJd3TSBc2jxyX63+KRt ykbXeldNRzYekfmF98iyKa8czKZmgpduA6BNIX/BJIfdSAel7rsErpQnHFYe+BWm/MUi 1vgN9+HanAUbdd/MJApiA6KFuW2kiyYlKPJWJm3K4eWHSzdhRUoDsppelADr+WSLPqHB rPt1oYBsF2bNP+/jQ/DQUgbt6KviV20/N3j6tDha1IJgnKNCgUxVD5RNtXMuzsr6x7F+ UbIg== X-Gm-Message-State: AO0yUKU7sxZFUKuTsYh/5fKtGlEbON6IqLW7L+sBmicasFezURwJ0GJn o5xpR8orHi3oP/pDyisc+oLW4vA+0UU/mucvjwMsfc5o6edbn0XKB+U= X-Google-Smtp-Source: AK7set8CYbhoNht1D3rU+6UmrX5HgiTTmnIP1pxpbWtVnb4GWOcBFojAVm61ytsanUFO1OmKsoPkK6hGA/f4uaxHVhI= X-Received: by 2002:a05:6808:1a23:b0:378:9f53:b321 with SMTP id bk35-20020a0568081a2300b003789f53b321mr283683oib.170.1676858160837; Sun, 19 Feb 2023 17:56:00 -0800 (PST) MIME-Version: 1.0 References: <20220713082608.1695497-1-maskray@google.com> In-Reply-To: From: Nelson Chu Date: Mon, 20 Feb 2023 09:55:49 +0800 Message-ID: Subject: Re: [PATCH] RISC-V: Add --[no-]relax-gp to ld To: Fangrui Song Cc: Fangrui Song , binutils@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,DATE_IN_PAST_06_12,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 wrote: > > On Wed, Jul 13, 2022 at 1:26 AM Fangrui Song wrote: > > > > From: Fangrui Song > > > > --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 < > #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 < > 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]+ > > +.*:[ ]+[0-9a-f]+[ ]+lui[ ]+a2,0x[0-9a-f]+ > > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a2,a2,[0-9]+ # [0-9a-f]+ > > +.*:[ ]+[0-9a-f]+[ ]+addi[ ]+a3,tp,0 # 0 > > +.*:[ ]+[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.