aot: avoid possible relocations around "stack_sizes" for indirect mode #2322
Conversation
core/iwasm/aot/aot_runtime.c
Outdated
| == 13 * sizeof(uint64) + 128 + 11 * sizeof(uint64)); | ||
| bh_static_assert(offsetof(AOTModuleInstance, global_table_data) | ||
| == 13 * sizeof(uint64) + 128 + 12 * sizeof(uint64)); |
There was a problem hiding this comment.
i'm not happy to break offsets.
should i add the new member after the variable parts of the structure?
There was a problem hiding this comment.
Adding new fields in the WASMModuleInstance/AOTModuleInstance may cause compatibility issue: the AOT code also use pre-calculated offsets to access the global data and table instance (which are at the end of AOTModuleInstance and are allocated together with AOTModuleInstance), these offsets will change if new field is inserted, and the old AOT file (generated by older wamrc) won't run well with the new runtime.
How about adding the aot_stack_sizes into exec_env, after field native_stack_top_min:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_exec_env.h#L91
There was a problem hiding this comment.
is there a longer term plan about compatibility breakage?
like having a flag day for #2144 when we can clean up these things?
There was a problem hiding this comment.
i moved it to aot inst extra.
There was a problem hiding this comment.
is there a longer term plan about compatibility breakage? like having a flag day for #2144 when we can clean up these things?
Yes, we hope to upgrade the AOT file format in the GC AOT feature and don't support the old AOT file format again after the feature is implemented. The GC AOT is supposed to be finished in this year, and we may merge it into main branch sometime this year.
Fixes bytecodealliance#2316 Lightly tested on riscv64 qemu.
by moving aot_stack_sizes from aot inst to inst extra
yamt
force-pushed
the
stack-sizes-indirect
branch
from
ebb6a68
to
f7ec28b
Compare
core/iwasm/aot/aot_runtime.h
Outdated
| @@ -88,6 +88,7 @@ typedef struct AOTFunctionInstance { | |||
| } AOTFunctionInstance; | |||
|
|
|||
| typedef struct AOTModuleInstanceExtra { | |||
| const uint32 *stack_sizes; | |||
There was a problem hiding this comment.
Had better use DefPointer(const uint32 *, stack_sizes)? Suppose the aot code may access more fields here in the future, we can ensure the offsets of these fields are fixed, and are the same in compilation time and execution time.
core/iwasm/compilation/aot_llvm.c
Outdated
| LLVMValueRef offset = I32_CONST(offset_u32); | ||
| if (!offset) { | ||
| goto fail; | ||
| } | ||
| LLVMValueRef stack_sizes_p = |
There was a problem hiding this comment.
Had better declare the variables at the beginning of code piece { ... }. And had better add empty lines if needed. We usually keep that coding style to make the code easier to read.
For example, there is no empty line from L342 to L402, a little intensive:)
There was a problem hiding this comment.
i generally feel less lines are easier to read. but ok. i will follow your style.
not really necessary at this point. but just in case.
|
LGTM |
Fixes #2316
Lightly tested on riscv64 qemu.