Allow merging modules and skipping DWARF reading on load #257
Conversation
There was a problem hiding this comment.
Thanks for sharing. I'm open to landing the feature, but am not convinced we have covered all the necessary index changes.
I think to land we would absolutely need tests which have an existing module consisting of an import and a local of all core types, to which we then add new imports and locals of all core types. That would be the minimum I think to convince me this works comprehensively, especially given I don't have a completely thorough understanding of the codebase myself.
Completely understood if it's too much work.
Also, if you wanted to split out the skip_debug_section flag as its own PR that might also be easier to land independently.
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for making the changes. This PR still adds a lot of public functions:
- ModuleConfig::read_dwarf
- ModuleConfig::skip_dwarf
- ModuleConfig::parse_into
- Module::merge_buffer
- Module::parse_in
Firstly, why does this configuration option need to have a function variant for both turning it on and off? We don't seem to do this for any other configuration options.
Then secondly, for the main method - can we not just design this to be a single new function? It would be much more preferable than introducing three new functions.
Finally this can only land with tests, so we will of course need to add that too.
|
By the way, did WABT change its syntax again? |
|
CI should be passing here again now. |
I added this because, as far as I know, no other Rust WASM library can easily do this. I also need this for a project.