-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add feature matrix item for Legacy Wasm EH and Wasm Exnref EH, and separate test skipping to two environment variables #25423
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
Changes from all commits
4d53056
68e26e3
59e1ce8
36bd048
32dfa3d
d499c8a
7594f8a
fa2dc4f
4dcf581
3ab97c9
c003d0c
351e67a
23cdb4b
1911a91
80b1754
6047664
860c50d
458d190
c8077e5
9fcd1b9
9b89c2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,8 @@ class Feature(IntEnum): | |
MEMORY64 = auto() | ||
WORKER_ES6_MODULES = auto() | ||
OFFSCREENCANVAS_SUPPORT = auto() | ||
WASM_LEGACY_EXCEPTIONS = auto() | ||
WASM_EXCEPTIONS = auto() | ||
|
||
|
||
disable_override_features = set() | ||
|
@@ -105,6 +107,26 @@ class Feature(IntEnum): | |
'safari': 170000, | ||
'node': 0, # This is a browser only feature, no requirements on Node.js | ||
}, | ||
# Legacy Wasm exceptions was the first (now legacy) format for native | ||
# exception handling in WebAssembly. | ||
Feature.WASM_LEGACY_EXCEPTIONS: { | ||
'chrome': 95, | ||
'firefox': 100, | ||
'safari': 150200, | ||
'node': 170000, | ||
}, | ||
# Wasm exceptions is a newer format for native exception handling in | ||
# WebAssembly. | ||
Feature.WASM_EXCEPTIONS: { | ||
'chrome': 137, | ||
'firefox': 131, | ||
'safari': 180400, | ||
# Supported with flag --experimental-wasm-exnref (TODO: Change this to | ||
# unflagged version of Node.js 260000 that ships Wasm EH enabled, after | ||
# Emscripten unit testing has migrated to Node.js 26, and Emsdk ships | ||
# Node.js 26) | ||
'node': 240000, | ||
}, | ||
} | ||
|
||
# Static assertion to check that we actually need each of the above feature flags | ||
|
@@ -165,7 +187,7 @@ def enable_feature(feature, reason, override=False): | |
diagnostics.warning( | ||
'compatibility', | ||
f'{name}={user_settings[name]} is not compatible with {reason} ' | ||
f'({min_version} or above required)') | ||
f'({name}={min_version} or above required)') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A drive-by improvement to help error message
which one might at first glance read as needing to pass |
||
else: | ||
# If no conflict, bump the minimum version to accommodate the feature. | ||
logger.debug(f'Enabling {name}={min_version} to accommodate {reason}') | ||
|
@@ -199,3 +221,8 @@ def apply_min_browser_versions(): | |
enable_feature(Feature.WORKER_ES6_MODULES, 'EXPORT_ES6 with -sWASM_WORKERS') | ||
if settings.OFFSCREENCANVAS_SUPPORT: | ||
enable_feature(Feature.OFFSCREENCANVAS_SUPPORT, 'OFFSCREENCANVAS_SUPPORT') | ||
if settings.WASM_EXCEPTIONS or settings.SUPPORT_LONGJMP == 'wasm': # Wasm longjmp support will lean on Wasm (Legacy) EH | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code looks fine, but Wasm longjmp support does not particularly require the legacy EH. It should work fine with both legacy and non-legacy EH. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, how do you mean? Is the comment wrong, or? Should I update to something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, I missed the parentheses, and read that sentence as "Wasm longjmp support will lean on Wasm legacy EH". So I said "No Wasm longjmp supports works fine with exnref too". Now I understand what you meant. |
||
if settings.WASM_LEGACY_EXCEPTIONS: | ||
enable_feature(Feature.WASM_LEGACY_EXCEPTIONS, 'Wasm Legacy exceptions (-fwasm-exceptions with -sWASM_LEGACY_EXCEPTIONS=1)') | ||
else: | ||
enable_feature(Feature.WASM_EXCEPTIONS, 'Wasm exceptions (-fwasm-exceptions with -sWASM_LEGACY_EXCEPTIONS=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.
@aheejin now I realize my root source of the confusion. What was happening is that the semantics of
EMTEST_SKIP_EH=1
was to not actually skip the test, if the node.js shell was detected to support the target EH.I would like to change the semantics to mean that if user passes
EMTEST_SKIP_EH=1
(orEMTEST_SKIP_WASM_LEGACY_EH=1
), then do skip the tests, independent of Node.js version. (the tests are running in the browser, so Node.js version does not matter)So what was happening all the time on my CI tests was that I was passing
EMTEST_SKIP_EH=1
andEMTEST_SKIP_WASM_LEGACY_EH=1
and running in Firefox 91, which doesn't support either; but the tests would not be skipped because I am running on Node.js 24, and they would then proceed to fail in the old browser.Uh oh!
There was an error while loading. Please reload this page.
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 agree that's confusing. I actually wasn't aware that we don't skip the tests even if we have
EMTEST_SKIP_***
set if the node version is high enough. But it looks this has been this way for years and it doesn't only apply to Wasm EH. Other methods likerequire_simd
andrequire_wasm64
have the same problem. I think it would be better to change it in a separate PR for allrequire_***
s if we are to change this. @sbc100 What do you think about changing the meaning ofEMTEST_SKIP_***
? Is there a reason that we don't honor these skip variables when the node version is high?Uh oh!
There was an error while loading. Please reload this page.
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.
Seems reasonable to always skip yes.
I guess the original meaning was more like "EMTEST_ALLOW_SKIPPING_OF_XXX_IF_NEEDED", but don't see why it shouldn't be "EMTEST_ALWAYS_SKIP_XX".
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 agree we that if we make this change it should be in its own PR.
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.
#25445