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

Optimization O3 fails when importing Wasi functions #16294

Open
rstz opened this issue Feb 15, 2022 · 4 comments
Open

Optimization O3 fails when importing Wasi functions #16294

rstz opened this issue Feb 15, 2022 · 4 comments

Comments

@rstz
Copy link

rstz commented Feb 15, 2022

The implementation of PThreadFS imports Wasi functions such as fd_write(). Unfortunately, this fails under optimization level -O3. Under optimization level -O2, everything works well. Is there a way to avoid this issue?

Minimal example

#include <emscripten.h>
#include <wasi/api.h>

EM_IMPORT(fd_close) __wasi_errno_t env_fd_close(__wasi_fd_t fd);

int main() {
  env_fd_close(1);
  return 0;
}

Compile with emcc -o o3bug.html o3bug.cpp -O3, then instantiation fails with

RuntimeError: Aborted(LinkError: WebAssembly.instantiate(): Import #0 module="a" function="a" error: function import requires a callable). Build with -s ASSERTIONS=1 for more info.

Using Emsdk 3.1.3, Chrome 98.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 15, 2022

We have two different namespaces from which we import symbols. "env" and "wasi_snapshot_preview1".

"fd_close" is part of WASI and so should come from the latter... but somehow importing from the former is confusing that minifiier. If you import from "wasi_snapshot_preview1" then it fixes the issue.

To do that you would do something like:
__attribute__((__import_module__("wasi_snapshot_preview1"), __import_name__("fd_close")))

@sbc100
Copy link
Collaborator

sbc100 commented Feb 15, 2022

@kripken do you think is worth fixing the import/export minifier to deal with this case? The above example fails .. but not until runtime... whereas importing from wasi_snapshot_preview1 works as expected.

@rstz
Copy link
Author

rstz commented Feb 15, 2022

Thank you for the quick reply, works like a charm. The world may now enjoy hyper-optimized PThreadFS :)

@kripken
Copy link
Member

kripken commented Feb 15, 2022

do you think is worth fixing the import/export minifier to deal with this case?

Hmm, metadce sees that the symbol is unused (because the declaration is declaring it in another import space - so it's declaring something else entirely). I think that is as expected. Perhaps we could try to get a better error message at compile time here, but at a glance that's not trivial: early in metadce we assume that the import namespaces are merged, so we forget about which import comes from where. By the time we run applyDCEGraphRemovals which is where the error might be noticed, we don't have info about the namespace.

The more surprising thing to me is that this works with -O2... I guess the merging of the namespaces makes that "just work"? That is, metadce knows more information than it should...

This is fixable, but I'm not sure it's worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants