From: "Kewen.Lin" <linkw@linux.ibm.com>
To: richard.sandiford@arm.com
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Jakub Jelinek <jakub@redhat.com>,
Segher Boessenkool <segher@kernel.crashing.org>,
Peter Bergner <bergner@linux.ibm.com>,
jlaw@ventanamicro.com, AlanM <amodra@gmail.com>,
David Edelsohn <dje.gcc@gmail.com>,
Richard Biener <richard.guenther@gmail.com>,
"H.J. Lu" <hjl.tools@gmail.com>
Subject: Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
Date: Tue, 22 Nov 2022 10:58:04 +0800 [thread overview]
Message-ID: <e0fabd5e-2965-5a36-320d-6ec357f47032@linux.ibm.com> (raw)
In-Reply-To: <mptfsecbd9p.fsf@arm.com>
Hi Richard,
Many thanks for your review comments!
>>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote:
>>>> Hi,
>>>>
>>>> As discussed in PR98125, -fpatchable-function-entry with
>>>> SECTION_LINK_ORDER support doesn't work well on powerpc64
>>>> ELFv1 because the filled "Symbol" in
>>>>
>>>> .section name,"flags"o,@type,Symbol
>>>>
>>>> sits in .opd section instead of in the function_section
>>>> like .text or named .text*.
>>>>
>>>> Since we already generates one label LPFE* which sits in
>>>> function_section of current_function_decl, this patch is
>>>> to reuse it as the symbol for the linked_to section. It
>>>> avoids the above ABI specific issue when using the symbol
>>>> concluded from current_function_decl.
>>>>
>>>> Besides, with this support some previous workarounds for
>>>> powerpc64 ELFv1 can be reverted.
>>>>
>>>> btw, rs6000_print_patchable_function_entry can be dropped
>>>> but there is another rs6000 patch which needs this rs6000
>>>> specific hook rs6000_print_patchable_function_entry, not
>>>> sure which one gets landed first, so just leave it here.
>>>>
>>>> Bootstrapped and regtested on below:
>>>>
>>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27
>>>> and latest binutils 2.39.
>>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30).
>>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30).
>>>> 4) x86_64-redhat-linux with default binutils 2.30
>>>> and latest binutils 2.39.
>>>> 5) aarch64-linux-gnu with default binutils 2.30
>>>> and latest binutils 2.39.
>>>>
[snip...]
>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
>>>> index 4db8506b106..d4de6e164ee 100644
>>>> --- a/gcc/varasm.cc
>>>> +++ b/gcc/varasm.cc
>>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags,
>>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE);
>>>> if (flags & SECTION_LINK_ORDER)
>>>> {
>>>> - tree id = DECL_ASSEMBLER_NAME (decl);
>>>> - ultimate_transparent_alias_target (&id);
>>>> - const char *name = IDENTIFIER_POINTER (id);
>>>> - name = targetm.strip_name_encoding (name);
>>>> - fprintf (asm_out_file, ",%s", name);
>>>> + /* For now, only section "__patchable_function_entries"
>>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE*
>>>> + was emitted in default_print_patchable_function_entry,
>>>> + just place it here for linked_to section. */
>>>> + gcc_assert (!strcmp (name, "__patchable_function_entries"));
>
> I like the idea of removing the rs600 workaround in favour of making the
> target-independent more robust. But this seems a bit hackish. What
> would we do if SECTION_LINK_ORDER was used for something else in future?
>
Good question! I think it depends on how we can get the symbol for the
linked_to section, if adopting the name of the decl will suffer the
similar issue which this patch wants to fix, we have to reuse the label
LPFE* or some kind of new artificial label in the related section; or
we can just go with the name of the given decl, or something related to
that decl. Since we can't predict any future uses, I just placed an
assertion here to ensure that we would revisit and adjust this part at
that time. Does it sound reasonable to you?
BR,
Kewen
next prev parent reply other threads:[~2022-11-22 2:58 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-24 8:17 Kewen.Lin
2022-09-28 5:41 ` PING^1 " Kewen.Lin
2022-11-10 8:15 ` PING^2 " Kewen.Lin
2022-11-21 14:20 ` Richard Sandiford
2022-11-22 2:58 ` Kewen.Lin [this message]
2022-11-22 16:08 ` Richard Sandiford
2022-11-25 3:26 ` Kewen.Lin
2023-07-19 6:33 ` Fangrui Song
2023-07-19 8:49 ` Kewen.Lin
2022-09-29 20:31 ` Segher Boessenkool
2022-09-30 12:47 ` Kewen.Lin
2022-09-30 17:47 ` Segher Boessenkool
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e0fabd5e-2965-5a36-320d-6ec357f47032@linux.ibm.com \
--to=linkw@linux.ibm.com \
--cc=amodra@gmail.com \
--cc=bergner@linux.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
--cc=jakub@redhat.com \
--cc=jlaw@ventanamicro.com \
--cc=richard.guenther@gmail.com \
--cc=richard.sandiford@arm.com \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).