[opentelemetry-php-instrumentation] - Allow adding parameters even if stack frame needs to be extended

[agoallikmaa]

There is a case for both internal functions and PHP functions where it may be desirable to add parameters in a way that would require extending the call stack.

Related issue: #1155

PHP functions

For PHP functions, stack frame does not need to be extended if replacing parameters that were already provided at call site (even if those weren't part of function definition), or adding parameters that were part of function definition but not provided at call site. Effectively, index < max(defined_params, callsite_params).

An example where the above condition is not met is when for example we have:

• Definition: function demo($a, $b)
• Callsite: demo(1, 2, 3, 4)
• Begin hook: return [1, 2, 3, 4, 5, 6]

In this case, memory layout is (P = defined param slot (always reserved), T = auxiliary slots, E = extra param):

• Initial: P0 P1 T0 ... TN E0 E1
• Desired: P0 P1 T0 ... TN E0 E1 E2 E3

Internal functions

For internal functions, there are no reserved slots on the call frame, thus in every case where new parameters are added (compared to call site), stack frame would need to be extended.

An example of this would be the following:

• Definition: function internal($a, $b, $c, $d)
• Callsite: internal(1, 2)
• Begin hook: return [1, 2, 3, 4]

In this case, memory layout is (P = param slot, T = auxiliary slots):

• Initial: P0 P1 T0 ... TN
• Desired: P0 P1 P2 P3 T0 ... TN

Extending stack frame

PHP Zend engine features a growable stack. This is done by allocating a new stack page and keeping a link between different pages - pages do not need to be contiguous in memory, nor is the entire old stack copied to new "grown" one. Instead, a linked list of stack pages is maintained. A common size for stack page is 16K "slots", where an average call frame for a function with 4 parameters would use ~16 slots. As such, many applications never need more than 1 page.

The problematic case with extending an already allocated stack frame is when the stack frame currently fits within the current page, but would not fit in there if extended. The built-in function to extend stack frame allocates a new page and moves the current stack frame entirely into that page instead. However, it is only safe to call this function if no references remain to the old location of the frame. This is not the case within observers, because the functions inside Zend that called the observer keep references to the stack frame in their local variables and continue using those once observer returns.

Therefore extending the frame can probably be done in a case where there is enough space left in the current stack page, but there seems to be no reliable method to do it if there isn't. This presents a somewhat unexpected corner case which would be explained as "instrumentation might not work correctly with deep stacks" which may lead to hard to debug situations when it does happen.

This leads to several questions. Should a feature that is inherently "unreliable" be enabled by default, or unlocked with a configuration option? How should it be logged if this corner case occurs that the user would actually understand the issue? If we are trying to add 4 more parameters, but only 2 would fit, should it be done in a "best effort" manner and 2 are added, or "all or nothing" and none are?

Lastly, I should specify that the hypothesis that extending does not present any challenges other than this corner case is not certain. Further experimentation may reveal other blocking issues.

[brettmc]

If I understood correctly, and this can work (sometimes but not always) to enable adding an extra parameter to a built-in, then we should try it out, disabled by default and enabled via .ini
If we can predict in code that a new page would be required, perhaps the error message includes a link to this issue, or some other documentation that we control.
The use-case we currently have is to add one extra parameter, so I'd argue for the "best effort" approach - maybe that also could be configured via .ini

Releasing the feature behind a configuration (with the caveat that it's experimental) is I think a good way to get some field testing and gain some more bug reports and/or confidence.