Stream: git-wasmtime

Topic: wasmtime / issue #4540 Move `emit_to_memory` to `MachComp...

Wasmtime GitHub notifications bot (Jul 27 2022 at 16:30):

bjorn3 commented on issue #4540:

I don't necessarily like the unsafe aspect of emit_to_memory. It feels out of place in a crate that otherwise has no unsafe code: the details of putting the bytes where they are needed should be in the hands of the embedder. In my mind the ideal interface is to expose the machine code as a &[u8] off of the MachCompileResult; this more closely mirrors reality (the struct owns the bytes as a Vec<u8>, and is a value representing the result of compilation, so this is just a projection of that part of the value) and feels more idiomatic.

This is an artifact of the old way Cranelift worked before machinst. It was possible to calculate the size of the necessary buffer before actually emitting any bytes, so bytes could be emitted in place without requiring any reallocation. With machinst this is no longer the case. As such it should indeed expose an &[u8] now IMHO.

We have an accessor .compile_result() on the Context (this feels like a little thing, but methods are generally more obvious than public fields) that still returns an Option<&MachCompileResult>; but more importantly,

This one I don't necessarily agree with, but...

We return a CodegenResult<&MachCompileResult> from compile(). The CodeInfo (the current return value) is already accessible from the compile result object with .code_info(). This then enables something like: copy_into_jit_memory(ctx.compile(isa)?.machine_code()).

...this one I definitely do agree with.

Wasmtime GitHub notifications bot (Jul 27 2022 at 16:46):

cfallin commented on issue #4540:

And while we're at it, I might bikeshed the struct type name MachCompileResult a bit. I came up with it when initially doing the machinst framework where types were often distinguished with the Mach prefix (MachInst, MachBuffer, ...) but IMHO it's a bit of an antipattern now. I also don't necessarily like CompileResult because it sounds more like the "type alias of Result with a particular error type" idiom. Maybe something like CompiledCode ?

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:13):

bnjbvr commented on issue #4540:

Sounds great! I am hesitant when I hear CompiledCode: does it mean the final code mmapped to its final executable region, or the code with relocations? (and it's the latter)
No obvious better name comes to mind, though. For instance, CompiledPicCode or PicCode aren't so self-explanatory, and the Pic bit is a topic for discussion. That's only a renaming and quite easy to change later, though, so I'll start with CompiledCode for this PR.

Wasmtime GitHub notifications bot (Jul 28 2022 at 16:42):

bnjbvr edited a comment on issue #4540:

Sounds great! I am incertain when I hear CompiledCode: does it mean the final code mmapped to its final executable region, or the code with relocations? (and it's the latter)
No obvious better name comes to mind, though. For instance, CompiledPicCode or PicCode aren't so self-explanatory, and the Pic bit is a topic for discussion. That's only a renaming and quite easy to change later, though, so I'll start with CompiledCode for this PR.

Wasmtime GitHub notifications bot (Aug 01 2022 at 10:53):

bnjbvr commented on issue #4540:

Implemented all the ideas mentioned here that would make this move forward. In particular, returning a ref to the compiled result from the compile funcs turned out a bit worse than anticipated in 670dcab, so we could decide to get rid of that instead.

We have a common pattern in the code base of doing:

ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
let result = ctx.mach_compile_result();

which becomes, with the naive change of just returning &MachCompileResult from the compile function:

let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;

but now, ctx is mutably borrowed even in the map_err function's body (because result is a reference into ctx's fields), so borrowck says we can't immutably read ctx.func in the prettifier method.

The solution I opted for was to have a thin wrapper for the compile functions, that contained both the CodegenError and the reference to the Function we're trying to compile, so this code becomes:

let result = ctx.compile(...).map_err(|err: CompileError<'_>| pretty_anyhow_error(&err.func, err.inner /* the CodegenError */))?;

but that's a bit of cognitive overhead for relatively low-value, so would be happy to hear opinions if it's something we want to keep or not.

Wasmtime GitHub notifications bot (Aug 01 2022 at 10:54):

bnjbvr edited a comment on issue #4540:

Implemented all the ideas mentioned here that would make this move forward. In particular, returning a ref to the compiled result from the compile funcs turned out a bit worse than anticipated in 670dcab, so we could decide to get rid of that instead.

We have a common pattern in the code base of doing:

ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
let result = ctx.mach_compile_result();

which becomes, with the naive change of just returning &MachCompileResult from the compile function:

let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;

but now, ctx is mutably borrowed even in the map_err function's body (because result is a reference into ctx's fields), so borrowck says we can't immutably read ctx.func in the prettifier method. I should note that this is a limitation from borrowck, and we humans know better: the mach_compile_result field and the func fields are different and separated, so this is safe to do, but borrowck can't know about this because of all the visible function calls.

The solution I opted for was to have a thin wrapper for the compile functions, that contained both the CodegenError and the reference to the Function we're trying to compile, so this code becomes:

let result = ctx.compile(...).map_err(|err: CompileError<'_>| pretty_anyhow_error(&err.func, err.inner /* the CodegenError */))?;

but that's a bit of cognitive overhead for relatively low-value, so would be happy to hear opinions if it's something we want to keep or not.

Wasmtime GitHub notifications bot (Aug 01 2022 at 10:54):

bnjbvr edited a comment on issue #4540:

Implemented all the ideas mentioned here that would make this move forward. In particular, returning a ref to the compiled result from the compile funcs turned out a bit worse than anticipated in 670dcab, so we could decide to get rid of that instead.

We have a common pattern in the code base of doing:

ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
let result = ctx.mach_compile_result();

which becomes, with the naive change of just returning &MachCompileResult from the compile function:

let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;

but now, ctx is mutably borrowed even in the map_err function's body (because result is a reference into ctx's fields), so borrowck says we can't immutably read ctx.func in the prettifier method. I should note that this is a limitation from borrowck, and we humans know better: the mach_compile_result field and the func fields are different and separated, so this is safe to do, but borrowck can't know about this because of all the high-level function calls that take &self.

The solution I opted for was to have a thin wrapper for the compile functions, that contained both the CodegenError and the reference to the Function we're trying to compile, so this code becomes:

let result = ctx.compile(...).map_err(|err: CompileError<'_>| pretty_anyhow_error(&err.func, err.inner /* the CodegenError */))?;

but that's a bit of cognitive overhead for relatively low-value, so would be happy to hear opinions if it's something we want to keep or not.

Last updated: Oct 23 2024 at 20:03 UTC