-
Notifications
You must be signed in to change notification settings - Fork 318
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
Support explicit template instantiation #402
Comments
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! |
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 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. |
If you cannot do templates in extern, you may be better off using #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 extern "C" fn foobar(arg: CResult<u16>) {
} right? |
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. |
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. |
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:
|
And if you instantiate it: https://godbolt.org/z/dNyyVe. It doesn’t seem to have any negative effect on the other compilers. |
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. |
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: 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: |
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. |
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). |
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:
As long as you don’t repeat names, this works. Or maybe add a config to “blacklist” explicit instantionations. |
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 |
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). |
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 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 That would generate the desired code without duplicates and such. |
I wonder if it is possible to add an annotation for functions to customize the return type of a function, something like this:
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!
The text was updated successfully, but these errors were encountered: