[opentelemetry-php-instrumentation] PHP 8.4 compatibility (10 failed tests)

[remicollet]

Using extension 1.1.0beta2 and PHP 8.4.0beta4

(build and test suite OK with 8.0 to 8.3)

=====================================================================
TIME START 2024-09-04 13:00:24
=====================================================================
PASS Check if opentelemetry extension is loaded [tests/001.phpt] 
PASS Check if hook returns true [tests/002.phpt] 
PASS Check if hooks are invoked [tests/003.phpt] 
PASS Check if multiple hooks are invoked [tests/004.phpt] 
PASS Check if hooks receives function information [tests/005.phpt] 
PASS Check if hooks receives arguments and return value [tests/006.phpt] 
PASS Check if post hook receives exception [tests/007.phpt] 
PASS Check if pre hook can replace arguments [tests/008.phpt] 
PASS Check if pre hook can modify not provided arguments [tests/009.phpt] 
PASS Check if post hook can modify return value [tests/010.phpt] 
PASS Check if calling die or exit will finish gracefully [tests/calling_die_lead_to_segfault.phpt] 
PASS Check if opentelemetry disable ignores bad input [tests/disable_does_nothing_with_bad_data.phpt] 
PASS Disabling hook validation [tests/disable_hook_function_validation.phpt] 
PASS Check if opentelemetry extension is loaded but disabled with a conflicting extension [tests/disabled_with_conflicting_extension.phpt] 
PASS Check if pre hook can expand params of function if they are part of function definition [tests/expand_params.phpt] 
PASS Check if pre hook can expand params of function when that requires extending the stack [tests/expand_params_extend.phpt] 
TEST 17/67 [tests/expand_params_extend_internal.phpt]
========DIFF========
001- array(1) {
002-   [1]=>
003-   int(2)
004- }
001+ Termsig=11
========DONE========
FAIL Check if pre hook can expand params of internal function when that requires extending the stack [tests/expand_params_extend_internal.phpt] 
TEST 18/67 [tests/expand_params_extend_internal_long.phpt]
========DIFF========
001- Fatal error: Uncaught ArgumentCountError: array_slice() expects at most 4 arguments, 13 given in %s
002- Stack trace:
003- #0 %s: array_slice(Array, 1, 1, true, true, true, true, true, true, true, true, true, true)
004- #1 {main}
005-   thrown in %s on line %d
001+ Termsig=11
========DONE========
FAIL Check if pre hook can expand params of internal function when that requires extending the stack (many params) [tests/expand_params_extend_internal_long.phpt] 
PASS Check if pre hook can expand params of function when that requires extending the stack only until hardcoded limit [tests/expand_params_extend_limit.phpt] 
PASS Check if pre hook can expand params of function with extra parameters not provided by call site [tests/expand_params_extra.phpt] 
TEST 21/67 [tests/expand_params_internal.phpt]
========DIFF========
001- Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2 - stack extension must be enabled with opentelemetry.allow_stack_extension option, class=null function=array_slice in %s
002- array(2) {
003-   [0]=>
004-   int(2)
005-   [1]=>
006-   int(3)
007- }
001+ Termsig=11
========DONE========
FAIL Check if pre hook can expand params of internal function [tests/expand_params_internal.phpt] 
PASS Check if hooks are invoked for closures [tests/function_closure.phpt] 
PASS Check if hooks are invoked for first class callables [tests/function_first_class_callable.phpt] 
TEST 24/67 [tests/function_internal.phpt]
========DIFF========
001- string(3) "PRE"
002- string(5) "HELLO"
003- string(4) "POST"
001+ Termsig=11
========DONE========
FAIL Check if hooks are invoked for internal functions [tests/function_internal.phpt] 
PASS Check if exceptions thrown in hooks are isolated and logged [tests/hooks_throw_exception.phpt] 
PASS Check if exceptions thrown in hooks work if custom error handler throws [tests/hooks_throw_exception_error_handler.phpt] 
PASS Check if exceptions thrown in hooks interfere with internal exceptions [tests/hooks_throw_exception_for_failed_call.phpt] 
PASS Check if process exits gracefully after using ini_set with an opentelemetry option [tests/ini_set.phpt] 
PASS Test invalid post callback signature [tests/invalid_post_callback_signature.phpt] 
PASS Test invalid pre callback signature [tests/invalid_pre_callback_signature.phpt] 
PASS Test invalid pre callback signature for function without class [tests/invalid_pre_callback_signature_with_function.phpt] 
PASS Check if pre hook can modify extra parameters [tests/modify_extra_params.phpt] 
TEST 33/67 [tests/modify_extra_params_internal.phpt]
========DIFF========
001- string(50) "array_slice() expects at most 4 arguments, 5 given"
001+ Termsig=11
========DONE========
FAIL Check if pre hook trying to modify extra params of internal functions crashes [tests/modify_extra_params_internal.phpt] 
PASS Check if pre hook can modify named params of function [tests/modify_named_params.phpt] 
TEST 35/67 [tests/modify_named_params_internal.phpt]
========DIFF========
001- array(1) {
002-   [0]=>
003-   int(2)
004- }
001+ Termsig=11
========DONE========
FAIL Check if pre hook can modify named params of internal function [tests/modify_named_params_internal.phpt] 
PASS Check if pre hook can try to modify invalid named params of function [tests/modify_named_params_invalid.phpt] 
PASS Check if pre hook can modify same param via name and index at once [tests/modify_named_params_twice.phpt] 
PASS Check if pre hook can modify params of function [tests/modify_params.phpt] 
PASS Check if hooks receive modified arguments [tests/multiple_hooks_modify_params.phpt] 
PASS Check if hooks receive modified return value [tests/multiple_hooks_modify_returnvalue.phpt] 
PASS Check if post hook return value is ignored if return typehint not supplied [tests/post_hook_return_ignored_without_type.phpt] 
TEST 42/67 [tests/post_hook_returns_cloned_modified_object.phpt]
========DIFF========
001+ Deprecated: Foo::__construct(): Implicitly marking parameter $a as nullable is deprecated, the explicit nullable type must be used instead in /dev/shm/BUILD/php84-php-pecl-opentelemetry-1.1.0~beta2/opentelemetry-1.1.0beta2/tests/post_hook_returns_cloned_modified_object.php on line 5
     object(Foo)#%d (1) {
       ["a"]=>
       string(1) "b"
--
========DONE========
FAIL Check if post hook can returned modified clone
----DESCRIPTION--
A different object might be returned than the one provided to post hook. For example, PSR-7 messages are immutable and modifying
one creates a new instance. [tests/post_hook_returns_cloned_modified_object.phpt] 
PASS Check if throwing an exception in post hook after IO operation will finish gracefully [tests/post_hook_throws_exception.phpt] 
PASS Check if type error in post hook is handled [tests/post_hook_type_error.phpt] 
PASS Check if hooks are invoked only once for reimplemented interfaces [tests/reimplemented_interface.phpt] 
PASS Check if pre hook can expand then return $params [tests/return_expanded_params.phpt] 
TEST 47/67 [tests/return_expanded_params_internal.phpt]
========DIFF========
001- Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2 - stack extension must be enabled with opentelemetry.allow_stack_extension option, class=null function=array_slice in %s
002- array(2) {
003-   [0]=>
004-   string(1) "b"
005-   [1]=>
006-   string(1) "c"
007- }
001+ Termsig=11
========DONE========
FAIL Check if pre hook can expand and then return $params of internal function [tests/return_expanded_params_internal.phpt] 
PASS Check if pre hook can return $params [tests/return_params.phpt] 
TEST 49/67 [tests/return_params_internal.phpt]
========DIFF========
001- string(5) "HELLO"
001+ Termsig=11
========DONE========
FAIL Check if pre hook can return $params for internal function [tests/return_params_internal.phpt] 
PASS Check if pre hook can reduce then return $params [tests/return_reduced_params.phpt] 
PASS Check if SpanAttribute can be applied to interface [tests/span_attribute/attribute_on_interface.phpt] 
PASS Check if function params can be passed via SpanAttribute [tests/span_attribute/function_params.phpt] 
TEST 53/67 [tests/span_attribute/function_params_non_simple.phpt]
========DIFF========
--
       object(stdClass)#1 (0) {
       }
       ["three"]=>
012-   object(Closure)#2 (0) {
012+   object(Closure)#2 (3) {
013+     ["name"]=>
014+     string(145) "{closure:/dev/shm/BUILD/php84-php-pecl-opentelemetry-1.1.0~beta2/opentelemetry-1.1.0beta2/tests/span_attribute/function_params_non_simple.php:24}"
015+     ["file"]=>
016+     string(132) "/dev/shm/BUILD/php84-php-pecl-opentelemetry-1.1.0~beta2/opentelemetry-1.1.0beta2/tests/span_attribute/function_params_non_simple.php"
017+     ["line"]=>
018+     int(24)
       }
       ["four"]=>
       NULL
--
========DONE========
FAIL Check if function non-simple types can be passed as function params [tests/span_attribute/function_params_non_simple.phpt] 
PASS Check if method params can be passed via SpanAttribute [tests/span_attribute/method_params.phpt] 
PASS Check if attributes from SpanAttribute replace attributes with same name from WithSpan [tests/span_attribute/span_attribute_attribute_priority.phpt] 
PASS Check hooking static class methods provides class name as 1st param [tests/static_method.phpt] 
PASS Test UnwindExit from die/exit is not exposed to userland code [tests/unwind_exit.phpt] 
PASS Check if named attribute parameters are passed to pre hook [tests/with_span/attribute_named_params_passed_to_pre_hook.phpt] 
PASS Check if custom attribute loaded [tests/with_span/attribute_on_function.phpt] 
PASS Check if custom attribute can be applied to an interface [tests/with_span/attribute_on_interface.phpt] 
PASS Check if WithSpan can be applied to an interface with attribute args [tests/with_span/attribute_on_interface_with_params.phpt] 
PASS Check if WithSpan can be applied to a method [tests/with_span/attribute_on_method.phpt] 
PASS Check if WithSpan parameters are passed to pre hook [tests/with_span/attribute_params_passed_to_pre_hook.phpt] 
PASS Check if hooking a method takes priority over WithSpan [tests/with_span/attribute_params_skipped_if_hooked.phpt] 
PASS Check if WithSpan handlers can be changed via config [tests/with_span/customize_handlers.phpt] 
PASS Check if attribute hooks can be disabled by config [tests/with_span/disable.phpt] 
PASS Invalid callback is ignored [tests/with_span/invalid_callback.phpt] 
=====================================================================
TIME END 2024-09-04 13:00:25

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :     0
Exts tested     :    64
---------------------------------------------------------------------

Number of tests :    67                67
Tests skipped   :     0 (  0.0%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :    10 ( 14.9%) ( 14.9%)
Tests passed    :    57 ( 85.1%) ( 85.1%)
---------------------------------------------------------------------
Time taken      : 1.408 seconds
=====================================================================

[brettmc]

@remicollet does the latest stable (1.0.3) version of the extension also fail on 8.4 ?
edit: verified locally that 1.0.3 (latest stable) is also broken against 8.4.0beta4 (and nightly/master)

[brettmc]

@agoallikmaa aside from an obvious "implicit nullable" deprecation, the failing tests on 8.4 are all related to observing internal functions, specifically this line is the last point in our code before segfault: https://github.com/open-telemetry/opentelemetry-php-instrumentation/blob/main/ext/otel_observer.c#L612
I have a branch in my fork which I've set up with a php-nightly image where I've been able to reproduce and also look around with gdb

update: when we add a pre or post hook that is a Callable, we add it to a zend_llist. If I immediately grab it back from the linked list and zend_print_zval_r, it seems fine. But, when we grab it from the linked list later (it's been stored in an op array extension), attempting to zend_print_zval_r will segfault. Also calling Z_TYPE_P() on it will segfault - looking at the zval, it seems to have lost its type info (u1.v.type)...it's possibly pointing to a different piece of memory.

[andypost]

Looks this changes are related to php/php-src@a22a872

[brettmc]

Looks this changes are related to

I think it might be this: php/php-src@62ebe82 specifically this comment relates to something that we do (and getting an internal function back from op_array_extension is part of the problem):

zend_get_internal_function_extension_handles() must now be used over
zend_get_op_array_extension_handles() when registering run_time_cache slots
for internal functions. If you need a cache slot for both internal and user
functions, you may obtain a slot for each through the corresponding function.