diff mbox series

Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]

Message ID 0558633c-b553-5ef1-aa6f-c76fcf297454@linux.ibm.com
State New, archived
Headers
Series Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] |

Commit Message

Kewen.Lin Aug. 24, 2022, 8:17 a.m. UTC 
  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(-)

--

Comments

Kewen.Lin Sept. 28, 2022, 5:41 a.m. UTC | #1 
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"));
> +	  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" } } */
> --
Segher Boessenkool Sept. 29, 2022, 8:31 p.m. UTC | #2 
Hi!

On Wed, Aug 24, 2022 at 04:17:07PM +0800, Kewen.Lin wrote:
> --- 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);
>  }

Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all,
instead, and remove this whole function?

The rs6000 changes are okay like that, thanks!


Segher
Kewen.Lin Sept. 30, 2022, 12:47 p.m. UTC | #3 
Hi Segher,

on 2022/9/30 04:31, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Aug 24, 2022 at 04:17:07PM +0800, Kewen.Lin wrote:
>> --- 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);
>>  }
> 
> Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all,
> instead, and remove this whole function?

This hook is still needed for "ELFv2 support rework" which
was just committed in r13-2984.  There is also a note
explaining this in the original mail: 

"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."

> 
> The rs6000 changes are okay like that, thanks!

Thanks!

BR,
Kewen
Segher Boessenkool Sept. 30, 2022, 5:47 p.m. UTC | #4 
On Fri, Sep 30, 2022 at 08:47:53PM +0800, Kewen.Lin wrote:
> on 2022/9/30 04:31, Segher Boessenkool wrote:
> > Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all,
> > instead, and remove this whole function?
> 
> This hook is still needed for "ELFv2 support rework" which
> was just committed in r13-2984.  There is also a note
> explaining this in the original mail: 
> 
> "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."

Ah, so this would be just churn?  Okay then :-)

Thanks,


Segher
Kewen.Lin Nov. 10, 2022, 8:15 a.m. UTC | #5 
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"));
>> +	  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" } } */
>> --
>
Richard Sandiford Nov. 21, 2022, 2:20 p.m. UTC | #6 
"Kewen.Lin" <linkw@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" } } */
>>> --
>>
Kewen.Lin Nov. 22, 2022, 2:58 a.m. UTC | #7 
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
Richard Sandiford Nov. 22, 2022, 4:08 p.m. UTC | #8 
"Kewen.Lin" <linkw@linux.ibm.com> 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
Kewen.Lin Nov. 25, 2022, 3:26 a.m. UTC | #9 
Hi Richard,

on 2022/11/23 00:08, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> 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
Fangrui Song July 19, 2023, 6:33 a.m. UTC | #10 
On Thu, Nov 24, 2022 at 7:26 PM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Richard,
>
> on 2022/11/23 00:08, Richard Sandiford wrote:
> > "Kewen.Lin" <linkw@linux.ibm.com> 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.
Kewen.Lin July 19, 2023, 8:49 a.m. UTC | #11 
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
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi Richard,
>>
>> on 2022/11/23 00:08, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@linux.ibm.com> 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 case by checking the given
".section.*__patchable_function_entries.*,\.LPFE[012]".

Thanks for the suggestion!

BR,
Kewen
diff mbox series

Patch

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"));
+	  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" } } */