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

Mark all NuttX targets as tier 3 target and support the standard library #136037

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Jan 25, 2025

The support for standard library added by #130595.

@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2025

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2025
@taiki-e
Copy link
Member

taiki-e commented Jan 25, 2025

Could you update std field in metadata in target specs too? (Otherwise, Cargo will treat the target as not supporting std by default.)

metadata: crate::spec::TargetMetadata {
description: Some("ARMv7-A Cortex-A with NuttX".into()),
tier: Some(3),
host_tools: Some(false),
std: Some(false),
},

@no1wudi
Copy link
Contributor Author

no1wudi commented Jan 25, 2025

Could you update std field in metadata in target specs too? (Otherwise, Cargo will treat the target as not supporting std by default.)

metadata: crate::spec::TargetMetadata {
description: Some("ARMv7-A Cortex-A with NuttX".into()),
tier: Some(3),
host_tools: Some(false),
std: Some(false),
},

OK, I'll update the metadata later

@jieyouxu jieyouxu assigned jieyouxu and unassigned GuillaumeGomez Jan 25, 2025
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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 Jan 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2025

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

@no1wudi no1wudi changed the title Mark all NuttX targets as supporting the standard library Mark all NuttX targets as tier 3 target and support the standard library Jan 25, 2025
@no1wudi
Copy link
Contributor Author

no1wudi commented Jan 25, 2025

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2025
@taiki-e
Copy link
Member

taiki-e commented Jan 25, 2025

std requires atomic CAS (std uses Arc and Arc requires atomic CAS), but it is disabled in the thumbv6m-nuttx-eabi target spec.

// The ARMv6-M architecture doesn't support unaligned loads/stores so we disable them
// with +strict-align.
// Also force-enable 32-bit atomics, which allows the use of atomic load/store only.
// The resulting atomics are ABI incompatible with atomics backed by libatomic.
features: "+strict-align,+atomics-32".into(),
// There are no atomic CAS instructions available in the instruction set of the ARMv6-M
// architecture
atomic_cas: false,

Therefore, std should not actually be supported at thumbv6m-nuttx-eabi at this time.

However, IIUC this platform provide atomic builtins, so this problem should be fixed by just removing +atomics-32 and atomic_cas: false. (as already done in riscv32imc-unknown-nuttx-elf that has no atomic CAS instructions)

(In practice, it would also be necessary to audit whether atomic builtins are implemented correctly (like #115577 (comment) and #115577 (comment)), but that is a separate issue since riscv32imc nuttx already uses it.)

@jieyouxu
Copy link
Member

@rustbot author (see review above)

@rustbot rustbot 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 Jan 25, 2025
@no1wudi
Copy link
Contributor Author

no1wudi commented Jan 25, 2025

Therefore, std should not actually be supported at thumbv6m-nuttx-eabi at this time.

However, IIUC this platform provide atomic builtins, so this problem should be fixed by just removing +atomics-32 and atomic_cas: false. (as already done in riscv32imc-unknown-nuttx-elf that has no atomic CAS instructions)

Thanks for point that, after verification, there is indeed an issue with compiling the standard library on this platform.

Following your suggestion, adopting a method similar to that used on the riscv32imc platform has not been able to pass the compilation:

error[E0599]: no method named `compare_exchange` found for struct `AtomicUsize` in the current scope
  --> /home/huang/Work/rust/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/src/rust/library/std/src/sys/sync/thread_parking/pthread.rs:97:23
   |
97 |         if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Relaxed).is_ok() {
   |                       ^^^^^^^^^^^^^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `compare_exchange` found for struct `AtomicUsize` in the current scope
   --> /home/huang/Work/rust/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/src/rust/library/std/src/sys/sync/thread_parking/pthread.rs:102:26
    |
102 |         match self.state.compare_exchange(EMPTY, PARKED, Relaxed, Relaxed) {
    |                          ^^^^^^^^^^^^^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `swap` found for struct `AtomicUsize` in the current scope
   --> /home/huang/Work/rust/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/src/rust/library/std/src/sys/sync/thread_parking/pthread.rs:106:38
    |
106 |                 let old = self.state.swap(EMPTY, Acquire);
    |                                      ^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `swap` found for struct `AtomicUsize` in the current scope
   --> /home/huang/Work/rust/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/src/rust/library/std/src/sys/sync/thread_parking/pthread.rs:124:26

At present, I am planning to keep the current status of the thumbv6m platform until it is able to function properly.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2025
@taiki-e
Copy link
Member

taiki-e commented Jan 25, 2025

Therefore, std should not actually be supported at thumbv6m-nuttx-eabi at this time.
However, IIUC this platform provide atomic builtins, so this problem should be fixed by just removing +atomics-32 and atomic_cas: false. (as already done in riscv32imc-unknown-nuttx-elf that has no atomic CAS instructions)

Thanks for point that, after verification, there is indeed an issue with compiling the standard library on this platform.

Following your suggestion, adopting a method similar to that used on the riscv32imc platform has not been able to pass the compilation:

Oh, perhaps max_atomic_width: Some(32) needs to be added. (I remember that the behavior was something odd when max_atomic_width was None.)

@no1wudi
Copy link
Contributor Author

no1wudi commented Jan 25, 2025

Oh, perhaps max_atomic_width: Some(32) needs to be added. (I remember that the behavior was something odd when max_atomic_width was None.)

Thank you!!! It works with:

        options: TargetOptions {
            families: cvs!["unix"],
            os: "nuttx".into(),
            abi: "eabi".into(),
            llvm_floatabi: Some(FloatAbi::Soft),
            // The ARMv6-M architecture doesn't support unaligned loads/stores so we disable them
            // with +strict-align.
            // The ARMv6-M doesn't support hardware atomic operations, use atomic builtins instead.
            features: "+strict-align".into(),
            max_atomic_width: Some(32),
            ..base::thumb::opts()
        },

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! And thanks @taiki-e for the review

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 25, 2025

📌 Commit e44e345 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang#133631 (Support QNX 7.1 with `io-sock`+libstd and QNX 8.0 (`no_std` only))
 - rust-lang#134358 (compiler: Set `target_abi = "ilp32e"` on all riscv32e targets)
 - rust-lang#135764 (Fix tests on LLVM 20)
 - rust-lang#135812 (Fix GDB `OsString` provider on Windows )
 - rust-lang#135842 (TRPL: more backward-compatible Edition changes)
 - rust-lang#135946 (Remove extra whitespace from rustdoc breadcrumbs for copypasting)
 - rust-lang#135953 (ci.py: check the return code in `run-local`)
 - rust-lang#136019 (Add an `unchecked_div` alias to the `Div<NonZero<_>>` impls)

Failed merges:

 - rust-lang#136037 (Mark all NuttX targets as tier 3 target and support the standard library)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jan 26, 2025

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants