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.

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

Jump to bottom

Fix a debug assertion in externref garbage collections #3734

Merged
merged 4 commits into from
Jan 28, 2022
Merged

Fix a debug assertion in externref garbage collections #3734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cbc6f60
Fix a debug assertion in `externref` garbage collections
fitzgen Jan 27, 2022
cc8d777
Make the `table_ops` test case generator use globals as well
fitzgen Jan 27, 2022
f292ff5
Do another GC after running Wasm in the `table_ops` test oracle
fitzgen Jan 27, 2022
19f8d94
Expand on activations table invariants comment in `libcalls.rs`
fitzgen Jan 28, 2022
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
152 changes: 127 additions & 25 deletions crates/fuzzing/src/generators/table_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@
use arbitrary::Arbitrary;
use std::ops::Range;
use wasm_encoder::{
CodeSection, EntityType, Export, ExportSection, Function, FunctionSection, ImportSection,
Instruction, Module, TableSection, TableType, TypeSection, ValType,
CodeSection, EntityType, Export, ExportSection, Function, FunctionSection, GlobalSection,
ImportSection, Instruction, Module, TableSection, TableType, TypeSection, ValType,
};

/// A description of a Wasm module that makes a series of `externref` table
/// operations.
#[derive(Arbitrary, Debug)]
pub struct TableOps {
num_params: u8,
num_globals: u8,
table_size: u32,
ops: Vec<TableOp>,
}

const NUM_PARAMS_RANGE: Range<u8> = 1..10;
const NUM_GLOBALS_RANGE: Range<u8> = 1..10;
const TABLE_SIZE_RANGE: Range<u32> = 1..100;
const MAX_OPS: usize = 100;

Expand All @@ -28,6 +30,13 @@ impl TableOps {
num_params
}

/// Get the number of globals this module has.
pub fn num_globals(&self) -> u8 {
let num_globals = std::cmp::max(self.num_globals, NUM_GLOBALS_RANGE.start);
let num_globals = std::cmp::min(num_globals, NUM_GLOBALS_RANGE.end);
num_globals
}

/// Get the size of the table that this module uses.
pub fn table_size(&self) -> u32 {
let table_size = std::cmp::max(self.table_size, TABLE_SIZE_RANGE.start);
Expand Down Expand Up @@ -98,6 +107,18 @@ impl TableOps {
maximum: None,
});

// Define our globals.
let mut globals = GlobalSection::new();
for _ in 0..self.num_globals() {
globals.global(
wasm_encoder::GlobalType {
val_type: wasm_encoder::ValType::ExternRef,
mutable: true,
},
Instruction::RefNull(wasm_encoder::ValType::ExternRef),
);
}

// Define the "run" function export.
let mut functions = FunctionSection::new();
functions.function(1);
Expand All @@ -111,7 +132,12 @@ impl TableOps {

func.instruction(Instruction::Loop(wasm_encoder::BlockType::Empty));
for op in self.ops.iter().take(MAX_OPS) {
op.insert(&mut func, self.num_params() as u32, self.table_size());
op.insert(
&mut func,
self.num_params() as u32,
self.table_size(),
self.num_globals() as u32,
);
}
func.instruction(Instruction::Br(0));
func.instruction(Instruction::End);
Expand All @@ -125,89 +151,118 @@ impl TableOps {
.section(&imports)
.section(&functions)
.section(&tables)
.section(&globals)
.section(&exports)
.section(&code);

module.finish()
}
}

#[derive(Arbitrary, Debug)]
#[derive(Arbitrary, Copy, Clone, Debug)]
pub(crate) enum TableOp {
// `call $gc; drop; drop; drop;`
Gc,

// `(drop (table.get x))`
Get(i32),

// `(drop (global.get i))`
GetGlobal(u32),

// `(table.set x (local.get y))`
SetFromParam(i32, u32),

// `(table.set x (table.get y))`
SetFromGet(i32, i32),

// `call $make_refs; table.set x; table.set y; table.set z`
SetFromMake(i32, i32, i32),

// `(global.set x (local.get y))`
SetGlobalFromParam(u32, u32),

// `(global.set x (table.get y))`
SetGlobalFromGet(u32, i32),

// `call $make_refs; global.set x; global.set y; global.set z`
SetGlobalFromMake(u32, u32, u32),

// `call $make_refs; drop; drop; drop;`
Make,

// `local.get x; local.get y; local.get z; call $take_refs`
TakeFromParams(u32, u32, u32),

// `table.get x; table.get y; table.get z; call $take_refs`
TakeFromGet(i32, i32, i32),

// `global.get x; global.get y; global.get z; call $take_refs`
TakeFromGlobalGet(u32, u32, u32),

// `call $make_refs; call $take_refs`
TakeFromMake,

// `call $gc; call $take_refs`
TakeFromGc,
}

impl TableOp {
fn insert(&self, func: &mut Function, num_params: u32, table_size: u32) {
fn insert(self, func: &mut Function, num_params: u32, table_size: u32, num_globals: u32) {
assert!(num_params > 0);
assert!(table_size > 0);

// Add one to make sure that out of bounds table accesses are possible,
// but still rare.
let table_mod = table_size as i32 + 1;

let gc_func_idx = 0;
let take_refs_func_idx = 1;
let make_refs_func_idx = 2;

match self {
Self::Gc => {
func.instruction(Instruction::Call(0));
func.instruction(Instruction::Call(gc_func_idx));
func.instruction(Instruction::Drop);
func.instruction(Instruction::Drop);
func.instruction(Instruction::Drop);
}
Self::Get(x) => {
func.instruction(Instruction::I32Const(*x % table_mod));
func.instruction(Instruction::I32Const(x % table_mod));
func.instruction(Instruction::TableGet { table: 0 });
func.instruction(Instruction::Drop);
}
Self::SetFromParam(x, y) => {
func.instruction(Instruction::I32Const(*x % table_mod));
func.instruction(Instruction::LocalGet(*y % num_params));
func.instruction(Instruction::I32Const(x % table_mod));
func.instruction(Instruction::LocalGet(y % num_params));
func.instruction(Instruction::TableSet { table: 0 });
}
Self::SetFromGet(x, y) => {
func.instruction(Instruction::I32Const(*x % table_mod));
func.instruction(Instruction::I32Const(*y % table_mod));
func.instruction(Instruction::I32Const(x % table_mod));
func.instruction(Instruction::I32Const(y % table_mod));
func.instruction(Instruction::TableGet { table: 0 });
func.instruction(Instruction::TableSet { table: 0 });
}
Self::SetFromMake(x, y, z) => {
func.instruction(Instruction::Call(2));
func.instruction(Instruction::Call(make_refs_func_idx));

func.instruction(Instruction::LocalSet(num_params));
func.instruction(Instruction::I32Const(*x % table_mod));
func.instruction(Instruction::I32Const(x % table_mod));
func.instruction(Instruction::LocalGet(num_params));
func.instruction(Instruction::TableSet { table: 0 });

func.instruction(Instruction::LocalSet(num_params));
func.instruction(Instruction::I32Const(*y % table_mod));
func.instruction(Instruction::I32Const(y % table_mod));
func.instruction(Instruction::LocalGet(num_params));
func.instruction(Instruction::TableSet { table: 0 });

func.instruction(Instruction::LocalSet(num_params));
func.instruction(Instruction::I32Const(*z % table_mod));
func.instruction(Instruction::I32Const(z % table_mod));
func.instruction(Instruction::LocalGet(num_params));
func.instruction(Instruction::TableSet { table: 0 });
}
TableOp::Make => {
func.instruction(Instruction::Call(2));
func.instruction(Instruction::Call(make_refs_func_idx));
func.instruction(Instruction::Drop);
func.instruction(Instruction::Drop);
func.instruction(Instruction::Drop);
Expand All @@ -216,27 +271,52 @@ impl TableOp {
func.instruction(Instruction::LocalGet(x % num_params));
func.instruction(Instruction::LocalGet(y % num_params));
func.instruction(Instruction::LocalGet(z % num_params));
func.instruction(Instruction::Call(1));
func.instruction(Instruction::Call(take_refs_func_idx));
}
TableOp::TakeFromGet(x, y, z) => {
func.instruction(Instruction::I32Const(*x % table_mod));
func.instruction(Instruction::I32Const(x % table_mod));
func.instruction(Instruction::TableGet { table: 0 });

func.instruction(Instruction::I32Const(*y % table_mod));
func.instruction(Instruction::I32Const(y % table_mod));
func.instruction(Instruction::TableGet { table: 0 });

func.instruction(Instruction::I32Const(*z % table_mod));
func.instruction(Instruction::I32Const(z % table_mod));
func.instruction(Instruction::TableGet { table: 0 });

func.instruction(Instruction::Call(1));
func.instruction(Instruction::Call(take_refs_func_idx));
}
TableOp::TakeFromMake => {
func.instruction(Instruction::Call(2));
func.instruction(Instruction::Call(1));
func.instruction(Instruction::Call(make_refs_func_idx));
func.instruction(Instruction::Call(take_refs_func_idx));
}
Self::TakeFromGc => {
func.instruction(Instruction::Call(0));
func.instruction(Instruction::Call(1));
func.instruction(Instruction::Call(gc_func_idx));
func.instruction(Instruction::Call(take_refs_func_idx));
}
TableOp::GetGlobal(x) => {
func.instruction(Instruction::GlobalGet(x % num_globals));
func.instruction(Instruction::Drop);
}
TableOp::SetGlobalFromParam(global, param) => {
func.instruction(Instruction::LocalGet(param % num_params));
func.instruction(Instruction::GlobalSet(global % num_globals));
}
TableOp::SetGlobalFromGet(global, x) => {
func.instruction(Instruction::I32Const(x));
func.instruction(Instruction::TableGet { table: 0 });
func.instruction(Instruction::GlobalSet(global % num_globals));
}
TableOp::SetGlobalFromMake(x, y, z) => {
func.instruction(Instruction::Call(make_refs_func_idx));
func.instruction(Instruction::GlobalSet(x % num_globals));
func.instruction(Instruction::GlobalSet(y % num_globals));
func.instruction(Instruction::GlobalSet(z % num_globals));
}
TableOp::TakeFromGlobalGet(x, y, z) => {
func.instruction(Instruction::GlobalGet(x % num_globals));
func.instruction(Instruction::GlobalGet(y % num_globals));
func.instruction(Instruction::GlobalGet(z % num_globals));
func.instruction(Instruction::Call(take_refs_func_idx));
}
}
}
Expand All @@ -250,6 +330,7 @@ mod tests {
fn test_wat_string() {
let ops = TableOps {
num_params: 5,
num_globals: 1,
table_size: 20,
ops: vec![
TableOp::Gc,
Expand All @@ -261,6 +342,11 @@ mod tests {
TableOp::TakeFromParams(8, 9, 10),
TableOp::TakeFromGet(11, 12, 13),
TableOp::TakeFromMake,
TableOp::GetGlobal(14),
TableOp::SetGlobalFromParam(15, 16),
TableOp::SetGlobalFromGet(17, 18),
TableOp::SetGlobalFromMake(19, 20, 21),
TableOp::TakeFromGlobalGet(22, 23, 24),
],
};

Expand Down Expand Up @@ -320,9 +406,25 @@ mod tests {
call 1
call 2
call 1
global.get 0
drop
local.get 1
global.set 0
i32.const 18
table.get 0
global.set 0
call 2
global.set 0
global.set 0
global.set 0
global.get 0
global.get 0
global.get 0
call 1
br 0 (;@1;)
end)
(table (;0;) 20 externref)
(global (;0;) (mut externref) (ref.null extern))
(export "run" (func 3)))
"#;
eprintln!("expected WAT = {}", expected);
Expand Down
3 changes: 3 additions & 0 deletions crates/fuzzing/src/oracles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,9 @@ pub fn table_ops(mut fuzz_config: generators::Config, ops: generators::table_ops
.map(|_| Val::ExternRef(Some(ExternRef::new(CountDrops(num_dropped.clone())))))
.collect();
let _ = run.call(&mut store, &args, &mut []);

// Do a final GC after running the Wasm.
store.gc();
}

assert_eq!(num_dropped.load(SeqCst), expected_drops.load(SeqCst));
Expand Down
6 changes: 4 additions & 2 deletions crates/runtime/src/externref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ impl Deref for VMExternRef {
///
/// We use this so that we can morally put `VMExternRef`s inside of `HashSet`s
/// even though they don't implement `Eq` and `Hash` to avoid foot guns.
#[derive(Clone)]
#[derive(Clone, Debug)]
struct VMExternRefWithTraits(VMExternRef);

impl Hash for VMExternRefWithTraits {
Expand Down Expand Up @@ -940,7 +940,9 @@ pub unsafe fn gc(
debug_assert!(
r.is_null() || activations_table_set.contains(&r),
"every on-stack externref inside a Wasm frame should \
have an entry in the VMExternRefActivationsTable"
have an entry in the VMExternRefActivationsTable; \
{:?} is not in the table",
r
);
if let Some(r) = NonNull::new(r) {
VMExternRefActivationsTable::insert_precise_stack_root(
Expand Down
10 changes: 10 additions & 0 deletions crates/runtime/src/libcalls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,16 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc(
let externref = VMExternRef::clone_from_raw(externref);
let instance = (*vmctx).instance();
let (activations_table, module_info_lookup) = (*instance.store()).externref_activations_table();

// Invariant: all `externref`s on the stack have an entry in the activations
// table. So we need to ensure that this `externref` is in the table
// *before* we GC, even though `insert_with_gc` will ensure that it is in
// the table *after* the GC. This technically results in one more hash table
// look up than is strictly necessary -- which we could avoid by having an
// additional GC method that is aware of these GC-triggering references --
// but it isn't really a concern because this is already a slow path.
activations_table.insert_without_gc(externref.clone());
Copy link
Member

Choose a reason for hiding this comment

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

This can end up doing a double-insertion between this and insert_with_gc below, and it might be good to avoid that? This method is only called when the table is full so insert_without_gc is already guaranteed to go straight to insertion in the slow path, so should a specialized method of the table be added for basically this use case?

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 won't insert into the bump chunk, so I don't think there is anything to worry about here. We just do one more hash lookup than is strictly necessary, but that is fine as this is a slow path anyways.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, yeah. Could you update the comment here to indicate that this is doing some extra work but it's the slow path so it should be ok?


activations_table.insert_with_gc(externref, module_info_lookup);
}

Expand Down