Skip to content

fix deallocation of immutable allocations #85599

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

Merged
merged 5 commits into from
May 23, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_cranelift/src/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use rustc_span::DUMMY_SP;

use rustc_ast::Mutability;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::ErrorReported;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
Expand Down Expand Up @@ -175,6 +176,7 @@ pub(crate) fn codegen_const_value<'tcx>(
let mut alloc = Allocation::from_bytes(
std::iter::repeat(0).take(size.bytes_usize()).collect::<Vec<u8>>(),
align,
Mutability::Not,
Copy link
Member Author

Choose a reason for hiding this comment

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

@bjorn3 on the last PR I developed, ./x.py check compiler/rustc checked the cranelift files -- but now that does not seem to be the case any more, I only got the cranelift build errors from PR CI.

Copy link
Member

@bjorn3 bjorn3 May 23, 2021

Choose a reason for hiding this comment

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

Are you sure you previously used ./x.py check compiler/rustc? That explicitly says that compiler/rustc and its dependencies should be checked, but nothing else. cg_clif is not a dependency. ./x.py check compiler/rustc compiler/rustc_codegen_cranelift should work though. I think compiler/rustc is even unnecessary as it is marked as dep of cg_clif.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I am pretty sure -- I had that set in my vscode config.

Copy link
Member Author

Choose a reason for hiding this comment

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

(and I think I saw the errors in vscode... though there is a slim chance that I am hallucinating this and actually did ./x.py check on the console and copy-pasted the paths into vscode. I think I stopped doing that a while ago but maybe I misremember the timeline here.)

);
alloc.write_scalar(fx, alloc_range(Size::ZERO, size), x.into()).unwrap();
let alloc = fx.tcx.intern_const_alloc(alloc);
Expand Down
14 changes: 9 additions & 5 deletions compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,22 +99,26 @@ impl AllocRange {

// The constructors are all without extra; the extra gets added by a machine hook later.
impl<Tag> Allocation<Tag> {
/// Creates a read-only allocation initialized by the given bytes
pub fn from_bytes<'a>(slice: impl Into<Cow<'a, [u8]>>, align: Align) -> Self {
/// Creates an allocation initialized by the given bytes
pub fn from_bytes<'a>(
slice: impl Into<Cow<'a, [u8]>>,
align: Align,
mutability: Mutability,
) -> Self {
let bytes = slice.into().into_owned();
let size = Size::from_bytes(bytes.len());
Self {
bytes,
relocations: Relocations::new(),
init_mask: InitMask::new(size, true),
align,
mutability: Mutability::Not,
mutability,
extra: (),
}
}

pub fn from_byte_aligned_bytes<'a>(slice: impl Into<Cow<'a, [u8]>>) -> Self {
Allocation::from_bytes(slice, Align::ONE)
pub fn from_bytes_byte_aligned_immutable<'a>(slice: impl Into<Cow<'a, [u8]>>) -> Self {
Allocation::from_bytes(slice, Align::ONE, Mutability::Not)
}

pub fn uninit(size: Size, align: Align) -> Self {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ impl<'tcx> TyCtxt<'tcx> {
/// Allocates a read-only byte or string literal for `mir::interpret`.
pub fn allocate_bytes(self, bytes: &[u8]) -> interpret::AllocId {
// Create an allocation that just contains these bytes.
let alloc = interpret::Allocation::from_byte_aligned_bytes(bytes);
let alloc = interpret::Allocation::from_bytes_byte_aligned_immutable(bytes);
let alloc = self.intern_const_alloc(alloc);
self.create_memory_alloc(alloc)
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_mir/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,9 @@ pub(super) fn op_to_const<'tcx>(
(ecx.tcx.global_alloc(ptr.alloc_id).unwrap_memory(), ptr.offset.bytes())
}
Scalar::Int { .. } => (
ecx.tcx
.intern_const_alloc(Allocation::from_byte_aligned_bytes(b"" as &[u8])),
ecx.tcx.intern_const_alloc(Allocation::from_bytes_byte_aligned_immutable(
b"" as &[u8],
)),
0,
),
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::convert::TryFrom;

use rustc_ast::Mutability;
use rustc_hir::lang_items::LangItem;
use rustc_middle::mir::TerminatorKind;
use rustc_middle::ty::subst::Subst;
Expand Down Expand Up @@ -79,7 +80,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
line: u32,
col: u32,
) -> MPlaceTy<'tcx, M::PointerTag> {
let file = self.allocate_str(&filename.as_str(), MemoryKind::CallerLocation);
let file =
self.allocate_str(&filename.as_str(), MemoryKind::CallerLocation, Mutability::Not);
let line = Scalar::from_u32(line);
let col = Scalar::from_u32(col);

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/interpret/intrinsics/type_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,6 @@ impl Write for AbsolutePathPrinter<'_> {
/// Directly returns an `Allocation` containing an absolute path representation of the given type.
crate fn alloc_type_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> &'tcx Allocation {
let path = AbsolutePathPrinter { tcx, path: String::new() }.print_type(ty).unwrap().path;
let alloc = Allocation::from_byte_aligned_bytes(path.into_bytes());
let alloc = Allocation::from_bytes_byte_aligned_immutable(path.into_bytes());
tcx.intern_const_alloc(alloc)
}
10 changes: 6 additions & 4 deletions compiler/rustc_mir/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,11 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
pub fn allocate_bytes(
&mut self,
bytes: &[u8],
align: Align,
kind: MemoryKind<M::MemoryKind>,
mutability: Mutability,
) -> Pointer<M::PointerTag> {
let alloc = Allocation::from_byte_aligned_bytes(bytes);
let alloc = Allocation::from_bytes(bytes, align, mutability);
self.allocate_with(alloc, kind)
}

Expand Down Expand Up @@ -321,6 +323,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
}
};

if alloc.mutability == Mutability::Not {
throw_ub_format!("deallocating immutable allocation {}", ptr.alloc_id);
}
if alloc_kind != kind {
throw_ub_format!(
"deallocating {}, which is {} memory, using {} deallocation operation",
Expand Down Expand Up @@ -625,9 +630,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
// Need to make a copy, even if `get_global_alloc` is able
// to give us a cheap reference.
let alloc = Self::get_global_alloc(memory_extra, tcx, id, /*is_write*/ true)?;
if alloc.mutability == Mutability::Not {
throw_ub!(WriteToReadOnly(id))
}
let kind = M::GLOBAL_KIND.expect(
"I got a global allocation that I have to copy but the machine does \
not expect that to happen",
Expand Down
12 changes: 9 additions & 3 deletions compiler/rustc_mir/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::convert::TryFrom;
use std::fmt::Debug;
use std::hash::Hash;

use rustc_ast::Mutability;
use rustc_macros::HashStable;
use rustc_middle::mir;
use rustc_middle::ty::layout::{PrimitiveExt, TyAndLayout};
Expand Down Expand Up @@ -1024,18 +1025,23 @@ where
MPlaceTy::from_aligned_ptr(ptr, layout)
}

/// Returns a wide MPlace.
/// Returns a wide MPlace of type `&'static [mut] str` to a new 1-aligned allocation.
pub fn allocate_str(
&mut self,
str: &str,
kind: MemoryKind<M::MemoryKind>,
mutbl: Mutability,
) -> MPlaceTy<'tcx, M::PointerTag> {
let ptr = self.memory.allocate_bytes(str.as_bytes(), kind);
let ptr = self.memory.allocate_bytes(str.as_bytes(), Align::ONE, kind, mutbl);
let meta = Scalar::from_machine_usize(u64::try_from(str.len()).unwrap(), self);
let mplace =
MemPlace { ptr: ptr.into(), align: Align::ONE, meta: MemPlaceMeta::Meta(meta) };

let layout = self.layout_of(self.tcx.mk_static_str()).unwrap();
let ty = self.tcx.mk_ref(
self.tcx.lifetimes.re_static,
ty::TypeAndMut { ty: self.tcx.types.str_, mutbl },
);
let layout = self.layout_of(ty).unwrap();
MPlaceTy { mplace, layout }
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_build/src/thir/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ crate fn lit_to_const<'tcx>(
let lit = match (lit, &ty.kind()) {
(ast::LitKind::Str(s, _), ty::Ref(_, inner_ty, _)) if inner_ty.is_str() => {
let s = s.as_str();
let allocation = Allocation::from_byte_aligned_bytes(s.as_bytes());
let allocation = Allocation::from_bytes_byte_aligned_immutable(s.as_bytes());
let allocation = tcx.intern_const_alloc(allocation);
ConstValue::Slice { data: allocation, start: 0, end: s.len() }
}
(ast::LitKind::ByteStr(data), ty::Ref(_, inner_ty, _))
if matches!(inner_ty.kind(), ty::Slice(_)) =>
{
let allocation = Allocation::from_byte_aligned_bytes(data as &[u8]);
let allocation = Allocation::from_bytes_byte_aligned_immutable(data as &[u8]);
let allocation = tcx.intern_const_alloc(allocation);
ConstValue::Slice { data: allocation, start: 0, end: data.len() }
}
Expand Down