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

Return value from wasm module via WASI #32093

Closed
jendrikw opened this issue Mar 4, 2020 · 18 comments · Fixed by #32101
Closed

Return value from wasm module via WASI #32093

jendrikw opened this issue Mar 4, 2020 · 18 comments · Fixed by #32101
Labels
wasi Issues and PRs related to the WebAssembly System Interface.

Comments

@jendrikw
Copy link

jendrikw commented Mar 4, 2020

Is your feature request related to a problem? Please describe.
I have a WebAssembly file compiled from a simple C++ program. I would like to get the return value from main. I am calling the function via wasm.start()

Describe the solution you'd like
start() should return the value from _start() and/or __wasi_unstable_reactor_start.

Describe alternatives you've considered
The WASI object could also store the return value and make it available via a getter function, but I think the solution via the return value cleaner.

@devsnek devsnek added the wasi Issues and PRs related to the WebAssembly System Interface. label Mar 4, 2020
@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

those functions don't have a return value.

@jendrikw
Copy link
Author

jendrikw commented Mar 4, 2020

Oh, I somehow thought they did. Could there be an alternative way?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

what are you trying to achieve?

@jendrikw
Copy link
Author

jendrikw commented Mar 4, 2020

I'm trying to use this simple C program that detects if a number is even:

#include <stdlib.h>

#define WASM_EXPORT __attribute__((visibility("default")))

WASM_EXPORT
int main(int argc, char *argv[]) {
  char *rest;
  long i = strtol(argv[1], &rest, 10);
  return i % 2;
}

I compiled it with wasi-sdk. It works fine when running with wasmtime and I get the intended return value. I expected it to be usable with node. Or have I misunderstood something?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

@jendrikw if main returns non-zero it will exit the whole node program. kind of a weird design atm.

also fwiw, you don't need to set the visibility of main (it shouldn't be exported anyway)

@cjihrig perhaps we should have better behaviour for when __wasi_proc_exit is called?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

also this is basically what _start does:

void _start(void) {
  int r = main();
  if (r != 0) { __wasi_proc_exit(r); }
}

@guybedford
Copy link
Contributor

Yeah this definitely seems odd - can start just not return the code directly?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

_start is doing the correct thing. The odd thing is that we don't scope __wasi_proc_exit to the WASI instance.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 4, 2020

__wasi_proc_exit() terminates the process unconditionally because if it doesn't, the result will be a failed assertion in the wasm runtime (__wasi_proc_exit() is not allowed to return).

@guybedford
Copy link
Contributor

Right, but there is no reason wasi.start() can't return the exit code that __wasi_proc_exit() stores as a side effect.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 4, 2020

I agree, and that would be preferable. But that's not currently in our control.

@guybedford
Copy link
Contributor

Can you clarify why that is the case? wasi.start() is very much a Node.js-specific API?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

@cjihrig indeed... i'm just saying it should not exit the node process. for the moment we can throw an exception, since wasm can't catch them yet.

@guybedford __wasi_proc_exit can happen at any point, we should probably represent it as some sort of event, not a return value.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 4, 2020

@guybedford wasi.start() just calls into wasm here. Once inside of wasm, there is an eventual call to __wasi_proc_exit() (implemented as uvwasi_proc_exit() in node). That exits the process in C code. If __wasi_proc_exit() returns, there will be a failed assertion, that is also out of our control.

I think the proper fix would need to happen in the wasi-libc. We could also try @devsnek's suggestion of throwing an exception (by overwriting the __wasi_proc_exit() with JavaScript) in the short term.

@guybedford
Copy link
Contributor

There is no need for a return value from __wasi_proc_exit() - from a VM perspective, one can consider there should be a "process state" which represents the state of the process (in progress, completed, code, etc). __wasi_proc_exit can manipulate this state and wasi.start() can return this state. That way there can also be assertions that it can only run once etc etc.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 4, 2020

Thinking of these WASI apps as nanoprocesses (I think that's what the WASI folks are calling them), I think having an exit code would be useful for the same reasons real processes have exit codes. I think that could be discussed separately from this issue though. IMO, the real problem here is that __wasi_proc_exit() terminates the process unconditionally, which means that you can't use Node to orchestrate multiple WASI applications without using a child process.

@devsnek
Copy link
Member

devsnek commented Mar 4, 2020

It's worth noting, __wasi_proc_exit is in an odd place atm, and how it works will likely change in the future (or it will be removed). For the moment, just making it not exit the entire process is enough.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 4, 2020

@devsnek I'll look into this tonight.

@cjihrig cjihrig closed this as completed in 6aff62f Mar 8, 2020
MylesBorins pushed a commit that referenced this issue Mar 9, 2020
This commit adds a WASI option allowing the __wasi_proc_exit()
function to return an exit code instead of forcefully terminating
the process.

PR-URL: #32101
Fixes: #32093
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
This commit adds a WASI option allowing the __wasi_proc_exit()
function to return an exit code instead of forcefully terminating
the process.

PR-URL: nodejs#32101
Fixes: nodejs#32093
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this issue Apr 28, 2020
This commit adds a WASI option allowing the __wasi_proc_exit()
function to return an exit code instead of forcefully terminating
the process.

PR-URL: #32101
Fixes: #32093
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants