handle reexported wasm globals #7467
Conversation
|
For maintainers only:
|
lib/wasm/WebAssemblyGenerator.js
Outdated
| @@ -225,7 +227,7 @@ const rewriteImportedGlobals = state => bin => { | |||
|
|
|||
| node.init = [ | |||
| // Poisong globals, they are meant to be rewritten | |||
| t.objectInstruction("const", valtype, [t.numberLiteralFromRaw(666)]) | |||
| t.objectInstruction("const", valtype, [t.numberLiteralFromRaw(0)]) | |||
There was a problem hiding this comment.
That was useful for testing, do you think we shouldn't do it? (also comment above)
There was a problem hiding this comment.
It increases the size of the wasm. But we can use 66.
| }) | ||
| ); | ||
| }; | ||
| let needExportsCopy = false; |
There was a problem hiding this comment.
It's not really a copy? It's more a rewire or a connection
There was a problem hiding this comment.
This variable is about a shallow copy of the exports object.
If false it can use module.exports = instance.exports.
If true it has to use for(var key in instance.exports) exports[key] = instance.exports[key]
There was a problem hiding this comment.
95c2f7d to
1e4d2b7
Compare
lib/wasm/WebAssemblyGenerator.js
Outdated
| @@ -403,14 +411,20 @@ class WebAssemblyGenerator extends Generator { | |||
| const nextTypeIndex = getNextTypeIndex(ast); | |||
|
|
|||
| const usedDependencyMap = getUsedDependencyMap(module); | |||
| const invalidExports = new Set( | |||
There was a problem hiding this comment.
invalidExports sounds confusing to me, these are the exports that are handled by Webpack now, right? Maybe something like exportsToRemove?
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
| (import "./env.js" "n" (global i32)) | ||
| (import "./env.js" "m" (global $g2 f64)) | ||
| (export "v" (global 0)) | ||
| (global $g i32 (get_global 0)) |
There was a problem hiding this comment.
I don't understand why this is valid. Doesn't it break WebAssembly validation to have a non-constant initialization expression, per https://webassembly.github.io/spec/core/bikeshed/index.html#globals%E2%91%A2 ?
There was a problem hiding this comment.
Oh, right, I'll dig further into figuring out why the test doesn't pass in Node 11.
There was a problem hiding this comment.
which test? (sorry I'm just catching up here)
There was a problem hiding this comment.
Running this test, modified to check for WebAssembly.Global objects, against a Node 11 nightly build, failed for me. My next step is to see if I can make a smaller reproduction directly in V8.
There was a problem hiding this comment.
OK, I was just confused. V8 supports this just fine (it's tested in test/mjsunit/wasm/globals.js TestImportedExported), and now I'm wondering if the issue is actually the logic in this patch to improve handling of re-exported globals is responsible for getting a Number rather than a WebAssembly.Global. I'm wondering, for systems that have WebAssembly.Global, should we just wrap the Number in a Global?
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
yes
Does this PR introduce a breaking change?
no
What needs to be documented once your changes are merged?
nothing