binaryen
binaryen copied to clipboard
WebAssembly
Reference types crash wasm-opt‘s asyncify pass
(module
(type (;0;) (func))
(type (;1;) (func (result externref)))
(import "gl" "createProgram" (func $src/asc/gl/createProgram (type 1)))
(func $src/asc/lemonsoda/init (type 0)
call $src/asc/gl/createProgram
drop
)
(export "init" (func $src/asc/lemonsoda/init))
)
$ wat2wasm --enable-reference-types test.wat -o test.wasm
$ wasm-opt --asyncify --enable-reference-types test.wasm -o test2.wasm
invalid type
UNREACHABLE executed at /tmp/binaryen-20210320-71286-1g5tfwr/binaryen-version_100/src/wasm/wasm-type.cpp:488!
[1] 56081 abort wasm-opt --asyncify --enable-reference-types test.wasm -o test2.wasm
It seems to be specifically about using externref in imported functions. Both an externref parameter and return type will trigger this error, but not for an exported function.
FWIW, this is not limited to externref, it happens for all reference types since they can't be stored on memory.
Just out of curiosity, @kripken would this entail setting the reftype value on a table and store the index instead?
@martianboy Ah of course! That makes a lot of sense.
Since the reference types proposal also allows having multiple tables, it seems like your idea with the table is a really good one.
@surma Right, although this cannot happen yet in Binaryen, as it still doesn't have table operations and typed tables. But it shouldn't be too difficult to support reference types in Asyncify after those are done. I may still be missing something though.
I think that's correct @martianboy & @surma. Yes, the issue is writing references to linear memory. Instead we would need to create a new table in asyncify, and use that to spill values using table.get/table.set, etc., so as mentioned above we'd need support for those features.
Thanks for confirmation, @kripken. I'm in the middle of adding types to tables and element segments and after that I was planning to add table operations. I can see if I can fix Asyncify next. :)
Sounds great @martianboy ! I hope that's pretty straightforward when you get to it (only nontrivial part might be to have two "stack pointers"), let me know if you have questions I can help with.
Not to be that impatient dude on GH, but curious if there are any updates on this?
@surma So I'm "almost" done with implementation of table instructions which was a prerequisite. My guess is that I will be able to get to Asyncify in about 10 days. Sorry for the wait. :)
Hey @martianboy, just wanted to gently ask if you had any time to get to this? 🙇♂️
@kripken looks like the problem is still exists, can we elaborate a little bit more on how it should be solved, I'd be happy to spend time for this. any real blockers, or just a matter of implementation?
@alexdoesh I think we know how it should work, looks like a summary is in
https://github.com/WebAssembly/binaryen/issues/3739#issuecomment-808330306
In more detail, we spill stuff to a buffer in linear memory normally, in the existing code. For reference types we need a buffer in a table that we can write references to. So we'd need to add such a table to the wasm, and add some code that manages allocating buffers in it, and then add code to allocate the buffer and read/write from it (that is, if a local is a reference, we'd write to the table instead of linear memory).
If you want to do it that would be great!
@kripken I've drafted a very direct/naive approach https://github.com/nxpub/binaryen/commit/449dd409d856311ecb9b68763de379e253c43d45 (will make a PR when have a proper draft) just to verify the logic. I'll rewrite the way table(s) is generated, will add support for other kind of refs and polish it properly.
Did I get the idea right, or I misunderstood the approach in general?
The sample from the first comment:
(module
(type (;0;) (func))
(type (;1;) (func (result externref)))
(import "gl" "createProgram" (func $src/asc/gl/createProgram (type 1)))
(func $src/asc/lemonsoda/init (type 0)
call $src/asc/gl/createProgram
drop
)
(export "init" (func $src/asc/lemonsoda/init))
)
generates:
(module
(type (;0;) (func))
(type (;1;) (func (param i32)))
(type (;2;) (func (result externref)))
(type (;3;) (func (result i32)))
(import "gl" "createProgram" (func (;0;) (type 2)))
(func (;1;) (type 0)
(local externref externref i32 i32 i32 i32)
global.get 0
i32.const 2
i32.eq
if ;; label = @1
nop
global.get 1
i32.load
local.set 4
i32.const 0
table.get 0
local.set 0
end
block (result i32) ;; label = @1
block ;; label = @2
block ;; label = @3
global.get 0
i32.const 2
i32.eq
if ;; label = @4
global.get 1
global.get 1
i32.load
i32.const -4
i32.add
i32.store
global.get 1
i32.load
i32.load
local.set 3
end
block ;; label = @4
global.get 0
i32.const 0
i32.eq
if (result i32) ;; label = @5
i32.const 1
else
local.get 3
i32.const 0
i32.eq
end
if ;; label = @5
call 0
local.set 1
global.get 0
i32.const 1
i32.eq
if ;; label = @6
i32.const 0
br 5 (;@1;)
else
local.get 1
local.set 0
end
end
global.get 0
i32.const 0
i32.eq
if ;; label = @5
local.get 0
drop
end
end
end
return
end
unreachable
end
local.set 2
block ;; label = @1
global.get 1
i32.load
local.get 2
i32.store
global.get 1
global.get 1
i32.load
i32.const 4
i32.add
i32.store
end
block ;; label = @1
global.get 1
i32.load
local.set 5
i32.const 0
local.get 0
table.set 0
nop
end)
(func (;2;) (type 1) (param i32)
i32.const 1
global.set 0
local.get 0
global.set 1
global.get 1
i32.load
global.get 1
i32.load offset=4
i32.gt_u
if ;; label = @1
unreachable
end)
(func (;3;) (type 0)
i32.const 0
global.set 0
global.get 1
i32.load
global.get 1
i32.load offset=4
i32.gt_u
if ;; label = @1
unreachable
end)
(func (;4;) (type 1) (param i32)
i32.const 2
global.set 0
local.get 0
global.set 1
global.get 1
i32.load
global.get 1
i32.load offset=4
i32.gt_u
if ;; label = @1
unreachable
end)
(func (;5;) (type 0)
i32.const 0
global.set 0
global.get 1
i32.load
global.get 1
i32.load offset=4
i32.gt_u
if ;; label = @1
unreachable
end)
(func (;6;) (type 3) (result i32)
global.get 0)
(table (;0;) 0 0 externref)
(memory (;0;) 1 1)
(global (;0;) (mut i32) (i32.const 0))
(global (;1;) (mut i32) (i32.const 0))
(export "init" (func 1))
(export "asyncify_start_unwind" (func 2))
(export "asyncify_stop_unwind" (func 3))
(export "asyncify_start_rewind" (func 4))
(export "asyncify_stop_rewind" (func 5))
(export "asyncify_get_state" (func 6)))
@alexdoesh
Yes, that looks like the right idea!
Aside from polish and the other stuff you already mentioned, the only other thing is I wonder if we want to support more than one pause at a time. We do that currently, and in Emscripten the Fibers API uses it. To support that here, we couldn't use absolute offsets in the table, but would need to allocate regions in it, which would add significant complexity. Also I think atm the user allocates the main buffer in linear memory, but here we would need to do it in Asyncify (since the user isn't even aware of the new table added here). It's probably best to not do that for now, but we should document the limitation, and perhaps there's a way we can assert at runtime.
@kripken I had the feeling I'm not handling it properly, makes more sense now, thank you.
As I understand, we already have a helper to diagnose how many local variables (potentially affected by stack rewind) we have, I wanted to use it (or similar approach) anyway, it would allow us to understand the size of the region + what tables we would need (per ref-type) + some logic to "isolate" region per function. Does it seem enough? Or I misunderstood the meaning of "regions" in the ctx?
Do you think it's ok for asyncify pass to create a table if this would be explicit to the end user? Using a command line argument, for example (--reference-types-support or smth like this, and/or ability to provide a tables name prefix).
I'm not a C++ dev (I'm writing python compiler to wasm in python, inspired by asc), but still want to tackle the problem (if somebody would be patient to assist/review ofc), it looks pretty important at least for my pet project.
@alexdoesh Yes, I think the current approach is enough for now. Just a comment mentioning the multi-pause issue sounds good.
Yes, I think it's the right thing to create a table here. We can do that automatically/silently in this pass. That should not be observable to the user (at least not unless they read the wasm :smile: ) since the table is not exported or imported.
I think with some cleanup that code can land. Good work!
Any updates on this? @alexdoesh i tried your fork but it doesnt seem to work? Am I missing something here?
