Add builtin implementation for std targets. by Dirbaio · Pull Request #22 · rust-embedded/critical-section

Dirbaio · 2022-08-14T22:01:54Z

This adds an std Cargo feature that enables a critical-section implementation for std.

IMO this is the only impl that we should have "builtin". Rust's std is essentially "stable forever" so the impl won't need breaking changes, and it should work soundly on all targets where std is supported.

Note this uses Rust 1.63's "const Mutex::new()", so this makes our MSRV effectively that for std targets. I think it should be acceptable.

eldruin

Seems fine to me but I am no expert on CS. Maybe @adamgreig could review?
I noticed now that there is no MSRV notice or CI job at all here.
Being such a foundational crate I think we should have one, even if it is 1.63.
It's definitely not a breaking change if there was none before, right? 😉

Dirbaio · 2022-08-15T11:57:56Z

Added 1.63 MSRV notice to the README.

It's definitely not a breaking change if there was none before, right?

😂 It's not breaking anyway because 1.63 is only actually required if you enable the std feature, which didn't exist before.

reitermarkus

Some small suggestions, but in general looks good to me.

Dirbaio · 2022-08-16T11:37:37Z

Thanks @reitermarkus for the review, I've incorporated the feedback.

eldruin

Could you also add this feature to the test CI workflow?

adamgreig · 2022-08-17T01:51:24Z

I don't like the asymmetry in acquire being "lock, then store true to cell" while release is "unlock, then store false to cell", but I appreciate that since the cell is thread-local and the thread can't pre-empt itself it probably doesn't matter that another thread might have taken the mutex between us unlocking it and us storing false back to the cell. Indeed it means we hold the mutex lock for very slightly less time which is probably beneficial. But, it's more "obvious" to keep the lock until we've cleared the cell. What do you think?
Otherwise the std impl all seems fine to me 👍
We should document the new feature in the README so that it also appears in the library documentation. Probably in the "providing an implementation" section. It could briefly explain how it's implemented too.
I think it's fine for the std impl to require 1.63 and obviously therefore bump the MSRV, but I wonder if it would be kind to have a lower MSRV without the std feature, or at least mention it, as 1.63 is so new and we're basically rolling this out across the whole embedded ecosystem. Since we'll probably add a CI test case for the feature, that one can build with the higher MSRV, and the other case could use the lower one. Dunno.

Dirbaio · 2022-08-17T14:05:02Z

(@eldruin) Could you also add this feature to the test CI workflow?

I didn't because it runs the doctests showing how to set an impl, which then fails duplicate symbol errors because there's also the std impl. IMO it should be fine since there are no tests other than the doctests anyway.

(@adamgreig) But, it's more "obvious" to keep the lock until we've cleared the cell. What do you think?

Actually, we can also set the flag before acquiring the mutex. This way it's symmetric! :)
I've added some comments highlighting the fact that it's fine due to being thread local.

(@adamgreig) We should document the new feature in the README so that it also appears in the library documentation. Probably in the "providing an implementation" section. It could briefly explain how it's implemented too.

Done

(@adamgreig) I think it's fine for the std impl to require 1.63 and obviously therefore bump the MSRV, but I wonder if it would be kind to have a lower MSRV without the std feature

SGTM. Documented MSRV as 1.54 without std, 1.63 with. Added CI checks for both. (1.54 needed for #[doc(include_str!())] support)

Dirbaio · 2022-08-17T14:09:32Z

For some reason 1.54 doesn't like #[transparent] on RestoreState, so I've removed it.


error[E0690]: transparent struct needs exactly one non-zero-sized field, but has 0
   --> src/lib.rs:123:1
    |
123 | pub struct RestoreState(RawRestoreState);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ needs exactly one non-zero-sized field, but has 0

For some reason 1.54 doesn't like it. ``` error[E0690]: transparent struct needs exactly one non-zero-sized field, but has 0 --> src/lib.rs:123:1 | 123 | pub struct RestoreState(RawRestoreState); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ needs exactly one non-zero-sized field, but has 0 ```

notgull · 2022-08-17T14:18:07Z

I think it's because RawRestoreState can be a (), and ZSTs can't be the only field in a transparent struct.

Dirbaio · 2022-08-17T14:20:37Z

Yeah, but that was fine in 1.63 🤷 Either way, I think not having it is fine, it's very unlikely it'll result in worse code. Ssee #20 (comment)

eldruin · 2022-08-17T15:01:39Z

(@eldruin) Could you also add this feature to the test CI workflow?

I didn't because it runs the doctests showing how to set an impl, which then fails duplicate symbol errors because there's also the std impl. IMO it should be fine since there are no tests other than the doctests anyway.

Hmm, you can gate the doctest code with something like:

# #[cfg(not(feature = "std"))]
# {
// ... actual doctest code
# }

Dirbaio · 2022-08-17T15:10:48Z

Done (it's slightly abominable, but works...)

adamgreig · 2022-08-17T15:19:11Z

Thanks for the updates, LGTM.

eldruin

Alright, thank you everybody!
bors r+

bors · 2022-08-17T15:23:50Z

👎 Rejected by code reviews

eldruin · 2022-08-17T15:25:11Z

Ach, @reitermarkus could you review this again?

Dirbaio · 2022-08-17T16:14:21Z

bors r+

bors · 2022-08-17T16:14:24Z

👎 Rejected by code reviews

Dirbaio · 2022-08-17T16:14:53Z

ugh, why does a "neutral" review with comments count as "rejected"?

Dirbaio · 2022-08-17T16:15:42Z

bors r+

bors · 2022-08-17T16:15:44Z

👎 Rejected by code reviews

901: Update to critical-section 1.0, atomic-polyfill 1.0 r=Dirbaio a=Dirbaio TODO - [x] Wait for cortex-m 0.7.6 release rust-embedded/cortex-m#449 - [x] ~~Wait for defmt-rtt 0.4 release knurling-rs/defmt#689 we're still going to use defmt 0.3 for now, which will use the `critical-section` 0.2 default impl, which works. - [x] Wait for critical-secton `std` impl rust-embedded/critical-section#22 Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

Dirbaio requested a review from a team as a code owner August 14, 2022 22:01

Dirbaio force-pushed the std-impl branch from 55800f7 to 2bf0ad2 Compare August 14, 2022 22:05

Dirbaio mentioned this pull request Aug 14, 2022

Update to critical-section 1.0, atomic-polyfill 1.0 embassy-rs/embassy#901

Merged

3 tasks

eldruin reviewed Aug 15, 2022

View reviewed changes

reitermarkus suggested changes Aug 16, 2022

View reviewed changes

Comment thread src/std.rs Outdated

Comment thread src/std.rs Outdated

Comment thread src/std.rs Outdated

Comment thread src/std.rs Outdated

Comment thread src/std.rs Outdated

Comment thread Cargo.toml Outdated

Comment thread .github/workflows/clippy.yml Outdated

Dirbaio force-pushed the std-impl branch from 73f617e to d07b1f7 Compare August 16, 2022 11:36

eldruin reviewed Aug 16, 2022

View reviewed changes

Dirbaio added 4 commits August 17, 2022 15:32

Add builtin implementation for std targets.

b5bf3ce

Clippy check std.

1a50964

Add Rust 1.63 MSRV.

0037ac4

Expand docs: std impl, typical crates supplying no-std impls.

1120fb3

Dirbaio force-pushed the std-impl branch from d07b1f7 to ee0f906 Compare August 17, 2022 14:04

Set MSRV to 1.54 without std, 1.63 with.

fe12610

Dirbaio force-pushed the std-impl branch from ee0f906 to fe12610 Compare August 17, 2022 14:06

Make test work with --feature std.

5686ef0

eldruin approved these changes Aug 17, 2022

View reviewed changes

Dirbaio requested a review from reitermarkus August 17, 2022 16:15

Dirbaio merged commit b6ae622 into main Aug 17, 2022

bors Bot deleted the std-impl branch August 17, 2022 16:18

Dirbaio mentioned this pull request Aug 17, 2022

Add MSRV upgrade policy #24

Merged

Conversation

Dirbaio commented Aug 14, 2022

Uh oh!

eldruin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dirbaio commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reitermarkus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dirbaio commented Aug 16, 2022

Uh oh!

eldruin left a comment

Choose a reason for hiding this comment

Uh oh!

adamgreig commented Aug 17, 2022

Uh oh!

Dirbaio commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

notgull commented Aug 17, 2022

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

eldruin commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

adamgreig commented Aug 17, 2022

Uh oh!

eldruin left a comment

Choose a reason for hiding this comment

Uh oh!

bors Bot commented Aug 17, 2022

Uh oh!

eldruin commented Aug 17, 2022

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

bors Bot commented Aug 17, 2022

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

Dirbaio commented Aug 17, 2022

Uh oh!

bors Bot commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eldruin left a comment •

edited

Loading

Dirbaio commented Aug 15, 2022 •

edited

Loading

Dirbaio commented Aug 17, 2022 •

edited

Loading

eldruin commented Aug 17, 2022 •

edited

Loading