TSAN fast interpreter failure

[Zzzabiyaka]

During PoC of #2679 I've found another data race in exception handling https://gist.github.com/Zzzabiyaka/dcad23be15bad484966e91b8da3d659a.

set_exception_visitor in the stacktrace makes me suspicious about #2509. @yamt, would you be able to look into that issue ?

[wenyongh]

This seems to be because that before creating a new thread, runtime will instantiate the module instance for it (and do the instantiation in the parent thread), and at the end of instantiating, runtime executes the start of the module, and to make the exec_env consistent with the exec_envs in the parent thread's call stack, runtime temporarily replace exec_env->module_inst with the child module instance.

There may be two options: (1) since the start function has been called in the main thread in the first time instantiation, we don't call it again in the later instantiation, (2) add cluster->lock for replacing exec_env->module_inst, executing start and restoring the exec_env->module_inst in the later instantiation.

@yamt how do think?

[yamt]

This seems to be because that before creating a new thread, runtime will instantiate the module instance for it (and do the instantiation in the parent thread), and at the end of instantiating, runtime executes the start of the module, and to make the exec_env consistent with the exec_envs in the parent thread's call stack, runtime temporarily replace exec_env->module_inst with the child module instance.

There may be two options: (1) since the start function has been called in the main thread in the first time instantiation, we don't call it again in the later instantiation, (2) add cluster->lock for replacing exec_env->module_inst, executing start and restoring the exec_env->module_inst in the later instantiation.

@yamt how do think?

(1) doesn't work because the start function is expected be called for each threads.

(2) might work. but i feel it's still a bit dirty and fragile.

how about (3) create a temporary exec_env to run the start function?

[wenyongh]

This seems to be because that before creating a new thread, runtime will instantiate the module instance for it (and do the instantiation in the parent thread), and at the end of instantiating, runtime executes the start of the module, and to make the exec_env consistent with the exec_envs in the parent thread's call stack, runtime temporarily replace exec_env->module_inst with the child module instance.
There may be two options: (1) since the start function has been called in the main thread in the first time instantiation, we don't call it again in the later instantiation, (2) add cluster->lock for replacing exec_env->module_inst, executing start and restoring the exec_env->module_inst in the later instantiation.
@yamt how do think?

(1) doesn't work because the start function is expected be called for each threads.

Is there any reference document mentioning that the start function should be called for each thread? I checked the spec document of threads proposal:
https://webassembly.github.io/threads/core/syntax/modules.html#syntax-start
It mentions that the start "is automatically invoked when the module is instantiated after tables and memories have been initialized", per my understanding, we only create one shared memory and the start function should be called only once. It is to initialize some data in wasm memory/table, which may have been changed by the main thread after the first time instantiation, and if we call it again when creating a child thread, the data may be overwritten/re-initialized again, which may cause unexpected behavior.

(2) might work. but i feel it's still a bit dirty and fragile.

The traverse_list in thread manager is protected by the cluster->lock, we may expose APIs in thread manager to acquire/release the cluster->lock, and call them when calling start function in child thread. The change should be simple and the code can be concise.

how about (3) create a temporary exec_env to run the start function?

The parent thread has used its exec_env in the call stacks, we had better keep the same exec_env in the new call, and when the hw bound check is enabled, runtime will check whether the exec_env to use is the same as exec_env_tls saved, if not, runtime reports error.

[yamt]

This seems to be because that before creating a new thread, runtime will instantiate the module instance for it (and do the instantiation in the parent thread), and at the end of instantiating, runtime executes the start of the module, and to make the exec_env consistent with the exec_envs in the parent thread's call stack, runtime temporarily replace exec_env->module_inst with the child module instance.
There may be two options: (1) since the start function has been called in the main thread in the first time instantiation, we don't call it again in the later instantiation, (2) add cluster->lock for replacing exec_env->module_inst, executing start and restoring the exec_env->module_inst in the later instantiation.
@yamt how do think?

(1) doesn't work because the start function is expected be called for each threads.

Is there any reference document mentioning that the start function should be called for each thread? I checked the spec document of threads proposal: https://webassembly.github.io/threads/core/syntax/modules.html#syntax-start It mentions that the start "is automatically invoked when the module is instantiated after tables and memories have been initialized", per my understanding, we only create one shared memory and the start function should be called only once.

it should be called as a part of a module instantiation.
and with wasi-threads, a thread creation is a module instantiation.
when llvm synthesizes a start function, it generates thread-safe bytecode.

It is to initialize some data in wasm memory/table, which may have been changed by the main thread after the first time instantiation, and if we call it again when creating a child thread, the data may be overwritten/re-initialized again, which may cause unexpected behavior.

a runtime should not assume what a start function actually does.

(2) might work. but i feel it's still a bit dirty and fragile.

The traverse_list in thread manager is protected by the cluster->lock, we may expose APIs in thread manager to acquire/release the cluster->lock, and call them when calling start function in child thread. The change should be simple and the code can be concise.

how about (3) create a temporary exec_env to run the start function?

The parent thread has used its exec_env in the call stacks, we had better keep the same exec_env in the new call, and when the hw bound check is enabled, runtime will check whether the exec_env to use is the same as exec_env_tls saved, if not, runtime reports error.

thinking a bit more, i feel (2) or (3) isn't a great idea.
after all, switching exec_env->module_inst is not that exceptional.
eg. it happens on a call of imported functions.

i guess ideally we should stop using module_inst to store an exception.
instead, exception can be stored in exec_env directly.
cf. #2512
unfortunately it likely involves some incompatible api changes though.

how do you think?

[wenyongh]

Is there any reference document mentioning that the start function should be called for each thread? I checked the spec document of threads proposal: https://webassembly.github.io/threads/core/syntax/modules.html#syntax-start It mentions that the start "is automatically invoked when the module is instantiated after tables and memories have been initialized", per my understanding, we only create one shared memory and the start function should be called only once.

it should be called as a part of a module instantiation. and with wasi-threads, a thread creation is a module instantiation. when llvm synthesizes a start function, it generates thread-safe bytecode.

The actual module instantiation already occurred in the first time instantiation in the main thread, here runtime creates new module instance for the child thread, but it is not necessary in fact, runtime may just re-use the instance data of main thread except creating some data like data area for the auxiliary stack top global, and in the second time instantiation, runtime doesn't create memory again. Though llvm creates a thread-safe code for the start function, it doesn't mean that all the compilers will do the same way. And as the code prevents the second time initialization for the data, runtime also doesn't need to call start function again.

It is to initialize some data in wasm memory/table, which may have been changed by the main thread after the first time instantiation, and if we call it again when creating a child thread, the data may be overwritten/re-initialized again, which may cause unexpected behavior.

a runtime should not assume what a start function actually does.

But no document specifies that the start function won't re-initialize the data again if they have been initialized. If start re-initializes the data, unexpected behavior may occur - runtime had better not call it; if start function prevents re-initialization, runtime doesn't need to call it.

The parent thread has used its exec_env in the call stacks, we had better keep the same exec_env in the new call, and when the hw bound check is enabled, runtime will check whether the exec_env to use is the same as exec_env_tls saved, if not, runtime reports error.

thinking a bit more, i feel (2) or (3) isn't a great idea. after all, switching exec_env->module_inst is not that exceptional. eg. it happens on a call of imported functions.

i guess ideally we should stop using module_inst to store an exception. instead, exception can be stored in exec_env directly. cf. #2512 unfortunately it likely involves some incompatible api changes though.

how do you think?

Moving exception to exec_env may change the ABI of AOT and introduce big code change, if you stick to calling start when creating child thread, my suggestion is to use (2) temporarily.

[yamt]

Is there any reference document mentioning that the start function should be called for each thread? I checked the spec document of threads proposal: https://webassembly.github.io/threads/core/syntax/modules.html#syntax-start It mentions that the start "is automatically invoked when the module is instantiated after tables and memories have been initialized", per my understanding, we only create one shared memory and the start function should be called only once.

it should be called as a part of a module instantiation. and with wasi-threads, a thread creation is a module instantiation. when llvm synthesizes a start function, it generates thread-safe bytecode.

The actual module instantiation already occurred in the first time instantiation in the main thread, here runtime creates new module instance for the child thread, but it is not necessary in fact, runtime may just re-use the instance data of main thread except creating some data like data area for the auxiliary stack top global, and in the second time instantiation, runtime doesn't create memory again. Though llvm creates a thread-safe code for the start function, it doesn't mean that all the compilers will do the same way.

such a compiler is broken and a runtime can't fix it.

And as the code prevents the second time initialization for the data, runtime also doesn't need to call start function again.

It is to initialize some data in wasm memory/table, which may have been changed by the main thread after the first time instantiation, and if we call it again when creating a child thread, the data may be overwritten/re-initialized again, which may cause unexpected behavior.

a runtime should not assume what a start function actually does.

But no document specifies that the start function won't re-initialize the data again if they have been initialized. If start re-initializes the data, unexpected behavior may occur - runtime had better not call it; if start function prevents re-initialization, runtime doesn't need to call it.

the wasm spec is clear when the start function is called. isn't it enough for you?

even with wasi-threads, the start function is not dedicated to shared data initialization at all.
sometimes it initializes thread-local things like globals too.

The parent thread has used its exec_env in the call stacks, we had better keep the same exec_env in the new call, and when the hw bound check is enabled, runtime will check whether the exec_env to use is the same as exec_env_tls saved, if not, runtime reports error.

thinking a bit more, i feel (2) or (3) isn't a great idea. after all, switching exec_env->module_inst is not that exceptional. eg. it happens on a call of imported functions.
i guess ideally we should stop using module_inst to store an exception. instead, exception can be stored in exec_env directly. cf. #2512 unfortunately it likely involves some incompatible api changes though.
how do you think?

Moving exception to exec_env may change the ABI of AOT and introduce big code change,

i agree it's a big change and i don't intend to do it right now. i still think it's a good idea for longer term though.

if you stick to calling start when creating child thread, my suggestion is to use (2) temporarily.

ok.

[wenyongh]

OK, if the start function may instantiate the thread local data, it makes sense.

BTW, I saw you created PR #2685 to fix this issue, I just want to share another idea, how about we calling the wasm start function in the callback of thread routine, before calling the wasi thread_arg->start_func:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c#L59

In that time, the new thread was created, the exec_env for the new thread and module instance has been created and added into the exec_env list, so we don't need to modify the parent exec_env's module_inst, and don't need to add cluster->lock for it. How do you think?

[yamt]

OK, if the start function may instantiate the thread local data, it makes sense.

BTW, I saw you created PR #2685 to fix this issue, I just want to share another idea, how about we calling the wasm start function in the callback of thread routine, before calling the wasi thread_arg->start_func: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c#L59

In that time, the new thread was created, the exec_env for the new thread and module instance has been created and added into the exec_env list, so we don't need to modify the parent exec_env's module_inst, and don't need to add cluster->lock for it. How do you think?

it would work for thread creation. but it doesn't work for other cases like wasm_interp_call_wasm.

ideally i guess it makes more sense to tweak the api to create exec_env earlier as a module instantiation also needs it. but again, it's a big change.

[wenyongh]

Not sure why it doesn't work for wasm_interp_call_wasm? I mean that for the parent thread, the start function is executed at the end of instantiation as now it is, and for the child thread, it is changed to be called in the callback of the child thread.

[yamt]

Not sure why it doesn't work for wasm_interp_call_wasm? I mean that for the parent thread, the start function is executed at the end of instantiation as now it is, and for the child thread, it is changed to be called in the callback of the child thread.

sorry, i meant wasm_interp_call_func_import, not wasm_interp_call_wasm.

wasm_interp_call_func_import itself doesn't involve threads. it switches to the callee module.

[wenyongh]

I think you meant to call an import function in the multi-module feature, right? The sub modules that the current module relies on should have been instantiated after the current module instance was allocated:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/interpreter/wasm_runtime.c#L1644-L1646

So when runtime calls wasm_interp_call_func_import, the sub module should have been instantiated.

Another case is that in the native API of host embedder, it loads and instantiates another wasm module when the current module is running. Sounds a little tricky, I am not sure whether we should allow it?

[yamt]

I think you meant to call an import function in the multi-module feature, right? The sub modules that the current module relies on should have been instantiated after the current module instance was allocated: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/interpreter/wasm_runtime.c#L1644-L1646

So when runtime calls wasm_interp_call_func_import, the sub module should have been instantiated.

yes. but the instantiation is not a concern here.
as wasm_interp_call_func_import switches exec_env->module_inst,
it can race with thread-mgr cluster-traversing functions. (in a way TSAN would detect)

Another case is that in the native API of host embedder, it loads and instantiates another wasm module when the current module is running. Sounds a little tricky, I am not sure whether we should allow it?

depends on the use cases, it might be appropriate to simply allocate a separate exec_env.
otherwise, i agree it's a bit tricky.