Hi
On 8/7/24 22:33, Theodore Brown wrote:
I'm confused by this, since earlier in the thread Tim responded with examples
showing how the behavior of exit() can still be observed and correctly handled.
If that didn't address your issue, can you explain it further? It seems like
right now everyone is left bewildered about what functionality you couldn't
replicate.
Tideways generously sponsored 2 hours of my time to develop a proof of
concept patch to fix Xdebug against the current version of Gina's PR.
Much of that time was spent chasing a reference counting issue in an
unfamiliar codebase. Unfortunately ASAN was not of much help, because
running Xdebug's tests against PHP 8.4 reported some pre-existing memory
leaks.
I have attached two patches for Xdebug:
0001-Support-exit-as-function.patch:
This patch fixes the build of Xdebug, by removing the references to
ZEND_EXIT for PHP 8.4.
It also adds a hook on the new exit() function to "deinitialize" Xdebug,
which will ensure that profiling files are written for the use in CI. My
understanding is that an alternative solution would have been to add a
special `xdebug_finalize_profile()` function for use in CI instead of
overloading the exit() function.
All the existing tests in Xdebug's codebase pass after this patch,
except that I did not adjust the profiler test expectations to take the
the new exit() function call that was not previously there into account,
because I do not fully understand the output format of the tests.
Nevertheless the changes to the test output looked correct to me.
0002-Exit-coverage.patch:
This patch adds support for the coverage analysis, so that Xdebug
understands that any code after a call to exit() is unreachable. As far
as I can tell this case was not previously tested by any of Xdebug's
tests. I have thus added a test case, but I don't fully understand the
output format of the coverage tests, thus I may have overlooked
something. What I did verify is that the test fails when I remove the
implementation from code_coverage.c.
I'd like to note that this patch is not strictly necessary for Xdebug to
be usable by a user. It might erroneously report unreachable code as
reachable, but by definition unreachable code is useless. Thus a user of
Xdebug could work around this by simply removing the unreachable code.
My understanding is that some userland static analyzers and refactoring
tools are already capable of pointing out such unreachable code.
--------
Given my unfamiliarity of Xdebug's codebase and the limited time I spent
preparing these patches, they should be considered a proof of concept
only. They might or might not match the preferred coding style used in
the Xdebug codebase. It might also be possible that they did not
correctly handle an edge case, due to my lack of knowledge about the
test infrastructure.
Nevertheless I believe that someone more familiar with the codebase will
certainly be able to adapt these patches as necessary to make Xdebug
fully compatible with Gina's PR.
In any case I believe they clearly showcase that it is not impossible to
keep the existing functionality, even with Gina's RFC.
Best regards
Tim Düsterhus
From 2cd489fcbda122861e5b31065a2120777cdb046e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <t...@tideways-gmbh.com>
Date: Thu, 8 Aug 2024 16:03:58 +0200
Subject: [PATCH 1/2] Support exit() as function
---
src/base/base.c | 20 ++++++++++++++++++++
src/coverage/code_coverage.c | 4 ++++
src/profiler/profiler.c | 12 ++++++++++++
src/profiler/profiler.h | 1 +
4 files changed, 37 insertions(+)
diff --git a/src/base/base.c b/src/base/base.c
index 92cff19b..356b0504 100644
--- a/src/base/base.c
+++ b/src/base/base.c
@@ -55,6 +55,7 @@ zif_handler orig_error_reporting_func = NULL;
zif_handler orig_set_time_limit_func = NULL;
zif_handler orig_pcntl_exec_func = NULL;
zif_handler orig_pcntl_fork_func = NULL;
+zif_handler orig_exit_func = NULL;
#if PHP_VERSION_ID >= 80100
void (*xdebug_old_error_cb)(int type, zend_string *error_filename, const uint32_t error_lineno, zend_string *message);
@@ -81,6 +82,7 @@ PHP_FUNCTION(xdebug_set_time_limit);
PHP_FUNCTION(xdebug_error_reporting);
PHP_FUNCTION(xdebug_pcntl_exec);
PHP_FUNCTION(xdebug_pcntl_fork);
+PHP_FUNCTION(xdebug_exit);
/* {{{ zend_op_array xdebug_compile_file (file_handle, type)
@@ -1100,6 +1102,13 @@ static void xdebug_base_overloaded_functions_setup(void)
orig_pcntl_fork_func = orig->internal_function.handler;
orig->internal_function.handler = zif_xdebug_pcntl_fork;
}
+
+ /* Override exit with our own function to be able to write profiling summary */
+ orig = zend_hash_str_find_ptr(CG(function_table), "exit", sizeof("exit") - 1);
+ if (orig) {
+ orig_exit_func = orig->internal_function.handler;
+ orig->internal_function.handler = zif_xdebug_exit;
+ }
}
static int xdebug_closure_serialize_deny_wrapper(zval *object, unsigned char **buffer, size_t *buf_len, zend_serialize_data *data)
@@ -1603,6 +1612,17 @@ PHP_FUNCTION(xdebug_pcntl_exec)
}
/* }}} */
+/* {{{ proto void xdebug_exit(void)
+ Dummy function to stop profiling when we run exit */
+PHP_FUNCTION(xdebug_exit)
+{
+ orig_exit_func(INTERNAL_FUNCTION_PARAM_PASSTHRU);
+
+ /* We need to stop the profiler and trace files here */
+ xdebug_profiler_exit_function_handler();
+}
+/* }}} */
+
/* {{{ proto int xdebug_pcntl_fork(void)
Dummy function to set a new connection when forking a process */
PHP_FUNCTION(xdebug_pcntl_fork)
diff --git a/src/coverage/code_coverage.c b/src/coverage/code_coverage.c
index a84f2322..fbf9f48d 100644
--- a/src/coverage/code_coverage.c
+++ b/src/coverage/code_coverage.c
@@ -357,7 +357,9 @@ static int xdebug_find_jumps(zend_op_array *opa, unsigned int position, size_t *
} else if (
opcode.opcode == ZEND_GENERATOR_RETURN ||
+#if PHP_VERSION_ID < 80400
opcode.opcode == ZEND_EXIT ||
+#endif
opcode.opcode == ZEND_THROW ||
opcode.opcode == ZEND_MATCH_ERROR ||
opcode.opcode == ZEND_RETURN
@@ -448,6 +450,7 @@ static void xdebug_analyse_branch(zend_op_array *opa, unsigned int position, xde
break;
}
+#if PHP_VERSION_ID < 80400
/* See if we have an exit instruction */
if (opa->opcodes[position].opcode == ZEND_EXIT) {
/* fprintf(stderr, "X* Return found\n"); */
@@ -457,6 +460,7 @@ static void xdebug_analyse_branch(zend_op_array *opa, unsigned int position, xde
}
break;
}
+#endif
/* See if we have a return instruction */
if (
opa->opcodes[position].opcode == ZEND_RETURN
diff --git a/src/profiler/profiler.c b/src/profiler/profiler.c
index 1926e478..31003f97 100644
--- a/src/profiler/profiler.c
+++ b/src/profiler/profiler.c
@@ -22,6 +22,7 @@
#include "TSRM.h"
#include "php_globals.h"
#include "Zend/zend_alloc.h"
+#include "Zend/zend_exceptions.h"
#include "php_xdebug.h"
#include "profiler.h"
@@ -44,8 +45,10 @@ void xdebug_init_profiler_globals(xdebug_profiler_globals_t *xg)
void xdebug_profiler_minit(void)
{
+#if PHP_VERSION_ID < 80400
/* Overload the "exit" opcode */
xdebug_set_opcode_handler(ZEND_EXIT, xdebug_profiler_exit_handler);
+#endif
}
void xdebug_profiler_mshutdown(void)
@@ -82,6 +85,15 @@ void xdebug_profiler_pcntl_exec_handler(void)
deinit_if_active();
}
+void xdebug_profiler_exit_function_handler(void)
+{
+ function_stack_entry *fse = XDEBUG_VECTOR_TAIL(XG_BASE(stack));
+
+ deinit_if_active();
+
+ xdebug_profiler_free_function_details(fse);
+}
+
int xdebug_profiler_exit_handler(XDEBUG_OPCODE_HANDLER_ARGS)
{
const zend_op *cur_opcode = execute_data->opline;
diff --git a/src/profiler/profiler.h b/src/profiler/profiler.h
index 61f6d493..fd979044 100644
--- a/src/profiler/profiler.h
+++ b/src/profiler/profiler.h
@@ -47,6 +47,7 @@ void xdebug_profiler_rinit(void);
void xdebug_profiler_post_deactivate(void);
void xdebug_profiler_pcntl_exec_handler(void);
+void xdebug_profiler_exit_function_handler(void);
void xdebug_profiler_init_if_requested(zend_op_array *op_array);
void xdebug_profiler_execute_ex(function_stack_entry *fse, zend_op_array *op_array);
--
2.40.1
From d3560b7169a828919835b99a07c267fed297f6eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <t...@tideways-gmbh.com>
Date: Thu, 8 Aug 2024 16:54:14 +0200
Subject: [PATCH 2/2] Exit coverage
---
src/coverage/code_coverage.c | 40 +++++++++++++++++++++++++++++
tests/coverage/coverage_exit.inc | 12 +++++++++
tests/coverage/coverage_exit.phpt | 42 +++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+)
create mode 100644 tests/coverage/coverage_exit.inc
create mode 100644 tests/coverage/coverage_exit.phpt
diff --git a/src/coverage/code_coverage.c b/src/coverage/code_coverage.c
index fbf9f48d..4ec29660 100644
--- a/src/coverage/code_coverage.c
+++ b/src/coverage/code_coverage.c
@@ -367,6 +367,46 @@ static int xdebug_find_jumps(zend_op_array *opa, unsigned int position, size_t *
jumps[0] = XDEBUG_JMP_EXIT;
*jump_count = 1;
return 1;
+ } else if (
+ opcode.opcode == ZEND_INIT_FCALL
+ ) {
+ zval *func_name = RT_CONSTANT(&opa->opcodes[position], opcode.op2);
+ if (zend_string_equals_literal(Z_PTR_P(func_name), "exit")) {
+ int level = 0;
+ uint32_t start = position + 1;
+
+ for (;;) {
+ switch (opa->opcodes[start].opcode) {
+ case ZEND_INIT_FCALL:
+ case ZEND_INIT_FCALL_BY_NAME:
+ case ZEND_INIT_NS_FCALL_BY_NAME:
+ case ZEND_INIT_DYNAMIC_CALL:
+ case ZEND_INIT_USER_CALL:
+ case ZEND_INIT_METHOD_CALL:
+ case ZEND_INIT_STATIC_METHOD_CALL:
+ case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL:
+ case ZEND_NEW:
+ level++;
+ break;
+ case ZEND_DO_FCALL:
+ case ZEND_DO_FCALL_BY_NAME:
+ case ZEND_DO_ICALL:
+ case ZEND_DO_UCALL:
+ if (level == 0) {
+ goto done;
+ }
+ level--;
+ break;
+ }
+ start++;
+ }
+ done:
+ ZEND_ASSERT(opa->opcodes[start].opcode == ZEND_DO_ICALL);
+ jumps[0] = XDEBUG_JMP_EXIT;
+ *jump_count = 1;
+ return 1;
+ }
+
} else if (
opcode.opcode == ZEND_MATCH ||
opcode.opcode == ZEND_SWITCH_LONG ||
diff --git a/tests/coverage/coverage_exit.inc b/tests/coverage/coverage_exit.inc
new file mode 100644
index 00000000..3b0d5f33
--- /dev/null
+++ b/tests/coverage/coverage_exit.inc
@@ -0,0 +1,12 @@
+<?php
+function exit_test()
+{
+ if (random_int(0, 1) > 2) {
+ exit;
+
+ echo "foo";
+ }
+}
+
+exit_test();
+?>
diff --git a/tests/coverage/coverage_exit.phpt b/tests/coverage/coverage_exit.phpt
new file mode 100644
index 00000000..354944ad
--- /dev/null
+++ b/tests/coverage/coverage_exit.phpt
@@ -0,0 +1,42 @@
+--TEST--
+Dummy
+--SKIPIF--
+<?php
+require __DIR__ . '/../utils.inc';
+check_reqs('!opcache');
+?>
+--INI--
+xdebug.mode=coverage
+xdebug.trace_options=0
+xdebug.collect_return=0
+xdebug.collect_assignments=0
+xdebug.auto_profile=0
+xdebug.dump_globals=0
+xdebug.trace_format=0
+--FILE--
+<?php
+include 'dump-branch-coverage.inc';
+
+xdebug_start_code_coverage(XDEBUG_CC_UNUSED | XDEBUG_CC_DEAD_CODE | XDEBUG_CC_BRANCH_CHECK);
+
+include 'coverage_exit.inc';
+
+xdebug_stop_code_coverage(false);
+$c = xdebug_get_code_coverage();
+dump_branch_coverage($c);
+?>
+--EXPECT--
+exit_test
+- branches
+ - 00; OP: 00-06; line: 04-04 HIT; out1: 07 X ; out2: 12 HIT
+ - 07; OP: 07-08; line: 05-05 X ; out1: EX X
+ - 12; OP: 12-13; line: 09-09 HIT; out1: EX X
+- paths
+ - 0 7: X
+ - 0 12: HIT
+
+{main}
+- branches
+ - 00; OP: 00-03; line: 11-13 HIT; out1: EX X
+- paths
+ - 0: HIT
--
2.40.1
