[Bug target/112980] New: 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[Bug target/112980] New: 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[naveen at kernel dot org]

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

            Bug ID: 112980
           Summary: 64-bit powerpc ELFv2 does not allow nops to be
                    generated before function global entry point
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: naveen at kernel dot org
  Target Milestone: ---

Bug 99888 addressed issue with -fpatchable-function-entry not generating nops
at function local entry point with ppc64le ELFv2. However, the change also
meant that there is now no way to request for nops to be generated _before_ the
function global entry point.

This is problematic if we only want to have two nops generated for storing a
64-bit value. With the current implementation of -fpatchable-function-entry, we
are forced to emit at least 6 nops due to ABI restrictions on the number of
instructions between GEP and LEP. This is required to support Linux kernel
features such as DYNAMIC_FTRACE_WITH_CALL_OPS [1]. Furthermore, it may be
desirable to generate nops before the function entry point so as to minimize
icache usage with functions entered through the global entry point.

Please consider adding an option to allow nops to be generated before the
global entry point.

[1] https://lore.kernel.org/all/20230123134603.1064407-2-mark.rutland@arm.com/

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matz at gcc dot gnu.org
   Last reconfirmed|                            |2024-01-18
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org
     Ever confirmed|0                           |1
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=99888
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
[1] made me realize that I forgot to post some comments here. (I thought I did
but actually didn't).

As Segher's review comments in [2], to support "before NOPs" before global
entry and "after NOPs" after global entry, we need to introduce a separate
command line option, I think it can be a target specific option, which is
enabled by default and we should mention its default behavior and impact in the
current documentation for -fpatchable-function-entry. I don't have a good name
candidate, any suggestions?

Considering that the current behavior aligning with consecutive NOPs looks
useless (this request and [1]), an alternative is to aggressively change the
current behavior to "before NOPs" before global entry and "after NOPs" after
global entry.

Any preference or other ideas?  Any comments are highly appreciated.

I think with either (any) proposal it's inevitable to make the current behavior
of -fpatchable-function-entry on "before NOPs" change, we should also document
this change in releases/changes.html.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99888#c15
[2] https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600239.html

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[naveen at kernel dot org]

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

--- Comment #2 from Naveen N Rao <naveen at kernel dot org> ---
I don't really have a preference, though I tend to agree that nops before the
local entry point aren't that useful. Even with the current approach, not all
functions have instructions at the GEP and for those, the nops are being
generated outside the function. We also won't have a separate GEP/LEP with
pcrel, so we won't need a separate option eventually.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[matz at gcc dot gnu.org]

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

--- Comment #3 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #1)
> 
> As Segher's review comments in [2], to support "before NOPs" before global
> entry and "after NOPs" after global entry,

Just to be perfectly clear here: the "after NOPs" need to come after local
entry
(which strictly speaking is of course after the global one as well, but I'm
being anal :) ).

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Naveen N Rao from comment #2)
> I don't really have a preference, though I tend to agree that nops before
> the local entry point aren't that useful. Even with the current approach,
> not all functions have instructions at the GEP and for those, the nops are
> being generated outside the function. We also won't have a separate GEP/LEP
> with pcrel, so we won't need a separate option eventually.

Thanks for the input! Looking forward to the comments from the others,
especially Segher, David and Peter.

(In reply to Michael Matz from comment #3)
> (In reply to Kewen Lin from comment #1)
> > 
> > As Segher's review comments in [2], to support "before NOPs" before global
> > entry and "after NOPs" after global entry,
> 
> Just to be perfectly clear here: the "after NOPs" need to come after local
> entry
> (which strictly speaking is of course after the global one as well, but I'm
> being anal :) ).

Oops, good catch, I meant to type "after local entry", thanks for the
correction making it perfectly clear. :)

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[jamborm at gcc dot gnu.org]

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

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I'd like to ping this, are there plans to implement this in the near-ish term?

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #5)
> I'd like to ping this, are there plans to implement this in the near-ish
> term?

Some weeks ago, Naveen had been doing some experiments to see if there is a
better way for function tracer support, and if the idea works and the
experiment result is promising, he may request something different, so we are
still waiting for that. @Naveen Feel free to correct me if any
misunderstanding.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[naveen at kernel dot org]

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

--- Comment #7 from Naveen N Rao <naveen at kernel dot org> ---
I have been looking at an alternative approach to see if we can move the entire
function patching sequence out of line. However, the approach I am considering
is very specific to the linux kernel, and I don't see it applying for userspace
in a generic way. As such, I think there is value in addressing the current
limitation with -fpatchable-function-entry one way or another.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #8 from Kewen Lin <linkw at gcc dot gnu.org> ---
Hi @Michael, @Martin, could you help to confirm/clarify what triggers you to be
interested in this feature, is it for some user space usage or not?

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[giuliano.belinassi at gmail dot com]

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

--- Comment #9 from Giuliano Belinassi <giuliano.belinassi at gmail dot com> ---
Hello, Kewen.

(In reply to Kewen Lin from comment #8)
> Hi @Michael, @Martin, could you help to confirm/clarify what triggers you to
> be interested in this feature, is it for some user space usage or not?

Yes, this is for userspace livepatching.

Assume the following example:
https://godbolt.org/z/b9M8nMbo1

As one can see, the sequence of 14 nops are generated after the global function
entry point. In such way we can't use the this space to place a trampoline to
the new function. We need this sequence of nops to be placed *before* the
global function entry point.

Thanks,
Giuliano.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #10 from Kewen Lin <linkw at gcc dot gnu.org> ---
Created attachment 57844
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57844&action=edit
patch changing the current implementation

Considering the current implementation is not useful at all for both kernel and
userspace uses, I'm inclined to change the current implementation instead of
introducing another option, but updating the documentation to emphasize the
NOPs may not be consecutive for this case.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #11 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Giuliano Belinassi from comment #9)
> Yes, this is for userspace livepatching.
> 
> Assume the following example:
> https://godbolt.org/z/b9M8nMbo1
> 
> As one can see, the sequence of 14 nops are generated after the global
> function entry point. In such way we can't use the this space to place a
> trampoline to the new function. We need this sequence of nops to be placed
> *before* the global function entry point.
> 

Hi Giuliano, thanks for the inputs!

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[giuliano.belinassi at gmail dot com]

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

--- Comment #12 from Giuliano Belinassi <giuliano.belinassi at gmail dot com> ---
With your patch we have:

> .LPFE0:
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	nop
>	.type	function, @function
> function:
> .LFB0:
> 	.cfi_startproc
> .LCF0:
> 0:	addis 2,12,.TOC.-.LCF0@ha
> 	addi 2,2,.TOC.-.LCF0@l
>	.localentry	function,.-function
>	nop
>	nop
>	mflr %r0

Which seems what is expected.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #13 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Giuliano Belinassi from comment #12)
> With your patch we have:
> 
> > .LPFE0:
> > ...
> Which seems what is expected.

Hi Giuliano, thanks for your time on testing it!  Could you kindly help to
explain a bit on why "In such way we can't use the this space to place a
trampoline to the new function"? Is it due to inefficient code like needing
more branches?

global entry:
  [b localentry]
L1:
  [patched code]

localentry:
  [b L1]

Or some other reason which makes it unused at all?

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[matz at gcc dot gnu.org]

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

--- Comment #14 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #13)
> (In reply to Giuliano Belinassi from comment #12)
> > With your patch we have:
> > 
> > > .LPFE0:
> > > ...
> > Which seems what is expected.
> 
> Hi Giuliano, thanks for your time on testing it!  Could you kindly help to
> explain a bit on why "In such way we can't use the this space to place a
> trampoline to the new function"? Is it due to inefficient code like needing
> more branches?
> 
> global entry:
>   [b localentry]
> L1:
>   [patched code]
> localentry:
>   [b L1]
> 
> Or some other reason which makes it unused at all?

Hmm?  But this is not how the global-to-local hand-off is implemented (and
expected by tooling): a fall-through.  The global entry sets up the GOT
register, there simply is no '[b localentry]'.

If you mean to imply that also the '[b localentry]' should be patched in at
live-patch application time (and hence the GOT setup would need to be moved
to still somewhere else), then you have the problem that (in the
not-yet-patched 
case) as long as the L1-nops sit between global and local entry they will
always 
be executed when the global entry is called.  That's wasteful.

Additionally tooling will be surprised if the address difference between
global and local entry isn't exactly 8 (i.e. two instructions).  The psABI
allows for different values, of course.  But I'm willing to bet that there are
bugs in the wild when different values would be actually used.

So, the nops-between-gep-and-lep could probably be somehow made to work with
userspace live patching, but your most recent patch here makes this all mood.
It generates exactly the sequence we want: a single nop at the LEP, and
a configurable patching area outside of, but near to, the function (here: in
front of the GEP).

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #15 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #14)
> Hmm?  But this is not how the global-to-local hand-off is implemented (and
> expected by tooling): a fall-through.  The global entry sets up the GOT
> register, there simply is no '[b localentry]'.
> 
> If you mean to imply that also the '[b localentry]' should be patched in at
> live-patch application time (and hence the GOT setup would need to be moved
> to still somewhere else), then you have the problem that (in the
> not-yet-patched 
> case) as long as the L1-nops sit between global and local entry they will
> always 
> be executed when the global entry is called.

Sorry for confusion, I meant the sequence like:

global entry:
  [TOC base setup] // always here
  [b localentry] // which is added when patching
L1:
  [patched code] // from patching
  localentry: 
  [b L1]         // from patching

> That's wasteful.

I agree, nops are not zero cost on Power8/Power9.

> 
> Additionally tooling will be surprised if the address difference between
> global and local entry isn't exactly 8 (i.e. two instructions).  The psABI
> allows for different values, of course.  But I'm willing to bet that there
> are
> bugs in the wild when different values would be actually used.
> 

It's possible that some tooling doesn't conform the ABI doc well, but I think
the tooling should fix itself if that is the case. :)

> So, the nops-between-gep-and-lep could probably be somehow made to work with
> userspace live patching, but your most recent patch here makes this all mood.
> It generates exactly the sequence we want: a single nop at the LEP, and
> a configurable patching area outside of, but near to, the function (here: in
> front of the GEP).

I agree, thanks for the comments! btw, I'm not fighting for the current
implementation, just want to know more details why users are unable to make use
of the current implementation, is it just due to its inefficiency (like the
above sequence) or un-usability (unused at all). As your comments, I think it's
due to the former (inefficiency)?!

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[matz at gcc dot gnu.org]

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

--- Comment #16 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #15)
> I agree, thanks for the comments! btw, I'm not fighting for the current
> implementation, just want to know more details why users are unable to make
> use of the current implementation, is it just due to its inefficiency (like
> the above sequence) or un-usability (unused at all). As your comments, I
> think it's due to the former (inefficiency)?!

Okay.  So, yeah, I _think_ that other way (with NOPs between GEP and LEP,
plus a jump around them) could be made to work with userspace live patching.
It would just be inefficient.  But do note that that jump around was _not_
part of the original way of -fpatchable-function-entry, so a change to codegen
would have to have happened anyway to make that other way usable.  And it has
the
(perhaps theoretical, who knows :) ) problem of not using the normal 8-byte
difference between GEP and LEP.

I think your current proposal from comment #10 is the better from all
perspectives.

[Bug target/112980] 64-bit powerpc ELFv2 does not allow nops to be generated before function global entry point

[linkw at gcc dot gnu.org]

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

--- Comment #17 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #16)
> (In reply to Kewen Lin from comment #15)
> > I agree, thanks for the comments! btw, I'm not fighting for the current
> > implementation, just want to know more details why users are unable to make
> > use of the current implementation, is it just due to its inefficiency (like
> > the above sequence) or un-usability (unused at all). As your comments, I
> > think it's due to the former (inefficiency)?!
> 
> Okay.  So, yeah, I _think_ that other way (with NOPs between GEP and LEP,
> plus a jump around them) could be made to work with userspace live patching.
> It would just be inefficient.  But do note that that jump around was _not_
> part of the original way of -fpatchable-function-entry, so a change to
> codegen
> would have to have happened anyway to make that other way usable.  And it
> has the
> (perhaps theoretical, who knows :) ) problem of not using the normal 8-byte
> difference between GEP and LEP.
> 

Thanks again for confirming this understanding!

> I think your current proposal from comment #10 is the better from all
> perspectives.

Yeah, I agree. When reworking this support previously, comment #10 like
implementation was considered as a better one but it's not finally made due to
the concern that can break the assumption NOPs should be consecutive, based on
all the inputs here I think it's time to "fix" it by just underscoring this
special not-consecutive NOPs in documentation.