TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.22k stars 217 forks source link

Add opaque pointers #468

Closed xlambein closed 2 months ago

xlambein commented 5 months ago

Description

All changes apply to LLVM >= 15.0.

Note that I haven't found a way in the C API (at least as exposed by llvm_sys) to disable the opaque-by-default behaviour. Because of that, I'm unsure it even makes sense to have the is_opaque method. Because of this I haven't really documented it much, but I can add an example to the doc if needed.

Related Issue

421

How This Has Been Tested

Breaking Changes

None yet. In the future we might want to remove the ptr_type method on *Type for LLVM 17 onward, since only opaque pointers are supported.

Checklist

TheDan64 commented 5 months ago

Aren't ptrs always opaque in 16(?)+? (the version after whenever they were introduced?)

One of your tests seems to confirm this since context.i32_type().ptr_type(AddressSpace::default()).is_opaque() always passes in 15+ 🤔

xlambein commented 5 months ago

Aren't ptrs always opaque in 16(?)+? (the version after whenever they were introduced?)

One of your tests seems to confirm this since context.i32_type().ptr_type(AddressSpace::default()).is_opaque() always passes in 15+ 🤔

I think this section answers those questions: https://llvm.org/docs/OpaquePointers.html#temporarily-disabling-opaque-pointers

Basically, pointers are opaque by default starting in 15+, but this behaviour can be disabled in 15 & 16, at least in clang. I couldn't find a way to disable them in LLVM's C API, but tbf I didn't look very hard. Given that, I think I could remove the is_opaque method.

TheDan64 commented 4 months ago

@xlambein could you please rebase this on master? Thanks

nsumner commented 4 months ago

Not sure if it is relevant to this specific PR, but I think getting the value type of GlobalValues also matters now with opaque pointers. I just started playing with Inkwell, though, so there may be other idioms I am missing.

e.g.

fn get_value_type<'ctx>(global: GlobalValue<'ctx>) -> AnyTypeEnum<'ctx> {
    unsafe {
        AnyTypeEnum::new(llvm_sys::core::LLVMGlobalGetValueType(
            global.as_value_ref(),
        ))
    }
}
xlambein commented 4 months ago

I'm on holiday until late this month, so I'll finish this PR then :-)

@nsumner That seems right, I'll take a closer look later and perhaps add it to the PR.

xlambein commented 4 months ago

Rebased and added get_value_type to the PR.

xlambein commented 3 months ago

Any news? Still waiting for approval.

TheDan64 commented 3 months ago

Apologies for this taking a while, I am traveling this month and won't have much time to review until April

TheDan64 commented 3 months ago

Not sure why the build is failing suddenly...

TheDan64 commented 3 months ago

Ah, I tried updating the PR and it seems I broke something - please take a look and then we can get this merged

xlambein commented 3 months ago

@TheDan64 I've rebased and tested locally, seems to work again.

TheDan64 commented 3 months ago

Looks like this needs to be adjusted to handle other versions

xlambein commented 3 months ago

I've fixed that issue :-)

gavrilikhin-d commented 3 months ago

@TheDan64 can you have a look?

TheDan64 commented 2 months ago

This looks pretty good, but needs to be updated for llvm18

xlambein commented 2 months ago

@TheDan64 Rebased + added the necessary changes for LLVM 18. At the same time, there was a failing doctest for 18 in upstream/master that I fixed (in src/targets.rs), I assume because there isn't any CI for LLVM 18 yet.