Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

const_to_int of const_gep results in broken getelementptr call, const_gep segfaults with i64 indexes #213

Open
djozis opened this issue Oct 3, 2020 · 8 comments · May be fixed by #214
Open
Labels
Milestone

Comments

@djozis
Copy link

djozis commented Oct 3, 2020

Describe the Bug
I'm not sure if these are related or not. If not, I apologize for the two-for-one.
const_gep followed by const_to_int results in the gep getting butchered (wrong type and an index goes missing in this example). Also const_gep with i64 values segfaults.

To Reproduce
Run this. See comments in code for details.

use inkwell::context::Context;
use inkwell::AddressSpace;

fn main() {
    let context = Context::create();
    let module = context.create_module("my_module");
    let i32_type = context.i32_type();
    let i32_ptr_type = context.i32_type().ptr_type(AddressSpace::Generic);

    let struct_type = context.struct_type(&[i32_type.into(), i32_type.into()], false);

    unsafe { // just being lazy with gep calls.
        let init = struct_type.ptr_type(AddressSpace::Generic).const_zero().const_gep(&[
            i32_type.const_int(0, false).into(),
            i32_type.const_int(1, false).into()
        ]);

        let a = module.add_global(i32_ptr_type, Some(AddressSpace::Generic), "a");
        a.set_constant(true);
        a.set_initializer(&init);
        // Good, normal:
        // @a = constant i32* getelementptr ({ i32, i32 }, { i32, i32 }* null, i32 0, i32 1)

        let b = module.add_global(i32_type, Some(AddressSpace::Generic), "b");
        b.set_constant(true);
        b.set_initializer(&init.const_to_int(i32_type));
        // Why is GEP only passed one index? Where did my 0 go? -----------V
        // @b = constant i32 ptrtoint (i32* getelementptr (i32, i32* null, i32 1) to i32)
        // Types are wrong?^             ^

        let c = module.add_global(i32_ptr_type, Some(AddressSpace::Generic), "c");
        c.set_constant(true);
        c.set_initializer(&init);
        // Same as a - just showing the initializer isn't mutated or unusable or anything.
        // @c = constant i32* getelementptr ({ i32, i32 }, { i32, i32 }* null, i32 0, i32 1)
    }

    println!("{}", module.print_to_string().to_string());
    // This is where I got the above samples. Here's the whole output:
    // ; ModuleID = 'my_module'
    // source_filename = "my_module"
    //
    // @a = constant i32* getelementptr ({ i32, i32 }, { i32, i32 }* null, i32 0, i32 1)
    // @b = constant i32 ptrtoint (i32* getelementptr (i32, i32* null, i32 1) to i32)
    // @c = constant i32* getelementptr ({ i32, i32 }, { i32, i32 }* null, i32 0, i32 1)

    module.verify().unwrap();
    // No errors.

    // This instantly segfaults. Seems using i64s is not valid for the indexing.
    unsafe {
        struct_type.ptr_type(AddressSpace::Generic).const_zero().const_gep(&[
            context.i64_type().const_int(0, false).into(),
            context.i64_type().const_int(1, false).into(),
        ]);
    }
}

Expected Behavior
The gep on variable b should be the same as a, just with the pointer operation as well.
Less important (to me): The program should not segfault, unless gep with i64 indexes is just invalid or something - might be my mistake. If so, maybe it should take a u32 slice reference instead for the indexes?

LLVM Version (please complete the following information):

Desktop (please complete the following information):

  • OS: Windows 10

Additional Context
LLVM built from source.

In LLVM clone dir:

>git show HEAD
commit ef32c611aa214dea855364efd7ba451ec5ec3f74 (HEAD -> release/10.x, tag: llvmorg-10.0.1-rc4, tag: llvmorg-10.0.1, origin/release/10.x)

In the clone of inkwell that cargo pulled:

>git show HEAD
commit 67981a09816b705a7304aaa995e29848cb832426 (HEAD -> master, origin/master)
@djozis djozis changed the title const_gep segfaults with i64 indexes, const_to_int of const_gep results in broken getelementptr call const_to_int of const_gep results in broken getelementptr call, const_gep segfaults with i64 indexes Oct 3, 2020
@djozis
Copy link
Author

djozis commented Oct 3, 2020

Determined that it's the same upstream (perhaps just my misunderstanding of LLVM since I'm pretty new), so I've created: https://gitlab.com/taricorp/llvm-sys.rs/-/issues/10

@djozis djozis closed this as completed Oct 3, 2020
@djozis
Copy link
Author

djozis commented Oct 3, 2020

Actually, reopening because if using values other than i32 is really invalid for GEP instructions, should the API should probably be restricted in inkwell?

@djozis djozis reopened this Oct 3, 2020
@djozis
Copy link
Author

djozis commented Oct 4, 2020

With assertions on, the i64 indexes for GEP fails with:

Assertion failed: Ty && "Invalid GetElementPtrInst indices for type!", file <snip>\llvm-project\llvm\include\llvm/IR/Instructions.h, line 885

@TheDan64
Copy link
Owner

TheDan64 commented Oct 4, 2020

Thanks for looking into this. There's nothing we can do about this at compile-time, but I suppose we could panic or return an Err if we check every value provided to GEP and ensure it's 32bit wide

@TheDan64 TheDan64 added the bug label Oct 4, 2020
@TheDan64 TheDan64 added this to the 0.1.0 milestone Oct 4, 2020
@djozis
Copy link
Author

djozis commented Oct 4, 2020

Just to confirm, after reading through LLVM source code:
https://github.com/llvm-mirror/llvm/blob/master/lib/IR/Type.cpp#L558-L559

    // Structure indexes require (vectors of) 32-bit integer constants.  In the
    // vector case all of the indices must be equal.

The call to this results in a null being returned if the type isn't an i32 or vec of i32, which results in the misleading assertion above being thrown: Invalid GetElementPtrInst indices for type. The real issue is that struct indexes must be i32s presently as suspected above. I also tested with other int types and they all fail the same way. Because they're any constant values, not just literaly, we shouldn't just take a &[i32], but as you said, it could check that values are i32s.

@djozis
Copy link
Author

djozis commented Oct 5, 2020

I've figured out one of my GEP issues, digging through LLVM source - sharing here for anyone else who comes across this.
LLVM folds constants while creating constant terms. In this case,

Actual:
@b = constant i32 ptrtoint (i32* getelementptr (i32, i32* null, i32 1) to i32)
Expected:
@b = constant i32 ptrtoint (i32* getelementptr ({ i32, i32 }, { i32, i32 }* null, i32 0, i32 1) to i32)

They're actually the same. It's working as intended. It just simplified the constant expression.
I got confused while trying to create a minimal demonstration of the other issue, which was that i64 indexes in constant GEPs work if LLVM manages to fold them, and segfault (or trigger an assertion if enabled) otherwise - and the inconsistency and resulting behavior was confusing me. I was able to reproduce that in C.

I still agree with the validation - checking that GEP indexes are i32s - at least if debug assertions are turned on, perhaps.

@TheDan64
Copy link
Owner

TheDan64 commented Oct 5, 2020

Would you be interested in making this change?

djozis added a commit to djozis/inkwell that referenced this issue Oct 5, 2020
djozis added a commit to djozis/inkwell that referenced this issue Oct 5, 2020
@djozis djozis linked a pull request Oct 5, 2020 that will close this issue
1 task
@djozis
Copy link
Author

djozis commented Oct 5, 2020

Sure - opened #214.

@TheDan64 TheDan64 linked a pull request Oct 6, 2020 that will close this issue
1 task
@TheDan64 TheDan64 modified the milestones: 0.1.0, 0.2.0 Mar 29, 2022
@TheDan64 TheDan64 modified the milestones: 0.5.0, 0.6.0 Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants