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

interpret: Fix writing uninit to an allocation #96162

Merged
merged 4 commits into from
Apr 20, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -892,8 +892,11 @@ impl<'tcx, 'a, Tag: Provenance, Extra> AllocRefMut<'a, 'tcx, Tag, Extra> {
}

/// Mark the entire referenced range as uninitalized
pub fn write_uninit(&mut self) {
self.alloc.mark_init(self.range, false);
pub fn write_uninit(&mut self) -> InterpResult<'tcx> {
Ok(self
.alloc
.write_uninit(&self.tcx, self.range)
.map_err(|e| e.to_interp_error(self.alloc_id))?)
}
}

Expand Down Expand Up @@ -1053,8 +1056,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// This also avoids writing to the target bytes so that the backing allocation is never
// touched if the bytes stay uninitialized for the whole interpreter execution. On contemporary
// operating system this can avoid physically allocating the page.
dest_alloc.mark_init(dest_range, false); // `Size` multiplication
dest_alloc.mark_relocation_range(relocations);
dest_alloc
.write_uninit(&tcx, dest_range)
.map_err(|e| e.to_interp_error(dest_alloc_id))?;
// We can forget about the relocations, this is all not initialized anyway.
return Ok(());
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ where
// Zero-sized access
return Ok(());
};
alloc.write_uninit();
alloc.write_uninit()?;
Ok(())
}

Expand Down
20 changes: 16 additions & 4 deletions compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ impl<Tag: Provenance, Extra> Allocation<Tag, Extra> {
/// `get_bytes_with_uninit_and_ptr` instead,
///
/// This function also guarantees that the resulting pointer will remain stable
/// even when new allocations are pushed to the `HashMap`. `copy_repeatedly` relies
/// even when new allocations are pushed to the `HashMap`. `mem_copy_repeatedly` relies
/// on that.
///
/// It is the caller's responsibility to check bounds and alignment beforehand.
Expand Down Expand Up @@ -429,8 +429,7 @@ impl<Tag: Provenance, Extra> Allocation<Tag, Extra> {
let val = match val {
ScalarMaybeUninit::Scalar(scalar) => scalar,
ScalarMaybeUninit::Uninit => {
self.mark_init(range, false);
return Ok(());
return self.write_uninit(cx, range);
}
};

Expand All @@ -455,6 +454,13 @@ impl<Tag: Provenance, Extra> Allocation<Tag, Extra> {

Ok(())
}

/// Write "uninit" to the given memory range.
pub fn write_uninit(&mut self, cx: &impl HasDataLayout, range: AllocRange) -> AllocResult {
self.mark_init(range, false);
self.clear_relocations(cx, range)?;
return Ok(());
}
}

/// Relocations.
Expand Down Expand Up @@ -599,6 +605,9 @@ impl<Tag: Copy, Extra> Allocation<Tag, Extra> {
/// Applies a relocation copy.
/// The affected range, as defined in the parameters to `prepare_relocation_copy` is expected
/// to be clear of relocations.
///
/// This is dangerous to use as it can violate internal `Allocation` invariants!
/// It only exists to support an efficient implementation of `mem_copy_repeatedly`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried finding a nice safe API we could add here to support mem_copy_repeatedly and make all this "compressed range" stuff private, but couldn't come up with anything good.

Copy link
Contributor

Choose a reason for hiding this comment

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

The warnings are there now, so we can look at it again in the future

pub fn mark_relocation_range(&mut self, relocations: AllocationRelocations<Tag>) {
self.relocations.0.insert_presorted(relocations.relative_relocations);
}
Expand Down Expand Up @@ -1056,7 +1065,7 @@ impl<Tag: Copy, Extra> Allocation<Tag, Extra> {
})
}

pub fn mark_init(&mut self, range: AllocRange, is_init: bool) {
fn mark_init(&mut self, range: AllocRange, is_init: bool) {
if range.size.bytes() == 0 {
return;
}
Expand Down Expand Up @@ -1118,6 +1127,9 @@ impl<Tag, Extra> Allocation<Tag, Extra> {
}

/// Applies multiple instances of the run-length encoding to the initialization mask.
///
/// This is dangerous to use as it can violate internal `Allocation` invariants!
/// It only exists to support an efficient implementation of `mem_copy_repeatedly`.
pub fn mark_compressed_init_range(
&mut self,
defined: &InitMaskCompressed,
Expand Down