cbindgen icon indicating copy to clipboard operation
cbindgen copied to clipboard

Support explicit template instantiation

Open fanzeyi opened this issue 6 years ago • 15 comments

I wonder if it is possible to add an annotation for functions to customize the return type of a function, something like this:

/// cbindgen:return-type=Foobar
pub extern "C" fn foobar() -> Foobar<abc>;

Some background: we are trying to use cbindgen to generate some code that will be compiled with MSVC toolchain. cbindgen will generate template for Rust generics. This worked well with clang and gcc but not MSVC. So we decided to manually define the generic type with some void pointer and manually casting it to the concrete type (the only generic in the Rust struct is a pointer so we can do this). However, cbindgen will still generate the return type for functions with template parameter which we do not want. So I think it would be nice if cbindgen allows us to customize it with some annotations.

I understand this is not a very reasonable feature request, and I don't think many people would be in a situation like this. Please let me know what do you think. I can work on adding this feature if you guys think it's okay.

Thanks!

fanzeyi avatar Oct 15 '19 18:10 fanzeyi

Does using a type alias work around the issue? Could you post some concrete code so I can see how that looks?

In general it might not be totally unreasonable, but it feels odd to add this for return types and not arguments, or members...

Thanks!

emilio avatar Oct 17 '19 14:10 emilio

This is going to be a very niche case but nevertheless, we have a Rust type defined like this:

#[repr(C)]
pub struct CResult<T> {
    value: *mut T,
    error: *mut c_char,
}

and we have Rust extern functions returning this type to C/C++:

pub extern "C" fn foobar() -> CResult<u16> {
}

cbindgen will generate a C++ function with this signature:

CResult<uint16_t> foobar();

This works fine with gcc & clang. However, MSVC does not really like to have template in extern functions. So in order for this to work we have to manually define CResult as:

struct CResult {
    void *value;
    char *error;
}

However, cbindgen will still generate functions with return type with templates. This is correct and understandable. Since we are already manually defining the return type we should be defining these return types manually as well. But still, I want to avoid manually defining these things as much as possible, hence this issue. If we are allowed to customize the return type in the form of annotation, it would help us to avoid manually define these functions.

I hope this can help you understand the situation we are at. Thanks.

fanzeyi avatar Nov 06 '19 21:11 fanzeyi

If you cannot do templates in extern, you may be better off using --lang c, or c-with-cpp-compat? That monomorphizes the inputs, so you'd get something like:

#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdlib.h>

typedef struct {
  uint16_t *value;
  char *error;
} CResult_u16;

CResult_u16 foobar(void);

Which should work fine in msvc as well. I think that's less hacky than what you're proposing. My point above is that there's no reason you may want to tweak the return value and not arg in something like:

extern "C" fn foobar(arg: CResult<u16>) {
}

right?

emilio avatar Nov 06 '19 23:11 emilio

For whomever is running into this issue there is a "better" solution than the above "compat" one:

template struct CResult<uint16_t>;
CResult<uint16_t> foobar();

This seems to solve the problem, and I'd be willing to open a pull request to do this for all generic structs used in a function definition.

adrian-budau avatar Dec 20 '19 15:12 adrian-budau

So that explicitly instantiates it, right? Is that enough to appease msvc? Can you post the full msvc error message? it's unclear to me what it's complaining about.

emilio avatar Dec 20 '19 20:12 emilio

Sorry I wasn't able to help providing more information on this issue lately.

If you try to compile this C++ code:

template<typename T>
struct Result;

extern "C" Result<void> foobar();

You will get this error message with MSVC toolchain:

<source>(5): error C2526: 'foobar': C linkage function cannot return C++ class 'Result<void>'
<source>(5): note: see declaration of 'Result<void>'

godbolt

fanzeyi avatar Dec 22 '19 17:12 fanzeyi

And if you instantiate it: https://godbolt.org/z/dNyyVe.

It doesn’t seem to have any negative effect on the other compilers.

adrian-budau avatar Dec 22 '19 17:12 adrian-budau

Sure, sounds fine to do that then, but with the precondition of a bug reported to MSVC... I don't think the explicit instantiation should be needed.

emilio avatar Dec 23 '19 16:12 emilio

I can't tell if it's really a bug. clang seems to complain as well (altough just a warning): https://godbolt.org/z/EpRsrn

The explicit instantiation seems to fix it as in the MSVC case: https://godbolt.org/z/Q4FgYE

The only thing that buggs me with this "fix" is that we'd need a way to make sure we don't double instantiate, as it seems GCC and CLANG have a problem with that:

https://godbolt.org/z/5z8ZFZ, https://godbolt.org/z/C_6r5v

adrian-budau avatar Dec 24 '19 16:12 adrian-budau

Well, the clang warning makes sense, but it's just that, a warning. It just says that since it doesn't know whether there's an specialization or not somewhere else, someone could make it ABI-incompatible by, e.g., adding a destructor.

emilio avatar Dec 24 '19 18:12 emilio

And yeah, it's annoying that multiple instantiations are rejected... Probably we should output all the explicit instantiations right after the class definition (without duplicates).

emilio avatar Dec 24 '19 18:12 emilio

It’s still annoying because of typedefs (specialize for Mile and Kilometer, but both are actually int32_t).

A “hacky” solution would be to just implicitly instantiate:

Result<void> _random_unused_global;

As long as you don’t repeat names, this works.

Or maybe add a config to “blacklist” explicit instantionations.

adrian-budau avatar Dec 24 '19 18:12 adrian-budau

That kinda relies on the linker not linking them right? That is indeed a bit annoying as well.

I wonder if there's a way to force instantiation without affecting the compilation unit... maybe using static_assert or such?

emilio avatar Jan 25 '20 20:01 emilio

I found a "nicer" way. Needed a fix since it's something I use at work:

struct _Helper_0 {
  Result<void> field;
};

This seems to fix the problem in all compilers and it only "slightly" pollutes the symbol list in the binary (which LTO can eliminate).

adrian-budau avatar Jan 27 '20 10:01 adrian-budau

So, I did some work on FFI-safety, and the best way I've found to do this so far without tweaking cbindgen is just adding the explicit instantiations in the trailer. This seems to be enough for clang at least to realize, even though it's after the function declarations.

It doesn't seem to be enough for MSVC, but it's enough for my clang static analysis to realize.

That being said, cbindgen already has code to monomorphize all the templates. I think we should do something like adding a config option like instantiate_templates, that in C++ does an instantiate_monomorph pass, which should be generated right after the struct definition.

That would generate the desired code without duplicates and such.

emilio avatar Feb 04 '20 19:02 emilio