shared heap: Fix some issues and add basic unit test case (#3801)

WenLY1 · web-flow · commit 4dacef2d600d · 2024-09-20T14:24:38.000+08:00
Fix some issues and add basic unit test case for shared heap feature.

Signed-off-by: wenlingyun1 &lt;wenlingyun1@xiaomi.com&gt;
diff --git a/core/iwasm/common/wasm_memory.c b/core/iwasm/common/wasm_memory.c
@@ -148,22 +148,13 @@ static void
 wasm_munmap_linear_memory(void *mapped_mem, uint64 commit_size,
                           uint64 map_size);
 
-static void
-set_error_buf(char *error_buf, uint32 error_buf_size, const char *string)
-{
-    if (error_buf != NULL) {
-        snprintf(error_buf, error_buf_size,
-                 "Operation of shared heap failed: %s", string);
-    }
-}
-
 static void *
-runtime_malloc(uint64 size, char *error_buf, uint32 error_buf_size)
+runtime_malloc(uint64 size)
 {
     void *mem;
 
     if (size >= UINT32_MAX || !(mem = wasm_runtime_malloc((uint32)size))) {
-        set_error_buf(error_buf, error_buf_size, "allocate memory failed");
+        LOG_WARNING("Allocate memory failed");
         return NULL;
     }
 
@@ -172,27 +163,32 @@ runtime_malloc(uint64 size, char *error_buf, uint32 error_buf_size)
 }
 
 WASMSharedHeap *
-wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args, char *error_buf,
-                                uint32 error_buf_size)
+wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args)
 {
     uint64 heap_struct_size = sizeof(WASMSharedHeap);
     uint32 size = init_args->size;
     WASMSharedHeap *heap;
 
-    if (!(heap = runtime_malloc(heap_struct_size, error_buf, error_buf_size))) {
+    if (size == 0) {
         goto fail1;
     }
+
+    if (!(heap = runtime_malloc(heap_struct_size))) {
+        goto fail1;
+    }
+
     if (!(heap->heap_handle =
-              runtime_malloc(mem_allocator_get_heap_struct_size(), error_buf,
-                             error_buf_size))) {
+              runtime_malloc(mem_allocator_get_heap_struct_size()))) {
         goto fail2;
     }
+
+    size = align_uint(size, os_getpagesize());
+    heap->size = size;
     heap->start_off_mem64 = UINT64_MAX - heap->size + 1;
     heap->start_off_mem32 = UINT32_MAX - heap->size + 1;
 
-    size = align_uint(size, os_getpagesize());
     if (size > APP_HEAP_SIZE_MAX || size < APP_HEAP_SIZE_MIN) {
-        set_error_buf(error_buf, error_buf_size, "invalid size of shared heap");
+        LOG_WARNING("Invalid size of shared heap");
         goto fail3;
     }
 
@@ -201,7 +197,7 @@ wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args, char *error_buf,
     }
     if (!mem_allocator_create_with_struct_and_pool(
             heap->heap_handle, heap_struct_size, heap->base_addr, size)) {
-        set_error_buf(error_buf, error_buf_size, "init share heap failed");
+        LOG_WARNING("init share heap failed");
         goto fail4;
     }
 
@@ -365,20 +361,25 @@ wasm_runtime_shared_heap_malloc(WASMModuleInstanceCommon *module_inst,
     WASMMemoryInstance *memory =
         wasm_get_default_memory((WASMModuleInstance *)module_inst);
     WASMSharedHeap *shared_heap = get_shared_heap(module_inst);
+    void *native_addr = NULL;
 
     if (!memory || !shared_heap)
         return 0;
 
-    *p_native_addr = mem_allocator_malloc(shared_heap->heap_handle, size);
-    if (!*p_native_addr)
+    native_addr = mem_allocator_malloc(shared_heap->heap_handle, size);
+    if (!native_addr)
         return 0;
 
+    if (p_native_addr) {
+        *p_native_addr = native_addr;
+    }
+
     if (memory->is_memory64)
         return shared_heap->start_off_mem64
-               + ((uint8 *)*p_native_addr - shared_heap->base_addr);
+               + ((uint8 *)native_addr - shared_heap->base_addr);
     else
         return shared_heap->start_off_mem32
-               + ((uint8 *)*p_native_addr - shared_heap->base_addr);
+               + ((uint8 *)native_addr - shared_heap->base_addr);
 }
 
 void
diff --git a/core/iwasm/common/wasm_memory.h b/core/iwasm/common/wasm_memory.h
@@ -43,8 +43,7 @@ SET_LINEAR_MEMORY_SIZE(WASMMemoryInstance *memory, uint64 size)
 
 #if WASM_ENABLE_SHARED_HEAP != 0
 WASMSharedHeap *
-wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args, char *error_buf,
-                                uint32 error_buf_size);
+wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args);
 
 bool
 wasm_runtime_attach_shared_heap(WASMModuleInstanceCommon *module_inst,
diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c
@@ -4498,6 +4498,11 @@ wasm_runtime_invoke_native_raw(WASMExecEnv *exec_env, void *func_ptr,
                                uint32 *argv, uint32 argc, uint32 *argv_ret)
 {
     WASMModuleInstanceCommon *module = wasm_runtime_get_module_inst(exec_env);
+#if WASM_ENABLE_MEMORY64 != 0
+    WASMMemoryInstance *memory =
+        wasm_get_default_memory((WASMModuleInstance *)module);
+    bool is_memory64 = memory ? memory->is_memory64 : false;
+#endif
     typedef void (*NativeRawFuncPtr)(WASMExecEnv *, uint64 *);
     NativeRawFuncPtr invoke_native_raw = (NativeRawFuncPtr)func_ptr;
     uint64 argv_buf[16] = { 0 }, *argv1 = argv_buf, *argv_dst, size, arg_i64;
@@ -4525,11 +4530,11 @@ wasm_runtime_invoke_native_raw(WASMExecEnv *exec_env, void *func_ptr,
 #endif
             {
                 *(uint32 *)argv_dst = arg_i32 = *argv_src++;
-                /* TODO: memory64 if future there is a way for supporting
-                 * wasm64 and wasm32 in libc at the same time, remove the
-                 * macro control */
-#if WASM_ENABLE_MEMORY64 == 0
-                if (signature) {
+                if (signature
+#if WASM_ENABLE_MEMORY64 != 0
+                    && !is_memory64
+#endif
+                ) {
                     if (signature[i + 1] == '*') {
                         /* param is a pointer */
                         if (signature[i + 2] == '~')
@@ -4558,7 +4563,6 @@ wasm_runtime_invoke_native_raw(WASMExecEnv *exec_env, void *func_ptr,
                                 module, (uint64)arg_i32);
                     }
                 }
-#endif
                 break;
             }
             case VALUE_TYPE_I64:
@@ -4568,7 +4572,7 @@ wasm_runtime_invoke_native_raw(WASMExecEnv *exec_env, void *func_ptr,
                                 GET_I64_FROM_ADDR(argv_src));
                 argv_src += 2;
                 arg_i64 = *argv_dst;
-                if (signature) {
+                if (signature && is_memory64) {
                     /* TODO: memory64 pointer with length need a new symbol
                      * to represent type i64, with '~' still represent i32
                      * length */
@@ -4729,9 +4733,6 @@ wasm_runtime_invoke_native_raw(WASMExecEnv *exec_env, void *func_ptr,
 fail:
     if (argv1 != argv_buf)
         wasm_runtime_free(argv1);
-#if WASM_ENABLE_MEMORY64 == 0
-    (void)arg_i64;
-#endif
     return ret;
 }
 
@@ -5655,6 +5656,11 @@ wasm_runtime_invoke_native(WASMExecEnv *exec_env, void *func_ptr,
                            uint32 *argv_ret)
 {
     WASMModuleInstanceCommon *module = wasm_runtime_get_module_inst(exec_env);
+#if WASM_ENABLE_MEMORY64 != 0
+    WASMMemoryInstance *memory =
+        wasm_get_default_memory((WASMModuleInstance *)module);
+    bool is_memory64 = memory ? memory->is_memory64 : false;
+#endif
     uint64 argv_buf[32] = { 0 }, *argv1 = argv_buf, *ints, *stacks, size,
            arg_i64;
     uint32 *argv_src = argv, i, argc1, n_ints = 0, n_stacks = 0;
@@ -5720,11 +5726,11 @@ wasm_runtime_invoke_native(WASMExecEnv *exec_env, void *func_ptr,
             {
                 arg_i32 = *argv_src++;
                 arg_i64 = arg_i32;
-                /* TODO: memory64 if future there is a way for supporting
-                 * wasm64 and wasm32 in libc at the same time, remove the
-                 * macro control */
-#if WASM_ENABLE_MEMORY64 == 0
-                if (signature) {
+                if (signature
+#if WASM_ENABLE_MEMORY64 != 0
+                    && !is_memory64
+#endif
+                ) {
                     if (signature[i + 1] == '*') {
                         /* param is a pointer */
                         if (signature[i + 2] == '~')
@@ -5751,7 +5757,6 @@ wasm_runtime_invoke_native(WASMExecEnv *exec_env, void *func_ptr,
                             module, (uint64)arg_i32);
                     }
                 }
-#endif
                 if (n_ints < MAX_REG_INTS)
                     ints[n_ints++] = arg_i64;
                 else
@@ -5763,7 +5768,7 @@ wasm_runtime_invoke_native(WASMExecEnv *exec_env, void *func_ptr,
             {
                 arg_i64 = GET_I64_FROM_ADDR(argv_src);
                 argv_src += 2;
-                if (signature) {
+                if (signature && is_memory64) {
                     /* TODO: memory64 pointer with length need a new symbol
                      * to represent type i64, with '~' still represent i32
                      * length */
diff --git a/core/iwasm/include/wasm_export.h b/core/iwasm/include/wasm_export.h
@@ -323,11 +323,9 @@ typedef enum {
     WASM_LOG_LEVEL_VERBOSE = 4
 } log_level_t;
 
-#if WASM_ENABLE_SHARED_HEAP != 0
 typedef struct SharedHeapInitArgs {
-    uint32 size;
+    uint32_t size;
 } SharedHeapInitArgs;
-#endif
 
 /**
  * Initialize the WASM runtime environment, and also initialize
@@ -2119,51 +2117,59 @@ wasm_runtime_detect_native_stack_overflow_size(wasm_exec_env_t exec_env,
 WASM_RUNTIME_API_EXTERN bool
 wasm_runtime_is_underlying_binary_freeable(const wasm_module_t module);
 
-#if WASM_ENABLE_SHARED_HEAP != 0
 /**
  * Create a shared heap
+ *
  * @param init_args the initialization arguments
- * @param error_buf buffer to output the error info if failed
- * @param error_buf_size the size of the error buffer
+ * @return the shared heap created
  */
 WASM_RUNTIME_API_EXTERN wasm_shared_heap_t
-wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args, char *error_buf,
-                                uint32 error_buf_size);
+wasm_runtime_create_shared_heap(SharedHeapInitArgs *init_args);
 
 /**
  * Attach a shared heap to a module instance
+ *
  * @param module_inst the module instance
  * @param shared_heap the shared heap
+ * @return true if success, false if failed
  */
 WASM_RUNTIME_API_EXTERN bool
 wasm_runtime_attach_shared_heap(wasm_module_inst_t module_inst,
                                 wasm_shared_heap_t shared_heap);
 
 /**
  * Detach a shared heap from a module instance
+ *
  * @param module_inst the module instance
  */
 WASM_RUNTIME_API_EXTERN void
 wasm_runtime_detach_shared_heap(wasm_module_inst_t module_inst);
 
 /**
  * Allocate memory from a shared heap
+ *
  * @param module_inst the module instance
  * @param size required memory size
  * @param p_native_addr native address of allocated memory
+ *
+ * @return return the allocated memory address, which re-uses part of the wasm
+ * address space and is in the range of [UINT32 - shared_heap_size + 1, UINT32]
+ * (when the wasm memory is 32-bit) or [UINT64 - shared_heap_size + 1, UINT64]
+ * (when the wasm memory is 64-bit). Note that it is not an absolute address.
+ *         Return non-zero if success, zero if failed.
  */
-WASM_RUNTIME_API_EXTERN uint64
-wasm_runtime_shared_heap_malloc(wasm_module_inst_t module_inst, uint64 size,
+WASM_RUNTIME_API_EXTERN uint64_t
+wasm_runtime_shared_heap_malloc(wasm_module_inst_t module_inst, uint64_t size,
                                 void **p_native_addr);
 
 /**
  * Free the memory allocated from shared heap
+ *
  * @param module_inst the module instance
  * @param ptr the offset in wasm app
  */
 WASM_RUNTIME_API_EXTERN void
-wasm_runtime_shared_heap_free(wasm_module_inst_t module_inst, uint64 ptr);
-#endif
+wasm_runtime_shared_heap_free(wasm_module_inst_t module_inst, uint64_t ptr);
 
 #ifdef __cplusplus
 }
diff --git a/core/iwasm/interpreter/wasm_runtime.h b/core/iwasm/interpreter/wasm_runtime.h
@@ -97,7 +97,7 @@ typedef struct WASMSharedHeap {
     struct WASMSharedHeap *next;
     void *heap_handle;
     uint8 *base_addr;
-    uint32 size;
+    uint64 size;
     uint64 start_off_mem64;
     uint64 start_off_mem32;
 } WASMSharedHeap;
diff --git a/core/iwasm/libraries/shared-heap/shared_heap_wrapper.c b/core/iwasm/libraries/shared-heap/shared_heap_wrapper.c
@@ -31,8 +31,10 @@ shared_free_wrapper(wasm_exec_env_t exec_env, void *ptr)
 {
     wasm_module_inst_t module_inst = get_module_inst(exec_env);
 
-    if (!validate_native_addr(ptr, (uint64)sizeof(uint32)))
+    if (!validate_native_addr(ptr, (uint64)sizeof(uintptr_t))) {
+        LOG_WARNING("Invalid app address");
         return;
+    }
 
     module_shared_free(addr_native_to_app(ptr));
 }
diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt
@@ -50,3 +50,4 @@ add_subdirectory(linux-perf)
 add_subdirectory(gc)
 add_subdirectory(memory64)
 add_subdirectory(tid-allocator)
+add_subdirectory(shared-heap)
diff --git a/tests/unit/shared-heap/CMakeLists.txt b/tests/unit/shared-heap/CMakeLists.txt
@@ -0,0 +1,59 @@
+# Copyright (C) 2024 Xiaomi Corporation.  All rights reserved.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+cmake_minimum_required(VERSION 3.14)
+
+project(test-shared-heap)
+
+add_definitions(-DRUN_ON_LINUX)
+
+set(WAMR_BUILD_APP_FRAMEWORK 0)
+set(WAMR_BUILD_AOT 0)
+set(WAMR_BUILD_INTERP 1)
+set(WAMR_BUILD_FAST_INTERP 1)
+set(WAMR_BUILD_JIT 0)
+set(WAMR_BUILD_MEMORY64 1)
+set(WAMR_BUILD_SHARED_HEAP 1)
+
+# Compile wasm modules
+add_subdirectory(wasm-apps)
+
+# if only load this CMake other than load it as subdirectory
+include(../unit_common.cmake)
+
+set(LLVM_SRC_ROOT "${WAMR_ROOT_DIR}/core/deps/llvm")
+
+if (NOT EXISTS "${LLVM_SRC_ROOT}/build")
+    message(FATAL_ERROR "Cannot find LLVM dir: ${LLVM_SRC_ROOT}/build")
+endif ()
+
+set(CMAKE_PREFIX_PATH "${LLVM_SRC_ROOT}/build;${CMAKE_PREFIX_PATH}")
+find_package(LLVM REQUIRED CONFIG)
+include_directories(${LLVM_INCLUDE_DIRS})
+add_definitions(${LLVM_DEFINITIONS})
+message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
+message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")
+
+include(${IWASM_DIR}/compilation/iwasm_compl.cmake)
+
+include_directories(${CMAKE_CURRENT_SOURCE_DIR})
+
+file(GLOB_RECURSE source_all ${CMAKE_CURRENT_SOURCE_DIR}/*.cc)
+
+set(UNIT_SOURCE ${source_all})
+
+aux_source_directory(. SRC_LIST)
+
+set(unit_test_sources
+        ${UNIT_SOURCE}
+        ${WAMR_RUNTIME_LIB_SOURCE}
+        ${UNCOMMON_SHARED_SOURCE}
+        ${SRC_LIST}
+        )
+
+# Now simply link against gtest or gtest_main as needed. Eg
+add_executable(shared_heap_test ${unit_test_sources})
+
+target_link_libraries(shared_heap_test ${LLVM_AVAILABLE_LIBS} gtest_main)
+
+gtest_discover_tests(shared_heap_test)
diff --git a/tests/unit/shared-heap/shared_heap_test.cc b/tests/unit/shared-heap/shared_heap_test.cc
diff --git a/tests/unit/shared-heap/wasm-apps/CMakeLists.txt b/tests/unit/shared-heap/wasm-apps/CMakeLists.txt
diff --git a/tests/unit/shared-heap/wasm-apps/test.c b/tests/unit/shared-heap/wasm-apps/test.c

Original file line number	Diff line number	Diff line change
`@@ -31,8 +31,10 @@ shared_free_wrapper(wasm_exec_env_t exec_env, void *ptr)`
`31`	`31`	`{`
`32`	`32`	`wasm_module_inst_t module_inst = get_module_inst(exec_env);`
`33`	`33`
`34`		`- if (!validate_native_addr(ptr, (uint64)sizeof(uint32)))`
	`34`	`+ if (!validate_native_addr(ptr, (uint64)sizeof(uintptr_t))) {`
	`35`	`+ LOG_WARNING("Invalid app address");`
`35`	`36`	`return;`
	`37`	`+ }`
`36`	`38`
`37`	`39`	`module_shared_free(addr_native_to_app(ptr));`
`38`	`40`	`}`