Rust-bindgen: Make it possible so that function pointers aren't always wrapped in an Option.

[raphaelcohn]

I'm working on bindings of a C library (OpenUCX) where the majority, but not all, of function pointers used are never null (both typedefs and struct fields) - hence they shouldn't be generated as Option<extern fn ...> but as extern fn .... A very small number, however, can be null, and so needs to stay as Option<extern fn ..>. It'd be nice to do this via bindgen. Currently I post-process them using bindgen-wrapper (a horribly brittle tool of my own devising).

Would it possible for bindgen to take a list to override this behaviour, of structs and typedefs?

[emilio]

What's so bad about calling unwrap() / wrapping it with Some(fn)? We can certainly add an API for this but this hasn't looked really annoying so far, at least for me.

[raphaelcohn]

1. It uses code as documentation - what the code says is what's correct, and the type system makes it clear when something is genuinely null. (A similar argument could be made for NonNull<x> vs *mut X, although that's something I haven't explored with UCX).

2. There is a performance hit with unwrap of two extra x86-64 instructions for every unwrap of an Option in release mode. In OpenUCX this can actually matter - it abstracts away RDMA operations.

3. It is is annoying; it pollutes every call, making basic code obnoxiously verbose for no gain

4. It makes it tedious to work with C libraries which implement 'classes' as a gigantic (struct) table of function pointers... eg

/**
 * Transport interface operations.
 * Every operation exposed in the API should appear in the table below, to allow
 * creating interface/endpoint with custom operations.
 */
typedef struct uct_iface_ops {

    /* endpoint - put */

    ucs_status_t (*ep_put_short)(uct_ep_h ep, const void *buffer, unsigned length,
                                 uint64_t remote_addr, uct_rkey_t rkey);

    ssize_t      (*ep_put_bcopy)(uct_ep_h ep, uct_pack_callback_t pack_cb,
                                 void *arg, uint64_t remote_addr, uct_rkey_t rkey);

    ucs_status_t (*ep_put_zcopy)(uct_ep_h ep, const uct_iov_t *iov, size_t iovcnt,
                                 uint64_t remote_addr, uct_rkey_t rkey,
                                 uct_completion_t *comp);

    /* endpoint - get */

    ucs_status_t (*ep_get_bcopy)(uct_ep_h ep, uct_unpack_callback_t unpack_cb,
                                 void *arg, size_t length,
                                 uint64_t remote_addr, uct_rkey_t rkey,
                                 uct_completion_t *comp);

    ucs_status_t (*ep_get_zcopy)(uct_ep_h ep, const uct_iov_t *iov, size_t iovcnt,
                                 uint64_t remote_addr, uct_rkey_t rkey,
                                 uct_completion_t *comp);

    /* endpoint - active message */

    ucs_status_t (*ep_am_short)(uct_ep_h ep, uint8_t id, uint64_t header,
                                const void *payload, unsigned length);

    ssize_t      (*ep_am_bcopy)(uct_ep_h ep, uint8_t id,
                                uct_pack_callback_t pack_cb, void *arg,
                                unsigned flags);

    ucs_status_t (*ep_am_zcopy)(uct_ep_h ep, uint8_t id, const void *header,
                                unsigned header_length, const uct_iov_t *iov,
                                size_t iovcnt, unsigned flags,
                                uct_completion_t *comp);

    /* endpoint - atomics */

    ucs_status_t (*ep_atomic_add64)(uct_ep_h ep, uint64_t add,
                                    uint64_t remote_addr, uct_rkey_t rkey);

    ucs_status_t (*ep_atomic_fadd64)(uct_ep_h ep, uint64_t add,
                                     uint64_t remote_addr, uct_rkey_t rkey,
                                     uint64_t *result, uct_completion_t *comp);

    ucs_status_t (*ep_atomic_swap64)(uct_ep_h ep, uint64_t swap,
                                     uint64_t remote_addr, uct_rkey_t rkey,
                                     uint64_t *result, uct_completion_t *comp);

    ucs_status_t (*ep_atomic_cswap64)(uct_ep_h ep, uint64_t compare, uint64_t swap,
                                      uint64_t remote_addr, uct_rkey_t rkey,
                                      uint64_t *result, uct_completion_t *comp);

    ucs_status_t (*ep_atomic_add32)(uct_ep_h ep, uint32_t add,
                                    uint64_t remote_addr, uct_rkey_t rkey);

    ucs_status_t (*ep_atomic_fadd32)(uct_ep_h ep, uint32_t add,
                                     uint64_t remote_addr, uct_rkey_t rkey,
                                     uint32_t *result, uct_completion_t *comp);

    ucs_status_t (*ep_atomic_swap32)(uct_ep_h ep, uint32_t swap,
                                     uint64_t remote_addr, uct_rkey_t rkey,
                                     uint32_t *result, uct_completion_t *comp);

    ucs_status_t (*ep_atomic_cswap32)(uct_ep_h ep, uint32_t compare, uint32_t swap,
                                      uint64_t remote_addr, uct_rkey_t rkey,
                                      uint32_t *result, uct_completion_t *comp);

    /* endpoint - tagged operations */

    ucs_status_t (*ep_tag_eager_short)(uct_ep_h ep, uct_tag_t tag,
                                       const void *data, size_t length);

    ssize_t      (*ep_tag_eager_bcopy)(uct_ep_h ep, uct_tag_t tag, uint64_t imm,
                                       uct_pack_callback_t pack_cb, void *arg,
                                       unsigned flags);

    ucs_status_t (*ep_tag_eager_zcopy)(uct_ep_h ep, uct_tag_t tag, uint64_t imm,
                                       const uct_iov_t *iov, size_t iovcnt,
                                       unsigned flags, uct_completion_t *comp);

    ucs_status_ptr_t (*ep_tag_rndv_zcopy)(uct_ep_h ep, uct_tag_t tag,
                                          const void *header,
                                          unsigned header_length,
                                          const uct_iov_t *iov,
                                          size_t iovcnt, unsigned flags,
                                          uct_completion_t *comp);

    ucs_status_t (*ep_tag_rndv_cancel)(uct_ep_h ep, void *op);

    ucs_status_t (*ep_tag_rndv_request)(uct_ep_h ep, uct_tag_t tag,
                                        const void* header,
                                        unsigned header_length, unsigned flags);

    /* interface - tagged operations */

    ucs_status_t (*iface_tag_recv_zcopy)(uct_iface_h iface, uct_tag_t tag,
                                         uct_tag_t tag_mask,
                                         const uct_iov_t *iov,
                                         size_t iovcnt,
                                         uct_tag_context_t *ctx);

    ucs_status_t (*iface_tag_recv_cancel)(uct_iface_h iface,
                                          uct_tag_context_t *ctx,
                                          int force);

    /* endpoint - pending queue */

    ucs_status_t (*ep_pending_add)(uct_ep_h ep, uct_pending_req_t *n);

    void         (*ep_pending_purge)(uct_ep_h ep, uct_pending_purge_callback_t cb,
                                     void *arg);

    /* endpoint - synchronization */

    ucs_status_t (*ep_flush)(uct_ep_h ep, unsigned flags,
                             uct_completion_t *comp);

    ucs_status_t (*ep_fence)(uct_ep_h ep, unsigned flags);

    ucs_status_t (*ep_check)(uct_ep_h ep, unsigned flags, uct_completion_t *comp);

    /* endpoint - connection establishment */

    ucs_status_t (*ep_create)(uct_iface_h iface, uct_ep_h *ep_p);

    ucs_status_t (*ep_create_connected)(uct_iface_h iface,
                                        const uct_device_addr_t *dev_addr,
                                        const uct_iface_addr_t *iface_addr,
                                        uct_ep_h* ep_p);

    ucs_status_t (*ep_create_sockaddr)(uct_iface_h iface,
                                       const ucs_sock_addr_t *sockaddr,
                                       const void *priv_data, size_t length,
                                       uct_ep_h *ep_p);

    void         (*ep_destroy)(uct_ep_h ep);

    ucs_status_t (*ep_get_address)(uct_ep_h ep, uct_ep_addr_t *addr);

    ucs_status_t (*ep_connect_to_ep)(uct_ep_h ep,
                                     const uct_device_addr_t *dev_addr,
                                     const uct_ep_addr_t *ep_addr);

    /* interface - synchronization */

    ucs_status_t (*iface_flush)(uct_iface_h iface, unsigned flags,
                                uct_completion_t *comp);

    ucs_status_t (*iface_fence)(uct_iface_h iface, unsigned flags);

    /* interface - progress control */

    void         (*iface_progress_enable)(uct_iface_h iface, unsigned flags);

    void         (*iface_progress_disable)(uct_iface_h iface, unsigned flags);

    unsigned     (*iface_progress)(uct_iface_h iface);

    /* interface - events */

    ucs_status_t (*iface_event_fd_get)(uct_iface_h iface, int *fd_p);

    ucs_status_t (*iface_event_arm)(uct_iface_h iface, unsigned events);

    /* interface - management */

    void         (*iface_close)(uct_iface_h iface);

    ucs_status_t (*iface_query)(uct_iface_h iface,
                                uct_iface_attr_t *iface_attr);

    /* interface - connection establishment */

    ucs_status_t (*iface_get_device_address)(uct_iface_h iface,
                                             uct_device_addr_t *addr);

    ucs_status_t (*iface_get_address)(uct_iface_h iface, uct_iface_addr_t *addr);

    int          (*iface_is_reachable)(const uct_iface_h iface,
                                       const uct_device_addr_t *dev_addr,
                                       const uct_iface_addr_t *iface_addr);

} uct_iface_ops_t;

[emilio]

Well, if you do care about the instruction count I guess that you can unsafely match / transmute, like:

#[macro_use] extern crate debug_unreachable;

match my_func {
     Some(f) => f,
     None => debug_unreachable!(),
};

But yeah, I agree. Generally we've supported field annotations via doc comments, but if you're using a library that may not be a great solution. For C this is easy-ish to map to a struct + name, for C++.. not so much I guess.

I can mentor this if you want, I don't know if a ParseCallbacks API vs. just an add-hoc listing would suit this kind of use case better.

[raphaelcohn]

Thank for the offer to mentor. If I wasn't so snowed under with work I'd love to take up your offer. I make extensive use of bindgen in my projects and would love to get some changes to make it easier in my workflow.

[norcalli]

I'd like this not for some performance reason but so that I could declare function pointers for initializing a struct of function pointers which are guaranteed not to be null. The option wrapped version makes this impossible. This is a useful pattern when loading dynamic libraries rather than linking against a library at compile time.