[Bug target/99888] New: Add powerpc ELFv2 support for -fpatchable-function-entry*

[Bug target/99888] New: Add powerpc ELFv2 support for -fpatchable-function-entry*

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

            Bug ID: 99888
           Summary: Add powerpc ELFv2 support for
                    -fpatchable-function-entry*
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
                CC: amodra at gmail dot com, hjl.tools at gmail dot com,
                    jakub at gcc dot gnu.org, marxin at gcc dot gnu.org,
                    seurer at gcc dot gnu.org, unassigned at gcc dot gnu.org
        Depends on: 98125
  Target Milestone: ---
              Host: powerpc64*-linux-gnu
            Target: powerpc64*-linux-gnu
             Build: powerpc64*-linux-gnu

+++ This bug was initially created as a clone of Bug #98125 +++

As mentioned in PR98125, for ELFv2 the patchable nops need to be added after
the first two insns of the function rather than before it.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98125
[Bug 98125] [11 Regression] New test case g++.dg/pr93195a.C in r11-5656 has
excess errors

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888
Bug 99888 depends on bug 98125, which changed state.

Bug 98125 Summary: [11 Regression] New test case g++.dg/pr93195a.C in r11-5656 has excess errors
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98125

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
                 CC|                            |linkw at gcc dot gnu.org
   Last reconfirmed|                            |2022-08-03
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
cat test.c 

extern int a;
int foo (int b){
  return a + b;
}

on ppc64le, with option "-mcpu=power9 -fpatchable-function-entry=5,3", the
generated assembly looks like:

.LPFE1:
        nop
        nop
        nop
        .type   foo, @function
foo:
        nop
        nop
.LFB0:
        .cfi_startproc
.LCF0:
0:      addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        .localentry     foo,.-foo
        std 31,-8(1)

Jakub's #c0 noted that the "after" nops should be placed after local entry, but
there seem two choices to put the "before" nops, such as:

1)

0:      addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        nop
        nop
        nop
        .localentry     foo,.-foo
        nop
        nop
        std 31,-8(1)

2)

0:      nop
        nop
        nop
        addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        .localentry     foo,.-foo
        nop
        nop
        std 31,-8(1)

For either of them, the "before" nops only take effects when it's entered from
global entry, both are counted as five insns in global entry area, there is no
differences from these two perspectives. I'd like to go with 1) if no
objections.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
Created attachment 53405
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53405&action=edit
untested patch

With the attached patch, for -fpatchable-function-entry=5,2 it gets:

foo:
.LFB0:
        .cfi_startproc
.LCF0:
0:      addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        .section        __patchable_function_entries,"awo",@progbits,foo
        .align 3
        .8byte  .LPFE1
        .section        ".text"
.LPFE1:
        nop
        nop
        .localentry     foo,.-foo
        .section        __patchable_function_entries,"awo",@progbits,foo
        .align 3
        .8byte  .LPFE2
        .section        ".text"
.LPFE2:
        nop
        nop
        nop
        std 31,-8(1)

for -fpatchable-function-entry=5,1, it emits error msg:

test.c:4:1: error: ‘-fpatchable-function-entry=M,N’ N NOPs can cause assembler
error due to invalid .localentry expression.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Your second option isn't correct: all these nops should be consecutive.  Your
option 1 is fine :-)

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #3)
> Your second option isn't correct: all these nops should be consecutive.  Your
> option 1 is fine :-)

Good point! It's lucky that I chose option 1. :)

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[i at maskray dot me]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Fangrui Song <i at maskray dot me> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |i at maskray dot me

--- Comment #5 from Fangrui Song <i at maskray dot me> ---
* There is a restriction on the number of instructions between the function
label and the .localentry directive.
* For -fpatchable-function-entry=N[,M], M nops must precede the function label.

On aarch64/x86/etc, these nops are consecutive. Personally I think this
condition can be lifted for PowerPC ELFv2. The runtime library will need to
check st_other or do some instruction inspection, which may be fine.


        nop
        nop
        nop
foo:
.LCF0:
        .cfi_startproc
        addis 2,12,.TOC.-.LCF0@ha
        addi 2,2,.TOC.-.LCF0@l
        .localentry     foo,.-foo
        nop
        nop

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Fangrui Song from comment #5)
> * There is a restriction on the number of instructions between the function
> label and the .localentry directive.
> * For -fpatchable-function-entry=N[,M], M nops must precede the function
> label.
> 
> On aarch64/x86/etc, these nops are consecutive. Personally I think this
> condition can be lifted for PowerPC ELFv2. The runtime library will need to
> check st_other or do some instruction inspection, which may be fine.
>  
>  
>         nop
>         nop
>         nop
> foo:
> .LCF0:
>         .cfi_startproc
>         addis 2,12,.TOC.-.LCF0@ha
>         addi 2,2,.TOC.-.LCF0@l
>         .localentry     foo,.-foo
>         nop
>         nop

Thanks for the input! With your proposal, the nice thing is that we don't need
to bother the count of NOPs between GEP and LEP. But IMHO it seems confusing
since we take GEP as function entry when patching preceding NOPs while take LEP
as function entry when patching succeeding NOPs. The acceptable counts of NOPs
in the proposed patch conforms to ELFv2 ABI, I hope runtime library can survive
with it.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
'-fpatchable-function-entry=N[,M]'
     Generate N NOPs right at the beginning of each function, with the
     function entry point before the Mth NOP.

The nops have to be consecutive.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #8 from Alan Modra <amodra at gmail dot com> ---
(In reply to Segher Boessenkool from comment #7)
> '-fpatchable-function-entry=N[,M]'
>      Generate N NOPs right at the beginning of each function, with the
>      function entry point before the Mth NOP.

Bad doco.  Should be "after the Mth NOP" I think.  Or better written to avoid
the concept of a 0th nop.  Default for M is zero, placing all nops after the
function entry and before normal function prologue code.

> The nops have to be consecutive.

I hope you are making this statement based on an analysis of the purpose of
having M nops before the entry point and N-M after the entry point, because the
documentation you are quoting doesn't take into account the fact that ELFv2
functions have two entry points.  We don't have "the" entry point.

I admit I didn't analyse -fpatchable-function-entry usage in any depth before
writing the patches in PR98125.  All I did was look at the linux kernel to the
point of deciding that we want a patchable area after the local entry point to
catch all calls to the function.  That would be what
-fpatchable-function-entry=n does for ELFv2, and I think we all agree on that.

Why would someone want nops before a function entry?  Perhaps as space for a
pointer.  Or perhaps as the main patch area branched to from patched code after
the entry, to limit number of nops executed in an unpatched function.  Or
perhaps so that the function can be called by a trampoline or via function
pointer, to select either some extra code replacing those nops or the normal
function entry.  I think that means M nops go before the global entry point. 
(Note that the patch area before a function could well duplicate the normal
global entry code.)

I agree with comment #5.  nops *inside* the global entry code are a daft idea.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #9 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Alan Modra from comment #8)
> (In reply to Segher Boessenkool from comment #7)
> > '-fpatchable-function-entry=N[,M]'
> >      Generate N NOPs right at the beginning of each function, with the
> >      function entry point before the Mth NOP.
> 
> Bad doco.  Should be "after the Mth NOP" I think.  Or better written to
> avoid the concept of a 0th nop.  Default for M is zero, placing all nops
> after the function entry and before normal function prologue code.

It is correct as written?

Also, "M" isn't used in the current compiler (and *cannot* be used: it is a
local variable that goes out of scope after being set, patch_area_start in
process_options).

[The text is carefully written so that "anywhere before the Mth nop" will be
a valid implementation as well, btw, that perhaps explains the tortured
language here.  But maybe there is another explanation for that).

> > The nops have to be consecutive.
> 
> I hope you are making this statement based on

Based on just what is written.  "N nops right at the beginning of the
function".
Not very formal, but not open to other interpretations either.

> an analysis of the purpose of
> having M nops before the entry point and N-M after the entry point, because
> the documentation you are quoting doesn't take into account the fact that
> ELFv2 functions have two entry points.  We don't have "the" entry point.

If ELFv2 wants to do something with the LEP here, it should make some extra
flag here.  Abusing generic facilities for a different purpose never works.

> I admit I didn't analyse -fpatchable-function-entry usage in any depth
> before writing the patches in PR98125.  All I did was look at the linux
> kernel to the point of deciding that we want a patchable area after the
> local entry point to catch all calls to the function.  That would be what
> -fpatchable-function-entry=n does for ELFv2, and I think we all agree on
> that.

The PowerPC Linux kernel uses -mprofile-kernel instead, which works a lot
better for them AFAIUI.  Are people planning on changing that?

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #10 from Kewen Lin <linkw at gcc dot gnu.org> ---
By searching the history of this feature, I found its initial versions only
proposed to place nops after the function entry, such as: v2[1], then it's
requested to be more generic to handle some "exploited atomically" requirements
for RISV arches. Please see the below quoted content posted by Jose for
SPARC[2] and more [3] to extend it for preceding nops.

"
  How is this supposed to be exploited atomically in RISC arches such as
  sparc?  In such architectures you usually need to patch several
  instructions to load an absolute address into a register.

  If a general mechanism is what is intended I would suggest to offer the
  possibility of extending the nops _before_ the function entry point,
  like in:

  (a) nop   ! Load address
      nop   ! Load address
      nop   ! Load address
      nop   ! Load address
      nop   ! Jump to loaded address.
  entry:
  (b) nop   ! PC-relative jump to (a)
      save %sp, bleh, %sp
      ...

  So after the live-patcher patches the loading of the destination address
  and the jump, it can atomically patch (b) to effectively replace the
  implementation of `entry'.
"

So placing just only one nop after function entry and leaving multiple nops to
be patched before function entry was meant to make it exploited atomically.

I'm not sure if there will be this kind of requirement for future uses of this
feature on ppc64le. If we assume there is, we need to consider if the current
proposal can support it and even easily.

With proposal 1) in #c1, that is to place nops before and after local entry
point. It allows three kinds of counts for preceding nops: 2, 6 and 14. IMHO,
the count 14 seems to be enough for most cases? But people can blame it's not
flexible for all kinds of counts, and it can take more size if the required
count doesn't perfectly match one allowable count. Besides, it can offer bad
user experience when users port their workable cases here but get errors.

With proposal in #5, it doesn't have the restriction on the count of preceding
nops, it's a very good thing. The main concern is what Segher pointed out, the
patched nops are concluded to be consecutive, in the initial versions it's more
explicit as the option name is "prolog-pad". And the separated nop sequences
are for different function entry points, not for "the" function entry.

To offer more flexibility to users, proposal in #5 looks better, but it
requires one documentation update by saying the particularity on ppc64le, that
is dual entries and the patching way.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #11 from Kewen Lin <linkw at gcc dot gnu.org> ---
Oops, the reference links in #c10 are:

[1] https://gcc.gnu.org/pipermail/gcc-patches/2016-September/458210.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2016-September/458287.html
[3] https://gcc.gnu.org/pipermail/gcc-patches/2016-October/458587.html

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:c23b5006d3ffeda1a9edf5fd817765a6da3696ca

commit r13-2984-gc23b5006d3ffeda1a9edf5fd817765a6da3696ca
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Fri Sep 30 07:16:49 2022 -0500

    rs6000: Rework ELFv2 support for -fpatchable-function-entry* [PR99888]

    As PR99888 and its related show, the current support for
    -fpatchable-function-entry on powerpc ELFv2 doesn't work
    well with global entry existence.  For example, with one
    command line option -fpatchable-function-entry=3,2, it got
    below w/o this patch:

      .LPFE1:
              nop
              nop
              .type   foo, @function
      foo:
              nop
      .LFB0:
              .cfi_startproc
      .LCF0:
      0:      addis 2,12,.TOC.-.LCF0@ha
              addi 2,2,.TOC.-.LCF0@l
              .localentry     foo,.-foo

    , the assembly is unexpected since the patched nops have
    no effects when being entered from local entry.

    This patch is to update the nops patched before and after
    local entry, it looks like:

              .type   foo, @function
      foo:
      .LFB0:
              .cfi_startproc
      .LCF0:
      0:      addis 2,12,.TOC.-.LCF0@ha
              addi 2,2,.TOC.-.LCF0@l
              nop
              nop
              .localentry     foo,.-foo
              nop

            PR target/99888
            PR target/105649

    gcc/ChangeLog:

            * doc/invoke.texi (option -fpatchable-function-entry): Adjust the
            documentation for PowerPC ELFv2 ABI dual entry points.
            * config/rs6000/rs6000-internal.h
            (rs6000_print_patchable_function_entry): New function declaration.
            * config/rs6000/rs6000-logue.cc (rs6000_output_function_prologue):
            Support patchable-function-entry by emitting nops before and after
            local entry for the function that needs global entry.
            * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry):
Skip
            the function that needs global entry till global entry has been
            emitted.
            * config/rs6000/rs6000.h (struct machine_function): New bool member
            global_entry_emitted.

    gcc/testsuite/ChangeLog:

            * gcc.target/powerpc/pr99888-1.c: New test.
            * gcc.target/powerpc/pr99888-2.c: New test.
            * gcc.target/powerpc/pr99888-3.c: New test.
            * gcc.target/powerpc/pr99888-4.c: New test.
            * gcc.target/powerpc/pr99888-5.c: New test.
            * gcc.target/powerpc/pr99888-6.c: New test.
            * c-c++-common/patchable_function_entry-default.c: Adjust for
            powerpc_elfv2 to avoid compilation error.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |giuliano.belinassi at gmail dot co
                   |                            |m

--- Comment #13 from Kewen Lin <linkw at gcc dot gnu.org> ---
*** Bug 105649 has been marked as a duplicate of this bug. ***

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from Kewen Lin <linkw at gcc dot gnu.org> ---
Fixed.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Michael Matz <matz at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matz at gcc dot gnu.org

--- Comment #15 from Michael Matz <matz at gcc dot gnu.org> ---
Umm.  I just noticed this one as we now try to implement userspace live
patching
for ppc64le.  The point of the "before" NOPs is (and always was) that they are
completely out of the way of patchable but as-of-yet unpatched functions.

For ppc that means the "before" and "after" NOPs cannot be consecutive.  The
two
NOP sets being consecutive was never a design criteria or requirement.

So, while the original bug is fixed by what was committed (local entry was
skipping the patching-nops), the chosen solution is exactly the wrong one :-/

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

--- Comment #16 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #15)
> Umm.  I just noticed this one as we now try to implement userspace live
> patching
> for ppc64le.  The point of the "before" NOPs is (and always was) that they
> are
> completely out of the way of patchable but as-of-yet unpatched functions.
> 
> For ppc that means the "before" and "after" NOPs cannot be consecutive.  The
> two
> NOP sets being consecutive was never a design criteria or requirement.
> 
> So, while the original bug is fixed by what was committed (local entry was
> skipping the patching-nops), the chosen solution is exactly the wrong one :-/

Thanks for the input! Sigh, sorry that we picked up the wrong one :(, you may
have noticed that the main consideration to choose the current one is to keep
it align with the consecutive NOPs described by the documentation, we need a
separate command line option as Segher's review comment in
https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600239.html. Now we have
PR112980 filed for the requested behavior, let's discuss how to support it
there.

[Bug target/99888] Add powerpc ELFv2 support for -fpatchable-function-entry*

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0