Skip to content

GC: factor out visiting all machine values #2559

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.

Sign up for GitHub

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 1 commit into from
Sep 23, 2022
Merged

GC: factor out visiting all machine values #2559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 27 additions & 4 deletions src/concurrency/thread.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,6 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
&mut self.threads[self.active_thread].stack
}

pub fn iter(&self) -> impl Iterator<Item = &Thread<'mir, 'tcx>> {
self.threads.iter()
}

pub fn all_stacks(
&self,
) -> impl Iterator<Item = &[Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>]> {
Expand Down Expand Up @@ -628,6 +624,33 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
}
}

impl VisitMachineValues for ThreadManager<'_, '_> {
fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>)) {
// FIXME some other fields also contain machine values
Copy link
Member

Choose a reason for hiding this comment

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

I see there are other fields with machine values, but it's not clear to me that for example thread_local_alloc_ids is keeping pointers live. Are there other fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that the only way you will reach the current value of thread-local variables? And a thread-local variable can store a pointer to somewhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's similar, I think, to the global base tag map in Stacked Borrows -- I don't know how your GC is not failing left and right due to removing the base tags.

Copy link
Member

@saethlin saethlin Sep 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Stack::retain never removes the bottom-most tag. (I forgot about this until just now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. Yeah that would help but needs to be carefully documented.

let ThreadManager { threads, .. } = self;

for thread in threads {
// FIXME: implement VisitMachineValues for `Thread` and `Frame` instead.
// In particular we need to visit the `last_error` and `catch_unwind` fields.
if let Some(payload) = thread.panic_payload {
visit(&Operand::Immediate(Immediate::Scalar(payload)))
}
for frame in &thread.stack {
// Return place.
if let Place::Ptr(mplace) = *frame.return_place {
visit(&Operand::Indirect(mplace));
}
// Locals.
for local in frame.locals.iter() {
if let LocalValue::Live(value) = &local.value {
visit(value);
}
}
}
}
}
}

// Public interface to thread management.
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ pub use crate::range_map::RangeMap;
pub use crate::stacked_borrows::{
CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, SbTag, Stack, Stacks,
};
pub use crate::tag_gc::EvalContextExt as _;
pub use crate::tag_gc::{EvalContextExt as _, VisitMachineValues};

/// Insert rustc arguments at the beginning of the argument list that Miri wants to be
/// set per default, for maximal validation power.
Expand Down
14 changes: 14 additions & 0 deletions src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ impl<'mir, 'tcx: 'mir> PrimitiveLayouts<'tcx> {
}

/// The machine itself.
///
/// If you add anything here that stores machine values, remember to update
/// `visit_all_machine_values`!
pub struct MiriMachine<'mir, 'tcx> {
// We carry a copy of the global `TyCtxt` for convenience, so methods taking just `&Evaluator` have `tcx` access.
pub tcx: TyCtxt<'tcx>,
Expand Down Expand Up @@ -586,6 +589,17 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
}
}

impl VisitMachineValues for MiriMachine<'_, '_> {
fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>)) {
// FIXME: visit the missing fields: env vars, weak mem, the MemPlace fields in the machine,
// DirHandler, extern_statics, the Stacked Borrows base pointers; maybe more.
let MiriMachine { threads, tls, .. } = self;

threads.visit_machine_values(visit);
tls.visit_machine_values(visit);
}
}

/// A rustc InterpCx for Miri.
pub type MiriInterpCx<'mir, 'tcx> = InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>;

Expand Down
13 changes: 10 additions & 3 deletions src/shims/tls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,17 @@ impl<'tcx> TlsData<'tcx> {
data.remove(&thread_id);
}
}
}

impl VisitMachineValues for TlsData<'_> {
fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>)) {
let TlsData { keys, macos_thread_dtors, next_key: _, dtors_running: _ } = self;

pub fn iter(&self, mut visitor: impl FnMut(&Scalar<Provenance>)) {
for scalar in self.keys.values().flat_map(|v| v.data.values()) {
visitor(scalar);
for scalar in keys.values().flat_map(|v| v.data.values()) {
visit(&Operand::Immediate(Immediate::Scalar(*scalar)));
}
for (_, scalar) in macos_thread_dtors.values() {
visit(&Operand::Immediate(Immediate::Scalar(*scalar)));
}
}
}
Expand Down
125 changes: 52 additions & 73 deletions src/tag_gc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,34 @@
use crate::*;
use rustc_data_structures::fx::FxHashSet;

use crate::*;

pub trait VisitMachineValues {
fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>));
}

impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
/// Generic GC helper to visit everything that can store a value. The `acc` offers some chance to
/// accumulate everything.
fn visit_all_machine_values<T>(
&self,
acc: &mut T,
mut visit_operand: impl FnMut(&mut T, &Operand<Provenance>),
mut visit_alloc: impl FnMut(&mut T, &Allocation<Provenance, AllocExtra>),
) {
let this = self.eval_context_ref();

// Memory.
this.memory.alloc_map().iter(|it| {
for (_id, (_kind, alloc)) in it {
visit_alloc(acc, alloc);
}
});

// And all the other machine values.
this.machine.visit_machine_values(&mut |op| visit_operand(acc, op));
}

fn garbage_collect_tags(&mut self) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
// No reason to do anything at all if stacked borrows is off.
Expand All @@ -12,90 +38,43 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {

let mut tags = FxHashSet::default();

for thread in this.machine.threads.iter() {
if let Some(Scalar::Ptr(
Pointer { provenance: Provenance::Concrete { sb, .. }, .. },
_,
)) = thread.panic_payload
{
tags.insert(sb);
}
}

self.find_tags_in_tls(&mut tags);
self.find_tags_in_memory(&mut tags);
self.find_tags_in_locals(&mut tags)?;

self.remove_unreachable_tags(tags);

Ok(())
}

fn find_tags_in_tls(&mut self, tags: &mut FxHashSet<SbTag>) {
let this = self.eval_context_mut();
this.machine.tls.iter(|scalar| {
if let Scalar::Ptr(Pointer { provenance: Provenance::Concrete { sb, .. }, .. }, _) =
scalar
{
tags.insert(*sb);
}
});
}

fn find_tags_in_memory(&mut self, tags: &mut FxHashSet<SbTag>) {
let this = self.eval_context_mut();
this.memory.alloc_map().iter(|it| {
for (_id, (_kind, alloc)) in it {
for (_size, prov) in alloc.provenance().iter() {
if let Provenance::Concrete { sb, .. } = prov {
tags.insert(*sb);
}
}
}
});
}

fn find_tags_in_locals(&mut self, tags: &mut FxHashSet<SbTag>) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
for frame in this.machine.threads.all_stacks().flatten() {
// Handle the return place of each frame
if let Ok(return_place) = frame.return_place.try_as_mplace() {
if let Some(Provenance::Concrete { sb, .. }) = return_place.ptr.provenance {
let visit_scalar = |tags: &mut FxHashSet<SbTag>, s: &Scalar<Provenance>| {
if let Scalar::Ptr(ptr, _) = s {
if let Provenance::Concrete { sb, .. } = ptr.provenance {
tags.insert(sb);
}
}
};

for local in frame.locals.iter() {
let LocalValue::Live(value) = local.value else {
continue;
};
match value {
Operand::Immediate(Immediate::Scalar(Scalar::Ptr(ptr, _))) =>
if let Provenance::Concrete { sb, .. } = ptr.provenance {
tags.insert(sb);
},
this.visit_all_machine_values(
&mut tags,
|tags, op| {
match op {
Operand::Immediate(Immediate::Scalar(s)) => {
visit_scalar(tags, s);
}
Operand::Immediate(Immediate::ScalarPair(s1, s2)) => {
if let Scalar::Ptr(ptr, _) = s1 {
if let Provenance::Concrete { sb, .. } = ptr.provenance {
tags.insert(sb);
}
}
if let Scalar::Ptr(ptr, _) = s2 {
if let Provenance::Concrete { sb, .. } = ptr.provenance {
tags.insert(sb);
}
}
visit_scalar(tags, s1);
visit_scalar(tags, s2);
}
Operand::Immediate(Immediate::Uninit) => {}
Operand::Indirect(MemPlace { ptr, .. }) => {
if let Some(Provenance::Concrete { sb, .. }) = ptr.provenance {
tags.insert(sb);
}
}
Operand::Immediate(Immediate::Uninit)
| Operand::Immediate(Immediate::Scalar(Scalar::Int(_))) => {}
}
}
}
},
|tags, alloc| {
for (_size, prov) in alloc.provenance().iter() {
if let Provenance::Concrete { sb, .. } = prov {
tags.insert(*sb);
}
}
},
);

self.remove_unreachable_tags(tags);

Ok(())
}
Expand Down