Skip to content

Support for abi_stable #10

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

Merged
merged 5 commits into from
Jan 18, 2022
Merged

Support for abi_stable #10

merged 5 commits into from
Jan 18, 2022

Conversation

marioortizmanero
Copy link
Contributor

@marioortizmanero marioortizmanero commented Nov 23, 2021

This is my attempt at adding support for abi_stable, as explained in #9.

The only changes I had to make were:

  • Add cfg_attr with derive(StableAbi) in case the sabi feature is enabled
  • Introduce FfiWakerBase, which is fully FFI-safe for only having FFI-safe fields (only the vtable).

TODO:

  • Document new feature
  • Make sure the tests still work
  • Is f066b51 worth it? IMO it makes more sense, and it might also be useful because we'd be able to hold more fields in the waker base, other than the vtable. But not sure if it's necessary at all. AFAIK casting between the types in both cases is safe anyway.

@oxalica
Copy link
Owner

oxalica commented Nov 23, 2021

Seems you are actually reverting #6 and go back to the "base pointer" approach instead of a union.
Personally I'd prefer the former since it makes more sense and avoid the std Waker being present in the FFI struct. It is kind of wired, and, as you said, breaks abi_stable compatibility. But the union approach surely makes the code more clear.
cc @PonasKovas

I used to consider another approach that decompose the Waker back into two opaque pointers, maybe passing through FFI, and recompose them back. This could eliminate the pointer-inheritance dance and the Boxing (so we could be no_std).
But currently std only expose (data_ptr, vtable) -> RawWaker but not reverse (RawWaker is repr(transparent) of Waker, so they can be safely transmuted). It would definitely requires some layout-assumption[1] to achieve it. But the core of this crate is to avoid this.

[1]: RawWaker's doc said: It consists of a data pointer and a virtual function pointer table (vtable) that customizes the behavior of the RawWaker. So we it must consists of two pointer, and there are only two possible layout, (data_ptr, vtable) and (vtable, data_ptr). We can use a compile-time condition to know which is one correct for the current compiling unit, and decompose it into a deterministic FfiWaker.

@marioortizmanero
Copy link
Contributor Author

marioortizmanero commented Nov 23, 2021

I honestly wouldn't mess around with transmute and RawWaker until its fields are properly exposed publicly. It's a pretty cool idea, but even if it's safe right now under certain conditions, it might not be in the future. It just seems risky and I think makes this crate harder to understand than it already is. We could perhaps open up some discussion regarding that on an official Rust channel and see if it's possible to get support for that in std (Zulip, Discord, etc). But that wouldn't happen anytime soon anyway.

@oxalica
Copy link
Owner

oxalica commented Dec 18, 2021

rust-lang/rust#87021

@marioortizmanero
Copy link
Contributor Author

Hi, sorry for taking so long to reply, holidays have been busy. Hope you had a wonderful Christmas and New Years!

@oxalica
Copy link
Owner

oxalica commented Jan 5, 2022

I'm still kind of worrying that whether abi_stable could correctly handle our FfiWakerBase which is actually transmute-d from FfiWaker. Is that just bypassing the checks of abi_stable so would lost the reliability when changing the underlying impl of FfiWaker in the future?

@marioortizmanero
Copy link
Contributor Author

marioortizmanero commented Jan 6, 2022

If it's not handled correctly by abi_stable most of the times it means that the library isn't ABI-safe either. Casting between FfiWaker and FfiWakerBase is completely fine according to the C standard, because FfiWakerBase is the first member in FfiWaker. It's how inheritance is manually implemented in C, you can read this for more info (disclosure: it's my blog).

@oxalica
Copy link
Owner

oxalica commented Jan 7, 2022

If it's not handled correctly by abi_stable most of the times it means that the library isn't ABI-safe either. Casting between FfiWaker and FfiWakerBase is completely fine according to the C standard, because FfiWakerBase is the first member in FfiWaker. It's how inheritance is manually implemented in C, you can read this for more info (disclosure: it's my blog).
* Should I look for the specific part in the standard where this is mentioned so that we can link it in the README as a safety guarantee?

Well, I know it's valid and pretty common in C. I just want to confirm that abi_stable handles this manual inheritance correctly, though I believe it's should be fine.

Thanks for the advice, but it's out of the scope of this PR. I'd consider it later.

For this PR, I'm asking for the two changes in discussions above, which are field name changing and feature name changing.

Copy link
Owner

@oxalica oxalica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other diffs LGTM.

@oxalica oxalica merged commit 08ec188 into oxalica:master Jan 18, 2022
@oxalica
Copy link
Owner

oxalica commented Jan 18, 2022

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants