inkwell: Crash when implementing a new function to get parent of a global value

Hi,

Describe the Bug

I’m currenlty implement a function get_parent for the GlobalValule in global_value.rs:

Here is my implementation:

    pub fn get_parent(self) -> Module<'ctx> {
        unsafe {
            let m = LLVMGetGlobalParent(self.as_value_ref());
            Module::new(m)
        }
    }

However, when running a code to call to this new function, the program always crashes with the error message:

(signal: 11, SIGSEGV: invalid memory reference)

The crash is done by the line: Module::new(m):

I suspect that the error is due to the data structure Module, which stores module in a Cell:

https://github.com/sbip-sg/llvm-rust/blob/70a425a2e5b56ab6dc2bfd75b9145028558c130f/inkwell/src/module.rs#L181-L201

pub struct Module<'ctx> {
    data_layout: RefCell<oOption<DataLayout>>,
    pub(crate) module: Cell<LLVMModuleRef>,
    pub(crate) owned_by_ee: RefCell<Option<ExecutionEngine<'ctx>>>,
    _marker: PhantomData<&'ctx Context>,
}

Can you advise how to implement such a get_parent function to avoid crashes?

To Reproduce

Not reproducible with the current code, since it’s my own implementation.

Expected Behavior

We should allow Inkwell to return the parent Module of a GlobalValue.

LLVM Version (please complete the following information):

LLVM Version: LLVM 14
Inkwell Branch Used: llvm-14

Desktop (please complete the following information):

OS: Linux Mint 21 (Ubuntu 22 - based)

About this issue

Original URL
State: open
Created 2 years ago
Comments: 21 (21 by maintainers)

Most upvoted comments

I think I’ve sketched out a fix for this using two lifetimes but I’m considering punting it until 0.2.0 since it’ll be a fairly big breaking change

TheDan64 on Sep 16, 2022

I think aliased Module lives no longer than original as it derives from FunctionValue/GlobalValue binded to original’s lifetime (rather than context):

The generic type is no longer helpful once you base the lifetime on the module. Also, a slight problem is that it’ll have to keep track of both context and module lifetimes, to return context related data. But could work

TheDan64 on Aug 27, 2022

Actually, now that I’m thinking about it again, a way to fix ContextRef’s problem would be to duplicate all of the Context methods onto ContextRef and tweak the lifetimes on them. This might work but would be a lot of code duplication…

TheDan64 on Aug 27, 2022

for FunctionValue/GlobalValue::get_parent to be safe at all, they’d have to keep a copy of the module around with them. I’m hesitant to do so for a number of reasons, including the fact that BasicBlock::get_parent would have no way to get a copy of the module since BasicBlocks are derived from Contexts not Modules. Personally I think we should just make the get_parent methods unsafe. I don’t really see a great solution here atm

TheDan64 on Aug 27, 2022

This issue is closely related to #343 as it’s also SEGV issue with already disposed Modules. Thus, maybe we also need lifetime refers to “original” Module to reduce chances where bad references are exposed to users, in compile-time.

When these two features introduced, API maybe like:

impl<'ctx, State> Module<'ctx, State>{
    pub fn add_global(&self, /* omit */) -> GlobalValue<'ctx, 'module> {
        /* omit */
    } 
}

impl<'ctx, 'module> GlobalValue<'ctx, 'module>{
    pub fn get_parent(self) -> Module<'ctx, Alias<'module>> {
        /* omit */
    } 
}

Then we can prevent (but I’m not confident it’s enough):

Double free in Drop of Module
Trying to look into already dropped memory (for example trying to print the content of GlobalValue or FunctionFalue alive longer than original Module like #343)

ytoml on Aug 27, 2022