Allow loading of missing imports without errors when multi-module is enabled #3539

XeniaLu · 2024-06-18T00:53:03Z

The INTERP module loader was failing when multi-module support is on
and dependant modules are not found; this enforced AOT compiler
integrations to prepare dependant modules while it wasn't necessary.

This new option allows integrated AOT compilers to work as if the
multi-module support wasn't turned on.

This option is also turned on in the AOT compiler CLI, just in case
if WASM_ENABLE_MULTI_MODULE is turned on there one day.

WAMRC can generate AOT files even if the main module has
dependencies that are not found.
Expanded check_linked_symbol() to include checks for memories,
tables and tags during the instantiation phase to ensure error raises

lum1n0us · 2024-06-18T03:27:37Z

So, you're hoping a multi-aot feature which means linking happens when aot loader loading files. But current status is WAMRC requires all dependencies. I agree that your requirement is more nature. And even more it should be the default behaviour. Not an option.

Plus, Since I am not quite familiar with current implementation, may I ask why WAMRC needs to enable "multi-module" feature? In my imagination, people should use wamrc(w/o multi-module feature) to generate AOTs one by one for main module and its dependencies. And AOT loader(w/ multi-module feature) could load them like a ld.linux.

XeniaLu · 2024-06-19T02:42:54Z

So, you're hoping a multi-aot feature which means linking happens when aot loader loading files. But current status is WAMRC requires all dependencies. I agree that your requirement is more nature. And even more it should be the default behaviour. Not an option.

I have removed the option as you suggested and made it the default behavior.

Plus, Since I am not quite familiar with current implementation, may I ask why WAMRC needs to enable "multi-module" feature? In my imagination, people should use wamrc(w/o multi-module feature) to generate AOTs one by one for main module and its dependencies. And AOT loader(w/ multi-module feature) could load them like a ld.linux.

Yes, we don't need to enable "multi-module" for WAMRC. Actually we have integrated the compiler code directly into the same package as the runtime, our compiler and runtime share the same macro control. So we have to ensure that the compiler does not encounter failures when the "multi-module" feature is enabled.

lum1n0us · 2024-06-19T05:08:34Z

Yes, we don't need to enable "multi-module" for WAMRC

If so, original wasm_loader will not cause any trouble in your project. The question will be, why change? Because all modification disable original show error and quit action when couldn't matching import requirements

BTW, you might want to change the PR title. Now the PR is about something else.

lum1n0us · 2024-06-19T05:29:02Z

thanks for @wenyongh. I kindly understand the situation here. The project needs to -DWAMR_BUILD_MULTI_MODULE=1 when compiling WAMRC but actually don't want show error and quit, which is in #if WASM_ENABLE_MULTI_MODULE !=0 ... #endif block, to bother WAMRC compilation. Please correct me if I am wrong.

I guess it becomes another problem: under other non-aot running mode, like interpreter and jit, when enabling MULTI_MODULE feature, what kind of behavior the loader should be if can't match any import requirement?

There is be an instantiation check check_linked_symbol() in wasm_runtime.c. It could be our insurance. But it only cover function and global. Please add check for memory, table and tag. #if WASM_ENABLE_WAMR_COMPILER == 0 will protect WAMRC from linking failures.

XeniaLu · 2024-06-19T06:34:15Z

thanks for @wenyongh. I kindly understand the situation here. The project needs to -DWAMR_BUILD_MULTI_MODULE=1 when compiling WAMRC but actually don't want show error and quit, which is in #if WASM_ENABLE_MULTI_MODULE !=0 ... #endif block, to bother WAMRC compilation. Please correct me if I am wrong.

In our project, we don't directly use WAMRC, instead we integrated the compiler code into the runtime. And yes, we need -DWAMR_BUILD_MULTI_MODULE=1 and we perfer not to show errors and quit when dependent modules are not found.

I guess it becomes another problem: under other non-aot running mode, like interpreter and jit, when enabling MULTI_MODULE feature, what kind of behavior the loader should be if can't match any import requirement?

So I think using the LoadArgs.allow_missing_imports option is the most appropriate in this case. Without this option, there might be issues in non-aot mode.

yamt · 2024-06-20T04:04:08Z

thanks for @wenyongh. I kindly understand the situation here. The project needs to -DWAMR_BUILD_MULTI_MODULE=1 when compiling WAMRC but actually don't want show error and quit, which is in #if WASM_ENABLE_MULTI_MODULE !=0 ... #endif block, to bother WAMRC compilation. Please correct me if I am wrong.

In our project, we don't directly use WAMRC, instead we integrated the compiler code into the runtime. And yes, we need -DWAMR_BUILD_MULTI_MODULE=1 and we perfer not to show errors and quit when dependent modules are not found.

i suspect such a usage is simply unsupported.
i'm not sure if it should be supported either.
is this the only change you need for your "integrated compiler" use case?

XeniaLu · 2024-06-20T05:10:22Z

thanks for @wenyongh. I kindly understand the situation here. The project needs to -DWAMR_BUILD_MULTI_MODULE=1 when compiling WAMRC but actually don't want show error and quit, which is in #if WASM_ENABLE_MULTI_MODULE !=0 ... #endif block, to bother WAMRC compilation. Please correct me if I am wrong.

In our project, we don't directly use WAMRC, instead we integrated the compiler code into the runtime. And yes, we need -DWAMR_BUILD_MULTI_MODULE=1 and we perfer not to show errors and quit when dependent modules are not found.

i suspect such a usage is simply unsupported. i'm not sure if it should be supported either. is this the only change you need for your "integrated compiler" use case?

Yes, the commit with the LoadArgs.allow_missing_imports option is the only change addresses our immediate needs.

lum1n0us · 2024-06-20T06:31:20Z

IIUC, for some reason, in your project, WAMRC and IWASM are compiled in a same set of compilation options. It is different with what we did in wamr repo. (Maybe both executables share one library of WAMR?)

So there will be a problem that when -DWAMR_BUILD_MULTI_MODULE=1, how to

let wamrc generate aot files successfully if the input core module has dependencies and they are not available now
let interpreter/jit "raise (linking)errors and quit" if the core module import requirements can't be satisfied

@yamt hope those can explain the situation. @XeniaLu please feel free to add more comments

In order to resolve those issues, we hope to keep the code simple by:

not using an option as a switcher to control wasm_loader behavior
wasm_loader only uses WAMRC policy which is link as many as possible without raising errors.
do linking status check in instantiation phase. It is executed by check_linked_symbol().

Therefore, both wamrc and interpreter and jit can be taken care at the same time.

But, check_linked_symbol() only covers function and global now. Please add check for memory, table and tag. #if WASM_ENABLE_WAMR_COMPILER == 0 will protect WAMRC from linking failures.

yamt · 2024-06-20T07:46:16Z

IIUC, for some reason, in your project, WAMRC and IWASM are compiled in a same set of compilation options. It is different with what we did in wamr repo. (Maybe both executables share one library of WAMR?)

i'm interested in why.

if we want to support such a configuration, i guess we need a sample to run on the ci.

XeniaLu · 2024-06-21T00:36:09Z

Maybe both executables share one library of WAMR?

We are developing a Postgres extension that statically links the WAMR compiler and IWASM code together. Our extension provides a PG function for AOT compilation and also includes a runtime for execution.

If you are interested, I can add you to our Gitee repository. It is currently a pre-open source project.

lum1n0us · 2024-06-21T01:04:21Z

statically links the WAMR compiler and IWASM code together.

Never saw that coming. 🤔.

Even so, we can still use check_linked_symbol() to do instanation phase check to make sure every necessary symbols are all set before execution.

The INTERP module loader was failing when multi-module support is on and dependant modules are not found; this enforced AOT compiler integrations to prepare dependant modules while it wasn't necessary. This new option allows integrated AOT compilers to work as if the multi-module support wasn't turned on. This option is also turned on in the AOT compiler CLI, just in case if `WASM_ENABLE_MULTI_MODULE` is turned on there one day.

Update the AOT loader to make allowing missing dependent modules the default behavior.

…nd tags.

XeniaLu · 2024-06-21T02:58:56Z

@lum1n0us Thanks, I've commit the changes for check_linked_symbol(). However, the tests are currently failing. I'm investigating on it, and any help you can provide would be greatly appreciated.

wenyongh · 2024-06-23T12:39:23Z

@lum1n0us Thanks, I've commit the changes for check_linked_symbol(). However, the tests are currently failing. I'm investigating on it, and any help you can provide would be greatly appreciated.

@XeniaLu I did some test on this and created a patch based on your PR:
wenyongh#932
It basically works, which reports linking error in wasm instantiation when the import global/table/memory/tag isn't builtin and isn't linked in multi-module mode. I removed unnecessary WASM_ENABLE_WAMRC_COMPILER macro controls in wasm_runtime.c since the aot compiler doesn't run into the file, and also modified several patch files for spec tests since the check is a little stricter than before. Maybe you can check whether it fits your requirement?

…dule is enabled" This reverts commit 535b9ed. This reverts commit e10ce09. This reverts commit afc08e1. This reverts commit 1ecc3af.

XeniaLu · 2024-06-24T02:27:22Z

@XeniaLu I did some test on this and created a patch based on your PR: wenyongh#932 It basically works, which reports linking error in wasm instantiation when the import global/table/memory/tag isn't builtin and isn't linked in multi-module mode. I removed unnecessary WASM_ENABLE_WAMRC_COMPILER macro controls in wasm_runtime.c since the aot compiler doesn't run into the file, and also modified several patch files for spec tests since the check is a little stricter than before. Maybe you can check whether it fits your requirement?

@wenyongh Super thanks for the patch. It works perfectly for our needs. I have cherry-picked your patch into this PR.

wenyongh

LGTM

lum1n0us · 2024-06-24T05:09:28Z

core/iwasm/common/wasm_runtime_common.c

@@ -1267,6 +1267,9 @@ wasm_runtime_is_built_in_module(const char *module_name)
            || !strcmp("wasi_snapshot_preview1", module_name)
 #if WASM_ENABLE_SPEC_TEST != 0
            || !strcmp("spectest", module_name)
+#endif
+#if WASM_ENABLE_LIB_WASI_THREADS != 0
+            || !strcmp("foo", module_name)


wondering why explicit "foo" here? If it came from a test case, better be surrounded by a #if...#endif.

Yes, maybe we can add a cmake variable cmake -DWAMR_BUILD_WASI_TEST=1/0 and add gcc macro WASM_ENABLE_WASI_TEST, like WAMR_BUILD_SPEC_TEST. And enable it when testing wasi_certification in test_wamr.sh (just a guess, to be confirmed):

diff --git a/tests/wamr-test-suites/test_wamr.sh b/tests/wamr-test-suites/test_wamr.sh index 135940e1..bbfd5259 100755 --- a/tests/wamr-test-suites/test_wamr.sh +++ b/tests/wamr-test-suites/test_wamr.sh @@ -1003,6 +1003,9 @@ function trigger() EXTRA_COMPILE_FLAGS+=" -DWAMR_BUILD_LIBC_UVWASI=0 -DWAMR_BUILD_LIBC_WASI=1" break fi + if [[ "$test" == "wasi_certification" ]] + EXTRA_COMPILE_FLAGS+=" -DWAMR_BUILD_WASI_TEST=1" + fi

I have added the cmake variable WAMR_BUILD_WASI_TEST and the corresponding GCC macro WASM_ENABLE_WASI_TEST. Since I'm not very familiar with this part, please let me know if there's anything missing or if further adjustments are needed.

And I have no idea about the failing tests. Could you please help me take a look?

Add lines in core/config.h:

#ifndef WASM_ENABLE_WASI_TEST #define WASM_ENABLE_WASI_TEST 0 #endif

lum1n0us · 2024-06-24T05:14:18Z

core/iwasm/interpreter/wasm_loader.c

@@ -2920,6 +2904,29 @@ load_memory_import(const uint8 **p_buf, const uint8 *buf_end,
        declare_init_page_count = spectest_memory_init_page;
        declare_max_page_count = spectest_memory_max_page;
    }
+#if WASM_ENABLE_LIB_WASI_THREADS != 0
+    /* a case in wasi-testsuite which imports ("foo" "bar") */


if so(to make test cases happy), we'd better make a new preprocessor conditionals. like WASM_ENABLE_WASI_TEST.

@wenyongh we need a new marco here to control if in some wasi-testsuites

lum1n0us · 2024-06-24T05:20:05Z

core/iwasm/interpreter/wasm_runtime.c

-        if (!global->is_linked) {
+
+        if (!global->is_linked
+            && !wasm_runtime_is_built_in_module(global->module_name)) {


wondering why need to check wasm_runtime_is_built_in_module() here. IIUC, if requires a built_in module, it should be satisfied in wasm_loader.c.

I am not sure, I just refactored the code based on @XeniaLu's original modification. Maybe @XeniaLu can help remove this line to have a try?

Sure, I will remove it.

lum1n0us · 2024-06-24T23:23:46Z

core/iwasm/interpreter/wasm_runtime.c

+    for (i = 0; i < module->import_table_count; i++) {
+        WASMTableImport *table = &((module->import_tables + i)->u.table);
+
+        if (!wasm_runtime_is_built_in_module(table->module_name)


please reconsider whether check is_built_in on table too.

lum1n0us · 2024-06-24T23:23:54Z

core/iwasm/interpreter/wasm_runtime.c

+    for (i = 0; i < module->import_memory_count; i++) {
+        WASMMemoryImport *memory = &((module->import_memories + i)->u.memory);
+
+        if (!wasm_runtime_is_built_in_module(memory->module_name)


please reconsider whether check is_built_in on memory too.

I added the is_built_in check because the CI test was failing at that time.

🤔 it is a good fix.

Although we've linked (table and memory) in wasm_laoder.c for spectest only, there is no flag to store the status. Then we use wasm_runtime_is_built_in_module() to implement a strong assumption that all built-in modules requirements has been satisfied.

we can fix it in other PR

build-scripts/config_common.cmake

lum1n0us

LGTM

XeniaLu force-pushed the fix-multi-module branch from cdc80de to 4164d04 Compare June 19, 2024 02:14

XeniaLu changed the title ~~Add LoadArgs.allow_missing_imports for multi-module AOT~~ Allow loading of missing imports without errors when multi-module is enabled Jun 21, 2024

XeniaLu added 3 commits June 21, 2024 09:09

Allow missing dependent moudles by default

e10ce09

Update the AOT loader to make allowing missing dependent modules the default behavior.

Expanded checks in check_linked_symbol() to cover memories, tables, a…

afc08e1

…nd tags.

XeniaLu force-pushed the fix-multi-module branch from 4164d04 to afc08e1 Compare June 21, 2024 01:09

bypass linking status checks for built-in module

1ecc3af

XeniaLu and others added 4 commits June 24, 2024 10:14

Revert "Allow loading of missing imports without errors when multi-mo…

3473216

…dule is enabled" This reverts commit 535b9ed. This reverts commit e10ce09. This reverts commit afc08e1. This reverts commit 1ecc3af.

Allow missing imports in loader and report error in instantiation

e1f106f

add wasi-threads builtin module

7e9861f

update loader

bcf9261

wenyongh approved these changes Jun 24, 2024

View reviewed changes

lum1n0us reviewed Jun 24, 2024

View reviewed changes

Remove built in check for global and add WASM_ENABLE_WASI_TEST macro

3a680ec

lum1n0us reviewed Jun 24, 2024

View reviewed changes

wenyongh reviewed Jun 25, 2024

View reviewed changes

build-scripts/config_common.cmake Show resolved Hide resolved

Fix the nuttx CI failures

1ff2438

lum1n0us approved these changes Jun 25, 2024

View reviewed changes

wenyongh merged commit f7d2826 into bytecodealliance:main Jun 25, 2024
378 checks passed

XeniaLu deleted the fix-multi-module branch June 25, 2024 13:19

wenyongh mentioned this pull request Aug 8, 2024

[RFC] Core module Dynamic Linking #3678

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow loading of missing imports without errors when multi-module is enabled #3539

Allow loading of missing imports without errors when multi-module is enabled #3539

XeniaLu commented Jun 18, 2024 •

edited

Loading

lum1n0us commented Jun 18, 2024 •

edited

Loading

XeniaLu commented Jun 19, 2024

lum1n0us commented Jun 19, 2024 •

edited

Loading

lum1n0us commented Jun 19, 2024

XeniaLu commented Jun 19, 2024

yamt commented Jun 20, 2024

XeniaLu commented Jun 20, 2024

lum1n0us commented Jun 20, 2024

yamt commented Jun 20, 2024

XeniaLu commented Jun 21, 2024

lum1n0us commented Jun 21, 2024

XeniaLu commented Jun 21, 2024

wenyongh commented Jun 23, 2024

XeniaLu commented Jun 24, 2024

wenyongh left a comment

lum1n0us Jun 24, 2024

wenyongh Jun 24, 2024

XeniaLu Jun 24, 2024

wenyongh Jun 25, 2024

lum1n0us Jun 24, 2024

lum1n0us Jun 24, 2024

wenyongh Jun 24, 2024

XeniaLu Jun 24, 2024

lum1n0us Jun 24, 2024

lum1n0us Jun 24, 2024

XeniaLu Jun 24, 2024

lum1n0us Jun 25, 2024

lum1n0us left a comment

Allow loading of missing imports without errors when multi-module is enabled #3539

Allow loading of missing imports without errors when multi-module is enabled #3539

Conversation

XeniaLu commented Jun 18, 2024 • edited Loading

lum1n0us commented Jun 18, 2024 • edited Loading

XeniaLu commented Jun 19, 2024

lum1n0us commented Jun 19, 2024 • edited Loading

lum1n0us commented Jun 19, 2024

XeniaLu commented Jun 19, 2024

yamt commented Jun 20, 2024

XeniaLu commented Jun 20, 2024

lum1n0us commented Jun 20, 2024

yamt commented Jun 20, 2024

XeniaLu commented Jun 21, 2024

lum1n0us commented Jun 21, 2024

XeniaLu commented Jun 21, 2024

wenyongh commented Jun 23, 2024

XeniaLu commented Jun 24, 2024

wenyongh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lum1n0us left a comment

Choose a reason for hiding this comment

XeniaLu commented Jun 18, 2024 •

edited

Loading

lum1n0us commented Jun 18, 2024 •

edited

Loading

lum1n0us commented Jun 19, 2024 •

edited

Loading