-
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
Conversation
…y vs Exnref EH skipping.
'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 comment
The reason will be displayed to describe this comment to others. Learn more.
A drive-by improvement to help error message
MIN_FIREFOX_VERSION=91 is not compatible with -sWASM_LEGACY_EXCEPTIONS=0 (131 or above required)
which one might at first glance read as needing to pass -sWASM_LEGACY_EXCEPTIONS=131
to fix.
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 have been mostly trying to avoid using the term "exnref" in the code, especially in possibly user-facing parts. Others may disagree, but I think, that we implement EH using exnref is an implementation detail that does not necessarily need to be exposed, but was exposed anyway due to the unfortunate churns in the proposal development process. So I've been trying to call each of them "Wasm EH" and "Wasm legacy EH" respectively. I think it's fine (or even clarifying in some cases) to say this is the exnref version in the description or comments though.
I think having a setting specific to the legacy EH is good, but can we keep the name EMTEST_SKIP_EH
and others? I think it's fine they mean the newest (exnref) EH. The same for the feature names. How about WASM_LEGACY_EXCEPTIONS
and WASM_EXCEPTIONS
?
test/common.py
Outdated
if 'EMTEST_SKIP_WASM_EXNREF_EH' in os.environ: | ||
self.skipTest('test requires a browser with new Exnref Wasm exceptions support (and EMTEST_SKIP_WASM_EXNREF_EH is set)') | ||
return |
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 looks like a repetition of line 1234-1235. Can't we check this only once at the top like we used to do?
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 wanted to keep the original error message for node v24 descriptive, rather than give a generic error mentioning all browser environments. That is because it might not be obvious to user that node 24 can be used to target exnref exceptions with the flag.
tools/feature_matrix.py
Outdated
if not settings.WASM_LEGACY_EXCEPTIONS: | ||
enable_feature(Feature.WASM_EXNREF_EXCEPTIONS, '-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.
This is not nested in any other if
s. Doesn't this mean we always enable exnref when WASM_LEGACY_EXCEPTIONS
is not set, even if we are not using Wasm EH at all?
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 that is correct. I was a bit surprised about that at first as well, but that is the behavior as it currently exists. E.g.
def require_wasm_eh(self):
self.set_setting('WASM_LEGACY_EXCEPTIONS', 0)
only reverts legacy exceptions off, and does not pass -fwasm-exceptions
. (The passing of -fwasm-exceptions
seems to be occurring somewhere else)
That may be behavior to change in the future, at which point this test matrix setup should also then change.
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.
Not sure if I understand. That code just means in Wasm EH (meaning exnref EH here), we turn off legacy EH. And that require_
function runs only to check whether the current environment meets the Wasm EH requirements in the tests. But it looks this apply_min_browser_versions
is used in phase_linker_setup
the linking process. What this will do is, even when Wasm EH is not being used and there's no -fwasm-exceptions
, this will set the minimum browser versions to that of the Wasm (exnref) EH, no?
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 do see Wasm Exnref exceptions being enabled in a build that does not pass -fwasm-exceptions
, which is what is confusing me.
But that looks like some kind of a bug instead in SDL2_image. Posted #25430 about that.
I understand that the intent is that -fwasm-exceptions -sWASM_LEGACY_EXCEPTIONS=1
should target Wasm Legacy EH, and -fwasm-exceptions -sWASM_LEGACY_EXCEPTIONS=0
should target Wasm Exnref EH. Updated the code to match that.
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.
For the tests, -fwasm-exceptions
is enabled here:
Line 800 in 0d529ee
self.cflags.append('-fwasm-exceptions') |
"a.out.js": 19539, | ||
"a.out.js.gz": 8086, | ||
"a.out.nodebug.wasm": 144630, | ||
"a.out.nodebug.wasm.gz": 54894, | ||
"total": 164238, | ||
"total_gz": 63003, | ||
"total": 164169, | ||
"total_gz": 62980, |
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.
Why would this patch change the code size?
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.
That is because enabling Wasm EH via -sWASM_LEGACY_EXCEPTIONS=0
will now result in the MIN_*_VERSIONS
being automatically bumped upwards to the minimum required versions that have this feature.
So as corollary, other places of code can assume those versions, and they will drop old code targeting older browser versions. This is good, and as expected.
(On the other hand, if the user explicitly tried to set, say -sWASM_LEGACY_EXCEPTIONS=0 -sMIN_FIREFOX_VERSION=100
, then the compilation would explicitly fail with emcc complaining that this version of Firefox cannot target Exnref exceptions)
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.
Do MIN_*_VERSIONS
options change code size?
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.
Yeah, for example in https://github.com/emscripten-core/emscripten/pull/25407/files , if user is targeting older browsers, a more verbose code pattern is used. But if all target browser versions are new enough, a shorter code pattern is used.
I brought that name up because that is the user-facing name also advertised in WebAssembly Roadmap, which developers publicly use.
I find the name "Wasm EH" very ambiguous. It reads like it is what should be getting enabled if one passes Even if it is an internal implementation detail, that detail cannot change without requiring a full new spec? So it would be safe to use as a technical name for the new Wasm EH? |
Separating the environment variables to skip tests that verify legacy Wasm EH versus Exnref Wasm EH are important, since those were introduced to browsers at very different times. That allows testing legacy Wasm EH in browsers that do not have Exnref Wasm EH support. |
So the plan has been that we deprecate the legacy EH in near future. I'm not sure if that's a realistic short-term goal at this point, but we'd like to at least make the legacy EH a rarely used old special case we keep only for the compatibility's sake, even if we are not successful in completely deprecating it from the web. That's the reason I wanted to just call the new EH without any qualifiers, while calling the old EH "legacy EH", like here and other places: Lines 815 to 817 in 834eb88
Yes that's not the case because
I don't mean I want to change that detail with a new spec, no. In near future, I just want people to use the new (exnref) Wasm EH via |
What I meant was to use |
Since Node 24 doesn't support the new eh without an experimental flag and Node 26 isn't out yet, it seems to me that the legacy eh should be the default at least until Node 26 is out and probably a bit longer after that. |
I hope that web will not remove Wasm Legacy EH, because all Unity output targets it today. There is a lot of content with Wasm Legacy EH in the wild out there. I was excited about roadmapping the work of migrating Unity to use Wasm Exnref EH, but after seeing #25365 , my excitement fell considerably. It looks like Unity will be on the Wasm Legacy EH for quite a while from now. |
Ok, updated this PR to not use the term exnref, but just refer to Wasm exception handling. Should I open a ticket over to WebAssembly/website to adjust the WebAssembly Roadmap to change "Exception Handling with exnref" to "Exception Handling"? |
…ts, even if current environment would support them.
|
||
def require_wasm_legacy_eh(self): | ||
if 'EMTEST_SKIP_WASM_LEGACY_EH' in os.environ: | ||
self.skipTest('test requires node >= 17 or d8 (and EMTEST_SKIP_WASM_LEGACY_EH is set)') |
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
(or EMTEST_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
and EMTEST_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.
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 like require_simd
and require_wasm64
have the same problem. I think it would be better to change it in a separate PR for all require_***
s if we are to change this. @sbc100 What do you think about changing the meaning of EMTEST_SKIP_***
? Is there a reason that we don't honor these skip variables when the node version is high?
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.
Hmm, not sure. I don't mind either way, but people in CG might think that's confusing. (Which you might think applies to the users here with the same rationale, which I don't disagree completely, but I somehow feel I just don't want to expose "exnref" to day-to-day users.) |
Updated this PR on top of main. |
.circleci/config.yml
Outdated
EMTEST_SKIP_WASM_LEGACY_EH: "1" | ||
EMTEST_SKIP_EH: "1" |
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 for the confusion, but if we want to skip Wasm EH (exnref), maybe this should be EMTEST_SKIP_WASM_EH
(And this has been used to only skip Wasm EH so far)
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 for the confusion, but if we want to skip Wasm EH (exnref), maybe this should be
EMTEST_SKIP_WASM_EH
Oh, I renamed it back to EMTEST_SKIP_EH
because you previously reviewed that you wanted to use EMTEST_SKIP_EH=1
to do this: "What I meant was to use EMTEST_SKIP_EH to mean skipping the exnref EH"
(And this has been used to only skip Wasm EH so far)
I don't think that is the case. EMTEST_SKIP_EH
would skip both Legacy Wasm EH and Exnref Wasm EH tests the same before this PR. This PR is separating these two with two different skip environment variables.
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 for the confusion, but if we want to skip Wasm EH (exnref), maybe this should be
EMTEST_SKIP_WASM_EH
Oh, I renamed it back to
EMTEST_SKIP_EH
because you previously reviewed that you wanted to useEMTEST_SKIP_EH=1
to do this: "What I meant was to use EMTEST_SKIP_EH to mean skipping the exnref EH"
Sorry I should've asked to rename it to EMTEST_SKIP_WASM_EH
. What I was saying was mostly that I didn't want the name 'exnref' in there. Given that legacy / exnref differs only in whether it's legacy or not, I think WASM
should be in both of them.
(And this has been used to only skip Wasm EH so far)
I don't think that is the case.
EMTEST_SKIP_EH
would skip both Legacy Wasm EH and Exnref Wasm EH tests the same before this PR. This PR is separating these two with two different skip environment variables.
Sorry for the confusion again. What I meant by "Wasm EH" was not exnref but just the Wasm proposal-using EH in general, i.e., not "Emscripten EH" that's simulated by JS. And currently EMTEST_SKIP_EH
is used to skip both kinds of Wasm EH (legacy and exnref), but not Emscripten EH:
Lines 1132 to 1165 in 7b36fc8
def require_wasm_legacy_eh(self): | |
if 'EMTEST_SKIP_EH' in os.environ: | |
self.skipTest('test requires node >= 17 or d8 (and EMTEST_SKIP_EH is set)') | |
self.set_setting('WASM_LEGACY_EXCEPTIONS') | |
if self.try_require_node_version(17): | |
return | |
v8 = self.get_v8() | |
if v8: | |
self.cflags.append('-sENVIRONMENT=shell') | |
self.js_engines = [v8] | |
return | |
self.fail('either d8 or node >= 17 required to run wasm-eh tests. Use EMTEST_SKIP_EH to skip') | |
def require_wasm_eh(self): | |
if 'EMTEST_SKIP_EH' in os.environ: | |
self.skipTest('test requires node v24 or d8 (and EMTEST_SKIP_EH is set)') | |
self.set_setting('WASM_LEGACY_EXCEPTIONS', 0) | |
if self.try_require_node_version(24): | |
self.node_args.append('--experimental-wasm-exnref') | |
return | |
if self.is_browser_test(): | |
return | |
v8 = self.get_v8() | |
if v8: | |
self.cflags.append('-sENVIRONMENT=shell') | |
self.js_engines = [v8] | |
self.v8_args.append('--experimental-wasm-exnref') | |
return | |
self.fail('either d8 or node v24 required to run wasm-eh tests. Use EMTEST_SKIP_EH to skip') |
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.
Ok, I renamed EMTEST_SKIP_EH
to EMTEST_SKIP_WASM_EH
. How does it look now?
test/common.py
Outdated
return | ||
|
||
self.fail('either d8 or node v24 required to run JSPI tests. Use EMTEST_SKIP_JSPI to skip') | ||
self.fail('either d8 or node v24 required to run JSPI tests. Use EMTEST_SKIP_JSPI to skip') |
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.
Was this meant to be included?
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.
No, that was a bad git automerge. Cleaned up.
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 comment
The 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 comment
The 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 comment
The 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.
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.
Thanks!
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 comment
The 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.
Add feature for Wasm Exnref support and separate test skips for legacy vs Exnref EH skipping.
This will automatically set min browser versions when either Wasm legacy or Exnref exception handling is used, and error our if the users explictly requests a too-old browser version.