"Kewen.Lin" <li...@linux.ibm.com> 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/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?
Thanks,
Richard
>>> + fprintf (asm_out_file, ",");
>>> + char buf[256];
>>> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE",
>>> + current_function_funcdef_no);
>>> + assemble_name_raw (asm_out_file, buf);
>>> }
>>> if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
>>> {
>>> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
>>> index b15ae19bcb6..e80caecc418 100644
>>> --- a/gcc/targhooks.cc
>>> +++ b/gcc/targhooks.cc
>>> @@ -1979,15 +1979,17 @@ default_compare_by_pieces_branch_ratio
>>> (machine_mode)
>>> return 1;
>>> }
>>>
>>> -/* Helper for default_print_patchable_function_entry and other
>>> - print_patchable_function_entry hook implementations. */
>>> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
>>> + entry. If RECORD_P is true and the target supports named sections,
>>> + the location of the NOPs will be recorded in a special object section
>>> + called "__patchable_function_entries". This routine may be called
>>> + twice per function to put NOPs before and after the function
>>> + entry. */
>>>
>>> void
>>> -default_print_patchable_function_entry_1 (FILE *file,
>>> - unsigned HOST_WIDE_INT
>>> - patch_area_size,
>>> - bool record_p,
>>> - unsigned int flags)
>>> +default_print_patchable_function_entry (FILE *file,
>>> + unsigned HOST_WIDE_INT patch_area_size,
>>> + bool record_p)
>>> {
>>> const char *nop_templ = 0;
>>> int code_num;
>>> @@ -2001,13 +2003,17 @@ default_print_patchable_function_entry_1 (FILE
>>> *file,
>>> if (record_p && targetm_common.have_named_sections)
>>> {
>>> char buf[256];
>>> - static int patch_area_number;
>>> section *previous_section = in_section;
>>> const char *asm_op = integer_asm_op (POINTER_SIZE_UNITS, false);
>>>
>>> gcc_assert (asm_op != NULL);
>>> - patch_area_number++;
>>> - ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
>>> + /* If SECTION_LINK_ORDER is supported, this internal label will
>>> + be filled as the symbol for linked_to section. */
>>> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE",
>>> current_function_funcdef_no);
>>> +
>>> + unsigned int flags = SECTION_WRITE | SECTION_RELRO;
>>> + if (HAVE_GAS_SECTION_LINK_ORDER)
>>> + flags |= SECTION_LINK_ORDER;
>>>
>>> section *sect = get_section ("__patchable_function_entries",
>>> flags, current_function_decl);
>>> @@ -2029,25 +2035,6 @@ default_print_patchable_function_entry_1 (FILE *file,
>>> output_asm_insn (nop_templ, NULL);
>>> }
>>>
>>> -/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function
>>> - entry. If RECORD_P is true and the target supports named sections,
>>> - the location of the NOPs will be recorded in a special object section
>>> - called "__patchable_function_entries". This routine may be called
>>> - twice per function to put NOPs before and after the function
>>> - entry. */
>>> -
>>> -void
>>> -default_print_patchable_function_entry (FILE *file,
>>> - unsigned HOST_WIDE_INT patch_area_size,
>>> - bool record_p)
>>> -{
>>> - unsigned int flags = SECTION_WRITE | SECTION_RELRO;
>>> - if (HAVE_GAS_SECTION_LINK_ORDER)
>>> - flags |= SECTION_LINK_ORDER;
>>> - default_print_patchable_function_entry_1 (file, patch_area_size,
>>> record_p,
>>> - flags);
>>> -}
>>> -
>>> bool
>>> default_profile_before_prologue (void)
>>> {
>>> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
>>> index ecce55ebe79..5c1216fad0b 100644
>>> --- a/gcc/targhooks.h
>>> +++ b/gcc/targhooks.h
>>> @@ -229,9 +229,6 @@ extern bool default_use_by_pieces_infrastructure_p
>>> (unsigned HOST_WIDE_INT,
>>> bool);
>>> extern int default_compare_by_pieces_branch_ratio (machine_mode);
>>>
>>> -extern void default_print_patchable_function_entry_1 (FILE *,
>>> - unsigned HOST_WIDE_INT,
>>> - bool, unsigned int);
>>> extern void default_print_patchable_function_entry (FILE *,
>>> unsigned HOST_WIDE_INT,
>>> bool);
>>> diff --git a/gcc/testsuite/g++.dg/pr93195a.C
>>> b/gcc/testsuite/g++.dg/pr93195a.C
>>> index b14f1b3e341..26d265da74e 100644
>>> --- a/gcc/testsuite/g++.dg/pr93195a.C
>>> +++ b/gcc/testsuite/g++.dg/pr93195a.C
>>> @@ -1,5 +1,4 @@
>>> /* { dg-do link { target { ! { nvptx*-*-* visium-*-* } } } } */
>>> -/* { dg-skip-if "not supported" { { powerpc*-*-* } && lp64 } } */
>>> // { dg-require-effective-target o_flag_in_section }
>>> /* { dg-options "-O0 -fpatchable-function-entry=1" } */
>>> /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
>>> b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
>>> index 0e75657a153..12465213aef 100644
>>> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
>>> @@ -1,7 +1,7 @@
>>> /* { dg-do "compile" } */
>>> /* { dg-options "-O1" } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> __attribute__ ((target("branch-protection=bti"),
>>> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"),
>>> f10_bti ()
>>> {
>>> }
>>> -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti
>>> c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */
>>> +/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti
>>> c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
>>> b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
>>> index 0a1f74d4096..2c6a73789f0 100644
>>> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
>>> @@ -1,7 +1,7 @@
>>> /* { dg-do "compile" } */
>>> /* { dg-options "-O1" } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> __attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
>>> @@ -9,4 +9,4 @@ __attribute__
>>> ((target("branch-protection=bti+pac-ret+leaf"),
>>> f10_pac ()
>>> {
>>> }
>>> -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti
>>> c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 //
>>> autiasp\n.*\tret\n" } } */
>>> +/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti
>>> c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 //
>>> autiasp\n.*\tret\n" } } */
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c
>>> b/gcc/testsuite/gcc.target/i386/pr93492-2.c
>>> index 3d67095fd10..ede8c2077b7 100644
>>> --- a/gcc/testsuite/gcc.target/i386/pr93492-2.c
>>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c
>>> @@ -1,7 +1,7 @@
>>> /* { dg-do "compile" { target *-*-linux* } } */
>>> /* { dg-options "-O1 -fcf-protection -mmanual-endbr
>>> -fasynchronous-unwind-tables" } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> __attribute__ ((cf_check,patchable_function_entry (1, 0)))
>>> @@ -9,4 +9,4 @@ f10_endbr (void)
>>> {
>>> }
>>>
>>> -/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */
>>> +/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n\tret\n" } } */
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c
>>> b/gcc/testsuite/gcc.target/i386/pr93492-3.c
>>> index a625c927f4f..b68da30bd36 100644
>>> --- a/gcc/testsuite/gcc.target/i386/pr93492-3.c
>>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c
>>> @@ -2,7 +2,7 @@
>>> /* { dg-require-effective-target mfentry } */
>>> /* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg
>>> -fasynchronous-unwind-tables" } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> __attribute__ ((cf_check,patchable_function_entry (1, 0)))
>>> @@ -10,4 +10,4 @@ f10_endbr (void)
>>> {
>>> }
>>>
>>> -/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n"
>>> } } */
>>> +/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n"
>>> } } */
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c
>>> b/gcc/testsuite/gcc.target/i386/pr93492-4.c
>>> index 8f205c345b8..c73034a4624 100644
>>> --- a/gcc/testsuite/gcc.target/i386/pr93492-4.c
>>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c
>>> @@ -1,11 +1,11 @@
>>> /* { dg-do "compile" { target *-*-linux* } } */
>>> /* { dg-options "-O1 -fpatchable-function-entry=1
>>> -fasynchronous-unwind-tables" } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> foo (void)
>>> {
>>> }
>>>
>>> -/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */
>>> +/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c
>>> b/gcc/testsuite/gcc.target/i386/pr93492-5.c
>>> index 1ca5ba1fac1..ee9849ae852 100644
>>> --- a/gcc/testsuite/gcc.target/i386/pr93492-5.c
>>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c
>>> @@ -2,11 +2,11 @@
>>> /* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg
>>> -fasynchronous-unwind-tables" } */
>>> /* { dg-additional-options "-fno-PIE" { target ia32 } } */
>>>
>>> -/* Test the placement of the .LPFE1 label. */
>>> +/* Test the placement of the .LPFE0 label. */
>>>
>>> void
>>> foo (void)
>>> {
>>> }
>>>
>>> -/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n"
>>> } } */
>>> +/* { dg-final { scan-assembler
>>> "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n"
>>> } } */
>>> --
>>
