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

Adjust Allocation Bytes used by Miri to custom MiriAllocBytes #3526

Merged
merged 1 commit into from
May 17, 2024

Conversation

Strophox
Copy link
Contributor

@Strophox Strophox commented Apr 28, 2024

Previously, the MiriMachine used type Bytes = Box<[u8]> for its allocations.
This PR swaps this out for a custom MiriAllocBytes type implemented in alloc_bytes.rs.
This is in anticipation of an extension to Miri's FFI, which will require its allocations to take care of alignment (the methods in impl AllocBytes for Box<[u8]> ignore this _align: Align argument).

Needs rust-lang/rust#124492

@Strophox Strophox marked this pull request as ready for review April 29, 2024 13:40
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good start!

src/alloc_bytes.rs Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
src/provenance_gc.rs Outdated Show resolved Hide resolved
src/alloc_bytes.rs Outdated Show resolved Hide resolved
@RalfJung

This comment was marked as outdated.

@RalfJung RalfJung mentioned this pull request May 3, 2024
bors added a commit that referenced this pull request May 3, 2024
Preparing for merge from rustc

Unblocks #3526.
@RalfJung
Copy link
Member

RalfJung commented May 4, 2024

If you rebase over the latest Miri master, you should get the updated rustc that includes your PR, so that this one can finally build. :)

RalfJung pushed a commit to RalfJung/rust that referenced this pull request May 4, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this pull request May 4, 2024
@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 16, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/miri.git master
$ git push --force-with-lease

The following commits are merge commits:

src/alloc_bytes.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Looks good, thanks!

Please squash this into a single commit.

attempt changing Bytes in MiriMachine to MiriAllocBytes

rename miri_alloc_bytes to alloc_bytes

generalize impl VisitProvenance for Allocation for any Bytes: AllocBytes

mend MiriAllocBytes -> Self::Bytes

fix Invariant documentation and bugs (drop), impl Clone

Update MiriAllocBytes description

Co-authored-by: Ralf Jung <post@ralfj.de>

Rephrase MiriAllocBytes ptr invariant

Co-authored-by: Ralf Jung <post@ralfj.de>

Update MiriAllocBytes ptr documentation

Co-authored-by: Ralf Jung <post@ralfj.de>

fix safety comment in MiriAllocBytes::clone

fix safety comment in MiriAllocBytes::from_bytes

try implementing clone without unsafe

remove derive(PartialEq,Eq,Hash), fix fmt

move ptr.is_null() check inside only necessary branch

use std::ptr::without_provenance_mut, requiring feature(strict_provenance)

align.bytes_usize() instead of align.bytes().try_into().unwrap()

Update src/provenance_gc.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

fix clippy error on deref
@RalfJung
Copy link
Member

Thanks!

Force-pushes do not always trigger a notification on github, so you have to let the reviewer know that the PR is ready again. In Rust we have the @rustbot ready command for that.

@bors r+

@bors
Copy link
Contributor

bors commented May 17, 2024

📌 Commit cf6330c has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 17, 2024

⌛ Testing commit cf6330c with merge fef1043...

@bors
Copy link
Contributor

bors commented May 17, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing fef1043 to master...

@bors bors merged commit fef1043 into rust-lang:master May 17, 2024
8 checks passed
bors added a commit that referenced this pull request May 24, 2024
Bugfix `MiriAllocBytes` to guarantee different addresses

Fix in `alloc_bytes.rs` following #3526

Currently when an allocation of `size == 0` is requested we return a `std::ptr::without_provenance_mut(align)`, but this means returned `ptr`s may overlap, which breaks things.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request May 26, 2024
Bugfix `MiriAllocBytes` to guarantee different addresses

Fix in `alloc_bytes.rs` following rust-lang/miri#3526

Currently when an allocation of `size == 0` is requested we return a `std::ptr::without_provenance_mut(align)`, but this means returned `ptr`s may overlap, which breaks things.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Bugfix `MiriAllocBytes` to guarantee different addresses

Fix in `alloc_bytes.rs` following rust-lang/miri#3526

Currently when an allocation of `size == 0` is requested we return a `std::ptr::without_provenance_mut(align)`, but this means returned `ptr`s may overlap, which breaks things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants