Skip to content

Avoid using WasmOffsetConverter when Module.instantiateWasm #13425

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

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 5, 2021

Module.instantiateWasm is a callback the user provides, and then the user is
in change of fetching and instantiating the wasm. In that code path emscripten
never sees the wasm binary, which means we cannot create a
WasmOffsetConverter.

With this PR we will not hang on startup, and we will still detect problems, but
the sanitizer's own stack traces will mark function names as "unknown". That will
limit the santizer's usefulness, but they do still detect errors, and the browser's
own stack traces may be enough to diagnose things.

Fixes #13424

cc @pfaffe

@kripken kripken requested a review from sbc100 February 5, 2021 18:36
src/preamble.js Outdated
return 'unknown-due-to-instantiateWasm';
}
};
{{{ runOnMainThread("removeRunDependency('offset-converter');") }}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this effect threaded programs? IIRC we use instantiateWasm in worker.js to inject the module on pthreads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't affect them, as we send workers the offset data. But I refactored this to do the new work only on the main thread, which is clearer I think.

@pfaffe
Copy link
Collaborator

pfaffe commented Feb 5, 2021

receiveInstance gets the module and instance. Would that be sufficient for the offset converter?

@kripken
Copy link
Member Author

kripken commented Feb 5, 2021

@pfaffe

No, we need the binary. The conversion is of binary offsets to function names.

(And neither the module nor the instance have a reference to the binary, for VM efficiency reasons.)

@pfaffe
Copy link
Collaborator

pfaffe commented Feb 5, 2021

I see, makes sense. Change looks good then!

What about making receiveInstance take an optional third argument with the ArrayBuffer in addition to your change, when offset computations are enabled? instantiateWasm users could opt in into accurate names then. Not sure I like that API, but better accuracy with sanitizers seems useful.

@kripken
Copy link
Member Author

kripken commented Feb 5, 2021

@pfaffe That's possible, but the method already has one optional param, and I'm hoping to avoid complexity here. I'd prefer to focus on figuring out ways for users to not need instantiateWasm in the first place, too.

@pfaffe
Copy link
Collaborator

pfaffe commented Feb 5, 2021

Yeah I'm not super happy with that API either. We could avoid instantiateWasm if the emscripten glue would take care of the caching of the module for us (the browser helps with caching the fetch/binary data, but not the module).

@kripken
Copy link
Member Author

kripken commented Feb 5, 2021

(the browser helps with caching the fetch/binary data, but not the module).

That's not accurate - the browser should help caching the wasm module's compiled code. Unless I'm not understanding what you mean by "cache the module"?

(In general, if there was something to cache at the user level, emscripten should do it. But the policy is that users should not cache modules, and rely on the browser.)

cc @billbudge

@kripken kripken merged commit e0138df into master Feb 8, 2021
@kripken kripken deleted the nooc branch February 8, 2021 18:27
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

Successfully merging this pull request may close these issues.

offset-converter and caller-provided instantiateWasm hooks
3 participants