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

Separate "no UnsafeCell" property into separate Immutable trait; allow FromZeros, FromBytes, and AsBytes on types with UnsafeCells #251

Closed
10 of 11 tasks
Tracked by #671
joshlf opened this issue Aug 11, 2023 · 6 comments · Fixed by #682
Labels
compatibility-breaking Changes that are (likely to be) breaking

Comments

@joshlf
Copy link
Member

joshlf commented Aug 11, 2023

Summary

Introduce a Immutable trait which indicates that a type contains no UnsafeCells. Allow FromZeros, FromBytes, and AsBytes on types with UnsafeCells and use Immutable as a bound on individual methods/functions.

This enables by-value transmutations such as transmute! on types containing UnsafeCells.

See also #694.

Progress

Details

We require that FromZeros, FromBytes, and AsBytes types don't permit interior mutability. This is a requirement in order for reference casts (e.g., &T to &[u8] or vice-versa) to be valid, but is not a requirement for value casts. As a result, we don't support some transmutations that are sound (namely, value transmutations on types with interior mutability).

If we added a separate Immutable trait to describe this "has no UnsafeCells" property, then we could decouple it from the other traits and allow them to support types with UnsafeCells. Immutable would then be required as a bound on some functions/methods/macros. This would make our API somewhat more complicated, but would unlock a new class of use cases.

Note that Immutable would be equivalent to the stdlib-internal Freeze trait. There is a proposal to make that trait public, but we shouldn't bank on that happening any time soon.

TryFromBytes and unsized UnsafeCell

In principle, there's no reason we couldn't implement FromZeros, FromBytes, and AsBytes for UnsafeCell<T> even when T: ?Sized. However, this would not play nicely with TryFromBytes (currently in progress in #5), which we want to eventually be a super-trait of FromZeros.

In particular, in order to implement TryFromBytes for a type, we have to implement the is_bit_valid method. For UnsafeCell<T> where T: Sized, we can make a copy of the referent UnsafeCell<T>, transmute it to a MaybeUninit<T>, and then construct a Ptr<T> to that value. This workaround isn't possible for unsized UnsafeCells.

Given that the whole problem with UnsafeCells arises when they're handled by-reference, and given that the only way to operate on unsized values in Rust (at least today) is by reference, supporting unsized UnsafeCells is unlikely to be useful to any of our users. Thus, by implementing TryFromBytes (and, by implication, the other traits) only for sized UnsafeCell, we get most of the benefit.

Motivation

This was originally motivated by chipsalliance/caliptra-sw#634. @korran has provided this minimized example to motivate the feature request:

#[repr(C)]
pub struct Prng {
    // It is critical for safety that every bit-pattern of this struct
    // is valid (no padding, no enums, no references), similar to the requirements for
    // zerocopy::FromBytes.
    s0: Cell<u32>,
    s1: Cell<u32>,
    s2: Cell<u32>,
    s3: Cell<u32>,
}

impl Prng {
    /// # Safety
    ///
    /// Caller must verify that the memory locations between `addr` and
    /// `addr + size_of::<Prng>()` are valid and meet the alignment
    /// requirements of Prng, and are not used for anything else.
    pub unsafe fn from_address(addr: u32) -> &'static Self {
        &*(addr as *const Self)
    }

    pub fn next(&self) -> u32 { ... }
}

If FromBytes didn't forbid interior mutability, the API of from_address would be the same, but it would not require the authors of from_address to justify that it's sound to construct a Prng at the given memory location regardless of its previous contents. Instead, the safety argument can just be "Prng: FromBytes, so it's sound regardless of the memory contents".

@joshlf joshlf added the compatibility-breaking Changes that are (likely to be) breaking label Aug 12, 2023
@joshlf joshlf mentioned this issue Aug 20, 2023
@joshlf joshlf changed the title Consider separating FromBytes and RefFromBytes (and same for AsBytes) Consider separating FromBytes and RefFromBytes (and same for AsBytes) or supporting NoCell Oct 12, 2023
joshlf added a commit that referenced this issue Nov 28, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Nov 28, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 2, 2023
joshlf added a commit that referenced this issue Dec 4, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 4, 2023
@joshlf joshlf mentioned this issue Dec 4, 2023
87 tasks
joshlf added a commit that referenced this issue Dec 6, 2023
Makes progress on #251
joshlf added a commit that referenced this issue Dec 6, 2023
Makes progress on #251
github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2023
joshlf added a commit that referenced this issue Dec 6, 2023
joshlf added a commit that referenced this issue Dec 6, 2023
joshlf added a commit that referenced this issue Dec 6, 2023
github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2023
joshlf added a commit that referenced this issue Dec 6, 2023
TODO:
- Require `T: NoCell` and `U: NoCell` in `transmute_ref!` and
  `transmute_mut!`
- Tests
  - Exercise transmuting `UnsafeCell`s by value
- Fix compilation with some `AsBytes` methods
- Time to rename `AsBytes` to `ToBytes`?

Closes ##251
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 27, 2024
Clarify atomic bit validity

The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for `AtomicPtr<T>`, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/`*mut T`. For `AtomicBool`, the new wording clarifies that size, alignment, and bit validity are guaranteed to match `bool`.

Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true - `AtomicXxx` doesn't store an `xxx`, but rather an `UnsafeCell<xxx>`. This distinction is important for some `unsafe` code, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 27, 2024
Clarify atomic bit validity

The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for `AtomicPtr<T>`, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/`*mut T`. For `AtomicBool`, the new wording clarifies that size, alignment, and bit validity are guaranteed to match `bool`.

Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true - `AtomicXxx` doesn't store an `xxx`, but rather an `UnsafeCell<xxx>`. This distinction is important for some `unsafe` code, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 28, 2024
Rollup merge of rust-lang#121943 - joshlf:patch-11, r=scottmcm

Clarify atomic bit validity

The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for `AtomicPtr<T>`, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/`*mut T`. For `AtomicBool`, the new wording clarifies that size, alignment, and bit validity are guaranteed to match `bool`.

Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true - `AtomicXxx` doesn't store an `xxx`, but rather an `UnsafeCell<xxx>`. This distinction is important for some `unsafe` code, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).
joshlf added a commit that referenced this issue May 9, 2024
This commit adds the `pointer::aliasing_safety::AliasingSafe` trait,
which is implemented for pointer conversions which do not violate
aliasing. This can happen either because the aliasing is exclusive or
because neither type contains `UnsafeCell`s.

This paves the way for us to remove `Immutable` bounds from some of our
API, including from some derives.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
joshlf added a commit that referenced this issue May 9, 2024
This commit adds the `pointer::aliasing_safety::AliasingSafe` trait,
which is implemented for pointer conversions which do not violate
aliasing. This can happen either because the aliasing is exclusive or
because neither type contains `UnsafeCell`s.

This paves the way for us to remove `Immutable` bounds from some of our
API, including from some derives.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
joshlf added a commit that referenced this issue May 9, 2024
This commit adds the `pointer::aliasing_safety::AliasingSafe` trait,
which is implemented for pointer conversions which do not violate
aliasing. This can happen either because the aliasing is exclusive or
because neither type contains `UnsafeCell`s.

This paves the way for us to remove `Immutable` bounds from some of our
API, including from some derives.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
joshlf added a commit that referenced this issue May 9, 2024
There are some APIs which still spuriously require `Immutable`, but
which require more work to update. These will be fixed in a separate
commit.

Makes progress on #251
joshlf added a commit that referenced this issue May 9, 2024
There are some APIs which still spuriously require `Immutable`, but
which require more work to update. These will be fixed in a separate
commit.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
github-merge-queue bot pushed a commit that referenced this issue May 9, 2024
This commit adds the `pointer::aliasing_safety::AliasingSafe` trait,
which is implemented for pointer conversions which do not violate
aliasing. This can happen either because the aliasing is exclusive or
because neither type contains `UnsafeCell`s.

This paves the way for us to remove `Immutable` bounds from some of our
API, including from some derives.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
github-merge-queue bot pushed a commit that referenced this issue May 9, 2024
* Add `AliasingSafe` framework

This commit adds the `pointer::aliasing_safety::AliasingSafe` trait,
which is implemented for pointer conversions which do not violate
aliasing. This can happen either because the aliasing is exclusive or
because neither type contains `UnsafeCell`s.

This paves the way for us to remove `Immutable` bounds from some of our
API, including from some derives.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>

* Remove `Immutable` where it's no longer needed

There are some APIs which still spuriously require `Immutable`, but
which require more work to update. These will be fixed in a separate
commit.

Makes progress on #251

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>

---------

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
jswrenn added a commit that referenced this issue May 10, 2024
github-merge-queue bot pushed a commit that referenced this issue May 10, 2024
@joshlf joshlf changed the title Separate "no UnsafeCell" property into separate NoCell trait; allow FromZeros, FromBytes, and AsBytes on types with UnsafeCells Separate "no UnsafeCell" property into separate Immutable trait; allow FromZeros, FromBytes, and AsBytes on types with UnsafeCells May 18, 2024
@joshlf joshlf added the blocking-next-release This issue should be resolved before we release on crates.io label May 30, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
joshlf added a commit that referenced this issue Sep 8, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 8, 2024
@joshlf joshlf removed the blocking-next-release This issue should be resolved before we release on crates.io label Sep 8, 2024
@joshlf
Copy link
Member Author

joshlf commented Oct 6, 2024

@korran This is now available stably in our 0.8 release train.

@joshlf
Copy link
Member Author

joshlf commented Oct 6, 2024

The sole remaining concern is now in its own issue:

@joshlf joshlf closed this as completed Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-breaking Changes that are (likely to be) breaking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants