New instrumentation API

[morrisonlevi]

This is a new API that tries to get the benefits of zend_execute_ex without the C stack overflow issue. It allows extensions to register function-specific begin/end handlers when the function is called for the first time in a request. This is intended for use by deterministic profilers, tracers, debuggers, security auditing, etc.

An extension using the API can be found at sammyk/observer.

We plan to do these changes:

• Split the API for userland and internal functions. It's reasonable to instrument only one or the other and the infrastructure itself is already split so it won't be hard to do.
• Move tests from sammyk/observer into the zend_test extension.

Some known limitations:

• The scope for this PR is limited to instrumenting function/method calls, but this can be extended to other areas such as instrumenting exception throw/catch monitoring or error tracking. A good rule of thumb is if dtrace does it then all extensions should also be able to do it.
• Bailouts such as exit or extensions that call zend_bailout will not trigger the function close handler yet. We are hoping exit will be implemented via an exception for PHP 8, but if not we will try handling this another way.
• JIT; we haven't attempted to handle this at all. Feedback on implementation strategy is welcome and we definitely want to support it.

[beberlei]

Another prototype for our XHProf fork using this API: tideways/php-xhprof-extension#96

[morrisonlevi]

The failures are JIT related. A simple script that does var_dump("hello"):

$ phpdbg -p* tmp.php
function name: (null)
L1-5 {main}() /usr/local/src/php-src/tmp.php - 0x7f7463264380 + 4 ops
 L3    #0     INIT_FCALL<1>           96                   "var_dump"
 L3    #1     SEND_VAL                "hello"              1
 L3    #2     DO_ICALL
 L5    #3     RETURN<-1>              1

Running with -d opcache.enable_cli=1 -d opcache.protect_memory=1 -d opcache.jit_buffer_size=16M -d zend_extension=$PWD/modules/opcache.so shows this backtrace:

#0  0x000055d62b81b845 in zend_instrument_call_begin_handlers (execute_data=0x7efde3e14070, cache=0x0)
    at /usr/local/src/php-src/Zend/zend_instrument.h:64
#1  0x000055d62b81d370 in execute_internal (execute_data=0x7efde3e14070, return_value=0x7fff56b7f4b0)
    at /usr/local/src/php-src/Zend/zend_execute.h:49
#2  0x000055d62b82be21 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER () at /usr/local/src/php-src/Zend/zend_vm_execute.h:1235
#3  0x000000004818e5a7 in ?? ()
#4  0x0000000000000000 in ?? ()

And it's pretty simple to see the direct sigsegv cause: cache is NULL and at this point it's expected to always be initialized. I'm fairly certain JIT did something for ZEND_INIT_FCALL and so the VM handler didn't run. It's the opcode handler that does the initialization for the instrumentation cache in this case.

[nikic]

Split the API for userland and internal functions. It's reasonable to instrument only one or the other and the infrastructure itself is already split so it won't be hard to do.

Can you clarify what you have in mind here? Separate callback for whether to intrument and internal/userland function?

[morrisonlevi]

Yes, split the decision code. Today we have execute_ex and execute_internal and I want to match the ability to instrument none, one, or both.

…

On Wed, May 20, 2020, 8:50 AM Nikita Popov ***@***.***> wrote: Split the API for userland and internal functions. It's reasonable to instrument only one or the other and the infrastructure itself is already split so it won't be hard to do. Can you clarify what you have in mind here? Separate callback for whether to intrument and internal/userland function? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5582 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB53BGLMHEWIK5F7FZ3AXTRSPU2LANCNFSM4NBX3RLQ> .

[morrisonlevi]

@dstogov I'm working on the ~24 non-JIT failures. What do you think of the general approach? Everyone I've collaborated with thinks it's okay but we don't have the same expertise in the engine as you (except Nikita).

[dstogov]

@dstogov I'm working on the ~24 non-JIT failures. What do you think of the general approach? Everyone I've collaborated with thinks it's okay but we don't have the same expertise in the engine as you (except Nikita).

I afraid, the overhead of this patch is going to be enormous... :(

I would like to disable ability to override execute_ex and provide simple and efficient solution, but this is not simple.

1. Overriding excute_ex(), third party extensions use C stack to keep some information around the call. Splitting instrumentation into begin and end callbacks won't provide a simple solution to implement elementary logic.

function my_execute_ex() {
  $t = time();
  orig_execute_ex();
  echo time() - $t;
}

2. Performance. The most efficient (but limited) solution would be replacing ability to override execute_ex() with global before_call() and after_call() callbacks. DO_FCALL would check for "before_call" instead of "execute_ex" and call it before ZEND_VM_ENTER. The callback should set some flag in EX_CALL_INFO() to eliminate check for "after_call" on fast path of zend_leave_helper().

[dstogov]

3. It's possible to use zend_get_op_array_extension_handle() to reserve space associated with op_array (also works with persistent op_arrays) and ZEND_OP_ARRAY_EXTENSION() to access the associated data. e.g. for marking "interesting" for before/after callbacks op_arrays. We may even check this data through "if (EX(run_time_cache)[OBSERVER_HANDLE])" in DO_FCALL instead of existence of before/after callbacks.

4. I don't see a big reason in before/after callbacks for internal functions.

[morrisonlevi]

@dstogov Thanks for looking at this.

About performance: when something registers on every function it's on par with zend_execute_ex + zend_execute_internal overhead. So, don't be too worried. Of course that's not the only case we care about, but we do care about performance.

I agree about local stack use being convenient, but this is literally the problem we have to avoid. We cannot use the C stack or it will overflow. This is a hard requirement. If we need to provide a temporary value from begin to end then we can, but it cannot be C stack based.

Why don't you see a reason for before/after callbacks for internal functions? It's the same as user functions: if I only instrument a few of them I should only pay costs for a few, not all of them. As mentioned it we want to split the API so you can instrument only USERLAND or only INTERNAL or BOTH; patch right now does both.

[morrisonlevi]

I don't particularly care about this PR; what I (and other extension authors whom I've collaborated with) care about is an API that:

• Can target specific functions for instrumentation whether internal or userland, and pay a lower CPU cost than instrumenting all functions. This can sort-of be done today with ZEND_OP_ARRAY_EXTENSION on user functions and the old style reserved slot on internal functions combined with zend_execute_ex and zend_execute_internal.
• Can work even in highly nested code without segmentation faults. Cannot easily be done today because of zend_execute_ex overflowing the C stack; requires fcall handlers which are very error prone.
• Makes it trivial to work alongside other extensions doing similar things. Over time we've seen a significant increase in the number of tools in this space and there have been issues.
• Works with opcache, and ideally with JIT. This includes the cases where extensions are changed but opcache doesn't invalidate the cache despite the extensions loaded being changed.

Hopefully you can see how we took these constraints and came up with the design of this PR. As mentioned we still have some things to do, but with my level of expertise in the engine I don't see how we can significantly change the design and achieve these goals.

Global handlers are prone to users not calling previous handlers; see xdebug and the exception hooks, or basically any extension that uses opcode handlers; they ignore that other extensions might be doing the same thing. All it takes is one bad extension and it causes problems for all other extensions. We need to design it differently to make it harder to screw up.

Using zend_execute_ex will cause a segmentation fault with C stack overflow on nested code. Even if we limit the depth of this recursion I don't think we'll be happy with that situation; that means there is code that we cannot instrument, for example to inform the user of probable infinite recursion. Enabling a tool to detect such errors and then having that tool segfault or not work because the limit is too low is a non-starter. Imagine trying to sell that to customers.

---

What do you think? I'm open to an alternative design, but it has to solve these goals (and maybe some others I forgot). I think this PR is decent. It solves the design goals or at least is not too far off. Aside from the already mentioned pieces, we probably still need to reduce the overhead when hooks are not used, though.

[dstogov]

About performance: when something registers on every function it's on par with zend_execute_ex + zend_execute_internal overhead. So, don't be too worried. Of course that's not the only case we care about, but we do care about performance.

I don't care about performance lose when instrumentation is enabled. I do care only about the overhead introduced by the patch even when instrumentation is disabled.

Why don't you see a reason for before/after callbacks for internal functions? It's the same as user functions: if I only instrument a few of them I should only pay costs for a few, not all of them. As mentioned it we want to split the API so you can instrument only USERLAND or only INTERNAL or BOTH; patch right now does both.

The existing API (zend_execute_internal) doesn't suffer from C stack problem.

[dstogov]

What do you think? I'm open to an alternative design, but it has to solve these goals (and maybe some others I forgot). I think this PR is decent. It solves the design goals or at least is not too far off. Aside from the already mentioned pieces, we probably still need to reduce the overhead when hooks are not used, though.

I understand your intention and like it, but I don't think it's practical to introduce a possibility for instrumentation with significant cost for production use. May be introduce special PHP compiler flag, that will use "extended" versions of DO_FCALL instructions with instrumentation. This way, we may chose between performance and instrumentation.

[morrisonlevi]

May be introduce special PHP compiler flag, that will use "extended" versions of DO_FCALL instructions with instrumentation. This way, we may chose between performance and instrumentation.

If we can invalidate opcache (even file_cache) if this setting is changed (bidirectionally), then this is may work. Ideally it would also invalidate if the extensions list changes as there are already bugs there, but that's harder as it's a list of data instead of a single flag. If we can't invalidate opcache if this flag switches then it's not a viable approach, IMO.

We'll prepare some performance numbers, but I hope we can choose performance and instrumentation, not one or the other.

[morrisonlevi]

@dstogov Are there any op_array types that won't let me use the op_array extension? I'm trying to rework this PR to use the run_time_cache but I'm having issues with closures and __call/__callStatic magic. Do these types use the run_time_cache for something else?

[dstogov]

@dstogov Are there any op_array types that won't let me use the op_array extension?

op_array->reserved[] must not be changed for op_arrays with ZEND_ACC_IMMUTABLE flag. run_time_cache and ZEND_OP_ARRAY_EXTENSION() should be used instead.

I'm trying to rework this PR to use the run_time_cache but I'm having issues with closures and __call/__callStatic magic. Do these types use the run_time_cache for something else?

run_time_cache for closures may be duplicated during instantiation, but I don't see what kind of problems this may make.