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

RFC: add feature "abi_stable" and make types abi stable #1446

Closed
gwy15 opened this issue Feb 18, 2024 · 7 comments
Closed

RFC: add feature "abi_stable" and make types abi stable #1446

gwy15 opened this issue Feb 18, 2024 · 7 comments

Comments

@gwy15
Copy link

gwy15 commented Feb 18, 2024

Hello. In my project, I'm using Rust to load a dynamic library from an executable, and I'm utilizing certain chrono types across the ABI (Application Binary Interface) boundary. To ensure that the program does not crash, I'm employing the crate abi_stable to guarantee that the layouts of types passing through the ABI boundary are consistent.

At present, I have created my own fork of chrono and added an "abi_stable" feature to it. This modification ensures that types such as DateTime maintain ABI stability and can be safely passed through ABI boundaries. I am interested in contributing this modification to the upstream repository. What are your thoughts on this idea?

@pitdicker
Copy link
Collaborator

pitdicker commented Feb 21, 2024

I don't understand this request and its consequences well enough.

  • What stability guarantees would we have to give around our internal representation?
  • How does this work with DateTime, which takes a generic TimeZone that may not come from chrono?
  • Is the abi_stable crate the way to go and getting some acceptance in the ecosystem? It has existed for 5 years, but only has 1200 downloads/day.
  • Is any type supported, such as NonZero*?
  • What is the difference with making some types #[repr(C)]?
  • Is this something that can't be implemented in another crate? Why not?

At present, I have created my own fork of chrono and added an "abi_stable" feature to it.

Could you point to your branch (may be unpolished)?

@pitdicker
Copy link
Collaborator

Another question. How is the safety story for abi_stable?
Rust has an unstable ABI, this crate offers a workaround. abi_stable has its own unsafe_code_guidelines. What transmutes or things is it doing? Has anyone vetted this crates safety? Is there a chance it may break with a future rust version?

@pitdicker
Copy link
Collaborator

How is the safety story for abi_stable?

This comment makes me uneasy:

😦 This would be two orders of magnitude more work than every other time that the compiler decided to break my code combined.

It shows there is nontrivial unsafety, in a way that may not agree with the opinions of the rust language/compiler.

@gwy15
Copy link
Author

gwy15 commented Feb 21, 2024

  • The main reason I want to introduce abi_stable into chrono, instead of purely using #[repr(C)], is that it would provide a trait (StableAbi) and provides a link of trust and the ability to check memory layout in runtime, which #[repr(C)] does not provide.
  • There are no stability guarantees that chrono has to give. Let's say there's a breaking changes in memory layout, executables and dynamic libraries that replys on two different versions of chronos should detect the different memory layout in runtime using the StableAbi crate and thus prevents crashing, which is the reason I want to introduce abi_stable.
  • AFAIK, in my implementation, I bounded the StableAbi trait to only implement on Timezone that implements StableAbi.
  • AFAIK, abi_stable is currently the only way to do this (runtime check memory layout and abi stability)
  • StableAbi is implemented for NonZero* and Option<NonZero*>, but not all types (those who do not guarantees a memory layout stability in rust reference)
  • Due to the orphan rule, I cannot implement this in anther crate, even with wrapped new types.

@gwy15
Copy link
Author

gwy15 commented Feb 21, 2024

This is the proposed commit: 81f0c1a

@gwy15
Copy link
Author

gwy15 commented Feb 21, 2024

I agree that my use case may not be that common (putting a lot of computation and business code in dynamic library to keep the core code clean, and passing structs through ffi boundards). And I can totally understand that for that, introducing such a lesser-used crate into chrono may not be accepted.

But for those do have this use case, since Rust does not provide a stable ffi, abi_stable is by far the best solution. And it is exactly the fact that it is lesser-used, unlike serde which most of crates provide features, not many, actually very few crates provides this feature. And due to the orphan rule, I can only fork and modify structs passing through ffi boundary.

@djc
Copy link
Contributor

djc commented Feb 21, 2024

Given the lack of popularity of the abi_stable crate, I don't think we should do this. Note also that if we commit to abi_stable 0.11.x in chrono 0.4.x we can't bump abi_stable to 0.12.x or later on chrono 0.4.x because this dependency will become part of our public API.

I think your best bet would be either on using some kind of fast serialization (like postcard or rkyv) or (longer term) on the efforts to provide Rust with a more stable ABI.

@djc djc closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2024
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

No branches or pull requests

3 participants