PowerPC64 ELFv1 -fpatchable-function-entry

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.

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

>  }
>

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

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

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))
 	{