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

atomics: Add support for targets without atomics #413

Merged
merged 5 commits into from
Sep 22, 2020
Merged

atomics: Add support for targets without atomics #413

merged 5 commits into from
Sep 22, 2020

Conversation

alistair23
Copy link
Contributor

Some targets (such as RV32IMC) don't have atomic support. This PR allows the log crate to build for those targets.

As the platform doesn't have atomics we can't implement an Atomic version of AtomicUsize, so this version is not atomic. Any platform without atomics is unlikely to have multiple cores, so this shouldn't be a problem.

This is somewhat based on implementations in: https://github.com/japaric/heapless

Based on the implementation in the headless
crate (https://github.com/japaric/heapless/blob/master/build.rs#L26) let's
not enable CAS for three more targets.

Signed-off-by: Alistair Francis <[email protected]>
Some platforms don't provide a AtomicUsize. Instead of just failing to
build with this error:

291 | use std::sync::atomic::{AtomicUsize, Ordering};
    |                         ^^^^^^^^^^^ no `AtomicUsize` in `sync::atomic`

let's instead add a fake AtomicUsize.

As the platform doesn't have atomics we can't implement an Atomic
version of AtomicUsize, so this version is not atomic. Any platform
without atomics is unlikely to have multiple cores, so this shouldn't be
a problem.

This is somewhat based on:
rust-embedded/heapless@940d2e9

Signed-off-by: Alistair Francis <[email protected]>
@alistair23
Copy link
Contributor Author

Ping

@sfackler
Copy link
Member

This seems like a plausible approach, though I think we'll need to have CI target at least one of these architectures so the not(has_atomics) code doesn't rot.

@alistair23
Copy link
Contributor Author

Done, I have added a CI test to build RISC-V without atomics.

@sfackler
Copy link
Member

Thanks!

cc @KodrAus does this seem okay to you as well? It's a bit scary, but it seems reasonable that a platform with no atomics can't really have threads either.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I would like the rationale from the PR description to be documented on the unsafe impl.

// Any platform without atomics is unlikely to have multiple cores, so
// writing via Cell will not be a race condition.

build.rs Outdated Show resolved Hide resolved
@KodrAus
Copy link
Contributor

KodrAus commented Sep 22, 2020

I think I'd feel just a bit safer if we encapsulated the global logger code into a module, so that we limit the reachability of our "fake" AtomicUsize as much as possible, but I don't think that's a blocker.

Signed-off-by: Alistair Francis <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
@alistair23
Copy link
Contributor Author

I think I have addressed all comments, let me know if there is anything else.

@sfackler sfackler merged commit 9a1902d into rust-lang:master Sep 22, 2020
@sfackler
Copy link
Member

Thanks!

@alistair23 alistair23 deleted the alistair/atomics branch September 23, 2020 00:04
@alistair23
Copy link
Contributor Author

Thanks for merging. Do you know when the next release will be?

EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023
* Bump dep cargo_toml to v0.12.0
* FIx compilation error
* Fix test parse-meta

Signed-off-by: Jiahao XU <[email protected]>
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.

4 participants