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.
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 theMach
prefix (MachInst
,MachBuffer
, ...) but IMHO it's a bit of an antipattern now. I also don't necessarily likeCompileResult
because it sounds more like the "type alias ofResult
with a particular error type" idiom. Maybe something likeCompiledCode
?
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
orPicCode
aren't so self-explanatory, and thePic
bit is a topic for discussion. That's only a renaming and quite easy to change later, though, so I'll start withCompiledCode
for this PR.
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
orPicCode
aren't so self-explanatory, and thePic
bit is a topic for discussion. That's only a renaming and quite easy to change later, though, so I'll start withCompiledCode
for this PR.
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 thecompile
function:let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
but now,
ctx
is mutably borrowed even in themap_err
function's body (becauseresult
is a reference intoctx
's fields), so borrowck says we can't immutably readctx.func
in the prettifier method.The solution I opted for was to have a thin wrapper for the
compile
functions, that contained both theCodegenError
and the reference to theFunction
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.
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 thecompile
function:let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
but now,
ctx
is mutably borrowed even in themap_err
function's body (becauseresult
is a reference intoctx
's fields), so borrowck says we can't immutably readctx.func
in the prettifier method. I should note that this is a limitation from borrowck, and we humans know better: themach_compile_result
field and thefunc
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 theCodegenError
and the reference to theFunction
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.
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 thecompile
function:let result = ctx.compile(...).map_err(|err: CodegenError| pretty_anyhow_error(&ctx.func, err))?;
but now,
ctx
is mutably borrowed even in themap_err
function's body (becauseresult
is a reference intoctx
's fields), so borrowck says we can't immutably readctx.func
in the prettifier method. I should note that this is a limitation from borrowck, and we humans know better: themach_compile_result
field and thefunc
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 theCodegenError
and the reference to theFunction
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: Dec 23 2024 at 13:07 UTC