Merge pull request #58 from fitzgen/tombstone-arena

Tombstone arena

Cargo.toml

@@ -15,7 +15,7 @@ A library for performing WebAssembly transformations
 
 [dependencies]
 failure = "0.1.2"
-id-arena = { version = "2.2.0", features = ['rayon'] }
+id-arena = { version = "2.2.1", features = ['rayon'] }
 leb128 = "0.2.3"
 log = "0.4"
 rayon = "1.0.3"

crates/tests-utils/src/lib.rs

@@ -3,7 +3,11 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 use std::sync::{Once, ONCE_INIT};
 
-pub const FEATURES: &[&str] = &["--enable-threads", "--enable-bulk-memory", "--enable-reference-types"];
+pub const FEATURES: &[&str] = &[
+    "--enable-threads",
+    "--enable-bulk-memory",
+    "--enable-reference-types",
+];
 
 fn require_wat2wasm() {
     let status = Command::new("wat2wasm")

crates/tests/tests/spec-tests.rs

@@ -32,7 +32,6 @@ fn run(wast: &Path) -> Result<(), failure::Error> {
         // TODO: should get threads working
         Some("threads") => return Ok(()),
         // Some("threads") => &["--enable-threads"],
-
         Some(other) => panic!("unknown wasm proposal: {}", other),
     };
 
@@ -84,7 +83,8 @@ fn run(wast: &Path) -> Result<(), failure::Error> {
             }
             cmd => {
                 let wasm = fs::read(&path)?;
-                let mut wasm = config.parse(&wasm)
+                let mut wasm = config
+                    .parse(&wasm)
                     .context(format!("error parsing wasm (line {})", line))?;
 
                 // If a module is supposed to be unlinkable we'll often gc out
@@ -113,7 +113,8 @@ fn run(wast: &Path) -> Result<(), failure::Error> {
                     .emit_wasm()
                     .context(format!("error emitting wasm (line {})", line))?;
                 fs::write(&path, &wasm1)?;
-                let wasm2 = config.parse(&wasm1)
+                let wasm2 = config
+                    .parse(&wasm1)
                     .and_then(|m| m.emit_wasm())
                     .context(format!("error re-parsing wasm (line {})", line))?;
                 if wasm1 != wasm2 {

src/function_builder.rs

@@ -1,14 +1,14 @@
 use crate::ir::*;
+use crate::tombstone_arena::TombstoneArena;
 use crate::{FunctionId, LocalFunction, Module, TypeId, ValType};
-use crate::{ModuleTypes, ModuleFunctions};
-use id_arena::Arena;
+use crate::{ModuleFunctions, ModuleTypes};
 use std::mem;
 use std::ops::{Deref, DerefMut, Drop};
 
 /// A helpful struct used for building instances of `LocalFunction`
 #[derive(Default, Debug)]
 pub struct FunctionBuilder {
-    pub(crate) arena: Arena<Expr>,
+    pub(crate) arena: TombstoneArena<Expr>,
 }
 
 impl FunctionBuilder {

src/ir/mod.rs

@@ -526,8 +526,7 @@ pub enum Expr {
     },
 
     /// ref.null
-    RefNull {
-    },
+    RefNull {},
 
     /// ref.is_null
     RefIsNull {

src/lib.rs

@@ -15,10 +15,11 @@ mod map;
 mod module;
 mod parse;
 pub mod passes;
+mod tombstone_arena;
 mod ty;
 
 pub use crate::error::{ErrorKind, Result};
-pub use crate::function_builder::{FunctionBuilder, BlockBuilder};
+pub use crate::function_builder::{BlockBuilder, FunctionBuilder};
 pub use crate::init_expr::InitExpr;
 pub use crate::ir::{Local, LocalId};
 pub use crate::module::*;

src/module/data.rs

@@ -4,9 +4,9 @@ use crate::emit::{Emit, EmitContext, Section};
 use crate::ir::Value;
 use crate::parse::IndicesToIds;
 use crate::passes::Used;
+use crate::tombstone_arena::{Id, Tombstone, TombstoneArena};
 use crate::{InitExpr, Module, Result, ValType};
 use failure::{bail, ResultExt};
-use id_arena::{Arena, Id};
 
 /// A passive element segment identifier
 pub type DataId = Id<Data>;
@@ -29,6 +29,12 @@ pub struct Data {
     pub value: Vec<u8>,
 }
 
+impl Tombstone for Data {
+    fn on_delete(&mut self) {
+        self.value = Vec::new();
+    }
+}
+
 impl Data {
     /// Returns the id of this passive data segment
     pub fn id(&self) -> DataId {
@@ -40,7 +46,7 @@ impl Data {
 /// various instructions.
 #[derive(Debug, Default)]
 pub struct ModuleData {
-    arena: Arena<Data>,
+    arena: TombstoneArena<Data>,
 }
 
 impl ModuleData {
@@ -54,18 +60,25 @@ impl ModuleData {
         &mut self.arena[id]
     }
 
+    /// Delete a passive data segment from this module.
+    ///
+    /// It is up to you to ensure that all references to the deleted segment are
+    /// removed, eg `memory.init` and `data.drop` expressions.
+    pub fn delete(&mut self, id: DataId) {
+        self.arena.delete(id);
+    }
+
     /// Get a shared reference to this module's passive elements.
     pub fn iter(&self) -> impl Iterator<Item = &Data> {
         self.arena.iter().map(|(_, f)| f)
     }
 
     /// Adds a new passive data segment with the specified contents
     pub fn add(&mut self, value: Vec<u8>) -> DataId {
-        let id = self.arena.next_id();
-        self.arena.alloc(Data {
-            passive: true,
-            value,
+        self.arena.alloc_with_id(|id| Data {
             id,
+            value,
+            passive: true,
         })
     }
 
@@ -110,13 +123,11 @@ impl Module {
     /// validation.
     pub(crate) fn reserve_data(&mut self, count: u32, ids: &mut IndicesToIds) {
         for _ in 0..count {
-            let id = self.data.arena.next_id();
-            self.data.arena.alloc(Data {
+            ids.push_data(self.data.arena.alloc_with_id(|id| Data {
                 id,
                 passive: false, // this'll get set to `true` when parsing data
                 value: Vec::new(),
-            });
-            ids.push_data(id);
+            }));
         }
     }
 

src/module/elements.rs

@@ -3,9 +3,9 @@
 use crate::emit::{Emit, EmitContext, Section};
 use crate::ir::Value;
 use crate::parse::IndicesToIds;
+use crate::tombstone_arena::{Id, Tombstone, TombstoneArena};
 use crate::{FunctionId, InitExpr, Module, Result, TableKind, ValType};
 use failure::{bail, ResultExt};
-use id_arena::{Arena, Id};
 
 /// A passive element segment identifier
 pub type ElementId = Id<Element>;
@@ -16,11 +16,17 @@ pub struct Element {
     members: Vec<FunctionId>,
 }
 
+impl Tombstone for Element {
+    fn on_delete(&mut self) {
+        self.members = Vec::new();
+    }
+}
+
 /// All element segments of a wasm module, used to initialize `anyfunc` tables,
 /// used as function pointers.
 #[derive(Debug, Default)]
 pub struct ModuleElements {
-    arena: Arena<Element>,
+    arena: TombstoneArena<Element>,
 }
 
 impl ModuleElements {
@@ -34,6 +40,14 @@ impl ModuleElements {
         &mut self.arena[id]
     }
 
+    /// Delete an elements entry from this module.
+    ///
+    /// It is up to you to ensure that all references to this deleted element
+    /// are removed.
+    pub fn delete(&mut self, id: ElementId) {
+        self.arena.delete(id);
+    }
+
     /// Get a shared reference to this module's passive elements.
     pub fn iter(&self) -> impl Iterator<Item = &Element> {
         self.arena.iter().map(|(_, f)| f)

src/module/exports.rs

@@ -1,10 +1,9 @@
 //! Exported items in a wasm module.
 
 use crate::emit::{Emit, EmitContext, Section};
-use crate::map::IdHashSet;
 use crate::parse::IndicesToIds;
+use crate::tombstone_arena::{Id, Tombstone, TombstoneArena};
 use crate::{FunctionId, GlobalId, MemoryId, Module, Result, TableId};
-use id_arena::{Arena, Id};
 
 /// The id of an export.
 pub type ExportId = Id<Export>;
@@ -19,6 +18,12 @@ pub struct Export {
     pub item: ExportItem,
 }
 
+impl Tombstone for Export {
+    fn on_delete(&mut self) {
+        self.name = String::new();
+    }
+}
+
 impl Export {
     /// Returns the id of this export
     pub fn id(&self) -> ExportId {
@@ -43,8 +48,7 @@ pub enum ExportItem {
 #[derive(Debug, Default)]
 pub struct ModuleExports {
     /// The arena containing this module's exports.
-    arena: Arena<Export>,
-    blacklist: IdHashSet<Export>,
+    arena: TombstoneArena<Export>,
 }
 
 impl ModuleExports {
@@ -58,6 +62,11 @@ impl ModuleExports {
         &mut self.arena[id]
     }
 
+    /// Delete an export entry from this module.
+    pub fn delete(&mut self, id: ExportId) {
+        self.arena.delete(id);
+    }
+
     /// Get a shared reference to this module's exports.
     pub fn iter(&self) -> impl Iterator<Item = &Export> {
         self.arena.iter().map(|(_, f)| f)
@@ -72,18 +81,10 @@ impl ModuleExports {
         })
     }
 
-    /// Get a shared reference to this module's root export
-    ///
-    /// Exports are "root exports" by default, unless they're blacklisted with
-    /// the `remove_root` method.
-    pub fn iter_roots(&self) -> impl Iterator<Item = &Export> {
-        self.iter().filter(move |e| !self.blacklist.contains(&e.id))
-    }
-
-    /// Removes an `id` from the set of "root exports" returned from
-    /// `iter_roots`.
+    #[doc(hidden)]
+    #[deprecated(note = "Use `ModuleExports::delete` instead")]
     pub fn remove_root(&mut self, id: ExportId) {
-        self.blacklist.insert(id);
+        self.delete(id);
     }
 }
 
@@ -105,8 +106,7 @@ impl Module {
                 Memory => ExportItem::Memory(ids.get_memory(entry.index)?),
                 Global => ExportItem::Global(ids.get_global(entry.index)?),
             };
-            let id = self.exports.arena.next_id();
-            self.exports.arena.alloc(Export {
+            self.exports.arena.alloc_with_id(|id| Export {
                 id,
                 name: entry.field.to_string(),
                 item,
@@ -122,15 +122,15 @@ impl Emit for ModuleExports {
         // NB: exports are always considered used. They are the roots that the
         // used analysis searches out from.
 
-        let count = self.iter_roots().count();
+        let count = self.iter().count();
         if count == 0 {
             return;
         }
 
         let mut cx = cx.start_section(Section::Export);
         cx.encoder.usize(count);
 
-        for export in self.iter_roots() {
+        for export in self.iter() {
             cx.encoder.str(&export.name);
             match export.item {
                 ExportItem::Function(id) => {

src/module/functions/local_function/context.rs

@@ -227,9 +227,7 @@ impl<'a> FunctionContext<'a> {
 
     pub fn add_side_effect(&mut self, value: ExprId, side_effect: ExprId) -> ExprId {
         // If we can add it to an existing `WithSideEffects` expr for this value, then do that.
-        if let Expr::WithSideEffects(WithSideEffects { after, .. }) =
-            self.func.get_mut(value)
-        {
+        if let Expr::WithSideEffects(WithSideEffects { after, .. }) = self.func.get_mut(value) {
             after.push(side_effect);
             return value;
         }

src/module/functions/local_function/mod.rs

@@ -13,7 +13,7 @@ use crate::ir::*;
 use crate::map::{IdHashMap, IdHashSet};
 use crate::parse::IndicesToIds;
 use crate::passes::Used;
-use crate::{FunctionId, Module, Result, TypeId, ValType, FunctionBuilder, TableKind};
+use crate::{FunctionBuilder, FunctionId, Module, Result, TableKind, TypeId, ValType};
 use failure::{bail, ResultExt};
 use id_arena::Id;
 use std::collections::BTreeMap;
@@ -1245,7 +1245,11 @@ fn validate_instruction(ctx: &mut FunctionContext, inst: Operator) -> Result<()>
             };
             let (_, value) = ctx.pop_operand_expected(Some(expected_ty))?;
             let (_, index) = ctx.pop_operand_expected(Some(I32))?;
-            let expr = ctx.func.alloc(TableSet { table, index, value });
+            let expr = ctx.func.alloc(TableSet {
+                table,
+                index,
+                value,
+            });
             ctx.add_to_current_frame_block(expr);
         }
         Operator::TableGrow { table } => {
@@ -1256,7 +1260,11 @@ fn validate_instruction(ctx: &mut FunctionContext, inst: Operator) -> Result<()>
             };
             let (_, value) = ctx.pop_operand_expected(Some(expected_ty))?;
             let (_, amount) = ctx.pop_operand_expected(Some(I32))?;
-            let expr = ctx.func.alloc(TableGrow { table, amount, value });
+            let expr = ctx.func.alloc(TableGrow {
+                table,
+                amount,
+                value,
+            });
             ctx.push_operand(Some(I32), expr);
         }
         Operator::TableSize { table } => {
@@ -1265,7 +1273,7 @@ fn validate_instruction(ctx: &mut FunctionContext, inst: Operator) -> Result<()>
             ctx.push_operand(Some(I32), expr);
         }
         Operator::RefNull => {
-            let expr = ctx.func.alloc(RefNull { });
+            let expr = ctx.func.alloc(RefNull {});
             ctx.push_operand(Some(Anyref), expr);
         }
         Operator::RefIsNull => {