Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Required API: WASI.prototype.setMemory() #7

Closed
cjihrig opened this issue Sep 16, 2019 · 6 comments
Closed

Required API: WASI.prototype.setMemory() #7

cjihrig opened this issue Sep 16, 2019 · 6 comments

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Sep 16, 2019

As discussed a bit in this thread.

Currently, the memory is passed to the WASI constructor in JS, and set in the bindings here.

This needs to be extracted into a separate function so that the following sequence of operations is supported:

  • Construct WASI instance.
  • Use WASI instance's import object to create a WebAssembly.Instance.
  • Get the memory from the WebAssembly.Instance and associate it with the WASI instance.
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2019

After spending some time on this, and thinking about #4 (comment), I think this is the wrong approach.

The memory doesn't need to be part of the JS WASI instance at all. The memory should be passed to WASI.start() (or whatever ends up calling the _start() export). In the C++ layer, the underlying char* can be grabbed from the memory before calling _start() in JS. Storing the ArrayBuffer itself doesn't seem to work because once the WASI command starts to execute, the ArrayBuffer is detached. This means the syscalls lose access to the backing store.

@devsnek
Copy link
Member

devsnek commented Sep 17, 2019

the memory has to be updated at the start of every wasi function that will access the memory: https://github.com/devsnek/node-wasi/blob/master/src/index.js#L599

this will be fixed in the future by passing memory views directly via interface types.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2019

@devsnek the problem I'm encountering is that when I try to access the ArrayBuffer in C++ in one of the WASI system calls, the AB has become external, the data is a null pointer, and the byte length is zero. Prior to calling _start(), that is not the case. The other interesting part is that this only happens with memory coming from the wasm instance. If I pass in my own AB, that does not happen.

@devsnek
Copy link
Member

devsnek commented Sep 17, 2019

this happens when the memory is resized (that's why i have a test that specifically just calls memory.grow). You have to refresh your view on the wasm's memory every time it calls out because it might have resized the memory.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2019

If I'm understanding you correctly, that means this object must be the WebAssembly.Memory object, and not the underlying AB. Is that accurate?

EDIT: Based on https://webassembly.github.io/spec/js-api/#dom-memory-grow, I think that's accurate.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 18, 2019

This should be resolved in 4a5f984. Thanks @devsnek for the help here.

@cjihrig cjihrig closed this as completed Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants