[riot-notifications] [RIOT-OS/RIOT] WIP: pkg/wamr: add WAMR to provide WASM support in RIOT (#15329)

Marian Buschsieweke notifications at github.com
Thu Sep 23 12:07:48 CEST 2021


@maribu commented on this pull request.

some more comments and the style nits of the previous round are still open.

Would you mind moving the convenience functions out of the example? E.g. you could create a `wamr` module (which depends on the `wamr` package) that adds RIOT specific convenience stuff. Or you could just add a `contrib` and a `include` folder into the package and add code and headers there. Whatever you consider cleaner.

> +#LINK_FLAGS_bak =\
+#		-nostartfiles \
+#		--nostdlib \
+#		--nostdinc \
+#		--print-map\
+#		--initial-memory= \
+#		--allow-undefined-file=<value>
+#		--initial-memory=65536
+

```suggestion
```

> +# --export-all           Export all symbols (normally combined with --no-gc-sections)
+# --export-dynamic       Put symbols in the dynamic symbol table
+# --export-table         Export function table to the environment
+# --export=<value>       Force a symbol to be exported
+# --fatal-warnings       Treat warnings as errors
+# --import-memory        Import memory from the environment
+# --import-table         Import function table from the environment
+# --initial-memory=<value>
+#                        Initial size of the linear memory
+# --lto-O<opt-level>     Optimization level for LTO
+# --lto-partitions=<value>
+#                        Number of LTO codegen partitions
+# -L <dir>               Add a directory to the library search path
+# -l <libName>           Root name of library to use
+# --max-memory=<value>   Maximum size of the linear memory
+# --merge-data-segments  Enable merging data segments
+# --mllvm <value>        Options to pass to LLVM
+# --pie                  Create a position independent executable
+# --print-gc-sections    List removed unused sections
+# --relocatable          Create relocatable object file
+# --shared-memory        Use shared linear memory
+# --shared               Build a shared object
+# --stack-first          Place stack at start of linear memory rather than after data
+# --strip-all            Strip all symbols
+# --strip-debug          Strip debugging information
+# -S                     Alias for --strip-debug
+# -s                     Alias for --strip-all
+# --thinlto-cache-dir=<value>
+#                        Path to ThinLTO cached object file directory
+# --thinlto-cache-policy=<value>
+#                        Pruning policy for the ThinLTO cache
+# --thinlto-jobs=<value> Number of ThinLTO jobs
+# --threads              Run the linker multi-threaded
+# --undefined=<value>    Force undefined symbol during linking
+# --verbose              Verbose mode
+# --version              Display the version number and exit
+# -z <option>            Linker option extensions
+
+

```suggestion
```

This is identical to `--help` output, isn't it?

> +#COMPILE_FLAGS_mov=\
+		-Wl,--allow-undefined\
+		-Wl,--strip-debug \
+		-nostartfiles \
+		-std=c++14 \
+

```suggestion
```

> +		-emit-llvm \
+		-Os \
+		-flto \
+		-fvisibility=hidden \
+		-ffunction-sections \
+		-fdata-sections \
+
+%.show: %.wasm
+	wasm2wat $<
+
+%.wasm: %.o Makefile
+	wasm-ld-$(LLVM_VERSION) -o $@ $(LINK_FLAGS) $<
+
+
+%.o: %.cpp Makefile FORCE
+	clang++-$(LLVM_VERSION) \

```suggestion
	$(CLANGXX) \
```

> +%.show: %.wasm
+	wasm2wat $<
+
+%.wasm: %.o Makefile
+	wasm-ld-$(LLVM_VERSION) -o $@ $(LINK_FLAGS) $<
+
+
+%.o: %.cpp Makefile FORCE
+	clang++-$(LLVM_VERSION) \
+		-c \
+		$(COMPILE_FLAGS) \
+		-o $@ \
+		$<
+
+%.o: %.c Makefile FORCE
+	clang-$(LLVM_VERSION) \

```suggestion
	$(CLANG) \
```

> +		-std=c++14 \
+
+COMPILE_FLAGS = -Wall \
+		--target=wasm32-unknown-unknown-wasm \
+		-emit-llvm \
+		-Os \
+		-flto \
+		-fvisibility=hidden \
+		-ffunction-sections \
+		-fdata-sections \
+
+%.show: %.wasm
+	wasm2wat $<
+
+%.wasm: %.o Makefile
+	wasm-ld-$(LLVM_VERSION) -o $@ $(LINK_FLAGS) $<

```suggestion
	$(WASM_LD) -o $@ $(LINK_FLAGS) $<
```

> @@ -0,0 +1,101 @@
+LLVM_VERSION = 11

```suggestion
LLVM_VERSION ?=
ifeq (,$(LLVM_VERSION))
  CLANG ?= clang
  CLANGXX ?= clang++
  WASM_LD ?= wasm-ld
else
  CLANG ?= clang-$(LLVM_VERSION)
  CLANGXX ?= clang++-$(LLVM_VERSION)
  WASM_LD ?= wasm-ld-$(LLVM_VERSION)
endif
```

I'm pretty sure that most users will just have one version of LLVM/clang installed. So IMO this should just use the default `clang`, unless the user explicitly sets `LLVM_VERSION` (in which case she obviously wants to use that over all other potentially installed versions).

> +
+

```suggestion
```

> +
+    /* instantiate the module */
+    if (!(wasm_module_inst = wasm_runtime_instantiate(wasm_module, 8 * 1024,
+        8 * 1024, error_buf, sizeof(error_buf)))) {
+        puts(error_buf);
+        return -1;
+    }
+    ret = iwasm_instance_exec_main(wasm_module_inst, argc, argv);
+    /* destroy the module instance */
+    wasm_runtime_deinstantiate(wasm_module_inst);
+    return ret;
+}
+
+/* bytecode needs to be writeable*/
+int iwasm_bytecode_exec_main(void *bytecode, size_t bytecode_len, int argc, char **argv){
+//  (uint8_t *) bytecode;

```suggestion
```

> +        if (!(wasm_module = wasm_runtime_load((uint8_t * )bytecode, bytecode_len,
+                                          error_buf, sizeof(error_buf)))) {

```suggestion
        if (!(wasm_module = wasm_runtime_load(bytecode, bytecode_len,
                                              error_buf, sizeof(error_buf)))) {
```

In C `void *` can implicitly be casted to any other pointer type and this is considered as good style. (In C++ it is not possible, that's why we don't use implicit casts in header files why might end up being frowned upon by a C++ compiler.)

> +    if(!iwasm_runtime_init())
+        return (void *)-1;

```suggestion
    if (!iwasm_runtime_init()) {
        return (void *)-1;
    }
```

> +    (void) arg1; /*unused*/
+
+    if(!iwasm_runtime_init())
+        return (void *)-1;
+
+    event_queue_claim(&iwasm_q);
+    event_loop(&iwasm_q);
+
+    /*next lines should not be reached*/
+    wasm_runtime_destroy();
+    return NULL;
+}
+
+bool iwasm_start_thread(void)
+{
+    struct{

```suggestion
    struct {
```

> +typedef struct run_bytecode_event { event_t super;
+                                    void *bytecode;
+                                    size_t bytecode_len;
+                                    int argc;
+                                    char **argv;
+                                    mutex_t finish;
+} run_bytecode_event_t;

```suggestion
typedef struct run_bytecode_event {
    event_t super;
    void *bytecode;
    size_t bytecode_len;
    int argc;
    char **argv;
    mutex_t finish;
} run_bytecode_event_t;
```

> +typedef struct run_bytecode_event { event_t super;
+                                    void *bytecode;
+                                    size_t bytecode_len;
+                                    int argc;
+                                    char **argv;
+                                    mutex_t finish;
+} run_bytecode_event_t;
+
+void _wamr_run_call(event_t *event){
+    run_bytecode_event_t * e = (run_bytecode_event_t *) event;
+    e->argc = iwasm_bytecode_exec_main(e->bytecode, e->bytecode_len, e->argc, e->argv);
+    mutex_unlock(&e->finish);
+}
+
+/* this seems to be a very direct interpretation of "i like to have a wamr_run" */
+int wamr_run(void *bytecode, size_t bytecode_len, int argc, char **argv){

```suggestion
int wamr_run(void *bytecode, size_t bytecode_len, int argc, char **argv)
{
```

> +                                    void *bytecode;
+                                    size_t bytecode_len;
+                                    int argc;
+                                    char **argv;
+                                    mutex_t finish;
+} run_bytecode_event_t;
+
+void _wamr_run_call(event_t *event){
+    run_bytecode_event_t * e = (run_bytecode_event_t *) event;
+    e->argc = iwasm_bytecode_exec_main(e->bytecode, e->bytecode_len, e->argc, e->argv);
+    mutex_unlock(&e->finish);
+}
+
+/* this seems to be a very direct interpretation of "i like to have a wamr_run" */
+int wamr_run(void *bytecode, size_t bytecode_len, int argc, char **argv){
+    run_bytecode_event_t * e = malloc(sizeof(run_bytecode_event_t));

```suggestion
    run_bytecode_event_t *e = malloc(sizeof(run_bytecode_event_t));
    if (!e) {
        return -ENOMEM;
    }
```

> +
+void _wamr_run_call(event_t *event){
+    run_bytecode_event_t * e = (run_bytecode_event_t *) event;
+    e->argc = iwasm_bytecode_exec_main(e->bytecode, e->bytecode_len, e->argc, e->argv);
+    mutex_unlock(&e->finish);
+}
+
+/* this seems to be a very direct interpretation of "i like to have a wamr_run" */
+int wamr_run(void *bytecode, size_t bytecode_len, int argc, char **argv){
+    run_bytecode_event_t * e = malloc(sizeof(run_bytecode_event_t));
+    *e = (run_bytecode_event_t){ .super.handler = _wamr_run_call,
+                                    .bytecode = bytecode,
+                                    .bytecode_len = bytecode_len,
+                                    .argc = argc, .argv = argv,
+                                    .finish = MUTEX_INIT_LOCKED};
+    event_post(&iwasm_q, (event_t*) e);

```suggestion
    event_post(&iwasm_q, (event_t *)e);
```

> +
+

```suggestion

```

> +    *e = (run_bytecode_event_t){ .super.handler = _wamr_run_call,
+                                    .bytecode = bytecode,
+                                    .bytecode_len = bytecode_len,
+                                    .argc = argc, .argv = argv,
+                                    .finish = MUTEX_INIT_LOCKED};
+    event_post(&iwasm_q, (event_t*) e);
+
+    mutex_lock(&e->finish);
+
+    int ret = e->argc;
+    free(e);
+    return ret;
+}
+
+
+int wamr_run_cp(const void *bytecode, size_t bytecode_len, int argc, char **argv){

```suggestion
int wamr_run_cp(const void *bytecode, size_t bytecode_len, int argc, char **argv)
{
```

> +
+

```suggestion

```

> +
+    mutex_lock(&e->finish);
+
+    int ret = e->argc;
+    free(e);
+    return ret;
+}
+
+
+int wamr_run_cp(const void *bytecode, size_t bytecode_len, int argc, char **argv){
+    uint8_t *wasm_buf = NULL;
+    unsigned wasm_buf_size = 0;
+    /* we need the bytecode to be writable while loading
+     * loading adds size information to stack POPs */
+    //static uint8_t wasm_test_file[] = main_wasm;
+    wasm_buf = malloc(bytecode_len);

```suggestion
```

Not needed any more

> +    uint8_t *wasm_buf = NULL;
+    unsigned wasm_buf_size = 0;

```suggestion
    unsigned wasm_buf_size = 0;
    uint8_t *wasm_buf = malloc(bytecode_len);
    if (!wasm_buf) {
        return -ENOMEM;
    }
```

cpp-check doesn't like this, as `wasm_buf` is assigned a new value before the old value (`NULL`) is used. Rather than forward declare it here (uninitialzed) and later do the `malloc()`, it is IMO better to just directly initialize it with the result of `malloc()`.

> +    /* no copy need if architecture has const in writable mem */
+    /* wasm_buf = (uint8_t *) main_wasm; */
+    wasm_buf_size = bytecode_len;
+
+    int ret = wamr_run(wasm_buf, wasm_buf_size, argc, argv);
+    free(wasm_buf);
+    return ret;
+}
+
+
+#define telltruth(X) ((X) ? "true" : "false")
+#include "blob/hello.wasm.h"
+
+int main(void)
+{
+    printf("iwasm_thread_initilised: %s\n",telltruth(iwasm_start_thread()));

```suggestion
    printf("iwasm_thread_initilised: %s\n", telltruth(iwasm_start_thread()));
```

> +}
+
+
+#define telltruth(X) ((X) ? "true" : "false")
+#include "blob/hello.wasm.h"
+
+int main(void)
+{
+    printf("iwasm_thread_initilised: %s\n",telltruth(iwasm_start_thread()));
+
+    {
+        int app_argc = 1;
+        char *app_argv1 = "test";
+        char **app_argv = {&app_argv1};
+        int ret = wamr_run_cp(main_wasm, main_wasm_len, app_argc, app_argv);
+        printf("ret = %d\n",ret );

```suggestion
        printf("ret = %d\n", ret);
```

> +{
+    printf("iwasm_thread_initilised: %s\n",telltruth(iwasm_start_thread()));
+
+    {
+        int app_argc = 1;
+        char *app_argv1 = "test";
+        char **app_argv = {&app_argv1};
+        int ret = wamr_run_cp(main_wasm, main_wasm_len, app_argc, app_argv);
+        printf("ret = %d\n",ret );
+    }
+    {
+        int app_argc = 1;
+        char *app_argv1 = "test";
+        char **app_argv = {&app_argv1};
+        int ret = wamr_run_cp(hello_wasm, hello_wasm_len, app_argc, app_argv);
+        printf("ret = %d\n",ret );

```suggestion
        printf("ret = %d\n", ret);
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15329#pullrequestreview-761763013
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210923/71d35eda/attachment-0001.htm>


More information about the notifications mailing list