Add support for the thiscall ABI #1065
Conversation
|
|
|
Hello @liranringel ! Thanks for your PR! Unfortunately, we don't have nightly tests at the moment. But I believe that we should start to run them eventually. For example we have I'm not sure what is the best possible way here. |
|
For now, it is OK to land this without tests in CI, so long as the emitted bindings contain things that we can look at if/when the emitted bindings change in the future and verify that the changes look right or wrong manually. For this PR concretely, that would be the Eventually, we should be running rustc nightly tests in CI. I don't want to combinatorially explode our CI test jobs more than we already have, so let's not run all of our many test jobs in both stable and nightly. Instead, I think it makes sense to have just one more CI job that does expectations tests in nightly (since non-nightly can be used to verify that generated bindings aren't changing, and we just need nightly for compiling code with nightly features). When I said "For now, it is OK to land this without tests," I intentionally qualified that with "in CI." We can land tests that don't run in CI yet, but exist and can run locally:
Then, running should run this test locally! When we eventually add nightly CI, we can make it use the same feature. Ok, looking at the PR now :) |
There was a problem hiding this comment.
Overall, this is much more straightforward than I feared it might be :)
We have the concept of the "rust target" and its associated supported features. See for example
- definition: https://github.com/rust-lang-nursery/rust-bindgen/blob/723e93d1267e58cb1aafd04d221a0dff17e212d9/src/features.rs#L138-L159
- usage: https://github.com/rust-lang-nursery/rust-bindgen/blob/7bbfb4454e7c363c000b35a3b2e0d330c1435057/src/codegen/impl_partialeq.rs#L23
We use these to avoid generating bindings that require nightly features for the targeted Rust version/channel.
We should leverage this infrastructure for "thiscall" as well:
-
First, we should define a
thiscall_abifeature, that is only supported in the nightly rust target. -
Second, inside
impl Codegen for Function, if we see a"thiscall"ABI, andctx.options().rust_features().thiscall_abi()is nottrue, then we shouldwarn!("skipping function with thiscall ABI that isn't supported by the configured Rust target")and avoid generating any bindings for the function. -
Same as the last bullet but for
impl TryToRustTy for FunctionSig -
Finally, we should have two versions of the new test: one that targets nightly, and has the
cfgoptions to enable the"thiscall"ABI, and another that targets Rust stable 1.0. We just want to make sure we don't panic or generate bad bindings in the latter case.
Thanks a bunch @liranringel ! If anything doesn't make sense, let me know and I can try and elucidate :)
|
@fitzgen thanks for the comprehensive explanation. |
This is what the
Make sense? |
liranringel
force-pushed
the
thiscall
branch
3 times, most recently
from
8c02105
to
9b95ce6
Compare
|
Yes, you may check it. |
|
@bors-servo r+ |
|
📌 Commit 9b95ce6 has been approved by |
Add support for the thiscall ABI Fixes #541 The thiscall ABI is experimental, so in order to use it nightly is required and also the following statement: `#![feature(abi_thiscall)]` That's a problem because the `tests_expectations` crate (in the tests folder) tries to compile it (and stable is required).
|
☀️ Test successful - status-travis |
Fixes #541
The thiscall ABI is experimental, so in order to use it nightly is required and also the following statement:
#![feature(abi_thiscall)]That's a problem because the
tests_expectationscrate (in the tests folder) tries to compile it (and stable is required).