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

Add armv7a-vex-v5 tier three target #131530

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

Conversation

max-niederman
Copy link
Contributor

This adds a new tier three target with std support called armv7a-vex-v5, targeting the microcontroller used in the VEX V5 student robotics competition. This is a joint effort by the maintainers of the vexide project, which currently provides a no_std library targeting this device using a custom JSON target, and is intended to improve the UX for users of vexide or vex-rt, and anyone else writing Rust programs for VEX V5.

Tier 3 Target Policy Compliance

A tier 3 target must have a designated developer or developers (the "target maintainers") on record to be CCed when issues arise regarding the target. (The mechanism to track and CC such developers may evolve over time.)

As listed in the target docs, the following members of the vexide project are the designated target maintainers:

Targets must use naming consistent with any existing targets; for instance, a target for the same CPU or OS as an existing Rust target should use the same name for that CPU or OS. Targets should normally use the same names and naming conventions as used elsewhere in the broader ecosystem beyond Rust (such as in other toolchains), unless they have a very good reason to diverge. Changing the name of a target can be highly disruptive, especially once the target reaches a higher tier, so getting the name right is important even for a tier 3 target.

armv7a-vex-v5 follows the cpu-vendor-model convention used by most tier three targets. E.g., armv76k-nintendo-3ds or armv7k-apple-watchos.

Tier 3 targets may have unusual requirements to build or use, but must not create legal issues or impose onerous legal terms for the Rust project or for Rust developers or users.

Although the VEX V5 Brain is proprietary, this target does not link to any proprietary binaries or libraries, and is based solely on publicly available information about the VEX SDK.

Neither this policy nor any decisions made regarding targets shall create any binding agreement or estoppel by any party. If any member of an approving Rust team serves as one of the maintainers of a target, or has any legal or employment requirement (explicit or implicit) that might affect their decisions regarding a target, they must recuse themselves from any approval decisions regarding the target's tier status, though they may otherwise participate in discussions.

I understand and assent.

Tier 3 targets should attempt to implement as much of the standard libraries as possible and appropriate (core for most targets, alloc for targets that can support dynamic memory allocation, std for targets with an operating system or equivalent layer of system-provided functionality), but may leave some code unimplemented (either unavailable or stubbed out as appropriate), whether because the target makes it impossible to implement or challenging to implement. The authors of pull requests are not obligated to avoid calling any portions of the standard library on the basis of a tier 3 target not implementing those portions.

The parts of std which are not implemented are appropriately stubbed, and there are no parts of the standard library which authors are obligated not to call.

Tier 3 targets must not impose burden on the authors of pull requests, or other developers in the community, to maintain the target. In particular, do not post comments (automated or manual) on a PR that derail or suggest a block on the PR based on a tier 3 target. Do not send automated messages or notifications (via any medium, including via @) to a PR author or others involved with a PR regarding a tier 3 target, unless they have opted into such messages.

I understand and assent.

Patches adding or updating tier 3 targets must not break any existing tier 2 or tier 1 target, and must not knowingly break another tier 3 target without approval of either the compiler team or the maintainers of the other tier 3 target.

I understand and assent.

Tier 3 targets must be able to produce assembly using at least one of rustc's supported backends from any host target. (Having support in a fork of the backend is not sufficient, it must be upstream.)

armv7a-vex-v5 has nearly identical codegen to armv7a-none-eabihf, so this is not an issue.

If a tier 3 target stops meeting these requirements, or the target maintainers no longer have interest or time, or the target shows no signs of activity and has not built for some time, or removing the target would improve the quality of the Rust codebase, we may post a PR to remove it; any such PR will be CCed to the target maintainers (and potentially other people who have previously worked on the target), to check potential interest in improving the situation.

I understand.

@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Member

Urgau commented Oct 11, 2024

The parts of std which are not implemented are appropriately stubbed

Can you gives us a summary of which part of the std are implemented, which aren't, at what level and if there are plans or not to extend it.

@workingjubilee
Copy link
Member

they already implement some filesystem operations, it seems, so that's nice.

@max-niederman
Copy link
Contributor Author

max-niederman commented Oct 11, 2024

@Urgau

Here's the more detailed summary I just added to the target docs:

armv7a-vex-v5 implements as much of the standard library as is possible using only public VEX SDK functions.
This includes:

  • std::time, not including SystemTime as the SDK does not provide absolute time information.
  • std::io, including std::io::stdin() and std::io::stdout, but not std::stderr(), as stderr does not exist on this platform.
  • std::fs, with the exception of directory reading and file deletion, due to public SDK limitations.
  • modules which do not need to interact with the OS beyond allocation,
    such as std::collections, std::hash, std::future, std::sync, etc.

Notable modules which are not implemented include:

  • std::process
  • std::thread
  • std::net

As for future plans, we currently believe we have implemented everything that we can implement using only publicly available information about the VEX SDK, but if that changes we intend to add support for whatever we can.
For example, if VEX publicly releases information about an SDK call used to delete files, we would add support for that here.

@workingjubilee workingjubilee added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Oct 13, 2024
@Urgau
Copy link
Member

Urgau commented Oct 14, 2024

Thanks for detailed API view.

Notable modules which are not implemented include:

  • std::process
  • std::thread
  • std::net

Is this because of a fundamental "limitation" of the SDK? Can we expect those modules to ever be implemented?

You already have a custom runtime/HAL crate vex_rt, what's the reason for wanting to support alloc and std in addition to core?

I ask those questions because we have some bad experience with targets stubbing out part of std and then having the ecosystem needing to paper over (with ugly #[cfg]s) the part that we were stubbed out. Which leaves a bad experience on users.

@ion098
Copy link

ion098 commented Oct 14, 2024

@Urgau

Notable modules which are not implemented include:

  • std::process
  • std::thread
  • std::net

Is this because of a fundamental "limitation" of the SDK? Can we expect those modules to ever be implemented?

Note: I'm not a target maintainer, but I'll just give my two cents here
The Vex V5 Brain doesn't have the necessary hardware for Internet connectivity (let alone SDK support for it) so std::net will most likely never be supported. VEXos also doesn't support anything similar to processes so std::process will also most likely never be implemented. The SDK also doesn't have a publicly exposed threading API (VEXOs does have a private, undocumented cooperative scheduler), so there are no OS threads. It is possible to port something like FreeRTOS to the Vex V5 Brain as PROS has done, however my understanding is that the rust std should only rely on facilities provided by the underlying OS, so std::thread should not be implemented for this target.

@max-niederman
Copy link
Contributor Author

max-niederman commented Oct 14, 2024

re: @Urgau

Is this because of a fundamental "limitation" of the SDK? Can we expect those modules to ever be implemented?

Yes, as @ion098 says, these modules are not implemented because the VEX SDK does not provide the requisite syscalls to implement them. We have (as far as we know) implemented everything that is possible to implement using the public SDK, without adding a ridiculous amount of runtime (e.g. implementing std::thread is technically possible but only by implementing a full preemptive scheduler from scratch, which is not something I think anyone wants in libstd). If new SDK functions are made available, we intend to add support for those, but we aren't aware of any unimplemented APIs that will become possible to implement in the near future.

You already have a custom runtime/HAL crate vex_rt, what's the reason for wanting to support alloc and std in addition to core?

I ask those questions because we have some bad experience with targets stubbing out part of std and then having the ecosystem needing to paper over (with ugly #[cfg]s) the part that we were stubbed out. Which leaves a bad experience on users.

My understanding was that limiting any changes to the PAL (as we've done) obviates the need for any #[cfg]s elsewhere. We certainly haven't had to add any such #[cfg]s outside of library/std/src/sys to get things to compile and run.

As for why we want std and alloc support, the reason is the same as the reason that targets like wasm32-unknown-unknown (which actually stubs strictly more APIs than armv7a-vex-v5) have std support: it improves the user experience. Vexide's users are largely new to Rust, and having to immediately hit them with a no_std environment and a custom JSON target makes things much less approachable for new users and also less convenient for experienced users. Additionally, it significantly improves compatability with libraries that use std; for example, we can use libraries that use std::io::{Read, Write}, std::fs, etc.

@Urgau
Copy link
Member

Urgau commented Oct 15, 2024

Thanks you both for the addition context.

We certainly haven't had to add any such #[cfg]s outside of library/std/src/sys to get things to compile and run.

I was talking about ecosystem crates that assumes that if std is available that is fully available, and when it's not the case they have to sprinkle #[cfg] to avoid panics.

Anyway, I'm not a t-libs member/reviewer so I will leave this part to them.

Btw, PR target that adds the definition + std takes a long time to get merged, if you split them up I can review the compiler part.

@rust-log-analyzer

This comment has been minimized.

@thomcc
Copy link
Member

thomcc commented Oct 23, 2024

I'm not sure if we want to add more targets with broken functionality like wasm32-unknown-unknown. Maybe the existing stuff implemented is enough though. I'm going to nominate it for discussion in a libs meeting. For context, #131530 (comment) has the list of things not implemented.

@thomcc thomcc 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. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2024
@Amanieu
Copy link
Member

Amanieu commented Oct 23, 2024

We discussed this in the libs meeting today. While this is indeed missing a lot of what would normally be present in a std target, it does at least have the basics down like support for Instant (which is one of the biggest issues with wasm32-unknown-unknown). We're happy for this to be accepted at tier 3 but would likely reject it for higher tiers.

@Amanieu Amanieu removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Oct 23, 2024
Tropix126 and others added 19 commits January 5, 2025 16:46
This isn't necessarily fixing anything on the V5 in particular (exit requests are handled by CPU0, which then interrupt CPU1 after), but it's certainly more correct and may be required for other single-core VEXos targets if support is added in the future.
0.23.0 most notably removes uses of `#[no_mangle]` from symbols in `vex-sdk` to avoid conflicts with other versions of the crate.

Symbols were originally `#[no_mangle]` for the purposes of compiling to a C static library, but this never ended up being useful and would be actively harmful to include in `libstd`.
The Cortex-A9 only has 16 double-precision registers (`d0`-`d15`), so this is the correct VFP instructionset to use.
This release of `vex-sdk` notably removes any (previously misguided) usage of `#[no_mangle]` which would be unsuitable to have in `libstd`.
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling pkg-config v0.3.31
   Compiling anstyle v1.0.10
   Compiling heck v0.5.0
   Compiling clap_lex v0.7.2
   Compiling cc v1.2.0 (https://github.com/rust-lang/cc-rs?rev=6fe995c675dc1178f570752fa8f6de82913a91dc#6fe995c6)
   Compiling clap_builder v4.5.20
   Compiling semver v1.0.23
   Compiling log v0.4.22
   Compiling generic-array v0.14.7
---
This PR updated 'src/doc/reference', verifying if status is 'test-pass'...

We detected that this PR updated 'reference', but its tests failed.

If you do intend to update 'reference', please check the error messages above and


If you do NOT intend to update 'reference', please ensure you did not accidentally
change the submodule at 'src/doc/reference'. You may ask your reviewer for the
Build completed unsuccessfully in 0:00:00
  local time: Mon Jan  6 00:48:23 UTC 2025
  network time: Mon, 06 Jan 2025 00:48:23 GMT
##[error]Process completed with exit code 1.

@bors
Copy link
Contributor

bors commented Jan 11, 2025

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

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

The libs changes look fine to me. Sorry for the delay, it was a big PR.

@thomcc
Copy link
Member

thomcc commented Jan 14, 2025

There are compiler changes here, so

r? compiler

@rustbot rustbot assigned compiler-errors and unassigned thomcc and davidtwco Jan 14, 2025
@max-niederman
Copy link
Contributor Author

The libs changes look fine to me. Sorry for the delay, it was a big PR.

Don't worry about it, we really should have split it up from the start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.