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

epoch: Move away from pointers as usize #490

Closed
wants to merge 2 commits into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 10, 2020

Pointers stored as usize tend to cause miri to lose pointer provenance
tracking, which means we can't take advantage of its checking! See also
the discussion at
rust-lang/miri#940 (comment).

This does not yet compile since AtomicPtr does not have fetch_*
methods. They were added and then removed from the standard library back
in the day (rust-lang/rust#10154), but I think
the reason they were removed has now been remedied, so they will
hopefully come back again!

cc @RalfJung

Pointers stored as `usize` tend to cause miri to lose pointer provenance
tracking, which means we can't take advantage of its checking! See also
the discussion at
rust-lang/miri#940 (comment).

This does not yet compile since `AtomicPtr` does not have `fetch_*`
methods. They were added and then removed from the standard library back
in the day (rust-lang/rust#10154), but I think
the reason they were removed has now been remedied, so they will
hopefully come back again!
@jonhoo jonhoo changed the title Move away from pointers as usize epoch: Move away from pointers as usize Apr 10, 2020
fn data_with_tag<T>(data: *mut T, tag: usize) -> *mut T {
// transmute preserves pointer provenance
let data: usize = core::mem::transmute(data);
core::mem::transmute((data & !low_bits::<T>()) | (tag & low_bits::<T>()))
Copy link
Contributor

@RalfJung RalfJung Apr 10, 2020

Choose a reason for hiding this comment

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

Even if the transmute preserved provenance, doing integer operations on the pointer will still kill provenance unless we do really awful things (things that we used to do but that I very happily removed when integer-ptr-casts got properly supported).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, interesting. Is there any way to support provenance through tagged pointers then?

Copy link
Contributor

Choose a reason for hiding this comment

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

See ptr_int_arithmetic removed in this commit for the awful things we used to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened rust-lang/miri#1318 to track the Miri-side issue (and to avoid having this discussion in 4 threads in parallel^^).

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 10, 2020

Note in particular the (backwards-compatible) change to Pointer, which is sad, but pretty much necessary.

@taiki-e taiki-e added the S-blocked Status: Blocked on something else label Nov 25, 2020
@taiki-e
Copy link
Member

taiki-e commented Jan 2, 2021

@jonhoo What is the current status of this PR? It seems both rust-lang/rust#71004 and rust-lang/miri#1318 were closed.

@taiki-e taiki-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-blocked Status: Blocked on something else labels Jan 2, 2021
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 5, 2021

It's.. complicated. I suspect that any time we do usize arithmetic on pointers (i.e., not through the standard pointer operations), we're going to run into issues with miri. @RalfJung is probably a better person to ask to make the judgement call on this.

@RalfJung
Copy link
Contributor

RalfJung commented Jan 6, 2021

Miri (in its default settings) does support ptrs being cast to ints and back. Yes, provenance is lost, but that is a "feature" of int-to-ptr casts and Miri supports this. So you should be able to benefit from Miri's checking regardless.

However, the memory leak checker requires provenance to be intact to "recognize" pointers that it has to follow to memory that is still alive. You can either disable the memory leak checker (-Zmiri-ignore-leaks) or call the extern function miri_static_root to mark some memory as "not considered leaked even if it still exists when the program is done". If there are more elaborate APIs that you need to get proper memory leak checking, we can also try to offer those.

Furthermore, -Zmiri-track-raw-pointers puts Miri into a mode where more aliasing issues are found, but int-to-ptr casts are not supported any more. But there's a lot of code out there that is not compatible with this code, so that seems fine. However, if you are curious, there are also some hacks one can use to "finesse" pointer provenance such that this should still work, at least in many cases.

So... I guess what I am saying is, I am not entirely sure which problem this PR is solving. ;) IIRC it originated from discussions around the leak checker? It probably makes more sense to first get MIRIFLAGS=-Zmiri-ignore-leaks cargo miri test to work on this repo, and put that under CI, before trying to appease the leak checker.

@taiki-e
Copy link
Member

taiki-e commented Jan 6, 2021

@RalfJung Thanks for the detailed explanation!

It probably makes more sense to first get MIRIFLAGS=-Zmiri-ignore-leaks cargo miri test to work on this repo, and put that under CI, before trying to appease the leak checker.

Agree. ...and I have a draft PR for this: #578

@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jan 6, 2021
@jonhoo jonhoo closed this Jan 8, 2021
bors bot added a commit that referenced this pull request Jul 23, 2022
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 <[email protected]>
bors bot added a commit that referenced this pull request Jul 23, 2022
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants