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

target_os = "custom": selecting/swapping platform-specific parts of the libstd at runtime #115506

Closed
wants to merge 12 commits into from

Conversation

NathanRoyer
Copy link

@NathanRoyer NathanRoyer commented Sep 3, 2023

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:

  • clone this repository
  • create a new directory in library/std/src/sys
  • provide implementations for all the platform specific functions (maybe re-using the impl from unsupported)
  • compile a toolchain
  • linking it using rustup

As an alternative, this PR adds a "custom" value for target_os, which reduces this workflow to:

  • add "os": "custom", to their target.json
  • provide implementations for the platform specific functions that their use case requires (undefined functions simply panic)
  • use the 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:

pub(crate) static IMPL: RwLock<Option<Box<dyn ThatSubsystemTrait>>> = RwLock::new(None);

In library/std/src/sys, functions use a macro call:

pub fn decode_error_kind(errno: i32) -> std_io::ErrorKind {
    custom_os_impl!(os, decode_error_kind, errno)
}

which translates to:

pub fn decode_error_kind(errno: i32) -> std_io::ErrorKind {
    let errmsg = "std::os::custom::os::IMPL has not been initialized at this point";
    let rwlock = &crate::os::custom::os::IMPL;
    let reader = rwlock.read().expect("poisoned lock");
    let some_impl = reader.as_ref().expect(errmsg);
    some_impl.decode_error_kind(errno)
}

Initially, the singletons contains None. Once a singleton is set (to Some(_)), runtime implementations cannot be removed, they can only be swapped: a singleton will never return to a None 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 the global_allocator thing work, does it use static extern 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:

  • x86_64-unknown-custom
  • aarch64-unknown-custom
  • RISC-V?
  • 32-bit arm?
  • wasm? I'm not sure how the allocators work there, could there be complications specific to this target?

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 the inline 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 the inline 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 on Mutex & 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:

  • documenting each trait method
  • thread parking support
  • thread-safe Once support
  • invocation arguments (returns an empty iterator at the moment)
  • pipe::read2
  • Command::output
  • From<File> for Stdio

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2023

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 6, 2023

☔ The latest upstream changes (presumably #115593) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@cuviper
Copy link
Member

cuviper commented Sep 12, 2023

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

@rustbot rustbot assigned thomcc and unassigned cuviper Sep 12, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:8ade135a41bc03ea155e62e844d188df1ea18608)
Download action repository 'actions/upload-artifact@v3' (SHA:a8a3f3ad30e3422c9c7b888a15615d19a852ae32)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=NathanRoyer
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_a6ea5bb7-82f2-4f44-b2d2-2c4d246121a6
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=std-custom-platform
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_a6ea5bb7-82f2-4f44-b2d2-2c4d246121a6
GITHUB_REF=refs/pull/115506/merge
GITHUB_REF_NAME=115506/merge
GITHUB_REF_PROTECTED=false
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=634d71cc02f2ee2899b871fc3b87f8ac252e5242
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_a6ea5bb7-82f2-4f44-b2d2-2c4d246121a6
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_a6ea5bb7-82f2-4f44-b2d2-2c4d246121a6
GITHUB_TRIGGERING_ACTOR=NathanRoyer
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/115506/merge
GITHUB_WORKFLOW_SHA=634d71cc02f2ee2899b871fc3b87f8ac252e5242
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
Removing intermediate container 5f0e499efab9
 ---> bceb83598002
Step 6/10 : COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
 ---> 8e3e93660528
Step 7/10 : RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv
Collecting binaryornot==0.4.4
  Downloading binaryornot-0.4.4-py2.py3-none-any.whl (9.0 kB)
Collecting boolean-py==4.0
  Downloading boolean.py-4.0-py3-none-any.whl (25 kB)
---
  Downloading virtualenv-20.24.5-py3-none-any.whl (3.7 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.7/3.7 MB 21.6 MB/s eta 0:00:00
Collecting filelock<4,>=3.12.2
  Downloading filelock-3.12.4-py3-none-any.whl (11 kB)
Collecting distlib<1,>=0.3.7
  Downloading distlib-0.3.7-py2.py3-none-any.whl (468 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 468.9/468.9 KB 42.5 MB/s eta 0:00:00
Collecting platformdirs<4,>=3.9.1
  Downloading platformdirs-3.10.0-py3-none-any.whl (17 kB)
Installing collected packages: distlib, platformdirs, filelock, virtualenv
Successfully installed distlib-0.3.7 filelock-3.12.4 platformdirs-3.10.0 virtualenv-20.24.5
Removing intermediate container 2e3cfff3c05b
 ---> f603c4282d49
Step 8/10 : COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
 ---> 127888f324be
 ---> 127888f324be
Step 9/10 : COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/
 ---> 5f9c9f9cf16c
Step 10/10 : ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
Removing intermediate container d6c4ad009c95
 ---> bfe5a3f0d4be
Successfully built bfe5a3f0d4be
Successfully tagged rust-ci:latest
Successfully tagged rust-ci:latest
##[endgroup]
Built container sha256:bfe5a3f0d4be6a6eaa4822261c1623e382921593a6ec3a7089ff6f680201d340
Uploading finished image sha256:bfe5a3f0d4be6a6eaa4822261c1623e382921593a6ec3a7089ff6f680201d340 to https://ci-caches.rust-lang.org/docker/8849b25aebb63c7041ab10114da59fac9c6c89ff409673e53f6251b7e63c69daeaca7298d30885d05004ab27b231421908523f297222d07a53450f37e4691d72
IMAGE          CREATED              CREATED BY                                      SIZE      COMMENT
bfe5a3f0d4be   1 second ago         /bin/sh -c #(nop)  ENV SCRIPT=TIDY_PRINT_DIF…   0B        
127888f324be   2 seconds ago        /bin/sh -c #(nop) COPY file:078ea1d11e7b7cda…   367B      
f603c4282d49   3 seconds ago        |1 DEBIAN_FRONTEND=noninteractive /bin/sh -c…   23.9MB    
8e3e93660528   10 seconds ago       /bin/sh -c #(nop) COPY file:ac591dd6bc5afa66…   5.33kB    
bceb83598002   11 seconds ago       |1 DEBIAN_FRONTEND=noninteractive /bin/sh -c…   23.1MB    
---
<missing>      6 weeks ago          /bin/sh -c #(nop)  LABEL org.opencontainers.…   0B        
<missing>      6 weeks ago          /bin/sh -c #(nop)  ARG LAUNCHPAD_BUILD_ARCH     0B        
<missing>      6 weeks ago          /bin/sh -c #(nop)  ARG RELEASE                  0B        

<botocore.awsrequest.AWSRequest object at 0x7f5002daf410>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
https://ci-caches.rust-lang.org/docker/8849b25aebb63c7041ab10114da59fac9c6c89ff409673e53f6251b7e63c69daeaca7298d30885d05004ab27b231421908523f297222d07a53450f37e4691d72
sha256:bfe5a3f0d4be6a6eaa4822261c1623e382921593a6ec3a7089ff6f680201d340
---
DirectMap4k:      178112 kB
DirectMap2M:     7161856 kB
DirectMap1G:    11534336 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
    Finished dev [unoptimized] target(s) in 0.03s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/c5450191f313658d206dc7539311f665a274947f/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-c5450191f313658d206dc7539311f665a274947f-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
    Finished release [optimized] target(s) in 25.23s
##[endgroup]
fmt check
tidy check
##[error]tidy error: /checkout/library/std/src/sync/poison.rs:62: platform-specific cfg: cfg(target_os = "custom")
##[error]tidy error: /checkout/library/std/src/sync/mutex.rs:285: platform-specific cfg: cfg(target_os = "custom")
##[error]tidy error: /checkout/library/std/src/sync/rwlock.rs:223: platform-specific cfg: cfg(target_os = "custom")
##[error]tidy error: /checkout/library/std/src/sync/rwlock.rs:331: platform-specific cfg: cfg(target_os = "custom")
removing old virtual environment
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (23.2.1)
Collecting black==23.3.0 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 7))
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.7/1.7 MB 15.3 MB/s eta 0:00:00
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.7/1.7 MB 15.3 MB/s eta 0:00:00
Collecting click==8.1.3 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 34))
  Downloading click-8.1.3-py3-none-any.whl (96 kB)
Collecting importlib-metadata==6.7.0 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 38))
  Downloading importlib_metadata-6.7.0-py3-none-any.whl (22 kB)
  Downloading importlib_metadata-6.7.0-py3-none-any.whl (22 kB)
Collecting mypy-extensions==1.0.0 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 42))
  Downloading mypy_extensions-1.0.0-py3-none-any.whl (4.7 kB)
Collecting packaging==23.1 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 46))
  Downloading packaging-23.1-py3-none-any.whl (48 kB)
Collecting pathspec==0.11.1 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 50))
  Downloading pathspec-0.11.1-py3-none-any.whl (29 kB)
  Downloading pathspec-0.11.1-py3-none-any.whl (29 kB)
Collecting platformdirs==3.6.0 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 54))
  Downloading platformdirs-3.6.0-py3-none-any.whl (16 kB)
Collecting ruff==0.0.272 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 58))
  Downloading ruff-0.0.272-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (5.9 MB)
Collecting tomli==2.0.1 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 77))
  Downloading tomli-2.0.1-py3-none-any.whl (12 kB)
Collecting typed-ast==1.5.4 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 81))
  Downloading typed_ast-1.5.4-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl (877 kB)
  Downloading typed_ast-1.5.4-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl (877 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 877.7/877.7 kB 71.4 MB/s eta 0:00:00
Collecting typing-extensions==4.6.3 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 107))
  Downloading typing_extensions-4.6.3-py3-none-any.whl (31 kB)
Collecting zipp==3.15.0 (from -r /checkout/src/tools/tidy/config/requirements.txt (line 114))
  Downloading zipp-3.15.0-py3-none-any.whl (6.8 kB)
Installing collected packages: zipp, typing-extensions, typed-ast, tomli, ruff, platformdirs, pathspec, packaging, mypy-extensions, click, importlib-metadata, black
Successfully installed black-23.3.0 click-8.1.3 importlib-metadata-6.7.0 mypy-extensions-1.0.0 packaging-23.1 pathspec-0.11.1 platformdirs-3.6.0 ruff-0.0.272 tomli-2.0.1 typed-ast-1.5.4 typing-extensions-4.6.3 zipp-3.15.0
some tidy checks failed
Build completed unsuccessfully in 0:00:48
  local time: Fri Sep 29 15:26:40 UTC 2023
  network time: Fri, 29 Sep 2023 15:26:40 GMT

@thomcc
Copy link
Member

thomcc commented Sep 29, 2023

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.

@NathanRoyer
Copy link
Author

@thomcc thank you for your reply! I started a zulip topic for this minutes ago haha :)

@bors
Copy link
Contributor

bors commented Oct 14, 2023

☔ The latest upstream changes (presumably #116407) made this pull request unmergeable. Please resolve the merge conflicts.

@thomcc
Copy link
Member

thomcc commented Feb 1, 2024

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

@rustbot rustbot assigned Mark-Simulacrum and unassigned thomcc Feb 1, 2024
@Mark-Simulacrum
Copy link
Member

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.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2024
@NathanRoyer
Copy link
Author

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.

@m-ou-se
Copy link
Member

m-ou-se commented Feb 13, 2024

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.

@m-ou-se m-ou-se removed I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Feb 13, 2024
@NathanRoyer
Copy link
Author

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 no_std professionally though, so it's no longer a priority for me. If anyone wants to, feel free to link the RFC to this PR, one way or another.

@NathanRoyer NathanRoyer marked this pull request as draft February 13, 2024 16:04
@Dylan-DPC
Copy link
Member

This requires a (pre-)RFC so it's better to close this and create a new PR if interested once the RFC is approved

@Dylan-DPC Dylan-DPC closed this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants