Stream: cranelift

Topic: ✔ Moving a FunctionBuilder?

Jacob Sapoznikow (Feb 16 2024 at 05:10):

Hi! I am trying to implement a custom compiler based on Cranelift for a custom language, which will support both AOT and JIT modes, and I was wondering if I could get some help. Here's some of the code that's giving me some trouble:

//! src/codegen/backend/unify.rs

pub trait BackendInternal<'a> {
    // ...
    fn builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>>;
    fn new_builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>>;
}

impl<'a> BackendInternal<'a> for AotGenerator<'a> {
    // ...

    fn builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>> {
        self.builder.as_ref().unwrap().clone()
    }

    fn new_builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>> {
        self.internal_new_builder();
        self.builder.as_ref().unwrap().clone()
    }
}

impl<'a> BackendInternal<'a> for JitGenerator<'a> {
    // ...

    fn builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>> {
        self.builder.as_ref().unwrap().clone()
    }

    fn new_builder(&mut self) -> Rc<RefCell<FunctionBuilder<'a>>> {
        self.internal_new_builder();
        self.builder.as_ref().unwrap().clone()
    }
}

//! src/codegen/aot.rs

impl<'a> AotGenerator<'a> {
    // ...

    pub(crate) fn internal_new_builder(&mut self) {
        let b = FunctionBuilder::new(&mut self.ctx.func, &mut self.builder_ctx);
        let c = RefCell::new(b);

        self.builder = Some(Rc::new(c));
    }
}

Here's the full code: https://github.com/RedstoneWizard08/QuickScript

The issue is that in the internal_new_builder function, constructing the RefCell throws the error "lifetime may not live long enough". Is there a better way to do this? Please help me. Thanks!

Jacob Sapoznikow (Feb 16 2024 at 05:11):

I would like to note that I have tried many different methods, including using &mut references, no Rcs, and even without the RefCells entirely, and even using Arc. Nothing seems to work.

Chris Fallin (Feb 16 2024 at 05:20):

This is more of a Rust-memory-idioms issue than a Cranelift issue specifically, but the main question I have is: what is the intended lifetime relationship?

FunctionBuilder has a lifetime 'a that denotes the underlying Function's lifetime; it holds a borrow of this function body, and doesn't own it. That means there has to be some scope on the stack somewhere that owns a Function (or owns something that owns it, transitively) and the FunctionBuilder must have a lifetime within that.

The issue here is that AotGenerator is trying to own the Function (inside a FunctionBuilderContext) and the thing that borrows it. That pattern is called a "self-referential struct" and is possible in Rust, but not without tricks (there are crates for this that use unsafe code internally).

I'd recommend unwinding the design a bit, avoiding use of Rc/RefCell (these usually indicate that one is trying to work around lifetimes and make them all dynamic, which doesn't mix well with APIs, like Cranelift's, that use explicit lifetimes and borrows). In general, patterns from GC'd languages like Java/Python/..., or from languages with free-and-dangerous pointers like C/C++, are often a little hard to adopt because they don't fit the strict lifetime nesting regime of safe Rust. A simple workaround would be for you to create the FunctionBuilder in a scope that starts code generation and then pass it as a borrow through params wherever you need it. As a side-benefit, this tends to push toward more functional-style code: it avoids the use of implicit passing through struct members (which at least in my experience, makes more of the plumbing explicit and less error-prone).

Jacob Sapoznikow (Feb 16 2024 at 05:25):

Okay, I have tried to do this in the past (coming from a Java background as my first language xD), but unfortunately the structure of my code throughout leans towards using structs and traits, and I don't even want the lifetimes. Unfortunately, not being able to ".clone() and ignore" doesn't work here as the shared state is important, and I don't know what to do.

Jacob Sapoznikow (Feb 16 2024 at 05:26):

One of the main issues I'm facing is that the way I have organized this to reduce code duplication and allow for expandability in the future is that I use different functions in trait impls to do everything, and they rely on this .builder() method of BackendInternal to use the FunctionBuilder context.

Jacob Sapoznikow (Feb 16 2024 at 05:29):

I'd also like to mention that I am trying to avoid passing the FunctionBuilder object between the different impls as much as possible. I like having it kept in one place and being accessed as needed.

Chris Fallin (Feb 16 2024 at 05:35):

OK, it's hard to say much more without seeing your whole codebase; but again I'd recommend reconsidering the overall object-ownership design. I've found that Rust pushes me strongly away from certain designs, but often when something meets unexpected resistance, it's because there is some hidden tension -- here for example, you have a backend-thing that owns the function, but a function-builder that also owns the function, simultaneously, and that can result all sorts of unexpected bugs where state changes under your feet. (Rust strongly discourages "shared ownership" where there are multiple mutable paths to one thing.) So my only real recommendation (or at least the most honest one!) to an issue of "I prefer designing my software differently" is "learn the idiomatic Rust way first". I know it sounds cliche, sorry; but there's no magic trick to reveal here to solve the problem more easily

Jacob Sapoznikow (Feb 16 2024 at 05:37):

The whole codebase is here, I'm typing another response right now so I'm gonna get this out quickly first: https://github.com/RedstoneWizard08/QuickScript

Jacob Sapoznikow (Feb 16 2024 at 05:38):

I do know that the object-ownership design is probably a bad move. I just don't know how else to do this in a clean way. I can try to move it into a more function-based system, but to me I think that's less intuitive as I am trying to both build an API that others may be able to use or base code off of, and an API that I can come back to in a few years and quickly figure out what's going on. I guess that maybe I'm just not used to functional programming yet.

Chris Fallin (Feb 16 2024 at 05:39):

that's no problem, I just meant it as sort of a reference point if it helped to clarify; the important bit is the "explicit plumbing", i.e., passing the borrows around

Chris Fallin (Feb 16 2024 at 05:40):

One pattern I have found to help is to gather up a "context struct" of arguments that exist (as borrows or owned) during some phase; for example, while compiling a single function

Chris Fallin (Feb 16 2024 at 05:40):

So you have some toplevel generate_function that creates the Function, and then you create your JitContext that has a FunctionBuilder inside of it, and the JitContext is passed down the stack

Chris Fallin (Feb 16 2024 at 05:41):

the thing that this does that storing an Option<...> borrow in a field on your self struct does not is that it makes the lifetimes explicitly nested: Rust can more easily reason about parameters because, if a borrow is passed into a function, unless explicitly annotated it lasts only for that function

Jacob Sapoznikow (Feb 16 2024 at 05:41):

Interesting. I think that might be the way to go. I am loosely basing this on the Cranelift JIT example, and that is how they did it, I just didn't think it was as organized.

Chris Fallin (Feb 16 2024 at 05:41):

whereas a field on self lasts as long as self, so it's harder to prove to the type system that the borrow goes away before the borrowed thing does

Chris Fallin (Feb 16 2024 at 05:41):

(that's the heart of the "self-referential struct" problem)

Jacob Sapoznikow (Feb 16 2024 at 05:42):

Okay, I think I'll give that a shot then.

Chris Fallin (Feb 16 2024 at 05:42):

cool, best of luck! and do feel free to ask more here if still stuck :-)

Jacob Sapoznikow (Feb 16 2024 at 05:42):

Okay! Thanks though!

Jacob Sapoznikow (Feb 16 2024 at 05:44):

BTW, just to clarify, you meant something like this, right?

use std::collections::HashMap;

use cranelift_frontend::{FunctionBuilder, Variable};
use cranelift_module::DataId;

pub struct CodegenContext<'a> {
    pub locals: HashMap<String, DataId>,
    pub vars: HashMap<String, Variable>,
    pub builder: FunctionBuilder<'a>,
}

Jacob Sapoznikow (Feb 16 2024 at 06:14):

Okay, now the only issue is this:

let mut builder = FunctionBuilder::new(&mut self.ctx().func, self.builder_ctx());

It says, "cannot borrow '*self' as mutable more than once at a time".

Jacob Sapoznikow (Feb 16 2024 at 06:56):

omg... no errors... wow! thank you so much!!!

Notification Bot (Feb 16 2024 at 07:33):

Jacob Sapoznikow has marked this topic as resolved.

Chris Fallin (Feb 16 2024 at 15:53):

cool, glad that worked out!

Notification Bot (Feb 17 2024 at 03:29):

Lee Wei has marked this topic as unresolved.

Notification Bot (Feb 17 2024 at 03:29):

Lee Wei has marked this topic as resolved.

Lee Wei (Feb 17 2024 at 03:31):

Sorry, misclicked the button. :sweat_smile:

Last updated: Nov 22 2024 at 17:03 UTC