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 unsafe-assume-single-core and related Cargo features. #94

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

Dirbaio
Copy link
Contributor

@Dirbaio Dirbaio commented Apr 4, 2023

Previous discussion in tokio-rs/bytes#573 and rust-embedded/heapless#328 .

This adds the following Cargo features, equivalent to the previous --cfgs:

  • unsafe-assume-single-core
  • s-mode
  • disable-fiq

To maintain backwards compatibility, the features forward to the cfgs in build.rs, and the code still uses the cfgs.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Apr 24, 2023

ping @taiki-e is this something you'd consider merging? If yes, I'll put in the work to get it finished.

@taiki-e
Copy link
Owner

taiki-e commented Apr 24, 2023

Thanks for the PR and sorry for the late review!

First, the design principle for Cargo features is that features should be additive and not negative or mutually exclusive.
Whether or not to provide a feature that deviates from this principle depends on how sympathetic I feel to your use case.
Your use case that I sympathized with is about single-core cfg, and no-outline-atomics cfg is unrelated.


Next, given the behavior of the cargo v1 resolver, I mentioned in rust-embedded/heapless#328 (comment) my concern that feature flags that have different safety requirements for each platform but work on multiple platforms may be easy to misuse, and I mentioned an alternative. Since single-core cfg already works on multiple platforms, this is basically an issue of how easy it is to misuse the behavior of the cargo features.

For example, the following example compiles for both thumbv6m and riscv32i, despite the fact that critical-section is not explicitly enabled for thumbv6m.

[package]
name = "repro"
version = "0.1.0"
edition = "2018" # changing this to 2021 or adding resolver = "v2" field fixes the problem.

[target.thumbv6m-none-eabi.dependencies]
portable-atomic = { version = "1" } # no critical-section feature here
[target.riscv32i-unknown-none-elf.dependencies]
portable-atomic = { version = "1" , features = ["critical-section"] }
#![no_std]

pub fn f() {
    // compare_exchange should not be available for thumbv6m with no critical-section or single-core cfg.
    let _ = portable_atomic::AtomicUsize::compare_exchange;
}

The same thing would occur in the unsafe-assume-single-core feature, and in that case, can we say that "the user has guaranteed the safety requirements"? The safety requirements for the unsafe-assume-single-core feature in thumbv6m and riscv32i are different.

@taiki-e
Copy link
Owner

taiki-e commented Apr 24, 2023

The approach I mentioned in rust-embedded/heapless#328 (comment) is basically to create a feature for each platform. This alleviates the above problem, but I also feel that an interface different than cfg might be confusing.

I'm on board with providing single-core cfg as Cargo features (eventually), but I'm still torn on what a less misused and reasonable interface for that would be.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Apr 24, 2023

While I agree that as a general principle Cargo features should be additive, Cargo features are the de-facto standard way to configure the behavior of dependency crates. If there's some behvaior that can't be made additive, it's more pragmatic to have it as a feature even if it's not additive, than to use nonstandard --cfgs.

The rust embedded ecosystem is FULL of crates with non-additive features. It would be thorougly unusable if all these crates decided to make them --cfgs. Embedded users are already used to dealing with non-additive features, so having one more in portable-atomic doesn't hurt developer experience. Having to pass --cfgs does.

Rust defaults to resolver=2 since edition 2021, it is effectively dead. I wouldn't complicate the API just to mitigate possible misuses with it.

@taiki-e
Copy link
Owner

taiki-e commented Apr 24, 2023

While I agree that as a general principle Cargo features should be additive, Cargo features are the de-facto standard way to configure the behavior of dependency crates. If there's some behvaior that can't be made additive, it's more pragmatic to have it as a feature even if it's not additive, than to use nonstandard --cfgs.

The rust embedded ecosystem is FULL of crates with non-additive features. It would be thorougly unusable if all these crates decided to make them --cfgs. Embedded users are already used to dealing with non-additive features, so having one more in portable-atomic doesn't hurt developer experience. Having to pass --cfgs does.

To be clear: I'm not opposed to making single-core cfg (which is intended to be used in the embedded ecosystem) Cargo features.

I'm okay with following de-facto standards in the embedded ecosystem in areas that are related to the embedded ecosystem, but I will follow Cargo's recommendations in areas (in this case no-outline-atomic cfg) that are unrelated to the embedded ecosystem.

Rust defaults to resolver=2 since edition 2021, it is effectively dead.

Unfortunately, the v1 resolver is not dead when the 2021 edition becomes the default. It is not uncommon for v1 resolvers to be used in projects that use the 2021 edition. (rust-lang/cargo#10112, rust-lang/cargo#9996, rust-lang/cargo#11811, etc.)

I wouldn't complicate the API just to mitigate possible misuses with it.

I would have agreed with this if the worst case of result to misuse was not UB.

That said, I agree that having a complex API to handle this does not seem to help much if such features are common in the embedded ecosystem.

@Dirbaio Dirbaio changed the title Add unsafe-assume-single-core and no-outline-atomics Cargo features. Add unsafe-assume-single-core and related Cargo features. May 19, 2023
@Dirbaio
Copy link
Contributor Author

Dirbaio commented May 19, 2023

@taiki-e I've changed the PR to only create features for the embedded-related cfgs (unsafe-assume-single-core, s-mode, disable-fiq).

Unfortunately, the v1 resolver is not dead when the 2021 edition becomes the default. It is not uncommon for v1 resolvers to be used in projects that use the 2021 edition.

The v1 resolver is effectively dead in embedded Rust land. There's a lot of crates that break. For example cortex-m with the inline-asm feature will try to build ARM asm for x86 with resolver=1 which results in "unknown register r0" errors. Nobody has any interest in supporting it. I really wouldn't worry about it.

(Yes it's very unfortunate that they didn't make workspaces default to resolver=2... hopefully in Rust 2024?)

@Dirbaio
Copy link
Contributor Author

Dirbaio commented May 19, 2023

There's a few tests that break due to --all-features, for targets where assume-single-core is not supported. I'm not sure what the best fix is. Manually list all features?

taiki-e added a commit that referenced this pull request Jun 4, 2023
@taiki-e
Copy link
Owner

taiki-e commented Jun 4, 2023

Sorry for the late reply. I have completed the CI adjustment. I believe replacing __do_not_enable feature added in 8fcbe7c with unsafe-assume-single-core,s-mode,disable-fiq will fix CI issues.

I've changed the PR to only create features for the embedded-related cfgs (unsafe-assume-single-core, s-mode, disable-fiq).

Thanks.

The v1 resolver is effectively dead in embedded Rust land. There's a lot of crates that break.

My impression is that actively developed embedded projects are moving to v2 resolver, but I also see embedded projects still using v1 resolver. (it can also be found in templates/examples that are intended to be copied by beginners, e.g., rust-embedded/cortex-m-quickstart, rust-embedded/book, riscv-rust/riscv-rust-quickstart, rust-embedded/msp430-quickstart, etc.)

That said, I'm no longer so pessimistic about the situation on v1 resolver, as cargo recently added a warning about the footgun about workspace default resolver.

@Dirbaio Dirbaio force-pushed the no-cfgs branch 2 times, most recently from 5cca0b8 to 3db9af7 Compare June 21, 2023 11:42
@Dirbaio
Copy link
Contributor Author

Dirbaio commented Jun 26, 2023

ping @taiki-e it's all green now except test (aarch64-unknown-linux-gnu, glibc 2.17) which seems is due to unrelated flakiness.

Does the PR look good? If there's anything I need to fix please let me know!

Thanks! :)

@taiki-e
Copy link
Owner

taiki-e commented Jun 30, 2023

LGTM. I'll fix CI issue and then merge this PR.

@taiki-e taiki-e merged commit 3622664 into taiki-e:main Jul 3, 2023
@taiki-e
Copy link
Owner

taiki-e commented Jul 11, 2023

Published in 1.4.0. Thanks again @Dirbaio.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Jul 11, 2023

nooo, thank you! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants