Module does not hold reference to context about inkwell HOT 13 CLOSED

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

from inkwell.

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.

from inkwell.

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.

from inkwell.

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

from inkwell.

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").

from inkwell.

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)

from inkwell.

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.

from inkwell.

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.

from inkwell.

@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?

from inkwell.

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.

from inkwell.

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

from inkwell.

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)

from inkwell.

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.

from inkwell.