RFC: New function attribute "patchable-prologue"="<kind>"

[Proposed langref entry]

The "patchable-prologue" attribute on a function is a general
mechanism to control the form of a function's prologue in ways that
make it easy to patch at runtime.

Currently only one value is supported:

# "hotpatch-compact"

If a function is marked with "patchable-prologue"="hotpatch-compact"
then:

  1. The first instruction of the function is at least two bytes long.
  2. The first two bytes of the first instruction does not span a cache
     line boundary.
  3. The instruction denoted by (1) is preferably not a no-op, i.e.,
     we'd prefer to "re-use" an instruction already present. For
     instance, we can emit a two byte form of a "push CSR" instruction
     that we'd have needed anyway.

"hotpatch-compact" is useful for runtimes that want to thread-safely
overwrite the first instruction of a function with a short branch.

[End proposed langref entry]

We can consider adding more "schemes" in the future, for instance
"hotpatch-ms" for the Microsoft hotpatching scheme. I was initially
thinking of proposing "num-patchable-bytes"="<count>" as the
fundamental building block, but the more I thought about it the less I
liked it, since a specific patching scheme has several tightly
integrated constraints of which the number of bytes is just one.

What do you think?

-- Sanjoy

IIRC the motivation is to insert a branch instruction in a thread-safe way?
Isn’t the “two bytes” something that is target specific?

Mehdi Amini wrote:

[Proposed langref entry]

The "patchable-prologue" attribute on a function is a general
mechanism to control the form of a function's prologue in ways that
make it easy to patch at runtime.

Currently only one value is supported:

# "hotpatch-compact"

If a function is marked with "patchable-prologue"="hotpatch-compact"
then:

1. The first instruction of the function is at least two bytes long.

IIRC the motivation is to insert a branch instruction in a thread-safe way?

Yes.

Isn’t the “two bytes” something that is target specific?

Yes, that's a good point. I can think of two ways to fix this:

  - Change the spec to say "large enough to accommodate a short jump instruction"

  - Rename the "hotpatch-compact" scheme to "hotpatch-compact-x86-64" and basically live with the fact that it is a target specific attribute.

I like the first one better, but I can live with either since at this time I really only care about x86_64.

-- Sanjoy

Yeah this is somehow what I had in mind.

This looks great to me. I think the attribute using byte sizes was overly general, but we still want to leave room to implement a few different patching schemes. Other than your scheme, the obvious ones are the MS one and one that leaves space for a long jump in the prologue.

I’m assuming this attribute won’t affect inlining or other IPO in any way, but you should probably mention that in the langref.

Reid Kleckner wrote:

This looks great to me. I think the attribute using byte sizes was
overly general, but we still want to leave room to implement a few
different patching schemes. Other than your scheme, the obvious ones are
the MS one and one that leaves space for a long jump in the prologue.

Agreed.

I'm assuming this attribute won't affect inlining or other IPO in any
way, but you should probably mention that in the langref.

I've built a bad reputation for myself, haven't I? :wink:

-- Sanjoy

Reid Kleckner wrote:

I'm assuming this attribute won't affect inlining or other IPO in any
way, but you should probably mention that in the langref.

To directly answer this, this is just a *mechanism* to implement linkonce_odr type linkage. This in itself does not imply in IPO restrictions, that should come directly from the linkage type.

-- Sanjoy

What do you mean "this is just a *mechanism* to implement linkonce_odr type linkage”?
linkonce_odr are allowed to be inlined, and a client that is interested in “hot patching” a function implementation is probably not expecting this?

Mehdi Amini wrote:
>>
>> Reid Kleckner wrote:
>>> I'm assuming this attribute won't affect inlining or other IPO in any
>>> way, but you should probably mention that in the langref.
>> To directly answer this, this is just a *mechanism* to implement linkonce_odr type linkage. This in itself does not imply in IPO restrictions, that should come directly from the linkage type.
>
> What do you mean "this is just a *mechanism* to implement linkonce_odr type linkage”?
> linkonce_odr are allowed to be inlined, and a client that is interested in “hot patching” a function implementation is probably not expecting this?

What I mean is that "patchable-prologue" allows the client (user
of LLVM) to do "late-linking" in some sense (i.e. do what typically
the linker would have done, but at runtime). But //what// gets linked
to is still determined by the linkage type.

So for a callee if you have (linkonce +
"patchable-patchable"="hotpatch-compact") then you cannot inline since
the runtime may want to replace the body with something totally
different. But if you have (linkonce_odr +
"patchable-patchable"="hotpatch-compact") then you are allowed to
inline since the runtime will replace the callee with something for a
which you have *a* correct implementation. In other words, the mid
level optimizer can ignore "patchable-patchable" for purposes of IPO,
since it does not dictate what will happen, but only *how* it will
happen at a very low level.

Did that make sense?

-- Sanjoy

Two things:

a) I’m not against this
b) So, what’s your use case? I’ve got something I’m idly working on with someone else where we want patchable targets in both prologue and epilogue (and some other places…), and am thinking of how to make this someone generic enough to build off of there.

Thoughts?

-eric

Hi Eric,

Eric Christopher wrote:
> Two things:
>
> a) I'm not against this

Great!

> b) So, what's your use case? I've got something I'm idly working on with
> someone else where we want patchable targets in both prologue and
> epilogue (and some other places...), and am thinking of how to make this
> someone generic enough to build off of there.

We plan to use this to be able to divert control flow from an LLVM
compiled function to "somewhere else" where the "somewhere else" is
usually a differently optimized version of the same function. One
reason to do this is when some speculative optimization we did to the
old version becomes unprofitable, but still remains correct[0]. The
threads executing in that old version are still fine to continue
executing there, but threads that just called into the old version
should instead execute this new version that does not have the same
problematic speculative optimization. To do this we overwrite the
prologue of the old function with a jump to a runtime stub which in
turn figures out what the newly compiled function is and branches to
it.

[0] A real world example: say we turned a null check followed by a
load into just a load, with a fallback in the SIGSEGV handler; and now
we've realized that the value we're null checking this way *is*
sometimes null. Taking a SIGSEGV every time the value is null is
correct, but bad for performance so we compile a new "healed" copy of
the same function that does not fall back to the SIGSEGV handler for
that particular null check (but explicitly tests it instead).

-- Sanjoy

Right. So, I’ve got a use case that I’m working on over here that uses, basically, patchable prologue and epilogue and am hoping that this ends up being general enough for both.

I’ll take a look at the patch since you’ve sent it out, but would really like to not have to change a lot of how it works. :slight_smile:

-eric

Thanks for looping me in Eric!

If I was going to suggest anything here, I’d like to think about a more general approach than a very specific attribute like this. My preference is something like “patchable-function”=“kind,kind,…” (if that’s even possible). This allows us to have common infrastructure for being able to implement different kinds of function-level patching routines at the same time or even just generalising this mechanism with the instrumentation attribute(s).

It would help probably if I’m able to say more, potentially in the form of an RFC for what Eric and I are working on. :smiley:

Cheers

Hi Dean, Eric,

Dean Michael Berris wrote:
> If I was going to suggest anything here, I'd like to think about a more
> general approach than a very specific attribute like this. My preference
> is something like "patchable-function"="kind,kind,..." (if that's even
> possible). This allows us to have common infrastructure for being able
> to implement different kinds of function-level patching routines at the
> same time or even just generalising this mechanism with the
> instrumentation attribute(s).

I've updated D19046 to use "patchable-function", can one of you please
take a look?

Thanks,
-- Sanjoy

Thanks for looping me in Eric!

:slight_smile:

If I was going to suggest anything here, I’d like to think about a more general approach than a very specific attribute like this. My preference is something like “patchable-function”=“kind,kind,…” (if that’s even possible). This allows us to have common infrastructure for being able to implement different kinds of function-level patching routines at the same time or even just generalising this mechanism with the instrumentation attribute(s).

It would help probably if I’m able to say more, potentially in the form of an RFC for what Eric and I are working on. :smiley:

Yeah, we’re almost there right? Sanjoy is jumping the gun on us a bit :slight_smile:

patchable-function is a fairly terrible name, but an enum list of valid kinds seems to work for me. We might end up having to make it an exclusive list rather than a combined list due to people wanting different size patchable areas.

-eric

I think most function redirection patching schemes are going to be mutually
incompatible, so I'm not sure it makes sense to make this attribute a
comma-separated list.

I think Eric's and Dean's use case may be better addressed by a separate
attribute. My recollection is that they want to add nop slides to the
prologue and epilogue that can be hotpatched to enable and disable
instrumentation at runtime.

Isn’t this what I said? :slight_smile:

I’m not sure about this, it’d be nice not to have a bajillion more attributes.

-eric

Isn’t this what I said? :slight_smile:

While some schemes may be incompatible, I suspect composing some of them together makes some sense. For example:

patchable-function=hotpatch-short-prologue,hotpatch-short-epilogue

We can catch some incompatibilities in the implementation if there are serious problems with mixing them.

I’m not sure about this, it’d be nice not to have a bajillion more attributes.

I for one would like less attributes. It seems this is potentially something we can use for a broader purpose so I’d like to explore that possibility early (rather than doing it later).

Cheers