RLBox - Can we remove the dependency on cranelift from the lucet wasm runtime
Categories
(Core :: JavaScript: WebAssembly, defect, P3)
Tracking
()
People
(Reporter: shravanrn, Assigned: shravanrn)
References
(Blocks 1 open bug)
Details
It seems that lucet's runtime, depends on the cranelift-codegen package. See here
This means that cranelift-codegen is pulled in by lucet to support use of wasm sandboxed libraries... However, this is problematic as the version of cranelift-codegen used by lucet is not (and is unlikely to ever be) the same as the version used in Firefox. This leads to the requirement of including 2 different versions of cranelift in the code base, which greatly increases build times.
In the short term, we have forked lucet, and kept the version of cranelift used in sync with in-tree...
However longer term, it seems useful to remove the dependency of cranelift-codegen from the lucet runtime. From inspection of the source here and here, the 2 parts of cranelift-codegen needed are cranelift_codegen::entity::entity_impl and cranelift_codegen::ir
We need to investigate to see if these pieces are actually needed for the lucet wasm runtime. If not, we need to open a bug and get this fixed upstream. If yes, we should work the cranelift upstream to consider separating this into a different package.
|
Assignee | |
Comment 1•5 years ago
|
||
Have added a description of the cranelift dependency problem we are seeing... Would we able to get some cranelift folk to weigh in how this is best handled?
|
||
Comment 2•5 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
|
||
Comment 3•5 years ago
|
||
(In reply to Shravan Narayan from comment #1)
Have added a description of the cranelift dependency problem we are seeing... Would we able to get some cranelift folk to weigh in how this is best handled?
Benjamin & Dan, do you think there's something the Cranelift team could do to help loosen this dependency? For reference this has to do with our wasm sandboxing project.
|
||
Comment 4•5 years ago
|
||
No, sorry. Lucet being a wasm runtime, it uses a compiler under the hood to generate machine code for the host machine to run the wasm code, and this compiler is Cranelift, so unless Lucet wanted to implement a new compiler backend not based on Cranelift, it is very unlikely that the dependency could be removed.
|
||
Comment 5•5 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
No, sorry. Lucet being a wasm runtime, it uses a compiler under the hood to generate machine code for the host machine to run the wasm code, and this compiler is Cranelift, so unless Lucet wanted to implement a new compiler backend not based on Cranelift, it is very unlikely that the dependency could be removed.
I'm confused. Does the runtime actually require the compiler at runtime? Surely not? I think the ask is is whether certain parts of the compiler--really basic parts--can be split out and shared between the runtime and the compiler so that we don't have to drag along the entire compiler just for one or two bits.
|
|
||
Comment 6•5 years ago
|
||
Okay, the missing bits to understand was that wasm code is precompiled to machine code here, and we just need the runtime to call it.
Looking at the two dependencies:
- cranelift_entity is already shipped as its own crate (https://crates.io/crates/cranelift-entity) and it's rather stable. As it's supposed to be lightweight too, maybe compiling it twice isn't much of an issue.
- for the ir dependency, this is more tricky because this is part of the code that's getting generated at build time by Cranelift. Lucet uses a very small subset of the ir crate to know how to convert a Cranelift value type to a Lucet value type (and ditto for signatures). This information could instead be translated at compile time rather than at run time, so the dependency on Cranelift's ir would be removed.
|
|
Assignee | |
Comment 7•5 years ago
|
||
Thanks for the info! Seems like this is something we will have to follow up upstream. I have opened a bug on the lucet repo to track this.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 8•5 years ago
|
||
Thanks folks! I've submitted a patch which was accepted upstream.
For the context of wasm library sandboxing, we are currently running out of a fork or lucet, but this is one less patch we have to maintain for the long term.
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Comment 9•5 years ago
|
||
Great work; I think cranelift-codegen would have been the biggest offender in compile time / build size. Are you only maintaining the fork because of cranelift-entity? If so, as I said above, duplicating it in build shouldn't be a big issue, since it's a very small crate (2.7 KLoC, including tests).
|
|
Assignee | |
Comment 10•5 years ago
|
||
Nope it's not because of cranelift-entity... Right now lucet is missing support for a handful features we need for the library sandboxing case, such as support for mutable function pointer tables. My implementation of these features on the fork cannot be upstreamed as my implementation, while sufficient for our use case is not fully generalizable. If you're curious, let me know and I can provide a list of patches we maintain.
Description
•