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

Disable the BorrowedFd layout optimization on Haiku #112371

Closed

Conversation

sunfishcode
Copy link
Member

On Haiku (and apparently only Haiku), the platform AT_FDCWD has the value -1. However, BorrowedFd currently has layout optimizations that assume the value is not -1, and it's useful to be able to store AT_FDCWD in a BorrowedFd. For example, rustix has
a function which returns a BorrowedFd containing AT_FDCWD.

To support this on Haiku, disable the layout optimizations for BorrowedFd on Haiku and allow it to store the value -1.

This is technically a breaking change for Haiku users, as they could theoretically be depending on Option<BorrowedFd> having the same layout as RawFd, however this is not a common idiom, and Haiku support is Tier 3 in Rust.

On Haiku (and apparently only Haiku), the platform `AT_FDCWD` has the value
`-1`. However, `BorrowedFd` currently has layout optimizations that assume the
value is not `-1`, and it's useful to be able to store `AT_FDCWD` in a
`BorrowedFd`. For example, rustix has
[a function which returns a `BorrowedFd` containing `AT_FDCWD`].

To support this on Haiku, disable the layout optimizations for `BorrowedFd` on
Haiku and allow it to store the value `-1`.

This is technically a breaking change for Haiku users, as they could
theoretically be depending on `Option<BorrowedFd>` having the same layout as
`RawFd`, however this is not a common idiom, and Haiku support is [Tier 3]
in Rust.

[a function which returns a `BorrowedFd` containing `AT_FDCWD`]: https://docs.rs/rustix/latest/rustix/fs/fn.cwd.html
[Tier 3]: https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-3
@rustbot
Copy link
Collaborator

rustbot commented Jun 7, 2023

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 7, 2023
@joshtriplett
Copy link
Member

This is technically a breaking change for Haiku users, as they could theoretically be depending on Option<BorrowedFd> having the same layout as RawFd, however this is not a common idiom, and Haiku support is Tier 3 in Rust.

I don't think it's as simple as this. Whatever tier a target is, people are going to ask random crates for support for it, and those crates have to consider whether to support that target or not. And if there is any platform where Option<BorrowedFd> does not work in FFI, that seems like it will more broadly invalidate that usage; anyone using Option<BorrowedFd> in an FFI interface would be unable to ever work on Haiku (or, possibly, even compile at all).

I think we need to make this decision on a broader basis: what do we expect developers in the ecosystem to do?

  • Assume this layout optimization, and ignore Haiku?
  • Abandon this layout optimization everywhere?
  • Use this layout optimization with care and only in non-portable code, and never use it in portable code?

What does Haiku use for "invalid file descriptor"? Are there any Haiku syscalls or library calls where you can pass an optional file descriptor, and there's a (hopefully consistent) placeholder value used to represent not providing a file descriptor?

From what I can tell, Haiku syscalls seem to use large negative numbers as their error values: B_GENERAL_ERROR_BASE is INT_MIN, and then error codes go up from there. But that implies it can return a variety of invalid values, not just one. I don't know if the library functions wrapping those syscalls consistently return a single placeholder value or not.

@sunfishcode
Copy link
Member Author

I don't think it's as simple as this. Whatever tier a target is, people are going to ask random crates for support for it, and those crates have to consider whether to support that target or not. And if there is any platform where Option<BorrowedFd> does not work in FFI, that seems like it will more broadly invalidate that usage; anyone using Option<BorrowedFd> in an FFI interface would be unable to ever work on Haiku (or, possibly, even compile at all).

I think we need to make this decision on a broader basis: what do we expect developers in the ecosystem to do?

In most cases, this layout optimization is just an optimization, so it's not a major problem if some platforms get it and Haiku doesn't.

The one place that does legitimately depend on the layout optimization is FFI declarations. Here, Option<BorrowedFd> is uncommon, and when it does occur, with this PR users will get a "not FFI-safe" warning when compiling for Haiku. They may need to use RawFd instead in those places, which seems like a reasonable outcome.

What does Haiku use for "invalid file descriptor"? Are there any Haiku syscalls or library calls where you can pass an optional file descriptor, and there's a (hopefully consistent) placeholder value used to represent not providing a file descriptor?

Haiku's documentation just says it followed POSIX, so I assume open has to return -1 on failure, otherwise they'd gratuitously break a lot of code.

It happens that the places in POSIX where a -1 fd can meaningfully be passed or returned are disjoint from the places were AT_FDCWD can meaningfully be passed or returned.

@joshtriplett
Copy link
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 11, 2024
@rustbot rustbot assigned m-ou-se and unassigned joshtriplett Feb 11, 2024
@m-ou-se m-ou-se added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-nominated Nominated for discussion during a libs team meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 15, 2024
@sunfishcode
Copy link
Member Author

Haiku accepted a patch changing the AT_FDCWD value from -1 to -100, so there's now a path forward that doesn't need this PR.

@sunfishcode sunfishcode deleted the sunfishcode/haiku-at-fdcwd branch February 26, 2024 14:42
@workingjubilee workingjubilee added the O-haiku Target: Be extant; from mouldering old leaves; spring arrives again label Jul 17, 2024
@dtolnay dtolnay removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Sep 9, 2024
@sunfishcode
Copy link
Member Author

As of the newly released Haiku R1/beta5, and the Rust libc crate version 0.2.155, the AT_FDCWD value on Haiku is now -100, so now Haiku works like every other platform in this respect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-haiku Target: Be extant; from mouldering old leaves; spring arrives again S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants