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 feature to support platforms without atomic CAS #467

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jan 20, 2021

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS operations and cannot use Arc, Atomic*::compare_exchange, etc. And on those targets, bytes currently fails to build (#461).

EDIT: This PR has been updated to adopt the third idea of #573 (comment).

Alternatively, let the processing of those targets to an external crate such as portable-atomic, avoiding the implementation of our own logic

Perhaps it would be preferable to start with the third and then select the first or second in the future if necessary?

The latest version of this PR uses portable-atomic to support platforms without atomic CAS.
This currently uses the feature name ("extra-platforms") mentioned by carllerche in #461 (comment).

old version

This patch detects those targets using the TARGET environment variables provided by cargo for the build script, and a list of targets that do not support atomic CAS operations.

This way is the same as the way we recently adopted in futures and valuable, and was originally inspired by the way heapless and defmt do, but this doesn't maintain the target list manually. (It's not really fully automated, but it's very easy to update.)

Refs:

Closes #461
Closes #573

@taiki-e

This comment has been minimized.

@taiki-e taiki-e marked this pull request as draft May 8, 2021 13:15
@taiki-e

This comment has been minimized.

@taiki-e taiki-e marked this pull request as ready for review May 23, 2021 08:45
@taiki-e

This comment was marked as outdated.

@taiki-e

This comment was marked as outdated.

@MathiasKoch
Copy link

MathiasKoch commented Mar 27, 2023

Any reason this is still just a draft?

@taiki-e taiki-e force-pushed the cfg_target_has_atomic branch from 0a2e648 to 1e0640f Compare March 30, 2023 18:57
@taiki-e taiki-e force-pushed the cfg_target_has_atomic branch from 1e0640f to 2be8c5e Compare March 30, 2023 19:01
@taiki-e taiki-e marked this pull request as ready for review March 30, 2023 19:31
@taiki-e taiki-e changed the title Support non-atomic targets Add feature to support platforms without atomic CAS Jun 4, 2023
Cargo.toml Outdated Show resolved Hide resolved
taiki-e added 2 commits June 4, 2023 23:06
This provides a better error message if the end user forgets to use the
cfg or feature.
@ia0
Copy link

ia0 commented Nov 14, 2023

@Darksonn you seem to be the most active reviewer on this crate based on the latest commits.

Could you provide some light regarding the status of this PR? What is blocking it from being merged? What comes to mind would be:

  • Unclear whether there is a user need, but the PR has 7 thumbs up so far.
  • Unclear whether it is technically correct/sustainable, but there hasn't be any request for change.
  • Unclear who should review it and has time to do so. If this is true, what would you suggest? Could the community help?

@Darksonn
Copy link
Contributor

Taking new dependencies is always complicated. It raises various questions about to what extent we lock ourselves in for the future, what effect it has on MSRV and so on. I also see that there's another PR that uses a different crate for atomics, and I'll need to understand what happened there. I see that there's a bunch of discussion on the other PR.

Other than that, sometimes what a PR needs is just to ping someone and spur them to action. I'm at a conference right now, but I can try to take a look when I fly back to Europe (if I find the energy to look at my laptop during the flight).

@ia0
Copy link

ia0 commented Nov 14, 2023

Taking new dependencies is always complicated. It raises various questions about to what extent we lock ourselves in for the future, what effect it has on MSRV and so on.

Makes sense, let me try to give you some pointers in that regard, so you don't spend too much time doing archaeology.

I also see that there's another PR that uses a different crate for atomics, and I'll need to understand what happened there. I see that there's a bunch of discussion on the other PR.

Even though #573 is more recent and has more discussions, those discussions are reflected in this PR. The main point of contention as I see it (anyone correct me if I'm wrong) is how portable-atomic was designed back then, essentially preferring backend selection with --cfg rather than --feature (to avoid accidental unsoundess to the price of user convenience). But this point of contention has been resolved since then:

So my understanding would be that portable-atomic is now the defacto crate for portable atomic operations (that's even advertised on https://docs.rs/atomic-polyfill).

I'm at a conference right now, but I can try to take a look when I fly back to Europe (if I find the energy to look at my laptop during the flight).

No worries, I was just checking. Good luck at the conference!

@ia0
Copy link

ia0 commented Jan 22, 2024

Should a new PR be opened? Possibly with a smaller diff such that it's easier to review?

@Darksonn
Copy link
Contributor

Sorry, I never got around to it.

I'm happy to accept this feature, with the following changes:

  • We should document that this feature exists in the readme or lib.rs.
  • We should clarify that the list of platforms supported by extra-platforms is subject to change, and that we may drop platforms without a breaking change.

Also, the merge conflicts will need to be fixed.

@taiki-e

@marius-meissner
Copy link

@Darksonn I'm pleased that things still seem to be moving forward here. :)

@taiki-e: What is your current capacity? We (me or @HusseinAbdelhamid) could offer to support here and solve the last two changes, including merge conflicts.
In any case, we have already been temporarily maintaining a patch (https://github.com/atlas-aero/rt-bytes) for some time. It would therefore help us a lot if this PR is finally completed.
Anyway, thanks for your great work so far!

tjoslin added a commit to tjoslin/bytes that referenced this pull request Dec 22, 2024
squashed commits from upstream tokio-rs#467 and rebased with updated deps
tjoslin added a commit to tjoslin/bytes that referenced this pull request Dec 23, 2024
squashed commits from upstream tokio-rs#467 and rebased with updated deps
@tjoslin
Copy link

tjoslin commented Dec 23, 2024

@Darksonn as an exercise to aid in getting this merged, I squashed these commits and rebased the result onto master with an updated portable-atomic version while resolving the merge conflicts in #751

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.

1.0.1 does not compile for arm-thumb-v6m targets (Cortex-M0)
6 participants