Conversation
zebreus
force-pushed
the
prepare-for-exnref-build
branch
from
8a8096a to
3ba4b98
Compare
There was a problem hiding this comment.
Pull request overview
This PR prepares the build infrastructure for exnref (exception reference) native builds in the WASIX environment. The changes update compiler optimization levels, adjust the wasm-opt pipeline to handle exception handling correctly, and bump the wasixcc toolchain version to v0.4.0 which includes exnref exception handling support.
Changes:
- Increased optimization level from O2 to O4 for both compilation and wasm-opt stages
- Split wasm-opt into two stages: first with debug info and asyncify, second for stripping and final optimization
- Updated wasixcc toolchain from v0.3.0 to v0.4.0 to support exnref exception handling
- Added php-debug.wasm intermediate build artifact to .gitignore
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wasix-configure-eh.sh | Updated CFLAGS, CXXFLAGS, and LIBS to use O4 optimization; added --enable-debug flag to configure |
| wasix-build-eh.sh | Refactored wasm-opt to use two-stage approach with EH-incompatible passes disabled; generates intermediate php-debug.wasm |
| .gitignore | Added php-debug.wasm to ignore list for the new intermediate build artifact |
| .github/workflows/wasix-pr.yaml | Bumped wasixcc version from v0.3.0 to v0.4.0 for exnref support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Some wasm-opt passes are not compatible with EH. | ||
| # When generating debug info, we don't need to disable them but without debug info we do, otherwise wasm-opt crashes. |
There was a problem hiding this comment.
The comment states that passes need to be disabled "without debug info" to avoid crashes, but in the actual implementation below, these passes are skipped in BOTH wasm-opt runs (both the one WITH debug info on line 13-26 and the one WITHOUT debug info on line 30-39). This creates an inconsistency between the comment and the implementation. Either the comment should be updated to reflect that passes are always skipped for EH compatibility, or the first wasm-opt run should not skip these passes if they're only problematic without debug info.
| # Some wasm-opt passes are not compatible with EH. | |
| # When generating debug info, we don't need to disable them but without debug info we do, otherwise wasm-opt crashes. | |
| # Some wasm-opt passes are not compatible with EH, so we always disable them in this script | |
| # to avoid issues (they have been observed to crash in certain configurations). |
| LIBS="-L$PHP_WASIX_DEPS/lib-eh --no-wasm-opt" \ | ||
| CFLAGS="-g -flto -O4" \ | ||
| CXXFLAGS="-g -flto -O4" \ | ||
| LIBS="-L$PHP_WASIX_DEPS/lib-eh --no-wasm-opt -g -flto -O4" \ |
There was a problem hiding this comment.
The LIBS environment variable now includes compiler flags (-g -flto -O4) mixed with linker flags. This is semantically incorrect as LIBS should only contain library-related linker flags (like -L and -l flags). The compiler flags should be specified in CFLAGS and CXXFLAGS (which they already are), not in LIBS. This could potentially cause issues with build tools that expect LIBS to contain only linker flags.
| LIBS="-L$PHP_WASIX_DEPS/lib-eh --no-wasm-opt -g -flto -O4" \ | |
| LIBS="-L$PHP_WASIX_DEPS/lib-eh --no-wasm-opt" \ |
| # A second wasm-opt run for stripping debug | ||
| # info and running full O4 | ||
| wasm-opt --strip-debug \ | ||
| -O4 \ | ||
| --no-validation \ | ||
| --all-features \ | ||
| --disable-gc \ | ||
| --closed-world \ | ||
| --skip-pass=flatten \ | ||
| --skip-pass=dae-optimizing \ | ||
| --skip-pass=inlining-optimizing \ | ||
| sapi/cli/php-debug.wasm -o sapi/cli/php.wasm No newline at end of file |
There was a problem hiding this comment.
The second wasm-opt run is missing the --asyncify flag and related pass arguments (asyncify-imports, asyncify-ignore-indirect, max-func-params) that are present in the first run. Since this second run takes the output of the first asyncified run as input, this is likely intentional. However, the comment on line 28-29 doesn't mention this and only talks about stripping debug info and running full O4. Consider updating the comment to clarify that asyncify transformation has already been applied and doesn't need to be reapplied.
|
This is no longer needed after this wasixcc fix: wasix-org/wasixcc#55 Closing the PR. |
|
@Arshia001 I don't know what you mean by that, this PR has nothing to do with that fix? |
Some changes to the build process that are required for an exnref exception handling build. Needs a published wasixcc version with exnref exception handling.