Modify agent to conform with new PHP 8.0 internal API changes #49

zsistla · 2020-12-09T18:29:22Z

Items addressed:

Added defines for PHP8
Added TSRMLS_* macros (these have been voided out in php-src since PHP7.0, but were officially removed in PHP8.0)
The Object Handlers API was changed to receive zend_object* instead of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not.
The zend_fcall_info no_separation flag has been removed, and separation is never allowed. If you wish to pass (or allow passing) arguments by reference, explicitly create those arguments as references using ZEND_MAKE_REF. This function is passed as a pointer in the agent here. This removal also affects call_user_function_ex() (example in agent), which should be replaced by call_user_function().
Error notifications have a new API and the callback signature has changed. The current changes do not register a callback.
display_disabled_function has been removed from (and therefore zif_display_disabled_function) zend_API.c.. In PHP 8, a disabled function is indistinguishable from a function that does not exist.
Added newrelic_arginfo_void as PHP_FE no longer allows 0 as an argument.
Necessary changes to allow agent tests to compile.
Take into account new libphp.so and libphp.a naming scheme.
With PHP8, call_user_function_ex was removed and call_user_function

became the recommended function; however, the call handles param_values
differently. Prior to PHP8, we used the no_separation flag set to 0 which
then did some internal manipulation of param_values. There is no longer a
no separation flag, so that internal manipulation is no longer done and we
need to do it prior to sending param_values or call_user_function will
cause a segfault.

zend_get_resource_handle now takes a different type parameter.

Remaining items:

Regarding changing call_user_function_ex call to the call_user_function call in the agent function nr_php_call_user_func:
With call_user_function_ex, we used to use the no_separation flag = 0, which meant PHP itself would internally manipulate ours args appropriately.

With call_user_function the logic has changed here:
https://github.com/php/php-src/blob/master/Zend/zend_execute_API.c#L770

Currently any UNIT TEST call to nr_php_call or a direct call to nr_php_call_user_func that has parameters will choke because the internal handling no longer wraps them as proper zval refs.

Before we pass the params to PHP function call_user_function, we probably need to do something like:

if (!Z_ISREF_P(arg)) {
 nrl_verbosedebug(NRL_AGENT, "debug22: adding ref");
 Z_TRY_ADDREF_P(arg);
 ZVAL_NEW_REF(arg, arg);
}

This code could either be placed directly in the nr_php_call_user_func function or be an inline function declared in php_zval.h like:

static inline void nr_php_zval_wrap_out_arg(zval* zv) {
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */
  if (!Z_ISREF_P(zv)) {
    Z_TRY_ADDREF_P(arg);
    ZVAL_NEW_REF(zv, zv);
  }
#endif
}

Additionally, calls to call_user_func_array have changed (as evidenced by this https://www.drupal.org/project/drupal/issues/3174022 discussion), and we still need to ensure any tests and/or wrappers handle it accordingly.
The API internals ticket still has a few allocated days left for someone to pick up.
The unit tests will need to be updated to take into account the changes.

Fixes #46

… of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not.

…on is never allowed. If you wish to pass (or allow passing) arguments by reference, explicitly create those arguments as references using ZEND_MAKE_REF. This function is passed as a pointer in the agent here. This removal also affects call_user_function_ex() (example in agent), which should be replaced by call_user_function().

…nged. The current changes do not register a callback.

…_function`) has been removed `zend_API.c.`. In PHP 8, a disabled function is indistinguishable from a function that does not exist.

…argument.

…he recommended function; however, it no longer gracefully handled the case of the 2nd parameter being passed in as NULL. We no longer assume graceful handling, and now we check for NULL before calling the function.

agent/php_call.c

joshuabenuck · 2020-12-15T19:33:03Z

agent/php_compat.h

 #if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */
 #define PHP7


We updated our usages of PHP7 to use the more complex ifdef in order to make it clear we were referring to PHP version 7.0 or greater. This define doesn't allow for us to target things just to PHP 7 (and not 8 or 5). Should we update this definition to exclude PHP 8?

Yes, if we are sure we've replaced all PHP7 ifdefs.

There are still a bunch of PHP7 ifdefs left. I filed issue #56 to track the need to clean up our ifdef usage so it doesn't block this PR.

agent/php_zval.h

agent/php_newrelic.h

joshuabenuck · 2020-12-16T22:50:43Z

ok jenkins

kneitinger · 2020-12-16T23:58:58Z

.github/actions/ubuntu16-build-action/build.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
 make -j $(nproc) all
-make -j $(nproc) run_tests
+make -j run_tests


Wow, I never knew about -j without any flags 🧠

Makes two of us! That was a typo, but apparently -j without flags is a thing!

Oh I thought it was intentional for maximum test pedal to the metal 😆

kneitinger · 2020-12-17T00:23:13Z

agent/php_agent.h

+#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */
+/* PHP 8 expects zend_object not zval */
+#define ZVAL_OR_ZEND_OBJECT(x) Z_OBJ(*x)
+#else
+#define ZVAL_OR_ZEND_OBJECT(x) x
+#endif /* PHP8+ */


kneitinger · 2020-12-17T00:30:12Z

agent/php_includes.h

+/*
+ * The TSRMLS_* functions included below have actually been voided out since PHP
+ * 7.0.  They were formally removed in PHP 8.0 but we still support in our code,
+ * so we need to add the voided out macros here.


Perfect solution!

agent/php_internal_instrument.c

kneitinger · 2020-12-17T00:44:24Z

agent/php_hash.h

+#ifdef PHP8
+  add_assoc_zval(arr, key, &copy);
+  return 0;


Context in case anyone else was confused by this

php/php-src/pull/5805

…ound definition of `newrelic_arginfo_void` since all usages of this were also wrapped in PHP 8 `if` statements which would then result in an used arg error/warning on some builds.

zsistla · 2020-12-17T20:52:54Z

ok jenkins

joshuabenuck · 2020-12-17T21:38:40Z

ok jenkins

…ic#49) Fixes newrelic#46 * Added defines for PHP8. * Added `TSRMLS` macros. * The Object Handlers API was changed to receive zend_object* instead of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not. * The zend_fcall_info no_separation flag has been removed, and separation is never allowed. If you wish to pass (or allow passing) arguments by reference, explicitly create those arguments as references using ZEND_MAKE_REF. This function is passed as a pointer in the agent here. This removal also affects call_user_function_ex() (example in agent), which should be replaced by call_user_function(). * Error notifications have a new API and the callback signature has changed. The current changes do not register a callback. * `display_disabled_function` (and therefore also `zif_display_disabled_function`) has been removed `zend_API.c.`. In PHP 8, a disabled function is indistinguishable from a function that does not exist. * Added `newrelic_arginfo_void` as `PHP_FE` no longer allows `0` as an argument. * Modified unit tests to work with PHP8. * `call_user_function_ex` was removed and `call_user_function` became the recommended function; however, it no longer gracefully handled the case of the 2nd parameter being passed in as NULL. We no longer assume graceful handling, and now we check for NULL before calling the function. * Take into account new `libphp.so` and `libphp.a` naming scheme. * `zend_get_resource_handle` now takes a different type parameter. * Added `#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */` around definition of `newrelic_arginfo_void` since all usages of this were also wrapped in PHP 8 `if` statements which would then result in an used arg error/warning on some builds. * Update opcode inspection to properly detect calls to `call_user_func_array`. On PHP 8.0, there's an additional opcode added when the function is compiled. Co-authored-by: Joshua Benuck <[email protected]>

Fixes #46 * Added defines for PHP8. * Added `TSRMLS` macros. * The Object Handlers API was changed to receive zend_object* instead of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not. * The zend_fcall_info no_separation flag has been removed, and separation is never allowed. If you wish to pass (or allow passing) arguments by reference, explicitly create those arguments as references using ZEND_MAKE_REF. This function is passed as a pointer in the agent here. This removal also affects call_user_function_ex() (example in agent), which should be replaced by call_user_function(). * Error notifications have a new API and the callback signature has changed. The current changes do not register a callback. * `display_disabled_function` (and therefore also `zif_display_disabled_function`) has been removed `zend_API.c.`. In PHP 8, a disabled function is indistinguishable from a function that does not exist. * Added `newrelic_arginfo_void` as `PHP_FE` no longer allows `0` as an argument. * Modified unit tests to work with PHP8. * `call_user_function_ex` was removed and `call_user_function` became the recommended function; however, it no longer gracefully handled the case of the 2nd parameter being passed in as NULL. We no longer assume graceful handling, and now we check for NULL before calling the function. * Take into account new `libphp.so` and `libphp.a` naming scheme. * `zend_get_resource_handle` now takes a different type parameter. * Added `#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */` around definition of `newrelic_arginfo_void` since all usages of this were also wrapped in PHP 8 `if` statements which would then result in an used arg error/warning on some builds. * Update opcode inspection to properly detect calls to `call_user_func_array`. On PHP 8.0, there's an additional opcode added when the function is compiled. Co-authored-by: Joshua Benuck <[email protected]>

…ic#49) Fixes newrelic#46 * Added defines for PHP8. * Added `TSRMLS` macros. * The Object Handlers API was changed to receive zend_object* instead of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not. * The zend_fcall_info no_separation flag has been removed, and separation is never allowed. If you wish to pass (or allow passing) arguments by reference, explicitly create those arguments as references using ZEND_MAKE_REF. This function is passed as a pointer in the agent here. This removal also affects call_user_function_ex() (example in agent), which should be replaced by call_user_function(). * Error notifications have a new API and the callback signature has changed. The current changes do not register a callback. * `display_disabled_function` (and therefore also `zif_display_disabled_function`) has been removed `zend_API.c.`. In PHP 8, a disabled function is indistinguishable from a function that does not exist. * Added `newrelic_arginfo_void` as `PHP_FE` no longer allows `0` as an argument. * Modified unit tests to work with PHP8. * `call_user_function_ex` was removed and `call_user_function` became the recommended function; however, it no longer gracefully handled the case of the 2nd parameter being passed in as NULL. We no longer assume graceful handling, and now we check for NULL before calling the function. * Take into account new `libphp.so` and `libphp.a` naming scheme. * `zend_get_resource_handle` now takes a different type parameter. * Added `#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */` around definition of `newrelic_arginfo_void` since all usages of this were also wrapped in PHP 8 `if` statements which would then result in an used arg error/warning on some builds. * Update opcode inspection to properly detect calls to `call_user_func_array`. On PHP 8.0, there's an additional opcode added when the function is compiled. Co-authored-by: Joshua Benuck <[email protected]>

zsistla added 11 commits December 4, 2020 10:17

Added defines for PHP8.

c66d3b8

Added TSRMLS macros.

ab008b8

* The Object Handlers API was changed to receive zend_object* instead…

5af5cb9

… of zval* and zend_string* instead of zval* for property names. Modified agent to toggle between zval and zend_object depending on PHP8 or not.

Error notifications have a new API and the callback signature has cha…

470d24f

…nged. The current changes do not register a callback.

display_disabled_function (and therefore also `zif_display_disabled…

c7b6ce5

…_function`) has been removed `zend_API.c.`. In PHP 8, a disabled function is indistinguishable from a function that does not exist.

Added newrelic_arginfo_void as PHP_FE no longer allows 0 as an …

83db843

…argument.

Modified unit tests to work with PHP8.

7074af4

call_user_function_ex was removed and call_user_function became t…

cdb06ae

…he recommended function; however, it no longer gracefully handled the case of the 2nd parameter being passed in as NULL. We no longer assume graceful handling, and now we check for NULL before calling the function.

Take into account new libphp.so and libphp.a naming scheme.

2144870

zend_get_resource_handle now takes a different type parameter.

68e7e19

zsistla added this to the PHP 8 Support milestone Dec 9, 2020

zsistla requested review from joshuabenuck, kneitinger and Fahmy-Mohammed December 9, 2020 18:29

kneitinger mentioned this pull request Dec 10, 2020

[PHP 8] Internals Unit Test Updates #51

Closed

joshuabenuck reviewed Dec 15, 2020

View reviewed changes

agent/php_call.c Outdated Show resolved Hide resolved

joshuabenuck reviewed Dec 15, 2020

View reviewed changes

agent/php_zval.h Outdated Show resolved Hide resolved

zsistla commented Dec 16, 2020

View reviewed changes

agent/php_newrelic.h Show resolved Hide resolved

kneitinger reviewed Dec 16, 2020

View reviewed changes

kneitinger reviewed Dec 17, 2020

View reviewed changes

agent/php_internal_instrument.c Outdated Show resolved Hide resolved

kneitinger reviewed Dec 17, 2020

View reviewed changes

zsistla and others added 4 commits December 17, 2020 10:47

Cleanup for initial PR.

c5e1ae7

PHP 8 related unit test fixes

71023f6

use different static method on PHP 8

48fcb58

remove unused function

fb47ab0

Joshua Benuck and others added 9 commits December 17, 2020 10:47

update comment

49b4546

correct version comparisons

3ed3279

Fix conditional

e556c96

fix endif comments

b599c85

fix error building on php 8.0

bbf95d4

run test serially on ubuntu 16

b8b467b

run tests serially on ubuntu

e664e4e

go back to running the tests in parallel

c2896b2

Small change and updated comment.

126f7fe

joshuabenuck mentioned this pull request Dec 17, 2020

Improve consistency in PHP version ifdef usage #56

Open

Added #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 8.0+ */ ar…

77e55dd

…ound definition of `newrelic_arginfo_void` since all usages of this were also wrapped in PHP 8 `if` statements which would then result in an used arg error/warning on some builds.

joshuabenuck approved these changes Dec 17, 2020

View reviewed changes

joshuabenuck merged commit ed1aec7 into newrelic:main Dec 17, 2020

zsistla added the enhancement New feature or request label Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify agent to conform with new PHP 8.0 internal API changes #49

Modify agent to conform with new PHP 8.0 internal API changes #49

zsistla commented Dec 9, 2020 •

edited by kneitinger

Loading

joshuabenuck Dec 15, 2020

zsistla Dec 16, 2020

joshuabenuck Dec 17, 2020

joshuabenuck commented Dec 16, 2020

kneitinger Dec 16, 2020

joshuabenuck Dec 17, 2020

kneitinger Dec 17, 2020

kneitinger Dec 17, 2020

kneitinger Dec 17, 2020

kneitinger Dec 17, 2020

zsistla commented Dec 17, 2020

joshuabenuck commented Dec 17, 2020

		#if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */
		#define PHP7

Modify agent to conform with new PHP 8.0 internal API changes #49

Modify agent to conform with new PHP 8.0 internal API changes #49

Conversation

zsistla commented Dec 9, 2020 • edited by kneitinger Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuabenuck commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsistla commented Dec 17, 2020

joshuabenuck commented Dec 17, 2020

zsistla commented Dec 9, 2020 •

edited by kneitinger

Loading