fix data/elem drop #2747
fix data/elem drop #2747
Conversation
yamt
changed the title
yamt
force-pushed
the
2735
branch
3 times, most recently
from
89f4c38
to
9e22da7
Compare
a copy of core/iwasm/fast-jit/jit_utils.[ch]
Note: this commit also fixes a missing check of dropped element. cf. bytecodealliance#2772
| bh_bitmap *data_dropped; | ||
| bh_bitmap *elem_dropped; |
There was a problem hiding this comment.
The data drop is introduced in bulk memory and the elem drop is introduced in reference types, could we add macro WASM_ENABLE_BULK_MEMORY and WASM_ENABLE_REF_TYPES to control the code here, new and delete bitmap, so as to decrease footprint when they are not needed?
| wasm_module_inst_t module_inst[N]; | ||
| wasm_exec_env_t exec_env[N]; |
There was a problem hiding this comment.
Had better add = { 0 } to initialize module_inst and exec_env, or unexpected behavior may occur when destroying them.
There was a problem hiding this comment.
they are initialized below explicitly.
core/iwasm/aot/aot_runtime.c
Outdated
| common->data_dropped = bh_bitmap_new(0, module->mem_init_data_count); | ||
| common->elem_dropped = bh_bitmap_new(0, module->table_init_data_count); |
There was a problem hiding this comment.
Could we allocate memory only when mem_init_data_count/mem_init_data_count is larger than 0? Or there may be a warning:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_memory.c#L245
| wasm_module_inst_t module_inst[N]; | ||
| wasm_exec_env_t exec_env[N]; |
core/shared/utils/bh_bitmap.c
Outdated
| #include <limits.h> | ||
| #include <stddef.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
Had better remove these lines, normally they are included in platform_internal.h of each platforms, and we don't include libc header files in core/shared/utils.
they (or equivalents) are supposed to be provided by bh_platform.h.
|
the ci failure doesn't seem related to the changes. |
Currently, `data.drop` instruction is implemented by directly modifying the underlying module. It breaks use cases where you have multiple instances sharing a single loaded module. `elem.drop` has the same problem too. This PR fixes the issue by keeping track of which data/elem segments have been dropped by using bitmaps for each module instances separately, and add a sample to demonstrate the issue and make the CI run it. Also add a missing check of dropped elements to the fast-jit `table.init`. Fixes: bytecodealliance#2735 Fixes: bytecodealliance#2772
currently,
data.dropinstruction is implemented by directly modifying the underlying module.it breaks use cases where you have multiple instances sharing a single loaded module.
elem.drophas the same problem too.this fixes the issue by mirroring the implementation approarch from toywasm: https://github.com/yamt/toywasm/blob/fca52a0174eb1644fea9caa3fe1faad598864f6a/lib/type.h#L488-L494.
that is, keep track of which data/elem segments have been dropped by using bitmaps for each module instances separately.
an obvious drawback of the approach is to consume extra memory for tracking. however,
given the way the wasm spec defines these operations, i suspect this is the only way
to support shared modules. it isn't allowed to simply ignore
data.dropandelem.dropaswe should raise a trap when dropped segments are used.
also, add a sample to demonstrate the issue and make the ci run it.
also, add a missing check of dropped elements to the fast-jit version of
table.init.fixes: #2735
fixes: #2772
todo:
fast interpreter,aot,jit,fast-jit(i always feel we have too many variations!)investigateerror: unknown import or incompatible import typeseen on the ci.eg. https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/6875997752/job/18700854026?pr=2747
remove JitBitmap (JitBitmap seems unused #2754)iwill submit a separate PR as this PR is already too big for my tasteRemove unused JitBitmap #2775fix a fast-jit specific issue: fast-jit: missing check of dropped elements #2772