[wasm64] making JS bindings wasm64 aware #12869

aardappel · 2020-11-24T01:07:38Z

This is a large collection of fixes resulting from trying to run the tests under wasm64. It is not complete, i.e. does not pass all tests, and requires a specific local setup. It is unknown how much work is left to pass all of them, though this allows significant amount of code to work.

I am trying to land these changes, because, well, maintaining this large fork is getting onerous, and it would be beneficial for wasm64 related changes to be already present, even if not used.

This PR has the wasm64 tests OFF, meaning it is meant to be a NFC for wasm32, effectively.

Of course, this PR contains a lot of unrelated fixes that ideally could have been spread out over 10+ PRs, had I known what I was doing. I think it made sense to initially collect all these changes in one PR, since it was hard to tell what shape things would take, and in fact there have been large sweeping changes that I later undid, which we've now all been saved from.

The question remains, should this PR be split up, and if so, in how many PRs? I would request to not go too crazy on this, this will not be a neat PR no matter how you slice it. Then there's of course the issue that there may be dependencies.

Follow-up to the work in #12658 and #14613

sbc100

Nice!

src/library_wasi.js

src/preamble.js

sbc100 · 2020-11-24T17:59:15Z

src/runtime.js

-  POINTER_SIZE: 4,
-  QUANTUM_SIZE: 4,
+  POINTER_SIZE: getPointerSize(),
+  POINTER_TYPE: MEMORY64 ? 'i64' : 'i32',


get you just call getPointerType() here?

You mean pointerType() from parseTools.js ? Is that always available to this file? I am still unclear on how all these JS files fit together :)

My understanding is that all of these files (modules.py, parseTools.js, runtime.js at least) all loaded together when processing JS so they all shared a single namespace.

I tried, and I can't access variables from parseTools.js here, though I might be getting it wrong :)

src/runtime.js

src/runtime_safe_heap.js

system/lib/libc/musl/src/time/strftime.c

tests/runner.py

sbc100

I don't fully grok the makeGetValue / makeSetValue stuff bit everything else looks good!

system/lib/pthread/library_pthread_stub.c

src/preamble.js

sbc100 · 2020-12-04T21:17:43Z

If you feel like splitting of the Runtime.POINTER_SIZE -> POINTER_SIZE change that could land first? (No worries if not).

sbc100 · 2020-12-04T21:18:17Z

Is this still a draft?

aardappel · 2020-12-07T22:57:55Z

No, I'd rather not split that off.. and yes, still a draft until I get further along I think?

This reflects changes in the function signature in https://reviews.llvm.org/D101985 and also uses `from64` function defined in emscripten-core#12869. This has not been tested against wasm64 yet and I'm uploading this mainly for discussions at this point. Also in the team chat of the tools team we discussed other alternatives to `from64` function, which can be done separately and not reflected here.

There are a lot of places where pointers get cast to int, and to prepare for further Memory64 changes, those are changed to long. I chose long since it is most similar to the existing int, instead of e.g intptr_t, since long is more universal (there are many cases where the actual data stored is not a pointer). Since under wasm32 int and long are equal, this should in theory be a NFC for wasm32, making it easier to review. It also allows #12869 to be smaller

src/library.js

sbc100 · 2021-09-24T00:19:55Z

src/library.js

  // To support such allocations during startup, track them on __heap_base and
  // then when the main module is loaded it reads that value and uses it to
  // initialize sbrk (the main module is relocatable itself, and so it does not
  // have __heap_base hardcoded into it - it receives it from JS as an extern
  // global, basically).
-  __heap_base: '{{{ HEAP_BASE }}}',
+  __heap_base: '{{{ to64(HEAP_BASE) }}}',


Why is this ok to remain as is and not use new WebAssembly.Global but the above ones are not?

This was already not using new WebAssembly.Global before my change.. I have no idea why.

src/library_dylink.js

src/library_exceptions.js

tests/test_core.py

sbc100 · 2021-09-24T01:04:22Z

tools/shared.py

@@ -230,7 +230,7 @@ def run_js_tool(filename, jsargs=[], *args, **kw):
  This is used by emcc to run parts of the build process that are written
  implemented in javascript.
  """
-  command = config.NODE_JS + [filename] + jsargs
+  command = config.NODE_JS + ['--experimental-wasm-bigint'] + [filename] + jsargs


Is this needed for something?

Yes, the JS tools themselves may now contain code that uses bigints?

Is that something you have added in this PR? It seems odd that that would be needed just to process the library js code.. can you point to an example?

I'd have to run 64-bit test for that again to uncover that. Can we leave it in? If it's just for the tools it doesn't hurt. Or if you insist I can put it in a comment so it is easier to re-apply when we return to fixing 64-bit issues.

sbc100 · 2021-09-24T01:07:14Z

tools/gen_struct_info.py

@@ -266,6 +266,7 @@ def inspect_headers(headers, cflags):
                               '-Wno-format',
                               '-nostdlib',
                               compiler_rt,
+                               '-s', 'MEMORY64=' + str(settings.MEMORY64),


I don't think this will work. This gets run just once one first use so we don't want to depend on the settings use in that moment. If there are differences here then we would need a second set of json file and have them stored separately in the cache.

See generate_struct_info in emscripten.py.

Ok, I guess I am not clear on how to fix this.. maybe we can discuss that offline. Or at least maybe I can just put a FIXME in there for when testing with wasm64 resumes.. so far this has worked in allowing tests to pass.

The fix I think would be to have a different filename when running in 64bit mode. See:

emscripten/emscripten.py

Line 826 in bca56b3

generated_struct_info_name = 'generated_struct_info.json'

It won't work as it stands because if you have previously built the struct_info.json with -sMEMORY64 and then try to build something without that flag .. it would use the wasm64 version of the json file.. and things be broken in horrible ways, right?

Ok, so we need a generated_struct_info64.json ? I can look into how to set that up in a follow-up. I'll comment this out for now.

sbc100 · 2021-10-04T17:13:39Z

I mean you can put the quotes outside the `{{{ }}}` and just have `POINTER_TYPE` inside.. avoiding the backticks (I think).

…

On Mon, Oct 4, 2021 at 10:08 AM Wouter van Oortmerssen < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/runtime_safe_heap.js <#12869 (comment)> : > @@ -14,7 +14,7 @@ @param {number|boolean=} noSafe */ function setValue(ptr, value, type, noSafe) { type = type || 'i8'; - if (type.charAt(type.length-1) === '*') type = 'i32'; // pointers are 32-bit + if (type.charAt(type.length-1) === '*') type = {{{ `'${POINTER_TYPE}'` }}}; That would generate i32 and it needs to generate 'i32' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12869 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD55ZN7ZXBWUFMAF3OIOP3UFHNPXANCNFSM4UAGXGBQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

src/parseTools.js

emscripten.py

src/library_glfw.js

src/parseTools.js

src/postamble.js

src/preamble.js

system/lib/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h

system/lib/compiler-rt/lib/ubsan/ubsan_flags.cpp

system/lib/libc/musl/arch/emscripten/bits/alltypes.h

system/lib/libc/musl/src/internal/pthread_impl.h

aardappel · 2021-10-14T18:59:03Z

Ok, what I just pushed is what remains after rebasing on top of the 15 PRs that were split out.

For reference:

failfast flag: Add a failfast command-line option to runner.py #15111
command-line flags: [Memory64] new 64-bit related command-line flags #15218
core JS runtime: [Memory64] Core JS runtime changes for 64-bit #15219
embind / safe heap JS: [Memory64] embind / safe heap JS changes for 64-bit #15220
dylink JS: [Memory64] dylink JS changes for 64-bit #15221
pthread JS: [Memory64] pthread JS changes for 64-bit #15222
syscall JS: [Memory64] syscall JS changes for 64-bit #15223
wasi JS: [Memory64] wasi JS changes for 64-bit #15224
preamble JS: [Memory64] preamble JS changes for 64-bit #15225
embind C++: [Memory64] embind C++ changes for 64-bit #15226
sanitizer C++: [Memory64] sanitizer C++ changes for 64-bit #15227
sanitizer header: [Memory64] sanitizer header changes for 64-bit #15228
pthread libc: [Memory64] pthread libc changes for 64-bit #15229
network libc: [Memory64] network libc changes for 64-bit #15230
pthread rwlock libc: [Memory64] pthread rwlock libc changes for 64-bit #15231

There may be further things you want split out @sbc100 @kripken or is this good to land?

kripken

How about splitting out the strftime and webidl binder changes (so that if we end up bisecting it doesn't reach here)? Otherwise, I think this can stay in one PR.

system/include/emscripten/val.h

kripken · 2021-10-14T19:50:46Z

tests/test_benchmark.py

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


Wasm64 does seem special enough to justify this, I think. And we do have a few other global params here like profiling etc.

kripken · 2021-10-14T19:52:08Z

tools/gen_struct_info.py

+                               # separately in the cache.
+                               # Whereever generated_struct_info.json is generated, there now
+                               # needs to be a generated_struct_info64.json for MEMORY64 mode.
+                               # '-s', 'MEMORY64=' + str(settings.MEMORY64),


I think we'd need a separate cache dir for memory64 (like for LTO), and to pass this tool a flag in that case, but I'm not sure.

Yes, we discussed this should be fixed in a follow-up, see #15218 (and also elsewhere in this PR)

system/include/emscripten/val.h

sbc100 · 2021-10-14T19:51:08Z

system/lib/libc/musl/src/time/strftime.c

 			(tm->__tm_gmtoff)/3600,
-			abs(tm->__tm_gmtoff%3600)/60);
+			labs(tm->__tm_gmtoff%3600)/60); // XXX EMSCRIPTEN: abs => labs


Maybe better to disable that warning rather than hack musl like this. See:

emscripten/tools/system_libs.py

Lines 682 to 688 in dfb2d09

# Disable certain warnings for code patterns that are contained in upstream musl

cflags += ['-Wno-ignored-attributes',

'-Wno-dangling-else',

'-Wno-unknown-pragmas',

'-Wno-shift-op-parentheses',

'-Wno-string-plus-int',

'-Wno-pointer-sign']

Also, it looks like this was removed upstream so this issue will go away once I get around to landing #13006: https://github.com/emscripten-core/musl/

tests/core/test_typeid.cpp

sbc100 · 2021-10-14T19:52:11Z

tests/runner.py

@@ -359,6 +364,7 @@ def set_env(name, option_value):
  set_env('EMTEST_REBASELINE', options.rebaseline)
  set_env('EMTEST_VERBOSE', options.verbose)
  set_env('EMTEST_CORES', options.cores)
+  set_env('EMTEST_FORCE64', options.force64)


This still needs to be resolved.. we shouldn't need the global keyword above.

sbc100 · 2021-10-14T19:52:47Z

tests/test_benchmark.py

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


@kripken WDYT about these changes to test_benchmark.py

tests/test_core.py

kripken · 2021-10-14T21:22:51Z

@sbc100 benchmark changes look good to me. They aren't temporary changes, but a new mode for the runner. Makes sense to commit them I think.

aardappel · 2021-10-14T21:49:05Z

Two more:

webidl_binder: [Memory64] webidl_binder changes for 64-bit #15302
strftime: [Memory64] strftime changes for 64-bit #15303

kripken

Ok, if you'd rather do the getPointerSize() etc. stuff in a followup, that seems ok to me.

kripken · 2021-10-15T17:30:44Z

tests/test_benchmark.py

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


+1 from me for these changes to benchmarking.

This regressed in #12869 - while adding wasm64 support we reordered how the args are added. The extra emcc args must be at the end so that they apply on top of the other ones, in particular, some tests must disable minimal runtime.

This regressed in emscripten-core#12869 - while adding wasm64 support we reordered how the args are added. The extra emcc args must be at the end so that they apply on top of the other ones, in particular, some tests must disable minimal runtime.

aardappel force-pushed the master branch from 9e1c9b4 to ca371bf Compare November 24, 2020 01:51

sbc100 reviewed Nov 24, 2020

View reviewed changes

vadimkantorov mentioned this pull request Nov 25, 2020

Can't build GNU diffutils with emscripten #12879

Open

aardappel marked this pull request as draft December 4, 2020 00:22

aardappel force-pushed the master branch 2 times, most recently from 6e32d6b to f70b244 Compare December 4, 2020 20:46

sbc100 reviewed Dec 4, 2020

View reviewed changes

system/lib/pthread/library_pthread_stub.c Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

src/preamble.js Outdated Show resolved Hide resolved

aardappel force-pushed the master branch from f70b244 to a201904 Compare December 15, 2020 01:37

Base automatically changed from master to main March 8, 2021 23:49

aardappel force-pushed the master branch from a201904 to c3131ca Compare March 29, 2021 18:24

aardappel force-pushed the master branch from c3131ca to d0f6718 Compare May 3, 2021 23:17

aheejin mentioned this pull request May 6, 2021

Support Emscripten EH/SjLj in Wasm64 #14108

Closed

sbc100 mentioned this pull request Jul 2, 2021

Building and Running WASM in -sMEMORY64=1 mode #14582

Closed

aardappel mentioned this pull request Jul 9, 2021

[Memory64] Replace int (used as ptr) with long in tests #14613

Merged

aardappel force-pushed the master branch from d0f6718 to 8ec1015 Compare July 15, 2021 22:44

aardappel force-pushed the master branch from 8ec1015 to 99b1fe4 Compare August 5, 2021 20:24

aardappel force-pushed the master branch 8 times, most recently from 6f99f54 to 79c84d3 Compare September 13, 2021 23:27

aardappel marked this pull request as ready for review September 23, 2021 19:21

sbc100 reviewed Sep 24, 2021

View reviewed changes

sbc100 mentioned this pull request Sep 27, 2021

Where possible, we should move JavaScript to native/wasm code #15151

Open

aardappel force-pushed the master branch 4 times, most recently from f82e4b5 to 5c9a07b Compare September 30, 2021 23:43

aardappel force-pushed the master branch from 5c9a07b to 079db08 Compare October 4, 2021 17:14

juj reviewed Oct 4, 2021

View reviewed changes

src/parseTools.js Outdated Show resolved Hide resolved

aardappel force-pushed the master branch 2 times, most recently from a58191f to babc881 Compare October 4, 2021 19:01

kripken reviewed Oct 4, 2021

View reviewed changes

This was referenced Oct 4, 2021

[Memory64] Core JS runtime changes for 64-bit #15219

Merged

[Memory64] new 64-bit related command-line flags #15218

Merged

aardappel force-pushed the master branch from babc881 to 344f159 Compare October 14, 2021 18:55

kripken reviewed Oct 14, 2021

View reviewed changes

sbc100 reviewed Oct 14, 2021

View reviewed changes

aardappel force-pushed the master branch 2 times, most recently from fdd4d9e to 383146a Compare October 14, 2021 20:52

[wasm64] making JS bindings wasm64 aware

32ca8dc

aardappel force-pushed the master branch from 383146a to 32ca8dc Compare October 14, 2021 21:29

aardappel mentioned this pull request Oct 14, 2021

[Memory64] strftime changes for 64-bit #15303

Closed

kripken approved these changes Oct 15, 2021

View reviewed changes

aardappel merged commit 8d424d2 into emscripten-core:main Oct 15, 2021

kripken mentioned this pull request Nov 18, 2021

Fix benchmark suite #15566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm64] making JS bindings wasm64 aware #12869

[wasm64] making JS bindings wasm64 aware #12869

aardappel commented Nov 24, 2020 •

edited

Loading

sbc100 left a comment

sbc100 Nov 24, 2020

aardappel Dec 4, 2020

sbc100 Dec 4, 2020

aardappel Dec 4, 2020

sbc100 left a comment

sbc100 commented Dec 4, 2020

sbc100 commented Dec 4, 2020

aardappel commented Dec 7, 2020

sbc100 Sep 24, 2021

aardappel Sep 30, 2021

sbc100 Sep 24, 2021

aardappel Sep 30, 2021

sbc100 Sep 30, 2021

aardappel Oct 4, 2021

sbc100 Sep 24, 2021

aardappel Sep 30, 2021

sbc100 Sep 30, 2021

aardappel Oct 4, 2021

sbc100 commented Oct 4, 2021 via email

aardappel commented Oct 14, 2021

kripken left a comment

kripken Oct 14, 2021

kripken Oct 14, 2021

aardappel Oct 14, 2021

sbc100 Oct 14, 2021

sbc100 Oct 14, 2021

sbc100 Oct 14, 2021

kripken commented Oct 14, 2021

aardappel commented Oct 14, 2021

kripken left a comment

kripken Oct 15, 2021

	# Disable certain warnings for code patterns that are contained in upstream musl
	cflags += ['-Wno-ignored-attributes',
	'-Wno-dangling-else',
	'-Wno-unknown-pragmas',
	'-Wno-shift-op-parentheses',
	'-Wno-string-plus-int',
	'-Wno-pointer-sign']

[wasm64] making JS bindings wasm64 aware #12869

[wasm64] making JS bindings wasm64 aware #12869

Conversation

aardappel commented Nov 24, 2020 • edited Loading

sbc100 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

sbc100 left a comment

Choose a reason for hiding this comment

sbc100 commented Dec 4, 2020

sbc100 commented Dec 4, 2020

aardappel commented Dec 7, 2020

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

sbc100 commented Oct 4, 2021 via email

aardappel commented Oct 14, 2021

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

kripken commented Oct 14, 2021

aardappel commented Oct 14, 2021

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented Nov 24, 2020 •

edited

Loading