Stream: cranelift

Topic: wasm validation inside of cranelift?

Alex Crichton (Jul 13 2020 at 17:56):

I've got a question for folks who manage cranelift-in-spidermonkey, currently cranelift does no validation of the input wasm module assuming that the embedder does the actual validation (e.g. wasmtime calls wasmparser::validate and presumably spidermonkey validates elsewhere). I think though this may not be tenable too much further into the future.

Alex Crichton (Jul 13 2020 at 17:57):

One example is that ref.is_null no longer encodes its type in the instruction, so to figure out whether you're testing externref or funcref you need to look at the type stack

Alex Crichton (Jul 13 2020 at 17:57):

wasmtime gets away with this by using a different representation so we can look at the type of the cranelift value, but there may be instructions in the future which need non-local information about how to codegen something

Alex Crichton (Jul 13 2020 at 17:58):

Additionally validation has a nontrivial cost in wasmtime right now because we simply "run it up front" and throw away all the results, so for Wasmtime specifically it'd be best actually if the module was validated during translation (this would also make validation parallel, which it currently isn't in wasmtime)

Alex Crichton (Jul 13 2020 at 17:58):

so all of that to ask, what would folks think about adding validation interleaved with cranelift-wasm's translation?

Alex Crichton (Jul 13 2020 at 17:59):

Is this a cost specifically avoided in the spidermonkey integration? For example do we need to make validation a compile time or runtime switch for cranelift? Or would spidermonkey be ok if cranelift double-validated

Alex Crichton (Jul 13 2020 at 17:59):

as in, if cranelift validated what spidermonkey has already validated

Chris Fallin (Jul 13 2020 at 18:12):

AFAICT, the wasm module has already been parsed by other bits of SM's wasm frontend by the time that Cranelift is invoked to compile functions; there are a bunch of shims to move the types/signatures over that I haven't internalized. So validation that happens during wasm parsing in Cranelift would be an extra cost; but intuitively it seems to me that it might not be too much extra cost if the work is essentially shared by parsing and validation

Chris Fallin (Jul 13 2020 at 18:13):

My instinct at first, at least, is to say that it's better for CL to remain simple, rather than trying to make all validation conditional. If it's too much latency later, we can definitely come back and refactor. But @Benjamin Bouvier probably has better thoughts on this! (AFAIK he's out today and tomorrow)

Alex Crichton (Jul 13 2020 at 18:14):

That's a good point that I'm also not familiar with the spidermonkey integration, I'm assuming it just hands a blob of bytes to cranelift but I could be wrong

Alex Crichton (Jul 13 2020 at 18:14):

And yeah doing this conditionally would be tricky, mainly because I'm not sure how to conditionalize the translation phase where we actually need the type information

Alex Crichton (Jul 13 2020 at 18:14):

well, eventually would probably need the type information, today we're getting by ok

Chris Fallin (Jul 13 2020 at 18:17):

Yup, so here's the loop that drives function compilations -- uses a Decoder which is a SM-side wasm parser: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmCraneliftCompile.cpp#476 .

Chris Fallin (Jul 13 2020 at 18:19):

I'd need to read/think a bit more before saying what would make the most sense long-term... we definitely still need to parse for baseline compilation, before Cranelift is involved... but for now it seems parsing costs would be dwarfed by the other compilation stages so it doesn't seem like a big issue to me!

Alex Crichton (Jul 13 2020 at 18:19):

oh ok nice, I'll dig into that function

Alex Crichton (Jul 13 2020 at 18:20):

if cranelift-wasm changes in ways that spidermonkey doesn't even touch that's presumably A-OK heh

Chris Fallin (Jul 13 2020 at 18:20):

yup, for sure

Benjamin Bouvier (Jul 15 2020 at 10:41):

Right now this is conceptually what's happening:

• spidermonkey starts with the module validation, that is, everything before the function bodies section,
• when Ion or wasm baseline are used for compilation, the function's body is validated as we compile it, by iterating over the wasm bytes (so we validate byte by byte)
• when Cranelift is used for compilation, we first validate the whole function's body, and only if it's valid we send it to cranelift (otherwise report an error)

Benjamin Bouvier (Jul 15 2020 at 10:42):

My thought is that ideally, we'd have a way to provide cranelift-wasm with module-level information (type section info, etc.), while cranelift-wasm would do the function validation.

Benjamin Bouvier (Jul 15 2020 at 10:42):

(Spidermonkey's Wasm.validate is super fast, but that's still an extra 100ms overhead for very large modules that only Cranelift has)

Benjamin Bouvier (Jul 15 2020 at 10:43):

(also Cranelift-in-SM does validate function's bodies in parallel, so the cost is a bit amortized)

Alex Crichton (Jul 15 2020 at 13:46):

That actually sounds perfect, and sounds like my conclusions reading code yesterday were indeed correct! I'll see if I can work on wasmparser to enable this (wasmparser basically already has support so it should be easy)

Alex Crichton (Jul 21 2020 at 22:45):

Ok a late follow-up but I've now implemented this at https://github.com/bytecodealliance/wasmtime/pull/2059

Yury Delendik (Jul 21 2020 at 22:46):

/me will review first tomorrow

Alex Crichton (Jul 21 2020 at 22:46):

that PR now forcibly validates all wasm functions fed to cranelift-wasm, so this will affect Gecko (if that lands as-is). I'd definitely want folks familiar with the spidermonkey integration of Cranelift to take a look!

Alex Crichton (Jul 21 2020 at 22:47):

the knobs are now there for validation to be entirely deferred to cranelift (spidermonkey can provide its own hooks to learn about the module's other types), but that may not be sufficient. If so just let me know!

Benjamin Bouvier (Jul 22 2020 at 08:18):

Looks great! I'll be happy to integrate and test it in spidermonkey when the time comes !

Benjamin Bouvier (Jul 22 2020 at 08:21):

Thanks for working on this!

Benjamin Bouvier (Jul 24 2020 at 13:38):

@Alex Crichton hi! let me know if you want to chat about https://github.com/bytecodealliance/wasmtime/pull/2059, maybe I'm missing a way to properly use a primitive type as the Self::Type in this case!

Benjamin Bouvier (Jul 24 2020 at 14:03):

Oh i actually had another question about this: right now, Baldrdash will request module-level information lazily, so it'd make sense that we'd now implement a cache for this information, that we'd fill in incrementally as we compile the function (and share this information with other function compilations, since at this point the module-level info is immutable). Since the traits mention &self and not &mut self, I'd need to use interior mutability (RefCell) to be able to build up the cache information; are there other better ways that would avoid the dynamic RefCell checks on borrows?

Alex Crichton (Jul 28 2020 at 14:06):

@Benjamin Bouvier hey back now, thanks for taking a look! Is Self::Type the only thing you had an issue with?

Alex Crichton (Jul 28 2020 at 14:07):

If so I think I might just remove that trait and return wasmparser::Type instead since I'm not sure having an associated type for Type is all that worth it (all you can do is get wasmparser::Type anyway)

Benjamin Bouvier (Jul 28 2020 at 14:07):

also self vs mut self, but it's probably more open to discussion (and i can solve it locally with a RefCell iiuc, with an additional dynamic cost unfortunately)

Alex Crichton (Jul 28 2020 at 14:09):

oh? what was needing mutability for your implementation?

Alex Crichton (Jul 28 2020 at 14:10):

er right sorry I see now

Alex Crichton (Jul 28 2020 at 14:10):

this would probably use a OnceCell rather than a RefCell

Alex Crichton (Jul 28 2020 at 14:10):

since you'll want to get a long-lived borrow out of it

Alex Crichton (Jul 28 2020 at 14:10):

but that should work to lazily initialize these pieces

Alex Crichton (Jul 28 2020 at 14:12):

FWIW supporting both APIs would be somewhat tricky, but possible

Alex Crichton (Jul 28 2020 at 14:12):

I'd prefer though if we could find something that works from the get-go

Benjamin Bouvier (Jul 28 2020 at 14:13):

/me looks at OnceCell

Alex Crichton (Jul 28 2020 at 14:14):

oh and to clarify, by that I mean this type

Benjamin Bouvier (Jul 28 2020 at 14:16):

Ok! Is the OnceCell::get_mut method cheaper to call than a RefCell::borrow_mut ? (I imagine yes, but not sure)

Alex Crichton (Jul 28 2020 at 14:17):

nah it's basically the same cost AFAIK

Alex Crichton (Jul 28 2020 at 14:17):

it's a boolean check of a flag basically

Benjamin Bouvier (Jul 28 2020 at 14:21):

Ok! Since the cache is filled up as the trait methods are called, i'm not sure it's going to need a long-lived borrow, but yeah, both would work. So it's not a strong requirement for Spidermonkey.

Benjamin Bouvier (Jul 28 2020 at 14:21):

Thanks for checking this! having just return wasmparser::Type would indeed be ideal

Alex Crichton (Jul 28 2020 at 14:26):

ok, I'll make that change

Benjamin Bouvier (Jul 28 2020 at 14:28):

Will this allow returning some kind of references, though?

Alex Crichton (Jul 28 2020 at 14:33):

if you're returning wasmparser::Type it'll just be a Type

Alex Crichton (Jul 28 2020 at 14:33):

no need for references

Benjamin Bouvier (Jul 28 2020 at 14:34):

ah right, i was wondering if this would be future proof since the GC type proposal might introduced structured types that won't be primitive types

Benjamin Bouvier (Jul 28 2020 at 14:35):

but that might be irrelevant at this poitn

Alex Crichton (Jul 28 2020 at 14:37):

yeah I'm not sure how that will play out myself

Alex Crichton (Jul 28 2020 at 14:37):

I figured we can probably wait to see what happens there

Alex Crichton (Jul 28 2020 at 16:35):

@Benjamin Bouvier https://github.com/bytecodealliance/wasm-tools/pull/68 is what I'm thinking for your issues

Benjamin Bouvier (Jul 28 2020 at 16:39):

@Alex Crichton thanks! I'm worried about the reverting for TableType and other non-primitive types, though; this will require straight conversions from spidermonkey's types to wasmparser's types everywhere, or creating the converted types and caching them...

Alex Crichton (Jul 28 2020 at 16:39):

I think it should be cheap to do though? They're basically just Copy containers of types

Alex Crichton (Jul 28 2020 at 16:39):

and wasmparser validation would do the conversion anyway basically regardless

Alex Crichton (Jul 28 2020 at 16:40):

(and with monomorphization it should all go away in terms of overhead)

Benjamin Bouvier (Jul 28 2020 at 16:42):

Sounds good to me! If it's an issue, profiles will tell us anyways :-)

Last updated: Nov 22 2024 at 16:03 UTC