Skip to content
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

Automatically add unittests to generated bindings that validate that C and D struct sizes match #254

Open
Cogitri opened this issue May 5, 2020 · 5 comments

Comments

@Cogitri
Copy link

Cogitri commented May 5, 2020

Otherwise programs using the bindings can SIGSEGV when using the structs due to size mismatches.

@jacob-carlborg
Copy link
Owner

I'm not sure I understand. The only reason the struct sizes would differ is if there's a bug in DStep. The whole point of DStep is to automate this to make sure it's correct.

@Cogitri
Copy link
Author

Cogitri commented May 11, 2020

The only reason the struct sizes would differ is if there's a bug in DStep.

Yes, the point of having these would be to make sure that in case DStep does have a bug it's caught immediately, since debugging size mismatches in the bindings tends to be rather hard to do.

E.g. dstep misgenerated the following C struct for me:

#define APK_ARRAY(array_type_name, elem_type_name)                      \
        struct array_type_name {                                        \
                size_t num;                                             \
                elem_type_name item[];                                  \
        }; 

APK_ARRAY(apk_string_array, char*);

It generated the following D code for that:

struct apk_string_array {
    size_t num;
    char*[] item;
}

Which is actually 8 bytes too big (16 bytes instead of 8). The correct definition would be:

struct apk_string_array {
    size_t num;
    char*[0] item;
}

If there was a

version(X86_64) {
    static assert(apk_string_array.sizeof == 8);
}

generated by dstep for me as well this would've been an easy fix and instantly spotted upon building the bindings the first time, but because dstep currently doesn't generate these I just happened to notice random SIGSEGVs in my program and had a rather hard time to actually debug them.
I guess the tests would be kind of a safety fallback in case dstep does generate wrong code, so bugs are found more easily. FWIW other bindings generators, such as rust-bindgen, generate these checks automatically. I had a few more places where the sizes mismatched in my generated bindings and it'd be very nice if these were detected automatically.

If you think it'd be a good idea to add these, I'll try to look into that in a bit - but exam time starts here in a bit, so maybe I'd try to make dstep my semester holiday project :)

@jacob-carlborg
Copy link
Owner

Hmm, that might be useful.

DStep would only be able to generate the tests for the platform it currently generates the bindings.

@Cogitri
Copy link
Author

Cogitri commented May 11, 2020

Yup, but that'd still be very useful.

@jacob-carlborg
Copy link
Owner

If you think it'd be a good idea to add these, I'll try to look into that in a bit

Yes, please do. It should be possible to enable/disable with a flag.

I'd try to make dstep my semester holiday project

That sounds like a great idea 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants