Skip to content

Manually avoid declaring opaque structs #500

Closed

Description

@kvark

My code uses the following types on the FFI:

struct Foo(NonZeroU64);
struct Bar {
  foo: Option<Foo>,
}

What I want here really is foo: u64.

What cbindgen ends up producing is:

typedef struct Option_Foo Option_Foo;

It looks like this opaque type leaves me no way to redefine the binding in the header, or work around otherwise. Is there anything we could to do make this work?

Activity

  1. kvark commented on Mar 31, 2020

    @kvark
    ContributorAuthor

    @emilio @Gankra could you post your thoughts on that?

  2. Gankra commented on Mar 31, 2020

    @Gankra
    Contributor

    We need to confer with the Rust devs to verify that Option<NonZeroU64> actually has the Integer type kind in rustc's ABI lowering (naively it is an Aggregate), and that this is guaranteed. As is, the libs docs only guarantee that it has the same size.

    It's also non-obvious that lowering this to a raw u64 is good API ergonomics, but if that's the ABI it has there's not much else we can do, since aiui C++ doesn't have repr(transparent) for us to wrap up the value with.

  3. kvark commented on Mar 31, 2020

    @kvark
    ContributorAuthor

    Can we invite them here? Or are you in any of the WGs that may raise this question on a call?
    For some context, this is needed for WebGPU, where Google's native implementation identifies objects by pointers, and ours - by these Id things that are transparently non-zero u64. What we'd like to have is a nullable semantics on the C API side.

  4. emilio commented on Mar 31, 2020

    @emilio
    Collaborator

    The Option<NonZero> optimization that cbindgen has is very local / naive. If you were using Option<NonZeroU64> we could simplify it away to u64 very easily.

    But having an extra type in the middle complicates it a bit more. This is an issue for the existing code already:

    pub type StaticRef = &'static i32;                                                                                                                                                                                                            
                                                                                     
    #[repr(C)]                                                                       
    pub struct Foo {                                                                 
        a: Option<StaticRef>, // Doesn't work                                        
        b: Option<&'static i32>, // works                                            
    }                                                                                
                                                                                     
    #[no_mangle]                                                                     
    pub extern "C" fn root(f: Foo) {}

    This all can be fixed of course, it's just quite a bit of error-prone work.

    That being said. For C++, your test-case generates Option<Bar> which you can specialize.

    For C, you can do struct Option_Foo { uint64_t id; } right? This seems to build:

    #include <stdarg.h>
    #include <stdbool.h>
    #include <stdint.h>
    #include <stdlib.h>
    
    struct Option_Foo { uint64_t maybe_id; };
    
    typedef struct Option_Foo Option_Foo;
    
    typedef struct {
      Option_Foo foo;
    } Bar;
    
    void root(Bar f);

    And you can trivially add an include with that definition. Passing Option_Foo by value on functions may not work / be guaranteed (that's @Gankra's concern), but I think you can pretty easily do this. Or am I missing something @kvark?

    That being said, I would've expected:

    [export]
    exclude = [
        "Option_Foo"                                                                                                                                                                                                                              
    ]

    To do what you want, but right now it doesn't happen because we remove the excluded types before instantiating the monomorph types for C. So I think that could work if you needed by moving the remove_excluded() call a bit later:

    https://github.com/eqrion/cbindgen/blob/723e690027209c8e613c729def6a9367e197b00f/src/bindgen/library.rs#L57-L69

  5. kvark commented on Mar 31, 2020

    @kvark
    ContributorAuthor

    Oh great, that workaround should do it for us, thanks!

    Edit: actually, not just yet. Having struct Option_Foo { uint64_t maybe_id; }; on the C side is not what we need.

  6. added a commit that references this issue on Mar 31, 2020
    eb78f21
  7. added a commit that references this issue on Apr 1, 2020
    ec1631f
  8. added 2 commits that reference this issue on Apr 4, 2020
  9. Gankra commented on Apr 15, 2020

    @Gankra
    Contributor

    oh fwiw i have verified that Option<NonZero*> should ~incidentally have the bare integer ABI, but it's not strictly written down to be guaranteed. I would be comfortable relying on it with a handshake agreement with the compiler devs to not break it.

  10. Gankra commented on Apr 16, 2020

    @Gankra
    Contributor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions