[instantiation linking] Create and import WASMTableInstance #3898

lum1n0us · 2024-11-08T12:17:34Z

Based on #3893

…deinstantiation logic

… table instantiation logic and reducing unnecessary assertions

…in AOT loader and runtime

…execution environment

core/iwasm/aot/aot_loader.c

wenyongh · 2024-11-29T08:34:07Z

core/iwasm/aot/aot_runtime.c

+        memory_deinstantiate(memory);
+
+#if WASM_ENABLE_MULTI_MODULE == 0
+        uint16 rc = BH_ATOMIC_16_FETCH_OR(memory->ref_count, 0);


the ref_count is only available for shared memory, and normally we should access with with g_shared_memory_lock acquired, refer to:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_shared_memory.c#L85-L91

so here per my understanding, the code had better be like:

uint16 rc = 0; #if WASM_ENABLE_SHARED_MEMORY != 0 if (memory->is_shared_memory) { #if BH_ATOMIC_16_IS_ATOMIC == 0 os_mutex_lock(&g_shared_memory_lock); #endif rc = BH_ATOMIC_16_FETCH_OR(memory->ref_count, 0); #if BH_ATOMIC_16_IS_ATOMIC == 0 os_mutex_unlock(&g_shared_memory_lock); #endif } #endif if (rc == 0) { wasm_runtime_free(memory); }

Or how about adding an API like shared_memory_get_ref_count in wasm_shared_memory.c, and then here:

uint16 rc = 0; #if WASM_ENABLE_SHARED_MEMORY != 0 if (memory->is_shared_memory) rc = shared_memory_get_ref_count(memory); #endif ...

core/iwasm/aot/aot_runtime.c

wenyongh · 2024-12-01T03:58:35Z

core/iwasm/aot/aot_runtime.c

+        table =
+            (AOTTableInstance *)((uint8 *)table
+                                 + offsetof(AOTTableInstance, elems)
+                                 + sizeof(table_elem_type_t) * table->max_size);


offsetof(AOTTableInstance, elems) + sizeof(table_elem_type_t) * n are used in many places, maybe we can define it as a macro or function?

wenyongh · 2024-12-01T03:59:42Z

core/iwasm/compilation/aot_emit_aot_file.c

+
+    AOTImportTable *import_table = comp_data->import_tables;
+    for (uint32 i = 0; i < comp_data->import_table_count; i++, import_table++) {
+        offset = align_uint(offset, 2);


had better align_uint(offset, 4)

wenyongh · 2024-12-01T04:47:10Z

core/iwasm/compilation/aot_emit_aot_file.c

+         *   + u32 init_size + u32 max_size
+         *   + u32 elem_ref_type.heap_type
+         */
        size += sizeof(uint32) * 3;


Seems should be size += sizeof(uint32) * 2 since the elem_ref_type.heap_type is optional and is calculated below?

// aot_emit_import_table_info() EMIT_U8(import_table->table_type.elem_type); EMIT_U8(import_table->table_type.flags); EMIT_U8(import_table->table_type.possible_grow); #if WASM_ENABLE_GC != 0 if (comp_ctx->enable_gc && import_table->table_type.elem_ref_type) { EMIT_U8( import_table->table_type.elem_ref_type->ref_ht_common.nullable); } else #endif { /* emit one placeholder to keep the same size */ EMIT_U8(0); } // --------------------- U32 EMIT_U32(import_table->table_type.init_size); EMIT_U32(import_table->table_type.max_size); // --------------------- U32 * 3

got it, thanks.

wenyongh · 2024-12-01T04:57:49Z

core/iwasm/interpreter/wasm_runtime.c

+        memory_deinstantiate(memory);
+#else
+        memory_deinstantiate(memory);
+        uint16 rc = BH_ATOMIC_16_FETCH_OR(memory->ref_count, 0);


same as aot_runtime.c, suggest to add API shared_memory_get_ref_count, and here:

uint16 rc = 0; #if WASM_ENABLE_SHARED_MEMORY != 0 if (memory->is_shared_memory) rc = shared_memory_get_ref_count(memory); #endif ...

wenyongh · 2024-12-01T05:01:09Z

core/iwasm/interpreter/wasm_runtime.c

+        }
+
+        table_elem_type_t *table_elems =
+            wasm_locate_table_elems(module, table, i);


Same as aot_runtime.c, should we check whether table_elems is NULL, and free it only when it is not NULL?

…ation checks

… in AOT compilation

…mory support in AOT runtime and interpreter

wenyongh

LGTM with minor comments.

wenyongh · 2024-12-03T07:26:58Z

core/iwasm/compilation/aot_emit_aot_file.c

+         *   + u32 init_size + u32 max_size
+         *   + u32 elem_ref_type.heap_type
+         */
        size += sizeof(uint32) * 3;


got it, thanks.

wenyongh · 2024-12-03T07:30:10Z

core/iwasm/aot/aot_loader.c

        if (wasm_is_type_multi_byte_type(import_table->table_type.elem_type)) {
            read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable);
        }
+#else


had better add an else branch when WASM_ENABLE_GC is enabled, how about:

#if WASM_ENABLE_GC != 0 if (wasm_is_type_multi_byte_type(import_table->table_type.elem_type)) { read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable); } else #endif { ... }

wenyongh · 2024-12-03T07:32:37Z

core/iwasm/aot/aot_loader.c

        if (wasm_is_type_multi_byte_type(table->table_type.elem_type)) {
            read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable);
        }
+#else


same as above, how about:

#if WASM_ENABLE_GC != 0 if (wasm_is_type_multi_byte_type(table->table_type.elem_type)) { read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable); } else #endif { ... }

wenyongh · 2024-12-03T07:32:40Z

core/iwasm/aot/aot_loader.c

+
+        /* TBC: sync up with AOT_CURRENT_VERSION in config.h */
+        /* v3 doesn't emit names */
+        if (module->package_version >= 4) {


Seems that the import table in the older aot file isn't used at all or developer doesn't generate the import table? Maybe we can remove the line?

I'm not certain. I'll add a TODO note here, maintain the 'if' control, and run a full compatible test later.

The original aot table instantiation has bug when there are import tables: module->tables[i].table_type.elem_type and module->tables[i].table_type.elem_ref_type are used to set the table instance's fields, and i will be out of range: there are only module->table_count tables, while i can be module->import_table_count to module->import_table_count + module->table_count - 1. So accessing module->tables[i].table_type.elem_ref_type will access illegal memory. I guess that import table was not used, no one reported the error.

I uploaded a PR to fix it: #3946

… version compatibility in AOT loader

wenyongh · 2024-12-03T08:07:37Z

core/iwasm/aot/aot_loader.c

            read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable);
        }
 #endif
+        {


should add else before #endif

sync up with aot_emit_table_info()

#if WASM_ENABLE_GC != 0 if (comp_ctx->enable_gc && comp_data->import_tables[i].table_type.elem_ref_type) { EMIT_U8(comp_data->import_tables[i] .table_type.elem_ref_type->ref_ht_common.nullable); } else #endif { /* emit one placeholder to keep the same size */ EMIT_U8(0); }

ok, so you will add else before #endif?

#if WASM_ENABLE_GC != 0 if (wasm_is_type_multi_byte_type(import_table->table_type.elem_type)) { read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable); } else #endif { ... }

😨 oops

forgot sync up. Here is another one for main branch. #3940. please take a look

wenyongh · 2024-12-03T08:08:02Z

core/iwasm/aot/aot_loader.c

            read_uint8(buf, buf_end, ref_type.ref_ht_common.nullable);
        }
 #endif
+        {


should add else before #endif

sync up with aot_emit_table_info()

#if WASM_ENABLE_GC != 0 if (comp_ctx->enable_gc && comp_data->tables[i].table_type.elem_ref_type) { EMIT_U8(comp_data->tables[i] .table_type.elem_ref_type->ref_ht_common.nullable); } else #endif { /* emit one placeholder to keep the same size */ EMIT_U8(0); }

…ove code clarity

wenyongh

LGTM

…alliance#3898) - Implement import table and table creation - Enhance memory de-instantiation checks and adjust stack size for wasm execution environment - Improve shared memory reference handling and enhance table de-instantiation checks

lum1n0us force-pushed the feat/inst_linking_table branch from 0e0841f to 39c2cf1 Compare November 10, 2024 14:07

lum1n0us marked this pull request as ready for review November 11, 2024 07:02

lum1n0us requested review from loganek, no1wudi, wenyongh, xujuntwt95329 and yamt as code owners November 11, 2024 07:02

lum1n0us mentioned this pull request Nov 15, 2024

[RFC] Support instantiation linking #3807

Open

lum1n0us force-pushed the feat/inst_linking_table branch 2 times, most recently from be499a7 to f5b94f5 Compare November 20, 2024 05:55

lum1n0us mentioned this pull request Nov 21, 2024

[instantiation linking] Refactor wasm_runtime_instantiate() #3893

Merged

lum1n0us force-pushed the feat/inst_linking_table branch from f5b94f5 to d22f1ea Compare November 21, 2024 07:01

[inst-link table] Implement import table and table creation

4877adf

lum1n0us force-pushed the feat/inst_linking_table branch from d22f1ea to 4877adf Compare November 21, 2024 07:07

lum1n0us added 4 commits November 21, 2024 12:12

Refactor import handling to use uint32 for counts and improve memory …

bc71e5f

…deinstantiation logic

Optimize memory management in AOT runtime and interpreter by refining…

0cc64bc

… table instantiation logic and reducing unnecessary assertions

Add comments for version synchronization and conditional compilation …

4748526

…in AOT loader and runtime

Enhance memory deinstantiation checks and adjust stack size for wasm …

7b22636

…execution environment

wenyongh reviewed Dec 1, 2024

View reviewed changes

lum1n0us added 3 commits December 1, 2024 08:06

Improve shared memory reference handling and enhance table deinstanti…

14392c6

…ation checks

Adjust alignment and documentation for import table size calculations…

b4d2709

… in AOT compilation

Refactor memory deinstantiation checks for multi-module and shared me…

d8721a6

…mory support in AOT runtime and interpreter

lum1n0us force-pushed the feat/inst_linking_table branch from 564d11c to d8721a6 Compare December 1, 2024 08:49

lum1n0us mentioned this pull request Dec 1, 2024

Consume the placeholders that were put when emitting table info #3940

Merged

wenyongh approved these changes Dec 3, 2024

View reviewed changes

Refactor conditional compilation directives and add TODO comments for…

99bbdd9

… version compatibility in AOT loader

wenyongh reviewed Dec 3, 2024

View reviewed changes

lum1n0us added 2 commits December 4, 2024 11:11

Refactor import table loading to skip unnecessary byte reads and impr…

f47dc0b

…ove code clarity

Remove outdated version checks for import table loading in AOT loader

85f7d3d

wenyongh approved these changes Dec 5, 2024

View reviewed changes

lum1n0us merged commit ee2d60b into bytecodealliance:dev/instantiate_linking Dec 6, 2024
383 checks passed

lum1n0us deleted the feat/inst_linking_table branch December 6, 2024 02:13

[instantiation linking] Create and import WASMTableInstance #3898

[instantiation linking] Create and import WASMTableInstance #3898

Uh oh!

Conversation

lum1n0us commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyongh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lum1n0us Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyongh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lum1n0us commented Nov 8, 2024 •

edited

Loading

lum1n0us Dec 4, 2024 •

edited

Loading