test/wasm: compare EH paths in Node and Chrome - #2654
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: WASM exception-handling comparison dev tool
Nicely scoped, self-contained dev tool. The Python is clean and idiomatic, the temp-dir handling and subprocess centralization are good, and dev/wasm-eh-comparison.md is commendably honest — it explicitly labels the numbers a smoke comparison and enumerates what is not yet tested (Go/C++/JS boundary, goroutine unwinding, browser compat).
No security concerns: all subprocess.run calls use argument lists with shell=False, temp handling is safe, and tool resolution is developer-controlled. No production performance risk — the one available speedup (parallelizing the 4 independent em++ compiles) is optional for a manual dev tool.
Findings below are all minor/low; the doc/behavior mismatch on env vars is the most worthwhile to address.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
force-pushed
the
codex/wasm-eh-matrix
branch
from
September 23, 2026 11:08
207279c to
09d43e6
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
changed the title
cpunion
force-pushed
the
codex/wasm-eh-matrix
branch
from
September 26, 2026 14:38
dd6d709 to
9097e17
Compare
visualfc
reviewed
Sep 27, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
Review
Can merge. This is a comparison experiment plus a decision record; it does not change LLGo's default EH encoding. The conclusion is appropriately conservative: keep the current Emscripten/LLGo browser path, catch C++ exceptions inside the wrapper, translate a C ABI status into a Go panic, and do not switch the full link to exnref from an isolated C++ comparison.
The earlier fennoai notes (tool env docs, tool() error text, glue replace comment, sjlj counter) are addressed in 9097e17.
Suggestions
1. Node 22 cannot run the direct / translated variants
run_cpp invokes Node without enabling exnref. CI uses Node 24, where V8 enables it by default, so the job is green. Emscripten's bundled Node 22 defaults to --no-experimental-wasm-exnref. On Node v22.16.0, O2 direct fails with:
CompileError: WebAssembly.instantiate(): invalid value type 'exn',
enable with --experimental-wasm-exnref
The same module prints cpp catch and sjlj ok after adding --experimental-wasm-exnref. Legacy already runs on Node 22 without that flag.
Either pass --experimental-wasm-exnref (still accepted on Node 24+, where it is already the default) or document Node 24+ and fail clearly on older versions. Developers commonly use emsdk's Node 22; without this, a local run looks like exnref is broken.
2. The decision table does not include the encoding actually in use
The table compares three Wasm EH encodings: legacy (-fwasm-exceptions -sWASM_LEGACY_EXCEPTIONS=1), direct exnref, and Binaryen translation. LLGo's browser default link does not pass -fwasm-exceptions. The Go/C++ boundary uses JS EH (-sDEFAULT_TO_CXX -sDISABLE_EXCEPTION_CATCHING=0).
Keeping the current encoding is the right call, but readers can take the Legacy column as the status quo. A row or note on the table would help: the current browser contract is JS EH plus catch-inside-wrapper; the three Wasm EH columns are later full-link candidates.
3. DWARF --verify is tightly coupled to the pinned toolchain
The script treats llvm-dwarfdump --verify failure as fatal. On Emscripten 6.0.2 + upstream Binaryen 132, legacy -O0 reports No errors. while direct -O2 reports parent-range and overlap errors (exit 1). CI is Emscripten 6.0.8 + llgo-v132.3, so it passes. The size snapshot in the doc is also from llgo-v132.2.
Worth stating in the doc that the DWARF check is meaningful only with emsdk 6.0.8 and EM_BINARYEN_ROOT / WASMOPT pointing at the LLGo Binaryen. Otherwise a local python3 dev/compare_wasm_eh.py fails on DWARF and looks like an EH regression.
Nits
- Doc still cites Binaryen
llgo-v132.2; CI is pinned tollgo-v132.3. The sizes are a snapshot; labeling the toolchain version is enough. - The Go/C++ "translation" is a predicate, not a dataflow: after
Catch()==7the code panics a fixed string, so the payload is independent of the status value 7. Enough for catch-inside-wrapper → C ABI → Go panic/recover; it does not show the status being carried into the panic. - The stronger Go panic fixture (
internal/build/testdata/wasm-runtime, panic/recover inside loop defers) runs only on Node.--browserdoes not execute it; Chrome only sees the trivial panic/recover in the wrapper. - The translated path reuses legacy JS glue and only rewrites the wasm filename. Fine for same-module throw/catch; it is not a full Emscripten
exnreflink. One sentence in the doc would make that explicit. - CI step
timeout-minutes: 10covers eight serial Chrome runs (six C++ + two wrappers), each up to 90s on the Python side. The current job is ~15m, so this is fine unless Chrome hangs. - The
try_tablesubstring check is whole-module (including libc++abi), not proof thatmainstill takes the EH path. O2 IR still hasinvoke __cxa_throw, so this is OK today.
visualfc
approved these changes
Sep 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LLGo needs an EH encoding decision that preserves C++ and Go behavior through the Emscripten/Asyncify browser build. This PR compares legacy EH, direct standard
exnref, and Binaryen--translate-to-exnrefwith the same C++ throw/catch and setjmp/longjmp fixture at O0 and O2. Every variant is validated, DWARF-verified, and executed in Node and Chrome. It also checks Go panic/recover and a Go→C++ wrapper that catches C++ exceptions internally, returns a C ABI status, and translates that status to a Go panic.The supported boundary remains the current Emscripten/LLGo JS EH encoding with C++ catch inside the wrapper; it is distinct from the three Wasm EH encodings compared here. The standalone comparison does not authorize a C++ exception to unwind through Go or switch the full LLGo link to
exnref. The result and revisit criteria are recorded indev/wasm-eh-comparison.md.This branch is stacked on #2652 so the new
wasm-runtimeCI step selects the publishedllgo-v132.3Binaryen release throughEM_BINARYEN_ROOTandWASMOPT. Once #2652 lands, its commits will disappear from this PR's diff.Local validation before the pin update used Emscripten 6.0.8, Binaryen
llgo-v132.2, Node 26.8.1, Chrome 153.0.8010.53, wasm-tools 1.258.0, and LLVM 22.1.8: all six C++ variants and both Go/C++ wrappers passed Node and Chrome at O0/O2; Go panic/recover passed;actionlint, Python compilation, andgit diff --checkpassed.The current head carries the published
llgo-v132.3pin. The comparison requires Emscripten 6.0.8's JS glue and Node >=24.15.0; its preflight rejects Node 22.16.0 with a clear diagnostic before building. The DWARF verifier is likewise scoped to this Emscripten version and LLGo-patched Binaryen. The complete Node 26.8.1 and Chrome EH comparison passed again with that release at O0/O2, including Go panic/recover and a Go/C++ wrapper that propagates the actual C ABI status into the Go panic; full PR CI is re-running.