Upstreaming fns from walrus_ops to walrus

[vados-cosmonic]

Hey @guybedford thanks for this awesome project.

I'd want to make use of the code around injecting data and bumping the stack appropriately in a project I'm working on (basically what the project does is modify data segments in an existing wasm module dynamically) and while copying it (with attribution of course) is cool, importing it is even cooler.

To that end, I was wondering if some of the walrus_ops stuff could/should be upstreamed to walrus (or maybe some sort of walrus-extras crate)?

[EDIT] to be clear -- I'd be willing to do the work here, assuming we can figure out a way you'd like to do it!

[guybedford]

@vados-cosmonic thanks for looking into it. I'm open to upstreaming methods that make sense into Walrus, although we need to be careful to ensure they are as well-defined as possible. That might be a little tricky since the stack bumping technique is based on a lot of assumptions about the shape of the Wasm binary, and to formalize it requires formalizing those assumptions as well. That is, I don't think this is something that can just be copying and pasting the code unfortunately. So not to say no, but to say we need to have some clear guidelines for the process.

[vados-cosmonic]

Hey thanks for the feedback, this makes sense... So I'll have to be really careful about what's worth even trying to take (if we can find anything that works at all).

How do you feel about the following, they seem somewhat general:

fn name	notes
get_memory_id	Maybe renamed to something like get_first_memory_id(). I could see this being much less important as multi-memory stuff lands/gains usage
get_exported_func	Maybe renamed to get_exported_func_by_name?
add_stub_exported_func	Maybe renamed to swap_exported_function? I could see this generalized to take a Fn that builds the function body? Or taking the Function as a whole?
stub_imported_func	Same as above
remove_exported_func	This could be useful in the general case even though it's very simple... definitely more of a walrus-extras kind of function... Also adding remove_imported_func may make sense of course.

Basically I've included everything but the bumping stuff... But if we know that the bumping work for core modules, preview1 and preview2 (?) then maybe it's fine to just check for only those cases and reject all other cases to be overly cautious?

Really feeling like a walrus-extras makes sense for some of this functionality.

[guybedford]

This is a really good list, thanks for thinking about what makes the most general sense here.

get_memory_id and get_exported_func both seem absolutely fine and without concern for upstreaming.

Then I wonder if we could replace both add_stub_exported_func and stub_imported_func with a singular replace_function_by_id? One that could take eg some type of builder as input say as well so the stubbing aspect becomes just the null case of that?

Having both ModuleFunctions.remove for both imports and exports also seems like a more general pattern to me as well.

[vados-cosmonic]

Hey @guybedford I've got a little draft PR up if you'd like to take a look -- rustwasm/walrus#248 .

Mostly what I'm wondering about is how we can differentiate in the more general functions between imports and exports... I'd like to use/overload FunctionKind but it doesn't have Export as a variant (and that seems like a much bigger change, since all we're trying to convey is location).

[vados-cosmonic]

Changes have been upstreamed to walrus and almost made their way completely in the codebase (#24 ), closing this!