8000 Avoid usage of atomic types if not supported by the target arch by bigspider · Pull Request #1705 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid usage of atomic types if not supported by the target arch #1705

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 17, 2023
Merged

Avoid usage of atomic types if not supported by the target arch #1705

merged 1 commit into from
Mar 17, 2023

Conversation

bigspider
Copy link
Contributor

Hi! I'm still a beginner with both Rust and cross-compilation, so please take the rest with a few grains of salt!

I'm setting up a project targeting riscv32i-unknown-none-elf, which seems not to support atomic types. Even with --no-default-features and no-std, I would get this error:

error[E0432]: unresolved import `alloc::sync`
  --> rust-bitcoin/bitcoin/src/blockdata/script/mod.rs:52:12
   |
52 | use alloc::sync::Arc;
   |            ^^^^ could not find `sync` in `alloc`

error[E0432]: unresolved import `alloc::sync`
   --> rust-bitcoin/bitcoin/src/lib.rs:163:114
    |
163 |     pub use alloc::{string::{String, ToString}, vec::Vec, boxed::Box, borrow::{Borrow, Cow, ToOwned}, slice, rc, sync};
    |                                                                                                                  ^^^^ no `sync` in the root
    |
    = help: consider importing one of these items instead:
            bitcoin_hashes::_export::_core::sync
            core::sync

This PR gates the usage of Arc so that it's only enabled with the std feature.

@TheBlueMatt
Copy link
Member

Ugh, that's really annoying. I didn't realize any platforms were missing atomicusize/arc, including most of the embedded ones. Given this is only used in a few relatively minor places I think its fine, but it would be pretty legitimate for another no-std user to want this :/.

Kixunil
Kixunil requested changes Mar 10, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Concept ACK, just needs proper gates and doc.

bitcoin/src/consensus/encode.rs Outdated
@@ -720,6 +720,7 @@ impl<T: Encodable> Encodable for rc::Rc<T> {
}
}

#[cfg(feature = "std")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is imprecise, there are alloc targets that don't need std for atomics.

#[cfg(target_has_atomic = "ptr")] needs to be used instead

Also needs docsrs gate to document it - see our other gates.

bitcoin/src/blockdata/script/mod.rs
@@ -279,6 +280,7 @@ impl<'a> From<&'a Script> for Cow<'a, Script> {
}
}

#[cfg(feature = "std")]
impl<'a> From<&'a Script> for Arc<Script> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apart from different cfg, this needs docsrs too.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 10, 2023

Also thanks for trying out the unreleased version of our crate and catching this! :)

@bigspider
Copy link
Contributor Author

Thanks both for the quick review!

Hopefully the gating and docsrs are correct, now.

@bigspider bigspider requested a review from Kixunil March 10, 2023 10:46
Kixunil
Kixunil previously approved these changes Mar 10, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 73fdef04ae68972e3900b5103bd0a1f0b4a12a4e

@Kixunil Kixunil dismissed their stale review March 10, 2023 10:57

CI failed

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 10, 2023

Crap, this is only available since Rust 1.60.

Would it be OK to drop non-atomic support for Rust <1.60? That seems the best way to do it. Add 1.60 to build.rs and then use this gate:

#cfg(any(not(rust_v_1_60), target_has_atomic = "ptr"))

I would keep the docsrs as-is (without Rust version) because it can't render it. Just add doc comment:

/// Note: This will fail to compile on old Rust for targets that don't support atomics

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 10, 2023

Also needs a similar gate in bitcoin/src/consensus/encode.rs:725

@bigspider
Copy link
Contributor Author

No problem on my side to limit non-atomic support to recent versions − it's for a new project where I don't have any version constraints.

Added the additional version gating in df58661307801410d2e06b4f58d49acb008605d4; now praying for the next round of CI :D

@bigspider bigspider requested a review from Kixunil March 10, 2023 12:40
@apoelstra
Copy link
Member

Looks good to me, though it looks like there's still a missing gate or something according to CI.

Agree with Matt that this is pretty frustrating ... but fortunately for this library we don't directly depend on the functionality.

@bigspider
Copy link
Contributor Author

I'll try to debug the CI next week (or feel free to take over the PR if you know how to fix it).

Apparently there are also some polyfills if lack of atomics is problematic: https://github.com/embassy-rs/atomic-polyfill + https://github.com/rust-embedded/critical-section#usage-in-libraries; although I don't see the riscv32i target there, only riscv32imc − so maybe it still wouldn't solve it for me.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 10, 2023

@bigspider that polyfill doesn't provide Arc anyway. I wouldn't worry about it until someone actually requests it. (But really anyone can create their Arc<Script>

@bigspider
Copy link
Contributor Author

Rebased, hopefully #1708 also fixes the issue I had in the CI.

@bigspider
Copy link
Contributor Author

Failing CIs seem to be related to the no-std feature, and my best guess is that it's because of the incompatible gating between the std vs no-std used in bitcoin/src/lib.rs (where I removed sync in no-std).

Attempting to fix it with f745630, although the gating gets even uglier there.

@bigspider bigspider marked this pull request as draft March 15, 2023 20:57
@bigspider
Copy link
Contributor Author

Alright, moved to draft until I figure out how to debug it locally without playing the CI lottery.
Thanks for the patience.

@apoelstra
Copy link
Member

You're welcome to play the CI lottery @bigspider. (Probably you don't want to, but don't avoid it because you're worried about bugging us :))

@bigspider
Copy link
Contributor Author

The last commit was just botched, hopefully 69136b6 does the trick.

It would be great to be able to define a simple gate as an alias for any(not(rust_v_1_60), target_has_atomic = "ptr"), but it appears that there's no clean way to detect the presence of atomics in build.rs, as people end up hardcoding the architectures for this purpose.

@bigspider bigspider marked this pull request as ready for review March 16, 2023 09:26
@bigspider
Copy link
Contributor Author

Rebased on latest master branch.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 16, 2023

The nightly build failure is unrelated: rust-lang/rust#109209

@bigspider bigspider changed the title Avoid usage of atomic types when the 'std' feature is not enabled Avoid usage of atomic types if not supported by the target arch Mar 16, 2023
Kixunil
Kixunil reviewed Mar 16, 2023
View reviewed changes
< 6D40 /path> Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Looks good, you just need to squash the commits (we require all commits to pass all tests).

@Kixunil Kixunil mentioned this pull request Mar 16, 2023
Merged
8 tasks
@bigspider
Exclude usage of atomic types if not supported for the target
2961c0c 
The gate is only added for Rust >= v1.60, since earlier versions don't support #[cfg(target_has_atomic = ...)]
@bigspider
Copy link
Contributor Author

Done!

apoelstra
apoelstra approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 2961c0c

Kixunil
Kixunil approved these changes Mar 17, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 2961c0c

@apoelstra apoelstra merged commit e658d34 into rust-bitcoin:master Mar 17, 2023
apoelstra added a commit that referenced this pull request Mar 22, 2023
@apoelstra
Merge #1699: Release tracking PR - v0.30.0
d65646c 
ffee8ad Bump version to v0.30.0 (Tobin C. Harding)

Pull request description:

  Add changelog 
8CD9
notes and bump the version number to v0.30.0.

  ## TODO - pre-merge

  - [x] Release `bitcoin_hashes` 0.12: #1694
  - [x] Release secp 0.27: rust-bitcoin/rust-secp256k1#588
    - rust-bitcoin/rust-secp256k1#590
  - [x] Update `secp256k1` dependency to use newly released v0.27: #1714
  - [x] Merge
    - ~#1696
    - #1695
    -  #1111
  - [x] If time permits merge these:
    - #1710
    - #1705
    - #1713
  - [x] Set the release date in changelog header
  - [x] And merge these:
    - #1721
    - #1720
    - #1719
    - #1717

  ## TODO  - post release
  - [ ] Release the blogpost: rust-bitcoin/www.rust-bitcoin.org#2
     - ~Set the date in the blog post to match the date 0.30 is released~

ACKs for top commit:
  sanket1729:
    reACK ffee8ad
  Kixunil:
    ACK ffee8ad
  apoelstra:
    ACK ffee8ad

Tree-SHA512: b0ea113ee1726fd9b263d0e01fe14bd544c007c05a9ac43b6c2d4edbeef3bb3ad456b061ef086626e1e1b27a0cda49cb6bc28aac3ad1691d72ffe00400ed5b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bigspider @TheBlueMatt @Kixunil @apoelstra
0