-
Notifications
You must be signed in to change notification settings - Fork 322
[ php-wasm ] Add xdebug
shared extension to @php-wasm/node ASYNCIFY
#2326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ php-wasm ] Add xdebug
shared extension to @php-wasm/node ASYNCIFY
#2326
Conversation
c07a975
to
7874d17
Compare
I didn't expect Xdebug asyncify to be implemented so easily. In a comment from the previous pull request, I stated this :
For php: + zend_extension_statement_handler
+ zend_llist_apply_with_argument
+ ZEND_EXT_STMT_SPEC_HANDLER
+ zend_post_deactivate_modules
+ call
+ zend_observer_fcall_begin_prechecked
+ zend_observer_fcall_begin For Xdebug : + zm_post_zend_deactivate_xdebug
+ xdebug_debugger_post_deactivate
+ xdebug_dbgp_deinit
+ xdebug_execute_begin
+ xdebug_execute_user_code_begin
+ xdebug_init_debugger
+ xdebug_debug_init_if_requested_at_startup
+ xdebug_dbgp_init
+ xdebug_execute_ex
+ xdebug_statement_call
+ xdebug_debugger_statement_call
+ xdebug_dbgp_breakpoint
+ xdebug_dbgp_cmdloop
+ xdebug_fd_read_line_delim
But, after I decided to check if all these functions were indeed still needed, I found out Xdebug didn't have to specify + zend_extension_statement_handler
+ ZEND_EXT_STMT_SPEC_HANDLER
+ zend_observer_fcall_begin_prechecked // PHP8.4 only
+ zend_observer_fcall_begin That's good news, but I don't know what changed in the project that made me compile Xdebug in Asyncify mode without the need of I will still try to run the Xdebug tests with the php-wasm binary in a separate project first, and share my findings here. Ah! FYI, Xdebug Asyncify tests passed successfully |
Oh, how lovely! Should we mark this as ready for review, then?
Asyncify detects all the functions it needs to instrument. For core PHP, it is a must. The autodetection misses some important functions and also instruments a lot of unimportant functions, which produces a much larger (+10MB AFAIR) and slower (~5s to startup) binary. That's why we use the leaner way. For XDebug, however, we're dealing with a much smaller codebase. If the difference in size and speed is not overly noticeable, relying on autodetection should be fine. See also other Asyncify build options, maybe something will pick your interest. |
Thank you for your highlights. Indeed we need
I will add this in this issue. |
|
Unfortunately the error occurs also in JSPI mode. The error occurs with this script : import { PHP } from '@php-wasm/universal';
import { loadNodeRuntime } from '@php-wasm/node';
const script = `<?php
$ch = curl_init( "https://github.com/xdebug/xdebug/archive/refs/tags/3.4.4.tar.gz" );
$fp = fopen( "xdebug.tar.gz", 'wb' );
curl_setopt( $ch, CURLOPT_FILE, $fp );
curl_setopt( $ch, CURLOPT_FOLLOWLOCATION, true );
curl_setopt( $ch, CURLOPT_SSL_VERIFYPEER, false ); // Use with caution
curl_exec( $ch );
if( curl_errno( $ch ) )
{
echo "Curl error: " . curl_error( $ch );
}
curl_close( $ch );
fclose( $fp );
echo "Download complete\n";
$phar = new PharData( "xdebug.tar.gz" );
$phar->extractTo( '/' );
echo "Extraction complete\n";
unlink( "xdebug.tar.gz" );
echo "Archive deletion complete\n";
`;
const php = new PHP( await loadNodeRuntime( '8.4' ) );
php.chdir( '/' );
const result = await php.runStream( { code : script } );
await result.exitCode;
console.log( php.listFiles( 'xdebug-3.4.4' ) );
const response = await php.cli( [ 'php', 'xdebug-3.4.4/run-xdebug-tests.php' ] );
response.stderr.pipeTo( new WritableStream( { write( chunk ){ process.stderr.write( chunk ) } } ) );
response.stdout.pipeTo( new WritableStream( { write( chunk ){ process.stdout.write( chunk ) } } ) ); I think this is probably related to the way I am switching from I decided to upload manually the Xdebug repository and ony run the CLI and tada :
Tests are all failing by the way. For the moment. For the record, Xdebug is enabled in PHP :
|
Was there a point where it worked? Like before when the asyncify xdebug tests have passed? If yes, let's just use that. And if this is a new crash and also happened then and happens now in trunk then yes, let's debug it. Perhaps that doesn't have to block this PR, though. |
Yeah, you can't do that. Once either of these was called, the php instance is no longer good for the other one. That could be the source of the error you're seeing. |
Ok! I focus on the ASYNCIFY optimization and let's mark this as ready for review. |
I tried some combinations with :
where I initially stored every functions that
I guess no optimization is necessary here. FYI, the |
I am about to recompile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have one comment, otherwise it looks good!
I will add an |
Motivation for the change
This is a pull request to dynamically load Xdebug in
@php-wasm
Node ASYNCIFY.Roadmap
Related issues and pull requests
Relative to XDebug
xdebug
dynamic extension to @php-wasm/node JSPI #2248Relative to dynamic library
Implementation details
Additional steps for asyncify are implemented :
WITH_JSPI
andWITH_DEBUG
arguments in Xdebug Dockerfilexdebug.so
files are compiled and dynamically loadedZend extensions and PHP versions
Important
Each version of PHP requires its own version of Xdebug because each version of PHP has a specific Zend Engine API version. Consequently, each version of Xdebug is designed to be compatible with a particular Zend Engine API version.
Example using PHP
8.2
with Xdebug8.3
:Testing Instructions
Two files are needed. A PHP file to test the step debugger. A JS file to run PHP 8.4 node ASYNCIFY.
php/xdebug.php
scripts/node.js
To achieve the test, you first need to start debugging [ with F5 or Run > Start debugging in VSCode ], then run :