Skip to content
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

@swc/wasm-typescript is failing on some architectures #9259

Closed
kdy1 opened this issue Jul 16, 2024 · 21 comments
Closed

@swc/wasm-typescript is failing on some architectures #9259

kdy1 opened this issue Jul 16, 2024 · 21 comments
Assignees
Milestone

Comments

@kdy1
Copy link
Member

kdy1 commented Jul 16, 2024

          @kdy1 it seems tests are failing on some architecture such as:
  • s390x
  • ppc64
19:22:06       code: 'ERR_ASSERTION'
19:22:06       name: 'AssertionError'
19:22:06       expected: ''
19:22:06       actual: |-
19:22:06         wasm://wasm/0072633a:1
19:22:06         
19:22:06         
19:22:06         RuntimeError: memory access out of bounds
19:22:06             at wasm://wasm/0072633a:wasm-function[1521]:0x1a6744
19:22:06             at wasm://wasm/0072633a:wasm-function[51]:0x60118
19:22:06             at wasm://wasm/0072633a:wasm-function[1264]:0x1a32e8
19:22:06             at module.exports.transformSync (node:internal/deps/swc/wasm:248:14)
19:22:06             at tsParse (node:internal/modules/helpers:310:18)
19:22:06             at node:internal/main/eval_string:27:3

Originally posted by @marco-ippolito in nodejs/node#53725 (comment)

@kdy1 kdy1 added this to the Planned milestone Jul 16, 2024
@kdy1 kdy1 self-assigned this Jul 16, 2024
@magic-akari
Copy link
Member

I suspect there might be an issue with wasm-bindgen.

Note

Note: WebAssembly memory is always in little-endian format, regardless of the platform it's run on. Therefore, for portability, you should read and write multi-byte values in JavaScript using DataView.

https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/Memory

@miladfarca
Copy link

Regarding the failures on powerpc, is it only on AIX or it includes Linux on little endian as well?

If it's only a problem on big endian then could you tell me if emscripten was used to generated the wasm code and it's JavaScript glue code?

We have previously patched emscripten to handle big endian platforms here emscripten-core/emscripten#13413 , you will need to add SUPPORT_BIG_ENDIAN=1 during compilation to apply BE patches.
We also maintain V8 as it's runtime to make sure BE platforms are supported.

@marco-ippolito
Copy link
Contributor

This is the list of failures, windows and osx seems unrelated
image

@miladfarca
Copy link

miladfarca commented Jul 16, 2024

Thanks, so plinux (being little endian) seems green. How is the wasm file with its js glue code generated?

@magic-akari
Copy link
Member

swc-binding_typescript_wasm-1.6.13.tgz

Could you try this package? @marco-ippolito
I made some manual adjustments. If it works, we need to correct the output of wasm-bindgen.

@marco-ippolito
Copy link
Contributor

swc-binding_typescript_wasm-1.6.13.tgz

Could you try this package? @marco-ippolito I made some manual adjustments. If it works, we need to correct the output of wasm-bindgen.

can you please publish it as a nightly release?

@kdy1
Copy link
Member Author

kdy1 commented Jul 17, 2024

@marco-ippolito I published it as @swc/binding_typescript_wasm@1.7.0-rc.0

@miladfarca
Copy link

how is rust compiled to wasm? is emscripten used or rust has its own internal compiler and needs to use wasm-bindgen afterwards?

@kdy1
Copy link
Member Author

kdy1 commented Jul 17, 2024

We use wasm-bindgen and wasm-pack.

export CARGO_PROFILE_RELEASE_LTO="fat"
export CARGO_PROFILE_RELEASE_OPT_LEVEL="z"
wasm-pack build --out-name wasm --release --scope=swc --target nodejs
ls -al ./pkg

@marco-ippolito
Copy link
Contributor

marco-ippolito commented Jul 17, 2024

It seems the fix release in the nightly is working on such architectures, but there is this failure when node is built without ICU

15:16:04       failureType: 'testCodeFailure'
15:16:04       error: |-
15:16:04         Expected values to be strictly equal:
15:16:04         + actual - expected
15:16:04         
15:16:04         + 'node:internal/encoding:487\n' +
15:16:04         +   `          throw new ERR_NO_ICU('"fatal" option');\n` +
15:16:04         +   '          ^\n' +
15:16:04         +   '\n' +
15:16:04         +   'TypeError [ERR_NO_ICU]: "fatal" option is not supported on Node.js compiled without ICU\n' +
15:16:04         +   '    at new TextDecoder (node:internal/encoding:487:17)\n' +
15:16:04         +   '    at node:internal/deps/swc/wasm:90:25\n' +
15:16:04         +   '    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:399:7)\n' +
15:16:04         +   '    at requireBuiltin (node:internal/bootstrap/realm:430:14)\n' +
15:16:04         +   '    at lazyLoadTSParser (node:internal/modules/helpers:304:15)\n' +
15:16:04         +   '    at tsParse (node:internal/modules/helpers:309:17)\n' +
15:16:04         +   '    at node:internal/main/eval_string:27:3 {\n' +
15:16:04         +   "  code: 'ERR_NO_ICU'\n" +
15:16:04         +   '}\n' +
15:16:04         +   '\n' +
15:16:04         +   'Node.js v23.0.0-pre\n'

I guess TextDecoder cannot be used without full-icu

@magic-akari
Copy link
Member

It seems the fix release in the nightly is working on such architectures, but there is this failure when node is built without ICU

15:16:04       failureType: 'testCodeFailure'
15:16:04       error: |-
15:16:04         Expected values to be strictly equal:
15:16:04         + actual - expected
15:16:04         
15:16:04         + 'node:internal/encoding:487\n' +
15:16:04         +   `          throw new ERR_NO_ICU('"fatal" option');\n` +
15:16:04         +   '          ^\n' +
15:16:04         +   '\n' +
15:16:04         +   'TypeError [ERR_NO_ICU]: "fatal" option is not supported on Node.js compiled without ICU\n' +
15:16:04         +   '    at new TextDecoder (node:internal/encoding:487:17)\n' +
15:16:04         +   '    at node:internal/deps/swc/wasm:90:25\n' +
15:16:04         +   '    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:399:7)\n' +
15:16:04         +   '    at requireBuiltin (node:internal/bootstrap/realm:430:14)\n' +
15:16:04         +   '    at lazyLoadTSParser (node:internal/modules/helpers:304:15)\n' +
15:16:04         +   '    at tsParse (node:internal/modules/helpers:309:17)\n' +
15:16:04         +   '    at node:internal/main/eval_string:27:3 {\n' +
15:16:04         +   "  code: 'ERR_NO_ICU'\n" +
15:16:04         +   '}\n' +
15:16:04         +   '\n' +
15:16:04         +   'Node.js v23.0.0-pre\n'
// `ignoreBOM` is needed so that the BOM will be preserved when sending a string from Rust to JS
// `fatal` is needed to catch any weird encoding bugs when sending a string from Rust to JS

@magic-akari
Copy link
Member

TextDecoder is used for decoding strings from Rust.
Perhaps we could simply eliminate the fatal option.
What do you think? @kdy1

@kdy1
Copy link
Member Author

kdy1 commented Jul 17, 2024

I agree and I left a question at rustwasm/wasm-bindgen#1730 (comment)

For now, I think we can do some string replace in patch.mjs. We already do it.

const patchedJsFile = origJsFile
.replace(`const path = require('path').join(__dirname, 'wasm_bg.wasm');`, '')
.replace(`const bytes = require('fs').readFileSync(path);`, `
const { Buffer } = require('node:buffer');
const bytes = Buffer.from('${base64}', 'base64');`)

@kdy1
Copy link
Member Author

kdy1 commented Jul 17, 2024

I published 1.7.0-rc.1, which includes manual fix of fatal option by @magic-akari.
@marco-ippolito Can you try this version?

@marco-ippolito
Copy link
Contributor

It seems to be working! CI is green, (only windows on arm seems to be flaky but its probably unrelated) @kdy1 thank you sooo much 🙏🏼

@kdy1
Copy link
Member Author

kdy1 commented Jul 18, 2024

@marco-ippolito Can you try @swc/wasm-typescript@1.7.0? We applied necessary patches to the package.

@marco-ippolito
Copy link
Contributor

marco-ippolito commented Jul 18, 2024

Updating breaks it

    
    '\n' +
      '  #  /Users/marcoippolito/Documents/projects/forks/node/out/Release/node[63807]: void node::contextify::CompileFunctionForCJSLoader(const FunctionCallbackInfo<Value> &) at ../../src/node_contextify.cc:1648\n' +
      '  #  Assertion failed: args[0]->IsString()\n' +
      '\n' +
      '----- Native stack trace -----\n' +
      '\n' +
      ' 1: 0x1027ca020 node::Assert(node::AssertionInfo const&) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 2: 0x104561a7c node::contextify::CompileFunctionForCJSLoader(v8::FunctionCallbackInfo<v8::Value> const&) (.cold.6) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 3: 0x1027bab40 node::contextify::CompileFunctionForCJSLoader(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 4: 0x10354f118 Builtins_CallApiCallbackGeneric [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 5: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 6: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 7: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 8: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      ' 9: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '10: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '11: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '12: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '13: 0x10354cef0 Builtins_InterpreterEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '14: 0x10354ac0c Builtins_JSEntryTrampoline [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '15: 0x10354a8f4 Builtins_JSEntry [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '16: 0x102b4667c v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '17: 0x102b45fd4 v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '18: 0x1029f5370 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '19: 0x1027a69f4 node::builtins::BuiltinLoader::CompileAndCall(v8::Local<v8::Context>, char const*, node::Realm*) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '20: 0x102854d4c node::Realm::ExecuteBootstrapper(char const*) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '21: 0x102785c90 node::StartExecution(node::Environment*, char const*) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '22: 0x102785be4 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '23: 0x1026dd6f4 node::LoadEnvironment(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>, std::__1::function<void (node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>)>) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '24: 0x102810d00 node::NodeMainInstance::Run() [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '25: 0x102789070 node::Start(int, char**) [/Users/marcoippolito/Documents/projects/forks/node/out/Release/node]\n' +
      '26: 0x18819e0e0 start [/usr/lib/dyld]\n' +
      '\n' +
      '----- JavaScript stack trace -----\n' +
      '\n' +
      '1: wrapSafe (node:internal/modules/cjs/loader:1458:18)\n' +
      '2: Module._compile (node:internal/modules/cjs/loader:1480:20)\n' +
      '3: loadTS (node:internal/modules/cjs/loader:1667:10)\n' +
      '4: Module.load (node:internal/modules/cjs/loader:1313:32)\n' +
      '5: Module._load (node:internal/modules/cjs/loader:1123:12)\n' +
      '6: traceSync (node:diagnostics_channel:315:14)\n' +
      '7: wrapModuleLoad (node:internal/modules/cjs/loader:217:24)\n' +
      '8: executeUserEntryPoint (node:internal/modules/run_main:166:5)\n' +
      '9: node:internal/main/run_main_module:30:49\n' +
      '\n' +
      '\n'

it breaks this check https://github.com/nodejs/node/blob/014dad5953a632f44e668f9527f546c6e1bb8b86/src/node_contextify.cc#L1648
The source is expected to be a string, while now the transformSync function returns something else.
Maybe because its not const { code } = transformSync
NVM it was caused by this change, I'll run the ci and let you know

@kdy1
Copy link
Member Author

kdy1 commented Jul 18, 2024

Yeah, it was inevitable, as we now may have to emit a source map. Sorry, I forgot to tell you.

@marco-ippolito
Copy link
Contributor

I can confirm it works fine

@kdy1 kdy1 modified the milestones: Planned, v1.7.1 Jul 24, 2024
@kdy1
Copy link
Member Author

kdy1 commented Jul 24, 2024

Closing as @swc/wasm-typescript@1.7.1 is published

@kdy1 kdy1 closed this as completed Jul 24, 2024
@swc-bot
Copy link
Collaborator

swc-bot commented Aug 23, 2024

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

5 participants