-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Add libstd support for Trusty targets #136842
base: master
Are you sure you want to change the base?
Conversation
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the static
-backed TLS implementation (the one also used by UEFI)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't tested against that implementation yet, unfortunately. We made these patches when before the static
TLS implementation was added (I think, I'm not entirely sure when that was added), so we've been using our own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static
-based implementation is very old, as it is used on WASM without atomics as well. Would it be possible for you to try switching to it? For one, it would reduce the amount of code that has to be maintained in std
. And this would fix the unsoundness mentioned in my other comment.
I'm concerned with how much is stubbed out. So far it seems like this only allows writing to stdout and provides an entropy source? Is there any possibility that more things may be implemented in the future? E.g. |
A lot ends up being stubbed out because Trusty userspace is pretty limited in terms of what you can do: No filesystem access, no threads, can't spawn processes, no network sockets, etc. We still make use of a good chunk of I do think we'll want to add |
Ok, I've nominated this for the libs team to discuss. While tier 3 libstd targets have a lot of latitude, there is some expectation that they should have or be working towards some basic level of libstd support. My concern would be that Trusty is fundamentally too limited for that and should remain |
@@ -207,6 +214,7 @@ impl AsRawFd for io::Stderr { | |||
} | |||
|
|||
#[stable(feature = "asraw_stdio_locks", since = "1.35.0")] | |||
#[cfg(not(target_os = "trusty"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a bunch of cfg
seems like a tacit admission that this implementation is not very useful, and depends on certain implementations never being even referenced.
In general, we have avoided adding cfg
to the implementations of std. I do not think we should get in the habit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better approach to follow for disabling unsupported parts of std? Would it be better to instead stub out platform APIs with unimplemented!
? That would involve fewer cfg
s, though I'm not sure how to handle tests with that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is really no satisfactory answer, to be clear, which is why we have shifted towards being increasingly reluctant to allow targets to add only partial support for std.
We discussed this in the libs meeting just now. Given that a few more things will likely be added (Instant, etc.), we're okay with moving forward with this as a tier 3 target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static
-based implementation is very old, as it is used on WASM without atomics as well. Would it be possible for you to try switching to it? For one, it would reduce the amount of code that has to be maintained in std
. And this would fix the unsoundness mentioned in my other comment.
@@ -178,7 +180,7 @@ pub mod vxworks; | |||
#[cfg(target_os = "xous")] | |||
pub mod xous; | |||
|
|||
#[cfg(any(unix, target_os = "hermit", target_os = "wasi", doc))] | |||
#[cfg(any(unix, target_os = "hermit", target_os = "trusty", target_os = "wasi", doc))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole module is about providing safe abstractions for file descriptors that maintain IO safety. But if I understand the documentation correctly, there is no way to open or close file descriptors on Trusty, meaning the whole concept of IO safety is unnecessary. Thus I don't think that this module should exist on Trusty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do make heavy use of OwnedFd
in Trusty. While userspace applications can't directly open e.g. a file, we do have a concept of handles that are used for things like IPC. We'd also like to have access to the os::fd
module because Trusty shares code with Android, and it'd be difficult if we had two different fd types. Sharing the same fd infrastructure in libstd simplifies things for us a lot.
@joboet Fair point, I've wanted to switch us to the common static TLS implementation anyway. That's done now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm broadly ok with this now but it will need a rebase to take account of the reorganisational of the standard library. E.g. library/std/src/sys/fs/
.
Unclear why this needs to be done manually and is not done by the existing Trusty patches.
34ca6c0
to
2b3b0bd
Compare
I have rebased the changes and updated them to account for the latest libstd reorganization. |
Ok, I'm still a bit hesitant about a number of the @bors r+ |
@bors r- |
Co-authored-by: bjorn3 <[email protected]>
@bors r+ |
…hrisDenton Add libstd support for Trusty targets This PR adds support for `alloc` and `std` for the Trusty targets based on the internal patches used in Android. The original patches can be seen [here](https://android.googlesource.com/toolchain/android_rust/+/refs/heads/main/patches/development/rustc-0023-Add-Trusty-OS-support-to-Rust-std.patch) and [here](https://android.googlesource.com/toolchain/android_rust/+/refs/heads/main/patches/development/rustc-0054-Add-std-os-fd-support-for-Trusty.patch). Please let me know if there's any additional context I need to add.
…hrisDenton Add libstd support for Trusty targets This PR adds support for `alloc` and `std` for the Trusty targets based on the internal patches used in Android. The original patches can be seen [here](https://android.googlesource.com/toolchain/android_rust/+/refs/heads/main/patches/development/rustc-0023-Add-Trusty-OS-support-to-Rust-std.patch) and [here](https://android.googlesource.com/toolchain/android_rust/+/refs/heads/main/patches/development/rustc-0054-Add-std-os-fd-support-for-Trusty.patch). Please let me know if there's any additional context I need to add.
Rollup of 25 pull requests Successful merges: - rust-lang#134076 (Stabilize `std::io::ErrorKind::InvalidFilename`) - rust-lang#136842 (Add libstd support for Trusty targets) - rust-lang#137314 (change definitely unproductive cycles to error) - rust-lang#137504 (Move methods from Map to TyCtxt, part 4.) - rust-lang#137621 (Add std support to cygwin target) - rust-lang#137701 (Convert `ShardedHashMap` to use `hashbrown::HashTable`) - rust-lang#138109 (make precise capturing args in rustdoc Json typed) - rust-lang#138161 (Add PeekMut::refresh) - rust-lang#138162 (Update the standard library to Rust 2024) - rust-lang#138174 (Elaborate trait assumption in `receiver_is_dispatchable`) - rust-lang#138175 (Support rmeta inputs for --crate-type=bin --emit=obj) - rust-lang#138269 (uefi: fs: Implement FileType, FilePermissions and FileAttr) - rust-lang#138313 (Update books) - rust-lang#138318 (Rustdoc: remove a bunch of `@ts-expect-error` from main.js) - rust-lang#138331 (Use `RUSTC_LINT_FLAGS` more) - rust-lang#138333 (Rebuild llvm spuriously less frequently) - rust-lang#138343 (Enable `f16` tests for `powf`) - rust-lang#138345 (Some autodiff cleanups) - rust-lang#138346 (naked functions: on windows emit `.endef` without the symbol name) - rust-lang#138347 (Reduce `kw::Empty` usage, part 2) - rust-lang#138360 (Fix false-positive in `expr_or_init` and in the `invalid_from_utf8` lint) - rust-lang#138371 (Update compiletest's `has_asm_support` to match rustc) - rust-lang#138372 (Refactor `pick2_mut` & `pick3_mut` to use `get_disjoint_mut`) - rust-lang#138376 (Item-related cleanups) - rust-lang#138377 (Remove unnecessary lifetime from `PatInfo`.) r? `@ghost` `@rustbot` modify labels: rollup
This PR adds support for
alloc
andstd
for the Trusty targets based on the internal patches used in Android. The original patches can be seen here and here. Please let me know if there's any additional context I need to add.