Add memory import support

[bnason-nf]

This adds support to the public export API and internal implementation for import memory objects. The exception to this is that the AOT compiler and loader are currently hardcoded not to support import memories, so that will need to be fixed in a future change.

Please let me know if there's anything about the approach used here that should be improved or re-done.

[lum1n0us]

The WASM_ENABLE_MULTI_MODULE setting is currently restricted to importing only globals and functions. We prefer to keep it this way unless there are explicit requirements for change.

We're in the process of developing instantiation linking (as opposed to MULTI_MODULE, which is loader linking). This new approach will encompass all four elements: global variables, functions, memory, and tables. Therefore, it seems more appropriate to incorporate "memory import support" into the new linking framework rather than considering it a separate feature.

In this series of PRs, I believe we can complete APIs to obtain import types and export items.

[bnason-nf]

Hi @lum1n0us,

This change is not intended to have any special support for multiple modules, it's intended to add support for import memories in general. Would you mind clarifying what you saw in the change that might not fit with that?

Thanks,
Benbuck

[bnason-nf]

To be clear, I fully agree with you adding support for importing globals, functions, memories, and tables, and linking them at instantiation time. This change is intended to be the first piece of that, with others to come later, please see our previous brief discussion here:

#3786 (comment)

[lum1n0us]

Apologies for the confusion. Yes, this pull request is about how to import memory, which is also a part of the linking process. Multi-module is a type of linking technology, referred to as loading linking. This is different from the instantiation linking defined by the Spec., which WAMR has not yet supported. I have opened a new issue to track the implementation of the linking as defined by the Spec

[bnason-nf]

Hi @lum1n0us,

I believe that this PR does implement instantiation linking for memory objects. For example in wasm_export.h, the new WASMImportInst imports member was added to InstantiationArgs. Would you mind explaining please what you see that doesn't meet that criteria, or what I'm misunderstanding?

Thanks,
Benbuck

[lum1n0us]

I believe that this PR does implement instantiation linking for memory objects. For example in wasm_export.h, the new WASMImportInst imports member was added to InstantiationArgs.

You are absolutely right. I am just hoping that we can design the entire instantiation linking process as much as we can, which includes not only memory linking but also other aspects, to ensure it fully aligns with the Spec. definition.

Some of this work has already been completed in your previous pull requests. Thank you, by the way.

[bnason-nf]

Yes we're in full agreement on wanting to fully support the specification for importing all types. I tried to plan for that by building scaffolding in the WASMImportInst structure:

    wasm_import_export_kind_t kind;
    union {
        wasm_memory_inst_t memory_inst;
    } u;

Which should allow us to easily add other types. But if you don't like this approach in general, that's fine, I'm just trying to understand what design choices you think would be better.

[lum1n0us]

I've created an issue to gather ideas about instantiation linking. Please leave your comments and thoughts there.

[lum1n0us]

About WASMImportInst:

- Include other types of instances in the union, such as function, global, table, and tag.

• The name WASMImportInst might need to be changed. My initial thought is WASMExternal, similar to wasm_c_api.

It looks like we can make a simple, workable version of the instantiation linking idea using this PR to test it out. Also, importing a memory is the toughest part of putting our linking idea into action. It's a good place to start.

Let me know if everyone is okay with issue #3807 and if nothing is left out. After that, I'll share my thoughts on this PR.

[wenyongh]

About WASMImportInst:

- Include other types of instances in the union, such as function, global, table, and tag.

• The name WASMImportInst might need to be changed. My initial thought is WASMExternal, similar to wasm_c_api.

Using extern seems good but WASMExternal makes me a little confused with wasm-c-api's structures. Since now there are type wasm_memory_inst_t, wasm_function_inst_t, wasm_global_inst_t and wasm_table_inst_t, how about using WASMExternInst or WASMExternInstance? And then typedef WASMExternInst *wasm_extern_inst_t.

[wenyongh]

BTW, I think the PR is to allow the developer to create the import memory instance by calling wasm_runtime_memory_create, then pass it with WASMImportInst array to wasm_runtime_instantiate, and then during the instantiation, runtime uses this developer-created import memory directly, instead of creating it. Seems that it is a good way, similar to wasm-c-api, we also pass the imports with wasm_extern_vec_t * to wasm_instance_new.

An issue is about the WASMMemoryWrapper structure, I think we had better not add this structure and change WASMModuleInstance's field WASMMemoryInstance ** memories to WASMMemoryWrapper * memories, since (1) the AOT code accesses this WASMMemoryInstance ** field with fixed layout, changing it may cause incompatibility and it is also complex to change the AOT compiler to access the new field, (2) it wastes memory since only import memory needs the field, and in fact only it is needed when the import memory is created by developer.

My suggestion is to create a name to memory inst map list in the wasm/aot module instance, somewhat like:

struct WASMImportMemoryMap {
    struct WASMImportMemoryMap *next;
    char *module_name;
    char *field_name;
    WASMMemoryInstance *memory_inst;
};

[bnason-nf]

Hi @wenyongh. Yeah, I also wasn't sure the memory wrapper approach would be the best choice, so I'm not surprised to hear you don't love it either. I could try to re-do this PR with a new approach if we want, but I'm not sure if that's worth doing?

[wenyongh]

Hi @wenyongh. Yeah, I also wasn't sure the memory wrapper approach would be the best choice, so I'm not surprised to hear you don't love it either. I could try to re-do this PR with a new approach if we want, but I'm not sure if that's worth doing?

It is up to you, but I think it helps, at least it is a good reference for the future instantiation linking implementation. And if you want to do it, could you modify WASMImportInst to WASMExternInst or WASMExternInstance?

[lum1n0us]

Plus, IIUC, we don't need WASMMemoryWrapper to package host created WASMMemoryInstance and runtime created WASMMemoryInstance. Both should be same thing.

That's why I also thought we should not have another implementation in wasm_runtime_memory_create(). It should use memory_instantiation() in some way.

[lum1n0us]

@bnason-nf please take a review of #3845, #3887, #3893. Or just #3893. Let me know your comments.