-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
target_os = "custom"
: selecting/swapping platform-specific parts of the libstd at runtime
#115506
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
|
Some changes occurred in src/tools/cargo cc @ehuss |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #115593) made this pull request unmergeable. Please resolve the merge conflicts. |
fce8fdc
to
6749490
Compare
This comment has been minimized.
This comment has been minimized.
Rerolling -- I'm afraid I don't have time to review such a large PR, especially since I'm not aware of prior context. r? libs |
6749490
to
4081b49
Compare
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
I suppose I'm a reasonable choice for a reviewer since I maintain a heavily patched out-of-tree libstd for a "custom" OS. I think whether we want to do this (and if this is the approach we want to take) should probably be discussed further though. |
@thomcc thank you for your reply! I started a zulip topic for this minutes ago haha :) |
☔ The latest upstream changes (presumably #116407) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
I'm going to nominate this PR for T-libs and T-libs-api, since I agree with others on thread that doing this is a larger decision than any individual reviewer should probably make. My sense is that it may be a good idea to write up a full RFC proposing this (presuming the teams are generally onboard), particularly so that we have space to explore the design outside of a specific implementation, consider what other languages do in this space, etc. |
Yep. I don't when I'll be able to, but I do want to create an RFC for this. |
There are a lot of higher level questions that need discussion and answering before it makes sense to dive into one possible implementaiton like in this PR. (There is also ongoing work to clean up and refactor std::sys, which should happen before anything like this.) It'd be great to have a better way to support operating systems out of tree, but we first need to have a discussion on the possible approaches and the relevant trade-offs. There are also questions around stability of this OS implementation interface. I'm hopeful that some day we will have a feature like proposed here that would allow the implementation of the OS-specific details to live outside std. I think the right approach would be to first research the various options, including the what a solution would look like with a new language feature such as proposed in RFC 2492, perhaps as part of a (pre) RFC. |
I fully agree that the proposal in RFC 2492 would allow for a better implementation of what I proposed here. I took the "runtime" approach but a compile-time one would certainly be better; I don't have enough knowledge on linkage to take such an approach. I also agree to create an RFC for this. I no longer develop for |
This requires a (pre-)RFC so it's better to close this and create a new PR if interested once the RFC is approved |
On experimental kernels and other
no_std
platform, it can be desirable to use the standard library, either directly or in dependencies.To do so, one currently needs to:
library/std/src/sys
unsupported
)As an alternative, this PR adds a "custom" value for
target_os
, which reduces this workflow to:"os": "custom",
to theirtarget.json
build-std
feature of cargo (note: see "Built-in target definitions" below)Suboptimal but cross platform implementations are included for thread local storage and locking primitives; additionally, an initial allocator is provided (with a fixed heap capacity of 16KiB).
API
Here are the built docs for this.
Design Decisions
For each subsystem, there is a singleton:
In
library/std/src/sys
, functions use a macro call:which translates to:
Initially, the singletons contains
None
. Once a singleton is set (toSome(_)
), runtime implementations cannot be removed, they can only be swapped: a singleton will never return to aNone
state.This boxed design is not as efficient as a direct extern call, but for experimental projects which actually lead to something useful, the backing implementation can easily be moved into the stdlib later.
Additionally, if I had taken the path of static
extern
definitions for this project, I feel like that would have complicated the use in projects which have custom link scripts and linking procedures. Also, this would prevent runtime swapping of implementations, which I feel can be useful to follow the booting procedure of custom kernels. I'm not sure how theglobal_allocator
thing work, does it use staticextern
definitions?Built-in target definitions
This PR doesn't add any target definition.
However, if we added built-in defs for common bare-bones environments:
That would ease the use of this work even more for relevant projects.
TLS: Inlined
__getit
In
library/std/src/sys/common/thread_local/os_local.rs
, I cfg-gated theinline
attribute for custom because for some reason, with the attribute, the__getit
function had an invalid address or wasn't linked in the final binary (I'm not sure which one was happening). Anyway, without theinline
attribute, it works fine, and I don't really understand the point of having this function inlined anyway since it's not ever called directly; it cannot be inlined, can it?Poison-check bypass in thread_local_key & default allocator
During panic handling, thread-local-storage and allocations are involved. However, the implementations that this PR brings use locking primitives, which have poison checks. These checks messed with panic handling and resulted in various sorts of double-panics and deadlocks. This was mostly a chicken and egg problem involving the initialization of
LOCAL_PANIC_COUNT
, which lives in TLS.To deal with this I tried two approaches, and the one I kept is having special
lock/read/write
methods onMutex
&RwLock
, which bypass poison checks. This works, and I think it's ok? Because thread_local_key and the allocator have no reason to panic except being out of memory, which is unrecoverable anyway.To-do
I'd like to get feedback before spending more time on this, but here are things I haven't done yet:
pipe::read2
Command::output
From<File> for Stdio