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.

Sign up for GitHub

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

Jump to bottom

std::thread support for the Nintendo 3DS #98514

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

std::thread support for the Nintendo 3DS #98514

wants to merge 5 commits into from
+206 −14

Conversation

AzureMarker
Copy link
Member

@AzureMarker AzureMarker commented Jun 26, 2022 •
edited by Dylan-DPC
Loading

Rustc supports compiling for the Nintendo 3DS using the armv6k-nintendo-3ds target (Tier 3). Just recently we merged std support in #95897. A notable exclusion was std::thread support, which was moved into this PR as a follow-up since it required more complicated changes.

See #95897 for background on how rustc is able to support std for the 3DS.

A notable change in this PR is the addition of a "native options" parameter to the internal implementation of the thread builder on each platform. This is due to the 3DS requiring the "processor ID" where the thread should be spawned (certain cores are either cooperative or preemptive) and the thread priority up-front. There is an unstable thread builder extension trait added which allows users to set these values.

There are 2 non-portable pthread APIs which we depend on. Where possible, we tried to reuse existing standard APIs, but there are some features of the 3DS thread system which required us to add non-portable functions.

  1. pthread_attr_setprocessorid_np: Used to set the new thread's processor ID.
  2. pthread_getprocessorid_np: Used to retrieve the current thread's processor ID.

These are both implemented by https://github.com/Meziu/pthread-3ds, which is already noted as in the platform support doc as a requirement for the target's std support (note: it provides the backend for other pthread-based types such as Mutex). These functions have already been added to libc and are in the currently used version (rust-lang/libc#2715).

Edit: libs team ACP has been opened for the changes to the thread builder: rust-lang/libs-team#71

Blocked on #101222 and ACP rust-lang/libs-team#71 rust-lang/libs-team#195

@AzureMarker
Support std threads again by using pthread
9280028 
Priority and affinity are passed through special pthread attr functions
which will be added to our libc fork.

The current thread's priority also needs to be fetched since it is the
default priority value. This is done using another new pthread function,
though this value/mechanism isn't exposed as part of std's API.
@AzureMarker
Add "native options" concept to thread builder and make std::os::hori…
fe1cdcf 
…zon::thread

Also renamed "affinity" to "processor_id".
@AzureMarker
Add std::os::horizon::thread::current_processor
1fb2f72
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 26, 2022
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2022
AzureMarker
AzureMarker commented Jun 26, 2022
View reviewed changes
library/std/src/os/horizon/thread.rs
//!
//! [`std::thread`]: crate::thread

#![unstable(feature = "horizon_thread_ext", issue = "none")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at creating a tracking issue for this, but held off for now due to this note in the template:

If the new feature is small, it may be fine to skip the RFC process. In that
case, you can use use issue = "none" in your initial implementation PR. The
reviewer will ask you to open a tracking issue if they agree your feature can be
added without an RFC.

So once someone comments that this is an acceptable feature to skip an RFC for, I can create the tracking issue.

@AzureMarker
Copy link
Member Author

AzureMarker commented Jun 26, 2022 •
edited
Loading

@rustbot label +T-libs-api

Since this adds an unstable library feature.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 26, 2022
@AzureMarker
Copy link
Member Author

r? @nagisa

You reviewed the std PR and were very helpful, so hopefully you can help review this PR as well :).

@rust-highfive rust-highfive assigned nagisa and unassigned kennytm Jun 26, 2022
@AzureMarker AzureMarker mentioned this pull request Jun 26, 2022
Closed
@nagisa
Copy link
Member

nagisa commented Jul 3, 2022

r? @rust-lang/libs

This seems reasonable enough to me, but I’m not in a position to vet changes to a stable and widely used type like the thread builder.

@Mark-Simulacrum
Copy link
Member

Per the latest guidance from T-libs-api, I believe you'll want to file an MCP on their repo (see the opening comment from rust-highfive):

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@AzureMarker AzureMarker mentioned this pull request Jul 16, 2022
Closed
@AzureMarker
Copy link
Member Author

AzureMarker commented Jul 16, 2022 •
edited
Loading

Per the latest guidance from T-libs-api, I believe you'll want to file an MCP on their repo

I've opened the ACP: rust-lang/libs-team#71

@joshtriplett
Copy link
Member

Registering as a quick comment that I'd prefer to see this not go in in its current form, and instead be changed in one of two ways. Will comment further on the ACP.

@ian-h-chamberlain ian-h-chamberlain mentioned this pull request Aug 31, 2022
Closed
@bors
Copy link
Contributor

bors commented Oct 25, 2022

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

@Dylan-DPC Dylan-DPC added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2023
@ian-h-chamberlain ian-h-chamberlain mentioned this pull request Mar 17, 2023
Open
5 tasks
@ian-h-chamberlain ian-h-chamberlain mentioned this pull request Mar 24, 2023
Open
@Dylan-DPC Dylan-DPC added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@AzureMarker @rust-highfive @nagisa @Mark-Simulacrum @rustbot @joshtriplett @bors @kennytm @Dylan-DPC