[JSInterop] Be aware of configureAll#8046
Conversation
|
What is |
|
RemoveUnusedModuleElements |
| return nullptr; | ||
| } | ||
|
|
||
| bool Intrinsics::isConfigureAll(Function* func) { |
There was a problem hiding this comment.
Let's check the type as well so all users can be guaranteed that the parameters have the correct types.
There was a problem hiding this comment.
We don't do that for the intrinsic above. Do you think it is worth the effort? I'm not sure what it would catch in practice that would be useful. (If the user has this wrong, the VM will error anyhow?)
There was a problem hiding this comment.
Yeah, maybe it's not worth it. configureAll has a single type whereas call.without.effects is overloaded for an infinite set of types, so at least we could check its type rather than letting the user find out about the problem at runtime.
| // several passes. | ||
| // TODO Specific fixes in those passes could replace this, and allow better | ||
| // optimization. | ||
| if (wasm.start) { |
There was a problem hiding this comment.
I imagine that eventually we'll need to handle this in places other than the start function, but starting with this is probably sufficient.
Co-authored-by: Thomas Lively <tlively123@gmail.com>
| return nullptr; | ||
| } | ||
|
|
||
| bool Intrinsics::isConfigureAll(Function* func) { |
There was a problem hiding this comment.
Yeah, maybe it's not worth it. configureAll has a single type whereas call.without.effects is overloaded for an infinite set of types, so at least we could check its type rather than letting the user find out about the problem at runtime.
| auto error = [&](const char* msg) { | ||
| Fatal() << "Invalid configureAll( " << msg << "): " << *call; | ||
| }; |
There was a problem hiding this comment.
We should definitely avoid erroring out on unrecognized configureAll patterns, since they would still be perfectly valid. Maybe we can print a warning and return an empty list if we see some other pattern.
There was a problem hiding this comment.
I think it might be better to error for now? A warning might be missed, and make things harder to debug. (We can always reduce an error to a warning later if it feels annoying in practice.)
There was a problem hiding this comment.
In particular, if a warning is missed, and configureAll is not noticed, we'd end up optimizing away code in ways that might be confusing, like we've seen already.
2 participants
This magic import refers to functions, which we must handle, and not
modify their signature or contents.
This fixes things optimally for RUME, but for all the passes that look
at public/private heap types, it just marks those functions as public,
which inhibits optimizations. This unblocks work on JS Interop for now
(and maybe those optimizations are not necessary, but if we do want
them, it would mean changes to each such pass).
Spec:
https://github.com/WebAssembly/custom-descriptors/blob/main/proposals/custom-descriptors/Overview.md#configuration-api