Skip to content

Add safe volatile functions. #47975

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

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Feb 2, 2018

This adds safe equivalents of read_volatile and write_volatile to std::mem, using references instead of pointers.

The move_opaque and drop_opaque functions are the value_fence and compute_and_drop mentioned in rust-lang/rfcs#1484 (comment). move_opaque is roughly equivalent to the unstable test::black_box.

copy_opaque and replace_opaque are added for completeness, as they are useful equivalents read_volatile and write_volatile that will work in most cases. This means that volatile reads and writes don't have to be annotated with unsafe on each occurrence, and that the programmer simply has to verify that the pointers are correct and cast them to references (probably with 'static lifetime).

If an RFC is required to add these, I'd be willing to write one. However, I think they're reasonable enough to have a PR for now.

@rust-highfive
Copy link
Contributor

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@withoutboats
Copy link
Contributor

r? @alexcrichton

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2018
@cramertj
Copy link
Member

cramertj commented Feb 3, 2018

I'm not sure we should land these without first establishing a more formal memory model. In particular, afaict a types-as-contracts-based interpretation would make copy_opaque useless, since the value behind the reference should not have mutated during the duration of the borrow.

@clarfonthey
Copy link
Contributor Author

@cramerj I see &T as guaranteeing that nothing in the same program has modified the value; that doesn't necessarily guarantee that the value wasn't changed. Plus, types like Cell break this guarantee through an API not unlike copy_opaque.

@cramertj
Copy link
Member

cramertj commented Feb 3, 2018

@clarcharr Cell and other !Freeze types are a special exception to this rule that the compiler understands.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Feb 3, 2018

I share the doubts about how useful a volatile load through a safe reference is. I don't believe it's likely to be miscompiled -- in practice LLVM will probably let volatile take precedence over any other information we give it, and there might be some way to semantically make sense of volatile accesses to UnsafeCell memory. But without an UnsafeCell involved it's inconsistent to create a &T reference (which implies no mutation of the referent for some duration) despite the referent beig mutated from some other source. I can think of a few other use cases and exculpatory circumstances that make this not UB and just a very hairy affair, but I believe the danger of wrongly implying safe references without interior mutability can be combined with "external" mutation outweight any (dubious, IMO) safety benefit for the niche use cases.

I also have some doubts about the robustness of the functions that are intended to partially replace black_box -- I haven't been able to formulate them precisely, but basically a volatile operation has much less power as an optimization barrier than inline asm with side effects, so it seems much trickier to ensure that it's actually doing its job.

@alexcrichton
Copy link
Member

In terms of review I'd personally like to see these functions used to see whether in practice this works or not first as well.

@shepmaster
Copy link
Member

It feels like there's a moderate amount of response to not accept this PR; am I reading the comments correct? Is the team leaning towards closing this PR, perhaps in favor of discussion on IRLO?

@BatmanAoD
Copy link
Member

@cramertj @alexcrichton @rkruppe Should this PR be closed in favor of an RFC? The author has offered to draft one.

@alexcrichton
Copy link
Member

Yeah let's close this for now, I think this may want to have more discussion on an RFC or perhaps on internals before we add to libstd. In the meantime as well this can certainly be added as a crate on crates.io!

@clarfonthey clarfonthey deleted the safe_volatile branch January 29, 2022 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants