Skip to content

Pausing execution of interpreter #687

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Pausing execution of interpreter #687

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e5af2f
Pausing execution of interpreter.
resulknad Nov 1, 2024
5e60115
Merge branch 'main' into dev/wasm-pause
ia0 Nov 12, 2024
3eb6b3d
Dedicated tests and interrupt bool on store instead of argument.
resulknad Nov 29, 2024
08a9d41
Remove obsolete comment.
resulknad Nov 29, 2024
f986ed4
Reworked interrupt.rs
resulknad Dec 2, 2024
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 crates/interpreter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ float-types = ["dep:libm"]
vector-types = []
# Enable caching for execution.
cache = ["dep:lru"]
# Enable interrupting execution.
interrupt = []

[lints]
clippy.unit-arg = "allow"
Expand Down
141 changes: 125 additions & 16 deletions crates/interpreter/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
// TODO: Some toctou could be used instead of panic.
use alloc::vec;
use alloc::vec::Vec;
#[cfg(feature = "interrupt")]
use core::sync::atomic::Ordering::Relaxed;

#[cfg(feature = "interrupt")]
use portable_atomic::AtomicBool;

use crate::error::*;
use crate::module::*;
Expand Down Expand Up @@ -58,6 +63,9 @@ pub struct Store<'m> {
// functions in `funcs` is stored to limit normal linking to that part.
func_default: Option<(&'m str, usize)>,
threads: Vec<Continuation<'m>>,

Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change

#[cfg(feature = "interrupt")]
interrupt: Option<&'m AtomicBool>,
}

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
Expand Down Expand Up @@ -99,6 +107,8 @@ impl Default for Store<'_> {
funcs: vec![],
func_default: None,
threads: vec![],
#[cfg(feature = "interrupt")]
interrupt: None,
}
}
}
Expand Down Expand Up @@ -195,9 +205,24 @@ impl<'m> Store<'m> {
let mut parser = self.insts[inst_id].module.func(ptr.index());
let mut locals = Vec::new();
append_locals(&mut parser, &mut locals);
let thread = Thread::new(parser, Frame::new(inst_id, 0, &[], locals));
let thread = Thread::new(
parser,
Frame::new(inst_id, 0, &[], locals),
#[cfg(feature = "interrupt")]
self.interrupt,
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to have a copy of interrupt in Thread if we pass the whole store to pop_label. This should simplify quite some the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not passing &mut Store to pop_label because let inst: &mut Instance<'m> = &mut store.insts[inst_id]; already has a mutable reference to store. So rustc will complain with cannot borrow store as mutable more than once at a time.

This is why I passed a mutable reference to store.threads only. Do you have a suggestion on how to work around this?

Copy link
Member

Choose a reason for hiding this comment

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

You're not gonna like it, but I actually think the solution is to do the safe thing and not piggyback interruption into host calls. The general design would be:

  • Create an enum Interrupt<'m> (see below)
  • Add an interrupt: Interrupt<'m> field to Store<'m> (gated with feature = "interrupt")
  • We don't need to pass store.threads anymore, we can pass store.interrupt instead (gated too)
#[cfg(feature = "interrupt")]
enum Interrupt<'m> {
    Unbounded,
    Bounded {
        control: &'m AtomicBool,
        state: Option<Thread<'m>>,
    },
}

Copy link
Member

Choose a reason for hiding this comment

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

Forgot some steps (that were in the older comment):

  • Add some is_interrupted() function similar to last_call() and some resume_interruption() (maybe with better names and types)
  • Make sure in last_call() to return None if we're interrupted

);

// Disable interrupts for the start section.
Copy link
Member

Choose a reason for hiding this comment

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

We want an abstraction to avoid losing the atomic bool.

fn Store::without_interrupt<R>(&mut self, operation: impl FnOnce(&mut Store) -> R) -> R {
    #[cfg(feature = "interrupt")]
    let saved = self.interrupt.take();
    let result = operation(self);
    #[cfg(feature = "interrupt")]
    self.interrupt = saved;
    result
}

Then here we would:

self.without_interrupt(|this| {
    let result = thread.run(this)?;
    assert!(matches!(...));
})?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, if we do not pass the atomic bool when creating the thread just above, there is no need to manipulate anything on the store right?

Also I was running into some lifetime issues with the snippet above. Without any lifetime annotations, it will require the Store to live forever ('static). My first attempt at adding some lifetime annotations wasn't really successful but we can look more into that in case we decide that we do need this function.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, my code snippet was wrong, we need to specify the store lifetime:

    fn without_interrupt<R>(&mut self, operation: impl FnOnce(&mut Store<'m>) -> R) -> R {

And then it can be used like that:

            let thread = Thread::new(parser, Frame::new(inst_id, 0, &[], locals));
            self.without_interrupt(|x| {
                let result = thread.run(x)?;
                assert!(matches!(result, RunResult::Done(x) if x.is_empty()));
                Ok(())
            })?;

#[cfg(feature = "interrupt")]
let interrupt = self.interrupt;
#[cfg(feature = "interrupt")]
self.set_interrupt(None);

let result = thread.run(self)?;
assert!(matches!(result, RunResult::Done(x) if x.is_empty()));

#[cfg(feature = "interrupt")]
self.set_interrupt(interrupt);
}
Ok(InstId { store_id: self.id, inst_id })
}
Expand Down Expand Up @@ -225,7 +250,13 @@ impl<'m> Store<'m> {
let mut locals = args;
append_locals(&mut parser, &mut locals);
let frame = Frame::new(inst_id, t.results.len(), &[], locals);
Thread::new(parser, frame).run(self)
Thread::new(
parser,
frame,
#[cfg(feature = "interrupt")]
self.interrupt,
)
.run(self)
}

/// Returns the value of a global of an instance.
Expand Down Expand Up @@ -303,6 +334,11 @@ impl<'m> Store<'m> {
Some(Call { store: self })
}
}

#[cfg(feature = "interrupt")]
pub fn set_interrupt(&mut self, interrupt: Option<&'m AtomicBool>) {
self.interrupt = interrupt;
}
}

impl<'a, 'm> Call<'a, 'm> {
Expand Down Expand Up @@ -339,6 +375,12 @@ impl<'a, 'm> Call<'a, 'm> {
thread.run(self.store)
}

// Returns if this call is due to an interrupt.
#[cfg(feature = "interrupt")]
pub fn is_interrupt(&self) -> bool {
self.cont().interrupted
}

fn cont(&self) -> &Continuation {
self.store.threads.last().unwrap()
}
Expand Down Expand Up @@ -460,6 +502,8 @@ struct Instance<'m> {
struct Thread<'m> {
parser: Parser<'m>,
frames: Vec<Frame<'m>>,
#[cfg(feature = "interrupt")]
interrupt: Option<&'m AtomicBool>,
}

/// Runtime result.
Expand All @@ -470,6 +514,10 @@ pub enum RunResult<'a, 'm> {

/// Execution is calling into the host.
Host(Call<'a, 'm>),

// Execution was interrupted by the host.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Execution was interrupted by the host.
/// Execution was interrupted by the host.

#[cfg(feature = "interrupt")]
Interrupt(Call<'a, 'm>),
}

/// Runtime result without host call information.
Expand All @@ -484,6 +532,8 @@ impl RunResult<'_, '_> {
match self {
RunResult::Done(result) => RunAnswer::Done(result),
RunResult::Host(_) => RunAnswer::Host,
#[cfg(feature = "interrupt")]
RunResult::Interrupt(_) => RunAnswer::Host,
}
}
}
Expand All @@ -494,6 +544,8 @@ struct Continuation<'m> {
index: usize,
args: Vec<Val>,
arity: usize,
#[cfg(feature = "interrupt")]
interrupted: bool,
}

impl<'m> Store<'m> {
Expand Down Expand Up @@ -724,21 +776,39 @@ enum ThreadResult<'m> {
Continue(Thread<'m>),
Done(Vec<Val>),
Host,
#[cfg(feature = "interrupt")]
Interrupt,
}

impl<'m> Thread<'m> {
fn new(parser: Parser<'m>, frame: Frame<'m>) -> Thread<'m> {
Thread { parser, frames: vec![frame] }
fn new(
parser: Parser<'m>, frame: Frame<'m>,
#[cfg(feature = "interrupt")] interrupt: Option<&'m AtomicBool>,
) -> Thread<'m> {
Thread {
parser,
frames: vec![frame],

#[cfg(feature = "interrupt")]
interrupt,
}
}

fn const_expr(store: &mut Store<'m>, inst_id: usize, mut_parser: &mut Parser<'m>) -> Val {
let parser = mut_parser.clone();
let mut thread = Thread::new(parser, Frame::new(inst_id, 1, &[], Vec::new()));
let mut thread = Thread::new(
parser,
Frame::new(inst_id, 1, &[], Vec::new()),
#[cfg(feature = "interrupt")]
store.interrupt,
);
let (parser, results) = loop {
let p = thread.parser.save();
match thread.step(store).unwrap() {
ThreadResult::Continue(x) => thread = x,
ThreadResult::Done(x) => break (p, x),
#[cfg(feature = "interrupt")]
ThreadResult::Interrupt => unreachable!(),
ThreadResult::Host => unreachable!(),
}
};
Expand All @@ -757,6 +827,8 @@ impl<'m> Thread<'m> {
ThreadResult::Continue(x) => self = x,
ThreadResult::Done(x) => return Ok(RunResult::Done(x)),
ThreadResult::Host => return Ok(RunResult::Host(Call { store })),
#[cfg(feature = "interrupt")]
ThreadResult::Interrupt => return Ok(RunResult::Interrupt(Call { store })),
}
}
}
Expand All @@ -765,7 +837,7 @@ impl<'m> Thread<'m> {
use Instr::*;
let saved = self.parser.save();
let inst_id = self.frame().inst_id;
let inst = &mut store.insts[inst_id];
let inst: &mut Instance<'m> = &mut store.insts[inst_id];
match self.parser.parse_instr().into_ok() {
Unreachable => return Err(trap()),
Nop => (),
Expand All @@ -783,15 +855,15 @@ impl<'m> Thread<'m> {
return Ok(self.exit_label());
}
End => return Ok(self.exit_label()),
Br(l) => return Ok(self.pop_label(inst, l)),
Br(l) => return self.pop_label(inst, l, &mut store.threads),
BrIf(l) => {
if self.pop_value().unwrap_i32() != 0 {
return Ok(self.pop_label(inst, l));
return self.pop_label(inst, l, &mut store.threads);
}
}
BrTable(ls, ln) => {
let i = self.pop_value().unwrap_i32() as usize;
return Ok(self.pop_label(inst, ls.get(i).cloned().unwrap_or(ln)));
return self.pop_label(inst, ls.get(i).cloned().unwrap_or(ln), &mut store.threads);
}
Return => return Ok(self.exit_frame()),
Call(x) => return self.invoke(store, store.func_ptr(inst_id, x)),
Expand Down Expand Up @@ -1035,20 +1107,50 @@ impl<'m> Thread<'m> {
self.labels().push(label);
}

fn pop_label(mut self, inst: &mut Instance<'m>, l: LabelIdx) -> ThreadResult<'m> {
#[allow(clippy::ptr_arg)]
fn check_interrupt_or_continue(self, _threads: &mut Vec<Continuation<'m>>) -> ThreadResult<'m> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this unbounded_continue, such that it doesn't mention interrupt.

#[cfg(feature = "interrupt")]
if self.interrupt.is_some_and(|interrupt| {
interrupt.compare_exchange_weak(true, false, Relaxed, Relaxed).is_ok()
}) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.interrupt.is_some_and(|interrupt| {
interrupt.compare_exchange_weak(true, false, Relaxed, Relaxed).is_ok()
}) {
if self.interrupt.is_some_and(|x|x.swap(false, Relaxed)) {

_threads.push(Continuation {
thread: self,
index: 0,
args: vec![],
arity: 0,
#[cfg(feature = "interrupt")]
interrupted: true,
});
return ThreadResult::Interrupt;
}

ThreadResult::Continue(self)
}

fn pop_label(
mut self, inst: &mut Instance<'m>, l: LabelIdx, threads: &mut Vec<Continuation<'m>>,
) -> Result<ThreadResult<'m>, Error> {
let i = self.labels().len() - l as usize - 1;
if i == 0 {
return self.exit_frame();
return Ok(self.exit_frame());
}
let values = core::mem::take(self.values());
let frame = self.frame();
let Label { arity, kind, .. } = frame.labels.drain(i ..).next().unwrap();
self.values().extend_from_slice(&values[values.len() - arity ..]);

match kind {
LabelKind::Loop(pos) => unsafe { self.parser.restore(pos) },
LabelKind::Block | LabelKind::If => self.skip_to_end(inst, l),
LabelKind::Loop(pos) => {
unsafe {
self.parser.restore(pos);
}
Ok(self.check_interrupt_or_continue(threads))
}
LabelKind::Block | LabelKind::If => {
self.skip_to_end(inst, l);
Ok(ThreadResult::Continue(self))
}
}
ThreadResult::Continue(self)
}

fn exit_label(mut self) -> ThreadResult<'m> {
Expand Down Expand Up @@ -1355,7 +1457,14 @@ impl<'m> Thread<'m> {
let t = store.funcs[index].1;
let arity = t.results.len();
let args = self.pop_values(t.params.len());
store.threads.push(Continuation { thread: self, arity, index, args });
store.threads.push(Continuation {
thread: self,
arity,
index,
args,
#[cfg(feature = "interrupt")]
interrupted: false,
});
return Ok(ThreadResult::Host);
}
Side::Wasm(x) => x,
Expand All @@ -1366,7 +1475,7 @@ impl<'m> Thread<'m> {
let ret = self.parser.save();
self.parser = parser;
self.frames.push(Frame::new(inst_id, t.results.len(), ret, locals));
Ok(ThreadResult::Continue(self))
Ok(self.check_interrupt_or_continue(&mut store.threads))
}
}

Expand Down
4 changes: 3 additions & 1 deletion crates/interpreter/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ list_files() {
-maxdepth 1 -name '*.wast' -execdir basename -s .wast {} \;
}
list_tests() {
sed -n 's/^test!(.*, "\([^"]*\)".*);$/\1/p;s/^test!(\([^,]*\).*);$/\1/p' tests/spec.rs
sed -n 's/^test!(.*, "\([^"]*\)".*);$/\1/p;s/^test!(\([^,]*\).*);$/\1/p;s/^test!("[^"]*",[^,]+,"\([^"]*\)");$/\1/p' tests/spec.rs | sort
}
diff_sorted tests/spec.rs "$(list_files | sort)" $(list_tests)

Expand All @@ -40,3 +40,5 @@ RUSTFLAGS=--cfg=portable_atomic_unsafe_assume_single_core \
cargo check --example=hello
# Run with `-- --test-threads=1 --nocapture` to see unsupported tests.
cargo test --test=spec --features=debug,toctou,float-types,vector-types
cargo test --test=spec --features=debug,toctou,float-types,vector-types,interrupt
cargo test --test=interrupt --all-features
Binary file added crates/interpreter/tests/infinite_loop.wasm
View file
Open in desktop
Binary file not shown.
21 changes: 21 additions & 0 deletions crates/interpreter/tests/infinite_loop.wat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
;; Use `wat2wasm infinite_loop.wat` to regenerate `.wasm`.
(module
(import "env" "count" (func $count (result i32)))

(memory (export "memory") 1)
(func (export "loopforever")
(local i32 i32)
(loop
(local.set 0 (call $count))
(local.set 1 (i32.const 1))
(block
(loop
(br_if 1 (i32.gt_u (local.get 1) (local.get 0)))
(local.set 1 (i32.add (local.get 1) (i32.const 1)))
(br 0)
)
)
(br 0)
)
)
)
Loading
Loading