Module does not hold reference to context

[nicokoch]

I noticed a major design flaw in the current implementation of modules.

The module needs to hold a reference to the context it was created in. Otherwise it's possible to drop the context before dropping the module, which will result in undefined behaviour.
I just ran into the error in a toy compiler I'm building.

The same also applies to Builder and its Context.

The easiest way to implement this is probably by having a member Rc<Context> in Module and Builder

[71]

Thanks for your report!

Oh, yep, that's a problem. A problem we have here is that the LLVM api manipulates opaque pointers to start with, which means that using Rc means reference-counting a reference in itself. It does appear to be one of the only solutions though.

[TheDan64]

Great catch! I was unaware of this issue.

I think we should be able to hand out &Builder references from a given Context, which should be the simpler approach. However Modules are far more complex, as ExecutionEngines need to take ownership of them. And indeed a ref counter may be the only solution like you stated.

I'll get to work on the Builder fix ASAP (unless either of you would like to give it a PR). Module warrants more research, I think.

[TheDan64]

Would you mind providing some simple examples of how to reproduce this issue? They'll be super helpful as test cases. Thanks!

[nicokoch]

Sure, the most simple test case would look something like this:

fn main() {
    let module = {
        let context = Context::create();
        context.create_module("test")
    };
    // do something with module here
}

Currently, this code will trigger undefined behaviour (on my macbook it spits out "an unknown error occured").

[nicokoch]

One library that also fixes this by using Arc to the parent object is vulkano.
Look at this code for example: https://github.com/vulkano-rs/vulkano/blob/master/vulkano/src/device.rs#L126

You should consider using Arc instead of Rc to, if multithreading is something you want to support down the line (but I'm not sure how threadsafe LLVM is in general, so maybe this isn't a concern)

[nicokoch]

One last note after thinking about this for a while.
Context handing out references to Builder is IMO not a good solution.

1. It will require the context to hold a Vec<Builder>, which is a lot more costly than a simple Rc (requires allocation)
2. The user will probably end up in lifetime hell (You usually want to store the builder in a wrapping struct when implementing a custom compiler)

In my compiler for example, I have a struct that looks something like this:

pub struct LLVMBuilder {
    ctx: Context,
    module: Module,
    builder: Builder,
}

Would this even be possible with &Builder, that is owned by the Context?
Even if it is, probably a pita to deal with.

[TheDan64]

I believe LLVM does support multithreading, but it isn't on my radar to support anytime soon. I see it as a way advanced feature that will likely have its own set of complications. I did create a ticket for it here, though: #28 I think it should be straightforward to convert any Rc types to Arc

That's a great point. Rc probably allocates, though, no? Arc would almost definitely do so. But anyway, I think I agree overall. I'll have to look into how Vulkano does it some more. Thanks!

ExecutionEngine currently takes ownership of Modules and deals out references to them, which makes it quite awkward to modify the EE. I wonder if it should follow this Rc pattern, too.

[TheDan64]

@nicokoch So I'm having trouble reproducing this for builder:

let builder = {
    let context = Context::create();

    context.create_builder()
};

builder.build_unreachable();

but there is no invalid memory access as far as I can tell. Tests pass fine (though I know that's not a perfect validation) Are you seeing otherwise on your system?

[nicokoch]

I can test again on monday, I haven't really investigated in the builder situation yet.

[nicokoch]

For the builder it doesn't crash on my machine either.

Nonetheless, even if no invalid memory access occurs, it doesn't mean it cannot occur under any environment.
We are probably on the safe side implementing the fix for builder too.

That's a great point. Rc probably allocates, though, no?

Yes, Rc does allocate, my bad.

[TheDan64]

Yeah, I agree but I would feel better about the builder fix if there was some way to verify it works.

[nicokoch]

There is! Just add a println! inside the drop implementation and verify that Context's drop is always called after Builder's drop (for example with the code snippet from earlier)

[TheDan64]

I'm not convinced builders are as tied to the hip to their context as modules are.

You suggested verifying the context dropped after builder:

let context = Context::create();
let builder = context.create_builder();

drop(builder);
drop(context);

but that did not yield any invalid memory access either.

For example using the builder with a context other than the one it was created with (and the original destroyed) works fine:

let builder = {
    let context = Context::create();

    context.create_builder()

    // Original Context drops fine
};

// Builder continues to function with different context
let context = Context::create();
let module = context.create_module("my_mod");
let void_type = context.void_type();
let fn_type = void_type.fn_type(&[], false);
let fn_value = module.add_function("my_fn", &fn_type, None);
let entry = fn_value.append_basic_block("entry");
builder.position_at_end(&entry);
builder.build_unreachable();

module.print_to_string();

// Prints: "; ModuleID = \'my_mod\'\n\ndefine void @my_fn() {\nentry:\n  unreachable\n}\n");

// 2nd Context drops fine
// Builds drops fine

This is looking like a Module only issue until we can prove otherwise. You are correct in stating it may be possible in some environment, but without having one as an example it's hard to verify.