Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
Hi Fangrui,
on 2023/7/19 14:33, Fangrui Song wrote:
> On Thu, Nov 24, 2022 at 7:26 PM Kewen.Lin via Gcc-patches
> wrote:
>>
>> Hi Richard,
>>
>> on 2022/11/23 00:08, Richard Sandiford wrote:
>>> "Kewen.Lin" writes:
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?
>>>
>>> Yeah, I guess that's good enough. If the old scheme ends up being
>>> correct for some future use, we can make the new behaviour conditional
>>> on __patchable_function_entries.
>>
>> Yes, we can check if the given section name is
>> "__patchable_function_entries".
>>
>>>
>>> So yeah, the patch LGTM to me, thanks.
>>
>> Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le},
>> just committed in r13-4294-gf120196382ac5a.
>>
>> BR,
>> Kewen
>
> Hi, Kewen, do you think whether your patch fixed
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110729
> (__patchable_function_entries has wrong sh_link) ?
I just had a check and confirmed that it did fix the wrong
sh_link, in the past it always uses the decl saved in
named.decl (always f for the test case in PR110729),
with this patch, it switches to use the label in its
corresponding .text* (function section).
> If yes, it may be useful to include some assembly tests... Right now
>
> rg '\.section.*__patchable' gcc/testsuite/
>
> returns nothing.
It's a good idea to add some testing coverage, I'm going
to make a test ca
Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
On Thu, Nov 24, 2022 at 7:26 PM Kewen.Lin via Gcc-patches
wrote:
>
> Hi Richard,
>
> on 2022/11/23 00:08, Richard Sandiford wrote:
> > "Kewen.Lin" writes:
> >> 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?
> >
> > Yeah, I guess that's good enough. If the old scheme ends up being
> > correct for some future use, we can make the new behaviour conditional
> > on __patchable_function_entries.
>
> Yes, we can check if the given section name is
> "__patchable_function_entries".
>
> >
> > So yeah, the patch LGTM to me, thanks.
>
> Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le},
> just committed in r13-4294-gf120196382ac5a.
>
> BR,
> Kewen
Hi, Kewen, do you think whether your patch fixed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110729
(__patchable_function_entries has wrong sh_link) ?
If yes, it may be useful to include some assembly tests... Right now
rg '\.section.*__patchable' gcc/testsuite/
returns nothing.
--
宋方睿
Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
Hi Richard,
on 2022/11/23 00:08, Richard Sandiford wrote:
> "Kewen.Lin" writes:
>> 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?
>
> Yeah, I guess that's good enough. If the old scheme ends up being
> correct for some future use, we can make the new behaviour conditional
> on __patchable_function_entries.
Yes, we can check if the given section name is
"__patchable_function_entries".
>
> So yeah, the patch LGTM to me, thanks.
Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le},
just committed in r13-4294-gf120196382ac5a.
BR,
Kewen
Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
"Kewen.Lin" writes:
> 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?
Yeah, I guess that's good enough. If the old scheme ends up being
correct for some future use, we can make the new behaviour conditional
on __patchable_function_entries.
So yeah, the patch LGTM to me, thanks.
Richard
Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
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
Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
"Kewen.Lin" writes:
> Hi,
>
> Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html
>
> Any comments are highly appreciated.
>
> BR,
> Kewen
>
> on 2022/9/28 13:41, Kewen.Lin via Gcc-patches wrote:
>> Hi,
>>
>> Gentle ping:
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html
>>
>> BR,
>> Kewen
>>
>> 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.
>>>
>>> Is it ok for trunk?
>>>
>>> BR,
>>> Kewen
>>> -
>>>
>>> PR target/99889
>>>
>>> gcc/ChangeLog:
>>>
>>> * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry):
>>> Adjust to call function default_print_patchable_function_entry.
>>> * targhooks.cc (default_print_patchable_function_entry_1): Remove and
>>> move the flags preparation ...
>>> (default_print_patchable_function_entry): ... here, adjust to use
>>> current_function_funcdef_no for label no.
>>> * targhooks.h (default_print_patchable_function_entry_1): Remove.
>>> * varasm.cc (default_elf_asm_named_section): Adjust code for
>>> __patchable_function_entries section support with LPFE label.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit.
>>> * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0.
>>> * gcc.target/aarch64/pr92424-3.c: Likewise.
>>> * gcc.target/i386/pr93492-2.c: Likewise.
>>> * gcc.target/i386/pr93492-3.c: Likewise.
>>> * gcc.target/i386/pr93492-4.c: Likewise.
>>> * gcc.target/i386/pr93492-5.c: Likewise.
>>> ---
>>> gcc/config/rs6000/rs6000.cc | 13 +-
>>> gcc/varasm.cc| 15 ---
>>> gcc/targhooks.cc | 45 +++-
>>> gcc/targhooks.h | 3 --
>>> gcc/testsuite/g++.dg/pr93195a.C | 1 -
>>> gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +-
>>> gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +-
>>> gcc/testsuite/gcc.target/i386/pr93492-2.c| 4 +-
>>> gcc/testsuite/gcc.target/i386/pr93492-3.c| 4 +-
>>> gcc/testsuite/gcc.target/i386/pr93492-4.c| 4 +-
>>> gcc/testsuite/gcc.target/i386/pr93492-5.c| 4 +-
>>> 11 files changed, 40 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>>> index df491bee2ea..dba28b8e647 100644
>>> --- a/gcc/config/rs6000/rs6000.cc
>>> +++ b/gcc/config/rs6000/rs6000.cc
>>> @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file,
>>>unsigned HOST_WIDE_INT patch_area_size,
>>>bool record_p)
>>> {
>>> - unsigned int flags = SECTION_WRITE | SECTION_RELRO;
>>> - /* When .opd section is emitted, the function symbol
>>> - default_print_patchable_function_entry_1 is emitted into the .opd
>>> section
>>> - while the patchable area is emitted into the function section.
>>> - Don't use SECTION_LINK_ORDER in that case. */
>>> - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2)
>>> - && HAVE_GAS_SECTION_LINK_ORDER)
>>> -flags |= SECTION_LINK_ORDER;
>>> - default_print_patchable_function_entry_1 (file, patch_area_size,
>>> record_p,
>>> - flags);
>>> + default_print_patchable_function_entry (file, patch_area_size, record_p);
>>> }
>>> -
>>>
>>> +
>>> enum rtx_code
>>> rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
>>> {
>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
>>> index 4db8506b106..d4de6e164ee 100644
>>> --- a
PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
Hi,
Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html
Any comments are highly appreciated.
BR,
Kewen
on 2022/9/28 13:41, Kewen.Lin via Gcc-patches wrote:
> Hi,
>
> Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html
>
> BR,
> Kewen
>
> 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.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -
>>
>> PR target/99889
>>
>> gcc/ChangeLog:
>>
>> * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry):
>> Adjust to call function default_print_patchable_function_entry.
>> * targhooks.cc (default_print_patchable_function_entry_1): Remove and
>> move the flags preparation ...
>> (default_print_patchable_function_entry): ... here, adjust to use
>> current_function_funcdef_no for label no.
>> * targhooks.h (default_print_patchable_function_entry_1): Remove.
>> * varasm.cc (default_elf_asm_named_section): Adjust code for
>> __patchable_function_entries section support with LPFE label.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit.
>> * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0.
>> * gcc.target/aarch64/pr92424-3.c: Likewise.
>> * gcc.target/i386/pr93492-2.c: Likewise.
>> * gcc.target/i386/pr93492-3.c: Likewise.
>> * gcc.target/i386/pr93492-4.c: Likewise.
>> * gcc.target/i386/pr93492-5.c: Likewise.
>> ---
>> gcc/config/rs6000/rs6000.cc | 13 +-
>> gcc/varasm.cc| 15 ---
>> gcc/targhooks.cc | 45 +++-
>> gcc/targhooks.h | 3 --
>> gcc/testsuite/g++.dg/pr93195a.C | 1 -
>> gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +-
>> gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +-
>> gcc/testsuite/gcc.target/i386/pr93492-2.c| 4 +-
>> gcc/testsuite/gcc.target/i386/pr93492-3.c| 4 +-
>> gcc/testsuite/gcc.target/i386/pr93492-4.c| 4 +-
>> gcc/testsuite/gcc.target/i386/pr93492-5.c| 4 +-
>> 11 files changed, 40 insertions(+), 61 deletions(-)
>>
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index df491bee2ea..dba28b8e647 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file,
>> unsigned HOST_WIDE_INT patch_area_size,
>> bool record_p)
>> {
>> - unsigned int flags = SECTION_WRITE | SECTION_RELRO;
>> - /* When .opd section is emitted, the function symbol
>> - default_print_patchable_function_entry_1 is emitted into the .opd
>> section
>> - while the patchable area is emitted into the function section.
>> - Don't use SECTION_LINK_ORDER in that case. */
>> - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2)
>> - && HAVE_GAS_SECTION_LINK_ORDER)
>> -flags |= SECTION_LINK_ORDER;
>> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p,
>> -flags);
>> + default_print_patchable_function_entry (file, patch_area_size, record_p);
>> }
>> -
>>
>> +
>> enum rtx_code
>> rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
>> {
>> 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,
>>
