Enable WASM_BIGINT by default #22993

dschuff · 2024-11-22T19:45:03Z

Enables by default by temporarily reducing required Safari version.

Also includes bugfixes for WASM_BIGINT compatibility.

Enables by default by temporarily reducing required Safari version. Also includes bugfixes for WASM_BIGINT compatibility.

src/library_wasmfs_jsimpl.js

src/shell.js

test/test_core.py

tools/feature_matrix.py

tools/link.py

sbc100

Can't wait to see this land!

dschuff · 2024-11-22T21:41:20Z

This is still WIP and has some tests and/or code under test that needs to be fixed up. I'll ping you again when it's ready or when I need other advice :)

test/test_other.py

test/test_core.py

kripken

Please mention this in the changelog.

test/test_sanity.py

kripken · 2024-12-18T01:13:26Z

src/shell.js

@@ -34,7 +34,8 @@ var Module = typeof {{{ EXPORT_NAME }}} != 'undefined' ? {{{ EXPORT_NAME }}} : {
 #endif // USE_CLOSURE_COMPILER

 #if POLYFILL
-#if WASM_BIGINT && MIN_SAFARI_VERSION < 150000
+#if WASM_BIGINT && MIN_SAFARI_VERSION < 140100
+// TODO(https://github.com/emscripten-core/emscripten/issues/23184): Fix this back to 150000


I don't follow these changes. This will make the polyfill be added in fewer cases - why is that correct?

The point of the general messing with safari versions ( in feature_matrix itself) is to turn BIGINT on by default without affecting other features. This one here is just so that we won't default to having BIGINT on with this polyfill.

Ok, thanks, I see. So the goal is to split out the safari version change? If so, I wonder if we can do that in this PR? The complexity of splitting it seems relatively high to me.

Well, the real goal is to avoid changing all 3 settings in a single commit. But the commits also tie the settings more directly to the browser versions. So the simplest way to make enabling the features independent from each other (and independent from other consequences of raising the default version target) was to lower the version target for each feature separately (which should be a 2-line change for this feature, and 1-line for the others).

Hmm, sorry, but why was that the real goal? Naively I'd think that adjusting the browser versions would be the natural way to enable features by default, so it fits in this PR, so I'm still missing something, sorry.

Hm. I guess after the release we aren't really bisecting like you do with revisions, we're just testing flags to find out what breaks. So the options are testing by enabling or disabling the feature flags, or by setting the fake MIN_SAFARI version numbers and seeing what changes. But at that point the feature flags will still be there, so it's not clear to me what keeping the fake safari versions adds.

Hm. I guess after the release we aren't really bisecting like you do with revisions, we're just testing flags to find out what breaks.

Was this in reference to my mention of bisecting on MIN_SAFARI?

If so then my point there was that if we add Safari 14.7, 14.8 etc., avoiding a single version that adds multiple features, then bisecting MIN_SAFARI in the future becomes more useful. I agree that using the feature flags could also be done instead, but that requires one to know which flags are relevant and manually set them up, while bisecting on MIN_SAFARI can be done by just mindless bisection on a range of numbers. Like mindless bisection on commits, that sounds useful.

Anyhow this is a minor advantage of 14.7, 14.8 imo. The larger benefit as it I see it is that once we decide on the 14.7 etc. scheme, it is permanent: we have nothing to revert in a later PR, and we have no odd things that show up during bisection that were temporary before being undone. It is a design that "smooths" out safari versions to make them incremental. If browsers enable multiple features in a single release in the future then the same idea could be used again, that is, I don't see this as a one-off hack: it is a principled approach to this problem, as I see it.

But, again, if no one else sees value in this, that's fine 😄 I'm ok with the alternative, the difference is not huge here.

Yeah, I think mainly where we disagree is that I don't really like the idea of keeping fake browser versions around in the codebase. Bisecting on browser versions isn't really mindless if you have to know that the fake version numbers exist and what they are for, and what they can be set to.

Fair enough, sgtm. Maybe I like the idea of smoothing the history because it sounds like a math operation... but I agree fake versions have the downside you mention too.

dschuff · 2024-12-18T01:22:50Z

I think the plan for the changelog is to make it say that the default Safari target has been raised to 15, and that includes all 3 features. This PR doesn't do that so I figured I'd update the changelog when I update the browser version.

sbc100 · 2024-12-18T01:30:12Z

I think the plan for the changelog is to make it say that the default Safari target has been raised to 15, and that includes all 3 features. This PR doesn't do that so I figured I'd update the changelog when I update the browser version.

Maybe add a temporary entry about WASM_BIGINT and then make it more general in the followups?

dschuff · 2024-12-18T19:42:34Z

BTW it looks like all the tests here (and separate testing with the FYI tests) are now green, so once we resolve the open comment I plan to land this (assuming no new comments of course).

dschuff · 2024-12-18T22:18:28Z

Argh, expectations changes keep landing on main and causing conflicts, blocking this from merging while the tests are running :(

sbc100 · 2024-12-18T22:21:23Z

Argh, expectations changes keep landing on main and causing conflicts, blocking this from merging while the tests are running :(

Yes, sorry about that. If you know that all other tests are passing, feel free update the expections and land the PR without waiting for the tests.

I do this sometimes when I know all the tests are otherwise green.

sbc100 · 2024-12-18T22:34:13Z

I'll try no to land any more changes until this one goes in :)

ChangeLog.md

dschuff · 2024-12-18T22:35:24Z

I just ran tools/maint/rebaseline_tests.py and it found 2 that I missed with my manual method 👍

I'll go ahead and land this.

sbc100 · 2024-12-18T22:37:23Z

test/return64bit/testbind.js

-  var low = _test_return64(0x11223344, 0xaabbccdd);
-  var high = getTempRet0();
+  // Use eval to create BigInt, as no support for Xn notation yet in JS
+  // optimizer.


Hmm.. how can that be true? We use the Xn notation all over the place and run the generated code through the JS optimizer

Oh I see this code is just moving around... I guess we can update it after.

That diff is a little deceptive. What I actually did is rename testbind.js to testbind_nobigint.js and testbind_bigint.js to testbind.js. So that is just the pre-existing diff between those files.

sbc100 · 2024-12-18T22:39:46Z

test/test_sanity.py


    make_fake_tool(self.in_dir('fake', 'bin', 'wasm-opt'), '70')
-    self.check_working([EMCC, test_file('hello_world.c')], 'unexpected binaryen version: 70 (expected ')
+    self.check_working([EMCC, test_file('hello_world.c'), '-O2'], 'unexpected binaryen version: 70 (expected ')


This is really good to see, since it confirms that wasm-opt doesn't run by default. Yay!

Yeah; maybe we could even add a new test that runs various options with ERROR_ON_WASM_CHANGES_AFTER_LINK.

dasa · 2024-12-20T00:56:25Z

ChangeLog.md

@@ -4,7 +4,7 @@ Note that version numbers do not necessarily reflect the amount of changes
 between versions. A version number reflects a release that is known to pass all
 tests, and versions may be tagged more or less frequently at different times.

-Note that there is *no* ABI compatibility guarantee between versions - the ABI
+nNote that there is *no* ABI compatibility guarantee between versions - the ABI


[WIP] Enable WASM_BIGINT by default

9eaa392

Enables by default by temporarily reducing required Safari version. Also includes bugfixes for WASM_BIGINT compatibility.

dschuff commented Nov 22, 2024

View reviewed changes

src/library_wasmfs_jsimpl.js Outdated Show resolved Hide resolved

dschuff commented Nov 22, 2024

View reviewed changes

src/shell.js Show resolved Hide resolved

sbc100 reviewed Nov 22, 2024

View reviewed changes

test/test_core.py Show resolved Hide resolved

sbc100 reviewed Nov 22, 2024

View reviewed changes

tools/feature_matrix.py Outdated Show resolved Hide resolved

sbc100 reviewed Nov 22, 2024

View reviewed changes

tools/link.py Outdated Show resolved Hide resolved

sbc100 reviewed Nov 22, 2024

View reviewed changes

tools/link.py Show resolved Hide resolved

sbc100 approved these changes Nov 22, 2024

View reviewed changes

review comments; fix name section emission

5532f29

dschuff added 13 commits November 22, 2024 13:51

one more debuginfo test works now

5d13302

fix more tests, use explicit need_name_section in finalize

152c205

Use i53abi for one more wasmfs function

4a6588e

update node version for bigint

52fedfd

dont use i53 in the wrong place

01d5e34

Merge branch 'main' into bigint_on

e19febb

rebaseline size tests

2c1b640

Merge branch 'main' into bigint_on

c0f2c4a

update browser.test_small_js_flags size

1f0232b

Merge branch 'main' into bigint_on

3cbc37c

fix WasmFS API

b9c8be1

Merge branch 'main' into bigint_on

fb50f90

rebaseline

513ce14

dschuff commented Dec 12, 2024

View reviewed changes

test/test_other.py Outdated Show resolved Hide resolved

dschuff added 6 commits December 12, 2024 16:24

fix test expectations

8185db1

rebaseline

22165af

set WASM_BIGINT setting, fix tests

dd34e39

Merge branch 'main' into bigint_on

166dcc7

fix doc, disable bigint with old node

3088726

fix old node

f81e0da

dschuff added 4 commits December 17, 2024 15:46

Merge branch 'main' into bigint_on

b70d46a

remove debug print

2ee2ce3

fix sanity.test_binaryen_version

759de04

fix test_dyncall_specific

f3c20a3

dschuff commented Dec 18, 2024

View reviewed changes

test/test_core.py Outdated Show resolved Hide resolved

kripken reviewed Dec 18, 2024

View reviewed changes

test/test_sanity.py Outdated Show resolved Hide resolved

kripken reviewed Dec 18, 2024

View reviewed changes

dschuff added 2 commits December 17, 2024 17:39

fix test_binaryen, fix min_browser_version

4abe5fb

add changelog, fix dyncall_specific again

b7d960a

dschuff added 2 commits December 18, 2024 12:43

Merge branch 'main' into bigint_on

8a3df8f

clarify comment

d9fb51c

kripken approved these changes Dec 18, 2024

View reviewed changes

dschuff enabled auto-merge (squash) December 18, 2024 21:10

dschuff added 2 commits December 18, 2024 13:52

Merge branch 'main' into bigint_on

ff8c662

Merge branch 'main' into bigint_on

9194564

rebaseline test_small_js_flags, and test_INCOMING_MODULE_JS_API

8e5ba19

sbc100 reviewed Dec 18, 2024

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

add PR number to changelog

02bc462

dschuff disabled auto-merge December 18, 2024 22:37

sbc100 reviewed Dec 18, 2024

View reviewed changes

dschuff merged commit 93d2f8d into emscripten-core:main Dec 18, 2024
9 of 14 checks passed

dasa reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable WASM_BIGINT by default #22993

Enable WASM_BIGINT by default #22993

dschuff commented Nov 22, 2024

sbc100 left a comment

dschuff commented Nov 22, 2024

kripken left a comment

kripken Dec 18, 2024

dschuff Dec 18, 2024

kripken Dec 18, 2024

dschuff Dec 18, 2024

kripken Dec 18, 2024

dschuff Dec 18, 2024

sbc100 Dec 18, 2024

kripken Dec 18, 2024

dschuff Dec 18, 2024

kripken Dec 18, 2024

dschuff commented Dec 18, 2024

sbc100 commented Dec 18, 2024

dschuff commented Dec 18, 2024 •

edited

Loading

dschuff commented Dec 18, 2024

sbc100 commented Dec 18, 2024

sbc100 commented Dec 18, 2024

dschuff commented Dec 18, 2024

sbc100 Dec 18, 2024

sbc100 Dec 18, 2024

dschuff Dec 18, 2024

sbc100 Dec 18, 2024

dschuff Dec 18, 2024

dasa Dec 20, 2024

Enable WASM_BIGINT by default #22993

Enable WASM_BIGINT by default #22993

Conversation

dschuff commented Nov 22, 2024

sbc100 left a comment

Choose a reason for hiding this comment

dschuff commented Nov 22, 2024

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Dec 18, 2024

sbc100 commented Dec 18, 2024

dschuff commented Dec 18, 2024 • edited Loading

dschuff commented Dec 18, 2024

sbc100 commented Dec 18, 2024

sbc100 commented Dec 18, 2024

dschuff commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Dec 18, 2024 •

edited

Loading