-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
[RFC] Add support for building to the wasm32-wasi target #10457
Conversation
This adds support for building In order to build in your own environment, you can use As an improvement, we can update the BuildingYou can build either by using Build with
|
Further context: we have successfully run wordpress in the browser, and also in the server side with Apache and mod_wasm. |
b726faa
to
a94519d
Compare
Please add a CI workflow for WASM, see https://github.com/php/php-src/blob/master/.github/workflows/push.yml |
a94519d
to
de67bf7
Compare
de67bf7
to
000954b
Compare
Thanks @mvorisek. Updated PR. Current result as of now:
Can somebody allow GH actions to execute on this PR? |
000954b
to
d2fada3
Compare
8ce4db8
to
e033a5a
Compare
38b96cf
to
4584dc2
Compare
Thanks a lot for the reviews. I have updated the PR. Let me know if by the compat layer you meant something specifically different than what this patch provides. I looked at the win32 compat layer, but I also see many ifdefs here and there on the source code, so I tried to update the patch from scratch in a similar form, while trying not to be very intrusive with the current codebase. Please, let me know if you have a specific example that would look better on the current codebase, I would be happy to adapt. The CI result with the wasi execution can be found at https://github.com/ereslibre/php-src/actions/runs/4403303495 |
There are a lot of segfaults in the CI: https://github.com/ereslibre/php-src/actions/runs/4403303495/jobs/7711483600#step:7:9775 |
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 think it tries to disable way too many function. As I understand wasm is missing some of libc functions, right? If so, then it would be probably better to wrap the missing pieces with functions that have the same signature like libc functions and always fail. Then you can link those for wasm and it won't be necessary to add that many ifdefs.
Zend/zend.c
Outdated
@@ -1175,9 +1177,9 @@ ZEND_COLD void zenderror(const char *error) /* {{{ */ | |||
} | |||
/* }}} */ | |||
|
|||
#ifndef __wasi__ |
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.
This might need some more checking of places where the bailout is used and veryfying if it might have any impact on the apllication under wasm. We basically don't want to introduce something that will result to future bug and I would imagine that disabling bailout might be one of those things without a proper checking.
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.
Yes, absolutely. Also, we will implement the bailout logic once we include setjmp/longjmp support.
main/fastcgi.c
Outdated
@@ -1209,6 +1223,12 @@ static int fcgi_read_request(fcgi_request *req) | |||
|
|||
return 1; | |||
} | |||
#else // __wasi__ | |||
static int fcgi_read_request(fcgi_request *req) |
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.
what's the actual point of using FCGI if it's not re-using connection (if it's 0, then it closes the connection)? Why don't you just use plain CGI?
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.
Updated the patch; fastcgi.c
requires some changes because cgi_main
uses certain symbols that come from fastcgi.c
; and we are interested in php-cgi
.
wasi/main/php_open_temporary_file.c
Outdated
|
||
PHPAPI const char* php_get_temporary_directory(void) | ||
{ | ||
return NULL; |
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.
use tabs and not spaces (this is not the place where you indent with spaces so please all other cases as well)
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.
Sorry, I am going to check everything, my editor is pretty stubborn at times.
f25535d
to
0188a82
Compare
WASM32_WASI: | ||
name: "WASM32_WASI_${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}" |
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.
WASM32_WASI: | |
name: "WASM32_WASI_${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}" | |
WASI: | |
name: "WASI_${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}" |
just to make the CI job name shorter/nicer...
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 would keep WASM32_WASI
if that's fine, given that WASM64 be added eventually
run: make -j$(/usr/bin/nproc) cgi cli | ||
- name: Optimize | ||
uses: ./.github/actions/optimize-wasi | ||
# To be defined how to approach test coverage on this platform. |
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.
All core tests (tests that does not require any unsupported extension) must pass or they must be exluded if they are expected to fail ATM.
setjmp/longjmp emulation
Fibers are core part of the PHP language since PHP 8.0 release. Can the support be added from the day zero?
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.
All core tests (tests that does not require any unsupported extension) must pass or they must be exluded if they are expected to fail ATM.
Will exclude tests that don't apply on this platform in a next pass.
Fibers are core part of the PHP language since PHP 8.0 release. Can the support be added from the day zero?
We should look into that. If it's a prerequisite for this patch to land, then yes. If it's fine to iterate I'd prefer so, to not block the whole patch on smaller specifics, given the patch is already somewhat big.
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 haven't actually finished the review. But the trend seemed to continue. I think we need a better approach for missing functions/features than stubbing them out.
ZEND_API ZEND_COLD ZEND_NORETURN void _zend_bailout(const char *filename, | ||
uint32_t lineno) /* {{{ */ | ||
{ | ||
// Not implemented yet, as WebAssembly has no stack switching |
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.
Is there a reason why this is in its own file?
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.
The idea was to be as less intrusive as possible with the existing tree, avoiding unnecessary ifdefs that might clutter existing files.
If you think there's a better way I'm fine with any proposal!
|
||
#include <errno.h> | ||
|
||
int dup(int fildes) |
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.
Does this need to be in a separate file? Separate files are a bit of a pain with autoconf because it require calling ./configure
again.
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.
Yes, same rationale as #10457 (comment), to be as less intrusive as possible.
I understand the ./configure
re-run concern, but at this stage I think is better to keep them well separated, and as WASI evolves with future snapshots start merging with the main tree as things establish on upcoming wasi-libc versions.
060ea6f
to
b7883f5
Compare
This change performs initial adaptations allowing the PHP interpreter to be compiled for the wasm32-wasi target. Co-Authored-By: Asen Alexandrov <[email protected]>
WebAssembly + WASI GH actions have specific tasks for the following steps: - apt: an existing PHP installation will be used as the test runner, as opposed to the binary being built. It also installs other dependencies used by later stages, such as Wasmtime (WebAssembly runtime), and Binaryen (contains `wasm-opt`, to be used on the optimization phase.) - configure-wasi: specific configure values. - optimize-wasi (new action): performs binaryen optimization passes on the resulting binary. - test-wasi: requires a wrapper that will use `wasmtime` to run the `php` CLI compiled to WebAssembly.
Thanks for your review @iluuu1994. I have updated the patch fixing all your comments, and I also commented about the separate files here: #10457 (comment). This change will evolve as the wasi-libc evolves. However, the principle is always to be as less intrusive as possible with the current codebase. When there are concepts that are not directly mappable to WebAssembly/WASI, we stubbed them out; in this patch this happened in a case by case basis, so please, let us know if you'd like to see things handled in a different way. The goal is to propose this patch for landing on the PHP tree, which I think might need an RFC process, but before that happens I think is worth getting done as much as possible. Because of this, feedback from PHP contributors is highly valuable. |
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.
There are still plenty of places where functions just "appear" to exist, but without doing anything.
php_log_err_with_severity(message, LOG_NOTICE); | ||
#else | ||
php_log_err_with_severity(message, 5); |
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.
Magic numbers are not a good idea. If WASM doesn't define LOG_NOTICE, then have a block somewhere else that defines this (and I suspect other constants) in one go.
|
||
#else | ||
|
||
void php_shell_exec_ex(INTERNAL_FUNCTION_PARAMETERS); |
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.
This reads really odd - if WASI is active, you only use a forward declaration. I would rather do it like:
#ifndef PHP_WASI
static void php_shell_exec_ex(INTERNAL_FUNCTION_PARAMETERS) /* {{{
{
…
}
#else
static void php_shell_exec_ex(INTERNAL_FUNCTION_PARAMETERS) /* {{{
{
php_wasi_shell_exec_ex(INTERNAL_FUNCTION_PARAMETERS_PASSTHRU);
}
#endif // PHP_WASI
And then have your new files implement the php_wasi_*
variant(s).
But considering that doesn't do anything on WASI, this function should also just not even exist on that platform.
|
||
PHPAPI void php_umask(INTERNAL_FUNCTION_PARAMETERS) /* {{{ */ | ||
{ | ||
RETURN_LONG(0); |
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.
Again, these functions do nothing, so they shouldn't exist.
I thank everyone who has provided feedback on this PR. I'm closing it as is. If there is interest in reviving this work I encourage folks to take the lead on that. |
Add support for building to the wasm32-wasi target
This change performs initial adaptations allowing the PHP interpreter
to be compiled for the wasm32-wasi target.
Co-Authored-By: Asen Alexandrov [email protected]