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

Allow merging modules and skipping DWARF reading on load #257

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 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
15 changes: 15 additions & 0 deletions src/module/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub struct ModuleConfig {
pub(crate) on_parse:
Option<Box<dyn Fn(&mut Module, &IndicesToIds) -> Result<()> + Sync + Send + 'static>>,
pub(crate) on_instr_loc: Option<Box<dyn Fn(&usize) -> InstrLocId + Sync + Send + 'static>>,
pub(crate) no_read_dwarf: bool,
}

impl Clone for ModuleConfig {
Expand All @@ -32,6 +33,7 @@ impl Clone for ModuleConfig {
skip_producers_section: self.skip_producers_section,
skip_name_section: self.skip_name_section,
preserve_code_transform: self.preserve_code_transform,
no_read_dwarf: self.no_read_dwarf,

// ... and this is left empty.
on_parse: None,
Expand All @@ -54,6 +56,7 @@ impl fmt::Debug for ModuleConfig {
ref preserve_code_transform,
ref on_parse,
ref on_instr_loc,
ref no_read_dwarf,
} = self;

f.debug_struct("ModuleConfig")
Expand All @@ -69,6 +72,7 @@ impl fmt::Debug for ModuleConfig {
.field("preserve_code_transform", preserve_code_transform)
.field("on_parse", &on_parse.as_ref().map(|_| ".."))
.field("on_instr_loc", &on_instr_loc.as_ref().map(|_| ".."))
.field("no_read_dwarf", no_read_dwarf)
guybedford marked this conversation as resolved.
Show resolved Hide resolved
.finish()
}
}
Expand All @@ -79,6 +83,11 @@ impl ModuleConfig {
ModuleConfig::default()
}

/// Sets a flag to whether DWARF debug sections are read for this module.
pub fn read_dwarf(&mut self, dw: bool) -> &mut ModuleConfig {
self.no_read_dwarf = !dw;
return self;
}
/// Sets a flag to whether DWARF debug sections are generated for this
/// module.
///
Expand Down Expand Up @@ -208,6 +217,12 @@ impl ModuleConfig {
Module::parse(wasm, self)
}

/// Parses an in-memory WebAssembly file into an existing `Module` using this
/// configuration.
pub fn parse_into(&self, wasm: &[u8], pre: &mut Module) -> Result<()> {
Module::parse_in(wasm, self, pre)
}

/// Parses a WebAssembly file into a `Module` using this configuration.
pub fn parse_file<P>(&self, path: P) -> Result<Module>
where
Expand Down
2 changes: 1 addition & 1 deletion src/module/functions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ impl Module {
// necessary and it's a bottleneck!
let mut bodies = Vec::with_capacity(functions.len());
for (i, (body, mut validator)) in functions.into_iter().enumerate() {
let index = (num_imports + i) as u32;
let index = (indices.num_fun_imports + i) as u32;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be curious to know the reason this is necessary - which is incorrect above - funcs.arena.len() or functions.len()? I'm also not particularly confident this covers all index changes necessary to support the feature, if you do want to land this it would help to provide a little more analysis / feedback on why you think this is enough to make this PR complete.

Copy link
Author

Choose a reason for hiding this comment

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

I did this because functions.len() is the length of the current module's functions, which includes the total amount of imports being merged and functions previously in the module. Previously, functions.len() worked because functions started out empty, but with merging, functions can contain values before imports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying. Are there any other cases where imports being added later might affect indexing, or do you feel pretty confident this is the only one?

let id = indices.get_func(index)?;
let ty = match self.funcs.arena[id].kind {
FunctionKind::Uninitialized(ty) => ty,
Expand Down
4 changes: 4 additions & 0 deletions src/module/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ impl Module {
entry.field.expect("module linking not supported"),
ty,
);
ids.num_fun_imports += 1;
ids.push_func(id.0);
}
wasmparser::ImportSectionEntryType::Table(t) => {
Expand All @@ -171,6 +172,7 @@ impl Module {
t.maximum,
ty,
);
ids.num_table_imports += 1;
ids.push_table(id.0);
}
wasmparser::ImportSectionEntryType::Memory(m) => {
Expand All @@ -184,6 +186,7 @@ impl Module {
m.initial as u32,
m.maximum.map(|m| m as u32),
);
ids.num_mem_imports += 1;
ids.push_memory(id.0);
}
wasmparser::ImportSectionEntryType::Global(g) => {
Expand All @@ -193,6 +196,7 @@ impl Module {
ValType::parse(&g.content_type)?,
g.mutable,
);
ids.num_global_imports += 1;
ids.push_global(id.0);
}
wasmparser::ImportSectionEntryType::Module(_)
Expand Down
37 changes: 31 additions & 6 deletions src/module/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub use crate::module::producers::ModuleProducers;
pub use crate::module::tables::{ModuleTables, Table, TableId};
pub use crate::module::types::ModuleTypes;
use crate::parse::IndicesToIds;
use crate::FunctionBuilder;
use anyhow::{bail, Context};
use id_arena::Id;
use log::warn;
Expand Down Expand Up @@ -128,9 +129,21 @@ impl Module {
ModuleConfig::new().parse(wasm)
}

/// Merge a module from the in-memory wasm buffer with the default configuration
pub fn merge_buffer(&mut self, wasm: &[u8]) -> Result<()> {
return Self::parse_in(wasm, &ModuleConfig::default(), self);
}

fn parse(wasm: &[u8], config: &ModuleConfig) -> Result<Module> {
let mut ret = Module::default();
ret.config = config.clone();
Self::parse_in(wasm, config, &mut ret)?;
return Ok(ret);
}
fn parse_in(wasm: &[u8], config: &ModuleConfig, ret: &mut Module) -> Result<()> {
guybedford marked this conversation as resolved.
Show resolved Hide resolved
// let mut ret = Module::default();
// ret.config = config.clone();
let old_start = ret.start.take();
let mut indices = IndicesToIds::default();
let mut validator = Validator::new();
validator.wasm_features(WasmFeatures {
Expand Down Expand Up @@ -299,19 +312,31 @@ impl Module {
config.on_instr_loc.as_ref().map(|f| f.as_ref()),
)
.context("failed to parse code section")?;

ret.parse_debug_sections(debug_sections)
.context("failed to parse debug data section")?;

if !config.no_read_dwarf {
ret.parse_debug_sections(debug_sections)
.context("failed to parse debug data section")?;
}
ret.producers
.add_processed_by("walrus", env!("CARGO_PKG_VERSION"));

match ret.start {
Some(a) => {
if let Some(r) = old_start {
let mut new_builder = FunctionBuilder::new(&mut ret.types, &[], &[]);
new_builder.func_body().call(r).call(a).return_();
let new = new_builder.local_func(vec![]);
let new = ret.funcs.add_local(new);
ret.start = Some(new);
}
}
None => ret.start = old_start,
}
if let Some(on_parse) = &config.on_parse {
on_parse(&mut ret, &indices)?;
on_parse(ret, &indices)?;
}

log::debug!("parse complete");
Ok(ret)
Ok(())
}

/// Emit this module into a `.wasm` file at the given path.
Expand Down
4 changes: 4 additions & 0 deletions src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ pub struct IndicesToIds {
elements: Vec<ElementId>,
data: Vec<DataId>,
locals: IdHashMap<Function, Vec<LocalId>>,
pub(crate) num_fun_imports: usize,
pub(crate) num_mem_imports: usize,
pub(crate) num_global_imports: usize,
pub(crate) num_table_imports: usize,
}

macro_rules! define_push_get {
Expand Down
Loading