Fix stacked borrows violations in epoch & skiplist, fix CI failures#871
Fix stacked borrows violations in epoch & skiplist, fix CI failures#871
Conversation
fcafef5 to
8e29940
Compare
f25a4cd to
a81e099
Compare
a81e099 to
e7c8fbf
Compare
e7c8fbf to
86c1680
Compare
796: epoch: Remove ptr-to-int casts r=taiki-e a=taiki-e Use [this hack](rust-lang/miri#1866 (comment)) to fix compatibility issues with Miri (see #490 (comment) for details). Due to the #545, still not compatible with stacked borrows. This will be fixed by the subsequent PR (#871). Note: this is a breaking change because changes API of Pointable and Pointer traits Fixes #579 881: Remove deprecated items r=taiki-e a=taiki-e This removes the following deprecated items: - crossbeam-epoch: - `CompareAndSetError` - `CompareAndSetOrdering` - `Atomic::compare_and_set` - `Atomic::compare_and_set_weak` - crossbeam-utils: - `AtomicCell::compare_and_swap` Co-authored-by: Taiki Endo <te316e89@gmail.com>
796: epoch: Remove ptr-to-int casts r=taiki-e a=taiki-e Use [this hack](rust-lang/miri#1866 (comment)) to fix compatibility issues with Miri (see #490 (comment) for details). Due to the #545, still not compatible with stacked borrows. This will be fixed by the subsequent PR (#871). Note: this is a breaking change because changes API of Pointable and Pointer traits Fixes #579 Co-authored-by: Taiki Endo <te316e89@gmail.com>
86c1680 to
525ae4f
Compare
a8485b4 to
f93970c
Compare
f93970c to
9d1ebd2
Compare
48e0dfd to
be5bcc4
Compare
crossbeam-epoch/src/atomic.rs
Outdated
There was a problem hiding this comment.
This is no longer a dereference, but I'm not sure what is a good API name and description.
be5bcc4 to
87eecad
Compare
87eecad to
f0f7323
Compare
f0f7323 to
6bd2fda
Compare
|
(Given that Miri appears to be trying to move to TB in the long term, I don't see the need to rush to merge this.) |
That's wrong. There is no plan to move to TB entirely, and in any case TB doesn't help here because the implementation in Miri (currently, at least) doesn't support the int-to-ptr that crossbeam-epoch currently relies on. |
That comment was my understanding based on reading several discussions and blog posts at 2023-06 and before, and I may have misunderstood something. (My understanding at the time was that they wanted to adopt a somewhat stricter version of TB. EDIT: Ralf's recent comment seem to suggest the future adoption of a stricter version of TB.) Well, to be honest, that comment should no longer be relevant to this PR. The remaining issues with the epoch and issues in skiplist that made me hesitate to progress this PR at the time have been fixed in the last two commits in this PR (thanks @Imberflur for the skiplist fix), so there is no longer any need to consider TB-only support.
That issue has already been resolved in the master branch: #796 |
6ddfad6 to
85e491b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes stacked borrows violations in crossbeam's epoch-based garbage collection and skiplist implementations, originally reported in issues #545 and #878. The changes involve significant API modifications to avoid creating intermediate references that violate Miri's stacked borrows model, allowing the code to pass under default Miri checks without requiring Tree Borrows. Additionally, the PR includes CI configuration updates and bumps the MSRV to 1.74 to support newly used standard library features.
Changes:
- Refactored
Pointabletrait to return raw pointers instead of references to avoid stacked borrows violations - Modified
IsElementtrait to use raw pointers throughout the API - Introduced
TowerRefandNodeRefwrapper types in skiplist to preserve provenance for dynamically-sized trailing arrays - Updated
Local::release_handleandLocal::finalizeto accept raw pointers - Replaced manual ceiling division with
div_ceilmethod (requires Rust 1.73+) - Updated MSRV from 1.61/1.60 to 1.74 across affected crates
- Removed Tree Borrows requirement from CI Miri tests
- Added new targets to
no_atomic.rslist
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crossbeam-epoch/src/atomic.rs | Breaking API change: Pointable trait methods now return raw pointers instead of references; added helper methods to Shared for safe pointer access |
| crossbeam-epoch/src/internal.rs | Changed Local::release_handle and finalize to use raw pointers to avoid stacked borrows violations |
| crossbeam-epoch/src/guard.rs | Updated call to Local::release_handle to work with new signature |
| crossbeam-epoch/src/sync/list.rs | Breaking API change: IsElement trait now uses raw pointers; updated all implementations and call sites |
| crossbeam-skiplist/src/base.rs | Major refactoring: introduced TowerRef and NodeRef types to preserve provenance; removed Index trait usage; updated all node traversal logic |
| crossbeam-deque/src/deque.rs | Replaced manual ceiling division (n+1)/2 with div_ceil(2) method |
| no_atomic.rs | Added armv4t-none-eabi, armv5te-none-eabi, thumbv4t-none-eabi, thumbv5te-none-eabi targets |
| ci/miri.sh | Removed Tree Borrows flag since Stacked Borrows violations are now fixed |
| .github/workflows/ci.yml | Updated MSRV to 1.74, upgraded Ubuntu ARM runner to 24.04, removed vm.mmap_rnd_bits workaround |
| Multiple Cargo.toml and README.md files | Updated rust-version and MSRV documentation from 1.60-1.61 to 1.74 |
| ci/check-features.sh | Updated comment and cargo command to reflect new MSRV requirement for atomic feature |
| Cargo.toml (root) | Allowed manual_map clippy lint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f14285f to
940a296
Compare
Co-authored-by: Imbris <imbrisf@gmail.com>
```
error: use `core::ptr::eq` when comparing raw pointers
--> crossbeam-epoch/src/sync/list.rs:343:17
|
343 | assert!(maybe_e3.unwrap().unwrap() as *const Entry == e3.as_raw());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `core::ptr::eq(maybe_e3.unwrap().unwrap(), e3.as_raw())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
= note: `-D clippy::ptr-eq` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::ptr_eq)]`
error: use `core::ptr::eq` when comparing raw pointers
--> crossbeam-epoch/src/sync/list.rs:346:17
|
346 | assert!(maybe_e2.unwrap().unwrap() as *const Entry == e2.as_raw());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `core::ptr::eq(maybe_e2.unwrap().unwrap(), e2.as_raw())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
error: use `core::ptr::eq` when comparing raw pointers
--> crossbeam-epoch/src/sync/list.rs:349:17
|
349 | assert!(maybe_e1.unwrap().unwrap() as *const Entry == e1.as_raw());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `core::ptr::eq(maybe_e1.unwrap().unwrap(), e1.as_raw())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
error: use `core::ptr::eq` when comparing raw pointers
--> crossbeam-epoch/src/sync/list.rs:382:17
|
382 | assert!(maybe_e3.unwrap().unwrap() as *const Entry == e3.as_raw());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `core::ptr::eq(maybe_e3.unwrap().unwrap(), e3.as_raw())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
error: use `core::ptr::eq` when comparing raw pointers
--> crossbeam-epoch/src/sync/list.rs:385:17
|
385 | assert!(maybe_e1.unwrap().unwrap() as *const Entry == e1.as_raw());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `core::ptr::eq(maybe_e1.unwrap().unwrap(), e1.as_raw())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
```
```
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> crossbeam-epoch/src/atomic.rs:1552:1
|
1552 | / impl<'g, T: ?Sized + Pointable> PartialOrd<Shared<'g, T>> for Shared<'g, T> {
1553 | | fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
| | __________________________________________________________________-
1554 | || Some(self.data.cmp(&other.data))
1555 | || }
| ||_____- help: change this to: `{ Some(self.cmp(other)) }`
1556 | | }
| |__^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.93.0/index.html#non_canonical_partial_ord_impl
= note: `-D clippy::non-canonical-partial-ord-impl` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::non_canonical_partial_ord_impl)]`
```
```
error: manually reimplementing `div_ceil`
--> crossbeam-deque/src/deque.rs:774:35
|
774 | let batch_size = cmp::min((len as usize + 1) / 2, limit);
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.div_ceil()`: `(len as usize).div_ceil(2)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.93.0/index.html#manual_div_ceil
= note: `-D clippy::manual-div-ceil` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_div_ceil)]`
error: manually reimplementing `div_ceil`
--> crossbeam-deque/src/deque.rs:1626:27
|
1626 | advance = ((len + 1) / 2).min(limit);
| ^^^^^^^^^^^^^^^ help: consider using `.div_ceil()`: `len.div_ceil(2)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.93.0/index.html#manual_div_ceil
error: manually reimplementing `div_ceil`
--> crossbeam-deque/src/deque.rs:1823:27
|
1823 | advance = ((len + 1) / 2).min(limit);
| ^^^^^^^^^^^^^^^ help: consider using `.div_ceil()`: `len.div_ceil(2)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.93.0/index.html#manual_div_ceil
```
This reverts commit 75d24e0.
This reverts commit 8131228.
940a296 to
0407e1b
Compare
…e inline asm support
9587ed2 to
bb14b07
Compare
Fixes #545
Fixes #878
Also rollups CI failures fixes from (the third and subsequent commits):
Closes #1218
Closes #1196
See #545 (comment) for the reported stacked borrows violations.
Note: this is a breaking change because changes API of Pointable trait