PR98125, PowerPC64 -fpatchable-function-entry

PR98125, PowerPC64 -fpatchable-function-entry

[Alan Modra]

This series of patches fixes -fpatchable-function-entry on PowerPC64
ELFv1 so that SECTION_LINK_ORDER (.section 'o' arg) is now supported,
and on PowerPC64 ELFv2 to not break the global entry code.

Bootstrapped powerpc64le-linux and x86_64-linux all langs.  I did see
one regression on both targets, libgo runtime/pprof.  It's unclear to
me what that means.

Alan Modra (3):
  PowerPC64 ELFv1 -fpatchable-function-entry
  Revert "rs6000: Avoid -fpatchable-function-entry* regressions on
    powerpc64 be [PR98125]"
  PowerPC64 ELFv2 -fpatchable-function-entry

 gcc/config/rs6000/rs6000-logue.c |  5 ++++
 gcc/config/rs6000/rs6000.c       | 47 +++++++++++++++++---------------
 gcc/targhooks.c                  | 38 ++++++++------------------
 gcc/targhooks.h                  |  3 --
 gcc/testsuite/g++.dg/pr93195a.C  |  1 -
 gcc/varasm.c                     | 37 ++++++++++++++++++-------
 6 files changed, 69 insertions(+), 62 deletions(-)

PowerPC64 ELFv1 -fpatchable-function-entry

[Alan Modra]

On PowerPC64 ELFv1 function symbols are defined on function
descriptors in an .opd section rather than in the function code.
.opd is not split up by the PowerPC64 backend for comdat groups or
other situations where per-function sections are required.  Thus
SECTION_LINK_ORDER can't use the function name to reference a suitable
section for ordering:  The .opd section might contain many other
function descriptors and they may be in a different order to the final
function code placement.  This patch arranges to use a code label
instead of the function name symbol.

I chose to emit the label inside default_elf_asm_named_section,
immediately before the .section directive using the label, and in case
someone uses .previous or the like, need to save and restore the
current section when switching to the function code section to emit
the label.  That requires a tweak to switch_to_section in order to get
the current section.  I checked all the TARGET_ASM_NAMED_SECTION
functions and unnamed.callback functions and it appears none will be
affected by that tweak.

	PR target/98125
	* varasm.c (default_elf_asm_named_section): Use a function
	code label rather than the function symbol as the "o" argument.
	(switch_to_section): Don't set in_section until section
	directive has been emitted.

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 97c1e6fff25..5f95f8cfa75 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6866,6 +6866,26 @@ default_elf_asm_named_section (const char *name, unsigned int flags,
       *f = '\0';
     }
 
+  char func_label[256];
+  if (flags & SECTION_LINK_ORDER)
+    {
+      static int recur;
+      if (recur)
+	gcc_unreachable ();
+      else
+	{
+	  ++recur;
+	  section *save_section = in_section;
+	  static int func_code_labelno;
+	  switch_to_section (function_section (decl));
+	  ++func_code_labelno;
+	  ASM_GENERATE_INTERNAL_LABEL (func_label, "LPFC", func_code_labelno);
+	  ASM_OUTPUT_LABEL (asm_out_file, func_label);
+	  switch_to_section (save_section);
+	  --recur;
+	}
+    }
+
   fprintf (asm_out_file, "\t.section\t%s,\"%s\"", name, flagchars);
 
   /* default_section_type_flags (above) knows which flags need special
@@ -6893,11 +6913,8 @@ 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);
+	  fputc (',', asm_out_file);
+	  assemble_name_raw (asm_out_file, func_label);
 	}
       if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
 	{
@@ -7821,11 +7838,6 @@ switch_to_section (section *new_section, tree decl)
   else if (in_section == new_section)
     return;
 
-  if (new_section->common.flags & SECTION_FORGET)
-    in_section = NULL;
-  else
-    in_section = new_section;
-
   switch (SECTION_STYLE (new_section))
     {
     case SECTION_NAMED:
@@ -7843,6 +7855,11 @@ switch_to_section (section *new_section, tree decl)
       break;
     }
 
+  if (new_section->common.flags & SECTION_FORGET)
+    in_section = NULL;
+  else
+    in_section = new_section;
+
   new_section->common.flags |= SECTION_DECLARED;
 }
 

Re: PowerPC64 ELFv1 -fpatchable-function-entry

[will schmidt]

On Fri, 2021-05-07 at 12:19 +0930, Alan Modra via Gcc-patches wrote:
> On PowerPC64 ELFv1 function symbols are defined on function
> descriptors in an .opd section rather than in the function code.
> .opd is not split up by the PowerPC64 backend for comdat groups or
> other situations where per-function sections are required.  Thus
> SECTION_LINK_ORDER can't use the function name to reference a
> suitable
> section for ordering:  The .opd section might contain many other
> function descriptors and they may be in a different order to the
> final
> function code placement.  This patch arranges to use a code label
> instead of the function name symbol.
> 
> I chose to emit the label inside default_elf_asm_named_section,
> immediately before the .section directive using the label, and in
> case
> someone uses .previous or the like, need to save and restore the
> current section when switching to the function code section to emit
> the label.  That requires a tweak to switch_to_section in order to
> get
> the current section.  I checked all the TARGET_ASM_NAMED_SECTION
> functions and unnamed.callback functions and it appears none will be
> affected by that tweak.


Hi,

good description.  thanks :-)


> 
> 	PR target/98125
> 	* varasm.c (default_elf_asm_named_section): Use a function
> 	code label rather than the function symbol as the "o" argument.
> 	(switch_to_section): Don't set in_section until section
> 	directive has been emitted.
> 
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 97c1e6fff25..5f95f8cfa75 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -6866,6 +6866,26 @@ default_elf_asm_named_section (const char
> *name, unsigned int flags,
>        *f = '\0';
>      }
> 
> +  char func_label[256];
> +  if (flags & SECTION_LINK_ORDER)
> +    {
> +      static int recur;
> +      if (recur)
> +	gcc_unreachable ();

Interesting..   Is there any anticipation of re-entry or parallel runs
through this function that requires the recur lock/protection?


> +      else
> +	{
> +	  ++recur;
> +	  section *save_section = in_section;
> +	  static int func_code_labelno;
> +	  switch_to_section (function_section (decl));
> +	  ++func_code_labelno;
> +	  ASM_GENERATE_INTERNAL_LABEL (func_label, "LPFC",
> func_code_labelno);
> +	  ASM_OUTPUT_LABEL (asm_out_file, func_label);
> +	  switch_to_section (save_section);
> +	  --recur;
> +	}
> +    }


ok

> +
>    fprintf (asm_out_file, "\t.section\t%s,\"%s\"", name, flagchars);
> 
>    /* default_section_type_flags (above) knows which flags need
> special
> @@ -6893,11 +6913,8 @@ 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);
> +	  fputc (',', asm_out_file);
> +	  assemble_name_raw (asm_out_file, func_label);


ok as far as I can tell :-)    assemble_name_raw is an if/else that
outputs 'name' or a LABELREF based on the file & name.  It's not an
obvious analog to the untimate_transparent_alias_target() and name
processing that is being replaced, but seems to fit the changes as
described.


>  	}
>        if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
>  	{
> @@ -7821,11 +7838,6 @@ switch_to_section (section *new_section, tree
> decl)
>    else if (in_section == new_section)
>      return;
> 
> -  if (new_section->common.flags & SECTION_FORGET)
> -    in_section = NULL;
> -  else
> -    in_section = new_section;
> -
>    switch (SECTION_STYLE (new_section))
>      {
>      case SECTION_NAMED:
> @@ -7843,6 +7855,11 @@ switch_to_section (section *new_section, tree
> decl)
>        break;
>      }
> 
> +  if (new_section->common.flags & SECTION_FORGET)
> +    in_section = NULL;
> +  else
> +    in_section = new_section;
> +
>    new_section->common.flags |= SECTION_DECLARED;


OK. 
lgtm, thx
-Will

>  }
> 

Re: PowerPC64 ELFv1 -fpatchable-function-entry

[Segher Boessenkool]

On Fri, May 07, 2021 at 08:47:02AM -0500, will schmidt wrote:
> On Fri, 2021-05-07 at 12:19 +0930, Alan Modra via Gcc-patches wrote:
> > --- a/gcc/varasm.c
> > +++ b/gcc/varasm.c
> > @@ -6866,6 +6866,26 @@ default_elf_asm_named_section (const char
> > *name, unsigned int flags,
> >        *f = '\0';
> >      }
> > 
> > +  char func_label[256];
> > +  if (flags & SECTION_LINK_ORDER)
> > +    {
> > +      static int recur;
> > +      if (recur)
> > +	gcc_unreachable ();
> 
> Interesting..   Is there any anticipation of re-entry or parallel runs
> through this function that requires the recur lock/protection?

Not parallel runs :-)  But:

> > +      else
> > +	{
> > +	  ++recur;
> > +	  section *save_section = in_section;
> > +	  static int func_code_labelno;
> > +	  switch_to_section (function_section (decl));

This could in theory call us again.  That should not be a problem, if

> > +	  ++func_code_labelno;

(Please use postfix increments btw)

...this is done *before* the call, so that we get two different labels.


Segher

Re: PowerPC64 ELFv1 -fpatchable-function-entry

[Segher Boessenkool]

Hi!

On Fri, May 07, 2021 at 12:19:50PM +0930, Alan Modra wrote:
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -6866,6 +6866,26 @@ default_elf_asm_named_section (const char *name, unsigned int flags,
>        *f = '\0';
>      }
>  
> +  char func_label[256];
> +  if (flags & SECTION_LINK_ORDER)
> +    {
> +      static int recur;
> +      if (recur)
> +	gcc_unreachable ();

That is written
  gcc_assert (!recur);
and no "else" after it please.

> +	{
> +	  ++recur;
> +	  section *save_section = in_section;
> +	  static int func_code_labelno;
> +	  switch_to_section (function_section (decl));
> +	  ++func_code_labelno;
> +	  ASM_GENERATE_INTERNAL_LABEL (func_label, "LPFC", func_code_labelno);
> +	  ASM_OUTPUT_LABEL (asm_out_file, func_label);
> +	  switch_to_section (save_section);
> +	  --recur;
> +	}

See the other mail.  You could just write
  recur = true;
etc.?  That avoids unintentionally overflowing as well.

>  	{
> -	  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);
> +	  fputc (',', asm_out_file);

Please don't use fputc and friends, just use fprintf.  The compiler will
make that fputc if that is a good idea.

Looks good with those things tweaked.


Segher

Re: PowerPC64 ELFv1 -fpatchable-function-entry

[Alan Modra]

On Fri, May 07, 2021 at 07:24:02PM -0500, Segher Boessenkool wrote:
> Looks good with those things tweaked.

Except that the patch chose the wrong section to emit the label,
because the decl is wrong.  But of course I was using the same decl as
used in existing SHF_LINK_ORDER support, so it was already broken.
You can see this on x86_64 gcc/testsuite/g++.dg/pr93195a.C output for
bar().

	.globl	_Z3barv
	.type	_Z3barv, @function
_Z3barv:
.LFB1:
	.cfi_startproc
	.section	__patchable_function_entries,"awo",@progbits,_Z3foov
	.align 8
	.quad	.LPFE2
	.text
.LPFE2:
	nop
	pushq	%rbp

Note the reference to _Z3foov!  Tracking down why this happens isn't
hard.  The first get_section ("__patchable_function_entries", ...)
saves the decl for foo, which is the one then used on all subsequent
calls.  Oops.  Jakub, commit 134afa38f0be didn't quite do the right
thing.

This problem can be fixed with a simplification of the patch I posted,
relying on the fact that currently in the only place that wants to
emit SHF_LINK_ORDER sections we've already switched to the correct
function code section.

Regression testing in progress.

	PR target/98125
	* varasm.c (default_elf_asm_named_section): Use a function
	code label rather than the function symbol as the "o" argument.

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 97c1e6fff25..6f10ac7762e 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6866,6 +6866,15 @@ default_elf_asm_named_section (const char *name, unsigned int flags,
       *f = '\0';
     }
 
+  char func_label[256];
+  if (flags & SECTION_LINK_ORDER)
+    {
+      static int func_code_labelno;
+      func_code_labelno++;
+      ASM_GENERATE_INTERNAL_LABEL (func_label, "LPFC", func_code_labelno);
+      ASM_OUTPUT_LABEL (asm_out_file, func_label);
+    }
+
   fprintf (asm_out_file, "\t.section\t%s,\"%s\"", name, flagchars);
 
   /* default_section_type_flags (above) knows which flags need special
@@ -6893,11 +6902,8 @@ 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);
+	  fprintf (asm_out_file, ",");
+	  assemble_name_raw (asm_out_file, func_label);
 	}
       if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
 	{

-- 
Alan Modra
Australia Development Lab, IBM

Revert "rs6000: Avoid -fpatchable-function-entry* regressions on powerpc64 be [PR98125]"

[Alan Modra]

This reverts commit b680b9049737198d010e49cf434704c6a6ed2b3f now
that the PowerPC64 ELFv1 regression is fixed properly.

	PR testsuite/98125
	* targhooks.h (default_print_patchable_function_entry_1): Delete.
	* targhooks.c (default_print_patchable_function_entry_1): Delete.
	(default_print_patchable_function_entry): Expand above.
	* config/rs6000/rs6000.c (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
	Don't define.
	(rs6000_print_patchable_function_entry): Delete.
	* testsuite/g++.dg/pr93195a.C: Revert 2021-04-03 change.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 663eed4f055..d43c36e7f1a 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -1362,10 +1362,6 @@ static const struct attribute_spec rs6000_attribute_table[] =
 #define TARGET_ASM_ASSEMBLE_VISIBILITY rs6000_assemble_visibility
 #endif
 
-#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
-#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY \
-  rs6000_print_patchable_function_entry
-
 #undef TARGET_SET_UP_BY_PROLOGUE
 #define TARGET_SET_UP_BY_PROLOGUE rs6000_set_up_by_prologue
 
@@ -14957,30 +14953,6 @@ rs6000_assemble_visibility (tree decl, int vis)
 }
 #endif
 \f
-/* 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
-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);
-}
-\f
 enum rtx_code
 rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
 {
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 952fad422eb..d69c9a2d819 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1832,15 +1832,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;
@@ -1862,6 +1864,9 @@ default_print_patchable_function_entry_1 (FILE *file,
       patch_area_number++;
       ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
 
+      unsigned int flags = SECTION_WRITE | SECTION_RELRO;
+      if (HAVE_GAS_SECTION_LINK_ORDER)
+	flags |= SECTION_LINK_ORDER;
       switch_to_section (get_section ("__patchable_function_entries",
 				      flags, current_function_decl));
       assemble_align (POINTER_SIZE);
@@ -1878,25 +1883,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 9928d064abd..39a6f82f143 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -230,9 +230,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*-*-* } } */

Re: Revert "rs6000: Avoid -fpatchable-function-entry* regressions on powerpc64 be [PR98125]"

[will schmidt]

On Fri, 2021-05-07 at 12:19 +0930, Alan Modra via Gcc-patches wrote:
> This reverts commit b680b9049737198d010e49cf434704c6a6ed2b3f now
> that the PowerPC64 ELFv1 regression is fixed properly.
> 
Hi,

Ok.  looks like that was initially handled by Jakub, on copy, good. :-)

Contents below appear to match that commit, reversed.
lgtm,
thanks
-Will


> 	PR testsuite/98125
> 	* targhooks.h (default_print_patchable_function_entry_1): Delete.
> 	* targhooks.c (default_print_patchable_function_entry_1): Delete.
> 	(default_print_patchable_function_entry): Expand above.
> 	* config/rs6000/rs6000.c (TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY):
> 	Don't define.
> 	(rs6000_print_patchable_function_entry): Delete.
> 	* testsuite/g++.dg/pr93195a.C: Revert 2021-04-03 change.
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 663eed4f055..d43c36e7f1a 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -1362,10 +1362,6 @@ static const struct attribute_spec rs6000_attribute_table[] =
>  #define TARGET_ASM_ASSEMBLE_VISIBILITY rs6000_assemble_visibility
>  #endif
> 
> -#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
> -#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY \
> -  rs6000_print_patchable_function_entry
> -
>  #undef TARGET_SET_UP_BY_PROLOGUE
>  #define TARGET_SET_UP_BY_PROLOGUE rs6000_set_up_by_prologue
> 
> @@ -14957,30 +14953,6 @@ rs6000_assemble_visibility (tree decl, int vis)
>  }
>  #endif
>  
> -/* 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
> -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);
> -}
> -
>  enum rtx_code
>  rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
>  {
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 952fad422eb..d69c9a2d819 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1832,15 +1832,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;
> @@ -1862,6 +1864,9 @@ default_print_patchable_function_entry_1 (FILE *file,
>        patch_area_number++;
>        ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number);
> 
> +      unsigned int flags = SECTION_WRITE | SECTION_RELRO;
> +      if (HAVE_GAS_SECTION_LINK_ORDER)
> +	flags |= SECTION_LINK_ORDER;
>        switch_to_section (get_section ("__patchable_function_entries",
>  				      flags, current_function_decl));
>        assemble_align (POINTER_SIZE);
> @@ -1878,25 +1883,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 9928d064abd..39a6f82f143 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -230,9 +230,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*-*-* } } */

Re: Revert "rs6000: Avoid -fpatchable-function-entry* regressions on powerpc64 be [PR98125]"

[Segher Boessenkool]

On Fri, May 07, 2021 at 12:19:51PM +0930, Alan Modra wrote:
> This reverts commit b680b9049737198d010e49cf434704c6a6ed2b3f now
> that the PowerPC64 ELFv1 regression is fixed properly.

This is okay when the rest goes in.  Do it in a bisectable order if
possible?  If that is easy :-)


Segher

PowerPC64 ELFv2 -fpatchable-function-entry

[Alan Modra]

PowerPC64 ELFv2 dual entry point functions have a couple of problems
with -fpatchable-function-entry.  One is that the nops added after the
global entry land in the global entry code which is constrained to be
a power of two number of instructions, and zero global entry code has
special meaning for linkage.  The other is that the global entry code
isn't always used on function entry, and some uses of
-fpatchable-function-entry might want to affect all entries to the
function.  So this patch arranges to put one batch of nops before the
global entry, and the other after the local entry point.

	PR target/98125
	* config/rs6000/rs6000.c (rs6000_print_patchable_function_entry): New
	function.
	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
	* config/rs6000/rs6000-logue.c: Include targhooks.h.
	(rs6000_output_function_prologue): Handle nops for
	-fpatchable-function-entry after local entry point.

diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index b0ac183ceff..ffa3bb3dcf1 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -51,6 +51,7 @@
 #include "gstab.h"  /* for N_SLINE */
 #include "dbxout.h" /* dbxout_ */
 #endif
+#include "targhooks.h"
 
 static int rs6000_ra_ever_killed (void);
 static void is_altivec_return_reg (rtx, void *);
@@ -3991,6 +3992,10 @@ rs6000_output_function_prologue (FILE *file)
       fputs (",1\n", file);
     }
 
+  int nops_after_entry = crtl->patch_area_size - crtl->patch_area_entry;
+  if (nops_after_entry > 0)
+    default_print_patchable_function_entry (file, nops_after_entry, false);
+
   /* Output -mprofile-kernel code.  This needs to be done here instead of
      in output_function_profile since it must go after the ELFv2 ABI
      local entry point.  */
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index d43c36e7f1a..97f1b3e0674 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -1404,6 +1404,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
 #undef TARGET_ASM_FUNCTION_EPILOGUE
 #define TARGET_ASM_FUNCTION_EPILOGUE rs6000_output_function_epilogue
 
+#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
+#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY \
+  rs6000_print_patchable_function_entry
+
 #undef TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA
 #define TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA rs6000_output_addr_const_extra
 
@@ -14953,6 +14957,33 @@ rs6000_assemble_visibility (tree decl, int vis)
 }
 #endif
 \f
+/* Write NOPs into the asm outfile FILE around a function entry.  This
+   routine may be called twice per function to put NOPs before and after
+   the function entry.  If RECORD_P is true the location of the NOPs will
+   be recorded by default_print_patchable_function_entry in a special
+   object section called "__patchable_function_entries".  Disable output
+   of any NOPs for the second call.  Those, if any, are output by
+   rs6000_output_function_prologue.  This means that for ELFv2 any NOPs
+   after the function entry are placed after the local entry point, not
+   the global entry point.  NOPs after the entry may be found at
+   record_loc + nops_before * 4 + local_entry_offset.  This holds true
+   when nops_before is zero.  */
+
+static void
+rs6000_print_patchable_function_entry (FILE *file,
+				       unsigned HOST_WIDE_INT patch_area_size ATTRIBUTE_UNUSED,
+				       bool record_p)
+{
+  /* Always call default_print_patchable_function_entry when RECORD_P in
+     order to output the location of the NOPs, but use the size of the
+     area before the entry on both possible calls.  If RECORD_P is true
+     on the second call then the area before the entry was zero size and
+     thus no NOPs will be output.  */
+  if (record_p)
+    default_print_patchable_function_entry (file, crtl->patch_area_entry,
+					    record_p);
+}
+\f
 enum rtx_code
 rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
 {

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[will schmidt]

On Fri, 2021-05-07 at 12:19 +0930, Alan Modra via Gcc-patches wrote:
> PowerPC64 ELFv2 dual entry point functions have a couple of problems
> with -fpatchable-function-entry.  One is that the nops added after the
> global entry land in the global entry code which is constrained to be
> a power of two number of instructions, and zero global entry code has
> special meaning for linkage.  The other is that the global entry code
> isn't always used on function entry, and some uses of
> -fpatchable-function-entry might want to affect all entries to the
> function.  So this patch arranges to put one batch of nops before the
> global entry, and the other after the local entry point.
> 

Hi,

Description good.  :-)

> 	PR target/98125
> 	* config/rs6000/rs6000.c (rs6000_print_patchable_function_entry): New
> 	function.
> 	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
> 	* config/rs6000/rs6000-logue.c: Include targhooks.h.
> 	(rs6000_output_function_prologue): Handle nops for
> 	-fpatchable-function-entry after local entry point.
> 
> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
> index b0ac183ceff..ffa3bb3dcf1 100644
> --- a/gcc/config/rs6000/rs6000-logue.c
> +++ b/gcc/config/rs6000/rs6000-logue.c
> @@ -51,6 +51,7 @@
>  #include "gstab.h"  /* for N_SLINE */
>  #include "dbxout.h" /* dbxout_ */
>  #endif
> +#include "targhooks.h"
> 
>  static int rs6000_ra_ever_killed (void);
>  static void is_altivec_return_reg (rtx, void *);
> @@ -3991,6 +3992,10 @@ rs6000_output_function_prologue (FILE *file)
>        fputs (",1\n", file);
>      }
> 
> +  int nops_after_entry = crtl->patch_area_size - crtl->patch_area_entry;
> +  if (nops_after_entry > 0)
> +    default_print_patchable_function_entry (file, nops_after_entry, false);
> +
>    /* Output -mprofile-kernel code.  This needs to be done here instead of
>       in output_function_profile since it must go after the ELFv2 ABI
>       local entry point.  */
ok


> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index d43c36e7f1a..97f1b3e0674 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -1404,6 +1404,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
>  #undef TARGET_ASM_FUNCTION_EPILOGUE
>  #define TARGET_ASM_FUNCTION_EPILOGUE rs6000_output_function_epilogue
> 
> +#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
> +#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY \
> +  rs6000_print_patchable_function_entry
> +

ok

>  #undef TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA
>  #define TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA rs6000_output_addr_const_extra
> 
> @@ -14953,6 +14957,33 @@ rs6000_assemble_visibility (tree decl, int vis)
>  }
>  #endif
>  
> +/* Write NOPs into the asm outfile FILE around a function entry.  This
> +   routine may be called twice per function to put NOPs before and after
> +   the function entry.  If RECORD_P is true the location of the NOPs will
> +   be recorded by default_print_patchable_function_entry in a special
> +   object section called "__patchable_function_entries".  Disable output
> +   of any NOPs for the second call.  Those, if any, are output by
> +   rs6000_output_function_prologue.  This means that for ELFv2 any NOPs
> +   after the function entry are placed after the local entry point, not
> +   the global entry point.  NOPs after the entry may be found at
> +   record_loc + nops_before * 4 + local_entry_offset.  This holds true
> +   when nops_before is zero.  */
> +
> +static void
> +rs6000_print_patchable_function_entry (FILE *file,
> +				       unsigned HOST_WIDE_INT patch_area_size ATTRIBUTE_UNUSED,
> +				       bool record_p)
> +{
> +  /* Always call default_print_patchable_function_entry when RECORD_P in

when RECORD_P is true?  (implied, but I like to be specific..)
> +     order to output the location of the NOPs, but use the size of the
> +     area before the entry on both possible calls.  If RECORD_P is true
> +     on the second call then the area before the entry was zero size and
> +     thus no NOPs will be output.  */
> +  if (record_p)
> +    default_print_patchable_function_entry (file, crtl->patch_area_entry,
> +					    record_p);
> +}

ok.

lgtm,thx
-Will

> +
>  enum rtx_code
>  rs6000_reverse_condition (machine_mode mode, enum rtx_code code)
>  {

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[Segher Boessenkool]

Hi!

On Fri, May 07, 2021 at 12:19:52PM +0930, Alan Modra wrote:
> PowerPC64 ELFv2 dual entry point functions have a couple of problems
> with -fpatchable-function-entry.  One is that the nops added after the
> global entry land in the global entry code which is constrained to be
> a power of two number of instructions, and zero global entry code has
> special meaning for linkage.  The other is that the global entry code
> isn't always used on function entry, and some uses of
> -fpatchable-function-entry might want to affect all entries to the
> function.  So this patch arranges to put one batch of nops before the
> global entry, and the other after the local entry point.

Wow ugly.  Not worse than patchable-funtion-enbtry itself I suppose :-)

> 	* config/rs6000/rs6000-logue.c: Include targhooks.h.

Huh, did it not already do that?!  Hrm, all the other hooks seem to be
called via rs6000.c currently.  But you do the same, so why do you need
to include the header in rs6000-logue.c?

> +/* Write NOPs into the asm outfile FILE around a function entry.  This
> +   routine may be called twice per function to put NOPs before and after
> +   the function entry.  If RECORD_P is true the location of the NOPs will
> +   be recorded by default_print_patchable_function_entry in a special
> +   object section called "__patchable_function_entries".  Disable output
> +   of any NOPs for the second call.  Those, if any, are output by
> +   rs6000_output_function_prologue.  This means that for ELFv2 any NOPs
> +   after the function entry are placed after the local entry point, not
> +   the global entry point.  NOPs after the entry may be found at
> +   record_loc + nops_before * 4 + local_entry_offset.  This holds true
> +   when nops_before is zero.  */
> +
> +static void
> +rs6000_print_patchable_function_entry (FILE *file,
> +				       unsigned HOST_WIDE_INT patch_area_size ATTRIBUTE_UNUSED,
> +				       bool record_p)

It is not a predicate.  Do not call it _p please.  Yes, I know the
generic code calls is this.  That needs fixing.

A better name (from the viewpoint of callers, which is what matters!)
might be first_time or similar?  Or something that really says what it
*does*?

> +{
> +  /* Always call default_print_patchable_function_entry when RECORD_P in
> +     order to output the location of the NOPs, but use the size of the
> +     area before the entry on both possible calls.  If RECORD_P is true
> +     on the second call then the area before the entry was zero size and
> +     thus no NOPs will be output.  */
> +  if (record_p)
> +    default_print_patchable_function_entry (file, crtl->patch_area_entry,
> +					    record_p);
> +}

That is trickiness that will break later.

Can you make this less hacky please?  Changing the generic code is just
fine as well, it needs some love.


Segher

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[Alan Modra]

On Mon, May 10, 2021 at 04:39:55PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, May 07, 2021 at 12:19:52PM +0930, Alan Modra wrote:
> > PowerPC64 ELFv2 dual entry point functions have a couple of problems
> > with -fpatchable-function-entry.  One is that the nops added after the
> > global entry land in the global entry code which is constrained to be
> > a power of two number of instructions, and zero global entry code has
> > special meaning for linkage.  The other is that the global entry code
> > isn't always used on function entry, and some uses of
> > -fpatchable-function-entry might want to affect all entries to the
> > function.  So this patch arranges to put one batch of nops before the
> > global entry, and the other after the local entry point.
> 
> Wow ugly.  Not worse than patchable-funtion-enbtry itself I suppose :-)
> 
> > 	* config/rs6000/rs6000-logue.c: Include targhooks.h.
> 
> Huh, did it not already do that?!  Hrm, all the other hooks seem to be
> called via rs6000.c currently.  But you do the same, so why do you need
> to include the header in rs6000-logue.c?

For the new call to default_print_patchable_function_entry in
rs6000_output_function_prologue.

> 
> > +/* Write NOPs into the asm outfile FILE around a function entry.  This
> > +   routine may be called twice per function to put NOPs before and after
> > +   the function entry.  If RECORD_P is true the location of the NOPs will
> > +   be recorded by default_print_patchable_function_entry in a special
> > +   object section called "__patchable_function_entries".  Disable output
> > +   of any NOPs for the second call.  Those, if any, are output by
> > +   rs6000_output_function_prologue.  This means that for ELFv2 any NOPs
> > +   after the function entry are placed after the local entry point, not
> > +   the global entry point.  NOPs after the entry may be found at
> > +   record_loc + nops_before * 4 + local_entry_offset.  This holds true
> > +   when nops_before is zero.  */
> > +
> > +static void
> > +rs6000_print_patchable_function_entry (FILE *file,
> > +				       unsigned HOST_WIDE_INT patch_area_size ATTRIBUTE_UNUSED,
> > +				       bool record_p)
> 
> It is not a predicate.  Do not call it _p please.  Yes, I know the
> generic code calls is this.  That needs fixing.
> 
> A better name (from the viewpoint of callers, which is what matters!)
> might be first_time or similar?  Or something that really says what it
> *does*?
> 
> > +{
> > +  /* Always call default_print_patchable_function_entry when RECORD_P in
> > +     order to output the location of the NOPs, but use the size of the
> > +     area before the entry on both possible calls.  If RECORD_P is true
> > +     on the second call then the area before the entry was zero size and
> > +     thus no NOPs will be output.  */
> > +  if (record_p)
> > +    default_print_patchable_function_entry (file, crtl->patch_area_entry,
> > +					    record_p);
> > +}
> 
> That is trickiness that will break later.

There is a fine line between trickiness and elegance.  Given the
current available crtl variables dealing with the patch area and the
current patchable_function_entry parameters, any supposedly "less
hacky" but correct expression will simplify down to what I wrote
here.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[Segher Boessenkool]

Hi!

On Tue, May 18, 2021 at 07:43:49PM +0930, Alan Modra wrote:
> On Mon, May 10, 2021 at 04:39:55PM -0500, Segher Boessenkool wrote:
> > Huh, did it not already do that?!  Hrm, all the other hooks seem to be
> > called via rs6000.c currently.  But you do the same, so why do you need
> > to include the header in rs6000-logue.c?
> 
> For the new call to default_print_patchable_function_entry in
> rs6000_output_function_prologue.

I see, thanks.

> > > +{
> > > +  /* Always call default_print_patchable_function_entry when RECORD_P in
> > > +     order to output the location of the NOPs, but use the size of the
> > > +     area before the entry on both possible calls.  If RECORD_P is true
> > > +     on the second call then the area before the entry was zero size and
> > > +     thus no NOPs will be output.  */
> > > +  if (record_p)
> > > +    default_print_patchable_function_entry (file, crtl->patch_area_entry,
> > > +					    record_p);
> > > +}
> > 
> > That is trickiness that will break later.
> 
> There is a fine line between trickiness and elegance.  Given the
> current available crtl variables dealing with the patch area and the
> current patchable_function_entry parameters, any supposedly "less
> hacky" but correct expression will simplify down to what I wrote
> here.

I wrote a bit later:

> > Can you make this less hacky please?  Changing the generic code is just
> > fine as well, it needs some love.

In effect making a callback / hook without making that explicit is bad
for maintainability.  We are in stage 1, now is a good time for
infrastructure changes.


Segher

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[Alan Modra]

On Tue, May 18, 2021 at 05:48:40AM -0500, Segher Boessenkool wrote:
> I wrote a bit later:
> 
> > > Can you make this less hacky please?  Changing the generic code is just
> > > fine as well, it needs some love.
> 
> In effect making a callback / hook without making that explicit is bad
> for maintainability.  We are in stage 1, now is a good time for
> infrastructure changes.

Yes, perhaps I could do that, and possibly even write a patch that is
accepted.  I'm not going to though, because I made a decision a little
while ago that I'm not going to contribute new gcc work.  This one was
just flushing some patches that I wrote a while ago.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PowerPC64 ELFv2 -fpatchable-function-entry

[Segher Boessenkool]

On Wed, May 19, 2021 at 09:39:30PM +0930, Alan Modra wrote:
> On Tue, May 18, 2021 at 05:48:40AM -0500, Segher Boessenkool wrote:
> > I wrote a bit later:
> > 
> > > > Can you make this less hacky please?  Changing the generic code is just
> > > > fine as well, it needs some love.
> > 
> > In effect making a callback / hook without making that explicit is bad
> > for maintainability.  We are in stage 1, now is a good time for
> > infrastructure changes.
> 
> Yes, perhaps I could do that, and possibly even write a patch that is
> accepted.  I'm not going to though, because I made a decision a little
> while ago that I'm not going to contribute new gcc work.  This one was
> just flushing some patches that I wrote a while ago.

Okay, thanks.  I opened PR100714 to keep track of the work that will
need to be done.

Cheers,


Segher

Re: PR98125, PowerPC64 -fpatchable-function-entry

[Segher Boessenkool]

On Fri, May 07, 2021 at 12:19:49PM +0930, Alan Modra wrote:
> This series of patches fixes -fpatchable-function-entry on PowerPC64
> ELFv1 so that SECTION_LINK_ORDER (.section 'o' arg) is now supported,
> and on PowerPC64 ELFv2 to not break the global entry code.
> 
> Bootstrapped powerpc64le-linux and x86_64-linux all langs.  I did see
> one regression on both targets, libgo runtime/pprof.  It's unclear to
> me what that means.

Probably nothing?  It is one of the tests that fail regularly.


Segher