-
Notifications
You must be signed in to change notification settings - Fork 839
WIP: Support for mutable global imports/exports #1644
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1754,7 +1754,7 @@ void SExpressionWasmBuilder::parseImport(Element& s) { | |
| auto& inner2 = *inner[j]; | ||
| if (inner2[0]->str() != MUT) throw ParseException("expected mut"); | ||
| im->globalType = stringToType(inner2[1]->str()); | ||
| throw ParseException("cannot import a mutable global", s.line, s.col); | ||
| im->mutable_ = true; | ||
| } | ||
| } else if (im->kind == ExternalKind::Table) { | ||
| if (j < inner.size() - 1) { | ||
|
|
@@ -1824,14 +1824,15 @@ void SExpressionWasmBuilder::parseGlobal(Element& s, bool preParseImport) { | |
| if (importModule.is()) { | ||
| // this is an import, actually | ||
| if (!preParseImport) throw ParseException("!preParseImport in global"); | ||
| if (mutable_) throw ParseException("cannot import a mutable global", s.line, s.col); | ||
| std::unique_ptr<Import> im = make_unique<Import>(); | ||
| im->name = global->name; | ||
| im->module = importModule; | ||
| im->base = importBase; | ||
| im->kind = ExternalKind::Global; | ||
| im->globalType = type; | ||
| if (wasm.getImportOrNull(im->name)) throw ParseException("duplicate import", s.line, s.col); | ||
| im->mutable_ = mutable_; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. testing revealed I missed adding the support to this file. |
||
| if (wasm.getImportOrNull(im->name)) | ||
| throw ParseException("duplicate import", s.line, s.col); | ||
| wasm.addImport(im.release()); | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| (module | ||
| (import "env" "memory" (memory $0 256 256)) | ||
| (import "env" "table" (table 0 0 anyfunc)) | ||
| (import "env" "memoryBase" (global $memoryBase i32)) | ||
| (import "env" "tableBase" (global $tableBase i32)) | ||
| (import "env" "memoryBase" (global $memoryBase (mut i32))) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kripken It's not clear why all these changed to mutable. Perhaps this is revealing a bug I introduced?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! Is it because those asmjs globals were actually mutable but previously we couldn’t represent that?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting - I think that's a coincidence actually, but a good one here as it pointed out something useful :) What I think is going on is that we forgot an initial value for It's also a good question what we should do for asm.js imports. Yeah, in asm.js they are mutable, and to work around that in wasm we created a mutable global with the initial value of the import, then we just use that global from then on. In theory we could simplify that to use a mutable import, however, I think it would be more complicated than it would be worth since we do want the old form supported too (and also a mutable import may have perf overhead). |
||
| (import "env" "tableBase" (global $tableBase (mut i32))) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| (module | ||
| (import "env" "memory" (memory $0 256 256)) | ||
| (import "env" "table" (table 0 0 anyfunc)) | ||
| (import "env" "memoryBase" (global $memoryBase i32)) | ||
| (import "env" "tableBase" (global $tableBase i32)) | ||
| (import "env" "memoryBase" (global $memoryBase (mut i32))) | ||
| (import "env" "tableBase" (global $tableBase (mut i32))) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| (module | ||
| (import "env" "memory" (memory $0 256 256)) | ||
| (import "env" "table" (table 0 0 anyfunc)) | ||
| (import "env" "memoryBase" (global $memoryBase i32)) | ||
| (import "env" "tableBase" (global $tableBase i32)) | ||
| (import "env" "memoryBase" (global $memoryBase (mut i32))) | ||
| (import "env" "tableBase" (global $tableBase (mut i32))) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| (module | ||
| (type $0 (func)) | ||
| (import "env" "global-mut" (global $global-mut (mut i32))) | ||
| (func $foo (type $0) | ||
| (set_global $global-mut | ||
| (i32.add | ||
| (get_global $global-mut) | ||
| (i32.const 1) | ||
| ) | ||
| ) | ||
| ) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| (module | ||
| (type $0 (func)) | ||
| (import "env" "global-mut" (global $global-mut (mut i32))) | ||
| (func $foo (; 0 ;) (type $0) | ||
| (set_global $global-mut | ||
| (i32.add | ||
| (get_global $global-mut) | ||
| (i32.const 1) | ||
| ) | ||
| ) | ||
| ) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| (module | ||
| (type $0 (func)) | ||
| (import "env" "global-mut" (global $gimport$0 (mut i32))) | ||
| (func $foo (; 0 ;) (type $0) | ||
| (set_global $gimport$0 | ||
| (i32.add | ||
| (get_global $gimport$0) | ||
| (i32.const 1) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| (module | ||
| (type $0 (func)) | ||
| (import "env" "global-mut" (global $gimport$0 (mut i32))) | ||
| (func $0 (; 0 ;) (type $0) | ||
| (set_global $gimport$0 | ||
| (i32.add | ||
| (get_global $gimport$0) | ||
| (i32.const 1) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| (module | ||
| (import "env" "memory" (memory $0 256 256)) | ||
| (import "env" "memoryBase" (global $memoryBase i32)) | ||
| (import "env" "memoryBase" (global $memoryBase (mut i32))) | ||
| (data (get_global $memoryBase) "use-import-and-drop.asm.js") | ||
| ) |
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 feels dirty but seemed like a simple solution without deciding to completely rethink about imports/globals are represented
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 me. Might be nice to eventually think about a
unionor other cleaner approach, but Imports are not that common, so I'm not worried about it.