Skip to content

Conversation

@SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented Sep 19, 2024

Objective

  • Remove an int2ptr cast in bevy_ptr::dangling_with_align
  • This is flagged by MIRI unless -Zmiri-permissive-provenance is used (like in CI)
  • Remove -Zmiri-permissive-provenance in CI

Solution

@SkiFire13 SkiFire13 changed the title Removing int2ptr cast in bevy_ptr::dangling_with_align Removing int2ptr cast in bevy_ptr::dangling_with_align and remove -Zmiri-permissive-provenance in CI Sep 19, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior A-Pointers Relating to Bevy pointer abstractions D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2024
@alice-i-cecile alice-i-cecile removed the A-ECS Entities, components, systems, and events label Sep 19, 2024
@alice-i-cecile alice-i-cecile requested a review from hymm September 19, 2024 18:24
@SkiFire13 SkiFire13 changed the title Removing int2ptr cast in bevy_ptr::dangling_with_align and remove -Zmiri-permissive-provenance in CI Remove int2ptr cast in bevy_ptr::dangling_with_align and remove -Zmiri-permissive-provenance in CI Sep 19, 2024
@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2024
Co-authored-by: François Mockers <[email protected]>
@iiYese iiYese removed their request for review September 19, 2024 21:35
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 19, 2024
Merged via the queue into bevyengine:main with commit 40b05b2 Sep 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 9, 2025
# Objective

- `bevy_ptr::dangling_with_align()` is only used once, in `bevy_ecs`'s
`BlobArray::with_capacity()`, and it isn't generally useful outside of
the engine's internals. We can remove the function and inline its
implementation into its call site.
- Additionally, `bevy_ptr::dangling_with_align()` has a TODO comment
that was leftover from
#15311 (comment),
where it was suggested that `dangling_with_align()` should use
`without_provenance()`.
- `with_addr()`, mentioned in the TODO comment, could also be used, but
it's a more roundabout solution. The reason it was mentioned was because
the original author thought it would be stabilized before
`without_provenance()`.

## Solution

- Remove `dangling_with_align()`.
- Replace its usage with `NonNull::without_provenance()` (since it is
now stable and doesn't require `unsafe` or pointer math)

## Testing

- I ran Miri with strict provenance checking enabled to ensure the
behavior is maintained.
  - `MIRIFLAGS="-Zmiri-strict-provenance" cargo +nightly miri test`

## But what is this provenance thingy?

[The official docs provide a more in-depth
explanation](https://doc.rust-lang.org/stable/std/ptr/index.html#provenance),
but basically a pointer is more than just a number referring to a memory
address. Pointers have _permissions_ associated with them that track:

- What set of memory addresses are allowed to be accessed
- When the pointer is allowed to access those addresses
- If the pointer is allowed to mutate the memory, or just read it

These permissions are pointer provenance. They aren't stored at runtime,
the Rust compiler doesn't know them at compile time! Miri is the only
tool that I know of that tracks provenance, which it uses to ensure
pointers adheres to their spatial, temporal, and mutability permissions.
That's why I mentioned Miri in the Testing section. :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Pointers Relating to Bevy pointer abstractions D-Unsafe Touches with unsafe code in some way P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants