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

Encapsulate interpreter stack(s) a bit better #104073

Closed
Closed
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
39 changes: 34 additions & 5 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use rustc_target::spec::abi::Abi as CallAbi;

use crate::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, Frame, ImmTy, InterpCx, InterpResult,
OpTy, PlaceTy, Pointer, Scalar, StackPopUnwind,
OpTy, Operand, PlaceTy, Pointer, Scalar, StackPopUnwind,
};

use super::error::*;
Expand Down Expand Up @@ -471,11 +471,40 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
&ecx.machine.stack
}

#[inline(always)]
fn stack_mut<'a>(
fn push_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
) {
ecx.machine.stack.push(frame);
}

fn pop_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
) -> Option<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
ecx.machine.stack.pop()
}

fn frame<'a>(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.machine.stack.last().expect("no call frames exist")
}

fn frame_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
&mut ecx.machine.stack
) -> &'a mut Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.machine.stack.last_mut().expect("no call frames exist")
}

fn access_local_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
frame: usize,
local: mir::Local,
) -> InterpResult<'tcx, &'a mut Operand<Self::Provenance>>
where
'tcx: 'mir,
{
ecx.machine.stack[frame].locals[local].access_mut()
}

fn before_access_global(
Expand Down
20 changes: 10 additions & 10 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -426,11 +426,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
M::stack(self)
}

#[inline(always)]
pub(crate) fn stack_mut(
&mut self,
) -> &mut Vec<Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>> {
M::stack_mut(self)
pub(crate) fn push_frame(&mut self, frame: Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>) {
M::push_frame(self, frame)
}

pub(crate) fn pop_frame(&mut self) -> Option<Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>> {
M::pop_frame(self)
}

#[inline(always)]
Expand All @@ -442,12 +443,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

#[inline(always)]
pub fn frame(&self) -> &Frame<'mir, 'tcx, M::Provenance, M::FrameExtra> {
self.stack().last().expect("no call frames exist")
M::frame(self)
}

#[inline(always)]
pub fn frame_mut(&mut self) -> &mut Frame<'mir, 'tcx, M::Provenance, M::FrameExtra> {
self.stack_mut().last_mut().expect("no call frames exist")
M::frame_mut(self)
}

#[inline(always)]
Expand Down Expand Up @@ -690,7 +691,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
extra: (),
};
let frame = M::init_frame_extra(self, pre_frame)?;
self.stack_mut().push(frame);
self.push_frame(frame);

// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
for ct in &body.required_consts {
Expand Down Expand Up @@ -830,8 +831,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

// All right, now it is time to actually pop the frame.
// Note that its locals are gone already, but that's fine.
let frame =
self.stack_mut().pop().expect("tried to pop a stack frame, but there were none");
let frame = self.pop_frame().expect("tried to pop a stack frame, but there were none");
// Report error from return value copy, if any.
copy_ret_result?;

Expand Down
24 changes: 16 additions & 8 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,17 +222,13 @@ pub trait Machine<'mir, 'tcx>: Sized {
///
/// Due to borrow checker trouble, we indicate the `frame` as an index rather than an `&mut
/// Frame`.
#[inline]
fn access_local_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
frame: usize,
local: mir::Local,
) -> InterpResult<'tcx, &'a mut Operand<Self::Provenance>>
where
'tcx: 'mir,
{
ecx.stack_mut()[frame].locals[local].access_mut()
}
'tcx: 'mir;

/// Called before a basic block terminator is executed.
/// You can use this to detect endlessly running programs.
Expand Down Expand Up @@ -394,10 +390,22 @@ pub trait Machine<'mir, 'tcx>: Sized {
ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>];

/// Mutably borrow the current thread's stack.
fn stack_mut<'a>(
fn push_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
);

fn pop_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
) -> Option<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>>;

fn frame<'a>(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>;

fn frame_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>>;
) -> &'a mut Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>;
Copy link
Member

Choose a reason for hiding this comment

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

These should all have doc comments. (In particular the last two need to explain which frame they return.)


/// Called immediately after a stack frame got pushed and its locals got initialized.
fn after_stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
};
match res {
Err(err) => {
self.stack_mut().pop();
self.pop_frame();
Err(err)
}
Ok(()) => Ok(()),
Expand Down
26 changes: 22 additions & 4 deletions compiler/rustc_mir_transform/src/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,29 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
&ecx.machine.stack
}

#[inline(always)]
fn stack_mut<'a>(
fn push_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
) {
ecx.machine.stack.push(frame);
}

fn pop_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
) -> Option<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
ecx.machine.stack.pop()
}

fn frame<'a>(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.machine.stack.last().expect("no call frames exist")
}

fn frame_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
&mut ecx.machine.stack
) -> &'a mut Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.machine.stack.last_mut().expect("no call frames exist")
}
}

Expand Down
1 change: 1 addition & 0 deletions src/tools/miri/src/concurrency/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ pub mod init_once;
pub mod thread;
mod vector_clock;
pub mod weak_memory;
mod stack;
47 changes: 47 additions & 0 deletions src/tools/miri/src/concurrency/stack.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
use crate::*;

/// An encapsulated call stack, so encapsulated to enable efficient caching of metadata about the
/// contained `Frame`.
Copy link
Member

Choose a reason for hiding this comment

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

This seems to give out mutable access to basically the entire thing; in which sense is this 'encapsulated'?

pub struct Stack<'mir, 'tcx> {
frames: Vec<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>>,
}

impl<'mir, 'tcx> Stack<'mir, 'tcx> {
pub fn new() -> Self {
Stack { frames: Vec::new() }
}

/// Does this `Stack` contain any `Frame`s?
pub fn is_empty(&self) -> bool {
self.frames.is_empty()
}

/// Borrow the call frames of a `Stack`.
pub fn frames(&self) -> &[Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>] {
&self.frames[..]
}

/// Mutably borrow the call frames of a `Stack`.
pub fn frames_mut(&mut self) -> &mut [Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>] {
&mut self.frames[..]
}

/// Push a new `Frame` onto a `Stack`.
pub fn push(&mut self, frame: Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>) {
self.frames.push(frame);
}

/// Try to pop a `Frame` from a `Stack`.
pub fn pop(&mut self) -> Option<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>> {
self.frames.pop()
}
}

impl VisitTags for Stack<'_, '_> {
fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) {
let Stack { frames } = self;
for frame in frames {
frame.visit_tags(visit);
}
}
}
38 changes: 22 additions & 16 deletions src/tools/miri/src/concurrency/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_target::spec::abi::Abi;

use crate::concurrency::data_race;
use crate::concurrency::sync::SynchronizationState;
use crate::concurrency::stack::Stack;
use crate::*;

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -116,7 +117,7 @@ pub struct Thread<'mir, 'tcx> {
thread_name: Option<Vec<u8>>,

/// The virtual call stack.
stack: Vec<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>>,
stack: Stack<'mir, 'tcx>,

/// The join status.
join_status: ThreadJoinStatus,
Expand Down Expand Up @@ -166,7 +167,7 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> {
Self {
state: ThreadState::Enabled,
thread_name: None,
stack: Vec::new(),
stack: Stack::new(),
join_status: ThreadJoinStatus::Joinable,
panic_payload: None,
last_error: None,
Expand All @@ -189,9 +190,7 @@ impl VisitTags for Thread<'_, '_> {

panic_payload.visit_tags(visit);
last_error.visit_tags(visit);
for frame in stack {
frame.visit_tags(visit)
}
stack.visit_tags(visit);
}
}

Expand Down Expand Up @@ -345,20 +344,18 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {

/// Borrow the stack of the active thread.
pub fn active_thread_stack(&self) -> &[Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>] {
&self.threads[self.active_thread].stack
self.threads[self.active_thread].stack.frames()
}

/// Mutably borrow the stack of the active thread.
fn active_thread_stack_mut(
&mut self,
) -> &mut Vec<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>> {
&mut self.threads[self.active_thread].stack
pub fn active_thread_stack_mut(&mut self) -> &mut [Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>] {
self.threads[self.active_thread].stack.frames_mut()
}

pub fn all_stacks(
&self,
) -> impl Iterator<Item = &[Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>]> {
self.threads.iter().map(|t| &t.stack[..])
self.threads.iter().map(|t| t.stack.frames())
}

/// Create a new thread and returns its id.
Expand Down Expand Up @@ -561,10 +558,11 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
// this allows us to have a deterministic scheduler.
for thread in self.threads.indices() {
match self.timeout_callbacks.entry(thread) {
Entry::Occupied(entry) =>
Entry::Occupied(entry) => {
if entry.get().call_time.get_wait_time(clock) == Duration::new(0, 0) {
return Some((thread, entry.remove().callback));
},
}
}
Entry::Vacant(_) => {}
}
}
Expand Down Expand Up @@ -863,13 +861,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}

#[inline]
fn active_thread_stack_mut(
&mut self,
) -> &mut Vec<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>> {
fn active_thread_stack_mut(&mut self) -> &mut [Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>] {
let this = self.eval_context_mut();
this.machine.threads.active_thread_stack_mut()
}

fn push_frame(&mut self, frame: Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>) {
let this = self.eval_context_mut();
this.machine.threads.active_thread_mut().stack.push(frame);
}

fn pop_frame(&mut self) -> Option<Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>> {
let this = self.eval_context_mut();
this.machine.threads.active_thread_mut().stack.pop()
}

/// Set the name of the current thread. The buffer must not include the null terminator.
#[inline]
fn set_thread_name(&mut self, thread: ThreadId, new_thread_name: Vec<u8>) {
Expand Down
36 changes: 33 additions & 3 deletions src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1138,10 +1138,40 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
ecx.active_thread_stack()
}

fn stack_mut<'a>(
fn push_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
) {
ecx.push_frame(frame);
}

fn pop_frame(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
) -> Option<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
ecx.pop_frame()
}

fn frame<'a>(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
) -> &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.active_thread_stack().last().expect("no call frames exist")
}

fn frame_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra> {
ecx.active_thread_stack_mut().last_mut().expect("no call frames exist")
}

fn access_local_mut<'a>(
ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
ecx.active_thread_stack_mut()
frame: usize,
local: mir::Local,
) -> InterpResult<'tcx, &'a mut Operand<Self::Provenance>>
where
'tcx: 'mir,
{
ecx.active_thread_stack_mut()[frame].locals[local].access_mut()
}

fn before_terminator(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
Expand Down