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
Attachment: [text/x-patch] 0001-Support-exit-as-function.patch
Attachment: [text/x-patch] 0002-Exit-coverage.patch