Fix three threading+callback issues #2173
Conversation
|
This is a superset of https://github.com/bytecodealliance/wasm-micr4o-runtime/pull/2170 and misses the mentioned future refactor and both cleanup codes (including the one included there). Also the copying code is inserted in a different place here and in pull 2170, the other one looks more in line with the existing pthread code. I suspect that this change in wasm_runtime.c makes the original workaround in lib_pthread_wrapper.c unnecessary. |
| ->e->c_api_func_imports | ||
| != NULL) { | ||
| /* workaround about passing instantiate-linking information */ | ||
| { |
There was a problem hiding this comment.
@cpetig It is to fix the issue of c_api_func_imports are not passed to new threads reported in #2149 , right? My suggestion is to apply the changes to lib_wasi_threads_wrapper.c like #2170 since (1) it is only related to multi-threading, adding code here may increase the footprint, (2) the fixing for AOT mode is invalid, the related code is in aot_instantiate: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/aot/aot_runtime.c#L1089
@eloparco How about we merging the fixing of #2170 to this PR? Just use this PR to fix all these issues? Thanks.
There was a problem hiding this comment.
BTW, there may be duplicated code in lib_wasi_threads_wrapper.c and lib_pthread_wrapper.c, after discussing with @lum1n0us, we suggest to add an API in core/iwasm/libraries/thread-mgr/thread_manager.{h|c}, e.g.,
bool
wasm_cluster_dup_c_api_imports(WASMModuleInstance *module_inst_dst,
const WASMModuleInstance *module_inst_src);
There was a problem hiding this comment.
lib_pthread_wrapper.c has a same workaround. It doesn't seem clever to write the same workaround twice. Shall we merge them together and store it somewhere public, maybe thread_manager?
There was a problem hiding this comment.
@wenyongh (2) the fixing for AOT mode is invalid, the related code is in
aot_instantiate
Do I understand you correctly that https://github.com/cpetig/wasm-micro-runtime/blob/main/core/iwasm/libraries/thread-mgr/thread_manager.c#L758 is not necessary because this is taken care of in aot_runtime.c ?
There was a problem hiding this comment.
How about we merging the fixing of #2170 to this PR? Just use this PR to fix all these issues? Thanks.
Makes sense, I'll close the other PR.
lib_pthread_wrapper.c has a same workaround. It doesn't seem clever to write the same workaround twice. Shall we merge them together and store it somewhere public, maybe thread_manager?
Yes, that's what I had in mind as well when in my PR I wrote "refactor common code".
There was a problem hiding this comment.
Do I understand you correctly that https://github.com/cpetig/wasm-micro-runtime/blob/main/core/iwasm/libraries/thread-mgr/thread_manager.c#L758 is not necessary because this is taken care of in aot_runtime.c ?
I think it's probably the other way around, at the beginning you were putting your changes in wasm_runtime.c so you would have needed to change aot_runtime.c as well. If the changes are in the pthread and wasi thread files no change in aot_runtime.c should be required.
|
I closed the other PR, let's discuss here instead. For #2170 (comment), yes, no need for the additional |
| wasm_cluster_dup_c_api_imports(wasm_module_inst_t module_inst_dst, | ||
| wasm_module_inst_t module_inst_src, | ||
| wasm_module_t module) |
There was a problem hiding this comment.
Had better use WASMModuleInstanceCommon * instead of wasm_module_inst_t, we normally use the former internally.
And no need to add argument module, we can get it from module_inst->module
| if (((const WASMModuleInstance *)module_inst_src)->e->c_api_func_imports | ||
| != NULL) { |
There was a problem hiding this comment.
Should not use assume that the module instance is type of bytecode here, it may be type of AOT, the module_inst_src->e may be WASMModuleInstanceExtra or AOTModuleInstanceExtra, had better:
CApiFuncImport **new_c_api_func_imports = NULL;
CApiFuncImport *c_api_func_imports;
uint32 import_func_count = 0;
uint32 size_in_bytes = 0;
#if WASM_ENABLE_INTERP != 0
if (module_inst_src->module_type == Wasm_Module_Bytecode) {
WASMModuleInstanceExtra *e_src =
(WASMModuleInstanceExtra *)((WASMModuleInstance *)module_inst_src)
->e;
if (e_src->c_api_func_imports != NULL) {
...
}
}
#endif
#if WASM_ENABLE_AOT != 0
if (module_inst_src->module_type == Wasm_Module_AoT) {
AOTModuleInstanceExtra *e_src =
(AOTModuleInstanceExtra *)((AOTModuleInstance *)module_inst_src)
->e;
if (e_src->c_api_func_imports != NULL) {
...
}
}
#endif
|
LGTM |
…ance#2173) Fix issue reported in bytecodealliance#2172: wasm-c-api `wasm_func_call` may use a wrong exec_env when multi-threading is enabled, with error "invalid exec env" reported Fix issue reported in bytecodealliance#2149: main instance's `c_api_func_imports` are not passed to the counterpart of new thread's instance in wasi-threads mode Fix issue of invalid size calculated to copy `c_api_func_imports` in pthread mode And refactor the code to use `wasm_cluster_dup_c_api_imports` to copy the `c_api_func_imports` to new thread for wasi-threads mode and pthread mode.
Testcase is available at #2172 , fixes also #2149 and an unreported mistake in the original code I copied the fix from.