[C API] Add refcount to wasm_engine_t singleton

[lucianoiam]

This patch allows safer (note: safer, not safe) embedding in a plugin
environment where multiple instances of the engine could be needed.

Original code initializes and tears down the full runtime during
wasm_engine_new() and wasm_engine_delete() respectively. After this
update the C API implementation keeps track of engine instances count
and inits/deinits the runtime only when needed.

This allows for example to call wasm_engine_new() twice and then call
wasm_engine_delete() once without rendering the first engine instance
invalid.

[lucianoiam]

Additional note absent from commit message: unmodified wasmer's C API implementation already allows to safely create and destroy multiple engine instances.

[lum1n0us]

This allows for example to call wasm_engine_new() twice and then call wasm_engine_delete() once without rendering the first engine instance invalid.

I am worried that unpaired wasm_engine_new() and wasm_engine_delete() calling is not a right and expected behavior. Callers should guarantee they will call wasm_XXX_delete() after wasm_XXX_new() just like they should free() every piece memory fragment allocated by malloc()

[lucianoiam]

Thanks for reviewing!

I agree new/delete calls should be paired otherwise memory leaks can occur. This patch basically allows this:

/* ABBA */

wasm_engine_t* a = wasm_engine_new();
wasm_engine_t* b = wasm_engine_new();
wasm_engine_delete(b);
// Continue working with engine A...
wasm_engine_delete(a);

Right now any attempts to do the above result in unexpected behavior because:

wasm_engine_delete(b);  // makes both A and B invalid

The C API is enforcing the following implicit convention:

/* AABB */

wasm_engine_t* a = wasm_engine_new();
wasm_engine_delete(a);
wasm_engine_t* b = wasm_engine_new();
wasm_engine_delete(b);

In other words in the current implementation the engine new/delete calls are not balanced, hence it is only possible to have a single engine instance at a given time. (maybe this is a requirement of the WASM C API standard I am not aware of).

The patch makes sure the runtime is not destroyed until the last call to wasm_engine_delete() has been performed.

[lucianoiam]

I made some edits to the examples above to correct errors and make it more explicit

[lum1n0us]

Thanks. It does make sense. And it leaves us another problem.

Although there is no accurate concept of an engine, it tends to be a logical object to represent runtime, or wasm_runtime. So engine should be at the top level, above store. In other words, wasm_store_t should not include wasm_engine_t to avoid bio-direction association (wasm_engine_t <-> wasm_store_t). The question here is how to store and keep tracing all-created engines.

Maybe a global engine list (wasm_engine_vec_t) will work. Let me know your idea.

[lucianoiam]

There are two different issues here actually:

• Who calls and when to call wasm_runtime_full_init() and wasm_runtime_destroy().
• Current wasm_store_delete() implementation requires access to the engine to remove itself from the list of the stores in the engine

I thought of a somehow pragmatic approach for addressing #1 which is decoupling the library itself from the engine/runtime at a code level. Treat the former as a WAMR implementation detail, and engine as a standard entity which is already represented by wasm_engine_t.

That would require two WAMR custom functions like wamr_init() and wamr_deinit(). The architecture looks more explicit this way, wamr_* deal with library things that not necessarily have to do with WebAssembly (setting OS handlers, etc). Then there would not be need for global/static data I think.

I must be honest here in the sense that I pursue the implementation with the least global data as possible since it is a requirement in my context. It sounds like a desirable concept anyways unless it makes the implementation over-complicated.

As a bonus wasm_engine_new() and wasm_engine_delete() would behave exactly the same any time they are called no matter the number of existing engines. But this is really a cosmetic detail, and pragmatism goes out of the window :)

A global engine list also works, and I've already seen lists implemented internally for keeping track of created stores.

For #2 I agree the reference cycle does not look very clean but having multiple engine instances instead of the current singleton means there is no other way for accessing the engine stores list from wasm_store_delete(). Unless you scan the global engines vector until hitting the correct engine though it sounds too complex.

Less invasive option could be leaving things as they are now (singleton included) and add a reference counter to know when to call wasm_runtime_full_init() and wasm_runtime_full_destroy(). That would be similar to this proposed patch, but leaving the singleton to avoid the engine<->store double reference. Total globals: one wasm_engine_t* and one int.

Please take all this with a grain of salt as my experience with WASM and WAMR is scarce.

[lum1n0us]

wasm_engine_t is a representation of wasm_runtime and an entry of the library. And it is why it is a singleton at the very beginning. And the problem here is when users choose to call wasm_engine_new and wasm_engine_delete multiple times in a form like "ABAB", the first wasm_engine_delete releases the singleton and cause all secondary releasing failed. It looks good to land a count but it may be a little be confused to create multiple engines since there is actually one wasm_runtime and one entry logically. So how about using a reference count instead of an instance count?

struct wasm_engine_t {
  // ...
  uint32_t reference_counts;
};

[lucianoiam]

wasm_engine_t is a representation of wasm_runtime and an entry of the library. And it is why it is a singleton at the very beginning.

This is clear now, thanks.

And the problem here is when users choose to call wasm_engine_new and wasm_engine_delete multiple times in a form like "ABAB", the first wasm_engine_delete releases the singleton and cause all secondary releasing failed. It looks good to land a count but it may be a little be confused to create multiple engines since there is actually one wasm_runtime and one entry logically. So how about using a reference count instead of an instance count?

struct wasm_engine_t {
  // ...
  uint32_t reference_counts;
};

Looks elegant and solves the problem!

I think we have a similar issue with wasm_store_new(), calling this function twice causes this to be executed twice:

wasm-micro-runtime/core/iwasm/common/wasm_c_api.c

Line 385 in c94e752

if (!wasm_runtime_init_thread_env()) {

I recall having to disable OS_ENABLE_HW_BOUND_CHECK on macOS to avoid crashing on the second call. I think the crash was on os_thread_signal_init(), sorry for the vagueness I'd need to create a test. Anyways, the store can be also refcounted to avoid calling single-time things more than once. Or probably move all runtime lifecycle calls (like wasm_runtime_init_thread_env()) to the wasm_engine_new() and wasm_engine_delete() functions?

[lum1n0us]

Nice catch. The big assumption about one engine and one store has been broken. And we will see some crashing related to that. Maybe ref-counters can help them. But let me do some investigation first.

Since it needs some tough work to fix all those, I am super curious about scenarios of multiple calling. Is it a multiple threads case?

[lucianoiam]

It is an audio plugin architecture where each plugin holds its own WASM engine. A plugin can be loaded multiple times into the same process (the DAW). The plugins are packaged into shared libraries with a standard interface (VST, LV2, etc). The goal is to run DSP modules precompiled from AssemblyScript.

So if I correctly understand what is going on, loading two instances of a plugin results in calling wasm_store_new() twice during the host process lifetime, causing trouble on some platforms.

This setup also reveals my previous concern with global/static data :)

There is some threading involved. The host can call the plugin from multiple threads, including realtime. These native calls result in wasm_func_call() and are currently protected by a lock.

If there is some obvious downside in doing this or recommendations, I'd be happy to know about them!

[lum1n0us]

I'd like to remove the possibility of "thread-unsafe bh_vector", please try the crash with a patch and see where it goes.

In wasm-c-api, we tend to follow the thread model.

• an wasm_engine_t instance may only be created once per process
• Every wasm_store_t and its objects must only be accessed in a single thread

[lucianoiam]

I'd like to remove the possibility of "thread-unsafe bh_vector", please try the crash with a patch and see where it goes.

In wasm-c-api, we tend to follow the thread model.

• an wasm_engine_t instance may only be created once per process

Thanks for that link... wish I had seen it earlier :) . That statement is pretty clear and explains the singleton. Still #1001 (comment) sounds good to have.

• Every wasm_store_t and its objects must only be accessed in a single thread

Mm I think I accidentally made things worse for my specific setup. The patch contains new os_mutex_* calls. Unfortunately these sound like a no-go in a realtime context. Doesn't it make more sense to leave threading mgmt to the application? or make it optional.

Also note the crashes weren't related to concurrency, access to WAMR was already synchronized using a spin lock. The crash happens when calling wasm_store_new() twice during the process lifetime and WAMR_DISABLE_HW_BOUND_CHECK=0. wasm_store_new() is doing some global initialization that causes a crash (macOS at least) on the second call. The C API link above says _ Multiple stores can be created_ so it looks like a different issue.

[lucianoiam]

-DWAMR_BUILD_LIBC_WASI=0 -DWAMR_BUILD_AOT=1 -DWAMR_BUILD_JIT=0 DWAMR_BUILD_INTERP=1 -DWAMR_DISABLE_HW_BOUND_CHECK=0

(lldb) bt
warning: could not execute support code to read Objective-C class data in the process. This may reduce the quality of type information available.
* thread #1, name = 'reaper', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x7ff7bf703ea0)
  * frame #0: 0x00000001109c22d0 d_jitdrum`touch_pages + 112
    frame #1: 0x00000001109c1f80 d_jitdrum`os_thread_signal_init + 192
    frame #2: 0x00000001109e94f0 d_jitdrum`aot_signal_init + 16
    frame #3: 0x00000001109c708f d_jitdrum`wasm_store_new + 47
    frame #4: 0x00000001109a9ae6 d_jitdrum`DISTRHO::WasmRuntime::WasmRuntime(this=0x0000600003071200) at WasmRuntime.cpp:46:14 [opt]

[lum1n0us]

Well, it is an interesting call stack. I am afraid I am going to ask for more details.

• is there any output log when crashing? I believe you noticed there are os_printf for every failed branch in os_thread_signal_init. hope one of them was triggered.
• is calling wasm_store_new() twice in one thread the only condition to trigger the crashing? I have a test case to new/delete in the main thread and 10 spawned threads multiple times and only catch the concurrence problem.
• it's better to check some stack related stuff:
  • parameters of touch_page. they are stack_min_addr and page_size

[lum1n0us]

Here is my test code, please take a look

[lucianoiam]

Thanks very much for the time on this and the test program. AOT is also crashing for my particular setup on Windows (MinGW build) but for different reasons, so it makes sense to have a thorough look before writing an answer.

[lucianoiam]

Well, it is an interesting call stack. I am afraid I am going to ask for more details.

Sure, very eager to contribute on this. I managed to create a minimal project that simulates the host/plugin environment and recreates the exact issue experienced on a real DAW with my VST plugin, more on that below.

• is there any output log when crashing? I believe you noticed there are os_printf for every failed branch in os_thread_signal_init. hope one of them was triggered.

No error messages printed. I think this is expected because WAMR does not even have a chance to test for failure in this case. The crashing call is alloca(). I determined this with the help of lldb + adding custom log lines. More on this below.

• is calling wasm_store_new() twice in one thread the only condition to trigger the crashing?

Made this specific test and no, calling it twice from the same thread works without issues.

I have a test case to new/delete in the main thread and 10 spawned threads multiple times and only catch the concurrence problem.

I tried threads.c and crashes maybe 1 out of 10 times. The crash happens in a different place compared to my project's, in wasm_store_delete() and not wasm_store_new():

(lldb) run
Process 16356 launched: '/Users/luciano/src/hiphop/a.out' (x86_64)
touch_pages(stack_min_addr=0x7ff7bf701000 page_size=4096)
> Test wasm_store ABCDABCD! in main 
Initializing thread 0...
Initializing thread 1...
Initializing thread 2...
Initializing thread 3...
Initializing thread 4...
touch_pages(stack_min_addr=0x70000d89b000 page_size=4096)
touch_pages(stack_min_addr=0x70000d91e000 page_size=4096)
Initializing thread 5...
touch_pages(stack_min_addr=0x70000da24000 page_size=4096)
touch_pages(stack_min_addr=0x70000daa7000 page_size=4096)
Initializing thread 6...
touch_pages(stack_min_addr=0x70000d9a1000 page_size=4096)
Initializing thread 7...
Initializing thread 8...
touch_pages(stack_min_addr=0x70000db2a000 page_size=4096)
Initializing thread 9...
Waiting for thread: 0
touch_pages(stack_min_addr=0x70000dbad000 page_size=4096)
> Test wasm_store ABCDABCD! in the thread 
touch_pages(stack_min_addr=0x70000dc30000 page_size=4096)
touch_pages(stack_min_addr=0x70000dd36000 page_size=4096)
> Test wasm_store ABCDABCD! in the thread 
touch_pages(stack_min_addr=0x70000dcb3000 page_size=4096)
> Test wasm_store ABCDABCD! in the thread 
Waiting for thread: 1
> Test wasm_store ABCDABCD! in the thread 
> Test wasm_store ABCDABCD! in the thread 
> Test wasm_store ABCDABCD! in the thread 
Waiting for thread: 2
> Test wasm_store ABCDABCD! in the thread 
Waiting for thread: 3
Waiting for thread: 4
Waiting for thread: 5
Waiting for thread: 6
Waiting for thread: 7
> Test wasm_store ABCDABCD! in the thread 
> Test wasm_store ABCDABCD! in the thread 
Waiting for thread: 8
> Test wasm_store ABCDABCD! in the thread 
Waiting for thread: 9
Process 16356 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x000000010000ad87 a.out`wasm_store_delete + 119
a.out`wasm_store_delete:
->  0x10000ad87 <+119>: movq   0x10(%r15), %rax
    0x10000ad8b <+123>: testq  %rax, %rax
    0x10000ad8e <+126>: je     0x10000adfc               ; <+236>
    0x10000ad90 <+128>: xorl   %ebx, %ebx
Target 0: (a.out) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010000ad87 a.out`wasm_store_delete + 119
    frame #1: 0x000000010000b43d a.out`wasm_engine_delete + 61
    frame #2: 0x0000000100002ff7 a.out`main(argc=1, argv=0x00007ff7bfeff918) at threads.c:81:5
    frame #3: 0x00000001000394fe dyld`start + 462
(lldb) 

Note: threads.c ends gracefully most of the times.

• it's better to check some stack related stuff:

  • parameters of touch_page. they are stack_min_addr and page_size

stack_min_addr is always non-NULL and page_size 4096 on my system (macOS 12.2.1 x86_64)

The alloca() crash (the "original" issue in my project) happens when code loaded from two different shared library files attempts to call wasm_store_new(). My knowledge of how runtime linking and the stack work is limited so frankly I don't know where to start looking. Note that creating multiple instances from the same .dylib does not produce a crash, only when a second .dylib is loaded. Both libraries contain identical code. Everything is commented in the source files:

wamr_test_macos-x86_64.zip

To run:

make && ./host

libvmlib.a compiled with -DWAMR_BUILD_LIBC_WASI=0 -DWAMR_DISABLE_HW_BOUND_CHECK=0 -DWAMR_BUILD_AOT=1 -DWAMR_BUILD_INTERP=0

Note: host.c ALWAYS crashes

The issue on MinGW looks totally different, more on that later.

Edit: the WAMR flags were incorrect, the attached static lib was compiled with -DWAMR_DISABLE_HW_BOUND_CHECK=0 . Worth noting that setting this to 1 fixes the crash.

[lum1n0us]

Thanks a lot for exposing that wired problem. It is about a restriction about TLS and dynamic libraries loading on MacOS. I've updated some test code on gist. It includes CMakeLists.txt, main.c and plugin.c.

I will keep you posted when I learne more or figure out how to fix that.

[lucianoiam]

Great. Some more evidence:

• Cannot reproduce the issue on Linux using the test attached before your CMake version (slightly modified to let it compile). But I do experience some other glitches in my "real" project, not crashes but unexpected behavior. These issues are also gone with DWAMR_DISABLE_HW_BOUND_CHECK=1. I'll hold on until there's something new to check on Mac, maybe that fixes Linux too.

• On MinGW the scenario looks different, my project crashes at invokeNative() when AOT is enabled. libvm is statically linked to my project. I temporarily solved the issue by loading a MSVC-built DLL during runtime instead. Worth mentioning the MinGW libvmlib.a uses the C version of invokeNative() because I've been lazy to find out how to compile the ASM version on MinGW. The MSVC DLL uses ASM, it was built following the official instructions on Windows.

Generally speaking, I found workarounds for everything in my specific use case. The performance gains using AOT are huge.

[lum1n0us]

A simple answer is: Every plugin should link with a dynamic library libiwasm.so instead of a static library libvmlib.a.

More details:
Those are observations from a simple test case. The case includes an executable main.c, a shared librariey plugin.c(will be dlopen() by main.c), a third library third.c(will be linked with plugin.c in two ways). The third.c mimics the libiwasm.so and libvmlib.a in this little story.

The test case creates two shared libraries with plugin.c, they are lib_a_xx and lib_b_xx. After loading both plugin libraries, main.c will compare:

• the tls variable address in lib_a and lib_b
• the normal variable address in lib_a and lib_b
• the external tls variable in lib_a and lib_b. It is defined in the dependency of the plugin
• the initialize value of local and external tls variables in lib_a and lib_b
• set and get local and external tls variables in lib_a and lib_b

The test case will run twice with two different conditions:

• plugin links third staticlly
• plugin links third dynamically

Run the test case on Ubuntu and MacOS and get observations:

• On both Ubuntu and MacOS, local variables(include tls variables and normal variables) in lib_a and lib_b are in different addresses. So can be operated separately.
• On Ubuntu, external tls variables(from third.c) in lib_a and lib_b are always in the same addresses. They can not be operated separately. Even they are from different libraries.
• On MacOS, if external tls variables are from a statically linked library, lib_a and lib_b have different symbol addresses for tls variables. They will be operated separately. This is the root cause of the crashing.
• On MacOS, if external tls variables are from a dynamic linked library, lib_a and lib_b have same symbol addresses for tls variables. They will be not operated separately. This is the right mode we are expecting.

In you are still interested, here are some thoughts:

• In third-library-dynamic-link mode, on both Ubuntu and MacOS, the loader will only load lib_third.so once and assign one module id to lib_third.so. A module in glibc can refer to either an executable or a dynamically shared object, a module ID, therefore, is an index number for a loaded ELF object in a process. In this case, external TLS variables of lib_a and lib_b are from the same TLS blocks for dynamically-loaded modules(please refer to https://uclibc.org/docs/tls.pdf). Because lib_third.so is loaded after main() execution.

• In third-library-static-link mode, it seems MacOS acts in the right way. Because under the static-linking, external TLS variables of lib_a and lib_b should be treated like their private own variables and they should have different addresses.

On Ubuntu, if we take a close look at the assemble code of lib_a or lib_b, we will see:

tls_get_addr is used to retrieve the address of a tls variable

0000000000001260 <get_tls>:
   1260:   55                      push   %rbp
   1261:   48 89 e5                mov    %rsp,%rbp
   1264:   48 8d 3d 65 2d 00 00    lea    0x2d65(%rip),%rdi        # 3fd0 <.got>
   126b:   e8 d0 fd ff ff          callq  1040 <__tls_get_addr@plt>
   1270:   8b 80 04 00 00 00       mov    0x4(%rax),%eax
   1276:   5d                      pop    %rbp
   1277:   c3                      retq

3fd0 is the address of a R_X86_64_DTPMOD64 relocation.

DYNAMIC RELOCATION RECORDS
OFFSET           TYPE              VALUE
0000000000003de0 R_X86_64_RELATIVE  *ABS*+0x0000000000001130
0000000000003de8 R_X86_64_RELATIVE  *ABS*+0x00000000000010f0
0000000000004038 R_X86_64_RELATIVE  *ABS*+0x0000000000004038
0000000000003fd0 R_X86_64_DTPMOD64  *ABS*
0000000000003fe0 R_X86_64_GLOB_DAT  _ITM_deregisterTMCloneTable
0000000000003fe8 R_X86_64_GLOB_DAT  __gmon_start__
0000000000003ff0 R_X86_64_GLOB_DAT  _ITM_registerTMCloneTable
0000000000003ff8 R_X86_64_GLOB_DAT  __cxa_finalize@GLIBC_2.2.5
0000000000004018 R_X86_64_JUMP_SLOT  set_tls_third@@Base
0000000000004020 R_X86_64_JUMP_SLOT  __tls_get_addr@GLIBC_2.3
0000000000004028 R_X86_64_JUMP_SLOT  get_tls_third@@Base
0000000000004030 R_X86_64_JUMP_SLOT  get_tls_addr_third@@Base

__tls_get_addr will return the tls variable address and depends on its first input argument which is the content of the R_X86_64_DTPMOD64 relocation entry.

By tracing the code, I realize the dynamic linker will put a l_tls_modid in it.

  case R_X86_64_DTPMOD64:
    /* Get the information from the link map returned by the
       resolve function.  */
    if (sym_map != NULL)
      *reloc_addr = sym_map->l_tls_modid;
    break;

That leaves us the finally problem, why "those tls variables from different libraries (although from the same static library)" have the same l_tls_modid? I am still looking for a answer for that.

[lucianoiam]

Thanks for the detailed analysis. I don't see a lot deeper than the tip of the iceberg here and unfortunately there is not much more I can contribute other than testing and reporting back. This also confirms my initial thought that it is not a trivial issue to solve.

Since mine is a somehow edge case, thinking practically, the first simple answer is already satisfying (do not link static). In fact I'm already doing this on Windows (my MinGW libiwasm.a build just does not work when AOT is enabled). Approach could be easily replicated on Linux and Mac. Fixing everything on every platform combination might be possible but I'm unsure about the need other than curiosity. I'm really repeating a mantra (prefer static for plugins), dynamic linking could come with its own side effects but I have to see those yet. That should be really studied on a case by case basis. Also it is a good opportunity to dig into OS internals.

Not sure this can be "fixed" or just regarded as part of the operating conditions, at any case this is already great documentation for other projects with similar setups.

Does it make sense that DWAMR_DISABLE_HW_BOUND_CHECK=1 fixes the issue, or is it just a coincidence and should not rely on it?

[lum1n0us]

bool
wasm_runtime_init_thread_env(void)
{
#ifdef BH_PLATFORM_WINDOWS
    if (os_thread_env_init() != 0)
        return false;
#endif

#if WASM_ENABLE_AOT != 0
#ifdef OS_ENABLE_HW_BOUND_CHECK   // => all from here
    if (!aot_signal_init()) {
#ifdef BH_PLATFORM_WINDOWS
        os_thread_env_destroy();
#endif
        return false;
    }
#endif
#endif
    return true;
}

WAMR_DISABLE_HW_BOUND_CHECK=1 will undefined OS_ENABLE_HW_BOUND_CHECK and will not call aot_signal_init() and will not call os_thread_signal_init () and touch_pages and will not trigger the EXC_BAD_ACCESS stop.

[lucianoiam]

Thank you, yes that is right. I wasn't sure about other implications -- so far things work smoothly with that setting.

[lum1n0us]

Hi, there. May I ask if this patch is still necessary for your current solution? If yes, does it target the singleton wasm_engine_t?

[lucianoiam]

Hi, my project does not depend on this very same patch but includes a different workaround .

So far the best solution seems to be what you proposed above, which has the same effect as my workaround but allows to embed WAMR without special care:

wasm_engine_t is a representation of wasm_runtime and an entry of the library. And it is why it is a singleton at the very beginning. And the problem here is when users choose to call wasm_engine_new and wasm_engine_delete multiple times in a form like "ABAB", the first wasm_engine_delete releases the singleton and cause all secondary releasing failed. It looks good to land a count but it may be a little be confused to create multiple engines since there is actually one wasm_runtime and one entry logically. So how about using a reference count instead of an instance count?

struct wasm_engine_t {
  // ...
  uint32_t reference_counts;
};

Maybe I should close this PR and eventually start a new one? the title is now misleading too.

[lum1n0us]

I would prefer to update this PR with the new patch and change the title.

[lucianoiam]

Just force-pushed a commit that adds a reference counter to wasm_engine_t that is used for deciding whether wasm_engine_delete() must tear down the full WAMR runtime or not. Reflects the new title of this PR.

[wenyongh]

Hi, currently wasm-c-api APIs are not thread-safe, if to make them thread-safe, there might be more work to do. If you insist on it, could you please add a global lock, initialize it and lock/unlock it in the operations related to singleton:
after L314, define: static korp_mutex c_api_global_lock;
and here: use os_mutex_init(&c_api_global_lock) to initialize it, and check return value

We will merge #1010 to ensure the vector related operations be safe-thread, but if there is other issue occurring, we need to fix it.