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

util: add portable_atomic_unstable_coerce_unsized cfg option #195

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Nov 6, 2024

(with some minor CI & documentation updates)

Adapted from Rust std library - this adds automatic casting to Arc as discussed in issue #143 behind portable_atomic_unstable_coerce_unsized cfg option ... of course this would only work with Rust nightly until custom unsized coercions is stabilized ref: rust-lang/rust#18598

MOTIVATION: enable more straight-forward support for rustls on no-std targets without built-in atomics ref: rustls/rustls#2068 (using an internal alias as an easy way to select between alloc::sync::Arc vs portable_atomic_util::Arc as needed) as @brodycj has proposed now in rustls/rustls#2200

P.S. IIRC I think I have seen a lot of the smaller embedded CPU targets more likely to be built with Rust nightly anyways, thinking it would be much more straightforward to add this coercion support than one of the other alternative workaround ideas discussed above.

ALTERNATIVE SOLUTIONS CONSIDERED for rustls/rustls#2068:

  • use internal aliasing of Arc to alloc::rc::Rc, if needed to support a target with no built-in atomics - I have already tried this solution in ALT DRAFT RFC: add option to use alloc::rc::Rc for no-std targets w/o built-in atomic ops - INITIAL HACK rustls/rustls#2088 ... this seems to suffer from multiple forms of API mangling with ugly API trait macros to conditionally include Send & Sync traits depending on using Arc vs Rc
  • add & support use of utility functions or macros to support multiple casting workarounds - I have started working on this kind of workaround and have found the required API changes to be a bit messy & potentially hard-to-document

I think this proposal should be much, much cleaner & more straightforward than either of the alternative solutions above.

CI TESTING - UPDATED:

  • added one more step to test with portable_atomic_unstable_coerce_unsized cfg option
  • skip testing with --cfg portable_atomic_test_outline_atomics_detect_false on arm-linux-androideabi, with note that this was removed due to an apparent issue I encountered between Rust nightly & QEmu in CI testing (note that there seems to be testing of --cfg portable_atomic_test_outline_atomics_detect_false with armv5te among some other targets so I suspect & hope that this should be sufficient to test --cfg portable_atomic_test_outline_atomics_detect_false)

In case we DO need to preserve testing with --cfg portable_atomic_test_outline_atomics_detect_false on arm-linux-androideabi, any pointers that could help me resolve the CI testing issue I encountered would be extremely helpful.


TODO ITEMS - UPDATED:

  • TODO items are tracked by XXX TODO & XXX @brodycj (TODO) comments.
  • I would like to raise separate PRs for some TODO improvements as already discussed in the XXX / TODO comments.

I have raised PR #199 to update a test label & think I have been able to resolve all other TODO items required for this proposal.

@brodycj brodycj marked this pull request as draft November 6, 2024 02:11
@taiki-e
Copy link
Owner

taiki-e commented Nov 6, 2024

Is there any plan regarding the stabilization of this feature? I do not want to rely unconditionally on a feature that is forever unstable or expected to change many times in the future.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 6, 2024

Is there any plan regarding the stabilization of this feature?

rust-lang/rust#18598 - unfortunately it seems to be held up on what looks like a somewhat theoretical discussion:

Things to consider prior to stabilization

Would you be open to adding this behind a new, optional feature?

@taiki-e
Copy link
Owner

taiki-e commented Nov 7, 2024

Would you be open to adding this behind a new, optional feature?

Cargo feature would be a bad idea for unstable ones. cfg like tokio_unstable is okay.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 8, 2024

cfg like tokio_unstable is okay.

Makes sense to me. The one thing is that this would make rustls non-atomic-ptr support I proposed in rustls/rustls#2200 a little harder to build, test, and document. Any ideas or pointers you may have would be very much appreciated. (Working with Rust embedded is still pretty new for me.)

/cc @bjoernQ

.github/workflows/ci.yml Outdated Show resolved Hide resolved
portable-atomic-util/src/lib.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
portable-atomic-util/build.rs Outdated Show resolved Hide resolved
@brodycj brodycj changed the title [DRAFT RFC] portable-atomic-util: custom unsized coercions support on Rust nightly only for now - WITH TODO ITEMS DO NOT MERGE [DRAFT RFC] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - Rust nightly only for now - WITH TODO ITEMS DO NOT MERGE Nov 11, 2024
@brodycj brodycj changed the title [DRAFT RFC] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - Rust nightly only for now - WITH TODO ITEMS DO NOT MERGE [DRAFT RFC] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE Nov 14, 2024
@brodycj brodycj force-pushed the custom-unsized-coercions branch from e6d3406 to ff4aa85 Compare November 14, 2024 18:56
@brodycj brodycj force-pushed the custom-unsized-coercions branch from e4c642a to c983bbc Compare November 19, 2024 02:19
@brodycj brodycj changed the title [DRAFT RFC] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE [DRAFT] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE YET Nov 19, 2024
@brodycj brodycj changed the title [DRAFT] portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE YET DRAFT: portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE YET Nov 19, 2024
@brodycj

This comment was marked as outdated.

@brodycj brodycj changed the title DRAFT: portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature - WITH TODO ITEMS DO NOT MERGE YET portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature Nov 19, 2024
@brodycj brodycj force-pushed the custom-unsized-coercions branch from b5b5ab9 to bf536c7 Compare November 20, 2024 00:05
@brodycj brodycj changed the title portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg feature portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg option Nov 20, 2024
@brodycj brodycj force-pushed the custom-unsized-coercions branch from bf536c7 to daefbb8 Compare November 20, 2024 00:29
@brodycj brodycj marked this pull request as ready for review November 20, 2024 01:21
@brodycj brodycj marked this pull request as draft November 20, 2024 01:28
@brodycj brodycj changed the title portable-atomic-util: portable_atomic_unstable_coerce_unsized cfg option util: portable_atomic_unstable_coerce_unsized cfg option Nov 20, 2024
@brodycj brodycj force-pushed the custom-unsized-coercions branch from daefbb8 to da6b1bd Compare November 20, 2024 01:33
@brodycj

This comment was marked as resolved.

@brodycj brodycj marked this pull request as draft November 20, 2024 20:30
@brodycj brodycj changed the title util: portable_atomic_unstable_coerce_unsized cfg option [DRAFT] util: add portable_atomic_unstable_coerce_unsized cfg option - DO NOT MERGE Nov 20, 2024
Copy link
Contributor Author

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

very minor rewording of some test comments

portable-atomic-util/tests/arc.rs Outdated Show resolved Hide resolved
portable-atomic-util/tests/arc.rs Outdated Show resolved Hide resolved
portable-atomic-util/tests/arc.rs Outdated Show resolved Hide resolved
@brodycj brodycj changed the title [DRAFT] util: add portable_atomic_unstable_coerce_unsized cfg option - DO NOT MERGE util: add portable_atomic_unstable_coerce_unsized cfg option Nov 21, 2024
with some minor CI & documentation updates

Authored-by: @brodycj - C. Jonathan Brody <[email protected]>
Co-authored-by: @taiki-e - Taiki Endo <[email protected]>
@brodycj brodycj force-pushed the custom-unsized-coercions branch from 1fee5a6 to d321a70 Compare November 21, 2024 23:16
@brodycj brodycj marked this pull request as ready for review November 21, 2024 23:17
@brodycj brodycj requested a review from taiki-e November 21, 2024 23:17
@brodycj

This comment was marked as outdated.

…l check that CI run is green & rusttls/rusttls#2200 is not affected by this
@brodycj brodycj marked this pull request as draft November 22, 2024 14:34
Cargo.toml Show resolved Hide resolved
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM!

@taiki-e taiki-e marked this pull request as ready for review November 22, 2024 16:20
@brodycj
Copy link
Contributor Author

brodycj commented Nov 22, 2024

Thanks please let me know if you need anything else for this

@taiki-e taiki-e merged commit d8e6bbf into taiki-e:main Nov 22, 2024
105 checks passed
@taiki-e
Copy link
Owner

taiki-e commented Nov 23, 2024

Published in portable-atomic-util 0.2.4. Thanks @brodycj!

@brodycj
Copy link
Contributor Author

brodycj commented Nov 25, 2024

@taiki-e thanks to you for your part in making & publishing this update - so much appreciated for this now unblocking my work in rustls/rustls#2200 🎉


P.S. You may see the one-time sponsorship I sent, in appreciation

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